From fe5d5d81aff627f86c5d9e2041395c383696ad21 Mon Sep 17 00:00:00 2001 From: TomerNewman Date: Wed, 3 Jul 2024 13:09:02 +0300 Subject: [PATCH] Shorten UpdateNodeLabelsAndRecordEvents function in nmc_reconciler.go Today, the UpdateNodeLabelsAndRecordEvents function has around 100 lines of code. This commit aims to enhance its readability and maintainability by using helper functions, thereby reducing its total length. Additionally, adding unit tests that covers the new helper functions. --- internal/controllers/mock_nmc_reconciler.go | 137 ++++- internal/controllers/nmc_reconciler.go | 156 ++++-- internal/controllers/nmc_reconciler_test.go | 558 +++++++++++++++++--- internal/node/mock_node.go | 14 + internal/node/node.go | 33 ++ internal/node/node_test.go | 91 +++- 6 files changed, 845 insertions(+), 144 deletions(-) diff --git a/internal/controllers/mock_nmc_reconciler.go b/internal/controllers/mock_nmc_reconciler.go index 6f82f97a1..328c5eb55 100644 --- a/internal/controllers/mock_nmc_reconciler.go +++ b/internal/controllers/mock_nmc_reconciler.go @@ -15,6 +15,8 @@ import ( v1beta1 "github.com/kubernetes-sigs/kernel-module-management/api/v1beta1" gomock "go.uber.org/mock/gomock" v1 "k8s.io/api/core/v1" + types "k8s.io/apimachinery/pkg/types" + sets "k8s.io/apimachinery/pkg/util/sets" client "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -83,6 +85,18 @@ func (mr *MocknmcReconcilerHelperMockRecorder) ProcessUnconfiguredModuleStatus(c return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProcessUnconfiguredModuleStatus", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).ProcessUnconfiguredModuleStatus), ctx, nmc, status) } +// RecordEvents mocks base method. +func (m *MocknmcReconcilerHelper) RecordEvents(node *v1.Node, loadedModules, unloadedModules []types.NamespacedName) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "RecordEvents", node, loadedModules, unloadedModules) +} + +// RecordEvents indicates an expected call of RecordEvents. +func (mr *MocknmcReconcilerHelperMockRecorder) RecordEvents(node, loadedModules, unloadedModules any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RecordEvents", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).RecordEvents), node, loadedModules, unloadedModules) +} + // RemovePodFinalizers mocks base method. func (m *MocknmcReconcilerHelper) RemovePodFinalizers(ctx context.Context, nodeName string) error { m.ctrl.T.Helper() @@ -111,18 +125,127 @@ func (mr *MocknmcReconcilerHelperMockRecorder) SyncStatus(ctx, nmc any) *gomock. return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SyncStatus", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).SyncStatus), ctx, nmc) } -// UpdateNodeLabelsAndRecordEvents mocks base method. -func (m *MocknmcReconcilerHelper) UpdateNodeLabelsAndRecordEvents(ctx context.Context, nmc *v1beta1.NodeModulesConfig) error { +// UpdateNodeLabels mocks base method. +func (m *MocknmcReconcilerHelper) UpdateNodeLabels(ctx context.Context, nmc *v1beta1.NodeModulesConfig, node *v1.Node) ([]types.NamespacedName, []types.NamespacedName, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "UpdateNodeLabelsAndRecordEvents", ctx, nmc) - ret0, _ := ret[0].(error) + ret := m.ctrl.Call(m, "UpdateNodeLabels", ctx, nmc, node) + ret0, _ := ret[0].([]types.NamespacedName) + ret1, _ := ret[1].([]types.NamespacedName) + ret2, _ := ret[2].(error) + return ret0, ret1, ret2 +} + +// UpdateNodeLabels indicates an expected call of UpdateNodeLabels. +func (mr *MocknmcReconcilerHelperMockRecorder) UpdateNodeLabels(ctx, nmc, node any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateNodeLabels", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).UpdateNodeLabels), ctx, nmc, node) +} + +// MocklabelPreparationHelper is a mock of labelPreparationHelper interface. +type MocklabelPreparationHelper struct { + ctrl *gomock.Controller + recorder *MocklabelPreparationHelperMockRecorder +} + +// MocklabelPreparationHelperMockRecorder is the mock recorder for MocklabelPreparationHelper. +type MocklabelPreparationHelperMockRecorder struct { + mock *MocklabelPreparationHelper +} + +// NewMocklabelPreparationHelper creates a new mock instance. +func NewMocklabelPreparationHelper(ctrl *gomock.Controller) *MocklabelPreparationHelper { + mock := &MocklabelPreparationHelper{ctrl: ctrl} + mock.recorder = &MocklabelPreparationHelperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MocklabelPreparationHelper) EXPECT() *MocklabelPreparationHelperMockRecorder { + return m.recorder +} + +// addEqualLabels mocks base method. +func (m *MocklabelPreparationHelper) addEqualLabels(nodeModuleReadyLabels sets.Set[types.NamespacedName], specLabels, statusLabels map[types.NamespacedName]v1beta1.ModuleConfig) []types.NamespacedName { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "addEqualLabels", nodeModuleReadyLabels, specLabels, statusLabels) + ret0, _ := ret[0].([]types.NamespacedName) + return ret0 +} + +// addEqualLabels indicates an expected call of addEqualLabels. +func (mr *MocklabelPreparationHelperMockRecorder) addEqualLabels(nodeModuleReadyLabels, specLabels, statusLabels any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "addEqualLabels", reflect.TypeOf((*MocklabelPreparationHelper)(nil).addEqualLabels), nodeModuleReadyLabels, specLabels, statusLabels) +} + +// getDeprecatedKernelModuleReadyLabels mocks base method. +func (m *MocklabelPreparationHelper) getDeprecatedKernelModuleReadyLabels(node v1.Node) sets.Set[string] { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "getDeprecatedKernelModuleReadyLabels", node) + ret0, _ := ret[0].(sets.Set[string]) + return ret0 +} + +// getDeprecatedKernelModuleReadyLabels indicates an expected call of getDeprecatedKernelModuleReadyLabels. +func (mr *MocklabelPreparationHelperMockRecorder) getDeprecatedKernelModuleReadyLabels(node any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getDeprecatedKernelModuleReadyLabels", reflect.TypeOf((*MocklabelPreparationHelper)(nil).getDeprecatedKernelModuleReadyLabels), node) +} + +// getNodeKernelModuleReadyLabels mocks base method. +func (m *MocklabelPreparationHelper) getNodeKernelModuleReadyLabels(node v1.Node) sets.Set[types.NamespacedName] { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "getNodeKernelModuleReadyLabels", node) + ret0, _ := ret[0].(sets.Set[types.NamespacedName]) + return ret0 +} + +// getNodeKernelModuleReadyLabels indicates an expected call of getNodeKernelModuleReadyLabels. +func (mr *MocklabelPreparationHelperMockRecorder) getNodeKernelModuleReadyLabels(node any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getNodeKernelModuleReadyLabels", reflect.TypeOf((*MocklabelPreparationHelper)(nil).getNodeKernelModuleReadyLabels), node) +} + +// getSpecLabelsAndTheirConfigs mocks base method. +func (m *MocklabelPreparationHelper) getSpecLabelsAndTheirConfigs(nmc *v1beta1.NodeModulesConfig) map[types.NamespacedName]v1beta1.ModuleConfig { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "getSpecLabelsAndTheirConfigs", nmc) + ret0, _ := ret[0].(map[types.NamespacedName]v1beta1.ModuleConfig) + return ret0 +} + +// getSpecLabelsAndTheirConfigs indicates an expected call of getSpecLabelsAndTheirConfigs. +func (mr *MocklabelPreparationHelperMockRecorder) getSpecLabelsAndTheirConfigs(nmc any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getSpecLabelsAndTheirConfigs", reflect.TypeOf((*MocklabelPreparationHelper)(nil).getSpecLabelsAndTheirConfigs), nmc) +} + +// getStatusLabelsAndTheirConfigs mocks base method. +func (m *MocklabelPreparationHelper) getStatusLabelsAndTheirConfigs(nmc *v1beta1.NodeModulesConfig) map[types.NamespacedName]v1beta1.ModuleConfig { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "getStatusLabelsAndTheirConfigs", nmc) + ret0, _ := ret[0].(map[types.NamespacedName]v1beta1.ModuleConfig) + return ret0 +} + +// getStatusLabelsAndTheirConfigs indicates an expected call of getStatusLabelsAndTheirConfigs. +func (mr *MocklabelPreparationHelperMockRecorder) getStatusLabelsAndTheirConfigs(nmc any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "getStatusLabelsAndTheirConfigs", reflect.TypeOf((*MocklabelPreparationHelper)(nil).getStatusLabelsAndTheirConfigs), nmc) +} + +// removeOrphanedLabels mocks base method. +func (m *MocklabelPreparationHelper) removeOrphanedLabels(nodeModuleReadyLabels sets.Set[types.NamespacedName], specLabels, statusLabels map[types.NamespacedName]v1beta1.ModuleConfig) []types.NamespacedName { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "removeOrphanedLabels", nodeModuleReadyLabels, specLabels, statusLabels) + ret0, _ := ret[0].([]types.NamespacedName) return ret0 } -// UpdateNodeLabelsAndRecordEvents indicates an expected call of UpdateNodeLabelsAndRecordEvents. -func (mr *MocknmcReconcilerHelperMockRecorder) UpdateNodeLabelsAndRecordEvents(ctx, nmc any) *gomock.Call { +// removeOrphanedLabels indicates an expected call of removeOrphanedLabels. +func (mr *MocklabelPreparationHelperMockRecorder) removeOrphanedLabels(nodeModuleReadyLabels, specLabels, statusLabels any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateNodeLabelsAndRecordEvents", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).UpdateNodeLabelsAndRecordEvents), ctx, nmc) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "removeOrphanedLabels", reflect.TypeOf((*MocklabelPreparationHelper)(nil).removeOrphanedLabels), nodeModuleReadyLabels, specLabels, statusLabels) } // MockpodManager is a mock of podManager interface. diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index 9e89107f2..0664fcd6c 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -153,9 +153,15 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r if err := r.helper.GarbageCollectInUseLabels(ctx, &nmcObj); err != nil { errs = append(errs, fmt.Errorf("failed to GC in-use labels for NMC %s: %v", req.NamespacedName, err)) } + node := v1.Node{} + if err := r.client.Get(ctx, types.NamespacedName{Name: nmcObj.Name}, &node); err != nil { + return ctrl.Result{}, fmt.Errorf("could not get node %s: %v", nmcObj.Name, err) + } - if err := r.helper.UpdateNodeLabelsAndRecordEvents(ctx, &nmcObj); err != nil { + if loaded, unloaded, err := r.helper.UpdateNodeLabels(ctx, &nmcObj, &node); err != nil { errs = append(errs, fmt.Errorf("could not update node's labels for NMC %s: %v", req.NamespacedName, err)) + } else { + r.helper.RecordEvents(&node, loaded, unloaded) } return ctrl.Result{}, errors.Join(errs...) @@ -213,7 +219,8 @@ type nmcReconcilerHelper interface { ProcessUnconfiguredModuleStatus(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, status *kmmv1beta1.NodeModuleStatus) error RemovePodFinalizers(ctx context.Context, nodeName string) error SyncStatus(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) error - UpdateNodeLabelsAndRecordEvents(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) error + UpdateNodeLabels(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, node *v1.Node) ([]types.NamespacedName, []types.NamespacedName, error) + RecordEvents(node *v1.Node, loadedModules, unloadedModules []types.NamespacedName) } type nmcReconcilerHelperImpl struct { @@ -221,6 +228,7 @@ type nmcReconcilerHelperImpl struct { pm podManager recorder record.EventRecorder nodeAPI node.Node + lph labelPreparationHelper } func newNMCReconcilerHelper(client client.Client, pm podManager, recorder record.EventRecorder, nodeAPI node.Node) nmcReconcilerHelper { @@ -229,6 +237,7 @@ func newNMCReconcilerHelper(client client.Client, pm podManager, recorder record pm: pm, recorder: recorder, nodeAPI: nodeAPI, + lph: newLabelPreparationHelper(), } } @@ -555,104 +564,147 @@ func (h *nmcReconcilerHelperImpl) SyncStatus(ctx context.Context, nmcObj *kmmv1b return errors.Join(errs...) } -func (h *nmcReconcilerHelperImpl) UpdateNodeLabelsAndRecordEvents(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) error { - node := v1.Node{} - if err := h.client.Get(ctx, types.NamespacedName{Name: nmc.Name}, &node); err != nil { - return fmt.Errorf("could not get node %s: %v", nmc.Name, err) - } +type labelPreparationHelper interface { + getDeprecatedKernelModuleReadyLabels(node v1.Node) sets.Set[string] + getNodeKernelModuleReadyLabels(node v1.Node) sets.Set[types.NamespacedName] + getSpecLabelsAndTheirConfigs(nmc *kmmv1beta1.NodeModulesConfig) map[types.NamespacedName]kmmv1beta1.ModuleConfig + getStatusLabelsAndTheirConfigs(nmc *kmmv1beta1.NodeModulesConfig) map[types.NamespacedName]kmmv1beta1.ModuleConfig + addEqualLabels(nodeModuleReadyLabels sets.Set[types.NamespacedName], + specLabels, statusLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig) []types.NamespacedName + removeOrphanedLabels(nodeModuleReadyLabels sets.Set[types.NamespacedName], + specLabels, statusLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig) []types.NamespacedName +} +type labelPreparationHelperImpl struct{} - // get all the kernel module ready labels of the node +func newLabelPreparationHelper() labelPreparationHelper { + return &labelPreparationHelperImpl{} +} + +func (lph *labelPreparationHelperImpl) getNodeKernelModuleReadyLabels(node v1.Node) sets.Set[types.NamespacedName] { nodeModuleReadyLabels := sets.New[types.NamespacedName]() - deprecatedNodeModuleReadyLabels := sets.New[string]() for label := range node.GetLabels() { if ok, namespace, name := utils.IsKernelModuleReadyNodeLabel(label); ok { nodeModuleReadyLabels.Insert(types.NamespacedName{Namespace: namespace, Name: name}) } + } + return nodeModuleReadyLabels +} +func (lph *labelPreparationHelperImpl) getDeprecatedKernelModuleReadyLabels(node v1.Node) sets.Set[string] { + deprecatedNodeModuleReadyLabels := sets.New[string]() + + for label := range node.GetLabels() { if utils.IsDeprecatedKernelModuleReadyNodeLabel(label) { deprecatedNodeModuleReadyLabels.Insert(label) } } + return deprecatedNodeModuleReadyLabels +} - // get spec labels and their config +func (lph *labelPreparationHelperImpl) getSpecLabelsAndTheirConfigs(nmc *kmmv1beta1.NodeModulesConfig) map[types.NamespacedName]kmmv1beta1.ModuleConfig { specLabels := make(map[types.NamespacedName]kmmv1beta1.ModuleConfig) + for _, module := range nmc.Spec.Modules { specLabels[types.NamespacedName{Namespace: module.Namespace, Name: module.Name}] = module.Config } + return specLabels +} - // get status labels and their config +func (lph *labelPreparationHelperImpl) getStatusLabelsAndTheirConfigs(nmc *kmmv1beta1.NodeModulesConfig) map[types.NamespacedName]kmmv1beta1.ModuleConfig { statusLabels := make(map[types.NamespacedName]kmmv1beta1.ModuleConfig) + for _, module := range nmc.Status.Modules { label := types.NamespacedName{Namespace: module.Namespace, Name: module.Name} statusLabels[label] = module.Config } + return statusLabels +} - unloaded := make([]types.NamespacedName, 0, len(nodeModuleReadyLabels)) - loaded := make([]types.NamespacedName, 0, len(specLabels)) +func (h *nmcReconcilerHelperImpl) UpdateNodeLabels(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, node *v1.Node) ([]types.NamespacedName, []types.NamespacedName, error) { - patchFrom := client.MergeFrom(node.DeepCopy()) + // get all the kernel module ready labels of the node + nodeModuleReadyLabels := h.lph.getNodeKernelModuleReadyLabels(*node) + deprecatedNodeModuleReadyLabels := h.lph.getDeprecatedKernelModuleReadyLabels(*node) - // label in node but not in spec or status - should be removed - for nsn := range nodeModuleReadyLabels { - _, inSpec := specLabels[nsn] - _, inStatus := statusLabels[nsn] - if !inSpec && !inStatus { - meta.RemoveLabel( - &node, - utils.GetKernelModuleReadyNodeLabel(nsn.Namespace, nsn.Name), - ) + // get spec labels and their config + specLabels := h.lph.getSpecLabelsAndTheirConfigs(nmc) - unloaded = append(unloaded, nsn) - } - } + // get status labels and their config + statusLabels := h.lph.getStatusLabelsAndTheirConfigs(nmc) - // v1 ready labels, deprecated - should be removed - for label := range deprecatedNodeModuleReadyLabels { - meta.RemoveLabel(&node, label) - } + // label in node but not in spec or status - should be removed + nsnLabelsToBeRemoved := h.lph.removeOrphanedLabels(nodeModuleReadyLabels, specLabels, statusLabels) // label in spec and status and config equal - should be added - for nsn, specConfig := range specLabels { - statusConfig, ok := statusLabels[nsn] - if ok && reflect.DeepEqual(specConfig, statusConfig) && !nodeModuleReadyLabels.Has(nsn) { - meta.SetLabel( - &node, - utils.GetKernelModuleReadyNodeLabel(nsn.Namespace, nsn.Name), - "", - ) + nsnLabelsToBeLoaded := h.lph.addEqualLabels(nodeModuleReadyLabels, specLabels, statusLabels) - loaded = append(loaded, nsn) - } + var loadedLabels []string + unloadedLabels := deprecatedNodeModuleReadyLabels.UnsortedList() + + for _, label := range nsnLabelsToBeRemoved { + unloadedLabels = append(unloadedLabels, utils.GetKernelModuleReadyNodeLabel(label.Namespace, label.Name)) + } + for _, label := range nsnLabelsToBeLoaded { + loadedLabels = append(loadedLabels, utils.GetKernelModuleReadyNodeLabel(label.Namespace, label.Name)) } - if err := h.client.Patch(ctx, &node, patchFrom); err != nil { - return fmt.Errorf("could not patch node: %v", err) + if err := h.nodeAPI.UpdateLabels(ctx, node, loadedLabels, unloadedLabels); err != nil { + return nil, nil, fmt.Errorf("could not update labels on the node: %v", err) } - for _, nsn := range unloaded { + return nsnLabelsToBeLoaded, nsnLabelsToBeRemoved, nil +} + +func (h *nmcReconcilerHelperImpl) RecordEvents(node *v1.Node, loadedModules, unloadedModules []types.NamespacedName) { + for _, nsn := range loadedModules { h.recorder.AnnotatedEventf( - &node, + node, map[string]string{"module": nsn.String()}, v1.EventTypeNormal, - "ModuleUnloaded", - "Module %s unloaded from the kernel", + "ModuleLoaded", + "Module %s loaded into the kernel", nsn.String(), ) } - - for _, nsn := range loaded { + for _, nsn := range unloadedModules { h.recorder.AnnotatedEventf( - &node, + node, map[string]string{"module": nsn.String()}, v1.EventTypeNormal, - "ModuleLoaded", - "Module %s loaded into the kernel", + "ModuleUnloaded", + "Module %s unloaded from the kernel", nsn.String(), ) } +} - return nil +func (lph *labelPreparationHelperImpl) removeOrphanedLabels(nodeModuleReadyLabels sets.Set[types.NamespacedName], + specLabels, statusLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig) []types.NamespacedName { + + unloaded := make([]types.NamespacedName, 0, len(nodeModuleReadyLabels)) + + for nsn := range nodeModuleReadyLabels { + _, inSpec := specLabels[nsn] + _, inStatus := statusLabels[nsn] + if !inSpec && !inStatus { + unloaded = append(unloaded, nsn) + } + } + return unloaded +} +func (lph *labelPreparationHelperImpl) addEqualLabels(nodeModuleReadyLabels sets.Set[types.NamespacedName], + specLabels, statusLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig) []types.NamespacedName { + + loaded := make([]types.NamespacedName, 0, len(nodeModuleReadyLabels)) + + for nsn, specConfig := range specLabels { + statusConfig, ok := statusLabels[nsn] + if ok && reflect.DeepEqual(specConfig, statusConfig) && !nodeModuleReadyLabels.Has(nsn) { + loaded = append(loaded, nsn) + } + } + return loaded } const ( diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index a973082e1..6a3ec746c 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "github.com/kubernetes-sigs/kernel-module-management/internal/node" + "k8s.io/apimachinery/pkg/util/sets" "path/filepath" "reflect" "strings" @@ -36,7 +37,15 @@ import ( "sigs.k8s.io/controller-runtime/pkg/reconcile" ) -const nmcName = "nmc" +const ( + nmcName = "nmc" + nsFirst = "example-ns-1" + nsSecond = "example-ns-2" + nameFirst = "example-name-1" + nameSecond = "example-name-2" + imageFirst = "example-image-1" + imageSecond = "example-image-2" +) var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { var ( @@ -101,6 +110,12 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { mod1Name = "mod1" mod2Name = "mod2" ) + var ( + loaded []types.NamespacedName + unloaded []types.NamespacedName + err error + node v1.Node + ) spec0 := kmmv1beta1.NodeModuleSpec{ ModuleItem: kmmv1beta1.ModuleItem{ Namespace: namespace, @@ -155,7 +170,9 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec1, nil), wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2), wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc), - wh.EXPECT().UpdateNodeLabelsAndRecordEvents(ctx, nmc), + kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &node).Return(nil), + wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(loaded, unloaded, err), + wh.EXPECT().RecordEvents(&node, loaded, unloaded), ) Expect( @@ -171,6 +188,11 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { mod1Name = "mod1" mod2Name = "mod2" ) + var ( + node v1.Node + err error + ) + spec0 := kmmv1beta1.NodeModuleSpec{ ModuleItem: kmmv1beta1.ModuleItem{ Namespace: namespace, @@ -217,10 +239,11 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0).Return(fmt.Errorf("some error")), wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2).Return(fmt.Errorf("some error")), wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc).Return(fmt.Errorf("some error")), - wh.EXPECT().UpdateNodeLabelsAndRecordEvents(ctx, nmc).Return(fmt.Errorf("some error")), + kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &node).Return(nil), + wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(nil, nil, fmt.Errorf("some error")), ) - _, err := r.Reconcile(ctx, req) + _, err = r.Reconcile(ctx, req) Expect(err).ToNot(BeNil()) }) }) @@ -1158,39 +1181,148 @@ var _ = Describe("nmcReconcilerHelperImpl_RemovePodFinalizers", func() { const ( moduleName = "my-module" moduleNamespace = "my-module-namespace" + nodeName = "node-name" ) -var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func() { +var kernelModuleLabelName = utils.GetKernelModuleReadyNodeLabel(moduleNamespace, moduleName) + +var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabels", func() { var ( - ctx = context.TODO() - client *testclient.MockClient - expectedNode v1.Node - fakeRecorder *record.FakeRecorder - nmc kmmv1beta1.NodeModulesConfig - wh nmcReconcilerHelper + ctx context.Context + client *testclient.MockClient + fakeRecorder *record.FakeRecorder + n *node.MockNode + nmc kmmv1beta1.NodeModulesConfig + wh nmcReconcilerHelper + mlph *MocklabelPreparationHelper + firstLabelName string + secondLabelName string ) BeforeEach(func() { + ctx = context.TODO() ctrl := gomock.NewController(GinkgoT()) client = testclient.NewMockClient(ctrl) - expectedNode = v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Name: "node name", - Labels: map[string]string{}, - }, - } nmc = kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: "nmcName"}, + Spec: kmmv1beta1.NodeModulesConfigSpec{ + Modules: []kmmv1beta1.NodeModuleSpec{ + { + ModuleItem: kmmv1beta1.ModuleItem{ + Namespace: nsFirst, + Name: nameFirst, + }, + Config: kmmv1beta1.ModuleConfig{}, + }, + { + ModuleItem: kmmv1beta1.ModuleItem{ + Namespace: nsSecond, + Name: nameSecond, + }, + Config: kmmv1beta1.ModuleConfig{}, + }, + }, + }, } fakeRecorder = record.NewFakeRecorder(10) - wh = newNMCReconcilerHelper(client, nil, fakeRecorder, nil) + n = node.NewMockNode(ctrl) + wh = newNMCReconcilerHelper(client, nil, fakeRecorder, n) + mlph = NewMocklabelPreparationHelper(ctrl) + wh = &nmcReconcilerHelperImpl{ + client: client, + pm: nil, + recorder: fakeRecorder, + nodeAPI: n, + lph: mlph, + } + firstLabelName = fmt.Sprintf("kmm.node.kubernetes.io/%s.%s.ready", nsFirst, nameFirst) + secondLabelName = fmt.Sprintf("kmm.node.kubernetes.io/%s.%s.ready", nsSecond, nameSecond) }) - moduleConfig := kmmv1beta1.ModuleConfig{ - KernelVersion: "some version", - ContainerImage: "some image", - InTreeModulesToRemove: []string{"some kernel module"}, - } + It("failed to get node", func() { + client.EXPECT().Get(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) + err := client.Get(ctx, types.NamespacedName{Name: nmc.Name}, &v1.Node{}) + Expect(err).To(HaveOccurred()) + }) + It("Should fail patching node after change in labels", func() { + node := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + firstLabelName: "", + }, + Name: nodeName, + }, + } + emptySet := sets.Set[types.NamespacedName]{} + emptyMap := map[types.NamespacedName]kmmv1beta1.ModuleConfig{} + + gomock.InOrder( + mlph.EXPECT().getNodeKernelModuleReadyLabels(node).Return(emptySet), + mlph.EXPECT().getDeprecatedKernelModuleReadyLabels(node).Return(sets.Set[string]{}), + mlph.EXPECT().getSpecLabelsAndTheirConfigs(&nmc).Return(emptyMap), + mlph.EXPECT().getStatusLabelsAndTheirConfigs(&nmc).Return(emptyMap), + mlph.EXPECT().removeOrphanedLabels(emptySet, emptyMap, emptyMap).Return([]types.NamespacedName{{Name: nameSecond, Namespace: nsSecond}}), + mlph.EXPECT().addEqualLabels(emptySet, emptyMap, emptyMap).Return([]types.NamespacedName{{Name: nameFirst, Namespace: nsFirst}}), + n.EXPECT(). + UpdateLabels( + ctx, + &node, + []string{firstLabelName}, + []string{secondLabelName}, + ).Return(fmt.Errorf("some error")), + ) + _, _, err := wh.UpdateNodeLabels(ctx, &nmc, &node) + Expect(err).To(HaveOccurred()) + }) + It("Should work as expected", func() { + node := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + firstLabelName: "", + }, + Name: nodeName, + }, + } + emptySet := sets.Set[types.NamespacedName]{} + emptyMap := map[types.NamespacedName]kmmv1beta1.ModuleConfig{} + + gomock.InOrder( + mlph.EXPECT().getNodeKernelModuleReadyLabels(node).Return(emptySet), + mlph.EXPECT().getDeprecatedKernelModuleReadyLabels(node).Return(sets.Set[string]{}), + mlph.EXPECT().getSpecLabelsAndTheirConfigs(&nmc).Return(emptyMap), + mlph.EXPECT().getStatusLabelsAndTheirConfigs(&nmc).Return(emptyMap), + mlph.EXPECT().removeOrphanedLabels(emptySet, emptyMap, emptyMap).Return([]types.NamespacedName{{Name: nameSecond, Namespace: nsSecond}}), + mlph.EXPECT().addEqualLabels(emptySet, emptyMap, emptyMap).Return([]types.NamespacedName{{Name: nameFirst, Namespace: nsFirst}}), + n.EXPECT(). + UpdateLabels( + ctx, + &node, + []string{firstLabelName}, + []string{secondLabelName}, + ).Return(nil), + ) + _, _, err := wh.UpdateNodeLabels(ctx, &nmc, &node) + Expect(err).ToNot(HaveOccurred()) + Expect(node.Labels).To(HaveKey(firstLabelName)) + + }) +}) + +var _ = Describe("nmcReconcilerHelperImpl_RecordEvents", func() { + var ( + client *testclient.MockClient + fakeRecorder *record.FakeRecorder + n node.Node + wh nmcReconcilerHelper + ) + + BeforeEach(func() { + ctrl := gomock.NewController(GinkgoT()) + client = testclient.NewMockClient(ctrl) + n = node.NewNode(client) + fakeRecorder = record.NewFakeRecorder(10) + wh = newNMCReconcilerHelper(client, nil, fakeRecorder, n) + }) closeAndGetAllEvents := func(events chan string) []string { elems := make([]string, 0) @@ -1206,13 +1338,6 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func _ = closeAndGetAllEvents(make(chan string)) - It("failed to get node", func() { - client.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) - - err := wh.UpdateNodeLabelsAndRecordEvents(ctx, &nmc) - Expect(err).To(HaveOccurred()) - }) - type testCase struct { nodeLabelPresent bool specPresent bool @@ -1225,61 +1350,10 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func } DescribeTable( - "nodes labels scenarios", - func(tc testCase) { - nodeLabels := map[string]string{"kmm.node.kubernetes.io/deprecated-label.ready": ""} - - if tc.nodeLabelPresent { - nodeLabels[utils.GetKernelModuleReadyNodeLabel(moduleNamespace, moduleName)] = "" - } - if tc.specPresent { - nmc.Spec.Modules = []kmmv1beta1.NodeModuleSpec{ - { - ModuleItem: kmmv1beta1.ModuleItem{ - Name: moduleName, - Namespace: moduleNamespace, - }, - Config: moduleConfig, - }, - } - } - if tc.statusPresent { - nmc.Status.Modules = []kmmv1beta1.NodeModuleStatus{ - { - ModuleItem: kmmv1beta1.ModuleItem{ - Name: moduleName, - Namespace: moduleNamespace, - }, - }, - } - if tc.statusConfigPresent { - statusConfig := moduleConfig - if !tc.configsEqual { - statusConfig.ContainerImage = "some other container image" - } - nmc.Status.Modules[0].Config = statusConfig - } - } - - if tc.resultLabelPresent { - resultLabels := map[string]string{utils.GetKernelModuleReadyNodeLabel(moduleNamespace, moduleName): ""} - expectedNode.SetLabels(resultLabels) - } - - gomock.InOrder( - client.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).DoAndReturn( - func(_ interface{}, _ interface{}, node *v1.Node, _ ...ctrlclient.GetOption) error { - node.SetName("node name") - node.SetLabels(nodeLabels) - return nil - }, - ), - client.EXPECT().Patch(ctx, &expectedNode, gomock.Any()).Return(nil), - ) - - err := wh.UpdateNodeLabelsAndRecordEvents(ctx, &nmc) - Expect(err).NotTo(HaveOccurred()) + "RecordEvents different scenarios", + func(tc testCase, loaded []types.NamespacedName, unloaded []types.NamespacedName, node v1.Node) { + wh.RecordEvents(&node, loaded, unloaded) events := closeAndGetAllEvents(fakeRecorder.Events) if !tc.addsReadyLabel && !tc.removesReadyLabel { @@ -1303,6 +1377,14 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func nodeLabelPresent: true, removesReadyLabel: true, }, + []types.NamespacedName{}, + []types.NamespacedName{{Namespace: moduleNamespace, Name: moduleName}}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + Name: nodeName, + }, + }, ), Entry( "node label present, spec present, status missing", @@ -1311,6 +1393,16 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func specPresent: true, resultLabelPresent: true, }, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + kernelModuleLabelName: "", + }, + Name: nodeName, + }, + }, ), Entry( "node label present, spec present, status present, status config missing", @@ -1320,6 +1412,16 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func statusPresent: true, resultLabelPresent: true, }, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + kernelModuleLabelName: "", + }, + Name: nodeName, + }, + }, ), Entry( "node label present, spec present, status present, status config present, configs not equal", @@ -1330,6 +1432,16 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func statusConfigPresent: true, resultLabelPresent: true, }, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + kernelModuleLabelName: "", + }, + Name: nodeName, + }, + }, ), Entry( "node label present, spec present, status present, status config present, configs equal", @@ -1341,14 +1453,40 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func configsEqual: true, resultLabelPresent: true, }, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + kernelModuleLabelName: "", + }, + Name: nodeName, + }, + }, ), Entry( "node label missing, spec missing, status missing", testCase{}, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + Name: nodeName, + }, + }, ), Entry( "node label missing, spec present, status missing", testCase{specPresent: true}, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + Name: nodeName, + }, + }, ), Entry( "node label missing, spec present, status present, status config missing", @@ -1356,6 +1494,14 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func specPresent: true, statusPresent: true, }, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + Name: nodeName, + }, + }, ), Entry( "node label missing, spec present, status present, status config present, configs not equal", @@ -1364,6 +1510,14 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func statusPresent: true, statusConfigPresent: true, }, + []types.NamespacedName{}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + Name: nodeName, + }, + }, ), Entry( "node label missing, spec present, status present, status config present, configs equal", @@ -1375,6 +1529,16 @@ var _ = Describe("nmcReconcilerHelperImpl_UpdateNodeLabelsAndRecordEvents", func resultLabelPresent: true, addsReadyLabel: true, }, + []types.NamespacedName{{Namespace: moduleNamespace, Name: moduleName}}, + []types.NamespacedName{}, + v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + kernelModuleLabelName: "", + }, + Name: nodeName, + }, + }, ), ) }) @@ -1905,3 +2069,229 @@ var _ = Describe("pullSecretHelperImpl_VolumesAndVolumeMounts", func() { Expect(resVolMounts).To(BeComparableTo(volMounts)) }) }) + +var _ = Describe("getKernelModuleReadyLabels", func() { + lph := newLabelPreparationHelper() + + DescribeTable("getKernelModuleReadyLabels different scenarios", func(labels map[string]string, + nodeModuleReadyLabelsEqual sets.Set[types.NamespacedName]) { + node := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: labels, + }, + } + + nodeModuleReadyLabels := lph.getNodeKernelModuleReadyLabels(node) + + Expect(nodeModuleReadyLabels).To(Equal(nodeModuleReadyLabelsEqual)) + }, + Entry("Should be empty", map[string]string{}, + sets.Set[types.NamespacedName]{}, + ), + + Entry("nodeModuleReadyLabels found", map[string]string{"invalid": ""}, + sets.Set[types.NamespacedName]{}, + ), + + Entry("nodeModuleReadyLabels found", map[string]string{fmt.Sprintf("kmm.node.kubernetes.io/%s.%s.ready", nsFirst, nameFirst): ""}, + sets.Set[types.NamespacedName]{ + {Namespace: nsFirst, Name: nameFirst}: {}, + }, + ), + ) +}) + +var _ = Describe("getDeprecatedKernelModuleReadyLabels", func() { + + lph := newLabelPreparationHelper() + DescribeTable("getDeprecatedKernelModuleReadyLabels different scenarios", func(labels map[string]string, + deprecatedNodeModuleReadyLabelsEqual sets.Set[string]) { + node := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: labels, + }, + } + + deprecatedNodeModuleReadyLabels := lph.getDeprecatedKernelModuleReadyLabels(node) + + Expect(deprecatedNodeModuleReadyLabels).To(Equal(deprecatedNodeModuleReadyLabelsEqual)) + }, + Entry("Should be empty", map[string]string{}, + sets.Set[string]{}, + ), + + Entry("deprecated node module ready labels not found", map[string]string{"invalid": ""}, + sets.Set[string]{}, + ), + + Entry("deprecated node module ready labels found", map[string]string{fmt.Sprintf("kmm.node.kubernetes.io/%s.ready", nameFirst): ""}, + sets.Set[string]{ + fmt.Sprintf("kmm.node.kubernetes.io/%s.ready", nameFirst): {}, + }, + ), + ) +}) + +var _ = Describe("getSpecLabelsAndTheirConfigs", func() { + + var ( + nmc kmmv1beta1.NodeModulesConfig + lph labelPreparationHelper + ) + + BeforeEach(func() { + nmc = kmmv1beta1.NodeModulesConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "nmcName"}, + Spec: kmmv1beta1.NodeModulesConfigSpec{ + Modules: []kmmv1beta1.NodeModuleSpec{ + { + ModuleItem: kmmv1beta1.ModuleItem{ + Namespace: nsFirst, + Name: nameFirst, + }, + Config: kmmv1beta1.ModuleConfig{ContainerImage: imageFirst}, + }, + }, + }, + } + lph = newLabelPreparationHelper() + }) + It("Should not have module not from nmc", func() { + specLabels := lph.getSpecLabelsAndTheirConfigs(&nmc) + Expect(specLabels).ToNot(HaveKey(types.NamespacedName{Namespace: nsSecond, Name: nameSecond})) + }) + + It("Should have module from nmc", func() { + specLabels := lph.getSpecLabelsAndTheirConfigs(&nmc) + Expect(specLabels).To(HaveKeyWithValue(types.NamespacedName{Namespace: nsFirst, Name: nameFirst}, kmmv1beta1.ModuleConfig{ContainerImage: imageFirst})) + }) +}) + +var _ = Describe("getStatusLabelsAndTheirConfigs", func() { + + var ( + nmc kmmv1beta1.NodeModulesConfig + lph labelPreparationHelper + ) + + BeforeEach(func() { + nmc = kmmv1beta1.NodeModulesConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "nmcName"}, + Status: kmmv1beta1.NodeModulesConfigStatus{ + Modules: []kmmv1beta1.NodeModuleStatus{ + { + ModuleItem: kmmv1beta1.ModuleItem{ + Namespace: nsFirst, + Name: nameFirst, + }, + Config: kmmv1beta1.ModuleConfig{ContainerImage: imageFirst}, + }, + }, + }, + } + lph = newLabelPreparationHelper() + }) + It("Should not have module not from nmc", func() { + statusLabels := lph.getStatusLabelsAndTheirConfigs(&nmc) + Expect(statusLabels).To(HaveKeyWithValue(types.NamespacedName{Namespace: nsFirst, Name: nameFirst}, kmmv1beta1.ModuleConfig{ContainerImage: imageFirst})) + }) + + It("Should have module from nmc", func() { + statusLabels := lph.getStatusLabelsAndTheirConfigs(&nmc) + Expect(statusLabels).ToNot(HaveKey(types.NamespacedName{Namespace: nsSecond, Name: nameSecond})) + }) + +}) + +var _ = Describe("removeOrphanedLabels", func() { + + var lph = labelPreparationHelperImpl{} + + DescribeTable("removeOrphanedLabels different scenarios", func(nodeModuleReadyLabels sets.Set[types.NamespacedName], + specLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig, + statusLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig, + node v1.Node, + expectedUnloaded []types.NamespacedName, + ) { + unloaded := lph.removeOrphanedLabels(nodeModuleReadyLabels, specLabels, statusLabels) + Expect(unloaded).To(Equal(expectedUnloaded)) + }, + Entry("Empty spec and status labels, should result of empty unloaded variable", + sets.Set[types.NamespacedName]{}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{}, + v1.Node{}, + []types.NamespacedName{}), + Entry("ModuleConfig obj exists in specLabels so it should not be in unloaded variable", + sets.Set[types.NamespacedName]{ + {Namespace: nsFirst, Name: nameFirst}: {}, + {Namespace: nsSecond, Name: nameSecond}: {}, + }, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {}}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{}, + v1.Node{}, + []types.NamespacedName{{Namespace: nsSecond, Name: nameSecond}}), + + Entry("ModuleConfig obj exists in statusLabels so it should not be in unloaded variable", + sets.Set[types.NamespacedName]{ + {Namespace: nsFirst, Name: nameFirst}: {}, + {Namespace: nsSecond, Name: nameSecond}: {}, + }, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {}}, + v1.Node{}, + []types.NamespacedName{{Namespace: nsSecond, Name: nameSecond}}), + + Entry("Both ModuleConfig obj exist in specLabels or statusLabels so they should not be in unloaded variable", + sets.Set[types.NamespacedName]{ + {Namespace: nsFirst, Name: nameFirst}: {}, + {Namespace: nsSecond, Name: nameSecond}: {}, + }, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {}}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsSecond, Name: nameSecond}: {}}, + v1.Node{}, + []types.NamespacedName{})) +}) + +var _ = Describe("addEqualLabels", func() { + var lph = labelPreparationHelperImpl{} + + DescribeTable("addEqualLabels different scenarios", func(nodeModuleReadyLabels sets.Set[types.NamespacedName], specLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig, + statusLabels map[types.NamespacedName]kmmv1beta1.ModuleConfig, + node v1.Node, + expectedUnloaded []types.NamespacedName, + ) { + loaded := lph.addEqualLabels(nodeModuleReadyLabels, specLabels, statusLabels) + Expect(loaded).To(Equal(expectedUnloaded)) + }, + Entry("Empty spec and status labels, should result of empty loaded variable", + sets.Set[types.NamespacedName]{}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{}, + v1.Node{}, + []types.NamespacedName{}), + Entry("specConfig and statusConfig are equal and nsn is not in nodeModuleReadyLabels, so nsn should be returned", + sets.Set[types.NamespacedName]{ + {Namespace: nsSecond, Name: nameSecond}: {}, + }, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {ContainerImage: imageFirst}}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {ContainerImage: imageFirst}}, + v1.Node{}, + []types.NamespacedName{{Namespace: nsFirst, Name: nameFirst}}), + Entry("specConfig and statusConfig aren't equal so nsn shouldn't not be returned", + sets.Set[types.NamespacedName]{ + {Namespace: nsSecond, Name: nameSecond}: {}, + }, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {ContainerImage: imageFirst}}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {ContainerImage: imageSecond}}, + v1.Node{}, + []types.NamespacedName{}), + Entry("nsn is in nodeModuleReadyLabels so nsn should not be returned", + sets.Set[types.NamespacedName]{ + {Namespace: nsFirst, Name: nameFirst}: {}, + }, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {ContainerImage: imageFirst}}, + map[types.NamespacedName]kmmv1beta1.ModuleConfig{{Namespace: nsFirst, Name: nameFirst}: {ContainerImage: imageFirst}}, + v1.Node{}, + []types.NamespacedName{})) +}) diff --git a/internal/node/mock_node.go b/internal/node/mock_node.go index 0db5f207e..74308443e 100644 --- a/internal/node/mock_node.go +++ b/internal/node/mock_node.go @@ -96,3 +96,17 @@ func (mr *MockNodeMockRecorder) IsNodeSchedulable(node any) *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsNodeSchedulable", reflect.TypeOf((*MockNode)(nil).IsNodeSchedulable), node) } + +// UpdateLabels mocks base method. +func (m *MockNode) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "UpdateLabels", ctx, node, toBeAdded, toBeRemoved) + ret0, _ := ret[0].(error) + return ret0 +} + +// UpdateLabels indicates an expected call of UpdateLabels. +func (mr *MockNodeMockRecorder) UpdateLabels(ctx, node, toBeAdded, toBeRemoved any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "UpdateLabels", reflect.TypeOf((*MockNode)(nil).UpdateLabels), ctx, node, toBeAdded, toBeRemoved) +} diff --git a/internal/node/node.go b/internal/node/node.go index 3b6bfdf4b..8e959631d 100644 --- a/internal/node/node.go +++ b/internal/node/node.go @@ -3,6 +3,7 @@ package node import ( "context" "fmt" + "github.com/kubernetes-sigs/kernel-module-management/internal/meta" v1 "k8s.io/api/core/v1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -13,6 +14,7 @@ type Node interface { GetNodesListBySelector(ctx context.Context, selector map[string]string) ([]v1.Node, error) GetNumTargetedNodes(ctx context.Context, selector map[string]string) (int, error) FindNodeCondition(cond []v1.NodeCondition, conditionType v1.NodeConditionType) *v1.NodeCondition + UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error } type node struct { @@ -72,3 +74,34 @@ func (n *node) FindNodeCondition(cond []v1.NodeCondition, conditionType v1.NodeC return nil } + +func (n *node) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error { + patchFrom := client.MergeFrom(node.DeepCopy()) + + addLabels(node, toBeAdded) + removeLabels(node, toBeRemoved) + + if err := n.client.Patch(ctx, node, patchFrom); err != nil { + return fmt.Errorf("could not patch node: %v", err) + } + return nil +} + +func addLabels(node *v1.Node, labels []string) { + for _, label := range labels { + meta.SetLabel( + node, + label, + "", + ) + } +} + +func removeLabels(node *v1.Node, labels []string) { + for _, label := range labels { + meta.RemoveLabel( + node, + label, + ) + } +} diff --git a/internal/node/node_test.go b/internal/node/node_test.go index a6e51f72c..ab4a060a3 100644 --- a/internal/node/node_test.go +++ b/internal/node/node_test.go @@ -4,10 +4,12 @@ import ( "context" "fmt" "github.com/kubernetes-sigs/kernel-module-management/internal/client" + "github.com/kubernetes-sigs/kernel-module-management/internal/utils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" "go.uber.org/mock/gomock" v1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) var _ = Describe("IsNodeSchedulable", func() { @@ -118,6 +120,59 @@ var _ = Describe("GetNodesListBySelector", func() { }) }) +var ( + loadedKernelModuleReadyNodeLabel = utils.GetKernelModuleReadyNodeLabel("loaded-ns", "loaded-n") + unloadedKernelModuleReadyNodeLabel = utils.GetKernelModuleReadyNodeLabel("unloaded-ns", "unloaded-n") +) + +var _ = Describe("UpdateLabels", func() { + var ( + ctrl *gomock.Controller + n Node + ctx context.Context + clnt *client.MockClient + ) + + BeforeEach(func() { + ctrl = gomock.NewController(GinkgoT()) + clnt = client.NewMockClient(ctrl) + ctx = context.TODO() + n = NewNode(clnt) + }) + + It("Should work as expected", func() { + node := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + }, + } + loaded := []string{loadedKernelModuleReadyNodeLabel} + unloaded := []string{unloadedKernelModuleReadyNodeLabel} + + clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) + + err := n.UpdateLabels(ctx, &node, loaded, unloaded) + Expect(err).ToNot(HaveOccurred()) + Expect(node.Labels).To(HaveKey(loadedKernelModuleReadyNodeLabel)) + }) + + It("Should fail to patch node", func() { + node := v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{}, + }, + } + loaded := []string{loadedKernelModuleReadyNodeLabel} + unloaded := []string{unloadedKernelModuleReadyNodeLabel} + + clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) + + err := n.UpdateLabels(ctx, &node, loaded, unloaded) + Expect(err).To(HaveOccurred()) + + }) +}) + var _ = Describe("GetNumTargetedNodes", func() { var ( ctrl *gomock.Controller @@ -131,7 +186,7 @@ var _ = Describe("GetNumTargetedNodes", func() { mn = NewNode(clnt) }) - It("There are not schedulable nodes", func() { + It("There are no schedulable nodes", func() { clnt.EXPECT().List(context.Background(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) numOfNodes, err := mn.GetNumTargetedNodes(context.Background(), map[string]string{}) @@ -220,3 +275,37 @@ var _ = Describe("FindNodeCondition", func() { Expect(conditionFound).To(BeNil()) }) }) + +var _ = Describe("addLabels", func() { + var node v1.Node + + BeforeEach(func() { + node = v1.Node{} + }) + + It("Should add labels", func() { + labels := []string{loadedKernelModuleReadyNodeLabel} + addLabels(&node, labels) + Expect(node.Labels).To(HaveKey(loadedKernelModuleReadyNodeLabel)) + }) +}) + +var _ = Describe("removeLabels", func() { + var node v1.Node + + BeforeEach(func() { + node = v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + loadedKernelModuleReadyNodeLabel: "", + }, + }, + } + }) + + It("Should remove labels", func() { + labels := []string{loadedKernelModuleReadyNodeLabel} + removeLabels(&node, labels) + Expect(node.Labels).ToNot(HaveKey(loadedKernelModuleReadyNodeLabel)) + }) +})