diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index dcf8bb868..9c1ad4f14 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -128,7 +128,7 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r } errs := make([]error, 0, len(nmcObj.Spec.Modules)+len(nmcObj.Status.Modules)) - + var readyLabelsToRemove []string for _, mod := range nmcObj.Spec.Modules { moduleNameKey := mod.Namespace + "/" + mod.Name @@ -136,6 +136,7 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r // skipping handling NMC spec module until node is ready if !r.nodeAPI.IsNodeSchedulable(&node, mod.Config.Tolerations) { + readyLabelsToRemove = append(readyLabelsToRemove, utils.GetKernelModuleReadyNodeLabel(mod.Namespace, mod.Name)) delete(statusMap, moduleNameKey) continue } @@ -166,8 +167,8 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r } // removing label of loaded kmods - if !r.nodeAPI.IsNodeSchedulable(&node, nil) { - if err := r.nodeAPI.RemoveNodeReadyLabels(ctx, &node); err != nil { + if readyLabelsToRemove != nil { + if err := r.nodeAPI.UpdateLabels(ctx, &node, nil, readyLabelsToRemove); err != nil { return ctrl.Result{}, fmt.Errorf("could remove node %s labels: %v", node.Name, err) } return ctrl.Result{}, nil diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index 08499b686..a0dec0f41 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -131,8 +131,8 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { It("should remove kmod labels and not continue if node is not schedulable", func() { spec0 := kmmv1beta1.NodeModuleSpec{ ModuleItem: kmmv1beta1.ModuleItem{ - Namespace: namespace, - Name: "mod0", + Namespace: "test-ns", + Name: "test-module", }, } nmc := &kmmv1beta1.NodeModulesConfig{ @@ -171,9 +171,8 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { }, ), nm.EXPECT().IsNodeSchedulable(&node, nil).Return(false), - nm.EXPECT().IsNodeSchedulable(&node, nil).Return(false), - nm.EXPECT().RemoveNodeReadyLabels(ctx, &node).DoAndReturn( - func(_ context.Context, obj ctrlclient.Object) error { + nm.EXPECT().UpdateLabels(ctx, &node, nil, []string{kmodName}).DoAndReturn( + func(_ context.Context, obj ctrlclient.Object, _, _ []string) error { delete(node.ObjectMeta.Labels, kmodName) return nil }, @@ -186,8 +185,17 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { }) It("should fail to remove kmod labels and not continue if node is not schedulable", func() { + spec0 := kmmv1beta1.NodeModuleSpec{ + ModuleItem: kmmv1beta1.ModuleItem{ + Namespace: "test-ns", + Name: "test-module", + }, + } nmc := &kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: nmcName}, + Spec: kmmv1beta1.NodeModulesConfigSpec{ + Modules: []kmmv1beta1.NodeModuleSpec{spec0}, + }, } node := v1.Node{ ObjectMeta: metav1.ObjectMeta{ @@ -219,8 +227,8 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { }, ), nm.EXPECT().IsNodeSchedulable(&node, nil).Return(false), - nm.EXPECT().RemoveNodeReadyLabels(ctx, &node).DoAndReturn( - func(_ context.Context, obj ctrlclient.Object) error { + nm.EXPECT().UpdateLabels(ctx, &node, nil, []string{kmodName}).DoAndReturn( + func(_ context.Context, obj ctrlclient.Object, _, _ []string) error { return fmt.Errorf("some error") }, ), @@ -299,7 +307,6 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true), wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec1, nil, &node), wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node), - nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true), wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc), wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(loaded, unloaded, err), wh.EXPECT().RecordEvents(&node, loaded, unloaded), @@ -378,7 +385,6 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true), wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0, &node).Return(fmt.Errorf(errorMeassge)), wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node).Return(fmt.Errorf(errorMeassge)), - nm.EXPECT().IsNodeSchedulable(&node, nil).Return(true), wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc).Return(fmt.Errorf(errorMeassge)), wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(nil, nil, fmt.Errorf(errorMeassge)), ) diff --git a/internal/node/mock_node.go b/internal/node/mock_node.go index 11c7cdc45..d47c5ce3b 100644 --- a/internal/node/mock_node.go +++ b/internal/node/mock_node.go @@ -98,20 +98,6 @@ func (mr *MockNodeMockRecorder) NodeBecomeReadyAfter(node, checkTime any) *gomoc return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NodeBecomeReadyAfter", reflect.TypeOf((*MockNode)(nil).NodeBecomeReadyAfter), node, checkTime) } -// RemoveNodeReadyLabels mocks base method. -func (m *MockNode) RemoveNodeReadyLabels(ctx context.Context, node *v1.Node) error { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "RemoveNodeReadyLabels", ctx, node) - ret0, _ := ret[0].(error) - return ret0 -} - -// RemoveNodeReadyLabels indicates an expected call of RemoveNodeReadyLabels. -func (mr *MockNodeMockRecorder) RemoveNodeReadyLabels(ctx, node any) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RemoveNodeReadyLabels", reflect.TypeOf((*MockNode)(nil).RemoveNodeReadyLabels), ctx, node) -} - // UpdateLabels mocks base method. func (m *MockNode) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error { m.ctrl.T.Helper() diff --git a/internal/node/node.go b/internal/node/node.go index e331977c5..4ca8de692 100644 --- a/internal/node/node.go +++ b/internal/node/node.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "github.com/kubernetes-sigs/kernel-module-management/internal/meta" - "github.com/kubernetes-sigs/kernel-module-management/internal/utils" v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -19,7 +18,6 @@ type Node interface { GetNumTargetedNodes(ctx context.Context, selector map[string]string, tolerations []v1.Toleration) (int, error) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeRemoved []string) error NodeBecomeReadyAfter(node *v1.Node, checkTime metav1.Time) bool - RemoveNodeReadyLabels(ctx context.Context, node *v1.Node) error } type node struct { @@ -87,20 +85,6 @@ func (n *node) UpdateLabels(ctx context.Context, node *v1.Node, toBeAdded, toBeR return nil } -func (n *node) RemoveNodeReadyLabels(ctx context.Context, node *v1.Node) error { - var labelsToRemove []string - for label := range node.GetLabels() { - if ok, _, _ := utils.IsKernelModuleReadyNodeLabel(label); ok || - utils.IsDeprecatedKernelModuleReadyNodeLabel(label) { - labelsToRemove = append(labelsToRemove, label) - } - } - if err := n.UpdateLabels(ctx, node, []string{}, labelsToRemove); err != nil { - return fmt.Errorf("could update node %s labels: %v", node.Name, err) - } - return nil -} - func (n *node) NodeBecomeReadyAfter(node *v1.Node, timestamp metav1.Time) bool { conds := node.Status.Conditions for i := 0; i < len(conds); i++ { diff --git a/internal/node/node_test.go b/internal/node/node_test.go index 72ed871ef..8b577d081 100644 --- a/internal/node/node_test.go +++ b/internal/node/node_test.go @@ -160,9 +160,10 @@ var _ = Describe("GetNodesListBySelector", func() { }) const ( - loadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/loaded-ns.loaded-n.ready" - unloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/unloaded-ns.unloaded-n.ready" - notKernelModuleReadyNodeLabel = "example.node.kubernetes.io/label-not-to-be-removed" + firstloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/loaded1-ns.loaded1-n.ready" + secondloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/loaded2-ns.loaded2-n.ready" + unloadedKernelModuleReadyNodeLabel = "kmm.node.kubernetes.io/unloaded-ns.unloaded-n.ready" + notKernelModuleReadyNodeLabel = "example.node.kubernetes.io/label-not-to-be-removed" ) var _ = Describe("UpdateLabels", func() { @@ -186,14 +187,14 @@ var _ = Describe("UpdateLabels", func() { Labels: map[string]string{}, }, } - loaded := []string{loadedKernelModuleReadyNodeLabel} + loaded := []string{firstloadedKernelModuleReadyNodeLabel} 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)) + Expect(node.Labels).To(HaveKey(firstloadedKernelModuleReadyNodeLabel)) }) It("Should fail to patch node", func() { @@ -202,7 +203,7 @@ var _ = Describe("UpdateLabels", func() { Labels: map[string]string{}, }, } - loaded := []string{loadedKernelModuleReadyNodeLabel} + loaded := []string{firstloadedKernelModuleReadyNodeLabel} unloaded := []string{unloadedKernelModuleReadyNodeLabel} clnt.EXPECT().Patch(ctx, gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) @@ -318,58 +319,6 @@ var _ = Describe("NodeBecomeReadyAfter", func() { }) }) -var _ = Describe("RemoveNodeReadyLabels", func() { - var ( - ctrl *gomock.Controller - n Node - node *v1.Node - ctx context.Context - clnt *client.MockClient - ) - - BeforeEach(func() { - ctrl = gomock.NewController(GinkgoT()) - clnt = client.NewMockClient(ctrl) - ctx = context.TODO() - n = NewNode(clnt) - node = &v1.Node{ - ObjectMeta: metav1.ObjectMeta{ - Labels: map[string]string{ - loadedKernelModuleReadyNodeLabel: "", - notKernelModuleReadyNodeLabel: "", - }, - }, - } - }) - - It("Should remove all kmod labels", func() { - clnt.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(nil) - err := n.RemoveNodeReadyLabels(ctx, node) - Expect(err).To(BeNil()) - Expect(node.Labels).ToNot(HaveKey(loadedKernelModuleReadyNodeLabel)) - Expect(node.Labels).To(HaveKey(notKernelModuleReadyNodeLabel)) - }) - It("Should fail", func() { - clnt.EXPECT().Patch(gomock.Any(), gomock.Any(), gomock.Any()).Return(fmt.Errorf("some error")) - err := n.RemoveNodeReadyLabels(ctx, node) - Expect(err).ToNot(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 @@ -377,15 +326,15 @@ var _ = Describe("removeLabels", func() { node = v1.Node{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{ - loadedKernelModuleReadyNodeLabel: "", + firstloadedKernelModuleReadyNodeLabel: "", }, }, } }) It("Should remove labels", func() { - labels := []string{loadedKernelModuleReadyNodeLabel} + labels := []string{firstloadedKernelModuleReadyNodeLabel} removeLabels(&node, labels) - Expect(node.Labels).ToNot(HaveKey(loadedKernelModuleReadyNodeLabel)) + Expect(node.Labels).ToNot(HaveKey(firstloadedKernelModuleReadyNodeLabel)) }) })