lvm: Enhancements for LVM2 RAID support

[mvollmer]

This exposes the health status etc and allows for repairs.

This is part of supporting LVM2 RAID in Cockpit:

storaged-project/udisks#969
cockpit-project/cockpit#17226

There is a first demo already: https://www.youtube.com/watch?v=86veT8waLZE
Please check the cockpit PR for updates.

[blivetghoul]

Can one of the admins verify this patch?

[mvollmer]

I guess I need to write some tests... :-)

[vojtechtrefny]

Jenkins, ok to test.

[vojtechtrefny]

The biggest issue I have with this is the DBus plugin -- we want to have the same functionality in both (even though UDisks doesn't use the DBus plugin). Good news is that it looks like it has all the properties we need (SyncPercent and Health and Devices for LVs and Hidden for PVs). Bad news is that it doesn't provide a function for Repair, but I can live with that (but we still need an "empty" function to return the "not implemented" error in the plugin, it wouldn't be the first and we can add it to lvmdbusd later).

[vojtechtrefny]

I guess I need to write some tests... :-)

Yes please :-)

[mvollmer]

The biggest issue I have with this is the DBus plugin

Ah, I have ignored it completely. Should we agree on how we want to extend the "lvm" plugin exactly, and then I port that over to the "lvm-dbus" plugin?

[mvollmer]

While writing the test, I noticed wait_for_sync, which waits for copy_percent to reach 100... What is the relation between copy_percent and sync_percent? Do we maybe only need one?

[vojtechtrefny]

While writing the test, I noticed wait_for_sync, which waits for copy_percent to reach 100... What is the relation between copy_percent and sync_percent? Do we maybe only need one?

Good question, we probably need to ask someone from the LVM team. The description for copy_percent and sync_percent says exactly the same:

copy_percent                - For Cache, RAID, mirrors and pvmove, current percentage in-sync.
sync_percent                - For Cache, RAID, mirrors and pvmove, current percentage in-sync.

[vojtechtrefny]

The biggest issue I have with this is the DBus plugin

Ah, I have ignored it completely. Should we agree on how we want to extend the "lvm" plugin exactly, and then I port that over to the "lvm-dbus" plugin?

Ok, good point. Let's deal with the DBus plugin later.

[mvollmer]

Good question, we probably need to ask someone from the LVM team.

I am collecting questions here: cockpit-project/cockpit#17226 (comment), I'll add this to the list.

For now, let's assume they are the same (as far as our use case is concerned).

[mvollmer]

I now see that lv_health_status is already encoded in lv_attr[8], so we don't need it either...

[mvollmer]

I think the test is failing because /tmp runs out of memory while the sparse files are being written to. I guess the new test is different in that the files are actually written fully and become unsparse when the mirror halves are synced up...

[mvollmer]

Bad news is that [the lvm2 d-bus api] doesn't provide a function for Repair, but I can live with that (but we still need an "empty" function to return the "not implemented" error in the plugin, it wouldn't be the first

@vojtechtrefny, can you point to an example of such a empty function in lvm-dbus.c ? I can't find any... However, bd_lvm_devices_add in lvm-dbus.c spawns "lvmdevices". Should we maybe just spawn "lvconvert --repair" in lvm-dbus.c as well?

and we can add it to lvmdbusd later).

Maybe lvmdbusd can just call libblockdev for that. :-)

[mvollmer]

lvm_dbus_tests.py doesn't seem to run during CI, right? (Because it doesn't match "*_test.py"? :-)

[mvollmer]

I think I have reached a stable point here. I have squashed the FIXUPs, please have another look!

[vojtechtrefny]

I think the test is failing because /tmp runs out of memory while the sparse files are being written to. I guess the new test is different in that the files are actually written fully and become unsparse when the mirror halves are synced up...

Yes, I had the same issue few months ago 4562941

can you point to an example of such a empty function in lvm-dbus.c

I actually fixed the missing functionality in lvm-dbus few months ago. Example from the older code: https://github.com/storaged-project/libblockdev/blob/2.26-1/src/plugins/lvm-dbus.c#L3529 it simply calls bd_lvm_is_tech_avail to return error for unsupported technologies.

However, bd_lvm_devices_add in lvm-dbus.c spawns "lvmdevices". Should we maybe just spawn "lvconvert --repair" in lvm-dbus.c as well?

I don't think it's necessary. UDisks forces the CLI plugin so it will never use the DBus one, we can deal with this in the future.

Maybe lvmdbusd can just call libblockdev for that. :-)

+1 Everyone loves circular dependencies :-)

lvm_dbus_tests.py doesn't seem to run during CI, right? (Because it doesn't match "*_test.py"? :-)

We use pattern='*_test*.py'. There was some reason for naming the LVM DBus test file differently, I don't remember why we did that (maybe to be able to run all tests except these in some cases?), we should probably rename it to make it less confusing.

I think I have reached a stable point here. I have squashed the FIXUPs, please have another look!

Great, I'll try to do the review as soon as possible.

[mvollmer]

However, bd_lvm_devices_add in lvm-dbus.c spawns "lvmdevices". Should we maybe just spawn "lvconvert --repair" in lvm-dbus.c as well?

I don't think it's necessary. UDisks forces the CLI plugin so it will never use the DBus one, we can deal with this in the future.

I copy/pasted the code from lvm.c and it seems to work. shrug

[vojtechtrefny]

Looks good to me in general. Let's wait for other opinions about the LVM DBus plugin repair support.

[vpodzime]

Thanks for the bold move!

[vojtechtrefny]

Looks good to me, thank you. If this is the final version, I think we can merge it now.