From eedbdfc8eeba864076e50f4d0539de3dce2e9857 Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Thu, 25 Jul 2024 00:20:25 +1000 Subject: [PATCH 01/15] test Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- .../agent/src/pkg/controller/block_device.go | 96 ++++++++++++++----- 1 file changed, 72 insertions(+), 24 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 022ead02..cbef0be2 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -28,7 +28,9 @@ import ( "fmt" "github.com/deckhouse/sds-node-configurator/api/v1alpha1" "os" + "reflect" "regexp" + "strconv" "strings" "time" @@ -41,7 +43,8 @@ import ( ) const ( - BlockDeviceCtrlName = "block-device-controller" + BlockDeviceCtrlName = "block-device-controller" + BlockDeviceLabelPrefix = "status.blockdevice.storage.deckhouse.io" ) func RunBlockDeviceController( @@ -100,8 +103,9 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log // create new API devices for _, candidate := range candidates { - if blockDevice, exist := apiBlockDevices[candidate.Name]; exist { - if !hasBlockDeviceDiff(blockDevice.Status, candidate) { + blockDevice, exist := apiBlockDevices[candidate.Name] + if exist { + if !hasBlockDeviceDiff(blockDevice, candidate) { log.Debug(fmt.Sprintf(`[RunBlockDeviceController] no data to update for block device, name: "%s"`, candidate.Name)) continue } @@ -122,6 +126,7 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log // add new api device to the map, so it won't be deleted as fantom apiBlockDevices[candidate.Name] = *device + } } @@ -135,24 +140,36 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log return false } -func hasBlockDeviceDiff(res v1alpha1.BlockDeviceStatus, candidate internal.BlockDeviceCandidate) bool { - return candidate.NodeName != res.NodeName || - candidate.Consumable != res.Consumable || - candidate.PVUuid != res.PVUuid || - candidate.VGUuid != res.VGUuid || - candidate.PartUUID != res.PartUUID || - candidate.LvmVolumeGroupName != res.LvmVolumeGroupName || - candidate.ActualVGNameOnTheNode != res.ActualVGNameOnTheNode || - candidate.Wwn != res.Wwn || - candidate.Serial != res.Serial || - candidate.Path != res.Path || - candidate.Size.Value() != res.Size.Value() || - candidate.Rota != res.Rota || - candidate.Model != res.Model || - candidate.HotPlug != res.HotPlug || - candidate.Type != res.Type || - candidate.FSType != res.FsType || - candidate.MachineId != res.MachineID +func hasBlockDeviceDiff(blockDevice v1alpha1.BlockDevice, candidate internal.BlockDeviceCandidate) bool { + hasBlockDeviceDiff := candidate.NodeName != blockDevice.Status.NodeName || + candidate.Consumable != blockDevice.Status.Consumable || + candidate.PVUuid != blockDevice.Status.PVUuid || + candidate.VGUuid != blockDevice.Status.VGUuid || + candidate.PartUUID != blockDevice.Status.PartUUID || + candidate.LvmVolumeGroupName != blockDevice.Status.LvmVolumeGroupName || + candidate.ActualVGNameOnTheNode != blockDevice.Status.ActualVGNameOnTheNode || + candidate.Wwn != blockDevice.Status.Wwn || + candidate.Serial != blockDevice.Status.Serial || + candidate.Path != blockDevice.Status.Path || + candidate.Size.Value() != blockDevice.Status.Size.Value() || + candidate.Rota != blockDevice.Status.Rota || + candidate.Model != blockDevice.Status.Model || + candidate.HotPlug != blockDevice.Status.HotPlug || + candidate.Type != blockDevice.Status.Type || + candidate.FSType != blockDevice.Status.FsType || + candidate.MachineId != blockDevice.Status.MachineID + if hasBlockDeviceDiff { + return hasBlockDeviceDiff + } + + newLabels := GetBlockDeviceLabels(blockDevice) + equal := reflect.DeepEqual(newLabels, blockDevice.Labels) + + if equal { + return false + } + + return true } func GetAPIBlockDevices(ctx context.Context, kc kclient.Client, metrics monitoring.Metrics) (map[string]v1alpha1.BlockDevice, error) { @@ -516,6 +533,8 @@ func UpdateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito MachineID: candidate.MachineId, } + blockDevice.Labels = GetBlockDeviceLabels(blockDevice) + start := time.Now() err := kc.Update(ctx, &blockDevice) metrics.ApiMethodsDuration(BlockDeviceCtrlName, "update").Observe(metrics.GetEstimatedTimeInSeconds(start)) @@ -528,8 +547,36 @@ func UpdateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito return nil } +func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { + + if blockDevice.Labels == nil { + blockDevice.Labels = make(map[string]string) + } + + blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.Name + blockDevice.Labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName + + blockDevice.Labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type + blockDevice.Labels[BlockDeviceLabelPrefix+"/fstype"] = blockDevice.Status.FsType + blockDevice.Labels[BlockDeviceLabelPrefix+"/pvuuid"] = blockDevice.Status.PVUuid + blockDevice.Labels[BlockDeviceLabelPrefix+"/vguuid"] = blockDevice.Status.VGUuid + blockDevice.Labels[BlockDeviceLabelPrefix+"/partuuid"] = blockDevice.Status.PartUUID + blockDevice.Labels[BlockDeviceLabelPrefix+"/lvmvolumegroupname"] = blockDevice.Status.LvmVolumeGroupName + blockDevice.Labels[BlockDeviceLabelPrefix+"/actualvgnameonthenode"] = blockDevice.Status.ActualVGNameOnTheNode + blockDevice.Labels[BlockDeviceLabelPrefix+"/wwn"] = blockDevice.Status.Wwn + blockDevice.Labels[BlockDeviceLabelPrefix+"/serial"] = blockDevice.Status.Serial + blockDevice.Labels[BlockDeviceLabelPrefix+"/path"] = blockDevice.Status.Path + blockDevice.Labels[BlockDeviceLabelPrefix+"/size"] = blockDevice.Status.Size.String() + blockDevice.Labels[BlockDeviceLabelPrefix+"/model"] = blockDevice.Status.Model + blockDevice.Labels[BlockDeviceLabelPrefix+"/rota"] = strconv.FormatBool(blockDevice.Status.Rota) + blockDevice.Labels[BlockDeviceLabelPrefix+"/hotplug"] = strconv.FormatBool(blockDevice.Status.HotPlug) + blockDevice.Labels[BlockDeviceLabelPrefix+"/machineid"] = blockDevice.Status.MachineID + + return blockDevice.Labels +} + func CreateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monitoring.Metrics, candidate internal.BlockDeviceCandidate) (*v1alpha1.BlockDevice, error) { - device := &v1alpha1.BlockDevice{ + blockDevice := &v1alpha1.BlockDevice{ ObjectMeta: metav1.ObjectMeta{ Name: candidate.Name, }, @@ -553,15 +600,16 @@ func CreateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito }, } + blockDevice.Labels = GetBlockDeviceLabels(*blockDevice) start := time.Now() - err := kc.Create(ctx, device) + err := kc.Create(ctx, blockDevice) metrics.ApiMethodsDuration(BlockDeviceCtrlName, "create").Observe(metrics.GetEstimatedTimeInSeconds(start)) metrics.ApiMethodsExecutionCount(BlockDeviceCtrlName, "create").Inc() if err != nil { metrics.ApiMethodsErrors(BlockDeviceCtrlName, "create").Inc() return nil, err } - return device, nil + return blockDevice, nil } func DeleteAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monitoring.Metrics, device *v1alpha1.BlockDevice) error { From a92638866e84b76c5a435ce6301f5415058ceba2 Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Thu, 25 Jul 2024 17:15:33 +1000 Subject: [PATCH 02/15] tryout naming fix Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index cbef0be2..4c32da9b 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -553,7 +553,7 @@ func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { blockDevice.Labels = make(map[string]string) } - blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.Name + blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.GenerateName blockDevice.Labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName blockDevice.Labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type From abd502e0f470ed19e25b0517a01242fbb4d2f9d2 Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Thu, 25 Jul 2024 17:49:36 +1000 Subject: [PATCH 03/15] fix Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 4c32da9b..899b8d98 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -553,9 +553,8 @@ func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { blockDevice.Labels = make(map[string]string) } - blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.GenerateName + blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name blockDevice.Labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName - blockDevice.Labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type blockDevice.Labels[BlockDeviceLabelPrefix+"/fstype"] = blockDevice.Status.FsType blockDevice.Labels[BlockDeviceLabelPrefix+"/pvuuid"] = blockDevice.Status.PVUuid From 6ef02973b67c8b3392a7a06f804c5b3788c42308 Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Thu, 25 Jul 2024 18:37:13 +1000 Subject: [PATCH 04/15] test Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 899b8d98..dd8b0e37 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -553,7 +553,7 @@ func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { blockDevice.Labels = make(map[string]string) } - blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name + // blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name blockDevice.Labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName blockDevice.Labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type blockDevice.Labels[BlockDeviceLabelPrefix+"/fstype"] = blockDevice.Status.FsType From 5d1896785a7bb3fe70b0cf435dd21256c747c29e Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Thu, 25 Jul 2024 18:54:30 +1000 Subject: [PATCH 05/15] tryout Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index dd8b0e37..edc40661 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -553,7 +553,7 @@ func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { blockDevice.Labels = make(map[string]string) } - // blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name + blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name blockDevice.Labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName blockDevice.Labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type blockDevice.Labels[BlockDeviceLabelPrefix+"/fstype"] = blockDevice.Status.FsType @@ -564,7 +564,7 @@ func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { blockDevice.Labels[BlockDeviceLabelPrefix+"/actualvgnameonthenode"] = blockDevice.Status.ActualVGNameOnTheNode blockDevice.Labels[BlockDeviceLabelPrefix+"/wwn"] = blockDevice.Status.Wwn blockDevice.Labels[BlockDeviceLabelPrefix+"/serial"] = blockDevice.Status.Serial - blockDevice.Labels[BlockDeviceLabelPrefix+"/path"] = blockDevice.Status.Path + //blockDevice.Labels[BlockDeviceLabelPrefix+"/path"] = blockDevice.Status.Path blockDevice.Labels[BlockDeviceLabelPrefix+"/size"] = blockDevice.Status.Size.String() blockDevice.Labels[BlockDeviceLabelPrefix+"/model"] = blockDevice.Status.Model blockDevice.Labels[BlockDeviceLabelPrefix+"/rota"] = strconv.FormatBool(blockDevice.Status.Rota) From 0d22c54e0ee8765debafc02e6a784ed7d966764b Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Thu, 25 Jul 2024 19:26:30 +1000 Subject: [PATCH 06/15] fix Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 1 - 1 file changed, 1 deletion(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index edc40661..a26311bd 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -564,7 +564,6 @@ func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { blockDevice.Labels[BlockDeviceLabelPrefix+"/actualvgnameonthenode"] = blockDevice.Status.ActualVGNameOnTheNode blockDevice.Labels[BlockDeviceLabelPrefix+"/wwn"] = blockDevice.Status.Wwn blockDevice.Labels[BlockDeviceLabelPrefix+"/serial"] = blockDevice.Status.Serial - //blockDevice.Labels[BlockDeviceLabelPrefix+"/path"] = blockDevice.Status.Path blockDevice.Labels[BlockDeviceLabelPrefix+"/size"] = blockDevice.Status.Size.String() blockDevice.Labels[BlockDeviceLabelPrefix+"/model"] = blockDevice.Status.Model blockDevice.Labels[BlockDeviceLabelPrefix+"/rota"] = strconv.FormatBool(blockDevice.Status.Rota) From 71eaf34f012d5c78c9f1491d970f698fdafe890e Mon Sep 17 00:00:00 2001 From: Nikolay Demchuk Date: Mon, 29 Jul 2024 21:23:41 +1000 Subject: [PATCH 07/15] labels Signed-off-by: Nikolay Demchuk Signed-off-by: Aleksandr Zimin --- .../controller/lvm_volume_group_watcher.go | 1 + .../lvm_volume_group_watcher_func.go | 27 +++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher.go index e80ea32b..2b09ffbd 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher.go @@ -43,6 +43,7 @@ import ( const ( LVMVolumeGroupWatcherCtrlName = "lvm-volume-group-watcher-controller" + LVGHostnameLabelKey = "kubernetes.io/metadata.name" ) func RunLVMVolumeGroupWatcherController( diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go index a87e907c..92ec4c40 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go @@ -73,12 +73,39 @@ func checkIfVGExist(vgName string, vgs []internal.VGData) bool { return false } +func checkLabels(log logger.Logger, newLVG *v1alpha1.LvmVolumeGroup) bool { + + if newLVG.Labels == nil { + log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has no labels", newLVG.Name)) + return false + } + + labelValue, exist := newLVG.Labels[LVGHostnameLabelKey] + + if !exist { + log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has no label %s", newLVG.Name, LVGHostnameLabelKey)) + return false + } + + if labelValue != newLVG.Name { + log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has label %s but the value is incorrect - %q, but should be %q", newLVG.Name, LVGHostnameLabelKey, labelValue, newLVG.Name)) + return false + } + + return true +} + func shouldLVGWatcherReconcileUpdateEvent(log logger.Logger, oldLVG, newLVG *v1alpha1.LvmVolumeGroup) bool { if newLVG.DeletionTimestamp != nil { log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has deletionTimestamp", newLVG.Name)) return true } + if !checkLabels(log, newLVG) { + log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup's %s labels have been changed", newLVG.Name)) + return true + } + if !reflect.DeepEqual(oldLVG.Spec, newLVG.Spec) { log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s configuration has been changed", newLVG.Name)) return true From 74564d60de6469b8808de41f525f8ac4ce79edfc Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Tue, 6 Aug 2024 00:22:03 +0300 Subject: [PATCH 08/15] add label with metadata.name to LVG Signed-off-by: Aleksandr Zimin --- .../controller/lvm_volume_group_watcher.go | 16 ++++++- .../lvm_volume_group_watcher_func.go | 43 +++++++++++++------ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher.go index 2b09ffbd..8b30cc78 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher.go @@ -26,6 +26,7 @@ import ( "context" "errors" "fmt" + "github.com/deckhouse/sds-node-configurator/api/v1alpha1" errors2 "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -43,7 +44,7 @@ import ( const ( LVMVolumeGroupWatcherCtrlName = "lvm-volume-group-watcher-controller" - LVGHostnameLabelKey = "kubernetes.io/metadata.name" + LVGMetadateNameLabelKey = "kubernetes.io/metadata.name" ) func RunLVMVolumeGroupWatcherController( @@ -124,6 +125,19 @@ func RunLVMVolumeGroupWatcherController( } log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] the LVMVolumeGroup %s belongs to the node %s. Starts to reconcile", lvg.Name, cfg.NodeName)) + log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] tries to add label %s to the LVMVolumeGroup %s", LVGMetadateNameLabelKey, cfg.NodeName)) + added, err = addLVGLabelIfNeeded(ctx, cl, log, lvg, LVGMetadateNameLabelKey, lvg.Name) + if err != nil { + log.Error(err, fmt.Sprintf("[RunLVMVolumeGroupWatcherController] unable to add label %s to the LVMVolumeGroup %s", LVGMetadateNameLabelKey, lvg.Name)) + return reconcile.Result{}, err + } + + if added { + log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] successfully added label %s to the LVMVolumeGroup %s", LVGMetadateNameLabelKey, lvg.Name)) + } else { + log.Debug(fmt.Sprintf("[RunLVMVolumeGroupWatcherController] no need to add label %s to the LVMVolumeGroup %s", LVGMetadateNameLabelKey, lvg.Name)) + } + // this case handles the situation when a user decides to remove LVMVolumeGroup resource without created VG vgs, _ := sdsCache.GetVGs() if !checkIfVGExist(lvg.Spec.ActualVGNameOnTheNode, vgs) && lvg.DeletionTimestamp != nil { diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go index 92ec4c40..a3eaebe5 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go @@ -25,15 +25,16 @@ import ( "context" "errors" "fmt" - "github.com/deckhouse/sds-node-configurator/api/v1alpha1" - "k8s.io/apimachinery/pkg/api/resource" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/strings/slices" "reflect" "strconv" "strings" "time" + "github.com/deckhouse/sds-node-configurator/api/v1alpha1" + "k8s.io/apimachinery/pkg/api/resource" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/strings/slices" + "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -73,22 +74,22 @@ func checkIfVGExist(vgName string, vgs []internal.VGData) bool { return false } -func checkLabels(log logger.Logger, newLVG *v1alpha1.LvmVolumeGroup) bool { +func checkLVGLabels(log logger.Logger, lvg *v1alpha1.LvmVolumeGroup, labelKey, labelValue string) bool { - if newLVG.Labels == nil { - log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has no labels", newLVG.Name)) + if lvg.Labels == nil { + log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has no labels.", lvg.Name)) return false } - labelValue, exist := newLVG.Labels[LVGHostnameLabelKey] + val, exist := lvg.Labels[labelKey] if !exist { - log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has no label %s", newLVG.Name, LVGHostnameLabelKey)) + log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has no label %s.", lvg.Name, labelKey)) return false } - if labelValue != newLVG.Name { - log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup %s has label %s but the value is incorrect - %q, but should be %q", newLVG.Name, LVGHostnameLabelKey, labelValue, newLVG.Name)) + if val != labelValue { + log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has label %s but the value is incorrect - %q (should be %q)", lvg.Name, labelKey, val, labelValue)) return false } @@ -101,7 +102,7 @@ func shouldLVGWatcherReconcileUpdateEvent(log logger.Logger, oldLVG, newLVG *v1a return true } - if !checkLabels(log, newLVG) { + if !checkLVGLabels(log, newLVG, LVGMetadateNameLabelKey, newLVG.Name) { log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup's %s labels have been changed", newLVG.Name)) return true } @@ -985,3 +986,21 @@ func ExtendThinPool(log logger.Logger, metrics monitoring.Metrics, lvg *v1alpha1 return nil } + +func addLVGLabelIfNeeded(ctx context.Context, cl client.Client, log logger.Logger, lvg *v1alpha1.LvmVolumeGroup, labelKey, labelValue string) (bool, error) { + if checkLVGLabels(log, lvg, labelKey, labelValue) { + return false, nil + } + + if lvg.Labels == nil { + lvg.Labels = make(map[string]string) + } + + lvg.Labels[labelKey] = labelValue + err := cl.Update(ctx, lvg) + if err != nil { + return false, err + } + + return true, nil +} From 4141f65ba18fb59ae0074be91983f5dd6373ee4f Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Mon, 19 Aug 2024 10:47:19 +0300 Subject: [PATCH 09/15] fix Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 7 +++---- images/agent/src/pkg/controller/block_device_test.go | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 46ebedc4..37ca6171 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -141,7 +141,6 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log return false } - func hasBlockDeviceDiff(blockDevice v1alpha1.BlockDevice, candidate internal.BlockDeviceCandidate) bool { hasBlockDeviceDiff := candidate.NodeName != blockDevice.Status.NodeName || candidate.Consumable != blockDevice.Status.Consumable || @@ -159,7 +158,7 @@ func hasBlockDeviceDiff(blockDevice v1alpha1.BlockDevice, candidate internal.Blo candidate.HotPlug != blockDevice.Status.HotPlug || candidate.Type != blockDevice.Status.Type || candidate.FSType != blockDevice.Status.FsType || - candidate.MachineId != blockDevice.Status.MachineID + candidate.MachineID != blockDevice.Status.MachineID if hasBlockDeviceDiff { return hasBlockDeviceDiff } @@ -604,8 +603,8 @@ func CreateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito start := time.Now() err := kc.Create(ctx, blockDevice) - metrics.ApiMethodsDuration(BlockDeviceCtrlName, "create").Observe(metrics.GetEstimatedTimeInSeconds(start)) - metrics.ApiMethodsExecutionCount(BlockDeviceCtrlName, "create").Inc() + metrics.APIMethodsDuration(BlockDeviceCtrlName, "create").Observe(metrics.GetEstimatedTimeInSeconds(start)) + metrics.APIMethodsExecutionCount(BlockDeviceCtrlName, "create").Inc() if err != nil { metrics.APIMethodsErrors(BlockDeviceCtrlName, "create").Inc() return nil, err diff --git a/images/agent/src/pkg/controller/block_device_test.go b/images/agent/src/pkg/controller/block_device_test.go index c70a802e..2f4ad041 100644 --- a/images/agent/src/pkg/controller/block_device_test.go +++ b/images/agent/src/pkg/controller/block_device_test.go @@ -388,7 +388,7 @@ func TestBlockDeviceCtrl(t *testing.T) { expected := []bool{false, true} for i, candidate := range candidates { - actual := hasBlockDeviceDiff(blockDevice.Status, candidate) + actual := hasBlockDeviceDiff(blockDevice, candidate) assert.Equal(t, expected[i], actual) } }) From d4818d29e1ca1268a1f967f8ffcb26cddff8e093 Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Mon, 19 Aug 2024 11:00:08 +0300 Subject: [PATCH 10/15] fix linter issues Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 8 +------- .../src/pkg/controller/lvm_logical_volume_watcher_func.go | 1 - 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 37ca6171..0de2e485 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -127,7 +127,6 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log // add new api device to the map, so it won't be deleted as fantom apiBlockDevices[candidate.Name] = *device - } } @@ -166,11 +165,7 @@ func hasBlockDeviceDiff(blockDevice v1alpha1.BlockDevice, candidate internal.Blo newLabels := GetBlockDeviceLabels(blockDevice) equal := reflect.DeepEqual(newLabels, blockDevice.Labels) - if equal { - return false - } - - return true + return !equal } func GetAPIBlockDevices(ctx context.Context, kc kclient.Client, metrics monitoring.Metrics) (map[string]v1alpha1.BlockDevice, error) { @@ -549,7 +544,6 @@ func UpdateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito } func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { - if blockDevice.Labels == nil { blockDevice.Labels = make(map[string]string) } diff --git a/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go b/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go index 91ad1c81..811168ce 100644 --- a/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go +++ b/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go @@ -73,7 +73,6 @@ func getLLVRequestedSize(llv *v1alpha1.LVMLogicalVolume, lvg *v1alpha1.LvmVolume } } } - return resource.Quantity{}, nil } From ee43c9f973882927fcc29145a9bfad5e797ebced Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Mon, 19 Aug 2024 11:02:17 +0300 Subject: [PATCH 11/15] fix linter issues Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go | 1 - 1 file changed, 1 deletion(-) diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go index 23ae987b..26a3dd51 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go @@ -75,7 +75,6 @@ func checkIfVGExist(vgName string, vgs []internal.VGData) bool { } func checkLVGLabels(log logger.Logger, lvg *v1alpha1.LvmVolumeGroup, labelKey, labelValue string) bool { - if lvg.Labels == nil { log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has no labels.", lvg.Name)) return false From 39bae490b50e7703cedc931602b9c6fbf5b3eb63 Mon Sep 17 00:00:00 2001 From: Viktor Kramarenko Date: Sun, 25 Aug 2024 13:25:50 +0300 Subject: [PATCH 12/15] test fixes and some refactoring Signed-off-by: Viktor Kramarenko --- .../agent/src/pkg/controller/block_device.go | 82 +++++++++---------- .../src/pkg/controller/block_device_test.go | 47 +++++++++++ .../lvm_logical_volume_watcher_func.go | 1 + .../lvm_volume_group_watcher_func.go | 21 +++-- .../lvm_volume_group_watcher_test.go | 45 ++++++++++ 5 files changed, 140 insertions(+), 56 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 0de2e485..843f1792 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -111,23 +111,24 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log continue } - if err := UpdateAPIBlockDevice(ctx, cl, metrics, blockDevice, candidate); err != nil { + if err = UpdateAPIBlockDevice(ctx, cl, metrics, blockDevice, candidate); err != nil { log.Error(err, "[RunBlockDeviceController] unable to update blockDevice, name: %s", blockDevice.Name) continue } log.Info(fmt.Sprintf(`[RunBlockDeviceController] updated APIBlockDevice, name: %s`, blockDevice.Name)) - } else { - device, err := CreateAPIBlockDevice(ctx, cl, metrics, candidate) - if err != nil { - log.Error(err, fmt.Sprintf("[RunBlockDeviceController] unable to create block device blockDevice, name: %s", candidate.Name)) - continue - } - log.Info(fmt.Sprintf("[RunBlockDeviceController] created new APIBlockDevice: %s", candidate.Name)) + continue + } - // add new api device to the map, so it won't be deleted as fantom - apiBlockDevices[candidate.Name] = *device + device, err := CreateAPIBlockDevice(ctx, cl, metrics, candidate) + if err != nil { + log.Error(err, fmt.Sprintf("[RunBlockDeviceController] unable to create block device blockDevice, name: %s", candidate.Name)) + continue } + log.Info(fmt.Sprintf("[RunBlockDeviceController] created new APIBlockDevice: %s", candidate.Name)) + + // add new api device to the map, so it won't be deleted as fantom + apiBlockDevices[candidate.Name] = *device } // delete api device if device no longer exists, but we still have its api resource @@ -141,7 +142,7 @@ func BlockDeviceReconcile(ctx context.Context, cl kclient.Client, log logger.Log } func hasBlockDeviceDiff(blockDevice v1alpha1.BlockDevice, candidate internal.BlockDeviceCandidate) bool { - hasBlockDeviceDiff := candidate.NodeName != blockDevice.Status.NodeName || + return candidate.NodeName != blockDevice.Status.NodeName || candidate.Consumable != blockDevice.Status.Consumable || candidate.PVUuid != blockDevice.Status.PVUuid || candidate.VGUuid != blockDevice.Status.VGUuid || @@ -157,15 +158,8 @@ func hasBlockDeviceDiff(blockDevice v1alpha1.BlockDevice, candidate internal.Blo candidate.HotPlug != blockDevice.Status.HotPlug || candidate.Type != blockDevice.Status.Type || candidate.FSType != blockDevice.Status.FsType || - candidate.MachineID != blockDevice.Status.MachineID - if hasBlockDeviceDiff { - return hasBlockDeviceDiff - } - - newLabels := GetBlockDeviceLabels(blockDevice) - equal := reflect.DeepEqual(newLabels, blockDevice.Labels) - - return !equal + candidate.MachineID != blockDevice.Status.MachineID || + !reflect.DeepEqual(ConfigureBlockDeviceLabels(blockDevice), blockDevice.Labels) } func GetAPIBlockDevices(ctx context.Context, kc kclient.Client, metrics monitoring.Metrics) (map[string]v1alpha1.BlockDevice, error) { @@ -529,7 +523,7 @@ func UpdateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito MachineID: candidate.MachineID, } - blockDevice.Labels = GetBlockDeviceLabels(blockDevice) + blockDevice.Labels = ConfigureBlockDeviceLabels(blockDevice) start := time.Now() err := kc.Update(ctx, &blockDevice) @@ -543,29 +537,27 @@ func UpdateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito return nil } -func GetBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { - if blockDevice.Labels == nil { - blockDevice.Labels = make(map[string]string) - } - - blockDevice.Labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name - blockDevice.Labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName - blockDevice.Labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type - blockDevice.Labels[BlockDeviceLabelPrefix+"/fstype"] = blockDevice.Status.FsType - blockDevice.Labels[BlockDeviceLabelPrefix+"/pvuuid"] = blockDevice.Status.PVUuid - blockDevice.Labels[BlockDeviceLabelPrefix+"/vguuid"] = blockDevice.Status.VGUuid - blockDevice.Labels[BlockDeviceLabelPrefix+"/partuuid"] = blockDevice.Status.PartUUID - blockDevice.Labels[BlockDeviceLabelPrefix+"/lvmvolumegroupname"] = blockDevice.Status.LvmVolumeGroupName - blockDevice.Labels[BlockDeviceLabelPrefix+"/actualvgnameonthenode"] = blockDevice.Status.ActualVGNameOnTheNode - blockDevice.Labels[BlockDeviceLabelPrefix+"/wwn"] = blockDevice.Status.Wwn - blockDevice.Labels[BlockDeviceLabelPrefix+"/serial"] = blockDevice.Status.Serial - blockDevice.Labels[BlockDeviceLabelPrefix+"/size"] = blockDevice.Status.Size.String() - blockDevice.Labels[BlockDeviceLabelPrefix+"/model"] = blockDevice.Status.Model - blockDevice.Labels[BlockDeviceLabelPrefix+"/rota"] = strconv.FormatBool(blockDevice.Status.Rota) - blockDevice.Labels[BlockDeviceLabelPrefix+"/hotplug"] = strconv.FormatBool(blockDevice.Status.HotPlug) - blockDevice.Labels[BlockDeviceLabelPrefix+"/machineid"] = blockDevice.Status.MachineID - - return blockDevice.Labels +func ConfigureBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { + labels := make(map[string]string, 16) + + labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name + labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName + labels[BlockDeviceLabelPrefix+"/type"] = blockDevice.Status.Type + labels[BlockDeviceLabelPrefix+"/fstype"] = blockDevice.Status.FsType + labels[BlockDeviceLabelPrefix+"/pvuuid"] = blockDevice.Status.PVUuid + labels[BlockDeviceLabelPrefix+"/vguuid"] = blockDevice.Status.VGUuid + labels[BlockDeviceLabelPrefix+"/partuuid"] = blockDevice.Status.PartUUID + labels[BlockDeviceLabelPrefix+"/lvmvolumegroupname"] = blockDevice.Status.LvmVolumeGroupName + labels[BlockDeviceLabelPrefix+"/actualvgnameonthenode"] = blockDevice.Status.ActualVGNameOnTheNode + labels[BlockDeviceLabelPrefix+"/wwn"] = blockDevice.Status.Wwn + labels[BlockDeviceLabelPrefix+"/serial"] = blockDevice.Status.Serial + labels[BlockDeviceLabelPrefix+"/size"] = blockDevice.Status.Size.String() + labels[BlockDeviceLabelPrefix+"/model"] = blockDevice.Status.Model + labels[BlockDeviceLabelPrefix+"/rota"] = strconv.FormatBool(blockDevice.Status.Rota) + labels[BlockDeviceLabelPrefix+"/hotplug"] = strconv.FormatBool(blockDevice.Status.HotPlug) + labels[BlockDeviceLabelPrefix+"/machineid"] = blockDevice.Status.MachineID + + return labels } func CreateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monitoring.Metrics, candidate internal.BlockDeviceCandidate) (*v1alpha1.BlockDevice, error) { @@ -593,7 +585,7 @@ func CreateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito }, } - blockDevice.Labels = GetBlockDeviceLabels(*blockDevice) + blockDevice.Labels = ConfigureBlockDeviceLabels(*blockDevice) start := time.Now() err := kc.Create(ctx, blockDevice) diff --git a/images/agent/src/pkg/controller/block_device_test.go b/images/agent/src/pkg/controller/block_device_test.go index 2f4ad041..7d305d13 100644 --- a/images/agent/src/pkg/controller/block_device_test.go +++ b/images/agent/src/pkg/controller/block_device_test.go @@ -20,6 +20,7 @@ import ( "bytes" "context" "fmt" + "strconv" "testing" "github.com/deckhouse/sds-node-configurator/api/v1alpha1" @@ -318,6 +319,50 @@ func TestBlockDeviceCtrl(t *testing.T) { } }) + t.Run("ConfigureBlockDeviceLabels", func(t *testing.T) { + blockDevice := v1alpha1.BlockDevice{ + Status: v1alpha1.BlockDeviceStatus{ + Type: "testTYPE", + FsType: "testFS", + NodeName: "test_node", + Consumable: false, + PVUuid: "testPV", + VGUuid: "testVGUID", + LvmVolumeGroupName: "testLVGName", + ActualVGNameOnTheNode: "testNameOnNode", + Wwn: "testWWN", + Serial: "testSERIAL", + Path: "testPATH", + Size: resource.MustParse("0"), + Model: "testMODEL", + Rota: false, + HotPlug: false, + MachineID: "testMACHINE", + }, + } + + expectedLabels := map[string]string{ + "kubernetes.io/metadata.name": blockDevice.ObjectMeta.Name, + "kubernetes.io/hostname": blockDevice.Status.NodeName, + BlockDeviceLabelPrefix + "/type": blockDevice.Status.Type, + BlockDeviceLabelPrefix + "/fstype": blockDevice.Status.FsType, + BlockDeviceLabelPrefix + "/pvuuid": blockDevice.Status.PVUuid, + BlockDeviceLabelPrefix + "/vguuid": blockDevice.Status.VGUuid, + BlockDeviceLabelPrefix + "/partuuid": blockDevice.Status.PartUUID, + BlockDeviceLabelPrefix + "/lvmvolumegroupname": blockDevice.Status.LvmVolumeGroupName, + BlockDeviceLabelPrefix + "/actualvgnameonthenode": blockDevice.Status.ActualVGNameOnTheNode, + BlockDeviceLabelPrefix + "/wwn": blockDevice.Status.Wwn, + BlockDeviceLabelPrefix + "/serial": blockDevice.Status.Serial, + BlockDeviceLabelPrefix + "/size": blockDevice.Status.Size.String(), + BlockDeviceLabelPrefix + "/model": blockDevice.Status.Model, + BlockDeviceLabelPrefix + "/rota": strconv.FormatBool(blockDevice.Status.Rota), + BlockDeviceLabelPrefix + "/hotplug": strconv.FormatBool(blockDevice.Status.HotPlug), + BlockDeviceLabelPrefix + "/machineid": blockDevice.Status.MachineID, + } + + assert.Equal(t, expectedLabels, ConfigureBlockDeviceLabels(blockDevice)) + }) + t.Run("hasBlockDeviceDiff", func(t *testing.T) { candidates := []internal.BlockDeviceCandidate{ // same state @@ -385,6 +430,8 @@ func TestBlockDeviceCtrl(t *testing.T) { MachineID: "testMACHINE", }, } + labels := ConfigureBlockDeviceLabels(blockDevice) + blockDevice.Labels = labels expected := []bool{false, true} for i, candidate := range candidates { diff --git a/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go b/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go index 811168ce..91ad1c81 100644 --- a/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go +++ b/images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go @@ -73,6 +73,7 @@ func getLLVRequestedSize(llv *v1alpha1.LVMLogicalVolume, lvg *v1alpha1.LvmVolume } } } + return resource.Quantity{}, nil } diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go index 26a3dd51..0e9ea991 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go @@ -74,25 +74,24 @@ func checkIfVGExist(vgName string, vgs []internal.VGData) bool { return false } -func checkLVGLabels(log logger.Logger, lvg *v1alpha1.LvmVolumeGroup, labelKey, labelValue string) bool { +func shouldUpdateLVGLabels(log logger.Logger, lvg *v1alpha1.LvmVolumeGroup, labelKey, labelValue string) bool { if lvg.Labels == nil { - log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has no labels.", lvg.Name)) - return false + log.Debug(fmt.Sprintf("[shouldUpdateLVGLabels] the LVMVolumeGroup %s has no labels.", lvg.Name)) + return true } val, exist := lvg.Labels[labelKey] - if !exist { - log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has no label %s.", lvg.Name, labelKey)) - return false + log.Debug(fmt.Sprintf("[shouldUpdateLVGLabels] the LVMVolumeGroup %s has no label %s.", lvg.Name, labelKey)) + return true } if val != labelValue { - log.Debug(fmt.Sprintf("[checkLabels] the LVMVolumeGroup %s has label %s but the value is incorrect - %q (should be %q)", lvg.Name, labelKey, val, labelValue)) - return false + log.Debug(fmt.Sprintf("[shouldUpdateLVGLabels] the LVMVolumeGroup %s has label %s but the value is incorrect - %s (should be %s)", lvg.Name, labelKey, val, labelValue)) + return true } - return true + return false } func shouldLVGWatcherReconcileUpdateEvent(log logger.Logger, oldLVG, newLVG *v1alpha1.LvmVolumeGroup) bool { @@ -101,7 +100,7 @@ func shouldLVGWatcherReconcileUpdateEvent(log logger.Logger, oldLVG, newLVG *v1a return true } - if !checkLVGLabels(log, newLVG, LVGMetadateNameLabelKey, newLVG.Name) { + if shouldUpdateLVGLabels(log, newLVG, LVGMetadateNameLabelKey, newLVG.Name) { log.Debug(fmt.Sprintf("[shouldLVGWatcherReconcileUpdateEvent] update event should be reconciled as the LVMVolumeGroup's %s labels have been changed", newLVG.Name)) return true } @@ -975,7 +974,7 @@ func ExtendThinPool(log logger.Logger, metrics monitoring.Metrics, lvg *v1alpha1 } func addLVGLabelIfNeeded(ctx context.Context, cl client.Client, log logger.Logger, lvg *v1alpha1.LvmVolumeGroup, labelKey, labelValue string) (bool, error) { - if checkLVGLabels(log, lvg, labelKey, labelValue) { + if !shouldUpdateLVGLabels(log, lvg, labelKey, labelValue) { return false, nil } diff --git a/images/agent/src/pkg/controller/lvm_volume_group_watcher_test.go b/images/agent/src/pkg/controller/lvm_volume_group_watcher_test.go index 3427d098..b1fa2bad 100644 --- a/images/agent/src/pkg/controller/lvm_volume_group_watcher_test.go +++ b/images/agent/src/pkg/controller/lvm_volume_group_watcher_test.go @@ -997,27 +997,45 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { t.Run("condition_vg_configuration_applied_is_updating_returns_false", func(t *testing.T) { oldLVG := &v1alpha1.LvmVolumeGroup{} newLVG := &v1alpha1.LvmVolumeGroup{} + newLVG.Name = "test-name" newLVG.Status.Conditions = []v1.Condition{ { Type: internal.TypeVGConfigurationApplied, Reason: internal.ReasonUpdating, }, } + newLVG.Labels = map[string]string{LVGMetadateNameLabelKey: newLVG.Name} assert.False(t, shouldLVGWatcherReconcileUpdateEvent(log, oldLVG, newLVG)) }) t.Run("condition_vg_configuration_applied_is_creating_returns_false", func(t *testing.T) { oldLVG := &v1alpha1.LvmVolumeGroup{} newLVG := &v1alpha1.LvmVolumeGroup{} + newLVG.Name = "test-name" newLVG.Status.Conditions = []v1.Condition{ { Type: internal.TypeVGConfigurationApplied, Reason: internal.ReasonCreating, }, } + newLVG.Labels = map[string]string{LVGMetadateNameLabelKey: newLVG.Name} assert.False(t, shouldLVGWatcherReconcileUpdateEvent(log, oldLVG, newLVG)) }) + t.Run("label_is_not_the_same_returns_true", func(t *testing.T) { + oldLVG := &v1alpha1.LvmVolumeGroup{} + newLVG := &v1alpha1.LvmVolumeGroup{} + newLVG.Name = "test-name" + newLVG.Status.Conditions = []v1.Condition{ + { + Type: internal.TypeVGConfigurationApplied, + Reason: internal.ReasonCreating, + }, + } + newLVG.Labels = map[string]string{LVGMetadateNameLabelKey: "some-other-name"} + assert.True(t, shouldLVGWatcherReconcileUpdateEvent(log, oldLVG, newLVG)) + }) + t.Run("dev_size_and_pv_size_are_diff_returns_true", func(t *testing.T) { oldLVG := &v1alpha1.LvmVolumeGroup{} newLVG := &v1alpha1.LvmVolumeGroup{} @@ -1037,6 +1055,33 @@ func TestLVMVolumeGroupWatcherCtrl(t *testing.T) { }) }) + t.Run("shouldUpdateLVGLabels", func(t *testing.T) { + t.Run("labels_nil_returns_true", func(t *testing.T) { + lvg := &v1alpha1.LvmVolumeGroup{} + assert.True(t, shouldUpdateLVGLabels(log, lvg, "key", "value")) + }) + t.Run("no_such_label_returns_true", func(t *testing.T) { + lvg := &v1alpha1.LvmVolumeGroup{} + lvg.Labels = map[string]string{"key": "value"} + assert.True(t, shouldUpdateLVGLabels(log, lvg, "other-key", "value")) + }) + t.Run("key_exists_other_value_returns_true", func(t *testing.T) { + const key = "key" + lvg := &v1alpha1.LvmVolumeGroup{} + lvg.Labels = map[string]string{key: "value"} + assert.True(t, shouldUpdateLVGLabels(log, lvg, key, "other-value")) + }) + t.Run("all_good_returns_false", func(t *testing.T) { + const ( + key = "key" + value = "value" + ) + lvg := &v1alpha1.LvmVolumeGroup{} + lvg.Labels = map[string]string{key: value} + assert.False(t, shouldUpdateLVGLabels(log, lvg, key, value)) + }) + }) + t.Run("checkIfVGExist", func(t *testing.T) { const targetName = "test" vgs := []internal.VGData{ From 2c4a0aeddd318ecfe96338a1c745420816f1ad7f Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Mon, 26 Aug 2024 12:55:18 +0300 Subject: [PATCH 13/15] Preserve users labels Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index 843f1792..ae246655 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -538,7 +538,16 @@ func UpdateAPIBlockDevice(ctx context.Context, kc kclient.Client, metrics monito } func ConfigureBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]string { - labels := make(map[string]string, 16) + var labels map[string]string + if blockDevice.Labels == nil { + labels = make(map[string]string, 16) + } else { + labels = make(map[string]string, len(blockDevice.Labels)) + } + + for k, v := range blockDevice.Labels { + labels[k] = v + } labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name labels["kubernetes.io/hostname"] = blockDevice.Status.NodeName From c648ce9fc47155bb3faf25f6d899ce2c0da9709c Mon Sep 17 00:00:00 2001 From: Aleksandr Zimin Date: Mon, 26 Aug 2024 13:00:54 +0300 Subject: [PATCH 14/15] fix naming Signed-off-by: Aleksandr Zimin --- images/agent/src/pkg/controller/block_device.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/images/agent/src/pkg/controller/block_device.go b/images/agent/src/pkg/controller/block_device.go index ae246655..c0e5bc32 100644 --- a/images/agent/src/pkg/controller/block_device.go +++ b/images/agent/src/pkg/controller/block_device.go @@ -545,8 +545,8 @@ func ConfigureBlockDeviceLabels(blockDevice v1alpha1.BlockDevice) map[string]str labels = make(map[string]string, len(blockDevice.Labels)) } - for k, v := range blockDevice.Labels { - labels[k] = v + for key, value := range blockDevice.Labels { + labels[key] = value } labels["kubernetes.io/metadata.name"] = blockDevice.ObjectMeta.Name From 9e2ef850bd1d03fc1f90925be06360739091856b Mon Sep 17 00:00:00 2001 From: Viktor Kramarenko Date: Mon, 26 Aug 2024 15:07:50 +0300 Subject: [PATCH 15/15] test fixes and some refactoring Signed-off-by: Viktor Kramarenko --- images/agent/src/pkg/controller/block_device_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/images/agent/src/pkg/controller/block_device_test.go b/images/agent/src/pkg/controller/block_device_test.go index 7d305d13..7bb31398 100644 --- a/images/agent/src/pkg/controller/block_device_test.go +++ b/images/agent/src/pkg/controller/block_device_test.go @@ -340,6 +340,10 @@ func TestBlockDeviceCtrl(t *testing.T) { MachineID: "testMACHINE", }, } + blockDevice.Labels = map[string]string{ + "some-custom-label1": "v", + "some-custom-label2": "v", + } expectedLabels := map[string]string{ "kubernetes.io/metadata.name": blockDevice.ObjectMeta.Name, @@ -358,6 +362,8 @@ func TestBlockDeviceCtrl(t *testing.T) { BlockDeviceLabelPrefix + "/rota": strconv.FormatBool(blockDevice.Status.Rota), BlockDeviceLabelPrefix + "/hotplug": strconv.FormatBool(blockDevice.Status.HotPlug), BlockDeviceLabelPrefix + "/machineid": blockDevice.Status.MachineID, + "some-custom-label1": "v", + "some-custom-label2": "v", } assert.Equal(t, expectedLabels, ConfigureBlockDeviceLabels(blockDevice))