feat: wait for resource to sync before updating#188
feat: wait for resource to sync before updating#188michaelhtm wants to merge 1 commit intoaws-controllers-k8s:mainfrom
Conversation
Currently, We've been setting a resource as NotSynced in ReadOne, if the resource status is not a certain way (Eg. waiting for EKS Cluster to be Active), and also checking the same resource Status in sdkUpdate. This is the first attempt at addressing that issue. We are going to need to ensure every controller/resource returns the proper NotSynced reason and the requeue error inside the condition. This will allow us to set a custom requeue right from ReadOne, that will be returned in preUpdate.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: michaelhtm The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knottnt
left a comment
knottnt
left a comment
There was a problem hiding this comment.
It would be nice if we included some integration and unit tests.
| return false | ||
| } | ||
|
|
||
| // IsSyncedInRes is like IsSynced, but instead of returnning false |
There was a problem hiding this comment.
nit: spelling
| // IsSyncedInRes is like IsSynced, but instead of returnning false | |
| // IsSyncedInRes is like IsSynced, but instead of returning false |
| // when the Synced condition is not there, it returns true. | ||
| // Note(michaelhtm) We will be using this as a preUpdate check to | ||
| // see if a resource has a NotSynced condition | ||
| func IsSyncedInRes(ctx context.Context, rm acktypes.AWSResourceManager, res acktypes.AWSResource) (bool, error) { |
There was a problem hiding this comment.
p: "Res" is a bit unclear. Maybe IsSyncedInAWSResource? It's more verbose, but explicit about what is actually being checked.
| } | ||
|
|
||
| // IsSyncedInRes is like IsSynced, but instead of returnning false | ||
| // when the Synced condition is not there, it returns true. |
There was a problem hiding this comment.
// when the Synced condition is not there, it returns true.
Is this this the case? If the for loop doesn't find a ConditionTypeResourceSynced condition it this function returns the result of IsSynced.
| } | ||
|
|
||
| // We don't want to attempt updating the resource until it is Synced! | ||
| if synced, err := IsSyncedInRes(ctx, rm, latest); !synced { |
There was a problem hiding this comment.
Do we want to fully block the update call? For example in Bedrock Agent we need to make an API call to PrepareAgent before the resource is in the synced state of Prepared.
There was a problem hiding this comment.
Additionally, I believe @a-hilaly mentioned that we might want to also allow updates, such as tags, that don't depend on the status of the AWS resource.
|
@michaelhtm: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
2 participants
Description of changes:
Currently, We've been setting a resource as NotSynced in ReadOne, if the
resource status is not a certain way (Eg. waiting for EKS Cluster to be
Active), and also checking the same resource Status in sdkUpdate.
This is the first attempt at addressing that issue. We are going to need
to ensure every controller/resource returns the proper NotSynced reason
and the requeue error inside the condition. This will allow us to set a
custom requeue right from ReadOne, that will be returned in preUpdate.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.