From 315c36db3ecda14b4100908646a5db91293ad0ee Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 21 Jul 2025 17:04:44 +0200 Subject: [PATCH 1/4] [cozystack-controller] Fix deleting workloads Signed-off-by: Andrei Kvapil --- internal/controller/workload_controller.go | 52 ++++------- .../controller/workload_controller_test.go | 87 +++++++++++++++++++ .../controller/workloadmonitor_controller.go | 21 ++++- 3 files changed, 120 insertions(+), 40 deletions(-) diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index e3e85b01..82607375 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -26,60 +26,38 @@ 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["workloadmonitor.cozystack.io/name"] + if !has { + return ctrl.Result{}, 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, types.NamespacedName{Namespace: w.Namespace, Name: monName}, monitor); apierrors.IsNotFound(err) { + return ctrl.Result{}, r.Delete(ctx, w) + } else if err != nil { + 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) } diff --git a/internal/controller/workload_controller_test.go b/internal/controller/workload_controller_test.go index 816c135e..b454adc3 100644 --- a/internal/controller/workload_controller_test.go +++ b/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) + } +} diff --git a/internal/controller/workloadmonitor_controller.go b/internal/controller/workloadmonitor_controller.go index 4408800e..561de772 100644 --- a/internal/controller/workloadmonitor_controller.go +++ b/internal/controller/workloadmonitor_controller.go @@ -137,10 +137,15 @@ 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 + if workload.Labels == nil { + workload.Labels = map[string]string{} + } + workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name + // Fill Workload status fields: workload.Status.Kind = monitor.Spec.Kind workload.Status.Type = monitor.Spec.Type @@ -184,7 +189,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) + + if workload.Labels == nil { + workload.Labels = map[string]string{} + } + workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name workload.Labels = pvc.Labels @@ -255,7 +265,12 @@ func (r *WorkloadMonitorReconciler) reconcilePodForMonitor( 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) + + if workload.Labels == nil { + workload.Labels = map[string]string{} + } + workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name // Copy labels from the Pod if needed for k, v := range pod.Labels { From 46662fe6bd29f88ef2444e7f6dffe3e53ebbb628 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 21 Jul 2025 17:54:33 +0200 Subject: [PATCH 2/4] [cozystack-controller] add retry.RetryOnConflict on updating status Signed-off-by: Andrei Kvapil --- .../controller/workloadmonitor_controller.go | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/internal/controller/workloadmonitor_controller.go b/internal/controller/workloadmonitor_controller.go index 561de772..aae108ed 100644 --- a/internal/controller/workloadmonitor_controller.go +++ b/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" @@ -385,15 +386,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 } From ee5a724374088128c2fc7d57feb3f4ea8e8d490c Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 21 Jul 2025 18:01:58 +0200 Subject: [PATCH 3/4] [cozystack-controller] ignore NotFound errors in Workload reconciler Signed-off-by: Andrei Kvapil --- internal/controller/workload_controller.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index 82607375..75dd9177 100644 --- a/internal/controller/workload_controller.go +++ b/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" @@ -44,9 +43,11 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c return ctrl.Result{}, r.Delete(ctx, w) } monitor := &cozyv1alpha1.WorkloadMonitor{} - if err := r.Get(ctx, types.NamespacedName{Namespace: w.Namespace, Name: monName}, monitor); apierrors.IsNotFound(err) { - return ctrl.Result{}, r.Delete(ctx, w) + 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 } From da3f233f87c1c177c98c6b608f45415275e30800 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 21 Jul 2025 18:19:50 +0200 Subject: [PATCH 4/4] [cozystack-controller] Refactor errors Signed-off-by: Andrei Kvapil --- internal/controller/workload_controller.go | 4 ++-- .../controller/workloadmonitor_controller.go | 23 +++++++------------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/internal/controller/workload_controller.go b/internal/controller/workload_controller.go index 75dd9177..d0d9e743 100644 --- a/internal/controller/workload_controller.go +++ b/internal/controller/workload_controller.go @@ -38,9 +38,9 @@ func (r *WorkloadReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c } // If my monitor is gone, delete me. - monName, has := w.Labels["workloadmonitor.cozystack.io/name"] + monName, has := w.Labels["workloads.cozystack.io/monitor"] if !has { - return ctrl.Result{}, r.Delete(ctx, w) + return ctrl.Result{}, client.IgnoreNotFound(r.Delete(ctx, w)) } monitor := &cozyv1alpha1.WorkloadMonitor{} if err := r.Get(ctx, client.ObjectKey{Namespace: w.Namespace, Name: monName}, monitor); apierrors.IsNotFound(err) { diff --git a/internal/controller/workloadmonitor_controller.go b/internal/controller/workloadmonitor_controller.go index aae108ed..d4d86b97 100644 --- a/internal/controller/workloadmonitor_controller.go +++ b/internal/controller/workloadmonitor_controller.go @@ -112,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)), }, } @@ -140,10 +141,8 @@ func (r *WorkloadMonitorReconciler) reconcileServiceForMonitor( // Update owner references with the new monitor updateOwnerReferences(workload.GetObjectMeta(), &svc) - workload.Labels = svc.Labels - - if workload.Labels == nil { - workload.Labels = map[string]string{} + for k, v := range svc.Labels { + workload.Labels[k] = v } workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name @@ -174,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)), }, } @@ -192,13 +192,11 @@ func (r *WorkloadMonitorReconciler) reconcilePVCForMonitor( // Update owner references with the new monitor updateOwnerReferences(workload.GetObjectMeta(), &pvc) - if workload.Labels == nil { - workload.Labels = map[string]string{} + for k, v := range pvc.Labels { + workload.Labels[k] = v } workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name - workload.Labels = pvc.Labels - // Fill Workload status fields: workload.Status.Kind = monitor.Spec.Kind workload.Status.Type = monitor.Spec.Type @@ -259,7 +257,7 @@ 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)), }, } @@ -268,15 +266,10 @@ func (r *WorkloadMonitorReconciler) reconcilePodForMonitor( // Update owner references with the new monitor updateOwnerReferences(workload.GetObjectMeta(), &pod) - if workload.Labels == nil { - workload.Labels = map[string]string{} - } - workload.Labels["workloads.cozystack.io/monitor"] = monitor.Name - - // 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 {