[controller] Fix LVMVolumeGroup spec.blockDeviceSelector update Issue#130
Draft
AleksZimin wants to merge 2 commits into
Draft
[controller] Fix LVMVolumeGroup spec.blockDeviceSelector update Issue#130AleksZimin wants to merge 2 commits into
AleksZimin wants to merge 2 commits into
Conversation
AleksZimin
changed the title
astef
requested changes
Mar 3, 2025
astef
reviewed
Apr 9, 2025
Contributor
|
This is a quick fix. We are going to implement proper one. |
Update spec.blockDeviceSelector during reconciliation when new BlockDevices are added to the LVM Volume Group on the node, so the field reflects the actual set of devices in the VG. * Filter BlockDevices by node before computing candidates. * Add notMatchedBlockDeviceNames helper that uses k8s.io/apimachinery labels selector to determine which device names are not matched. * Add appendDeviceNamesToLabelSelector and updateBlockDeviceSelectorIfNeeded to extend an existing matchExpressions/matchLabels selector or create one from scratch when nil. * Include selector mismatch in hasLVMVolumeGroupDiff so the discoverer triggers an update when the selector falls behind the actual device set. * Add unit tests covering: no-op when up to date, creation from nil, appending to matchExpressions, migrating from matchLabels, and mixed selectors with unrelated keys. Co-authored-by: Anton Sergunov <anton.sergunov@flant.com> Co-authored-by: Aleksandr Stefurishin <aleksandr.stefurishin@flant.com> Signed-off-by: Aleksandr Zimin <alexandr.zimin@flant.com>
duckhawk
force-pushed
the
fix-lvg-discover
branch
from
ffe2f5d to
f7fb6fe
Compare
The previous attempt called updateBlockDeviceSelectorIfNeeded(...) and discarded the returned selector, then called only Status().Update(), so the change to Spec.BlockDeviceSelector was never sent to the API server: * When the existing selector was nil, the freshly-built selector was dropped on the floor (function return value assigned to "_"). * When the existing selector was non-nil, the in-memory mutation was performed but Status().Update() does not write the spec subresource, so the API state was never updated. As a side-effect, hasLVMVolumeGroupDiff kept returning true on every discoverer cycle (the selector mismatch never resolved), causing UpdateLVMVolumeGroupByCandidate to run forever. Fix: * Capture the returned selector and assign it to lvg.Spec.BlockDeviceSelector when non-nil. * Call cl.Update(ctx, lvg) to persist the spec change. controller-runtime refreshes lvg.ResourceVersion in place, so the subsequent Status().Update() picks up the new revision without a refetch. * Move the spec Update before populating the status fields. The API server (and the fake client used in tests) preserves status on Update(), so doing the spec write after we'd populated lvg.Status would wipe those fields back to whatever the server had stored. Add two regression tests: * UpdateLVMVolumeGroup_persists_spec_blockDeviceSelector covers the "selector existed but is missing new devices" path: candidate brings three devices; we read the LVG back from the fake client and assert notMatchedBlockDeviceNames(stored.Spec.BlockDeviceSelector, ...) is empty, plus that status was also persisted. * UpdateLVMVolumeGroup_creates_spec_blockDeviceSelector_when_nil covers the legacy LVG path: an LVG with nil selector gets a new one built from candidate.BlockDevicesNames. Both regressions fail without the fix above. Signed-off-by: Aleksandr Zimin <alexandr.zimin@flant.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses an issue where the
spec.blockDeviceSelectorfield of an existingLVMVolumeGroupresource was not updating during reconciliation, even when new physical volumes (PVs) were manually added on the node to the corresponding LVM Volume Group. With this fix, thespec.blockDeviceSelectornow correctly reflects all associatedBlockDeviceresources, ensuring accurate representation of the volume group's state.Why do we need it, and what problem does it solve?
Previously, when new
BlockDeviceresources were manually introduced to an existing LVM Volume Group on a node, thespec.blockDeviceSelectorfield in the correspondingLVMVolumeGroupresource remained unchanged. This discrepancy could lead to mismanagement of storage resources and potential inconsistencies in volume group configurations. By ensuring thatspec.blockDeviceSelectoris updated appropriately, we maintain consistency between the actual state of the LVM Volume Group and its representation within Kubernetes.Checklist