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.
| klog.V(2).InfoS("Executing the clusterStagedUpdateRun", "clusterStagedUpdateRun", runObjRef, "updatingStageIndex", updatingStageIndex, | ||
| "toBeUpdatedBindings count", len(toBeUpdatedBindings), "toBeDeletedBindings count", len(toBeDeletedBindings)) | ||
| return runtime.Result{RequeueAfter: stageUpdatingWaitTime}, nil | ||
| // The previous run is completed but the update to the status failed. |
There was a problem hiding this comment.
there was a check on the staged run status that got removed. I think this comment is not accurate without that.
There was a problem hiding this comment.
The check I removed is about when all the updating stages finish and the deletion stage is about to start. In that case, updatingStageIndex == len(updatingStages). When updatingStageIndex == -1, it's always when all the stages including the deletion have finished but the recordUpdateRunSucceeded on line 143 somehow fails.
| 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.
| } | ||
| klog.V(2).InfoS("Updated the status of a binding to bound", "binding", klog.KObj(binding), "cluster", clusterStatus.ClusterName, "stage", updatingStageStatus.StageName, "clusterStagedUpdateRun", updateRunRef) | ||
| } else { | ||
| if _, updateErr := checkClusterUpdateResult(binding, clusterStatus, updatingStageStatus, updateRun); updateErr != nil { |
There was a problem hiding this comment.
are those error always retriable?
There was a problem hiding this comment.
For now, yes. Because we currently do not identify any termination errors for cluster updates. I have a TODO in this function. But even in the future when we can identify some, I'll return an errExecutionAborted inside the checkClusterUpdateResult function and can still return the updateErr directly.
| // The delete stage is still updating. | ||
| if condition.IsConditionStatusTrue(deleteStageProgressingCond, updateRun.Generation) { | ||
| klog.InfoS("The delete stage is updating", "clusterStagedUpdateRun", updateRunRef) | ||
| return totalStages, nil | ||
| } | ||
| // All stages have finished, but the delete stage is not active or finished. | ||
| unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the delete stage is not active, but all stages finished")) | ||
| klog.ErrorS(unexpectedErr, "There is no stage active", "clusterStagedUpdateRun", updateRunRef) | ||
| return -1, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) | ||
| } |
There was a problem hiding this comment.
why delete this check?
There was a problem hiding this comment.
After the last updating stage completes, execution does not mark the deletion stage as started. Instead, it returns a 0 wait-time and moves on to the delete stage in the next reconcile loop, just like finishing a regular updating stage and moving on to the next updating stage. With this check, the validation in the next reconcile would fail because the deletion stage is not marked as started yet, so I removed this part.
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