feat: implement clusterStagedUpdateRun execution #1000
Conversation
| continue | ||
| } | ||
| // The cluster status is not deleting yet | ||
| if err := r.Client.Delete(ctx, binding); err != nil { |
There was a problem hiding this comment.
Compared with the original implementation, I did not check if binding is up-to-date before deleting. I don't think it's necessary. Do we want that?
There was a problem hiding this comment.
we do support multiple update run in parallel, I am not sure if there is any race condition that we may delete some other run's binidng.
There was a problem hiding this comment.
In case of both resource changes and scheduling policy changes, the binding to be deleted may not have up-to-date resources. If there are concurrent update runs, the same binding could be deleted by either updateRun. If there's a new scheduling policy change that marks this to-be-deleted binding as scheduled again, this updateRun can fail during validation. Or if there's chance that the change happens right after the validation passes and before deleting the bindings, I think I can add a check to make sure the binding's state is unScheduled or report unexpected error if not.
There was a problem hiding this comment.
I do have a concern: in rollout controller, we make sure to complete the deletion of the bindings on a cluster before scheduling a newly created binding on the same cluster: https://github.com/Azure/fleet/blob/main/pkg/controllers/rollout/controller.go#L226. I wonder if we should have similar logic in the updateRun controller too.
There was a problem hiding this comment.
the concurrent updateRun is not valid if the schedule results are different. This is only for the case one needs to release different versions of the resources so the bindings should all be scheduled or bound.
There was a problem hiding this comment.
As for the check no duplicate logic, should the check at
be suffice?There was a problem hiding this comment.
"the concurrent updateRun is not valid if the schedule results are different. This is only for the case one needs to release different versions of the resources so the bindings should all be scheduled or bound."
In this case, the bindings won't be marked as to-be-deleted because they both have the latest scheduling snapshot index.
| continue | ||
| } | ||
| // The cluster status is not deleting yet | ||
| if err := r.Client.Delete(ctx, binding); err != nil { |
There was a problem hiding this comment.
we do support multiple update run in parallel, I am not sure if there is any race condition that we may delete some other run's binidng.
Description of your changes
Implement clusterStagedUpdateRun execution. Also made some refactoring.
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer