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 39bae49
Show file tree
Hide file tree
Showing 5 changed files with 140 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
47 changes: 47 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 @@ -20,6 +20,7 @@ import (
"bytes"
"context"
"fmt"
"strconv"
"testing"

"github.com/deckhouse/sds-node-configurator/api/v1alpha1"
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
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 39bae49

Please sign in to comment.