Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[controller] Refactor code on linter issues and test improvements. #81

Merged
merged 6 commits into from
Aug 16, 2024
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
refactored src dir
Signed-off-by: Viktor Kramarenko <viktor.kramarenko@flant.com>
ViktorKram committed Aug 16, 2024
commit 4b9f5f2efd56605ce8f94d3b9a34f1722d933fde
1 change: 1 addition & 0 deletions .golangci.yaml
Original file line number Diff line number Diff line change
@@ -7,6 +7,7 @@ issues:
- "should not use dot imports"
- "don't use an underscore in package name"
- "exported: .*"
- "could not import"

linters-settings:
gci:
2 changes: 1 addition & 1 deletion images/agent/src/config/config_test.go
Original file line number Diff line number Diff line change
@@ -89,7 +89,7 @@ func TestNewConfig(t *testing.T) {
expMetricsPort := ":0000"
expNodeName := "test-node"
expErrorMsg := fmt.Sprintf("[NewConfig] unable to get %s, error: %s",
MachineID, "open /host-root/etc/machine-id: no such file or directory")
MachineID, "fork/exec /opt/deckhouse/sds/bin/nsenter.static: no such file or directory")

err := os.Setenv(MetricsPort, expMetricsPort)
if err != nil {
191 changes: 187 additions & 4 deletions images/agent/src/pkg/controller/block_device_test.go
Original file line number Diff line number Diff line change
@@ -17,11 +17,18 @@ limitations under the License.
package controller

import (
"agent/config"
"agent/internal"
"agent/pkg/cache"
"agent/pkg/logger"
"agent/pkg/monitoring"
"agent/pkg/utils"
"bytes"
"fmt"
"github.com/deckhouse/sds-node-configurator/api/v1alpha1"
errors2 "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"
"testing"

"k8s.io/apimachinery/pkg/api/resource"
@@ -30,6 +37,186 @@ import (
)

func TestBlockDeviceCtrl(t *testing.T) {
log, _ := logger.NewLogger("1")
cfg := config.Options{
NodeName: "test-node",
MachineId: "test-id",
}

t.Run("shouldDeleteBlockDevice", func(t *testing.T) {
t.Run("returns_true", func(t *testing.T) {
bd := v1alpha1.BlockDevice{
Status: v1alpha1.BlockDeviceStatus{
NodeName: cfg.NodeName,
Consumable: true,
},
}
actual := map[string]struct{}{}

assert.True(t, shouldDeleteBlockDevice(bd, actual, cfg.NodeName))
})

t.Run("returns_false_cause_of_dif_node", func(t *testing.T) {
bd := v1alpha1.BlockDevice{
Status: v1alpha1.BlockDeviceStatus{
NodeName: cfg.NodeName,
Consumable: true,
},
}
actual := map[string]struct{}{}

assert.False(t, shouldDeleteBlockDevice(bd, actual, "dif-node"))
})

t.Run("returns_false_cause_of_not_consumable", func(t *testing.T) {
bd := v1alpha1.BlockDevice{
Status: v1alpha1.BlockDeviceStatus{
NodeName: cfg.NodeName,
Consumable: false,
},
}
actual := map[string]struct{}{}

assert.False(t, shouldDeleteBlockDevice(bd, actual, cfg.NodeName))
})

t.Run("returns_false_cause_of_not_deprecated", func(t *testing.T) {
const name = "test"
bd := v1alpha1.BlockDevice{
ObjectMeta: metav1.ObjectMeta{
Name: name,
},
Status: v1alpha1.BlockDeviceStatus{
NodeName: cfg.NodeName,
Consumable: true,
},
}
actual := map[string]struct{}{
name: {},
}

assert.False(t, shouldDeleteBlockDevice(bd, actual, cfg.NodeName))
})
})

t.Run("RemoveDeprecatedAPIDevices", func(t *testing.T) {
const (
goodName = "test-candidate1"
badName = "test-candidate2"
)
cl := NewFakeClient()
candidates := []internal.BlockDeviceCandidate{
{
NodeName: cfg.NodeName,
Consumable: false,
PVUuid: "142412421",
VGUuid: "123123123",
LvmVolumeGroupName: "test-lvg",
ActualVGNameOnTheNode: "test-vg",
Wwn: "12414212",
Serial: "1412412412412",
Path: "/dev/vdb",
Size: resource.MustParse("1G"),
Rota: false,
Model: "124124-adf",
Name: goodName,
HotPlug: false,
MachineId: "1245151241241",
},
}

bds := map[string]v1alpha1.BlockDevice{
goodName: {
ObjectMeta: metav1.ObjectMeta{
Name: goodName,
},
},
badName: {
ObjectMeta: metav1.ObjectMeta{
Name: badName,
},
Status: v1alpha1.BlockDeviceStatus{
Consumable: true,
NodeName: cfg.NodeName,
},
},
}

for _, bd := range bds {
err := cl.Create(ctx, &bd)
if err != nil {
t.Error(err)
}
}

defer func() {
for _, bd := range bds {
cl.Delete(ctx, &bd)
}
}()

for _, bd := range bds {
createdBd := &v1alpha1.BlockDevice{}
err := cl.Get(ctx, client.ObjectKey{
Name: bd.Name,
}, createdBd)
if err != nil {
t.Error(err)
}
assert.Equal(t, bd.Name, createdBd.Name)
}

RemoveDeprecatedAPIDevices(ctx, cl, *log, monitoring.GetMetrics(cfg.NodeName), candidates, bds, cfg.NodeName)

_, ok := bds[badName]
assert.False(t, ok)

deleted := &v1alpha1.BlockDevice{}
err := cl.Get(ctx, client.ObjectKey{
Name: badName,
}, deleted)
if assert.True(t, errors2.IsNotFound(err)) {
assert.Equal(t, "", deleted.Name)
}
})

t.Run("GetBlockDeviceCandidates", func(t *testing.T) {
devices := []internal.Device{
{
Name: "valid1",
Size: resource.MustParse("1G"),
Serial: "131412",
},
{
Name: "valid2",
Size: resource.MustParse("1G"),
Serial: "12412412",
},
{
Name: "valid3",
Size: resource.MustParse("1G"),
Serial: "4214215",
},
{
Name: "invalid",
FSType: "ext4",
Size: resource.MustParse("1G"),
},
}

sdsCache := cache.New()
sdsCache.StoreDevices(devices, bytes.Buffer{})

candidates := GetBlockDeviceCandidates(*log, cfg, sdsCache)

assert.Equal(t, 3, len(candidates))
for i := range candidates {
assert.Equal(t, devices[i].Name, candidates[i].Path)
assert.Equal(t, cfg.MachineId, candidates[i].MachineId)
assert.Equal(t, cfg.NodeName, candidates[i].NodeName)
}
})

t.Run("CheckConsumable", func(t *testing.T) {
t.Run("Good device returns true", func(t *testing.T) {
goodDevice := internal.Device{
@@ -205,10 +392,6 @@ func TestBlockDeviceCtrl(t *testing.T) {
})

t.Run("validateTestLSBLKOutput", func(t *testing.T) {
log, err := logger.NewLogger("1")
if err != nil {
t.Fatal(err)
}
testLsblkOutputBytes := []byte(testLsblkOutput)
devices, err := utils.UnmarshalDevices(testLsblkOutputBytes)
if assert.NoError(t, err) {
9 changes: 7 additions & 2 deletions images/agent/src/pkg/controller/controller_reconcile_test.go
Original file line number Diff line number Diff line change
@@ -21,8 +21,9 @@ import (
"agent/pkg/controller"
"agent/pkg/monitoring"
"context"

"github.com/deckhouse/sds-node-configurator/api/v1alpha1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
@@ -138,7 +139,11 @@ var _ = Describe("Storage Controller", func() {
})

It("DeleteAPIBlockDevice", func() {
err := controller.DeleteAPIBlockDevice(ctx, cl, testMetrics, deviceName)
err := controller.DeleteAPIBlockDevice(ctx, cl, testMetrics, &v1alpha1.BlockDevice{
ObjectMeta: metav1.ObjectMeta{
Name: deviceName,
},
})
Expect(err).NotTo(HaveOccurred())

devices, err := controller.GetAPIBlockDevices(context.Background(), cl, testMetrics)
16 changes: 10 additions & 6 deletions images/agent/src/pkg/controller/lvm_logical_volume_bench_test.go
Original file line number Diff line number Diff line change
@@ -20,10 +20,10 @@ import (
)

var (
lvgName = "hdd-lvg-on-node-0"
poolName = "hhd-thin"
lvCount = 600
size, err = resource.ParseQuantity("1Gi")
lvgName = "hdd-lvg-on-node-0"
poolName = "hhd-thin"
lvCount = 600
size = "1Gi"

resizeOn = false

@@ -85,7 +85,9 @@ func BenchmarkRunThickLLVCreationSingleThread(b *testing.B) {
continue
}

llv.Spec.Size.Add(add)
lvsize := resource.MustParse(llv.Spec.Size)
lvsize.Add(add)
llv.Spec.Size = lvsize.String()
err = e2eCL.Update(ctx, &llv)
if err != nil {
b.Logf(err.Error())
@@ -150,7 +152,9 @@ func BenchmarkRunThinLLVCreationSingleThread(b *testing.B) {
continue
}

llv.Spec.Size.Add(add)
lvsize := resource.MustParse(llv.Spec.Size)
lvsize.Add(add)
llv.Spec.Size = lvsize.String()
err = e2eCL.Update(ctx, &llv)
if err != nil {
b.Logf(err.Error())
20 changes: 10 additions & 10 deletions images/agent/src/pkg/controller/lvm_logical_volume_watcher_func.go
Original file line number Diff line number Diff line change
@@ -98,9 +98,9 @@ func removeLLVFinalizersIfExist(

if removed {
log.Trace(fmt.Sprintf("[removeLLVFinalizersIfExist] removed finalizer %s from the LVMLogicalVolume %s", internal.SdsNodeConfiguratorFinalizer, llv.Name))
err := updateLVMLogicalVolume(ctx, metrics, cl, llv)
err := updateLVMLogicalVolumeSpec(ctx, metrics, cl, llv)
if err != nil {
log.Error(err, fmt.Sprintf("[updateLVMLogicalVolume] unable to update the LVMVolumeGroup %s", llv.Name))
log.Error(err, fmt.Sprintf("[updateLVMLogicalVolumeSpec] unable to update the LVMVolumeGroup %s", llv.Name))
return err
}
}
@@ -194,7 +194,7 @@ func addLLVFinalizerIfNotExist(ctx context.Context, cl client.Client, log logger
llv.Finalizers = append(llv.Finalizers, internal.SdsNodeConfiguratorFinalizer)

log.Trace(fmt.Sprintf("[addLLVFinalizerIfNotExist] added finalizer %s to the LVMLogicalVolume %s", internal.SdsNodeConfiguratorFinalizer, llv.Name))
err := updateLVMLogicalVolume(ctx, metrics, cl, llv)
err := updateLVMLogicalVolumeSpec(ctx, metrics, cl, llv)
if err != nil {
return false, err
}
@@ -300,13 +300,13 @@ func validateLVMLogicalVolume(sdsCache *cache.Cache, llv *v1alpha1.LVMLogicalVol

// if a specified Thick LV name matches the existing Thin one
lv := sdsCache.FindLV(lvg.Spec.ActualVGNameOnTheNode, llv.Spec.ActualLVNameOnTheNode)
if lv != nil && len(lv.LVAttr) == 0 {
reason.WriteString(fmt.Sprintf("LV %s was found on the node, but can't be validated due to its attributes is empty string. ", lv.LVName))
}

if lv != nil {
if !checkIfLVBelongsToLLV(llv, lv) {
reason.WriteString(fmt.Sprintf("Specified LV %s is already created and it is doesnt match the one on the node.", lv.LVName))
if len(lv.LVAttr) == 0 {
reason.WriteString(fmt.Sprintf("LV %s was found on the node, but can't be validated due to its attributes is empty string. ", lv.LVName))
} else {
if !checkIfLVBelongsToLLV(llv, lv) {
reason.WriteString(fmt.Sprintf("Specified LV %s is already created and it is doesnt match the one on the node.", lv.LVName))
}
}
}

@@ -342,7 +342,7 @@ func updateLVMLogicalVolumePhaseIfNeeded(ctx context.Context, cl client.Client,
return nil
}

func updateLVMLogicalVolume(ctx context.Context, metrics monitoring.Metrics, cl client.Client, llv *v1alpha1.LVMLogicalVolume) error {
func updateLVMLogicalVolumeSpec(ctx context.Context, metrics monitoring.Metrics, cl client.Client, llv *v1alpha1.LVMLogicalVolume) error {
return cl.Update(ctx, llv)
}

320 changes: 161 additions & 159 deletions images/agent/src/pkg/controller/lvm_logical_volume_watcher_test.go

Large diffs are not rendered by default.

354 changes: 64 additions & 290 deletions images/agent/src/pkg/controller/lvm_volume_group_discover_test.go

Large diffs are not rendered by default.

162 changes: 2 additions & 160 deletions images/agent/src/pkg/controller/lvm_volume_group_test.go
Original file line number Diff line number Diff line change
@@ -115,11 +115,11 @@ func TestLvmVolumeGroupAPIObjects(t *testing.T) {
ThinPools: []v1alpha1.LvmVolumeGroupThinPoolSpec{
{
Name: "test-name",
Size: *convertSize("10G", t),
Size: "10G",
},
{
Name: "test-name2",
Size: *convertSize("1G", t),
Size: "1G",
},
},
},
@@ -176,164 +176,6 @@ func TestLvmVolumeGroupAPIObjects(t *testing.T) {
assert.Equal(t, expected, actual)
}
})

t.Run("Marshal_LvmVolumeGroup_struct_to_json", func(t *testing.T) {
expected := `{
"apiVersion": "storage.deckhouse.io/v1alpha1",
"kind": "LvmVolumeGroup",
"metadata": {
"creationTimestamp": null,
"name": "lvg-test-1"
},
"spec": {
"actualVGNameOnTheNode": "testVGname",
"blockDeviceNames": [
"test-bd",
"test-bd2"
],
"thinPools": [
{
"name": "test-name",
"size": "10G"
},
{
"name": "test-name2",
"size": "1G"
}
],
"type": "local"
},
"status": {
"conditions": null,
"allocatedSize": "20G",
"health": "operational",
"message": "all-good",
"nodes": [
{
"devices": [
{
"blockDevice": "test/BD",
"devSize": "1G",
"path": "test/path1",
"pvSize": "1G",
"pvUUID": "testPV1"
},
{
"blockDevice": "test/BD2",
"devSize": "1G",
"path": "test/path2",
"pvSize": "2G",
"pvUUID": "testPV2"
}
],
"name": "node1"
},
{
"devices": [
{
"blockDevice": "test/DB3",
"devSize": "2G",
"path": "test/path3",
"pvSize": "3G",
"pvUUID": "testPV3"
}
],
"name": "node2"
}
],
"phase": "",
"thinPools": [
{
"name": "test-name",
"actualSize": "1G",
"usedSize": "500M",
"ready": true,
"message": ""
}
],
"vgSize": "30G",
"vgUUID": "test-vg-uuid"
}
}`
testObj := v1alpha1.LvmVolumeGroup{
ObjectMeta: metav1.ObjectMeta{
Name: "lvg-test-1",
CreationTimestamp: metav1.Time{},
},
TypeMeta: metav1.TypeMeta{
Kind: "LvmVolumeGroup",
APIVersion: "storage.deckhouse.io/v1alpha1",
},
Spec: v1alpha1.LvmVolumeGroupSpec{
ActualVGNameOnTheNode: "testVGname",
BlockDeviceNames: []string{"test-bd", "test-bd2"},
ThinPools: []v1alpha1.LvmVolumeGroupThinPoolSpec{
{
Name: "test-name",
Size: *convertSize("10G", t),
},
{
Name: "test-name2",
Size: *convertSize("1G", t),
},
},
Type: "local",
},
Status: v1alpha1.LvmVolumeGroupStatus{
AllocatedSize: resource.MustParse("20G"),
Nodes: []v1alpha1.LvmVolumeGroupNode{
{
Devices: []v1alpha1.LvmVolumeGroupDevice{
{
BlockDevice: "test/BD",
DevSize: *convertSize("1G", t),
PVSize: resource.MustParse("1G"),
PVUuid: "testPV1",
Path: "test/path1",
},
{
BlockDevice: "test/BD2",
DevSize: *convertSize("1G", t),
PVSize: resource.MustParse("2G"),
PVUuid: "testPV2",
Path: "test/path2",
},
},
Name: "node1",
},
{
Devices: []v1alpha1.LvmVolumeGroupDevice{
{
BlockDevice: "test/DB3",
DevSize: *convertSize("2G", t),
PVSize: resource.MustParse("3G"),
PVUuid: "testPV3",
Path: "test/path3",
},
},
Name: "node2",
},
},
ThinPools: []v1alpha1.LvmVolumeGroupThinPoolStatus{
{
Name: "test-name",
ActualSize: *convertSize("1G", t),
UsedSize: resource.MustParse("500M"),
Ready: true,
Message: "",
},
},
VGSize: resource.MustParse("30G"),
VGUuid: "test-vg-uuid",
},
}

actual, err := json.Marshal(testObj)

if assert.NoError(t, err) {
assert.JSONEq(t, expected, string(actual))
}
})
}

func convertSize(size string, t *testing.T) *resource.Quantity {