Skip to content

storage: Detect partition tables in unsupported places #22039

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

Merged

Conversation

mvollmer
Copy link
Member

@mvollmer mvollmer commented May 22, 2025

These likely belong to virtual machines and we shouldn't touch them.

See https://bugzilla.redhat.com/show_bug.cgi?id=2364699

Demo: https://youtu.be/o-osf8NMZYo

@mvollmer mvollmer changed the title storage: Detect partition tables in supported places storage: Detect partition tables in unsupported places May 22, 2025
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from a98db30 to 46e2396 Compare May 22, 2025 09:39
@mvollmer mvollmer requested a review from Venefilyn May 22, 2025 11:03
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from 46e2396 to e3ef9c4 Compare May 22, 2025 11:03
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we even show GPT partitions for LVM2 logical volumes page? If they aren't partitions this shouldn't be shown at all IMO and we can drop the card completely

image

As for showing vs not showing the mappers at all, it might have value to show them, at least within the logical volume itself (though AFAICT we would only ever show one mapper here right?

We still show the "LVM2 volume group" as a "logical volume" and the logical volume itself as a GPT partition in the storage table too. Going to the page itself shows it correctly, just not within the main table.

image

@mvollmer mvollmer marked this pull request as draft May 26, 2025 06:44
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch 2 times, most recently from 0a0fa60 to 45f372a Compare May 26, 2025 08:05
@mvollmer
Copy link
Member Author

Should we even show GPT partitions for LVM2 logical volumes page? If they aren't partitions this shouldn't be shown at all IMO and we can drop the card completely

Good question...

I think ideally we would explain exactly what is going on: "This logical volume is a disk in a virtual machine." This would be on the same level as "This logical volume is a XFS filesystem mounted on /blah.", I would say.

If we follow that logic, then there would be a card with title "Virtual machine disk" where otherwise there might be the "xfs filesystem" card, no?

Should we just go ahead and pretend that it is a disk of a virtual machine? The messaging would be less confusing and less vague, but it might be incorrect.

@mvollmer
Copy link
Member Author

(Really, if any block device is used as a disk in a virtual machine, there should be a link in Cockpit Storage to that machine in Cockpit Machines. But Linux doesn't give us enough information out of the box, we would have to build the plumbing for that first.)

@mvollmer
Copy link
Member Author

mvollmer commented May 26, 2025

As for showing vs not showing the mappers at all, it might have value to show them, at least within the logical volume itself (though AFAICT we would only ever show one mapper here right?

I am afraid I don't know what you mean with "mappers". The partitions?

We still show the "LVM2 volume group" as a "logical volume"

It is labeled as "LVM2 logical volumes", plural. Did you get confused there maybe? :-)

It's a questionable choice that I made back during the big Storage Redesign. In a table, a "thing" is labeled by what's on it. Take this typical boot disk:

image

The type of "vda2" is "xfs filesystem" (and location is "/boot"). The type is not "Partition", although that is the topmost card when you navigate to it:

image

Or in other words, the "Type" of a thing in a table is the title of the last card of the page for that thing.

Which means, of course that since a block device with a partition table has a page like this:

image

it will have a type of "GPT partitions" in a table. See the first screenshot, the "vda - VirtIO Disk". (But also note that for top-level things, we add more information into the "ID" column.)

This helps reading the table a bit, maybe. The "vda2" thing is not called a "Partition", but it is a child of something called "Partitions", so you could infer from that that "vda2" must be one of those partitions.

So that's very logical, in a way, but maybe not intuitive.

For LVM2 volume groups it means that their type is "LVM2 logical volumes", since that's the last card on their page. But unlike a disk (which have something else than "GPT partitions" as their type), LVM2 volume groups will always be "LVM" logical volumes" and there is no extra information here, and we might just as well use "LVM2 volume group" as their type because I guess that is what everyone expects.

and the logical volume itself as a GPT partition in the storage table too. Going to the page itself shows it correctly, just not within the main table.

Yeah, I think there is no outright bug here, it just all too logical and not intuitive enough. :-)

Should we change it? (Separate PR)

@mvollmer mvollmer marked this pull request as ready for review May 27, 2025 10:04
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from 45f372a to fbee3f9 Compare May 27, 2025 10:05
@Venefilyn
Copy link
Member

Venefilyn commented May 27, 2025

@mvollmer You'll have to forgive me if I repeat or don't make sense, groggy today but wanted to say something instead of leaving it for tomorrow.

In short for me there is only one blocker. If an LVM2 LV has GPT partitions or other fs types but we don't want to be able to modify them, show them anyway but as disabled and only within the LVM2 LV, not the main storage page.

The rest of my comments I'd say were addressed, thanks for explaining it. I'm also not fully convinced of changing to Volume Group for the labeling if their type is already a Logical Volume.

For LVM2 volume groups it means that their type is "LVM2 logical volumes", since that's the last card on their page. But unlike a disk (which have something else than "GPT partitions" as their type), LVM2 volume groups will always be "LVM" logical volumes" and there is no extra information here, and we might just as well use "LVM2 volume group" as their type because I guess that is what everyone expects.

Can a LVM2 Volume Group also contain "partitions"? If an LVM2 group can only only contain other LVM2 Logical Volumes I do think we can make that association and label it as that. But then we go into the question of should we label the fs differently for the LVM2 Logical Volumes and I see where things might get messy. Though I'm not certain of LVM2's structure, maybe we leave this for now then and improve in future PRs :D

But definitely show the mappers in the detailed view of the LVM2 LV. Not just say you can't modify the partitions and not show them.

Mappers in this case would be similar to how I have LUKS here on my personal desktop. I managed to see mappers in my VM when I just added sit to the test and modified the code to register the partitions anyway so I could see what info it would show otherwise
image

@mvollmer
Copy link
Member Author

mvollmer commented May 30, 2025

If an LVM2 LV has GPT partitions or other fs types but we don't want to be able to modify them, show them anyway but as disabled and only within the LVM2 LV, not the main storage page.

The issue here is that Cockpit doesn't know the partitions and will thus currently "lie" to the user. Cockpit shows this:

image

while parted says this:

# parted /dev/vmgroup/vol1 -s p
Model: Linux device-mapper (linear) (dm)
Disk /dev/dm-1: 1074MB
Sector size (logical/physical): 512B/512B
Partition Table: msdos
Disk Flags: 

Number  Start   End     Size   Type     File system     Flags
 1      1049kB  316MB   315MB  primary  ext4            boot
 2      316MB   508MB   192MB  primary  linux-swap(v1)  swap
 3      508MB   1074MB  566MB  primary  ext4

Cockpit let's the user create new partitions that would overwrite the existing ones, but this is caught further down the stack, fortunately:

image

This is the main issue that we need to solve: Cockpit giving misleading information in this specific but common scenario: using logical volumes as raw disks for virtual machines.

So this PR effectively disables the normal partition card by not offering any actions on it (and explaining why). It will also not list any partitions since we must assume that the OS will not tell us about them consistently.

@mvollmer
Copy link
Member Author

mvollmer commented May 30, 2025

Can a LVM2 Volume Group also contain "partitions"?

Yes and no. This is Linux, impossible is nothing. :-)

"Technically", any block device can have partitions in the sense that /usr/bin/partprobe will happily tell the kernel about them and the kernel will happily create the necessary block devices for the partitions.

But out of the box, not all block devices are probed for partitions during boot. No device mapper device is probed, as far as I know, and logical volumes are implemented with the device mapper. Device mapper devices are the /dev/dm-NUMBER ones, and are also used for LUKS and Stratis, for example.

Thus, you can put a partition table on a logical volume (or luks cleartext device), and they do work just fine, but unless you hack your boot code (udev rules, probably), they will disappear after the next boot.

And because of all this, people don't seem to want to put partitions on device mapper devices.

And consequently, Cockpit doesn't let you put partition tables on these kind of block devices either. (And is not well prepared for when they do show up, hence this PR...)

Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the explanations about it, to me the comments make sense so only have one visual suggestion.

But in short for me: keeping the partitions card in the LVM2 LV is better than not showing, indicates that there is something but we just don't support it. So current implementation is good.

Venefilyn
Venefilyn previously approved these changes May 30, 2025
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from 557d7c4 to a545278 Compare May 30, 2025 12:47
Venefilyn
Venefilyn previously approved these changes May 30, 2025
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from a545278 to 0841cef Compare May 30, 2025 15:15
@mvollmer mvollmer marked this pull request as draft May 31, 2025 07:41
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch 2 times, most recently from 5d5eb4f to ad9552b Compare June 2, 2025 08:11
@mvollmer
Copy link
Member Author

mvollmer commented Jun 2, 2025

@Venefilyn, I did in fact clean up the "available spaces" handling and added storage: Collect "available spaces" during page creation.

That removes quite a bit of old and hairy code.

@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from ad9552b to dbe892f Compare June 2, 2025 09:24
@mvollmer mvollmer marked this pull request as ready for review June 2, 2025 09:25
@mvollmer mvollmer requested a review from Venefilyn June 2, 2025 09:25
Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a sanity question. I do like this approach as it simplifies things, but I also just wanna double-check things 🙏

Comment on lines -116 to +119
} else if (client.blocks_available[content_block.path]) {
} else if (!content_block.IdUsage) {
if (!block.HintIgnore)
register_available_block_space(client, content_block);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(So this is coming in as someone who doesn't know much at all about storaged)


Is the comparison here then changing from multiple checks to just checking if IdUsage is undefined? Do we not need to care about stuff like swap, mpath member, fs label, ic container, etc.?

Also from what you said and what I see this is all going from creating the available blocks on load to page creation then right? Will this make it easier or more difficult if we decide to implement another filesystem?

I see this from storaged docs that say:

If blank, no known signature was detected. This doesn't necessarily mean the device contains no structured data; it only means that no signature known to the probing code was detected.

Applications should not rely on the value in this or the "IdType" property - instead, applications should check for whether the object in question implements interfaces such as e.g. org.freedesktop.UDisks2.Filesystem, org.freedesktop.UDisks2.Swapspace or org.freedesktop.UDisks2.Encrypted.

Wouldn't that mean that we could technically show this as an available space even if it isn't?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the comparison here then changing from multiple checks to just checking if IdUsage is undefined? Do we not need to care about stuff like swap, mpath member, fs label, ic container, etc.?

The rest of the big create_pages function cares about swap, mpath, partitions, etc. When it comes to this place in its if-else-if chain, it only needs to distinguish between "empty" and "unrecognized".

The old is_available_block function would have to recreate the same logic as create_pages and arrive at the same conclusions. This was tedious to keep synchronized and the improvement here is that now there is only one place with the complex logic.

Also from what you said and what I see this is all going from creating the available blocks on load to page creation then right? Will this make it easier or more difficult if we decide to implement another filesystem?

It should make it easier.

I see this from storaged docs that say:

If blank, no known signature was detected. This doesn't necessarily mean the device contains no structured data; it only means that no signature known to the probing code was detected.

Applications should not rely on the value in this or the "IdType" property - instead, applications should check for whether the object in question implements interfaces such as e.g. org.freedesktop.UDisks2.Filesystem, org.freedesktop.UDisks2.Swapspace or org.freedesktop.UDisks2.Encrypted.

Oh, interesting! We do in fact check IdUsage == "filesystem" instead of checking whether there is a Filesystem interface... We could change that, and now it's much easier since there is only one place. :-)

Wouldn't that mean that we could technically show this as an available space even if it isn't?

Yes, true, if UDisks2 somehow has a better idea of what's on a block device than udev. That would be an unwelcome inconsistency, I'd say, but as you found out, it's clearly documented.

I'll go over the code and try to improve it.

(We are really digging into the oldest pieces of the Cockpit Storage code here... :-)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll go over the code and try to improve it.

In a separate PR. We would in any case still check IdUsage to distinguish between "unrecognized" and "empty". The improvements would come from the rest of the code recognizing more things so that we don't reach this point in the code as often.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

Venefilyn
Venefilyn previously approved these changes Jun 3, 2025
mvollmer added 2 commits June 3, 2025 11:31
Currently, get_available_spaces implements its own logic for finding
empty block devices and unused space in partition tables. This should
match the logic used during creation of the page tree, and currently
needs to be done with careful coding.

Instead, let's collect the available spaces as a side effect of
creating the page tree.

This has made two edge more consistent with the rule that only empty
block devices (aka "Unformatted data") should be offered as available:

 - LVM2 physical volumes that are not associated with any known volume
   group are no longer considered "available" since they are not
   unformatted.

 - Devices that can be accessed via multiple pathes but that don't
   have a "mpath" device to manage them, might now be offered as
   available via one path.
These likely belong to virtual machines and we shouldn't touch them.

See https://bugzilla.redhat.com/show_bug.cgi?id=2364699
@mvollmer mvollmer force-pushed the storage-unexpected-partitions branch from dbe892f to 6e10240 Compare June 3, 2025 08:32
@@ -188,7 +188,7 @@ function lvol_or_part_and_fsys_resize(client, lvol_or_part, size, offline, passp
return Promise.reject(_("Stratis blockdevs can not be made smaller")); // not-covered: safety check
else
return Promise.resolve();
} else if (client.blocks_available[block.path]) {
} else if (!block.IdUsage) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This added line is not executed by any test.

@Venefilyn
Copy link
Member

/packit retest-failed

Copy link
Member

@Venefilyn Venefilyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Failing test unrelated

@Venefilyn Venefilyn merged commit a757f5c into cockpit-project:main Jun 3, 2025
89 of 90 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants