From f6be2714676f1cd44de5766436b6453b757567a9 Mon Sep 17 00:00:00 2001 From: Viktor Kramarenko Date: Sun, 25 Aug 2024 13:25:50 +0300 Subject: [PATCH] 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 | 6 ++ .../lvm_logical_volume_watcher_func.go | 1 + .../lvm_volume_group_watcher_func.go | 21 +++-- .../lvm_volume_group_watcher_test.go | 45 ++++++++++ 5 files changed, 99 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..080783e8 100644 --- a/images/agent/src/pkg/controller/block_device_test.go +++ b/images/agent/src/pkg/controller/block_device_test.go @@ -318,6 +318,10 @@ func TestBlockDeviceCtrl(t *testing.T) { } }) + t.Run("ConfigureBlockDeviceLabels", func(t *testing.T) { + + }) + t.Run("hasBlockDeviceDiff", func(t *testing.T) { candidates := []internal.BlockDeviceCandidate{ // same state @@ -385,6 +389,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{