feat: add stopping stage update run implementation #374
Conversation
britaniar
force-pushed
the
stopUpdateRun
branch
from
040bf28 to
878fa65
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
britaniar
force-pushed
the
stopUpdateRun
branch
from
878fa65 to
723e9c9
Compare
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
britaniar
force-pushed
the
stopUpdateRun
branch
from
723e9c9 to
4bee175
Compare
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
| return runtime.Result{}, nil | ||
| } | ||
|
|
||
| func (r *Reconciler) handleIncompleteUpdateRun(ctx context.Context, updateRun placementv1beta1.UpdateRunObj, waitTime time.Duration, err error, state placementv1beta1.State, runObjRef klog.ObjectRef) (runtime.Result, error) { |
There was a problem hiding this comment.
Lets rename it to handleIncompleteUpdateRun, I misunderstood the flow. handleIncompleteUpdateRun makes more sense
| if finishedClusterCount == 0 { | ||
| markStageUpdatingStarted(updatingStageStatus, updateRun.GetGeneration()) | ||
| } | ||
| markStageUpdatingStarted(updatingStageStatus, updateRun.GetGeneration()) |
There was a problem hiding this comment.
Why are we always setting markStageUpdatingStarted now?
There was a problem hiding this comment.
If it was previous false it will update to true (progressing), but it is already true then it won't do anything. But this makes sure it has been marked progressing true when we find any updating cluster.
There was a problem hiding this comment.
I talked about this with Wantong and this was what she had suggested doing. If we keep the previous check then when we restart and we have clusters that have succeed then the finished cluster count is not equal to 0, but we need to mark it as starting since the stage is resuming (progressing).
There was a problem hiding this comment.
Do we set Started condition to false for a stage ?
There was a problem hiding this comment.
While we are trying to stop ("stopping") the progressing condition is unknown, and when it is done stopping ("stopped") the progressing condition is false for the stage and the update run. Only the stage that in the middle of being updated when stopped is updated with these conditions.
There was a problem hiding this comment.
Lets rename markStageUpdatingStarted to markStageUpdatingProgressStarted, I got confused by the name and thought stage has a Started condition but in this case we set Progressing condition to true with Started reason
Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>
| }) | ||
| } | ||
|
|
||
| func checkIfErrorStagedUpdateAborted(err error, updateRun placementv1beta1.UpdateRunObj, updatingStageStatus *placementv1beta1.StageUpdatingStatus) { |
There was a problem hiding this comment.
nit: we should move this to a common util file since it's used across execution, stop states
| for i := 0; i < len(updatingStageStatus.Clusters); i++ { | ||
| clusterStatus := &updatingStageStatus.Clusters[i] | ||
| clusterStartedCond := meta.FindStatusCondition(clusterStatus.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted)) | ||
| if clusterStartedCond == nil || condition.IsConditionStatusFalse(clusterStartedCond, updateRun.GetGeneration()) { |
There was a problem hiding this comment.
We never seem to mark ClusterUpdatingConditionStarted as false, https://github.com/Arvindthiru/kubefleet/blob/9961b09f4f716c97d02619bed8f6f071e39b0e08/apis/placement/v1beta1/stageupdate_types.go#L473-L476
This is what I got confused with https://github.com/kubefleet-dev/kubefleet/pull/374/changes#r2615781769
| klog.ErrorS(unexpectedErr, "The binding should be deleting before we mark a cluster deleting", "clusterStatus", curCluster, "updateRun", updateRunRef) | ||
| return false, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) | ||
| } | ||
| return false, nil |
There was a problem hiding this comment.
Why do we return here ?, we just know that this binding is deleting at the moment why not check other toBeDeletedBindings ?
3 participants
Description of your changes
I have:
Added a check to make sure no cluster start updating and let currently updating cluster finish within a stage before marking the stage and update run as stopped.
Update the stage condition status as stopping or stopped.
Update integration tests and added UTs.
Run
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
While waiting for cluster to finish updating, stage and update run will have a progressing unknown condition with the reason as stopping.