Skip to content

Commit

Permalink
Use original status when making api-server Status.Update call in ad…
Browse files Browse the repository at this point in the history 
…option reconciler (#71)

Issue #, if available: aws-controllers-k8s/community#1161

Description of changes:
* The Status update from AdoptionReconciler was not useful because the Create call before Status.Update was resetting status of the CustomResource.
* Earlier this similar kind of problem was also present in `reconciler.go` and it was fixed by keeping an original copy of Status before making Create/Update calls
* I used the same solution in AdoptionReconciler.

----------

* Discovered this issue while debugging SageMaker ModelPackage adoption test. ModelPackage uses ARN as identifier which needs to be set inside `Status.ACKResourceMetadata.ARN` field of CustomResource.
* Since the Identifier was never getting correctly set, the `ReadOne` call inside `reconciler.go` was failing while reconciling the CustomResource.

--------------

* Validated by running SageMaker end-to-end tests successfully.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
  • Loading branch information
@vijtrip2
vijtrip2 authored Feb 7, 2022
1 parent 7037a49 commit 65b9179
Show file tree
Hide file tree
Showing 2 changed files with 42 additions and 24 deletions.
8 changes: 7 additions & 1 deletion pkg/runtime/adoption_reconciler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,10 +224,16 @@ func (r *adoptionReconciler) Sync(
}, described.RuntimeObject()); err != nil {
if apierrors.IsNotFound(err) {
// If Adopted AWS resource was not found in k8s, create it.

// Before creation, Keep the copy of original described object
// because after the create call, Status gets set to empty
describedCopy := described.DeepCopy()
if err := r.kc.Create(ctx, described.RuntimeObject()); err != nil {
return r.onError(ctx, desired, err)
}

// reset the status of described object to original value before
// making the Status Update call
described.SetStatus(describedCopy)
if err := r.kc.Status().Update(ctx, described.RuntimeObject()); err != nil {
return r.onError(ctx, desired, err)
}
Expand Down
58 changes: 35 additions & 23 deletions pkg/runtime/adoption_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,14 @@ func mockReconciler() (acktypes.AdoptedResourceReconciler, *ctrlrtclientmock.Cli
), kc, apiReader
}

func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource) {
func mockDescriptorAndAWSResource() (*ackmocks.AWSResourceDescriptor, *ackmocks.AWSResource, *ackmocks.AWSResource) {
des := &ackmocks.AWSResourceDescriptor{}
emptyRuntimeObject := &ctrlrtclientmock.Object{}
res := &ackmocks.AWSResource{}
resDeepCopy := &ackmocks.AWSResource{}
des.On("EmptyRuntimeObject").Return(emptyRuntimeObject)
des.On("ResourceFromRuntimeObject", emptyRuntimeObject).Return(res)
return des, res
return des, res, resDeepCopy
}

func mockManager() *ackmocks.AWSResourceManager {
Expand All @@ -91,7 +92,11 @@ func setupMockClient(kc *ctrlrtclientmock.Client, statusWriter *ctrlrtclientmock
kc.On("Patch", ctx, adoptedRes, mock.AnythingOfType("*client.mergeFromPatch")).Return(nil)
}

func setupMockAwsResource(res *ackmocks.AWSResource, adoptedRes *ackv1alpha1.AdoptedResource) {
func setupMockAwsResource(
res *ackmocks.AWSResource,
resDeepCopy *ackmocks.AWSResource,
adoptedRes *ackv1alpha1.AdoptedResource,
) {
res.On("SetIdentifiers", adoptedRes.Spec.AWS).Return(nil)
res.On("SetObjectMeta", mock.AnythingOfType("ObjectMeta")).Run(func(args mock.Arguments) {})

Expand All @@ -108,6 +113,8 @@ func setupMockAwsResource(res *ackmocks.AWSResource, adoptedRes *ackv1alpha1.Ado
rmo.On("GetFinalizers").Return(make([]string, 0))
rmo.On("GetOwnerReferences").Return(make([]v1.OwnerReference, 0))
rmo.On("GetGenerateName").Return("")
res.On("DeepCopy").Return(resDeepCopy)
res.On("SetStatus", resDeepCopy).Run(func(args mock.Arguments) {})
}

func setupMockManager(manager *ackmocks.AWSResourceManager, ctx context.Context, res *ackmocks.AWSResource) {
Expand Down Expand Up @@ -148,15 +155,15 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
res.On("SetIdentifiers", adoptedRes.Spec.AWS).Return(errors.New("unable to set Identifier"))
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)

// Call
Expand All @@ -176,7 +183,7 @@ func TestSync_FailureInSettingIdentifiers(t *testing.T) {
Namespace: Namespace,
Name: Name,
}, res.RuntimeObject())
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res)
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy)
assertManaged(false, t, ctx, kc, adoptedRes)
assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes)
}
Expand All @@ -186,14 +193,14 @@ func TestSync_FailureInReadOne(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)
manager.On("ReadOne", ctx, res).Return(res, errors.New("failed to perform ReadOne"))

Expand All @@ -213,7 +220,7 @@ func TestSync_FailureInReadOne(t *testing.T) {
Namespace: Namespace,
Name: Name,
}, res.RuntimeObject())
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res)
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy)
assertManaged(false, t, ctx, kc, adoptedRes)
assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes)
}
Expand All @@ -223,14 +230,14 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)
setupMockManager(manager, ctx, res)
setupMockDescriptor(descriptor, res)
Expand All @@ -246,7 +253,7 @@ func TestSync_AWSResourceAlreadyExists(t *testing.T) {
//Assertions
require.Nil(err)
assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res)
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res)
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy)
assertManaged(true, t, ctx, kc, adoptedRes)
assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes)
}
Expand All @@ -256,14 +263,14 @@ func TestSync_APIReaderUnknownError(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)
setupMockManager(manager, ctx, res)
setupMockDescriptor(descriptor, res)
Expand All @@ -280,7 +287,7 @@ func TestSync_APIReaderUnknownError(t *testing.T) {
require.NotNil(err)
require.Equal("unknown error", err.Error())
assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res)
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res)
assertAWSResourceCreation(false, t, ctx, kc, statusWriter, res, resDeepCopy)
assertManaged(false, t, ctx, kc, adoptedRes)
assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes)
}
Expand All @@ -290,14 +297,14 @@ func TestSync_ErrorInResourceCreation(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)
setupMockManager(manager, ctx, res)
setupMockDescriptor(descriptor, res)
Expand All @@ -323,14 +330,14 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)
setupMockManager(manager, ctx, res)
setupMockDescriptor(descriptor, res)
Expand All @@ -345,7 +352,7 @@ func TestSync_ErrorInStatusUpdate(t *testing.T) {
require.NotNil(err)
require.Equal("status update failure", err.Error())
assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res)
assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res)
assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy)
assertManaged(false, t, ctx, kc, adoptedRes)
assertAdoptedCondition("False", require, t, ctx, kc, statusWriter, adoptedRes)
}
Expand All @@ -355,14 +362,14 @@ func TestSync_HappyCase(t *testing.T) {
require := require.New(t)
// Mock resource creation
r, kc, apiReader := mockReconciler()
descriptor, res := mockDescriptorAndAWSResource()
descriptor, res, resDeepCopy := mockDescriptorAndAWSResource()
manager := mockManager()
adoptedRes := adoptedResource(Namespace, Name)
ctx := context.TODO()
statusWriter := &ctrlrtclientmock.StatusWriter{}

//Mock behavior setup
setupMockAwsResource(res, adoptedRes)
setupMockAwsResource(res, resDeepCopy, adoptedRes)
setupMockClient(kc, statusWriter, ctx, adoptedRes)
setupMockManager(manager, ctx, res)
setupMockDescriptor(descriptor, res)
Expand All @@ -376,7 +383,7 @@ func TestSync_HappyCase(t *testing.T) {
//Assertions
require.Nil(err)
assertAWSResourceRead(t, ctx, manager, apiReader, adoptedRes, res)
assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res)
assertAWSResourceCreation(true, t, ctx, kc, statusWriter, res, resDeepCopy)
assertManaged(true, t, ctx, kc, adoptedRes)
assertAdoptedCondition("True", require, t, ctx, kc, statusWriter, adoptedRes)
}
Expand Down Expand Up @@ -432,12 +439,17 @@ func assertAWSResourceCreation(
kc *ctrlrtclientmock.Client,
statusWriter *ctrlrtclientmock.StatusWriter,
res *ackmocks.AWSResource,
resDeepCopy *ackmocks.AWSResource,
) {
if expectedCreation {
kc.AssertCalled(t, "Create", ctx, res.RuntimeObject())
res.AssertCalled(t, "DeepCopy")
res.AssertCalled(t, "SetStatus", resDeepCopy)
statusWriter.AssertCalled(t, "Update", ctx, res.RuntimeObject())
} else {
kc.AssertNotCalled(t, "Create", ctx, res.RuntimeObject())
res.AssertNotCalled(t, "DeepCopy")
res.AssertNotCalled(t, "SetStatus", resDeepCopy)
statusWriter.AssertNotCalled(t, "Update", ctx, res.RuntimeObject())
}
}
Expand Down

0 comments on commit 65b9179

Please sign in to comment.