diff --git a/internal/controllers/mock_nmc_reconciler.go b/internal/controllers/mock_nmc_reconciler.go index 17064d10b..27ee3fc8b 100644 --- a/internal/controllers/mock_nmc_reconciler.go +++ b/internal/controllers/mock_nmc_reconciler.go @@ -58,31 +58,31 @@ func (mr *MocknmcReconcilerHelperMockRecorder) GarbageCollectInUseLabels(ctx, nm } // ProcessModuleSpec mocks base method. -func (m *MocknmcReconcilerHelper) ProcessModuleSpec(ctx context.Context, nmc *v1beta1.NodeModulesConfig, spec *v1beta1.NodeModuleSpec, status *v1beta1.NodeModuleStatus) error { +func (m *MocknmcReconcilerHelper) ProcessModuleSpec(ctx context.Context, nmc *v1beta1.NodeModulesConfig, spec *v1beta1.NodeModuleSpec, status *v1beta1.NodeModuleStatus, node *v1.Node) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ProcessModuleSpec", ctx, nmc, spec, status) + ret := m.ctrl.Call(m, "ProcessModuleSpec", ctx, nmc, spec, status, node) ret0, _ := ret[0].(error) return ret0 } // ProcessModuleSpec indicates an expected call of ProcessModuleSpec. -func (mr *MocknmcReconcilerHelperMockRecorder) ProcessModuleSpec(ctx, nmc, spec, status any) *gomock.Call { +func (mr *MocknmcReconcilerHelperMockRecorder) ProcessModuleSpec(ctx, nmc, spec, status, node any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProcessModuleSpec", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).ProcessModuleSpec), ctx, nmc, spec, status) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProcessModuleSpec", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).ProcessModuleSpec), ctx, nmc, spec, status, node) } // ProcessUnconfiguredModuleStatus mocks base method. -func (m *MocknmcReconcilerHelper) ProcessUnconfiguredModuleStatus(ctx context.Context, nmc *v1beta1.NodeModulesConfig, status *v1beta1.NodeModuleStatus) error { +func (m *MocknmcReconcilerHelper) ProcessUnconfiguredModuleStatus(ctx context.Context, nmc *v1beta1.NodeModulesConfig, status *v1beta1.NodeModuleStatus, node *v1.Node) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ProcessUnconfiguredModuleStatus", ctx, nmc, status) + ret := m.ctrl.Call(m, "ProcessUnconfiguredModuleStatus", ctx, nmc, status, node) ret0, _ := ret[0].(error) return ret0 } // ProcessUnconfiguredModuleStatus indicates an expected call of ProcessUnconfiguredModuleStatus. -func (mr *MocknmcReconcilerHelperMockRecorder) ProcessUnconfiguredModuleStatus(ctx, nmc, status any) *gomock.Call { +func (mr *MocknmcReconcilerHelperMockRecorder) ProcessUnconfiguredModuleStatus(ctx, nmc, status, node any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProcessUnconfiguredModuleStatus", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).ProcessUnconfiguredModuleStatus), ctx, nmc, status) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProcessUnconfiguredModuleStatus", reflect.TypeOf((*MocknmcReconcilerHelper)(nil).ProcessUnconfiguredModuleStatus), ctx, nmc, status, node) } // RecordEvents mocks base method. diff --git a/internal/controllers/nmc_reconciler.go b/internal/controllers/nmc_reconciler.go index 4603b4b14..ee4c5497c 100644 --- a/internal/controllers/nmc_reconciler.go +++ b/internal/controllers/nmc_reconciler.go @@ -125,6 +125,11 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r statusMap[status.Namespace+"/"+status.Name] = &nmcObj.Status.Modules[i] } + 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) + } + errs := make([]error, 0, len(nmcObj.Spec.Modules)+len(nmcObj.Status.Modules)) for _, mod := range nmcObj.Spec.Modules { @@ -132,7 +137,7 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r logger := logger.WithValues("module", moduleNameKey) - if err := r.helper.ProcessModuleSpec(ctrl.LoggerInto(ctx, logger), &nmcObj, &mod, statusMap[moduleNameKey]); err != nil { + if err := r.helper.ProcessModuleSpec(ctrl.LoggerInto(ctx, logger), &nmcObj, &mod, statusMap[moduleNameKey], &node); err != nil { errs = append( errs, fmt.Errorf("error processing Module %s: %v", moduleNameKey, err), @@ -150,7 +155,7 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r for statusNameKey, status := range statusMap { logger := logger.WithValues("status", statusNameKey) - if err := r.helper.ProcessUnconfiguredModuleStatus(ctrl.LoggerInto(ctx, logger), &nmcObj, status); err != nil { + if err := r.helper.ProcessUnconfiguredModuleStatus(ctrl.LoggerInto(ctx, logger), &nmcObj, status, &node); err != nil { errs = append( errs, fmt.Errorf("error processing orphan status for Module %s: %v", statusNameKey, err), @@ -161,10 +166,6 @@ 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 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)) @@ -223,8 +224,8 @@ func GetContainerStatus(statuses []v1.ContainerStatus, name string) v1.Container type nmcReconcilerHelper interface { GarbageCollectInUseLabels(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) error - ProcessModuleSpec(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, spec *kmmv1beta1.NodeModuleSpec, status *kmmv1beta1.NodeModuleStatus) error - ProcessUnconfiguredModuleStatus(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, status *kmmv1beta1.NodeModuleStatus) error + ProcessModuleSpec(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, spec *kmmv1beta1.NodeModuleSpec, status *kmmv1beta1.NodeModuleStatus, node *v1.Node) error + ProcessUnconfiguredModuleStatus(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, status *kmmv1beta1.NodeModuleStatus, node *v1.Node) error RemovePodFinalizers(ctx context.Context, nodeName string) error SyncStatus(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) error UpdateNodeLabels(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig, node *v1.Node) ([]types.NamespacedName, []types.NamespacedName, error) @@ -314,6 +315,7 @@ func (h *nmcReconcilerHelperImpl) ProcessModuleSpec( nmcObj *kmmv1beta1.NodeModulesConfig, spec *kmmv1beta1.NodeModuleSpec, status *kmmv1beta1.NodeModuleStatus, + node *v1.Node, ) error { podName := workerPodName(nmcObj.Name, spec.Name) @@ -344,13 +346,7 @@ func (h *nmcReconcilerHelperImpl) ProcessModuleSpec( return h.pm.CreateLoaderPod(ctx, nmcObj, spec) } - node := v1.Node{} - - if err = h.client.Get(ctx, types.NamespacedName{Name: nmcObj.Name}, &node); err != nil { - return fmt.Errorf("could not get node %s: %v", nmcObj.Name, err) - } - - if h.nodeAPI.NodeBecomeReadyAfter(&node, status.LastTransitionTime) { + if h.nodeAPI.NodeBecomeReadyAfter(node, status.LastTransitionTime) { logger.Info("node has been rebooted and become ready after kernel module was loaded; creating loader Pod") return h.pm.CreateLoaderPod(ctx, nmcObj, spec) } @@ -392,11 +388,20 @@ func (h *nmcReconcilerHelperImpl) ProcessUnconfiguredModuleStatus( ctx context.Context, nmcObj *kmmv1beta1.NodeModulesConfig, status *kmmv1beta1.NodeModuleStatus, + node *v1.Node, ) error { podName := workerPodName(nmcObj.Name, status.Name) logger := ctrl.LoggerFrom(ctx).WithValues("pod name", podName) + /* node was rebooted, spec not set so no kernel module is loaded, no need to unload. + it also fixes the scenario when node's kernel was upgraded, so unload pod will fail anyway + */ + if h.nodeAPI.NodeBecomeReadyAfter(node, status.LastTransitionTime) { + logger.Info("node was rebooted, no need to unload kernel module that is not present in kernel, will wait until NMC spec is updated") + return nil + } + pod, err := h.pm.GetWorkerPod(ctx, podName, status.Namespace) if err != nil { return fmt.Errorf("error while getting the worker Pod %s: %v", podName, err) diff --git a/internal/controllers/nmc_reconciler_test.go b/internal/controllers/nmc_reconciler_test.go index b37d6be72..69973e78e 100644 --- a/internal/controllers/nmc_reconciler_test.go +++ b/internal/controllers/nmc_reconciler_test.go @@ -106,6 +106,26 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { Expect(err).To(HaveOccurred()) }) + It("should fail if we could not get the node of the NMC", func() { + nmc := &kmmv1beta1.NodeModulesConfig{ + ObjectMeta: metav1.ObjectMeta{Name: nmcName}, + } + node := v1.Node{} + gomock.InOrder( + kubeClient. + EXPECT(). + Get(ctx, nmcNsn, &kmmv1beta1.NodeModulesConfig{}). + Do(func(_ context.Context, _ types.NamespacedName, kubeNmc ctrlclient.Object, _ ...ctrlclient.Options) { + *kubeNmc.(*kmmv1beta1.NodeModulesConfig) = *nmc + }), + wh.EXPECT().SyncStatus(ctx, nmc), + kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &node).Return(fmt.Errorf("some error")), + ) + + _, err := r.Reconcile(ctx, req) + Expect(err).To(HaveOccurred()) + }) + It("should process spec entries and orphan statuses", func() { const ( mod0Name = "mod0" @@ -168,11 +188,11 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { *kubeNmc.(*kmmv1beta1.NodeModulesConfig) = *nmc }), wh.EXPECT().SyncStatus(ctx, nmc), - wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0), - wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec1, nil), - wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2), - wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc), kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &node).Return(nil), + wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0, &node), + wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec1, nil, &node), + wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node), + wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc), wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(loaded, unloaded, err), wh.EXPECT().RecordEvents(&node, loaded, unloaded), ) @@ -246,10 +266,10 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() { *kubeNmc.(*kmmv1beta1.NodeModulesConfig) = *nmc }), wh.EXPECT().SyncStatus(ctx, nmc).Return(nil), - wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0).Return(fmt.Errorf(errorMeassge)), - wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2).Return(fmt.Errorf(errorMeassge)), - wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc).Return(fmt.Errorf(errorMeassge)), kubeClient.EXPECT().Get(ctx, types.NamespacedName{Name: nmc.Name}, &node).Return(nil), + wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0, &node).Return(fmt.Errorf(errorMeassge)), + wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node).Return(fmt.Errorf(errorMeassge)), + wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc).Return(fmt.Errorf(errorMeassge)), wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(nil, nil, fmt.Errorf(errorMeassge)), ) @@ -425,7 +445,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { ) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, nil), + wh.ProcessModuleSpec(ctx, nmc, spec, nil, nil), ).NotTo( HaveOccurred(), ) @@ -458,7 +478,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { ) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, nil), ).NotTo( HaveOccurred(), ) @@ -491,50 +511,12 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { ) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, nil), ).NotTo( HaveOccurred(), ) }) - It("should return an error if we could not get the node", func() { - nmc := &kmmv1beta1.NodeModulesConfig{ - ObjectMeta: metav1.ObjectMeta{Name: nmcName}, - } - - cfg := kmmv1beta1.ModuleConfig{ContainerImage: "some-image"} - - spec := &kmmv1beta1.NodeModuleSpec{ - ModuleItem: kmmv1beta1.ModuleItem{ - Name: name, - Namespace: namespace, - }, - Config: cfg, - } - - status := &kmmv1beta1.NodeModuleStatus{ - ModuleItem: kmmv1beta1.ModuleItem{ - Name: name, - Namespace: namespace, - }, - Config: cfg, - } - - gomock.InOrder( - pm.EXPECT().GetWorkerPod(ctx, podName, namespace), - client. - EXPECT(). - Get(ctx, types.NamespacedName{Name: nmcName}, &v1.Node{}). - Return(errors.New("random error")), - ) - - Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), - ).To( - HaveOccurred(), - ) - }) - nmc := &kmmv1beta1.NodeModulesConfig{ ObjectMeta: metav1.ObjectMeta{Name: nmcName}, } @@ -546,6 +528,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { Namespace: namespace, }, } + node := &v1.Node{} now := metav1.Now() @@ -569,8 +552,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { gomock.InOrder( pm.EXPECT().GetWorkerPod(ctx, podName, namespace), - client.EXPECT().Get(ctx, types.NamespacedName{Name: nmcName}, &v1.Node{}), - nm.EXPECT().NodeBecomeReadyAfter(gomock.Any(), status.LastTransitionTime).Return(returnValue), + nm.EXPECT().NodeBecomeReadyAfter(node, status.LastTransitionTime).Return(returnValue), ) if shouldCreate { @@ -578,7 +560,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { } Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, node), ).NotTo( HaveOccurred(), ) @@ -594,7 +576,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { Return(&v1.Pod{}, nil) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, nil), ).NotTo( HaveOccurred(), ) @@ -613,7 +595,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { Return(&pod, nil) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, nil), ).NotTo( HaveOccurred(), ) @@ -646,7 +628,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { ) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, nil), ).To( HaveOccurred(), ) @@ -686,7 +668,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessModuleSpec", func() { ) Expect( - wh.ProcessModuleSpec(ctx, nmc, spec, status), + wh.ProcessModuleSpec(ctx, nmc, spec, status, nil), ).NotTo( HaveOccurred(), ) @@ -702,6 +684,7 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func client *testclient.MockClient pm *MockpodManager + nm *node.MockNode helper nmcReconcilerHelper ) @@ -709,7 +692,8 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func ctrl := gomock.NewController(GinkgoT()) client = testclient.NewMockClient(ctrl) pm = NewMockpodManager(ctrl) - helper = newNMCReconcilerHelper(client, pm, nil, nil) + nm = node.NewMockNode(ctrl) + helper = newNMCReconcilerHelper(client, pm, nil, nm) }) nmc := &kmmv1beta1.NodeModulesConfig{ @@ -723,14 +707,27 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func }, } + node := v1.Node{} + + It("should do nothing , if the node has been rebooted/ready lately", func() { + nm.EXPECT().NodeBecomeReadyAfter(&node, status.LastTransitionTime).Return(true) + + Expect( + helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status, &node), + ).NotTo( + HaveOccurred(), + ) + }) + It("should create an unloader Pod if no worker Pod exists", func() { gomock.InOrder( + nm.EXPECT().NodeBecomeReadyAfter(&node, status.LastTransitionTime).Return(false), pm.EXPECT().GetWorkerPod(ctx, podName, namespace), pm.EXPECT().CreateUnloaderPod(ctx, nmc, status), ) Expect( - helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status), + helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status, &node), ).NotTo( HaveOccurred(), ) @@ -746,12 +743,13 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func } gomock.InOrder( + nm.EXPECT().NodeBecomeReadyAfter(&node, status.LastTransitionTime).Return(false), pm.EXPECT().GetWorkerPod(ctx, podName, namespace).Return(&pod, nil), pm.EXPECT().DeletePod(ctx, &pod), ) Expect( - helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status), + helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status, &node), ).NotTo( HaveOccurred(), ) @@ -765,10 +763,13 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func }, } - pm.EXPECT().GetWorkerPod(ctx, podName, namespace).Return(&pod, nil) + gomock.InOrder( + nm.EXPECT().NodeBecomeReadyAfter(&node, status.LastTransitionTime).Return(false), + pm.EXPECT().GetWorkerPod(ctx, podName, namespace).Return(&pod, nil), + ) Expect( - helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status), + helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status, &node), ).NotTo( HaveOccurred(), ) @@ -791,12 +792,13 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func } gomock.InOrder( + nm.EXPECT().NodeBecomeReadyAfter(&node, status.LastTransitionTime).Return(false), pm.EXPECT().GetWorkerPod(ctx, podName, namespace).Return(&pod, nil), pm.EXPECT().UnloaderPodTemplate(ctx, nmc, status).Return(nil, errors.New("random error")), ) Expect( - helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status), + helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status, &node), ).To( HaveOccurred(), ) @@ -823,13 +825,14 @@ var _ = Describe("nmcReconcilerHelperImpl_ProcessUnconfiguredModuleStatus", func podTemplate.Annotations[hashAnnotationKey] = "456" gomock.InOrder( + nm.EXPECT().NodeBecomeReadyAfter(&node, status.LastTransitionTime).Return(false), pm.EXPECT().GetWorkerPod(ctx, podName, namespace).Return(&pod, nil), pm.EXPECT().UnloaderPodTemplate(ctx, nmc, status).Return(podTemplate, nil), pm.EXPECT().DeletePod(ctx, &pod), ) Expect( - helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status), + helper.ProcessUnconfiguredModuleStatus(ctx, nmc, status, &node), ).NotTo( HaveOccurred(), )