Fixing Unloading Kmod on a previously failed Load Pod (#530)

This PR fixes 2 issues:
1) In case Load Pod is failing, its Config in status is nil.
   if NMC is configured to un load the module, for whatever reason,
   the code must also check if the Config is nil, and then just to
   delete the status for module, in order to avoid panic
2) When unload Pod is failing, its status should be retained as is,
   otherwise the Config is lost, and the following unload pod won't be
   able to run

Author: yevgeny-shnaidman

internal/controllers/nmc_reconciler.go

@@ -274,7 +274,7 @@ func (w *workerHelperImpl) ProcessModuleSpec(
 
 func (w *workerHelperImpl) ProcessOrphanModuleStatus(
 	ctx context.Context,
-	nmc *kmmv1beta1.NodeModulesConfig,
+	nmcObj *kmmv1beta1.NodeModulesConfig,
 	status *kmmv1beta1.NodeModuleStatus,
 ) error {
 	logger := ctrl.LoggerFrom(ctx)
@@ -284,9 +284,16 @@ func (w *workerHelperImpl) ProcessOrphanModuleStatus(
 		return nil
 	}
 
+	if status.Config == nil {
+		logger.Info("Missing status config and pod is not running: previously failed pod, no need to unload")
+		patchFrom := client.MergeFrom(nmcObj.DeepCopy())
+		nmc.RemoveModuleStatus(&nmcObj.Status.Modules, status.Namespace, status.Name)
+		return w.client.Status().Patch(ctx, nmcObj, patchFrom)
+	}
+
 	logger.Info("Creating unloader Pod")
 
-	return w.pm.CreateUnloaderPod(ctx, nmc, status)
+	return w.pm.CreateUnloaderPod(ctx, nmcObj, status)
 }
 
 func (w *workerHelperImpl) RemoveOrphanFinalizers(ctx context.Context, nodeName string) error {
@@ -347,27 +354,32 @@ func (w *workerHelperImpl) SyncStatus(ctx context.Context, nmcObj *kmmv1beta1.No
 
 		logger.Info("Processing worker Pod")
 
-		status := kmmv1beta1.NodeModuleStatus{
-			Namespace: modNamespace,
-			Name:      modName,
+		status := nmc.FindModuleStatus(nmcObj.Status.Modules, modNamespace, modName)
+		if status == nil {
+			status = &kmmv1beta1.NodeModuleStatus{
+				Namespace: modNamespace,
+				Name:      modName,
+			}
 		}
 
 		deletePod := false
-		statusDeleted := false
+		updateModuleStatus := false
 
 		switch phase {
 		case v1.PodFailed:
+			status.InProgress = false
 			deletePod = true
+			updateModuleStatus = true
 		case v1.PodSucceeded:
 			deletePod = true
 
 			if p.Labels[actionLabelKey] == WorkerActionUnload {
 				nmc.RemoveModuleStatus(&nmcObj.Status.Modules, modNamespace, modName)
-				deletePod = true
-				statusDeleted = true
 				break
 			}
 
+			updateModuleStatus = true
+
 			config := kmmv1beta1.ModuleConfig{}
 
 			if err = yaml.UnmarshalStrict([]byte(p.Annotations[configAnnotationKey]), &config); err != nil {
@@ -388,9 +400,9 @@ func (w *workerHelperImpl) SyncStatus(ctx context.Context, nmcObj *kmmv1beta1.No
 
 			status.LastTransitionTime = &podLTT
 
-			deletePod = true
 		case v1.PodPending, v1.PodRunning:
 			status.InProgress = true
+			updateModuleStatus = true
 			// TODO: if the NMC's spec changed compared to the Pod's config, recreate the Pod
 		default:
 			errs = append(
@@ -410,8 +422,8 @@ func (w *workerHelperImpl) SyncStatus(ctx context.Context, nmcObj *kmmv1beta1.No
 			}
 		}
 
-		if !statusDeleted {
-			nmc.SetModuleStatus(&nmcObj.Status.Modules, status)
+		if updateModuleStatus {
+			nmc.SetModuleStatus(&nmcObj.Status.Modules, *status)
 		}
 	}
 

internal/controllers/nmc_reconciler_test.go

@@ -355,14 +355,33 @@ var _ = Describe("workerHelper_ProcessOrphanModuleStatus", func() {
 		)
 	})
 
+	It("should remove status in case Config is nil", func() {
+		nmc := &kmmv1beta1.NodeModulesConfig{}
+		status := &kmmv1beta1.NodeModuleStatus{}
+
+		ctrl := gomock.NewController(GinkgoT())
+		client := testclient.NewMockClient(ctrl)
+		sw := testclient.NewMockStatusWriter(ctrl)
+		client.EXPECT().Status().Return(sw)
+		sw.EXPECT().Patch(ctx, nmc, gomock.Any())
+		Expect(
+			NewWorkerHelper(client, nil).ProcessOrphanModuleStatus(ctx, nmc, status),
+		).NotTo(
+			HaveOccurred(),
+		)
+	})
+
 	It("should create an unloader Pod", func() {
 		ctrl := gomock.NewController(GinkgoT())
 		client := testclient.NewMockClient(ctrl)
+
 		pm := NewMockpodManager(ctrl)
 		wh := NewWorkerHelper(client, pm)
 
 		nmc := &kmmv1beta1.NodeModulesConfig{}
-		status := &kmmv1beta1.NodeModuleStatus{}
+		status := &kmmv1beta1.NodeModuleStatus{
+			Config: &kmmv1beta1.ModuleConfig{},
+		}
 
 		pm.EXPECT().CreateUnloaderPod(ctx, nmc, status)
 

internal/nmc/helper.go

@@ -75,7 +75,7 @@ func (h *helper) GetModuleEntry(nmc *kmmv1beta1.NodeModulesConfig, modNamespace,
 	return nil, 0
 }
 
-func findModuleStatus(statuses []kmmv1beta1.NodeModuleStatus, moduleNamespace, moduleName string) *kmmv1beta1.NodeModuleStatus {
+func FindModuleStatus(statuses []kmmv1beta1.NodeModuleStatus, moduleNamespace, moduleName string) *kmmv1beta1.NodeModuleStatus {
 	for i := 0; i < len(statuses); i++ {
 		s := statuses[i]
 
@@ -108,7 +108,7 @@ func SetModuleStatus(statuses *[]kmmv1beta1.NodeModuleStatus, status kmmv1beta1.
 		return
 	}
 
-	s := findModuleStatus(*statuses, status.Namespace, status.Name)
+	s := FindModuleStatus(*statuses, status.Namespace, status.Name)
 
 	if s != nil {
 		*s = status