Skip to content

Commit 42938ce

Browse files
openshift-merge-bot[bot]openshift-merge-bot[bot]
authored
Merge pull request #2208 from kaovilai/OADP-7829-fix-annotation-reconcile-1.4
OADP-7943, OADP-7829: Fix DPA annotation changes not triggering reconciliation
2 parents ed460aa + a1d8b12 commit 42938ce

3 files changed

Lines changed: 273 additions & 5 deletions

File tree

‎controllers/dpa_controller.go‎

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -32,13 +32,16 @@ import (
3232
"github.com/go-logr/logr"
3333
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
3434
oadpClient "github.com/openshift/oadp-operator/pkg/client"
35+
"github.com/openshift/oadp-operator/pkg/common"
3536
"k8s.io/apimachinery/pkg/runtime"
3637
"k8s.io/apimachinery/pkg/types"
3738
"k8s.io/client-go/tools/record"
3839
"k8s.io/client-go/util/workqueue"
3940
ctrl "sigs.k8s.io/controller-runtime"
41+
"sigs.k8s.io/controller-runtime/pkg/builder"
4042
"sigs.k8s.io/controller-runtime/pkg/client"
4143
"sigs.k8s.io/controller-runtime/pkg/event"
44+
"sigs.k8s.io/controller-runtime/pkg/handler"
4245
"sigs.k8s.io/controller-runtime/pkg/log"
4346
"sigs.k8s.io/controller-runtime/pkg/reconcile"
4447
)
@@ -139,20 +142,45 @@ func (r *DPAReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.R
139142
// SetupWithManager sets up the controller with the Manager.
140143
func (r *DPAReconciler) SetupWithManager(mgr ctrl.Manager) error {
141144
return ctrl.NewControllerManagedBy(mgr).
142-
For(&oadpv1alpha1.DataProtectionApplication{}).
145+
For(&oadpv1alpha1.DataProtectionApplication{}, builder.WithPredicates(veleroPredicate(r.Scheme))).
143146
Owns(&appsv1.Deployment{}).
144147
Owns(&velerov1.BackupStorageLocation{}).
145148
Owns(&velerov1.VolumeSnapshotLocation{}).
146149
Owns(&appsv1.DaemonSet{}).
147150
Owns(&security.SecurityContextConstraints{}).
148151
Owns(&corev1.Service{}).
149152
Owns(&routev1.Route{}).
153+
// Owns watches ConfigMaps created by the operator (with ownerReference to DPA).
150154
Owns(&corev1.ConfigMap{}).
151155
Watches(&corev1.Secret{}, &labelHandler{}).
152-
WithEventFilter(veleroPredicate(r.Scheme)).
156+
// Watches for user-created ConfigMaps referenced by DPA annotations
157+
// (not owned by DPA, so Owns above does not cover them).
158+
Watches(&corev1.ConfigMap{}, handler.EnqueueRequestsFromMapFunc(r.mapUnsupportedArgsConfigMap)).
153159
Complete(r)
154160
}
155161

162+
// mapUnsupportedArgsConfigMap maps ConfigMap events to DPA reconcile requests
163+
// when the ConfigMap is referenced by a DPA's unsupported server args annotation.
164+
func (r *DPAReconciler) mapUnsupportedArgsConfigMap(ctx context.Context, obj client.Object) []reconcile.Request {
165+
var dpas oadpv1alpha1.DataProtectionApplicationList
166+
if err := r.List(ctx, &dpas, client.InNamespace(obj.GetNamespace())); err != nil {
167+
return nil
168+
}
169+
var requests []reconcile.Request
170+
for i := range dpas.Items {
171+
if dpas.Items[i].Annotations[common.UnsupportedVeleroServerArgsAnnotation] == obj.GetName() ||
172+
dpas.Items[i].Annotations[common.UnsupportedNodeAgentServerArgsAnnotation] == obj.GetName() {
173+
requests = append(requests, reconcile.Request{
174+
NamespacedName: types.NamespacedName{
175+
Name: dpas.Items[i].Name,
176+
Namespace: dpas.Items[i].Namespace,
177+
},
178+
})
179+
}
180+
}
181+
return requests
182+
}
183+
156184
type labelHandler struct {
157185
}
158186

‎controllers/predicate.go‎

Lines changed: 26 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package controllers
22

33
import (
44
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
5+
"github.com/openshift/oadp-operator/pkg/common"
56
"k8s.io/apimachinery/pkg/runtime"
67
"sigs.k8s.io/controller-runtime/pkg/client"
78
"sigs.k8s.io/controller-runtime/pkg/event"
@@ -12,10 +13,15 @@ func veleroPredicate(scheme *runtime.Scheme) predicate.Predicate {
1213
return predicate.Funcs{
1314
// Update returns true if the Update event should be processed
1415
UpdateFunc: func(e event.UpdateEvent) bool {
15-
if e.ObjectOld.GetGeneration() == e.ObjectNew.GetGeneration() {
16-
return false
16+
if e.ObjectOld.GetGeneration() != e.ObjectNew.GetGeneration() {
17+
return isObjectOurs(scheme, e.ObjectOld)
1718
}
18-
return isObjectOurs(scheme, e.ObjectOld)
19+
// Generation unchanged — still reconcile if a functionally
20+
// consumed annotation was added, removed, or modified.
21+
if hasRelevantAnnotationChange(e.ObjectOld, e.ObjectNew) {
22+
return isObjectOurs(scheme, e.ObjectOld)
23+
}
24+
return false
1925
},
2026
// Create returns true if the Create event should be processed
2127
CreateFunc: func(e event.CreateEvent) bool {
@@ -28,6 +34,23 @@ func veleroPredicate(scheme *runtime.Scheme) predicate.Predicate {
2834
}
2935
}
3036

37+
// hasRelevantAnnotationChange returns true if any functionally consumed
38+
// DPA annotation has changed between the old and new object.
39+
func hasRelevantAnnotationChange(oldObj, newObj client.Object) bool {
40+
relevantAnnotations := []string{
41+
common.UnsupportedVeleroServerArgsAnnotation,
42+
common.UnsupportedNodeAgentServerArgsAnnotation,
43+
}
44+
oldAnnotations := oldObj.GetAnnotations()
45+
newAnnotations := newObj.GetAnnotations()
46+
for _, key := range relevantAnnotations {
47+
if oldAnnotations[key] != newAnnotations[key] {
48+
return true
49+
}
50+
}
51+
return false
52+
}
53+
3154
// isObjectOurs returns true if the object is ours.
3255
// it first checks if the object has our group, version, and kind
3356
// else it will check for non empty OadpOperatorlabel labels

‎controllers/predicate_test.go‎

Lines changed: 217 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,217 @@
1+
package controllers
2+
3+
import (
4+
"testing"
5+
6+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
7+
"k8s.io/apimachinery/pkg/runtime"
8+
"sigs.k8s.io/controller-runtime/pkg/event"
9+
10+
oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1"
11+
"github.com/openshift/oadp-operator/pkg/common"
12+
)
13+
14+
func TestHasRelevantAnnotationChange(t *testing.T) {
15+
tests := []struct {
16+
name string
17+
oldAnnotations map[string]string
18+
newAnnotations map[string]string
19+
want bool
20+
}{
21+
{
22+
name: "no annotations on either object",
23+
oldAnnotations: nil,
24+
newAnnotations: nil,
25+
want: false,
26+
},
27+
{
28+
name: "velero server args annotation added",
29+
oldAnnotations: nil,
30+
newAnnotations: map[string]string{
31+
common.UnsupportedVeleroServerArgsAnnotation: "my-configmap",
32+
},
33+
want: true,
34+
},
35+
{
36+
name: "velero server args annotation removed",
37+
oldAnnotations: map[string]string{
38+
common.UnsupportedVeleroServerArgsAnnotation: "my-configmap",
39+
},
40+
newAnnotations: nil,
41+
want: true,
42+
},
43+
{
44+
name: "velero server args annotation value changed",
45+
oldAnnotations: map[string]string{
46+
common.UnsupportedVeleroServerArgsAnnotation: "old-configmap",
47+
},
48+
newAnnotations: map[string]string{
49+
common.UnsupportedVeleroServerArgsAnnotation: "new-configmap",
50+
},
51+
want: true,
52+
},
53+
{
54+
name: "node agent server args annotation added",
55+
oldAnnotations: nil,
56+
newAnnotations: map[string]string{
57+
common.UnsupportedNodeAgentServerArgsAnnotation: "my-configmap",
58+
},
59+
want: true,
60+
},
61+
{
62+
name: "node agent server args annotation removed",
63+
oldAnnotations: map[string]string{
64+
common.UnsupportedNodeAgentServerArgsAnnotation: "my-configmap",
65+
},
66+
newAnnotations: nil,
67+
want: true,
68+
},
69+
{
70+
name: "irrelevant annotation changed",
71+
oldAnnotations: map[string]string{
72+
"some-other-annotation": "old-value",
73+
},
74+
newAnnotations: map[string]string{
75+
"some-other-annotation": "new-value",
76+
},
77+
want: false,
78+
},
79+
{
80+
name: "relevant annotation unchanged, irrelevant changed",
81+
oldAnnotations: map[string]string{
82+
common.UnsupportedVeleroServerArgsAnnotation: "same-configmap",
83+
"some-other-annotation": "old-value",
84+
},
85+
newAnnotations: map[string]string{
86+
common.UnsupportedVeleroServerArgsAnnotation: "same-configmap",
87+
"some-other-annotation": "new-value",
88+
},
89+
want: false,
90+
},
91+
{
92+
name: "both relevant annotations changed",
93+
oldAnnotations: map[string]string{
94+
common.UnsupportedVeleroServerArgsAnnotation: "old-velero-cm",
95+
common.UnsupportedNodeAgentServerArgsAnnotation: "old-nodeagent-cm",
96+
},
97+
newAnnotations: map[string]string{
98+
common.UnsupportedVeleroServerArgsAnnotation: "new-velero-cm",
99+
common.UnsupportedNodeAgentServerArgsAnnotation: "new-nodeagent-cm",
100+
},
101+
want: true,
102+
},
103+
}
104+
105+
for _, tt := range tests {
106+
t.Run(tt.name, func(t *testing.T) {
107+
oldObj := &oadpv1alpha1.DataProtectionApplication{
108+
ObjectMeta: metav1.ObjectMeta{
109+
Annotations: tt.oldAnnotations,
110+
},
111+
}
112+
newObj := &oadpv1alpha1.DataProtectionApplication{
113+
ObjectMeta: metav1.ObjectMeta{
114+
Annotations: tt.newAnnotations,
115+
},
116+
}
117+
got := hasRelevantAnnotationChange(oldObj, newObj)
118+
if got != tt.want {
119+
t.Errorf("hasRelevantAnnotationChange() = %v, want %v", got, tt.want)
120+
}
121+
})
122+
}
123+
}
124+
125+
func TestVeleroPredicateUpdateFunc_AnnotationChange(t *testing.T) {
126+
scheme := runtime.NewScheme()
127+
_ = oadpv1alpha1.AddToScheme(scheme)
128+
129+
pred := veleroPredicate(scheme)
130+
131+
tests := []struct {
132+
name string
133+
old *oadpv1alpha1.DataProtectionApplication
134+
new *oadpv1alpha1.DataProtectionApplication
135+
want bool
136+
}{
137+
{
138+
name: "generation changed - should reconcile",
139+
old: &oadpv1alpha1.DataProtectionApplication{
140+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
141+
},
142+
new: &oadpv1alpha1.DataProtectionApplication{
143+
ObjectMeta: metav1.ObjectMeta{Generation: 2},
144+
},
145+
want: true,
146+
},
147+
{
148+
name: "generation same, no annotation change - should NOT reconcile",
149+
old: &oadpv1alpha1.DataProtectionApplication{
150+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
151+
},
152+
new: &oadpv1alpha1.DataProtectionApplication{
153+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
154+
},
155+
want: false,
156+
},
157+
{
158+
name: "generation same, velero annotation added - should reconcile",
159+
old: &oadpv1alpha1.DataProtectionApplication{
160+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
161+
},
162+
new: &oadpv1alpha1.DataProtectionApplication{
163+
ObjectMeta: metav1.ObjectMeta{
164+
Generation: 1,
165+
Annotations: map[string]string{
166+
common.UnsupportedVeleroServerArgsAnnotation: "my-cm",
167+
},
168+
},
169+
},
170+
want: true,
171+
},
172+
{
173+
name: "generation same, node agent annotation removed - should reconcile",
174+
old: &oadpv1alpha1.DataProtectionApplication{
175+
ObjectMeta: metav1.ObjectMeta{
176+
Generation: 1,
177+
Annotations: map[string]string{
178+
common.UnsupportedNodeAgentServerArgsAnnotation: "my-cm",
179+
},
180+
},
181+
},
182+
new: &oadpv1alpha1.DataProtectionApplication{
183+
ObjectMeta: metav1.ObjectMeta{Generation: 1},
184+
},
185+
want: true,
186+
},
187+
{
188+
name: "generation same, irrelevant annotation changed - should NOT reconcile",
189+
old: &oadpv1alpha1.DataProtectionApplication{
190+
ObjectMeta: metav1.ObjectMeta{
191+
Generation: 1,
192+
Annotations: map[string]string{"kubectl.kubernetes.io/last-applied": "old"},
193+
},
194+
},
195+
new: &oadpv1alpha1.DataProtectionApplication{
196+
ObjectMeta: metav1.ObjectMeta{
197+
Generation: 1,
198+
Annotations: map[string]string{"kubectl.kubernetes.io/last-applied": "new"},
199+
},
200+
},
201+
want: false,
202+
},
203+
}
204+
205+
for _, tt := range tests {
206+
t.Run(tt.name, func(t *testing.T) {
207+
e := event.UpdateEvent{
208+
ObjectOld: tt.old,
209+
ObjectNew: tt.new,
210+
}
211+
got := pred.Update(e)
212+
if got != tt.want {
213+
t.Errorf("veleroPredicate.Update() = %v, want %v", got, tt.want)
214+
}
215+
})
216+
}
217+
}

0 commit comments

Comments
 (0)