Reset the object status after patching resource metadata & spec. #48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
surajkota
approved these changes
Aug 25, 2021
|
Some log artifacts showing that
the call looks like err = r.kc.Patch(
ctx,
latest.RuntimeObject(),
client.MergeFrom(clearedDesired.RuntimeObject()),
)I have verified by creating diff that
|
jaypipes
reviewed
Aug 25, 2021
| latestCopy := latest.DeepCopy() | ||
| err = r.kc.Patch( | ||
| ctx, | ||
| latest.RuntimeObject(), |
There was a problem hiding this comment.
could we accomplish the same thing as this PR's changes by just doing this here?
latest.RuntimeObject().DeepCopy(),There was a problem hiding this comment.
The SetStatus() method in AWSResource interface uses AWSResource as parameter type. If we do the above we will need to create another method that can accept runtime.Object and update the status using it.
There was a problem hiding this comment.
But what I'm saying is that you wouldn't need to call latest.SetStatus() below if latest.RuntimeObject().DeepCopy() is used here because the latest variable would not be mutated, and you are calling latest.SetStatus(latestCopy) below to "undo" that mutation.
There was a problem hiding this comment.
And if we do latest.RuntimeObject().DeepCopy() inside kc.Patch, then we will run into resource version and optimistic lock issues we saw earlier.right? @RedbackThomson
the latest variable would not be mutated
I believe we want metadata of latest to be mutated after kc.Patch call.
surajkota
requested changes
Aug 26, 2021
a-hilaly
approved these changes
Aug 26, 2021
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: A-Hilaly, RedbackThomson, vijtrip2 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
ack-bot
pushed a commit
to aws-controllers-k8s/code-generator
that referenced
this pull request
Aug 26, 2021
Description of changes: Commit#1 - Adds the implementation of AWSResource.DeepCopy() method. aws-controllers-k8s/runtime#48 Commit#2 - Fixes the patching bug in lateInitialize code to unblock sagemaker team - BUG: rm.LateInitialize() method was adding lateInitialized fields to the same object passed in the parameter and returning as output. ```go lateInitializedLatest, err := rm.LateInitialize(ctx, latest) rlog.Exit("rm.LateInitialize", err) // Always patch after late initialize because some fields may have been initialized while // others require a retry after some delay. // This patching does not hurt because if there is no diff then 'patchResourceMetadataAndSpec' // acts as a no-op. if ackcompare.IsNotNil(lateInitializedLatest) { patchErr := r.patchResourceMetadataAndSpec(ctx, latest, lateInitializedLatest) // Throw the patching error if reconciler is unable to patch the resource with late initializations if patchErr != nil { err = patchErr } } ``` - Since lateInitializedLatest and latest were same object, above `patchResourceMetadataAndSpec` call sees no diff and does not patch lateInitialized fields into etcd. - This PR solves the bug by adding lateInitialized fields in a copy of latest and returning that copy (without modifying latest) By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of changes:
DeepCopymethod in AWSResource interface(a) Passing empty status in the base parameter during
kc.Patch()call, but thekc.Patchcall resets the status by reading it from etcd.kc.Patchoperation better, we can improve on it.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.