Skip to content

Commit 8665453

Browse files
authored
Reset the object status after patching resource metadata & spec. (#48)
Description of changes: * Introduces a new `DeepCopy` method in AWSResource interface * After the fixes made in release v0.12.0, the status of latest object was getting modified during PatchMetadataAndSpec call. * The problem was surfaced in one of the test in SageMaker that rely on waiting on Status.Condition. * Since the condition was being reset by the PatchMetadataAndSpec call, the test was never passing. * I tried multiple things like (a) Passing empty status in the base parameter during `kc.Patch()` call, but the `kc.Patch` call resets the status by reading it from etcd. * At this point, to understand this problem more, I will have to deep dive into kc.Patch source code. * To unblock sagemaker team from developing their controller, I propose the change in following PR which is safe and unblocks the team. * It is some redundant work in copying the status again but hopefully when we understand the `kc.Patch` operation better, we can improve on it. * I have tested with sagemaker e2e that following solution works. By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
1 parent a506412 commit 8665453

File tree

4 files changed

+29
-0
lines changed

4 files changed

+29
-0
lines changed

mocks/pkg/types/aws_resource.go

Lines changed: 16 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

pkg/runtime/reconciler.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -403,12 +403,18 @@ func (r *resourceReconciler) patchResourceMetadataAndSpec(
403403
}
404404

405405
rlog.Enter("kc.Patch (metadata + spec)")
406+
// Save a copy of the latest object, to reset 'Status' after performing
407+
// the kc.Patch() operation
408+
latestCopy := latest.DeepCopy()
406409
err = r.kc.Patch(
407410
ctx,
408411
latest.RuntimeObject(),
409412
client.MergeFrom(desired.RuntimeObject().DeepCopyObject()),
410413
)
414+
// Reset the status of latest object after patching.
415+
latest.SetStatus(latestCopy)
411416
rlog.Exit("kc.Patch (metadata + spec)", err)
417+
412418
if err != nil {
413419
return err
414420
}

pkg/runtime/reconciler_test.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,9 @@ func resourceMocks() (
7474
res := &ackmocks.AWSResource{}
7575
res.On("MetaObject").Return(metaObj)
7676
res.On("RuntimeObject").Return(rtObj)
77+
res.On("DeepCopy").Return(res)
78+
// DoNothing on SetStatus call.
79+
res.On("SetStatus", res).Return(func(res ackmocks.AWSResource) {})
7780

7881
return res, rtObj, metaObj
7982
}
@@ -244,6 +247,8 @@ func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInMetadata(t *testing.T) {
244247
// Only the HandleReconcilerError wrapper function ever calls patchResourceStatus
245248
kc.AssertNotCalled(t, "Status")
246249
rm.AssertCalled(t, "LateInitialize", ctx, latest)
250+
latest.AssertCalled(t, "DeepCopy")
251+
latest.AssertCalled(t, "SetStatus", latest)
247252
}
248253

249254
func TestReconcilerUpdate_PatchMetadataAndSpec_DiffInSpec(t *testing.T) {

pkg/types/aws_resource.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,6 @@ type AWSResource interface {
5757
SetIdentifiers(*ackv1alpha1.AWSIdentifiers) error
5858
// SetStatus will set the Status field for the resource
5959
SetStatus(AWSResource)
60+
// DeepCopy will return a copy of the resource
61+
DeepCopy() AWSResource
6062
}

0 commit comments

Comments
 (0)