[cozystack-controller] Fix deleting workloads (#1229)

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

<!-- Thank you for making a contribution! Here are some tips for you:
- Start the PR title with the [label] of Cozystack component:
- For system components: [platform], [system], [linstor], [cilium],
[kube-ovn], [dashboard], [cluster-api], etc.
- For managed apps: [apps], [tenant], [kubernetes], [postgres],
[virtual-machine] etc.
- For development and maintenance: [tests], [ci], [docs], [maintenance].
- If it's a work in progress, consider creating this PR as a draft.
- Don't hesistate to ask for opinion and review in the community chats,
even if it's still a draft.
- Add the label `backport` if it's a bugfix that needs to be backported
to a previous version.
-->

## What this PR does

fixes https://github.com/cozystack/cozystack/issues/1222
### Release note

<!--  Write a release note:
- Explain what has changed internally and for users.
- Start with the same [label] as in the PR title
- Follow the guidelines at
https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md.
-->

```release-note
[cozystack-controller] Fix deleting workloads
```

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Refactor**
* Simplified workload reconciliation by directly verifying monitor
labels and associated resources.
* Enhanced logging consistency and updated comments for improved
clarity.
* Improved status update reliability with retry logic during
reconciliation.

* **Bug Fixes**
* Ensured workload labels are initialized before adding monitor
references.
  * Corrected owner references to point to actual resource objects.

* **Tests**
* Added tests confirming workloads are deleted if their referenced
monitor is missing and retained when all dependencies exist.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

internal/controller/workload_controller.go

@@ -8,7 +8,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/log"
@@ -26,60 +25,40 @@ type WorkloadReconciler struct {
 	Scheme *runtime.Scheme
 }
 
+// workload_controller.go
 func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	logger := log.FromContext(ctx)
+	log := log.FromContext(ctx)
+
 	w := &cozyv1alpha1.Workload{}
-	err := r.Get(ctx, req.NamespacedName, w)
-	if err != nil {
+	if err := r.Get(ctx, req.NamespacedName, w); err != nil {
 		if apierrors.IsNotFound(err) {
 			return ctrl.Result{}, nil
 		}
-		logger.Error(err, "Unable to fetch Workload")
 		return ctrl.Result{}, err
 	}
 
-	// it's being deleted, nothing to handle
-	if w.DeletionTimestamp != nil {
-		return ctrl.Result{}, nil
+	// If my monitor is gone, delete me.
+	monName, has := w.Labels["workloads.cozystack.io/monitor"]
+	if !has {
+		return ctrl.Result{}, client.IgnoreNotFound(r.Delete(ctx, w))
 	}
-
-	t := getMonitoredObject(w)
-
-	if t == nil {
-		err = r.Delete(ctx, w)
-		if err != nil {
-			logger.Error(err, "failed to delete workload")
-		}
+	monitor := &cozyv1alpha1.WorkloadMonitor{}
+	if err := r.Get(ctx, client.ObjectKey{Namespace: w.Namespace, Name: monName}, monitor); apierrors.IsNotFound(err) {
+		// Monitor is gone → delete the Workload.  Ignore NotFound here, too.
+		return ctrl.Result{}, client.IgnoreNotFound(r.Delete(ctx, w))
+	} else if err != nil {
+		// Some other error fetching the monitor
+		log.Error(err, "failed to get WorkloadMonitor", "monitor", monName)
 		return ctrl.Result{}, err
 	}
 
-	err = r.Get(ctx, types.NamespacedName{Name: t.GetName(), Namespace: t.GetNamespace()}, t)
-
-	// found object, nothing to do
-	if err == nil {
-		if !t.GetDeletionTimestamp().IsZero() {
-			return ctrl.Result{RequeueAfter: deletionRequeueDelay}, nil
-		}
-		return ctrl.Result{}, nil
-	}
-
-	// error getting object but not 404 -- requeue
-	if !apierrors.IsNotFound(err) {
-		logger.Error(err, "failed to get dependent object", "kind", t.GetObjectKind(), "dependent-object-name", t.GetName())
-		return ctrl.Result{}, err
-	}
-
-	err = r.Delete(ctx, w)
-	if err != nil {
-		logger.Error(err, "failed to delete workload")
-	}
-	return ctrl.Result{}, err
+	return ctrl.Result{}, nil
 }
 
 // SetupWithManager registers our controller with the Manager and sets up watches.
 func (r *WorkloadReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).
-		// Watch WorkloadMonitor objects
+		// Watch Workload objects
 		For(&cozyv1alpha1.Workload{}).
 		Complete(r)
 }

internal/controller/workload_controller_test.go

@@ -1,10 +1,17 @@
 package controller
 
 import (
+	"context"
 	"testing"
 
 	cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1"
 	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
 func TestUnprefixedMonitoredObjectReturnsNil(t *testing.T) {
@@ -24,3 +31,83 @@ func TestPodMonitoredObject(t *testing.T) {
 		t.Errorf(`getMonitoredObject(&Workload{Name: "%s"}) == %v, want &Pod{Name: "mypod"}`, w.Name, obj)
 	}
 }
+
+func TestWorkloadReconciler_DeletesOnMissingMonitor(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = cozyv1alpha1.AddToScheme(scheme)
+	_ = corev1.AddToScheme(scheme)
+
+	// Workload with a non-existent monitor
+	w := &cozyv1alpha1.Workload{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pod-foo",
+			Namespace: "default",
+			Labels: map[string]string{
+				"workloadmonitor.cozystack.io/name": "missing-monitor",
+			},
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(w).
+		Build()
+	reconciler := &WorkloadReconciler{Client: fakeClient}
+	req := reconcile.Request{NamespacedName: types.NamespacedName{Name: "pod-foo", Namespace: "default"}}
+
+	if _, err := reconciler.Reconcile(context.TODO(), req); err != nil {
+		t.Fatalf("Reconcile returned error: %v", err)
+	}
+
+	// Expect workload to be deleted
+	err := fakeClient.Get(context.TODO(), req.NamespacedName, &cozyv1alpha1.Workload{})
+	if !apierrors.IsNotFound(err) {
+		t.Errorf("expected workload to be deleted, got: %v", err)
+	}
+}
+
+func TestWorkloadReconciler_KeepsWhenAllExist(t *testing.T) {
+	scheme := runtime.NewScheme()
+	_ = cozyv1alpha1.AddToScheme(scheme)
+	_ = corev1.AddToScheme(scheme)
+
+	// Create a monitor and its backing Pod
+	monitor := &cozyv1alpha1.WorkloadMonitor{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "mon",
+			Namespace: "default",
+		},
+	}
+	pod := &corev1.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "foo",
+			Namespace: "default",
+		},
+	}
+	w := &cozyv1alpha1.Workload{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "pod-foo",
+			Namespace: "default",
+			Labels: map[string]string{
+				"workloadmonitor.cozystack.io/name": "mon",
+			},
+		},
+	}
+
+	fakeClient := fake.NewClientBuilder().
+		WithScheme(scheme).
+		WithObjects(monitor, pod, w).
+		Build()
+	reconciler := &WorkloadReconciler{Client: fakeClient}
+	req := reconcile.Request{NamespacedName: types.NamespacedName{Name: "pod-foo", Namespace: "default"}}
+
+	if _, err := reconciler.Reconcile(context.TODO(), req); err != nil {
+		t.Fatalf("Reconcile returned error: %v", err)
+	}
+
+	// Expect workload to remain
+	err := fakeClient.Get(context.TODO(), req.NamespacedName, &cozyv1alpha1.Workload{})
+	if err != nil {
+		t.Errorf("expected workload to persist, got error: %v", err)
+	}
+}

internal/controller/workloadmonitor_controller.go

@@ -9,6 +9,7 @@ import (
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
 	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -111,6 +112,7 @@ func (r *WorkloadMonitorReconciler) reconcileServiceForMonitor(
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("svc-%s", svc.Name),
 			Namespace: svc.Namespace,
+			Labels:    make(map[string]string, len(svc.Labels)),
 		},
 	}
 
@@ -137,9 +139,12 @@ func (r *WorkloadMonitorReconciler) reconcileServiceForMonitor(
 
 	_, err := ctrl.CreateOrUpdate(ctx, r.Client, workload, func() error {
 		// Update owner references with the new monitor
-		updateOwnerReferences(workload.GetObjectMeta(), monitor)
+		updateOwnerReferences(workload.GetObjectMeta(), &svc)
 
-		workload.Labels = svc.Labels
+		for k, v := range svc.Labels {
+			workload.Labels[k] = v
+		}
+		workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name
 
 		// Fill Workload status fields:
 		workload.Status.Kind = monitor.Spec.Kind
@@ -168,6 +173,7 @@ func (r *WorkloadMonitorReconciler) reconcilePVCForMonitor(
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("pvc-%s", pvc.Name),
 			Namespace: pvc.Namespace,
+			Labels:    make(map[string]string, len(pvc.Labels)),
 		},
 	}
 
@@ -184,9 +190,12 @@ func (r *WorkloadMonitorReconciler) reconcilePVCForMonitor(
 
 	_, err := ctrl.CreateOrUpdate(ctx, r.Client, workload, func() error {
 		// Update owner references with the new monitor
-		updateOwnerReferences(workload.GetObjectMeta(), monitor)
+		updateOwnerReferences(workload.GetObjectMeta(), &pvc)
 
-		workload.Labels = pvc.Labels
+		for k, v := range pvc.Labels {
+			workload.Labels[k] = v
+		}
+		workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name
 
 		// Fill Workload status fields:
 		workload.Status.Kind = monitor.Spec.Kind
@@ -248,19 +257,19 @@ func (r *WorkloadMonitorReconciler) reconcilePodForMonitor(
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      fmt.Sprintf("pod-%s", pod.Name),
 			Namespace: pod.Namespace,
-			Labels:    map[string]string{},
+			Labels:    make(map[string]string, len(pod.Labels)),
 		},
 	}
 
 	metaLabels := r.getWorkloadMetadata(&pod)
 	_, err := ctrl.CreateOrUpdate(ctx, r.Client, workload, func() error {
 		// Update owner references with the new monitor
-		updateOwnerReferences(workload.GetObjectMeta(), monitor)
+		updateOwnerReferences(workload.GetObjectMeta(), &pod)
 
-		// Copy labels from the Pod if needed
 		for k, v := range pod.Labels {
 			workload.Labels[k] = v
 		}
+		workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name
 
 		// Add workload meta to labels
 		for k, v := range metaLabels {
@@ -370,15 +379,24 @@ func (r *WorkloadMonitorReconciler) Reconcile(ctx context.Context, req ctrl.Requ
 	monitor.Status.ObservedReplicas = observedReplicas
 	monitor.Status.AvailableReplicas = availableReplicas
 
-	// Default to operational = true, but check MinReplicas if set
-	monitor.Status.Operational = pointer.Bool(true)
-	if monitor.Spec.MinReplicas != nil && availableReplicas < *monitor.Spec.MinReplicas {
-		monitor.Status.Operational = pointer.Bool(false)
-	}
-
 	// Update the WorkloadMonitor status in the cluster
-	if err := r.Status().Update(ctx, monitor); err != nil {
-		logger.Error(err, "Unable to update WorkloadMonitor status", "monitor", monitor.Name)
+	err = retry.RetryOnConflict(retry.DefaultBackoff, func() error {
+		fresh := &cozyv1alpha1.WorkloadMonitor{}
+		if err := r.Get(ctx, req.NamespacedName, fresh); err != nil {
+			return err
+		}
+		fresh.Status.ObservedReplicas = observedReplicas
+		fresh.Status.AvailableReplicas = availableReplicas
+
+		// Default to operational = true, but check MinReplicas if set
+		monitor.Status.Operational = pointer.Bool(true)
+		if monitor.Spec.MinReplicas != nil && availableReplicas < *monitor.Spec.MinReplicas {
+			monitor.Status.Operational = pointer.Bool(false)
+		}
+		return r.Status().Update(ctx, fresh)
+	})
+	if err != nil {
+		logger.Error(err, "unable to update WorkloadMonitor status after retries")
 		return ctrl.Result{}, err
 	}