deleting orphan worker pods

Previously, when worker pods failed for any reason and
the user deleted the associated Module CR,
the failing pods would remain in the cluster instead of being removed.
This commit introduces a garbage collection mechanism to ensure that
orphaned worker pods are properly deleted when
the corresponding Module CR is removed.

Author: TomerNewman

internal/controllers/mock_nmc_reconciler.go

@@ -159,6 +159,10 @@ func (r *NMCReconciler) Reconcile(ctx context.Context, req reconcile.Request) (r
 		errs = append(errs, fmt.Errorf("failed to GC in-use labels for NMC %s: %v", req.NamespacedName, err))
 	}
 
+	if err := r.helper.GarbageCollectWorkerPods(ctx, &nmcObj); err != nil {
+		errs = append(errs, fmt.Errorf("failed to GC orphan worker pods for NMC %s: %v", req.NamespacedName, 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))
 	} else {
@@ -212,6 +216,7 @@ func GetContainerStatus(statuses []v1.ContainerStatus, name string) v1.Container
 
 type nmcReconcilerHelper interface {
 	GarbageCollectInUseLabels(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) error
+	GarbageCollectWorkerPods(ctx context.Context, nmc *kmmv1beta1.NodeModulesConfig) 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
@@ -238,6 +243,41 @@ func newNMCReconcilerHelper(client client.Client, podManager pod.WorkerPodManage
 	}
 }
 
+func (h *nmcReconcilerHelperImpl) GarbageCollectWorkerPods(ctx context.Context, nmcObj *kmmv1beta1.NodeModulesConfig) error {
+	podsList, err := h.podManager.ListWorkerPodsOnNode(ctx, nmcObj.Name)
+	if err != nil {
+		return fmt.Errorf("could not list worker Pods: %v", err)
+	}
+
+	modulePresentInNMC := sets.New[string]()
+	for _, module := range nmcObj.Spec.Modules {
+		modulePresentInNMC.Insert(module.Name)
+	}
+	for _, module := range nmcObj.Status.Modules {
+		modulePresentInNMC.Insert(module.Name)
+	}
+
+	var errs []error
+	for _, workerPod := range podsList {
+		podModuleName := workerPod.Labels[constants.ModuleNameLabel]
+		if !modulePresentInNMC.Has(podModuleName) {
+			mergeFrom := client.MergeFrom(workerPod.DeepCopy())
+			if controllerutil.RemoveFinalizer(&workerPod, pod.NodeModulesConfigFinalizer) {
+				if err = h.client.Patch(ctx, &workerPod, mergeFrom); err != nil {
+					errs = append(
+						errs, fmt.Errorf("could not patch Pod %s/%s: %v", workerPod.Namespace, workerPod.Name, err),
+					)
+				}
+			}
+
+			if err = h.podManager.DeletePod(ctx, &workerPod); err != nil {
+				errs = append(errs, fmt.Errorf("could not delete pod %s: %v", workerPod.Name, err))
+			}
+		}
+	}
+	return errors.Join(errs...)
+}
+
 // GarbageCollectInUseLabels removes all module-in-use labels for which there is no corresponding entry either in
 // spec.modules or in status.modules.
 func (h *nmcReconcilerHelperImpl) GarbageCollectInUseLabels(ctx context.Context, nmcObj *kmmv1beta1.NodeModulesConfig) error {

internal/controllers/nmc_reconciler.go

@@ -303,6 +303,7 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
 			wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec1, nil, &node),
 			wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node),
 			wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc),
+			wh.EXPECT().GarbageCollectWorkerPods(ctx, nmc),
 			wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(loaded, unloaded, err),
 			wh.EXPECT().RecordEvents(&node, loaded, unloaded),
 		)
@@ -330,6 +331,7 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
 			fmt.Errorf("error processing Module %s: %v", namespace+"/"+mod0Name, errorMeassge),
 			fmt.Errorf("error processing orphan status for Module %s: %v", namespace+"/"+mod2Name, errorMeassge),
 			fmt.Errorf("failed to GC in-use labels for NMC %s: %v", types.NamespacedName{Name: nmcName}, errorMeassge),
+			fmt.Errorf("failed to GC orphan worker pods for NMC %s: %v", types.NamespacedName{Name: nmcName}, errorMeassge),
 			fmt.Errorf("could not update node's labels for NMC %s: %v", types.NamespacedName{Name: nmcName}, errorMeassge),
 		}
 
@@ -381,6 +383,7 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
 			wh.EXPECT().ProcessModuleSpec(contextWithValueMatch, nmc, &spec0, &status0, &node).Return(errors.New(errorMeassge)),
 			wh.EXPECT().ProcessUnconfiguredModuleStatus(contextWithValueMatch, nmc, &status2, &node).Return(errors.New(errorMeassge)),
 			wh.EXPECT().GarbageCollectInUseLabels(ctx, nmc).Return(errors.New(errorMeassge)),
+			wh.EXPECT().GarbageCollectWorkerPods(ctx, nmc).Return(errors.New(errorMeassge)),
 			wh.EXPECT().UpdateNodeLabels(ctx, nmc, &node).Return(nil, nil, errors.New(errorMeassge)),
 		)
 
@@ -389,6 +392,91 @@ var _ = Describe("NodeModulesConfigReconciler_Reconcile", func() {
 	})
 })
 
+var _ = Describe("nmcReconcilerHelperImpl_GarbageCollectWorkerPods", func() {
+	var (
+		ctx    = context.TODO()
+		client *testclient.MockClient
+		pm     *pod.MockWorkerPodManager
+		nrh    nmcReconcilerHelper
+	)
+
+	BeforeEach(func() {
+		ctrl := gomock.NewController(GinkgoT())
+		client = testclient.NewMockClient(ctrl)
+		pm = pod.NewMockWorkerPodManager(ctrl)
+		nrh = newNMCReconcilerHelper(client, pm, nil, nil)
+	})
+
+	It("should delete orphaned worker pod", func() {
+		nmcSpec := kmmv1beta1.NodeModulesConfigSpec{
+			Modules: []kmmv1beta1.NodeModuleSpec{
+				{
+					ModuleItem: kmmv1beta1.ModuleItem{Name: nameSecond},
+				},
+			},
+		}
+		nmcObj := &kmmv1beta1.NodeModulesConfig{
+			ObjectMeta: metav1.ObjectMeta{Name: nmcName},
+			Spec:       nmcSpec,
+		}
+		pod := v1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("kmm-worker-%s-%s", nmcObj.Name, nameFirst),
+				Namespace: "d",
+				Labels: map[string]string{
+					constants.ModuleNameLabel: nameFirst,
+				},
+				Finalizers: []string{pod.NodeModulesConfigFinalizer},
+			},
+		}
+
+		gomock.InOrder(
+			pm.EXPECT().ListWorkerPodsOnNode(ctx, nmcName).Return([]v1.Pod{pod}, nil),
+			client.EXPECT().Patch(ctx, gomock.Any(), gomock.Any()).Return(nil),
+			pm.EXPECT().DeletePod(ctx, gomock.Any()).Return(nil),
+		)
+
+		Expect(
+			nrh.GarbageCollectWorkerPods(ctx, nmcObj),
+		).NotTo(
+			HaveOccurred(),
+		)
+	})
+
+	It("should do nothing because there are not orphaned worker pods", func() {
+		nmcSpec := kmmv1beta1.NodeModulesConfigSpec{
+			Modules: []kmmv1beta1.NodeModuleSpec{
+				{
+					ModuleItem: kmmv1beta1.ModuleItem{Name: nameFirst},
+				},
+			},
+		}
+		nmcObj := &kmmv1beta1.NodeModulesConfig{
+			ObjectMeta: metav1.ObjectMeta{Name: nmcName},
+			Spec:       nmcSpec,
+		}
+		pod := v1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      fmt.Sprintf("kmm-worker-%s-%s", nmcObj.Name, nameFirst),
+				Namespace: "d",
+				Labels: map[string]string{
+					constants.ModuleNameLabel: nameFirst,
+				},
+			},
+		}
+
+		gomock.InOrder(
+			pm.EXPECT().ListWorkerPodsOnNode(ctx, nmcName).Return([]v1.Pod{pod}, nil),
+		)
+
+		Expect(
+			nrh.GarbageCollectWorkerPods(ctx, nmcObj),
+		).NotTo(
+			HaveOccurred(),
+		)
+	})
+})
+
 var _ = Describe("nmcReconcilerHelperImpl_GarbageCollectInUseLabels", func() {
 	var (
 		ctx = context.TODO()