-
Notifications
You must be signed in to change notification settings - Fork 113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(core, uuid): add dm support #495
Conversation
288b5ee
to
4b8469b
Compare
cmd/ndm_daemonset/probe/uuid.go
Outdated
@@ -53,6 +54,14 @@ func generateUUID(bd blockdevice.BlockDevice) (string, bool) { | |||
// the partition and the unique data will be stored in sectors where consumers do not have access. | |||
|
|||
switch { | |||
/*case bd.DeviceAttributes.DeviceType == blockdevice.BlockDeviceTypeLoop: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this because we haven't added the ability to mark a block device as loop device in udev package yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its because we havent decided how we should generate UUID for loop devices. Since loop devices are only present virtually on the node, we may use the devicename + hostname
method here.
cc: @kmova need your thoughts on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the loop names change on reboots @akhilerm.
Can we read a few chars from the loop device and then use them to generate an ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more questions in general :
|
Yes, there is. But how we implement that and will it be a separate component in NDM have to be thought through
Not as a single blockdevice. All devices, which does not have holders or partitions will be a blockdevice resource.
The devices |
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
- add dm support to os disk filter Signed-off-by: Akhil Mohan <[email protected]>
4b8469b
to
df69708
Compare
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
3f49d21
to
d9b55bd
Compare
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
Signed-off-by: Akhil Mohan <[email protected]>
The approach LGTM. Can be merged after @pawanpraka1 review. |
LGTM! On a side note: Support for loop devices will be very important because we can use loop devices in integration tests and test for detection of dm and md(in future). |
Signed-off-by: Akhil Mohan <[email protected]>
@harshthakur9030 , I have added support for loop devices also into the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good.
Why is this PR required? What issue does it fix?:
Adds DM support to NDM. With the changes in this PR: lvm, crypt, loop, dm devices will be natively supported in NDM.
What this PR does?:
Does this PR require any upgrade changes?:
No
If the changes in this PR are manually verified, list down the scenarios covered::
Any additional information for your reviewer? :
udevadm info
for dm devices (lvm and crypt)Mention if this PR is part of any design or a continuation of previous PRs
Checklist:
<type>(<scope>): <subject>