From 4fed427d8e29fcf8d7ecae793fc858641c83b8be Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Tue, 19 May 2026 12:34:56 -0400 Subject: [PATCH] OADP-7943: Fix DPA annotation changes not triggering reconciliation The veleroPredicate filtered updates by generation only. Kubernetes does not increment generation for annotation-only changes, so adding/removing the unsupported-velero-server-args or unsupported-node-agent-server-args annotations was silently ignored until the controller pod restarted. This commit: - Adds annotation change detection to veleroPredicate for the two functionally consumed DPA annotations - Moves the predicate from global WithEventFilter to For()-only so owned/watched resource events (ConfigMaps, Secrets) are no longer incorrectly blocked by generation filtering - Adds a ConfigMap Watch with EnqueueRequestsFromMapFunc that triggers reconciliation when a ConfigMap referenced by a DPA annotation changes - Adds unit tests for the predicate annotation change detection Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy Signed-off-by: Tiger Kaovilai --- .../dataprotectionapplication_controller.go | 32 ++- internal/controller/predicate.go | 29 ++- internal/controller/predicate_test.go | 217 ++++++++++++++++++ 3 files changed, 273 insertions(+), 5 deletions(-) create mode 100644 internal/controller/predicate_test.go diff --git a/internal/controller/dataprotectionapplication_controller.go b/internal/controller/dataprotectionapplication_controller.go index bbc2b86f9b9..73855d85ed2 100644 --- a/internal/controller/dataprotectionapplication_controller.go +++ b/internal/controller/dataprotectionapplication_controller.go @@ -35,13 +35,16 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/client-go/util/workqueue" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" oadpclient "github.com/openshift/oadp-operator/pkg/client" + "github.com/openshift/oadp-operator/pkg/common" ) // DataProtectionApplicationReconciler reconciles a DataProtectionApplication object @@ -165,7 +168,7 @@ func (r *DataProtectionApplicationReconciler) Reconcile(ctx context.Context, req // SetupWithManager sets up the controller with the Manager. func (r *DataProtectionApplicationReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). - For(&oadpv1alpha1.DataProtectionApplication{}). + For(&oadpv1alpha1.DataProtectionApplication{}, builder.WithPredicates(veleroPredicate(r.Scheme))). Owns(&appsv1.Deployment{}). Owns(&velerov1.BackupStorageLocation{}). Owns(&velerov1.VolumeSnapshotLocation{}). @@ -173,12 +176,37 @@ func (r *DataProtectionApplicationReconciler) SetupWithManager(mgr ctrl.Manager) Owns(&security.SecurityContextConstraints{}). Owns(&corev1.Service{}). Owns(&routev1.Route{}). + // Owns watches ConfigMaps created by the operator (with ownerReference to DPA). Owns(&corev1.ConfigMap{}). Watches(&corev1.Secret{}, &labelHandler{}). - WithEventFilter(veleroPredicate(r.Scheme)). + // Watches for user-created ConfigMaps referenced by DPA annotations + // (not owned by DPA, so Owns above does not cover them). + Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.mapUnsupportedArgsConfigMap)). Complete(r) } +// mapUnsupportedArgsConfigMap maps ConfigMap events to DPA reconcile requests +// when the ConfigMap is referenced by a DPA's unsupported server args annotation. +func (r *DataProtectionApplicationReconciler) mapUnsupportedArgsConfigMap(ctx context.Context, obj client.Object) []reconcile.Request { + var dpas oadpv1alpha1.DataProtectionApplicationList + if err := r.List(ctx, &dpas, client.InNamespace(obj.GetNamespace())); err != nil { + return nil + } + var requests []reconcile.Request + for i := range dpas.Items { + if dpas.Items[i].Annotations[common.UnsupportedVeleroServerArgsAnnotation] == obj.GetName() || + dpas.Items[i].Annotations[common.UnsupportedNodeAgentServerArgsAnnotation] == obj.GetName() { + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: dpas.Items[i].Name, + Namespace: dpas.Items[i].Namespace, + }, + }) + } + } + return requests +} + type labelHandler struct{} func (l *labelHandler) Create(ctx context.Context, evt event.TypedCreateEvent[client.Object], q workqueue.TypedRateLimitingInterface[reconcile.Request]) { diff --git a/internal/controller/predicate.go b/internal/controller/predicate.go index e187208a046..aaf3d8609d4 100644 --- a/internal/controller/predicate.go +++ b/internal/controller/predicate.go @@ -7,16 +7,22 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" + "github.com/openshift/oadp-operator/pkg/common" ) func veleroPredicate(scheme *runtime.Scheme) predicate.Predicate { return predicate.Funcs{ // Update returns true if the Update event should be processed UpdateFunc: func(e event.UpdateEvent) bool { - if e.ObjectOld.GetGeneration() == e.ObjectNew.GetGeneration() { - return false + if e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() { + return isObjectOurs(scheme, e.ObjectOld) } - return isObjectOurs(scheme, e.ObjectOld) + // Generation unchanged — still reconcile if a functionally + // consumed annotation was added, removed, or modified. + if hasRelevantAnnotationChange(e.ObjectOld, e.ObjectNew) { + return isObjectOurs(scheme, e.ObjectOld) + } + return false }, // Create returns true if the Create event should be processed CreateFunc: func(e event.CreateEvent) bool { @@ -29,6 +35,23 @@ func veleroPredicate(scheme *runtime.Scheme) predicate.Predicate { } } +// hasRelevantAnnotationChange returns true if any functionally consumed +// DPA annotation has changed between the old and new object. +func hasRelevantAnnotationChange(oldObj, newObj client.Object) bool { + relevantAnnotations := []string{ + common.UnsupportedVeleroServerArgsAnnotation, + common.UnsupportedNodeAgentServerArgsAnnotation, + } + oldAnnotations := oldObj.GetAnnotations() + newAnnotations := newObj.GetAnnotations() + for _, key := range relevantAnnotations { + if oldAnnotations[key] != newAnnotations[key] { + return true + } + } + return false +} + // isObjectOurs returns true if the object is ours. // it first checks if the object has our group, version, and kind // else it will check for non empty OadpOperatorlabel labels diff --git a/internal/controller/predicate_test.go b/internal/controller/predicate_test.go new file mode 100644 index 00000000000..3ef192979f4 --- /dev/null +++ b/internal/controller/predicate_test.go @@ -0,0 +1,217 @@ +package controller + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/event" + + oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" + "github.com/openshift/oadp-operator/pkg/common" +) + +func TestHasRelevantAnnotationChange(t *testing.T) { + tests := []struct { + name string + oldAnnotations map[string]string + newAnnotations map[string]string + want bool + }{ + { + name: "no annotations on either object", + oldAnnotations: nil, + newAnnotations: nil, + want: false, + }, + { + name: "velero server args annotation added", + oldAnnotations: nil, + newAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "my-configmap", + }, + want: true, + }, + { + name: "velero server args annotation removed", + oldAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "my-configmap", + }, + newAnnotations: nil, + want: true, + }, + { + name: "velero server args annotation value changed", + oldAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "old-configmap", + }, + newAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "new-configmap", + }, + want: true, + }, + { + name: "node agent server args annotation added", + oldAnnotations: nil, + newAnnotations: map[string]string{ + common.UnsupportedNodeAgentServerArgsAnnotation: "my-configmap", + }, + want: true, + }, + { + name: "node agent server args annotation removed", + oldAnnotations: map[string]string{ + common.UnsupportedNodeAgentServerArgsAnnotation: "my-configmap", + }, + newAnnotations: nil, + want: true, + }, + { + name: "irrelevant annotation changed", + oldAnnotations: map[string]string{ + "some-other-annotation": "old-value", + }, + newAnnotations: map[string]string{ + "some-other-annotation": "new-value", + }, + want: false, + }, + { + name: "relevant annotation unchanged, irrelevant changed", + oldAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "same-configmap", + "some-other-annotation": "old-value", + }, + newAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "same-configmap", + "some-other-annotation": "new-value", + }, + want: false, + }, + { + name: "both relevant annotations changed", + oldAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "old-velero-cm", + common.UnsupportedNodeAgentServerArgsAnnotation: "old-nodeagent-cm", + }, + newAnnotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "new-velero-cm", + common.UnsupportedNodeAgentServerArgsAnnotation: "new-nodeagent-cm", + }, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + oldObj := &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: tt.oldAnnotations, + }, + } + newObj := &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: tt.newAnnotations, + }, + } + got := hasRelevantAnnotationChange(oldObj, newObj) + if got != tt.want { + t.Errorf("hasRelevantAnnotationChange() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestVeleroPredicateUpdateFunc_AnnotationChange(t *testing.T) { + scheme := runtime.NewScheme() + _ = oadpv1alpha1.AddToScheme(scheme) + + pred := veleroPredicate(scheme) + + tests := []struct { + name string + old *oadpv1alpha1.DataProtectionApplication + new *oadpv1alpha1.DataProtectionApplication + want bool + }{ + { + name: "generation changed - should reconcile", + old: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + new: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{Generation: 2}, + }, + want: true, + }, + { + name: "generation same, no annotation change - should NOT reconcile", + old: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + new: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + want: false, + }, + { + name: "generation same, velero annotation added - should reconcile", + old: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + new: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Annotations: map[string]string{ + common.UnsupportedVeleroServerArgsAnnotation: "my-cm", + }, + }, + }, + want: true, + }, + { + name: "generation same, node agent annotation removed - should reconcile", + old: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Annotations: map[string]string{ + common.UnsupportedNodeAgentServerArgsAnnotation: "my-cm", + }, + }, + }, + new: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{Generation: 1}, + }, + want: true, + }, + { + name: "generation same, irrelevant annotation changed - should NOT reconcile", + old: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Annotations: map[string]string{"kubectl.kubernetes.io/last-applied": "old"}, + }, + }, + new: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Generation: 1, + Annotations: map[string]string{"kubectl.kubernetes.io/last-applied": "new"}, + }, + }, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + e := event.UpdateEvent{ + ObjectOld: tt.old, + ObjectNew: tt.new, + } + got := pred.Update(e) + if got != tt.want { + t.Errorf("veleroPredicate.Update() = %v, want %v", got, tt.want) + } + }) + } +}