#2128 - Fix device mapper partitioning #2129
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with device-mapper partitioning by changing how Ignition determines if a block device is in use. The change removes the check for device 'holders' (blockDevHeld) and now relies solely on checking if a device is mounted. This is a sensible fix, as the presence of holders doesn't necessarily mean a device is actively in use, which was causing problems for multipath devices. The implementation is straightforward. I've added one comment suggesting a minor code cleanup to remove a redundant variable initialization.
tlbueno
force-pushed
the
tb_fix_mpath
branch
3 times, most recently
from
aec2be0 to
427ea10
Compare
|
|
||
| holdersDir := fmt.Sprintf("/sys/class/block/%s/holders/", blockDevNode) | ||
| entries, err := os.ReadDir(holdersDir) | ||
| if err != nil { |
There was a problem hiding this comment.
Hmm, I am a little worried about removing this check on holder's I have to imagine there are edge cases that would need us to check. Lets flip this change so we make a more direct alteration when doing multi path explicitly.
There was a problem hiding this comment.
Thanks for the suggestion. Let me think and try to implement some checking for that.
There was a problem hiding this comment.
Made a change to check if the block device is a device mapper, if so, it will skip the blockDevHeld.
When run ignition on a device mapper, ie, multipath, it fails because the function blockDevHeld returns true as the block device contains holders. A block device with holders do not necessary means the block device is in use (like mounted). The function blockDevInUse will not check if it is a device mapper and if so, do not check for blockDevHeld. Signed-off-by: Tiago Bueno <[email protected]>
tlbueno
force-pushed
the
tb_fix_mpath
branch
from
427ea10 to
e3d639d
Compare
| ### Bug fixes | ||
|
|
||
| - Fix fetch-offline for Oracle Cloud Infrastructure | ||
| - Fix device mapper partitioning ([#2128](https://github.com/coreos/ignition/issues/2128)) |
There was a problem hiding this comment.
Hmm, we can make this a little better
wdyt
- Fix multipath partitioning: ignore DM holders when no partitions are mounted;continue to refuse if the disk or any partition is active. (#2128)
There was a problem hiding this comment.
Overall lgtm, have you had a change to verify that this fixes the issue?
I am wary of updating the partitioning logic checks simply because these changes will effect all specs as well, but there are a lot of partitioning tests so that makes me feel slightly better.
When run ignition on a device mapper, ie, multipath, it fails because the function blockDevHeld returns true as the block device contains holders. A block device with holders do not necessary means the block device is in use (like mounted).
The function blockDevInUse should look for mounted devices only.
fixes #2128