Skip to content

Commit

Permalink
test fixes and some refactoring
Browse files Browse the repository at this point in the history 
Signed-off-by: Viktor Kramarenko <[email protected]>
  • Loading branch information
@ViktorKram
ViktorKram committed Aug 25, 2024
1 parent ee43c9f commit f6be271
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 56 deletions.
82 changes: 37 additions & 45 deletions images/agent/src/pkg/controller/block_device.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 ||
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 6 additions & 0 deletions images/agent/src/pkg/controller/block_device_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
1 change: 1 addition & 0 deletions images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ func getLLVRequestedSize(llv *v1alpha1.LVMLogicalVolume, lvg *v1alpha1.LvmVolume
}
}
}

return resource.Quantity{}, nil
}

Expand Down
21 changes: 10 additions & 11 deletions images/agent/src/pkg/controller/lvm_volume_group_watcher_func.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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
}

Expand Down
45 changes: 45 additions & 0 deletions images/agent/src/pkg/controller/lvm_volume_group_watcher_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Expand All @@ -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{
Expand Down

0 comments on commit f6be271

Please sign in to comment.