chore: Set cluster status to updating after successful update #169
Conversation
Currently, when we successfully update an EKS cluster, trigger a controller 15 second requeue. During these 15 seconds, the actual cluster Status does not get updated, meaning, if previously it was Active, it will remain Active until the end of the 15 seconds where the controller will change it to Updating. With this change, we will be setting the Updating status ourselves when the update api call is successful, and during the next reconciliation (still 15 seconds) the controller will change it to whatever status is returned by the AWS API.
|
@michaelhtm: The following test failed, say
Full PR test history. Your PR dashboard. Instructions 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: a-hilaly, michaelhtm 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 |
There was a problem hiding this comment.
Q: @michaelhtm @a-hilaly with this change are there any concerns about us showing a partial state transition? If I'm understanding this change we'll immediately set the Cluster Status, but the rest of our Status fields will be pre-update values. So, users will see the status of the cluster change, but none of their changes reflected in the other Status fields. Maybe, not a huge issue since we won't be in a ResourceSynced=True state and we'll become eventually consistent.
| @@ -168,6 +168,7 @@ func clusterDeleting(r *resource) bool { | |||
| func returnClusterUpdating(r *resource) (*resource, error) { | |||
There was a problem hiding this comment.
The function description may need to updated. With this change forcing a re-queue is no longer necessary to see the "UPDATING" status.
| @@ -168,6 +168,7 @@ func clusterDeleting(r *resource) bool { | |||
| func returnClusterUpdating(r *resource) (*resource, error) { | |||
| msg := "Cluster is currently being updated" | |||
| ackcondition.SetSynced(r, corev1.ConditionFalse, &msg, nil) | |||
| r.ko.Status.Status = aws.String(string(svcsdktypes.ClusterStatusUpdating)) | |||
There was a problem hiding this comment.
Q: If we're setting the ResourceSynced condition to false does that not already inform users that the Status fields may not reflect the state of the cluster in AWS?
Description of changes:
Currently, when we successfully update an EKS cluster, trigger a
controller 15 second requeue.
During these 15 seconds, the actual cluster Status does not get updated,
meaning, if previously it was Active, it will remain Active until the
end of the 15 seconds where the controller will change it to Updating.
With this change, we will be setting the Updating status ourselves when
the update api call is successful, and during the next reconciliation
(still 15 seconds) the controller will change it to whatever status is
returned by the AWS API.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.