feat: add LVM status controller#13295
Conversation
b286266 to
8e59fa6
Compare
3a6324e to
e8b8c7a
Compare
There was a problem hiding this comment.
Pull request overview
Adds three new read-only resources (LVMPhysicalVolumeStatus, LVMVolumeGroupStatus, LVMLogicalVolumeStatus) and corresponding controllers that periodically scan host LVM state via lvm pvs/vgs/lvs --reportformat json. Refactors the existing block.LVMActivationController into a new storage package backed by a shared internal/pkg/lvm helper, moves the TestLVMActivation integration test into a new StorageSuite, and adds a TestLVMStatus integration test. Includes generated proto/deep-copy/docs updates.
Changes:
- New
internal/pkg/lvmpackage wrappinglvmCLI (PV/VG/LV listing,vgchange,pvscan). - New
storageresource types (machinery + proto + docs) and three polling controllers (30s interval). - Moves
LVMActivationControllerfromblocktostorage, threading the shared*lvm.LVMhelper.
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/pkg/lvm/{lvm,pv,vg,lv,activate}.go | New helper wrapping lvm binary, JSON report decoding, udev parsing. |
| internal/pkg/lvm/lvm_test.go + testdata/* | Unit tests covering pvs/vgs/lvs JSON parsing and Tags unmarshalling. |
| pkg/machinery/resources/storage/*.go | New LVMPhysicalVolumeStatus / LVMVolumeGroupStatus / LVMLogicalVolumeStatus resources, deep-copy, registration test. |
| pkg/machinery/api/resource/definitions/storage/storage.pb.go + api/.../storage.proto | Generated protobuf bindings for the new specs. |
| internal/app/machined/pkg/controllers/storage/*.go | New status controllers + relocated LVMActivationController. |
| internal/app/machined/pkg/runtime/v1alpha2/v1alpha2_{controller,state}.go | Wires new controllers and registers new resource types; constructs shared lvm.New(). |
| internal/integration/api/{storage,volumes,selinux}.go | Adds StorageSuite with TestLVMStatus and relocated TestLVMActivation; expects /run/lock/lvm SELinux label. |
| website/content/v1.14/reference/api.md | Auto-generated API documentation for new specs. |
| hack/release.toml | Release note describing new LVM status resources. |
Files not reviewed (2)
- pkg/machinery/api/resource/definitions/storage/storage.pb.go: Language not supported
- pkg/machinery/resources/storage/deep_copy.generated.go: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //go:generate go tool github.com/siderolabs/deep-copy -type LVMLogicalVolumeStatusSpec -type LVMPhysicalVolumeStatusSpec -type LVMVolumeGroupStatusSpec -header-file ../../../../hack/boilerplate.txt -o deep_copy.generated.go . | ||
|
|
||
| // NamespaceName contains storage resources. | ||
| const NamespaceName resource.Namespace = v1alpha1.NamespaceName |
There was a problem hiding this comment.
we could probably actually use proper namespace name, it was a bug when I originally started block that I put it to the wrong namespace, but it was too late when I discovered it
| // pollInterval is how often the LVM status controllers re-scan the system in | ||
| // the absence of input events. LVM has no inotify-style change feed, so a | ||
| // poll is the only way to catch attribute changes (e.g. grow operations). | ||
| const pollInterval = 30 * time.Second |
There was a problem hiding this comment.
how bad it is to poll on interval? would it e.g. wake up a sleeping disk just to check LVM metadata?
There was a problem hiding this comment.
pvs -a enumerates every block device so it will open each and read sector 0 / PV label region to check for LVM signature. On spun-down HDDs that wakes the disk every poll - so could probably significantly increase the wear of the device.
| case <-ctx.Done(): | ||
| return nil | ||
| case <-r.EventCh(): | ||
| case <-ticker.C: |
There was a problem hiding this comment.
can a PV change without ever writing to blockdevice? we have a flow which goes into DiscoverdVolumes and it notifies on any process opening a blockdevice for write, we could feed it here?
(happy to discuss more)
There was a problem hiding this comment.
PV not, but some kernel DM states can - e.g. active/inactive, suspended, open-count, kernel major/minor - lives in device-mapper, not on disk. So thing slike lvchange -ay/-an, dmsetup suspend, mount/umount can flip these w/o touching PV metadata. Reflected in lv_active/lv_suspended/lv_device_open columns.
There was a problem hiding this comment.
but I guess that would trigger DM blockdevice changes? We also listen for these (we listen for all blockdevice events).
ae26831 to
95df045
Compare
| case <-r.EventCh(): | ||
| } | ||
|
|
||
| if err := safe.WriterModify( |
There was a problem hiding this comment.
I wonder if we should throttle this (the easiest way possible is to add something like this after this line):
This way we would coalesce multiple updates into a single event, as e.g. BlockDevice -> Disk -> DV will always propagate in a sequence
select {
case <-time.After(time.Second): // or something bigger
case <-ctx.Done():
return nil
}88f7c92 to
a751d2e
Compare
Add resource types for physical volumes, volume groups, and logical volumes to track LVM inventory and state. Provides read-only discovery through the resource API for visibility into existing LVM setup. Signed-off-by: Mateusz Urbanek <mateusz.urbanek@siderolabs.com>
|
/m |
Add resource types for physical volumes, volume groups, and logical
volumes to track LVM inventory and state. Provides read-only discovery
through the resource API for visibility into existing LVM setup.
Manual Testing
Creating LVM objects:
Removing LVM objects:
Signed-off-by: Mateusz Urbanek mateusz.urbanek@siderolabs.com