Skip to content

Commit ba7d532

Browse files
ChunyiLyuChunyiLyu
authored
Merge pull request #449 from rabbitmq/label-annotation-inheritance
Stop triggering pod restarts for CR labels/annotations updates - Stop populate CR labels/annotations to pods. Instance labels and annotations are still populated and updated for statefulSet. - Stop updating label/annotation on server conf configMap, because updates to server-conf configMap triggers pod restart. Instance labels and annotations are still applied at creation. - Stop updating label/annotation on plugins configmap, because updates to the plugins configMap triggers unnecessary `rabbitmq-plugins set`.
2 parents a456659 + 06a5c83 commit ba7d532

7 files changed

+183
-168
lines changed

‎controllers/rabbitmqcluster_controller_test.go

-29
Original file line numberDiff line numberDiff line change
@@ -533,35 +533,6 @@ var _ = Describe("RabbitmqClusterController", func() {
533533
return roleBinding.Annotations
534534
}, 3).Should(HaveKeyWithValue(annotationKey, annotationValue))
535535
})
536-
537-
It("updates annotations for config maps", func() {
538-
Eventually(func() map[string]string {
539-
roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("server-conf"), metav1.GetOptions{})
540-
Expect(err).NotTo(HaveOccurred())
541-
return roleBinding.Annotations
542-
}, 3).Should(HaveKeyWithValue(annotationKey, annotationValue))
543-
544-
Eventually(func() map[string]string {
545-
roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("plugins-conf"), metav1.GetOptions{})
546-
Expect(err).NotTo(HaveOccurred())
547-
return roleBinding.Annotations
548-
}, 3).Should(HaveKeyWithValue(annotationKey, annotationValue))
549-
})
550-
551-
It("updates annotations for config maps", func() {
552-
Eventually(func() map[string]string {
553-
roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("server-conf"), metav1.GetOptions{})
554-
Expect(err).NotTo(HaveOccurred())
555-
return roleBinding.Annotations
556-
}, 3).Should(HaveKeyWithValue(annotationKey, annotationValue))
557-
558-
Eventually(func() map[string]string {
559-
roleBinding, err := clientSet.CoreV1().ConfigMaps(cluster.Namespace).Get(ctx, cluster.ChildResourceName("plugins-conf"), metav1.GetOptions{})
560-
Expect(err).NotTo(HaveOccurred())
561-
return roleBinding.Annotations
562-
}, 3).Should(HaveKeyWithValue(annotationKey, annotationValue))
563-
})
564-
565536
})
566537

567538
It("service type is updated", func() {

‎internal/resource/configmap.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -55,16 +55,16 @@ func (builder *RabbitmqResourceBuilder) ServerConfigMap() *ServerConfigMapBuilde
5555
func (builder *ServerConfigMapBuilder) Build() (runtime.Object, error) {
5656
return &corev1.ConfigMap{
5757
ObjectMeta: metav1.ObjectMeta{
58-
Name: builder.Instance.ChildResourceName(ServerConfigMapName),
59-
Namespace: builder.Instance.Namespace,
58+
Name: builder.Instance.ChildResourceName(ServerConfigMapName),
59+
Namespace: builder.Instance.Namespace,
60+
Labels: metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels),
61+
Annotations: metadata.ReconcileAndFilterAnnotations(nil, builder.Instance.Annotations),
6062
},
6163
}, nil
6264
}
6365

6466
func (builder *ServerConfigMapBuilder) Update(object runtime.Object) error {
6567
configMap := object.(*corev1.ConfigMap)
66-
configMap.Labels = metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels)
67-
configMap.Annotations = metadata.ReconcileAndFilterAnnotations(configMap.GetAnnotations(), builder.Instance.Annotations)
6868

6969
ini.PrettySection = false // Remove trailing new line because rabbitmq.conf has only a default section.
7070
cfg, err := ini.Load([]byte(defaultRabbitmqConf))

‎internal/resource/configmap_test.go

+67-86
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,22 @@ var _ = Describe("GenerateServerConfigMap", func() {
6666
var configMap *corev1.ConfigMap
6767

6868
BeforeEach(func() {
69+
instance.Labels = map[string]string{
70+
"app.kubernetes.io/foo": "bar",
71+
"foo": "bar",
72+
"rabbitmq": "is-great",
73+
"foo/app.kubernetes.io": "edgecase",
74+
}
75+
instance.Annotations = map[string]string{
76+
"my-annotation": "i-like-this",
77+
"kubernetes.io/name": "i-do-not-like-this",
78+
"kubectl.kubernetes.io/name": "i-do-not-like-this",
79+
"k8s.io/name": "i-do-not-like-this",
80+
"kubernetes.io/other": "i-do-not-like-this",
81+
"kubectl.kubernetes.io/other": "i-do-not-like-this",
82+
"k8s.io/other": "i-do-not-like-this",
83+
}
84+
6985
obj, err := configMapBuilder.Build()
7086
configMap = obj.(*corev1.ConfigMap)
7187
Expect(err).NotTo(HaveOccurred())
@@ -75,6 +91,32 @@ var _ = Describe("GenerateServerConfigMap", func() {
7591
Expect(configMap.Name).To(Equal(builder.Instance.ChildResourceName("server-conf")))
7692
Expect(configMap.Namespace).To(Equal(builder.Instance.Namespace))
7793
})
94+
95+
It("adds labels from the instance and default labels", func() {
96+
Expect(configMap.Labels).To(SatisfyAll(
97+
HaveLen(6),
98+
HaveKeyWithValue("foo", "bar"),
99+
HaveKeyWithValue("rabbitmq", "is-great"),
100+
HaveKeyWithValue("foo/app.kubernetes.io", "edgecase"),
101+
HaveKeyWithValue("app.kubernetes.io/name", instance.Name),
102+
HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"),
103+
HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"),
104+
Not(HaveKey("app.kubernetes.io/foo")),
105+
))
106+
})
107+
108+
It("adds annotations from the instance", func() {
109+
Expect(configMap.Annotations).To(SatisfyAll(
110+
HaveLen(1),
111+
HaveKeyWithValue("my-annotation", "i-like-this"),
112+
Not(HaveKey("kubernetes.io/name")),
113+
Not(HaveKey("kubectl.kubernetes.io/name")),
114+
Not(HaveKey("k8s.io/name")),
115+
Not(HaveKey("kubernetes.io/other")),
116+
Not(HaveKey("kubectl.kubernetes.io/other")),
117+
Not(HaveKey("k8s.io/other")),
118+
))
119+
})
78120
})
79121

80122
Context("Update", func() {
@@ -284,94 +326,33 @@ ssl_options.verify = verify_peer
284326
})
285327
})
286328

287-
Context("labels", func() {
288-
BeforeEach(func() {
289-
instance = rabbitmqv1beta1.RabbitmqCluster{
290-
ObjectMeta: metav1.ObjectMeta{
291-
Name: "rabbit-labelled",
292-
},
293-
}
294-
instance.Labels = map[string]string{
295-
"app.kubernetes.io/foo": "bar",
296-
"foo": "bar",
297-
"rabbitmq": "is-great",
298-
"foo/app.kubernetes.io": "edgecase",
299-
}
300-
301-
configMap = &corev1.ConfigMap{
302-
ObjectMeta: metav1.ObjectMeta{
303-
Labels: map[string]string{
304-
"app.kubernetes.io/name": instance.Name,
305-
"app.kubernetes.io/part-of": "rabbitmq",
306-
"this-was-the-previous-label": "should-be-deleted",
307-
},
308-
},
309-
}
310-
err := configMapBuilder.Update(configMap)
311-
Expect(err).NotTo(HaveOccurred())
312-
})
313-
314-
It("adds labels from the CR", func() {
315-
testLabels(configMap.Labels)
316-
})
317-
318-
It("restores the default labels", func() {
319-
labels := configMap.Labels
320-
Expect(labels["app.kubernetes.io/name"]).To(Equal(instance.Name))
321-
Expect(labels["app.kubernetes.io/component"]).To(Equal("rabbitmq"))
322-
Expect(labels["app.kubernetes.io/part-of"]).To(Equal("rabbitmq"))
323-
})
324-
325-
It("deletes the labels that are removed from the CR", func() {
326-
Expect(configMap.Labels).NotTo(HaveKey("this-was-the-previous-label"))
327-
})
329+
// this is to ensure that pods are not restarted when instance labels are updated
330+
It("does not update labels on the config map", func() {
331+
configMap.Labels = map[string]string{
332+
"app.kubernetes.io/name": instance.Name,
333+
"app.kubernetes.io/component": "rabbitmq",
334+
"app.kubernetes.io/part-of": "rabbitmq",
335+
}
336+
instance.Labels = map[string]string{
337+
"new-label": "test",
338+
}
339+
Expect(configMapBuilder.Update(configMap)).To(Succeed())
340+
Expect(configMap.Labels).To(SatisfyAll(
341+
HaveLen(3),
342+
HaveKeyWithValue("app.kubernetes.io/name", instance.Name),
343+
HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"),
344+
HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"),
345+
Not(HaveKey("new-label")),
346+
))
328347
})
329348

330-
Context("instance annotations", func() {
331-
BeforeEach(func() {
332-
instance = rabbitmqv1beta1.RabbitmqCluster{
333-
ObjectMeta: metav1.ObjectMeta{
334-
Name: "rabbit-labelled",
335-
},
336-
}
337-
instance.Annotations = map[string]string{
338-
"my-annotation": "i-like-this",
339-
"kubernetes.io/name": "i-do-not-like-this",
340-
"kubectl.kubernetes.io/name": "i-do-not-like-this",
341-
"k8s.io/name": "i-do-not-like-this",
342-
"kubernetes.io/other": "i-do-not-like-this",
343-
"kubectl.kubernetes.io/other": "i-do-not-like-this",
344-
"k8s.io/other": "i-do-not-like-this",
345-
}
346-
347-
configMap = &corev1.ConfigMap{
348-
ObjectMeta: metav1.ObjectMeta{
349-
Annotations: map[string]string{
350-
"my-annotation": "i-will-not-stay",
351-
"old-annotation": "old-value",
352-
"im-here-to-stay.kubernetes.io": "for-a-while",
353-
"kubernetes.io/name": "should-stay",
354-
"kubectl.kubernetes.io/name": "should-stay",
355-
"k8s.io/name": "should-stay",
356-
},
357-
},
358-
}
359-
err := configMapBuilder.Update(configMap)
360-
Expect(err).NotTo(HaveOccurred())
361-
})
362-
363-
It("updates config map annotations", func() {
364-
expectedAnnotations := map[string]string{
365-
"my-annotation": "i-like-this",
366-
"old-annotation": "old-value",
367-
"im-here-to-stay.kubernetes.io": "for-a-while",
368-
"kubernetes.io/name": "should-stay",
369-
"kubectl.kubernetes.io/name": "should-stay",
370-
"k8s.io/name": "should-stay",
371-
}
372-
373-
Expect(configMap.Annotations).To(Equal(expectedAnnotations))
374-
})
349+
// this is to ensure that pods are not restarted when instance annotations are updated
350+
It("does not update annotations on the config map", func() {
351+
instance.Annotations = map[string]string{
352+
"new-annotation": "test",
353+
}
354+
Expect(configMapBuilder.Update(configMap)).To(Succeed())
355+
Expect(configMap.Annotations).To(BeEmpty())
375356
})
376357
})
377358
})

‎internal/resource/rabbitmq_plugins.go

+4-4
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,10 @@ func (builder *RabbitmqResourceBuilder) RabbitmqPluginsConfigMap() *RabbitmqPlug
3636
func (builder *RabbitmqPluginsConfigMapBuilder) Build() (runtime.Object, error) {
3737
return &corev1.ConfigMap{
3838
ObjectMeta: metav1.ObjectMeta{
39-
Name: builder.Instance.ChildResourceName(PluginsConfigName),
40-
Namespace: builder.Instance.Namespace,
39+
Name: builder.Instance.ChildResourceName(PluginsConfigName),
40+
Namespace: builder.Instance.Namespace,
41+
Labels: metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels),
42+
Annotations: metadata.ReconcileAndFilterAnnotations(nil, builder.Instance.Annotations),
4143
},
4244
Data: map[string]string{
4345
"enabled_plugins": desiredPluginsAsString([]rabbitmqv1beta1.Plugin{}),
@@ -47,8 +49,6 @@ func (builder *RabbitmqPluginsConfigMapBuilder) Build() (runtime.Object, error)
4749

4850
func (builder *RabbitmqPluginsConfigMapBuilder) Update(object runtime.Object) error {
4951
configMap := object.(*corev1.ConfigMap)
50-
configMap.Labels = metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels)
51-
configMap.Annotations = metadata.ReconcileAndFilterAnnotations(configMap.GetAnnotations(), builder.Instance.Annotations)
5252

5353
if configMap.Data == nil {
5454
configMap.Data = make(map[string]string)

‎internal/resource/rabbitmq_plugins_test.go

+70
Original file line numberDiff line numberDiff line change
@@ -90,6 +90,22 @@ var _ = Describe("RabbitMQPlugins", func() {
9090
var configMap *corev1.ConfigMap
9191

9292
BeforeEach(func() {
93+
instance.Labels = map[string]string{
94+
"app.kubernetes.io/foo": "bar",
95+
"foo": "bar",
96+
"rabbitmq": "is-great",
97+
"foo/app.kubernetes.io": "edgecase",
98+
}
99+
instance.Annotations = map[string]string{
100+
"my-annotation": "i-like-this",
101+
"kubernetes.io/name": "i-do-not-like-this",
102+
"kubectl.kubernetes.io/name": "i-do-not-like-this",
103+
"k8s.io/name": "i-do-not-like-this",
104+
"kubernetes.io/other": "i-do-not-like-this",
105+
"kubectl.kubernetes.io/other": "i-do-not-like-this",
106+
"k8s.io/other": "i-do-not-like-this",
107+
}
108+
93109
obj, err := configMapBuilder.Build()
94110
configMap = obj.(*corev1.ConfigMap)
95111
Expect(err).NotTo(HaveOccurred())
@@ -112,6 +128,32 @@ var _ = Describe("RabbitMQPlugins", func() {
112128
configMap = obj.(*corev1.ConfigMap)
113129
Expect(configMap.Data).To(HaveKeyWithValue("enabled_plugins", expectedEnabledPlugins))
114130
})
131+
132+
It("adds labels from the instance and default labels", func() {
133+
Expect(configMap.Labels).To(SatisfyAll(
134+
HaveLen(6),
135+
HaveKeyWithValue("foo", "bar"),
136+
HaveKeyWithValue("rabbitmq", "is-great"),
137+
HaveKeyWithValue("foo/app.kubernetes.io", "edgecase"),
138+
HaveKeyWithValue("app.kubernetes.io/name", instance.Name),
139+
HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"),
140+
HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"),
141+
Not(HaveKey("app.kubernetes.io/foo")),
142+
))
143+
})
144+
145+
It("adds annotations from the instance", func() {
146+
Expect(configMap.Annotations).To(SatisfyAll(
147+
HaveLen(1),
148+
HaveKeyWithValue("my-annotation", "i-like-this"),
149+
Not(HaveKey("kubernetes.io/name")),
150+
Not(HaveKey("kubectl.kubernetes.io/name")),
151+
Not(HaveKey("k8s.io/name")),
152+
Not(HaveKey("kubernetes.io/other")),
153+
Not(HaveKey("kubectl.kubernetes.io/other")),
154+
Not(HaveKey("k8s.io/other")),
155+
))
156+
})
115157
})
116158

117159
Context("Update", func() {
@@ -177,6 +219,34 @@ var _ = Describe("RabbitMQPlugins", func() {
177219
})
178220
})
179221
})
222+
223+
// ensures that we are not unnecessarily running `rabbitmq-plugins set` when CR labels are updated
224+
It("does not update labels on the config map", func() {
225+
configMap.Labels = map[string]string{
226+
"app.kubernetes.io/name": instance.Name,
227+
"app.kubernetes.io/component": "rabbitmq",
228+
"app.kubernetes.io/part-of": "rabbitmq",
229+
}
230+
instance.Labels = map[string]string{
231+
"new-label": "test",
232+
}
233+
Expect(configMapBuilder.Update(configMap)).To(Succeed())
234+
Expect(configMap.Labels).To(SatisfyAll(
235+
HaveLen(3),
236+
HaveKeyWithValue("app.kubernetes.io/name", instance.Name),
237+
HaveKeyWithValue("app.kubernetes.io/component", "rabbitmq"),
238+
HaveKeyWithValue("app.kubernetes.io/part-of", "rabbitmq"),
239+
Not(HaveKey("new-label")),
240+
))
241+
})
242+
// ensures that we are not unnecessarily running `rabbitmq-plugins set` when CR annotations are updated
243+
It("does not update annotations on the config map", func() {
244+
instance.Annotations = map[string]string{
245+
"new-annotation": "test",
246+
}
247+
Expect(configMapBuilder.Update(configMap)).To(Succeed())
248+
Expect(configMap.Annotations).To(BeEmpty())
249+
})
180250
})
181251
})
182252
})

‎internal/resource/statefulset.go

+12-11
Original file line numberDiff line numberDiff line change
@@ -114,17 +114,12 @@ func (builder *StatefulSetBuilder) Update(object runtime.Object) error {
114114

115115
//Annotations
116116
sts.Annotations = metadata.ReconcileAndFilterAnnotations(sts.Annotations, builder.Instance.Annotations)
117-
defaultPodAnnotations := map[string]string{
118-
"prometheus.io/scrape": "true",
119-
"prometheus.io/port": "15692",
120-
}
121-
podAnnotations := metadata.ReconcileAnnotations(defaultPodAnnotations, metadata.ReconcileAndFilterAnnotations(sts.Spec.Template.Annotations, builder.Instance.Annotations))
122117

123118
//Labels
124-
updatedLabels := metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels)
125-
sts.Labels = updatedLabels
119+
sts.Labels = metadata.GetLabels(builder.Instance.Name, builder.Instance.Labels)
126120

127-
sts.Spec.Template = builder.podTemplateSpec(podAnnotations, updatedLabels)
121+
// pod template
122+
sts.Spec.Template = builder.podTemplateSpec(sts.Spec.Template.Annotations)
128123

129124
if !sts.Spec.Template.Spec.Containers[0].Resources.Limits.Memory().Equal(*sts.Spec.Template.Spec.Containers[0].Resources.Requests.Memory()) {
130125
logger := ctrl.Log.WithName("statefulset").WithName("RabbitmqCluster")
@@ -261,7 +256,13 @@ func sortEnvVar(envVar []corev1.EnvVar) {
261256
}
262257
}
263258

264-
func (builder *StatefulSetBuilder) podTemplateSpec(annotations, labels map[string]string) corev1.PodTemplateSpec {
259+
func (builder *StatefulSetBuilder) podTemplateSpec(previousPodAnnotations map[string]string) corev1.PodTemplateSpec {
260+
// default pod annotations used for prometheus metrics
261+
defaultPodAnnotations := map[string]string{
262+
"prometheus.io/scrape": "true",
263+
"prometheus.io/port": "15692",
264+
}
265+
265266
//Init Container resources
266267
cpuRequest := k8sresource.MustParse(initContainerCPU)
267268
memoryRequest := k8sresource.MustParse(initContainerMemory)
@@ -510,8 +511,8 @@ func (builder *StatefulSetBuilder) podTemplateSpec(annotations, labels map[strin
510511

511512
return corev1.PodTemplateSpec{
512513
ObjectMeta: metav1.ObjectMeta{
513-
Annotations: annotations,
514-
Labels: labels,
514+
Annotations: metadata.ReconcileAnnotations(defaultPodAnnotations, previousPodAnnotations),
515+
Labels: metadata.Label(builder.Instance.Name),
515516
},
516517
Spec: corev1.PodSpec{
517518
TopologySpreadConstraints: []corev1.TopologySpreadConstraint{

0 commit comments

Comments
 (0)