From 74a73123eed5c7b27abce3785d0c5657bf693766 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Wed, 5 Aug 2026 11:19:50 +0200 Subject: [PATCH 1/3] fix(deployd): force resynchronization of Application and Naisjob Applications and Naisjobs gained a status subresource in liberator bacd0593. Before that, deployd's update replaced the whole resource and wiped .status, so Naiserator always found a hash mismatch and synchronized. Now the API server keeps .status, an unchanged spec matches the stored hash, and Naiserator skips synchronization without emitting an event. The deployment then waits until it times out. Clear the hash through the status subresource after writing the spec, so every deployment resynchronizes regardless of spec changes. This also unblocks resources stuck in FailedSynchronization, which persist their hash and otherwise never reconcile again without a spec change. Ignore Naiserator's no-op rollout event while waiting. It reports the state from before the forced resynchronization, and accepting it would report success without observing the rollout it triggers. --- pkg/deployd/strategy/deploy.go | 27 ++++++ pkg/deployd/strategy/deploy_test.go | 145 ++++++++++++++++++++++++++++ pkg/deployd/strategy/nais.go | 11 +++ 3 files changed, 183 insertions(+) create mode 100644 pkg/deployd/strategy/deploy_test.go diff --git a/pkg/deployd/strategy/deploy.go b/pkg/deployd/strategy/deploy.go index 9d93bab0..054c21c9 100644 --- a/pkg/deployd/strategy/deploy.go +++ b/pkg/deployd/strategy/deploy.go @@ -9,6 +9,7 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" ) @@ -46,9 +47,35 @@ func (c createOrUpdateStrategy) Deploy(ctx context.Context, resource unstructure return nil, fmt.Errorf("updating resource: %w", transformStrictDecodingError(resource, err)) } + if shouldInvalidateSynchronizationHash(resource) { + err = c.invalidateSynchronizationHash(ctx, resource.GetName()) + if err != nil { + return nil, fmt.Errorf("invalidating synchronization hash: %w", err) + } + trace.AddEvent("Forced resynchronization of Nais resource") + } + return updated, nil } +func shouldInvalidateSynchronizationHash(resource unstructured.Unstructured) bool { + gvk := resource.GroupVersionKind() + return gvk.Group == "nais.io" && (gvk.Kind == "Application" || gvk.Kind == "Naisjob") +} + +// invalidateSynchronizationHash clears the hash Naiserator compares against to decide +// whether a resource needs synchronization. Without this, redeploying an unchanged spec +// is a no-op, and the deployment waits for a rollout event that never arrives. +// +// The resource spec is written before this call, so the resynchronization picks up the +// spec and correlation ID from this deployment. Only the hash field is patched, leaving +// the rest of the operator-owned status untouched. +func (c createOrUpdateStrategy) invalidateSynchronizationHash(ctx context.Context, name string) error { + _, err := c.client.Patch(ctx, name, types.MergePatchType, + []byte(`{"status":{"synchronizationHash":null}}`), metav1.PatchOptions{}, "status") + return err +} + func transformStrictDecodingError(resource unstructured.Unstructured, err error) error { msg := err.Error() diff --git a/pkg/deployd/strategy/deploy_test.go b/pkg/deployd/strategy/deploy_test.go new file mode 100644 index 00000000..ce48cea3 --- /dev/null +++ b/pkg/deployd/strategy/deploy_test.go @@ -0,0 +1,145 @@ +package strategy + +import ( + "testing" + + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/trace/noop" + "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/dynamic/fake" + k8stesting "k8s.io/client-go/testing" +) + +var ( + applicationGVK = schema.GroupVersionKind{Group: "nais.io", Version: "v1alpha1", Kind: "Application"} + applicationGVR = schema.GroupVersionResource{Group: "nais.io", Version: "v1alpha1", Resource: "applications"} + naisjobGVK = schema.GroupVersionKind{Group: "nais.io", Version: "v1", Kind: "Naisjob"} + naisjobGVR = schema.GroupVersionResource{Group: "nais.io", Version: "v1", Resource: "naisjobs"} +) + +// Redeploying an unchanged spec must still trigger Naiserator, which only synchronizes +// when its stored hash differs from the hash it computes. +func TestDeployInvalidatesSynchronizationHash(t *testing.T) { + tests := []struct { + name string + gvk schema.GroupVersionKind + gvr schema.GroupVersionResource + }{ + {name: "Application", gvk: applicationGVK, gvr: applicationGVR}, + {name: "Naisjob", gvk: naisjobGVK, gvr: naisjobGVR}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + existing := testResource(tt.gvk) + require.NoError(t, unstructured.SetNestedField(existing.Object, "current-hash", "status", "synchronizationHash")) + desired := existing.DeepCopy() + + client := fake.NewSimpleDynamicClient(runtime.NewScheme(), &existing) + resourceClient := client.Resource(tt.gvr).Namespace(existing.GetNamespace()) + + deployed, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) + require.NoError(t, err) + require.NotNil(t, deployed) + + // The spec must be written before the hash is cleared, so that the + // resynchronization picks up this deployment rather than the previous one. + require.Equal(t, []string{"get", "update", "patch"}, verbs(client.Actions())) + + patch := lastPatch(t, client.Actions()) + require.Equal(t, "status", patch.GetSubresource()) + require.Equal(t, types.MergePatchType, patch.GetPatchType()) + require.JSONEq(t, `{"status":{"synchronizationHash":null}}`, string(patch.GetPatch())) + }) + } +} + +func TestDeployDoesNotInvalidateSynchronizationHash(t *testing.T) { + tests := []struct { + name string + gvk schema.GroupVersionKind + gvr schema.GroupVersionResource + existing bool + }{ + { + name: "new Application is synchronized on creation", + gvk: applicationGVK, + gvr: applicationGVR, + }, + { + name: "ConfigMap has no synchronization hash", + gvk: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, + gvr: schema.GroupVersionResource{Version: "v1", Resource: "configmaps"}, + existing: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resource := testResource(tt.gvk) + client := fake.NewSimpleDynamicClient(runtime.NewScheme()) + resourceClient := client.Resource(tt.gvr).Namespace(resource.GetNamespace()) + if tt.existing { + _, err := resourceClient.Create(t.Context(), &resource, metav1.CreateOptions{}) + require.NoError(t, err) + client.ClearActions() + } + + _, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), resource, noop.Span{}) + require.NoError(t, err) + require.NotContains(t, verbs(client.Actions()), "patch") + }) + } +} + +// A resource whose hash was not cleared silently stops reconciling, so the deployment +// must fail loudly rather than wait for a rollout that never happens. +func TestDeployFailsWhenSynchronizationHashCannotBeInvalidated(t *testing.T) { + existing := testResource(applicationGVK) + desired := existing.DeepCopy() + + client := fake.NewSimpleDynamicClient(runtime.NewScheme(), &existing) + client.PrependReactor("patch", "applications", func(k8stesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.NewForbidden(applicationGVR.GroupResource(), existing.GetName(), nil) + }) + resourceClient := client.Resource(applicationGVR).Namespace(existing.GetNamespace()) + + _, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) + require.ErrorContains(t, err, "invalidating synchronization hash") + require.True(t, errors.IsForbidden(err)) +} + +func verbs(actions []k8stesting.Action) []string { + found := make([]string, 0, len(actions)) + for _, action := range actions { + found = append(found, action.GetVerb()) + } + return found +} + +func lastPatch(t *testing.T, actions []k8stesting.Action) k8stesting.PatchAction { + t.Helper() + for i := len(actions) - 1; i >= 0; i-- { + if patch, ok := actions[i].(k8stesting.PatchAction); ok { + return patch + } + } + t.Fatal("expected a patch action") + return nil +} + +func testResource(gvk schema.GroupVersionKind) unstructured.Unstructured { + resource := unstructured.Unstructured{Object: map[string]any{ + "metadata": map[string]any{ + "name": "test-resource", + "namespace": "test-namespace", + }, + }} + resource.SetGroupVersionKind(gvk) + return resource +} diff --git a/pkg/deployd/strategy/nais.go b/pkg/deployd/strategy/nais.go index bd487480..989639ca 100644 --- a/pkg/deployd/strategy/nais.go +++ b/pkg/deployd/strategy/nais.go @@ -23,6 +23,12 @@ type naisResource struct { client kubeclient.Interface } +// rolloutMessageNoop mirrors Naiserator's RolloutMessageNoop, which it emits when a +// redeploy has no spec changes. deployd forces a resynchronization on every deploy, so +// this message describes the state before that resynchronization and must not finish +// the wait. The real rollout event follows. +const rolloutMessageNoop = "No changes; deployment already up to date" + func (a naisResource) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span) *pb.DeploymentStatus { var err error @@ -73,6 +79,11 @@ func (a naisResource) Watch(op *operation.Operation, resource unstructured.Unstr continue } + if event.Message == rolloutMessageNoop { + op.Logger.Tracef("Ignoring no-op rollout event %s; awaiting forced resynchronization", event.Name) + continue + } + status := StatusFromEvent(event, op.Request) if status == nil { return pb.NewFailureStatus(op.Request, fmt.Errorf("this application has been redeployed, aborting monitoring")) From c661839c14d24bce7da2b9c94f17b5b310129f94 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Wed, 5 Aug 2026 11:20:05 +0200 Subject: [PATCH 2/3] test(deployd): cover forced resynchronization against envtest The envtest rig loads liberator CRDs that declare the status subresource, so these cases reproduce the timeout that plain unit tests cannot: an update no longer wipes .status, and only the deliberate patch clears the synchronization hash. Cover Naisjob as well, which had no coverage, and assert that the rest of the status survives so the patch stays scoped to the one field. --- pkg/deployd/deployd/deployd_test.go | 153 +++++++++++++++++- .../deployd/testdata/application-noop.json | 13 ++ .../deployd/testdata/application-resync.json | 13 ++ .../deployd/testdata/naisjob-resync.json | 14 ++ 4 files changed, 187 insertions(+), 6 deletions(-) create mode 100644 pkg/deployd/deployd/testdata/application-noop.json create mode 100644 pkg/deployd/deployd/testdata/application-resync.json create mode 100644 pkg/deployd/deployd/testdata/naisjob-resync.json diff --git a/pkg/deployd/deployd/deployd_test.go b/pkg/deployd/deployd/deployd_test.go index da10d3f6..b99e59e3 100644 --- a/pkg/deployd/deployd/deployd_test.go +++ b/pkg/deployd/deployd/deployd_test.go @@ -40,6 +40,8 @@ type testSpec struct { endStatus *pb.DeploymentStatus // which end state we expect deployedResources []client.Object // list of Kubernetes resources expected to be applied to the cluster - only checks name and namespace processing processCallback // processing that happens in a coroutine together with deployd.Run(). Requires all resources in `deployedResources` to exist. + setup processCallback // processing that happens before deployd.Run() + verify func(t *testing.T, ctx context.Context, rig *testRig, test testSpec) } var tests = []testSpec{ @@ -117,7 +119,7 @@ var tests = []testSpec{ }, }, processing: func(ctx context.Context, rig *testRig, test testSpec) error { - return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, "completed", "myapplication")) + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, "completed", "Application", "myapplication")) }, }, @@ -138,7 +140,7 @@ var tests = []testSpec{ }, }, processing: func(ctx context.Context, rig *testRig, test testSpec) error { - return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.FailedSynchronization, "oops", "myapplication-failedsynchronization")) + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.FailedSynchronization, "oops", "Application", "myapplication-failedsynchronization")) }, }, @@ -159,7 +161,7 @@ var tests = []testSpec{ }, }, processing: func(ctx context.Context, rig *testRig, test testSpec) error { - return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.FailedPrepare, "oops", "myapplication-failedprepare")) + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.FailedPrepare, "oops", "Application", "myapplication-failedprepare")) }, }, @@ -184,6 +186,133 @@ var tests = []testSpec{ }, deployedResources: nil, }, + + // Redeploying an unchanged Application clears the synchronization hash, + // so that Naiserator synchronizes it instead of skipping it. + { + fixture: "testdata/application-resync.json", + timeout: 5 * time.Second, + endStatus: &pb.DeploymentStatus{ + State: pb.DeploymentState_success, + Message: "Deployment completed successfully.", + }, + deployedResources: []client.Object{ + &nais_io_v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myapplication-resync", + Namespace: "aura", + }, + }, + }, + setup: func(ctx context.Context, rig *testRig, test testSpec) error { + app := &nais_io_v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myapplication-resync", + Namespace: "aura", + }, + Spec: nais_io_v1alpha1.ApplicationSpec{Image: "foo/bar"}, + } + return createWithStatus(ctx, rig, app, &app.Status) + }, + processing: func(ctx context.Context, rig *testRig, test testSpec) error { + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, "completed", "Application", "myapplication-resync")) + }, + verify: func(t *testing.T, ctx context.Context, rig *testRig, test testSpec) { + app := &nais_io_v1alpha1.Application{} + err := rig.client.Get(ctx, client.ObjectKey{Name: "myapplication-resync", Namespace: "aura"}, app) + assert.NoError(t, err) + assertStatusInvalidated(t, app.Status) + }, + }, + + // Naisjobs use the same synchronization hash mechanism as Applications. + { + fixture: "testdata/naisjob-resync.json", + timeout: 5 * time.Second, + endStatus: &pb.DeploymentStatus{ + State: pb.DeploymentState_success, + Message: "Deployment completed successfully.", + }, + deployedResources: []client.Object{ + &nais_io_v1.Naisjob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mynaisjob-resync", + Namespace: "aura", + }, + }, + }, + setup: func(ctx context.Context, rig *testRig, test testSpec) error { + job := &nais_io_v1.Naisjob{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mynaisjob-resync", + Namespace: "aura", + }, + Spec: nais_io_v1.NaisjobSpec{Image: "foo/bar", Schedule: "*/1 * * * *"}, + } + return createWithStatus(ctx, rig, job, &job.Status) + }, + processing: func(ctx context.Context, rig *testRig, test testSpec) error { + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, "completed", "Naisjob", "mynaisjob-resync")) + }, + verify: func(t *testing.T, ctx context.Context, rig *testRig, test testSpec) { + job := &nais_io_v1.Naisjob{} + err := rig.client.Get(ctx, client.ObjectKey{Name: "mynaisjob-resync", Namespace: "aura"}, job) + assert.NoError(t, err) + assertStatusInvalidated(t, job.Status) + }, + }, + + // Naiserator's no-op rollout event describes the state before the forced + // resynchronization. Accepting it would report success without observing the + // rollout, so the deploy must keep waiting and time out when nothing follows. + { + fixture: "testdata/application-noop.json", + timeout: 3 * time.Second, + endStatus: &pb.DeploymentStatus{ + State: pb.DeploymentState_failure, + Message: "timeout while waiting for deployment to succeed (total of 1 errors)", + }, + deployedResources: []client.Object{ + &nais_io_v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myapplication-noop", + Namespace: "aura", + }, + }, + }, + processing: func(ctx context.Context, rig *testRig, test testSpec) error { + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, rolloutMessageNoop, "Application", "myapplication-noop")) + }, + }, +} + +// rolloutMessageNoop mirrors Naiserator's RolloutMessageNoop. +const rolloutMessageNoop = "No changes; deployment already up to date" + +// createWithStatus persists a workload along with the status Naiserator would have +// written after a successful deployment. Status is a subresource, so it needs a +// separate write. +func createWithStatus(ctx context.Context, rig *testRig, resource client.Object, status *nais_io_v1.Status) error { + err := rig.client.Create(ctx, resource) + if err != nil { + return err + } + + *status = nais_io_v1.Status{ + SynchronizationHash: "synchronized-hash", + SynchronizationState: events.RolloutComplete, + CorrelationID: "previous-deployment", + } + + return rig.client.Status().Update(ctx, resource) +} + +// assertStatusInvalidated checks that only the synchronization hash was cleared, so +// that Naiserator resynchronizes without losing the rest of its status. +func assertStatusInvalidated(t *testing.T, status nais_io_v1.Status) { + assert.Empty(t, status.SynchronizationHash) + assert.Equal(t, events.RolloutComplete, status.SynchronizationState) + assert.Equal(t, "previous-deployment", status.CorrelationID) } type testRig struct { @@ -460,6 +589,14 @@ func subTest(t *testing.T, rig *testRig, test testSpec, team string) { panic(fmt.Sprintf("test data fixture error in '%s': %s", test.fixture, err)) } + if test.setup != nil { + err = test.setup(ctx, rig, test) + if err != nil { + t.Errorf("Set up fixture: %s", err) + t.FailNow() + } + } + opctx, cancel := context.WithCancel(ctx) defer cancel() @@ -507,9 +644,13 @@ func subTest(t *testing.T, rig *testRig, test testSpec, team string) { assert.NoError(t, err) wg.Wait() + + if test.verify != nil { + test.verify(t, ctx, rig, test) + } } -func naiseratorEvent(id, reason, message, app string) *v1.Event { +func naiseratorEvent(id, reason, message, kind, name string) *v1.Event { return &v1.Event{ ObjectMeta: metav1.ObjectMeta{ Name: "event-" + keygen.RandStringBytes(10), @@ -522,9 +663,9 @@ func naiseratorEvent(id, reason, message, app string) *v1.Event { Reason: reason, Message: message, InvolvedObject: v1.ObjectReference{ - Kind: "Application", + Kind: kind, Namespace: "aura", - Name: app, + Name: name, }, LastTimestamp: metav1.NewTime(time.Now()), } diff --git a/pkg/deployd/deployd/testdata/application-noop.json b/pkg/deployd/deployd/testdata/application-noop.json new file mode 100644 index 00000000..a2a57e7e --- /dev/null +++ b/pkg/deployd/deployd/testdata/application-noop.json @@ -0,0 +1,13 @@ +[ + { + "kind": "Application", + "apiVersion": "nais.io/v1alpha1", + "metadata": { + "name": "myapplication-noop", + "namespace": "aura" + }, + "spec": { + "image": "foo/bar" + } + } +] diff --git a/pkg/deployd/deployd/testdata/application-resync.json b/pkg/deployd/deployd/testdata/application-resync.json new file mode 100644 index 00000000..c46c8f49 --- /dev/null +++ b/pkg/deployd/deployd/testdata/application-resync.json @@ -0,0 +1,13 @@ +[ + { + "kind": "Application", + "apiVersion": "nais.io/v1alpha1", + "metadata": { + "name": "myapplication-resync", + "namespace": "aura" + }, + "spec": { + "image": "foo/bar" + } + } +] diff --git a/pkg/deployd/deployd/testdata/naisjob-resync.json b/pkg/deployd/deployd/testdata/naisjob-resync.json new file mode 100644 index 00000000..f91ed259 --- /dev/null +++ b/pkg/deployd/deployd/testdata/naisjob-resync.json @@ -0,0 +1,14 @@ +[ + { + "kind": "Naisjob", + "apiVersion": "nais.io/v1", + "metadata": { + "name": "mynaisjob-resync", + "namespace": "aura" + }, + "spec": { + "image": "foo/bar", + "schedule": "*/1 * * * *" + } + } +] From 3d895c380a6ef4f7b6e0abd24e27ec816fb179dc Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 6 Aug 2026 09:04:15 +0200 Subject: [PATCH 3/3] fix(deployd): scope forced resync handling to unchanged generations --- pkg/deployd/deployd/deployd.go | 9 +-- pkg/deployd/deployd/deployd_test.go | 56 +++++++++++++++++-- .../application-generation-change.json | 13 +++++ pkg/deployd/strategy/deploy.go | 33 +++++------ pkg/deployd/strategy/deploy_test.go | 47 +++++++++++++--- pkg/deployd/strategy/deploymentwatch.go | 2 +- pkg/deployd/strategy/jobwatch.go | 2 +- pkg/deployd/strategy/nais.go | 11 ++-- pkg/deployd/strategy/watch.go | 10 +++- 9 files changed, 136 insertions(+), 47 deletions(-) create mode 100644 pkg/deployd/deployd/testdata/application-generation-change.json diff --git a/pkg/deployd/deployd/deployd.go b/pkg/deployd/deployd/deployd.go index 10e8032c..0edffda4 100644 --- a/pkg/deployd/deployd/deployd.go +++ b/pkg/deployd/deployd/deployd.go @@ -119,8 +119,9 @@ func Run(op *operation.Operation, client kubeclient.Interface) { ) resourceInterface, err := client.ResourceInterface(&resource) + forcedResync := false if err == nil { - _, err = strategy.NewDeployStrategy(resourceInterface).Deploy(op.Context, resource, span) + _, forcedResync, err = strategy.NewDeployStrategy(resourceInterface).Deploy(op.Context, resource, span) } if err != nil { @@ -139,11 +140,11 @@ func Run(op *operation.Operation, client kubeclient.Interface) { op.StatusChan <- pb.NewInProgressStatus(op.Request, "Successfully applied %s", identifier.String()) wait.Add(1) - go func(logger *log.Entry, resource unstructured.Unstructured) { + go func(logger *log.Entry, resource unstructured.Unstructured, forcedResync bool) { deadline, _ := op.Context.Deadline() op.Logger.Debugf("Monitoring rollout status of '%s/%s' in namespace '%s', deadline %s", identifier.GroupVersionKind, identifier.Name, identifier.Namespace, deadline) strat := strategy.NewWatchStrategy(identifier.GroupVersionKind, client) - status := strat.Watch(op, resource, span) + status := strat.Watch(op, resource, span, forcedResync) if status != nil { span.AddEvent(status.Message) if status.GetState().IsError() { @@ -163,7 +164,7 @@ func Run(op *operation.Operation, client kubeclient.Interface) { op.Logger.Debugf("Finished monitoring rollout status of '%s/%s' in namespace '%s'", identifier.GroupVersionKind, identifier.Name, identifier.Namespace) wait.Done() span.End() - }(logger, resource) + }(logger, resource, forcedResync) } op.StatusChan <- pb.NewInProgressStatus(op.Request, "All resources saved to Kubernetes; waiting for completion") diff --git a/pkg/deployd/deployd/deployd_test.go b/pkg/deployd/deployd/deployd_test.go index b99e59e3..78b4f669 100644 --- a/pkg/deployd/deployd/deployd_test.go +++ b/pkg/deployd/deployd/deployd_test.go @@ -262,9 +262,46 @@ var tests = []testSpec{ }, }, - // Naiserator's no-op rollout event describes the state before the forced - // resynchronization. Accepting it would report success without observing the - // rollout, so the deploy must keep waiting and time out when nothing follows. + // A no-op event remains terminal when the update changed generation and did not + // force a resynchronization. + { + fixture: "testdata/application-generation-change.json", + timeout: 5 * time.Second, + endStatus: &pb.DeploymentStatus{ + State: pb.DeploymentState_success, + Message: "Deployment completed successfully.", + }, + deployedResources: []client.Object{ + &nais_io_v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myapplication-generation-change", + Namespace: "aura", + }, + }, + }, + setup: func(ctx context.Context, rig *testRig, test testSpec) error { + app := &nais_io_v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myapplication-generation-change", + Namespace: "aura", + }, + Spec: nais_io_v1alpha1.ApplicationSpec{Image: "foo/old"}, + } + return createWithStatus(ctx, rig, app, &app.Status) + }, + processing: func(ctx context.Context, rig *testRig, test testSpec) error { + return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, rolloutMessageNoop, "Application", "myapplication-generation-change")) + }, + verify: func(t *testing.T, ctx context.Context, rig *testRig, test testSpec) { + app := &nais_io_v1alpha1.Application{} + err := rig.client.Get(ctx, client.ObjectKey{Name: "myapplication-generation-change", Namespace: "aura"}, app) + assert.NoError(t, err) + assert.EqualValues(t, 2, app.Generation) + assert.Equal(t, "synchronized-hash", app.Status.SynchronizationHash) + }, + }, + + // Ignore a no-op event emitted before the forced resynchronization completes. { fixture: "testdata/application-noop.json", timeout: 3 * time.Second, @@ -280,13 +317,22 @@ var tests = []testSpec{ }, }, }, + setup: func(ctx context.Context, rig *testRig, test testSpec) error { + app := &nais_io_v1alpha1.Application{ + ObjectMeta: metav1.ObjectMeta{ + Name: "myapplication-noop", + Namespace: "aura", + }, + Spec: nais_io_v1alpha1.ApplicationSpec{Image: "foo/bar"}, + } + return createWithStatus(ctx, rig, app, &app.Status) + }, processing: func(ctx context.Context, rig *testRig, test testSpec) error { return rig.client.Create(ctx, naiseratorEvent(test.fixture, events.RolloutComplete, rolloutMessageNoop, "Application", "myapplication-noop")) }, }, } -// rolloutMessageNoop mirrors Naiserator's RolloutMessageNoop. const rolloutMessageNoop = "No changes; deployment already up to date" // createWithStatus persists a workload along with the status Naiserator would have @@ -307,8 +353,6 @@ func createWithStatus(ctx context.Context, rig *testRig, resource client.Object, return rig.client.Status().Update(ctx, resource) } -// assertStatusInvalidated checks that only the synchronization hash was cleared, so -// that Naiserator resynchronizes without losing the rest of its status. func assertStatusInvalidated(t *testing.T, status nais_io_v1.Status) { assert.Empty(t, status.SynchronizationHash) assert.Equal(t, events.RolloutComplete, status.SynchronizationState) diff --git a/pkg/deployd/deployd/testdata/application-generation-change.json b/pkg/deployd/deployd/testdata/application-generation-change.json new file mode 100644 index 00000000..bef5a3bf --- /dev/null +++ b/pkg/deployd/deployd/testdata/application-generation-change.json @@ -0,0 +1,13 @@ +[ + { + "kind": "Application", + "apiVersion": "nais.io/v1alpha1", + "metadata": { + "name": "myapplication-generation-change", + "namespace": "aura" + }, + "spec": { + "image": "foo/new" + } + } +] diff --git a/pkg/deployd/strategy/deploy.go b/pkg/deployd/strategy/deploy.go index 054c21c9..7ee6898c 100644 --- a/pkg/deployd/strategy/deploy.go +++ b/pkg/deployd/strategy/deploy.go @@ -18,25 +18,25 @@ func NewDeployStrategy(namespacedResource dynamic.ResourceInterface) DeployStrat } type DeployStrategy interface { - Deploy(ctx context.Context, resource unstructured.Unstructured, trace trace.Span) (*unstructured.Unstructured, error) + Deploy(ctx context.Context, resource unstructured.Unstructured, trace trace.Span) (*unstructured.Unstructured, bool, error) } type createOrUpdateStrategy struct { client dynamic.ResourceInterface } -func (c createOrUpdateStrategy) Deploy(ctx context.Context, resource unstructured.Unstructured, trace trace.Span) (*unstructured.Unstructured, error) { +func (c createOrUpdateStrategy) Deploy(ctx context.Context, resource unstructured.Unstructured, trace trace.Span) (*unstructured.Unstructured, bool, error) { existing, err := c.client.Get(ctx, resource.GetName(), metav1.GetOptions{}) if errors.IsNotFound(err) { deployed, err := c.client.Create(ctx, &resource, metav1.CreateOptions{ FieldValidation: metav1.FieldValidationStrict, }) if err != nil { - return nil, fmt.Errorf("creating resource: %w", transformStrictDecodingError(resource, err)) + return nil, false, fmt.Errorf("creating resource: %w", transformStrictDecodingError(resource, err)) } - return deployed, nil + return deployed, false, nil } else if err != nil { - return nil, fmt.Errorf("get existing resource: %w", err) + return nil, false, fmt.Errorf("get existing resource: %w", err) } resource.SetResourceVersion(existing.GetResourceVersion()) @@ -44,32 +44,27 @@ func (c createOrUpdateStrategy) Deploy(ctx context.Context, resource unstructure FieldValidation: metav1.FieldValidationStrict, }) if err != nil { - return nil, fmt.Errorf("updating resource: %w", transformStrictDecodingError(resource, err)) + return nil, false, fmt.Errorf("updating resource: %w", transformStrictDecodingError(resource, err)) } - if shouldInvalidateSynchronizationHash(resource) { + forcedResync := shouldInvalidateSynchronizationHash(existing, updated) + if forcedResync { + // Patch after Update so Naiserator sees this deployment's spec and correlation + // ID annotation without replacing other status fields. err = c.invalidateSynchronizationHash(ctx, resource.GetName()) if err != nil { - return nil, fmt.Errorf("invalidating synchronization hash: %w", err) + return nil, false, fmt.Errorf("invalidating synchronization hash: %w", err) } trace.AddEvent("Forced resynchronization of Nais resource") } - return updated, nil + return updated, forcedResync, nil } -func shouldInvalidateSynchronizationHash(resource unstructured.Unstructured) bool { - gvk := resource.GroupVersionKind() - return gvk.Group == "nais.io" && (gvk.Kind == "Application" || gvk.Kind == "Naisjob") +func shouldInvalidateSynchronizationHash(existing, updated *unstructured.Unstructured) bool { + return isNaisWorkload(updated.GroupVersionKind()) && updated.GetGeneration() == existing.GetGeneration() } -// invalidateSynchronizationHash clears the hash Naiserator compares against to decide -// whether a resource needs synchronization. Without this, redeploying an unchanged spec -// is a no-op, and the deployment waits for a rollout event that never arrives. -// -// The resource spec is written before this call, so the resynchronization picks up the -// spec and correlation ID from this deployment. Only the hash field is patched, leaving -// the rest of the operator-owned status untouched. func (c createOrUpdateStrategy) invalidateSynchronizationHash(ctx context.Context, name string) error { _, err := c.client.Patch(ctx, name, types.MergePatchType, []byte(`{"status":{"synchronizationHash":null}}`), metav1.PatchOptions{}, "status") diff --git a/pkg/deployd/strategy/deploy_test.go b/pkg/deployd/strategy/deploy_test.go index ce48cea3..4dec2a80 100644 --- a/pkg/deployd/strategy/deploy_test.go +++ b/pkg/deployd/strategy/deploy_test.go @@ -22,8 +22,6 @@ var ( naisjobGVR = schema.GroupVersionResource{Group: "nais.io", Version: "v1", Resource: "naisjobs"} ) -// Redeploying an unchanged spec must still trigger Naiserator, which only synchronizes -// when its stored hash differs from the hash it computes. func TestDeployInvalidatesSynchronizationHash(t *testing.T) { tests := []struct { name string @@ -37,15 +35,17 @@ func TestDeployInvalidatesSynchronizationHash(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { existing := testResource(tt.gvk) + existing.SetGeneration(1) require.NoError(t, unstructured.SetNestedField(existing.Object, "current-hash", "status", "synchronizationHash")) desired := existing.DeepCopy() client := fake.NewSimpleDynamicClient(runtime.NewScheme(), &existing) resourceClient := client.Resource(tt.gvr).Namespace(existing.GetNamespace()) - deployed, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) + deployed, forcedResync, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) require.NoError(t, err) require.NotNil(t, deployed) + require.True(t, forcedResync) // The spec must be written before the hash is cleared, so that the // resynchronization picks up this deployment rather than the previous one. @@ -90,15 +90,47 @@ func TestDeployDoesNotInvalidateSynchronizationHash(t *testing.T) { client.ClearActions() } - _, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), resource, noop.Span{}) + _, forcedResync, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), resource, noop.Span{}) require.NoError(t, err) + require.False(t, forcedResync) require.NotContains(t, verbs(client.Actions()), "patch") }) } } -// A resource whose hash was not cleared silently stops reconciling, so the deployment -// must fail loudly rather than wait for a rollout that never happens. +func TestDeployDoesNotInvalidateSynchronizationHashWhenGenerationChanges(t *testing.T) { + tests := []struct { + name string + gvk schema.GroupVersionKind + gvr schema.GroupVersionResource + }{ + {name: "Application", gvk: applicationGVK, gvr: applicationGVR}, + {name: "Naisjob", gvk: naisjobGVK, gvr: naisjobGVR}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + existing := testResource(tt.gvk) + existing.SetGeneration(1) + desired := existing.DeepCopy() + + client := fake.NewSimpleDynamicClient(runtime.NewScheme(), &existing) + client.PrependReactor("update", tt.gvr.Resource, func(action k8stesting.Action) (bool, runtime.Object, error) { + updated := action.(k8stesting.UpdateAction).GetObject().(*unstructured.Unstructured).DeepCopy() + updated.SetGeneration(2) + return true, updated, nil + }) + resourceClient := client.Resource(tt.gvr).Namespace(existing.GetNamespace()) + + deployed, forcedResync, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) + require.NoError(t, err) + require.EqualValues(t, 2, deployed.GetGeneration()) + require.False(t, forcedResync) + require.Equal(t, []string{"get", "update"}, verbs(client.Actions())) + }) + } +} + func TestDeployFailsWhenSynchronizationHashCannotBeInvalidated(t *testing.T) { existing := testResource(applicationGVK) desired := existing.DeepCopy() @@ -109,9 +141,10 @@ func TestDeployFailsWhenSynchronizationHashCannotBeInvalidated(t *testing.T) { }) resourceClient := client.Resource(applicationGVR).Namespace(existing.GetNamespace()) - _, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) + _, forcedResync, err := NewDeployStrategy(resourceClient).Deploy(t.Context(), *desired, noop.Span{}) require.ErrorContains(t, err, "invalidating synchronization hash") require.True(t, errors.IsForbidden(err)) + require.False(t, forcedResync) } func verbs(actions []k8stesting.Action) []string { diff --git a/pkg/deployd/strategy/deploymentwatch.go b/pkg/deployd/strategy/deploymentwatch.go index 8af98fb4..487fae33 100644 --- a/pkg/deployd/strategy/deploymentwatch.go +++ b/pkg/deployd/strategy/deploymentwatch.go @@ -21,7 +21,7 @@ type deployment struct { client kubeclient.Interface } -func (d deployment) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span) *pb.DeploymentStatus { +func (d deployment) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span, _ bool) *pb.DeploymentStatus { var cur *apps.Deployment var nova *apps.Deployment var err error diff --git a/pkg/deployd/strategy/jobwatch.go b/pkg/deployd/strategy/jobwatch.go index 0e6b51e6..eeb32380 100644 --- a/pkg/deployd/strategy/jobwatch.go +++ b/pkg/deployd/strategy/jobwatch.go @@ -18,7 +18,7 @@ type job struct { client kubeclient.Interface } -func (j job) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span) *pb.DeploymentStatus { +func (j job) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span, _ bool) *pb.DeploymentStatus { var job *v1.Job var err error diff --git a/pkg/deployd/strategy/nais.go b/pkg/deployd/strategy/nais.go index 989639ca..b5bb73ae 100644 --- a/pkg/deployd/strategy/nais.go +++ b/pkg/deployd/strategy/nais.go @@ -23,13 +23,12 @@ type naisResource struct { client kubeclient.Interface } -// rolloutMessageNoop mirrors Naiserator's RolloutMessageNoop, which it emits when a -// redeploy has no spec changes. deployd forces a resynchronization on every deploy, so -// this message describes the state before that resynchronization and must not finish -// the wait. The real rollout event follows. +// Naiserator may emit this before deployd clears the hash for an unchanged generation. +// Ignore it because it does not represent the forced resynchronization. +// https://github.com/nais/naiserator/blob/master/pkg/synchronizer/monitoring.go const rolloutMessageNoop = "No changes; deployment already up to date" -func (a naisResource) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span) *pb.DeploymentStatus { +func (a naisResource) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span, forcedResync bool) *pb.DeploymentStatus { var err error eventsClient := a.client.Kubernetes().CoreV1().Events(resource.GetNamespace()) @@ -79,7 +78,7 @@ func (a naisResource) Watch(op *operation.Operation, resource unstructured.Unstr continue } - if event.Message == rolloutMessageNoop { + if forcedResync && event.ReportingController == "naiserator" && event.Reason == events.RolloutComplete && event.Message == rolloutMessageNoop { op.Logger.Tracef("Ignoring no-op rollout event %s; awaiting forced resynchronization", event.Name) continue } diff --git a/pkg/deployd/strategy/watch.go b/pkg/deployd/strategy/watch.go index bbc5db76..6b855666 100644 --- a/pkg/deployd/strategy/watch.go +++ b/pkg/deployd/strategy/watch.go @@ -18,18 +18,18 @@ var ( ) type WatchStrategy interface { - Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span) *pb.DeploymentStatus + Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span, forcedResync bool) *pb.DeploymentStatus } type NoOp struct{} -func (c NoOp) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span) *pb.DeploymentStatus { +func (c NoOp) Watch(op *operation.Operation, resource unstructured.Unstructured, trace trace.Span, _ bool) *pb.DeploymentStatus { op.Logger.Debugf("Watch not implemented for resource %s/%s", resource.GroupVersionKind().String(), resource.GetName()) return nil } func NewWatchStrategy(gvk schema.GroupVersionKind, client kubeclient.Interface) WatchStrategy { - if gvk.Group == "nais.io" && (gvk.Kind == "Application" || gvk.Kind == "Naisjob") { + if isNaisWorkload(gvk) { return naisResource{client: client} } @@ -43,3 +43,7 @@ func NewWatchStrategy(gvk schema.GroupVersionKind, client kubeclient.Interface) return NoOp{} } + +func isNaisWorkload(gvk schema.GroupVersionKind) bool { + return gvk.Group == "nais.io" && (gvk.Kind == "Application" || gvk.Kind == "Naisjob") +}