Skip to content

Commit 15d86bb

Browse files
sjbermansjberman
committed
Reroll deployment when agent config is updated
1 parent f7ca041 commit 15d86bb

File tree

7 files changed

+275
-9
lines changed

7 files changed

+275
-9
lines changed

‎internal/framework/controller/labels.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,3 +7,6 @@ const (
77
AppInstanceLabel = "app.kubernetes.io/instance"
88
AppManagedByLabel = "app.kubernetes.io/managed-by"
99
)
10+
11+
// RestartedAnnotation is added to a Deployment or DaemonSet's PodSpec to trigger a rolling restart.
12+
const RestartedAnnotation = "kubectl.kubernetes.io/restartedAt"

‎internal/framework/controller/predicate/annotation.go

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,11 @@
11
package predicate
22

33
import (
4+
appsv1 "k8s.io/api/apps/v1"
45
"sigs.k8s.io/controller-runtime/pkg/event"
56
"sigs.k8s.io/controller-runtime/pkg/predicate"
7+
8+
"github.com/nginx/nginx-gateway-fabric/internal/framework/controller"
69
)
710

811
// AnnotationPredicate implements a predicate function based on the Annotation.
@@ -37,3 +40,39 @@ func (cp AnnotationPredicate) Update(e event.UpdateEvent) bool {
3740

3841
return oldAnnotationVal != newAnnotationVal
3942
}
43+
44+
// RestartDeploymentAnnotationPredicate skips update events if they are due to a rolling restart.
45+
// This type of event is triggered by adding an annotation to the deployment's PodSpec.
46+
// This is used by the provisioner to ensure it allows for rolling restarts of the nginx deployment
47+
// without reverting the annotation and deleting the new pod(s).
48+
type RestartDeploymentAnnotationPredicate struct {
49+
predicate.Funcs
50+
}
51+
52+
// Update filters UpdateEvents based on if the annotation is present or changed.
53+
func (cp RestartDeploymentAnnotationPredicate) Update(e event.UpdateEvent) bool {
54+
if e.ObjectOld == nil || e.ObjectNew == nil {
55+
// this case should not happen
56+
return false
57+
}
58+
59+
depOld, ok := e.ObjectOld.(*appsv1.Deployment)
60+
if !ok {
61+
return false
62+
}
63+
64+
depNew, ok := e.ObjectNew.(*appsv1.Deployment)
65+
if !ok {
66+
return false
67+
}
68+
69+
oldVal, oldExists := depOld.Spec.Template.Annotations[controller.RestartedAnnotation]
70+
71+
if newVal, ok := depNew.Spec.Template.Annotations[controller.RestartedAnnotation]; ok {
72+
if !oldExists || newVal != oldVal {
73+
return false
74+
}
75+
}
76+
77+
return true
78+
}

‎internal/framework/controller/predicate/annotation_test.go

Lines changed: 178 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,9 +4,13 @@ import (
44
"testing"
55

66
. "github.com/onsi/gomega"
7+
appsv1 "k8s.io/api/apps/v1"
8+
corev1 "k8s.io/api/core/v1"
79
apiext "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
810
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
911
"sigs.k8s.io/controller-runtime/pkg/event"
12+
13+
"github.com/nginx/nginx-gateway-fabric/internal/framework/controller"
1014
)
1115

1216
func TestAnnotationPredicate_Create(t *testing.T) {
@@ -222,3 +226,177 @@ func TestAnnotationPredicate_Update(t *testing.T) {
222226
})
223227
}
224228
}
229+
230+
func TestRestartDeploymentAnnotationPredicate_Update(t *testing.T) {
231+
t.Parallel()
232+
233+
tests := []struct {
234+
event event.UpdateEvent
235+
name string
236+
expUpdate bool
237+
}{
238+
{
239+
name: "annotation added",
240+
event: event.UpdateEvent{
241+
ObjectOld: &appsv1.Deployment{
242+
Spec: appsv1.DeploymentSpec{
243+
Template: corev1.PodTemplateSpec{
244+
ObjectMeta: metav1.ObjectMeta{
245+
Annotations: map[string]string{},
246+
},
247+
},
248+
},
249+
},
250+
ObjectNew: &appsv1.Deployment{
251+
Spec: appsv1.DeploymentSpec{
252+
Template: corev1.PodTemplateSpec{
253+
ObjectMeta: metav1.ObjectMeta{
254+
Annotations: map[string]string{
255+
controller.RestartedAnnotation: "true",
256+
},
257+
},
258+
},
259+
},
260+
},
261+
},
262+
expUpdate: false,
263+
},
264+
{
265+
name: "annotation changed",
266+
event: event.UpdateEvent{
267+
ObjectOld: &appsv1.Deployment{
268+
Spec: appsv1.DeploymentSpec{
269+
Template: corev1.PodTemplateSpec{
270+
ObjectMeta: metav1.ObjectMeta{
271+
Annotations: map[string]string{
272+
controller.RestartedAnnotation: "false",
273+
},
274+
},
275+
},
276+
},
277+
},
278+
ObjectNew: &appsv1.Deployment{
279+
Spec: appsv1.DeploymentSpec{
280+
Template: corev1.PodTemplateSpec{
281+
ObjectMeta: metav1.ObjectMeta{
282+
Annotations: map[string]string{
283+
controller.RestartedAnnotation: "true",
284+
},
285+
},
286+
},
287+
},
288+
},
289+
},
290+
expUpdate: false,
291+
},
292+
{
293+
name: "annotation removed",
294+
event: event.UpdateEvent{
295+
ObjectOld: &appsv1.Deployment{
296+
Spec: appsv1.DeploymentSpec{
297+
Template: corev1.PodTemplateSpec{
298+
ObjectMeta: metav1.ObjectMeta{
299+
Annotations: map[string]string{
300+
controller.RestartedAnnotation: "true",
301+
},
302+
},
303+
},
304+
},
305+
},
306+
ObjectNew: &appsv1.Deployment{
307+
Spec: appsv1.DeploymentSpec{
308+
Template: corev1.PodTemplateSpec{
309+
ObjectMeta: metav1.ObjectMeta{
310+
Annotations: map[string]string{},
311+
},
312+
},
313+
},
314+
},
315+
},
316+
expUpdate: true,
317+
},
318+
{
319+
name: "annotation unchanged",
320+
event: event.UpdateEvent{
321+
ObjectOld: &appsv1.Deployment{
322+
Spec: appsv1.DeploymentSpec{
323+
Template: corev1.PodTemplateSpec{
324+
ObjectMeta: metav1.ObjectMeta{
325+
Annotations: map[string]string{
326+
controller.RestartedAnnotation: "true",
327+
},
328+
},
329+
},
330+
},
331+
},
332+
ObjectNew: &appsv1.Deployment{
333+
Spec: appsv1.DeploymentSpec{
334+
Template: corev1.PodTemplateSpec{
335+
ObjectMeta: metav1.ObjectMeta{
336+
Annotations: map[string]string{
337+
controller.RestartedAnnotation: "true",
338+
},
339+
},
340+
},
341+
},
342+
},
343+
},
344+
expUpdate: true,
345+
},
346+
{
347+
name: "old object is nil",
348+
event: event.UpdateEvent{
349+
ObjectOld: nil,
350+
ObjectNew: &appsv1.Deployment{
351+
Spec: appsv1.DeploymentSpec{
352+
Template: corev1.PodTemplateSpec{
353+
ObjectMeta: metav1.ObjectMeta{
354+
Annotations: map[string]string{
355+
controller.RestartedAnnotation: "true",
356+
},
357+
},
358+
},
359+
},
360+
},
361+
},
362+
expUpdate: false,
363+
},
364+
{
365+
name: "new object is nil",
366+
event: event.UpdateEvent{
367+
ObjectOld: &appsv1.Deployment{
368+
Spec: appsv1.DeploymentSpec{
369+
Template: corev1.PodTemplateSpec{
370+
ObjectMeta: metav1.ObjectMeta{
371+
Annotations: map[string]string{
372+
controller.RestartedAnnotation: "true",
373+
},
374+
},
375+
},
376+
},
377+
},
378+
ObjectNew: nil,
379+
},
380+
expUpdate: false,
381+
},
382+
{
383+
name: "both objects are nil",
384+
event: event.UpdateEvent{
385+
ObjectOld: nil,
386+
ObjectNew: nil,
387+
},
388+
expUpdate: false,
389+
},
390+
}
391+
392+
p := RestartDeploymentAnnotationPredicate{}
393+
394+
for _, test := range tests {
395+
t.Run(test.name, func(t *testing.T) {
396+
t.Parallel()
397+
g := NewWithT(t)
398+
update := p.Update(test.event)
399+
g.Expect(update).To(Equal(test.expUpdate))
400+
})
401+
}
402+
}

‎internal/mode/static/handler.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -270,21 +270,21 @@ func (h *eventHandlerImpl) waitForStatusUpdates(ctx context.Context) {
270270
return
271271
}
272272

273+
// TODO(sberman): once we support multiple Gateways, we'll have to get
274+
// the correct Graph for the Deployment contained in the update message
275+
gr := h.cfg.processor.GetLatestGraph()
276+
if gr == nil {
277+
continue
278+
}
279+
273280
var nginxReloadRes graph.NginxReloadResult
274281
switch {
275282
case item.Error != nil:
276283
h.cfg.logger.Error(item.Error, "Failed to update NGINX configuration")
277284
nginxReloadRes.Error = item.Error
278-
default:
285+
case gr.Gateway != nil:
279286
h.cfg.logger.Info("NGINX configuration was successfully updated")
280287
}
281-
282-
// TODO(sberman): once we support multiple Gateways, we'll have to get
283-
// the correct Graph for the Deployment contained in the update message
284-
gr := h.cfg.processor.GetLatestGraph()
285-
if gr == nil {
286-
continue
287-
}
288288
gr.LatestReloadResult = nginxReloadRes
289289

290290
switch item.UpdateType {

‎internal/mode/static/provisioner/eventloop.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ func newEventLoop(
4343
k8spredicate.And(
4444
k8spredicate.GenerationChangedPredicate{},
4545
nginxResourceLabelPredicate,
46+
predicate.RestartDeploymentAnnotationPredicate{},
4647
),
4748
),
4849
},

‎internal/mode/static/provisioner/handler.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@ func (h *eventHandler) HandleEventBatch(ctx context.Context, logger logr.Logger,
5757
case *gatewayv1.Gateway:
5858
h.store.updateGateway(obj)
5959
case *appsv1.Deployment, *corev1.ServiceAccount, *corev1.ConfigMap:
60-
// TODO(sberman): if ConfigMap was updated, we need to reroll Deployment
6160
objLabels := labels.Set(obj.GetLabels())
6261
if h.labelSelector.Matches(objLabels) {
6362
gatewayName := objLabels.Get(controller.GatewayLabel)

‎internal/mode/static/provisioner/provisioner.go

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,13 @@ package provisioner
33
import (
44
"context"
55
"fmt"
6+
"strings"
67
"time"
78

89
"github.com/go-logr/logr"
910
"golang.org/x/text/cases"
1011
"golang.org/x/text/language"
12+
appsv1 "k8s.io/api/apps/v1"
1113
corev1 "k8s.io/api/core/v1"
1214
apierrors "k8s.io/apimachinery/pkg/api/errors"
1315
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -93,6 +95,7 @@ func NewNginxProvisioner(
9395
return provisioner, eventLoop, nil
9496
}
9597

98+
//nolint:gocyclo // will refactor at some point
9699
func (p *NginxProvisioner) provisionNginx(
97100
ctx context.Context,
98101
resourceName string,
@@ -107,6 +110,8 @@ func (p *NginxProvisioner) provisionNginx(
107110
"name", resourceName,
108111
)
109112

113+
var agentConfigMapUpdated, deploymentCreated bool
114+
var deploymentObj *appsv1.Deployment
110115
for _, obj := range objects {
111116
createCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
112117

@@ -145,6 +150,19 @@ func (p *NginxProvisioner) provisionNginx(
145150
continue
146151
}
147152

153+
switch o := obj.(type) {
154+
case *appsv1.Deployment:
155+
deploymentObj = o
156+
if res == controllerutil.OperationResultCreated {
157+
deploymentCreated = true
158+
}
159+
case *corev1.ConfigMap:
160+
if res == controllerutil.OperationResultUpdated &&
161+
strings.Contains(obj.GetName(), nginxAgentConfigMapNameSuffix) {
162+
agentConfigMapUpdated = true
163+
}
164+
}
165+
148166
result := cases.Title(language.English, cases.Compact).String(string(res))
149167
p.cfg.Logger.V(1).Info(
150168
fmt.Sprintf("%s nginx %s", result, obj.GetObjectKind().GroupVersionKind().Kind),
@@ -153,6 +171,34 @@ func (p *NginxProvisioner) provisionNginx(
153171
)
154172
}
155173

174+
// if agent configmap was updated, then we'll need to restart the deployment
175+
if agentConfigMapUpdated && !deploymentCreated {
176+
updateCtx, cancel := context.WithTimeout(ctx, 30*time.Second)
177+
defer cancel()
178+
179+
p.cfg.Logger.V(1).Info(
180+
"Restarting nginx deployment after agent configmap update",
181+
"name", deploymentObj.GetName(),
182+
"namespace", deploymentObj.GetNamespace(),
183+
)
184+
185+
if deploymentObj.Spec.Template.Annotations == nil {
186+
deploymentObj.Annotations = make(map[string]string)
187+
}
188+
deploymentObj.Spec.Template.Annotations[controller.RestartedAnnotation] = time.Now().Format(time.RFC3339)
189+
190+
if err := p.k8sClient.Update(updateCtx, deploymentObj); err != nil && !apierrors.IsConflict(err) {
191+
p.cfg.EventRecorder.Eventf(
192+
deploymentObj,
193+
corev1.EventTypeWarning,
194+
"RestartFailed",
195+
"Failed to restart nginx deployment after agent config update: %s",
196+
err.Error(),
197+
)
198+
return err
199+
}
200+
}
201+
156202
return nil
157203
}
158204

0 commit comments

Comments
 (0)