From 212698b74651ee7fe95445c35616bf128bdd7cee Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 12 Aug 2026 23:52:47 +0000 Subject: [PATCH 1/5] Initial plan From 1d37f2332fa8f2294e6480e385a2d5ccd2126a55 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:04:52 +0000 Subject: [PATCH 2/5] feat: gate delete stage with configured tasks Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com> --- .../2026-08-12-2353-delete-stage-tasks.md | 26 +++ apis/placement/v1beta1/commons.go | 4 + apis/placement/v1beta1/stageupdate_types.go | 9 + .../v1beta1/zz_generated.deepcopy.go | 7 + ...etes-fleet.io_clusterstagedupdateruns.yaml | 33 ++++ ...leet.io_clusterstagedupdatestrategies.yaml | 32 ++++ ....kubernetes-fleet.io_stagedupdateruns.yaml | 33 ++++ ...netes-fleet.io_stagedupdatestrategies.yaml | 32 ++++ pkg/controllers/updaterun/controller.go | 7 +- pkg/controllers/updaterun/controller_test.go | 22 +++ pkg/controllers/updaterun/execution.go | 60 +++++-- .../updaterun/execution_integration_test.go | 82 +++++++++ pkg/controllers/updaterun/execution_test.go | 156 ++++++++++++++++++ pkg/controllers/updaterun/initialization.go | 17 +- .../initialization_integration_test.go | 15 +- .../api_validation_integration_test.go | 52 ++++++ 16 files changed, 569 insertions(+), 18 deletions(-) create mode 100644 .github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md diff --git a/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md b/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md new file mode 100644 index 000000000..eda6d3631 --- /dev/null +++ b/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md @@ -0,0 +1,26 @@ +# Support Delete Stage Tasks + +## Overview + +Allow staged update strategies to gate the implicit deletion stage with the existing Approval and TimedWait task types. + +## Plan + +1. Add `deleteStageTasks` to the shared update strategy API with the same validation as after-stage tasks. +2. Snapshot and initialize delete-stage task statuses. +3. Refactor the after-stage task evaluator to accept a stage configuration and status directly, then use it before deleting bindings. +4. Add API validation, unit, and cluster-scoped/namespaced integration coverage. +5. Regenerate API code and CRDs, then run targeted tests and repository quality checks. + +## Success Criteria + +- [ ] Unset delete-stage tasks preserve immediate deletion. +- [ ] Approval and timed waits can independently gate deletion. +- [ ] Approval and timed waits run concurrently and both must pass. +- [ ] Cluster-scoped and namespaced runs retain bindings until their gate passes. +- [ ] Generated API and CRD artifacts are current. +- [ ] Targeted tests and repository quality checks pass. + +## Approval + +Implementation was explicitly requested in the issue task. diff --git a/apis/placement/v1beta1/commons.go b/apis/placement/v1beta1/commons.go index 3800d8817..712ac367a 100644 --- a/apis/placement/v1beta1/commons.go +++ b/apis/placement/v1beta1/commons.go @@ -168,6 +168,8 @@ const ( // UpdateRunDeleteStageName is the name of delete stage in the staged update run. UpdateRunDeleteStageName = FleetPrefix + "deleteStage" + // UpdateRunDeleteStageLabelValue is the label value used for the delete stage. + UpdateRunDeleteStageLabelValue = "deleteStage" // IsLatestUpdateRunApprovalLabel indicates if the approval is the latest approval on a staged run. IsLatestUpdateRunApprovalLabel = FleetPrefix + "isLatestUpdateRunApproval" @@ -186,6 +188,8 @@ const ( // AfterStageApprovalTaskNameFmt is the format of the after stage approval task name. AfterStageApprovalTaskNameFmt = "%s-after-%s" + // DeleteStageApprovalTaskNameFmt is the format of the delete stage approval task name. + DeleteStageApprovalTaskNameFmt = "%s-after-delete-stage" ) var ( diff --git a/apis/placement/v1beta1/stageupdate_types.go b/apis/placement/v1beta1/stageupdate_types.go index 0054bba0e..53231214a 100644 --- a/apis/placement/v1beta1/stageupdate_types.go +++ b/apis/placement/v1beta1/stageupdate_types.go @@ -266,6 +266,15 @@ type UpdateStrategySpec struct { // +kubebuilder:validation:MaxItems=31 // +kubebuilder:validation:Required Stages []StageConfig `json:"stages"` + + // DeleteStageTasks is the collection of tasks that must complete before the deletion stage starts. + // Each task is executed in parallel and there cannot be more than one task of the same type. + // +kubebuilder:validation:MaxItems=2 + // +kubebuilder:validation:Optional + // +kubebuilder:validation:XValidation:rule="!(self.size() == 2 && self[0].type == self[1].type)",message="deleteStageTasks cannot have two tasks of the same type" + // +kubebuilder:validation:XValidation:rule="!self.exists(e, e.type == 'Approval' && has(e.waitTime))",message="DeleteStageTaskType is Approval, waitTime is not allowed" + // +kubebuilder:validation:XValidation:rule="!self.exists(e, e.type == 'TimedWait' && !has(e.waitTime))",message="DeleteStageTaskType is TimedWait, waitTime is required" + DeleteStageTasks []StageTask `json:"deleteStageTasks,omitempty"` } // ClusterStagedUpdateStrategyList contains a list of StagedUpdateStrategy. diff --git a/apis/placement/v1beta1/zz_generated.deepcopy.go b/apis/placement/v1beta1/zz_generated.deepcopy.go index 73d66c8fa..ca3d28c03 100644 --- a/apis/placement/v1beta1/zz_generated.deepcopy.go +++ b/apis/placement/v1beta1/zz_generated.deepcopy.go @@ -3044,6 +3044,13 @@ func (in *UpdateStrategySpec) DeepCopyInto(out *UpdateStrategySpec) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.DeleteStageTasks != nil { + in, out := &in.DeleteStageTasks, &out.DeleteStageTasks + *out = make([]StageTask, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UpdateStrategySpec. diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml index 27dc987da..2bc41a455 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml @@ -3242,6 +3242,39 @@ spec: The update run fails to initialize if the strategy fails to produce a valid list of stages where each selected cluster is included in exactly one stage. properties: + deleteStageTasks: + description: |- + DeleteStageTasks is the collection of tasks that must complete before the deletion stage starts. + Each task is executed in parallel and there cannot be more than one task of the same type. + items: + description: StageTask is the pre or post stage task that needs + to be completed before starting or moving to the next stage. + properties: + type: + description: The type of the before or after stage task. + enum: + - TimedWait + - Approval + type: string + waitTime: + description: |- + The time to wait after all the clusters in the current stage complete the update before moving to the next stage. + Only hours (h), minutes (m), and seconds (s) units are accepted. + pattern: ^(?:(?:0|[1-9][0-9]*)(\.[0-9]+)?(?:s|m|h))+$ + type: string + required: + - type + type: object + maxItems: 2 + type: array + x-kubernetes-validations: + - message: deleteStageTasks cannot have two tasks of the same + type + rule: '!(self.size() == 2 && self[0].type == self[1].type)' + - message: DeleteStageTaskType is Approval, waitTime is not allowed + rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime is required + rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' stages: description: Stage specifies the configuration for each update stage. diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml index b134a75b5..d7003798e 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml @@ -375,6 +375,38 @@ spec: spec: description: The desired state of ClusterStagedUpdateStrategy. properties: + deleteStageTasks: + description: |- + DeleteStageTasks is the collection of tasks that must complete before the deletion stage starts. + Each task is executed in parallel and there cannot be more than one task of the same type. + items: + description: StageTask is the pre or post stage task that needs + to be completed before starting or moving to the next stage. + properties: + type: + description: The type of the before or after stage task. + enum: + - TimedWait + - Approval + type: string + waitTime: + description: |- + The time to wait after all the clusters in the current stage complete the update before moving to the next stage. + Only hours (h), minutes (m), and seconds (s) units are accepted. + pattern: ^(?:(?:0|[1-9][0-9]*)(\.[0-9]+)?(?:s|m|h))+$ + type: string + required: + - type + type: object + maxItems: 2 + type: array + x-kubernetes-validations: + - message: deleteStageTasks cannot have two tasks of the same type + rule: '!(self.size() == 2 && self[0].type == self[1].type)' + - message: DeleteStageTaskType is Approval, waitTime is not allowed + rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime is required + rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' stages: description: Stage specifies the configuration for each update stage. items: diff --git a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml index 81b506f4a..df0ab065d 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml @@ -2162,6 +2162,39 @@ spec: The update run fails to initialize if the strategy fails to produce a valid list of stages where each selected cluster is included in exactly one stage. properties: + deleteStageTasks: + description: |- + DeleteStageTasks is the collection of tasks that must complete before the deletion stage starts. + Each task is executed in parallel and there cannot be more than one task of the same type. + items: + description: StageTask is the pre or post stage task that needs + to be completed before starting or moving to the next stage. + properties: + type: + description: The type of the before or after stage task. + enum: + - TimedWait + - Approval + type: string + waitTime: + description: |- + The time to wait after all the clusters in the current stage complete the update before moving to the next stage. + Only hours (h), minutes (m), and seconds (s) units are accepted. + pattern: ^(?:(?:0|[1-9][0-9]*)(\.[0-9]+)?(?:s|m|h))+$ + type: string + required: + - type + type: object + maxItems: 2 + type: array + x-kubernetes-validations: + - message: deleteStageTasks cannot have two tasks of the same + type + rule: '!(self.size() == 2 && self[0].type == self[1].type)' + - message: DeleteStageTaskType is Approval, waitTime is not allowed + rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime is required + rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' stages: description: Stage specifies the configuration for each update stage. diff --git a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml index ccfb44e85..a3854c780 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml @@ -235,6 +235,38 @@ spec: spec: description: The desired state of StagedUpdateStrategy. properties: + deleteStageTasks: + description: |- + DeleteStageTasks is the collection of tasks that must complete before the deletion stage starts. + Each task is executed in parallel and there cannot be more than one task of the same type. + items: + description: StageTask is the pre or post stage task that needs + to be completed before starting or moving to the next stage. + properties: + type: + description: The type of the before or after stage task. + enum: + - TimedWait + - Approval + type: string + waitTime: + description: |- + The time to wait after all the clusters in the current stage complete the update before moving to the next stage. + Only hours (h), minutes (m), and seconds (s) units are accepted. + pattern: ^(?:(?:0|[1-9][0-9]*)(\.[0-9]+)?(?:s|m|h))+$ + type: string + required: + - type + type: object + maxItems: 2 + type: array + x-kubernetes-validations: + - message: deleteStageTasks cannot have two tasks of the same type + rule: '!(self.size() == 2 && self[0].type == self[1].type)' + - message: DeleteStageTaskType is Approval, waitTime is not allowed + rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime is required + rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' stages: description: Stage specifies the configuration for each update stage. items: diff --git a/pkg/controllers/updaterun/controller.go b/pkg/controllers/updaterun/controller.go index c551fa528..eeea30869 100644 --- a/pkg/controllers/updaterun/controller.go +++ b/pkg/controllers/updaterun/controller.go @@ -493,7 +493,7 @@ func handleApprovalRequestDelete(obj client.Object, q workqueue.TypedRateLimitin } func removeWaitTimeFromUpdateRunStatus(updateRun placementv1beta1.UpdateRunObj) { - // Remove waitTime from the updateRun status for BeforeStageTask and AfterStageTask for type Approval. + // Remove waitTime from the updateRun status for Approval tasks. updateRunStatus := updateRun.GetUpdateRunStatus() if updateRunStatus.UpdateStrategySnapshot != nil { for i := range updateRunStatus.UpdateStrategySnapshot.Stages { @@ -508,6 +508,11 @@ func removeWaitTimeFromUpdateRunStatus(updateRun placementv1beta1.UpdateRunObj) } } } + for i := range updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks { + if updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks[i].Type == placementv1beta1.StageTaskTypeApproval { + updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks[i].WaitTime = nil + } + } } } diff --git a/pkg/controllers/updaterun/controller_test.go b/pkg/controllers/updaterun/controller_test.go index 5bae2f70a..e20e3f981 100644 --- a/pkg/controllers/updaterun/controller_test.go +++ b/pkg/controllers/updaterun/controller_test.go @@ -986,6 +986,28 @@ func TestRemoveWaitTimeFromUpdateRunStatus(t *testing.T) { }, }, }, + "should remove waitTime from Approval tasks only for DeleteStageTasks": { + inputUpdateRun: &placementv1beta1.ClusterStagedUpdateRun{ + Status: placementv1beta1.UpdateRunStatus{ + UpdateStrategySnapshot: &placementv1beta1.UpdateStrategySpec{ + DeleteStageTasks: []placementv1beta1.StageTask{ + {Type: placementv1beta1.StageTaskTypeApproval, WaitTime: &waitTime}, + {Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &waitTime}, + }, + }, + }, + }, + wantUpdateRun: &placementv1beta1.ClusterStagedUpdateRun{ + Status: placementv1beta1.UpdateRunStatus{ + UpdateStrategySnapshot: &placementv1beta1.UpdateStrategySpec{ + DeleteStageTasks: []placementv1beta1.StageTask{ + {Type: placementv1beta1.StageTaskTypeApproval}, + {Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &waitTime}, + }, + }, + }, + }, + }, "should handle multiple stages": { inputUpdateRun: &placementv1beta1.ClusterStagedUpdateRun{ Status: placementv1beta1.UpdateRunStatus{ diff --git a/pkg/controllers/updaterun/execution.go b/pkg/controllers/updaterun/execution.go index b8c392806..cf89770af 100644 --- a/pkg/controllers/updaterun/execution.go +++ b/pkg/controllers/updaterun/execution.go @@ -101,8 +101,7 @@ func (r *Reconciler) execute( return false, waitTime, err } // All the stages have finished, now start the delete stage. - finished, err = r.executeDeleteStage(ctx, updateRun, toBeDeletedBindings) - return finished, clusterUpdatingWaitTime, err + return r.executeDeleteStage(ctx, updateRun, toBeDeletedBindings) } // checkBeforeStageTasksStatus checks if the before stage tasks have finished. @@ -318,7 +317,13 @@ func (r *Reconciler) handleStageCompletion( markStageUpdatingWaiting(updatingStageStatus, updateRun.GetGeneration(), "All clusters in the stage are updated, waiting for after-stage tasks to complete") klog.V(2).InfoS("The stage has finished all cluster updating", "stage", updatingStageStatus.StageName, "updateRun", updateRunRef) // Check if the after stage tasks are ready. - approved, waitTime, err := r.checkAfterStageTasksStatus(ctx, updatingStageIndex, updateRun) + updateRunStatus := updateRun.GetUpdateRunStatus() + approved, waitTime, err := r.checkAfterStageTasksStatus( + ctx, + &updateRunStatus.UpdateStrategySnapshot.Stages[updatingStageIndex], + updatingStageStatus, + updateRun, + ) if err != nil { return 0, err } @@ -340,10 +345,29 @@ func (r *Reconciler) executeDeleteStage( ctx context.Context, updateRun placementv1beta1.UpdateRunObj, toBeDeletedBindings []placementv1beta1.BindingObj, -) (bool, error) { +) (bool, time.Duration, error) { updateRunRef := klog.KObj(updateRun) updateRunStatus := updateRun.GetUpdateRunStatus() existingDeleteStageStatus := updateRunStatus.DeletionStageStatus + deleteStage := &placementv1beta1.StageConfig{ + Name: placementv1beta1.UpdateRunDeleteStageName, + AfterStageTasks: updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks, + } + + markUpdateRunWaiting(updateRun, fmt.Sprintf(condition.UpdateRunWaitingMessageFmt, "after-stage", existingDeleteStageStatus.StageName)) + markStageUpdatingWaiting(existingDeleteStageStatus, updateRun.GetGeneration(), "Waiting for delete stage tasks to complete") + approved, waitTime, err := r.checkAfterStageTasksStatus(ctx, deleteStage, existingDeleteStageStatus, updateRun) + if err != nil { + return false, 0, err + } + if !approved { + if waitTime < 0 { + waitTime = stageUpdatingWaitTime + } + return false, waitTime, nil + } + markUpdateRunProgressing(updateRun) + existingDeleteStageClusterMap := make(map[string]*placementv1beta1.ClusterUpdatingStatus, len(existingDeleteStageStatus.Clusters)) for i := range existingDeleteStageStatus.Clusters { existingDeleteStageClusterMap[existingDeleteStageStatus.Clusters[i].ClusterName] = &existingDeleteStageStatus.Clusters[i] @@ -357,7 +381,7 @@ func (r *Reconciler) executeDeleteStage( // This is unexpected because we already checked in validation. missingErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the to be deleted cluster `%s` is not in the deleting stage during execution", bindingSpec.TargetCluster)) klog.ErrorS(missingErr, "The cluster in the deleting stage does not include all the to be deleted binding", "updateRun", updateRunRef) - return false, fmt.Errorf("%w: %s", errStagedUpdatedAborted, missingErr.Error()) + return false, 0, fmt.Errorf("%w: %s", errStagedUpdatedAborted, missingErr.Error()) } // In validation, we already check the binding must exist in the status. delete(existingDeleteStageClusterMap, bindingSpec.TargetCluster) @@ -365,7 +389,7 @@ func (r *Reconciler) executeDeleteStage( if condition.IsConditionStatusTrue(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionSucceeded)), updateRun.GetGeneration()) { unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the deleted cluster `%s` in the deleting stage still has a binding", bindingSpec.TargetCluster)) klog.ErrorS(unexpectedErr, "The cluster in the deleting stage is not removed yet but marked as deleted", "cluster", curCluster.ClusterName, "updateRun", updateRunRef) - return false, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) + return false, 0, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) } if condition.IsConditionStatusTrue(meta.FindStatusCondition(curCluster.Conditions, string(placementv1beta1.ClusterUpdatingConditionStarted)), updateRun.GetGeneration()) { // The cluster status is marked as being deleted. @@ -373,14 +397,14 @@ func (r *Reconciler) executeDeleteStage( // The cluster is marked as deleting but the binding is not deleting. unexpectedErr := controller.NewUnexpectedBehaviorError(fmt.Errorf("the cluster `%s` in the deleting stage is marked as deleting but its corresponding binding is not deleting", curCluster.ClusterName)) 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, 0, fmt.Errorf("%w: %s", errStagedUpdatedAborted, unexpectedErr.Error()) } continue } // The cluster status is not deleting yet if err := r.Client.Delete(ctx, binding); err != nil { klog.ErrorS(err, "Failed to delete a binding in the update run", "binding", klog.KObj(binding), "cluster", curCluster.ClusterName, "updateRun", updateRunRef) - return false, controller.NewAPIServerError(false, err) + return false, 0, controller.NewAPIServerError(false, err) } klog.V(2).InfoS("Deleted a binding pointing to a to be deleted cluster", "binding", klog.KObj(binding), "cluster", curCluster.ClusterName, "updateRun", updateRunRef) markClusterUpdatingStarted(curCluster, updateRun.GetGeneration()) @@ -397,17 +421,19 @@ func (r *Reconciler) executeDeleteStage( if len(toBeDeletedBindings) == 0 { markStageUpdatingSucceeded(updateRunStatus.DeletionStageStatus, updateRun.GetGeneration()) } - return len(toBeDeletedBindings) == 0, nil + return len(toBeDeletedBindings) == 0, clusterUpdatingWaitTime, nil } // checkAfterStageTasksStatus checks if the after stage tasks have finished. // It returns if the after stage tasks have finished or error if the after stage tasks failed. // It also returns the time to wait before rechecking the wait type of task. It turns -1 if the task is not a wait type. -func (r *Reconciler) checkAfterStageTasksStatus(ctx context.Context, updatingStageIndex int, updateRun placementv1beta1.UpdateRunObj) (bool, time.Duration, error) { +func (r *Reconciler) checkAfterStageTasksStatus( + ctx context.Context, + updatingStage *placementv1beta1.StageConfig, + updatingStageStatus *placementv1beta1.StageUpdatingStatus, + updateRun placementv1beta1.UpdateRunObj, +) (bool, time.Duration, error) { updateRunRef := klog.KObj(updateRun) - updateRunStatus := updateRun.GetUpdateRunStatus() - updatingStageStatus := &updateRunStatus.StagesStatus[updatingStageIndex] - updatingStage := &updateRunStatus.UpdateStrategySnapshot.Stages[updatingStageIndex] if updatingStage.AfterStageTasks == nil { klog.V(2).InfoS("There is no after stage task for this stage", "stage", updatingStage.Name, "updateRun", updateRunRef) return true, 0, nil @@ -633,13 +659,17 @@ func checkClusterUpdateResult( // buildApprovalRequestObject creates an approval request object for before-stage or after-stage tasks. // It returns a ClusterApprovalRequest if namespace is empty, otherwise returns an ApprovalRequest. func buildApprovalRequestObject(namespacedName types.NamespacedName, stageName, updateRunName, stageTaskType string) placementv1beta1.ApprovalRequestObj { + stageLabelValue := stageName + if stageName == placementv1beta1.UpdateRunDeleteStageName { + stageLabelValue = placementv1beta1.UpdateRunDeleteStageLabelValue + } var approvalRequest placementv1beta1.ApprovalRequestObj if namespacedName.Namespace == "" { approvalRequest = &placementv1beta1.ClusterApprovalRequest{ ObjectMeta: metav1.ObjectMeta{ Name: namespacedName.Name, Labels: map[string]string{ - placementv1beta1.TargetUpdatingStageNameLabel: stageName, + placementv1beta1.TargetUpdatingStageNameLabel: stageLabelValue, placementv1beta1.TargetUpdateRunLabel: updateRunName, placementv1beta1.TaskTypeLabel: stageTaskType, placementv1beta1.IsLatestUpdateRunApprovalLabel: "true", @@ -656,7 +686,7 @@ func buildApprovalRequestObject(namespacedName types.NamespacedName, stageName, Name: namespacedName.Name, Namespace: namespacedName.Namespace, Labels: map[string]string{ - placementv1beta1.TargetUpdatingStageNameLabel: stageName, + placementv1beta1.TargetUpdatingStageNameLabel: stageLabelValue, placementv1beta1.TargetUpdateRunLabel: updateRunName, placementv1beta1.TaskTypeLabel: stageTaskType, placementv1beta1.IsLatestUpdateRunApprovalLabel: "true", diff --git a/pkg/controllers/updaterun/execution_integration_test.go b/pkg/controllers/updaterun/execution_integration_test.go index 12ef4bbb1..4db8f6783 100644 --- a/pkg/controllers/updaterun/execution_integration_test.go +++ b/pkg/controllers/updaterun/execution_integration_test.go @@ -1966,6 +1966,88 @@ var _ = Describe("UpdateRun execution tests - single stage", func() { }) }) +var _ = Describe("Delete stage task execution tests", func() { + It("Should gate cluster-scoped binding deletion on approval", func() { + verifyDeleteStageApprovalGate(false) + }) + + It("Should gate namespaced binding deletion on approval", func() { + verifyDeleteStageApprovalGate(true) + }) +}) + +func verifyDeleteStageApprovalGate(namespaced bool) { + name := fmt.Sprintf("delete-stage-gate-%d", GinkgoParallelProcess()) + namespace := "" + var updateRun placementv1beta1.UpdateRunObj + var binding placementv1beta1.BindingObj + var approvalRequest placementv1beta1.ApprovalRequestObj + if namespaced { + namespace = testNamespaceName + updateRun = &placementv1beta1.StagedUpdateRun{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace, Generation: 1}} + binding = &placementv1beta1.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: namespace}, + Spec: placementv1beta1.ResourceBindingSpec{TargetCluster: "cluster-1"}, + } + approvalRequest = &placementv1beta1.ApprovalRequest{} + } else { + updateRun = &placementv1beta1.ClusterStagedUpdateRun{ObjectMeta: metav1.ObjectMeta{Name: name, Generation: 1}} + binding = &placementv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: name}, + Spec: placementv1beta1.ResourceBindingSpec{TargetCluster: "cluster-1"}, + } + approvalRequest = &placementv1beta1.ClusterApprovalRequest{} + } + approvalRequestName := fmt.Sprintf(placementv1beta1.DeleteStageApprovalTaskNameFmt, name) + updateRunStatus := updateRun.GetUpdateRunStatus() + updateRunStatus.UpdateStrategySnapshot = &placementv1beta1.UpdateStrategySpec{ + DeleteStageTasks: []placementv1beta1.StageTask{{Type: placementv1beta1.StageTaskTypeApproval}}, + } + updateRunStatus.DeletionStageStatus = &placementv1beta1.StageUpdatingStatus{ + StageName: placementv1beta1.UpdateRunDeleteStageName, + Clusters: []placementv1beta1.ClusterUpdatingStatus{{ClusterName: "cluster-1"}}, + AfterStageTaskStatus: []placementv1beta1.StageTaskStatus{{ + Type: placementv1beta1.StageTaskTypeApproval, + ApprovalRequestName: approvalRequestName, + }}, + } + + Expect(k8sClient.Create(ctx, binding)).Should(Succeed()) + DeferCleanup(func() { + _ = k8sClient.Delete(ctx, binding) + _ = k8sClient.Delete(ctx, approvalRequest) + }) + + r := &Reconciler{Client: k8sClient} + finished, _, err := r.executeDeleteStage(ctx, updateRun, []placementv1beta1.BindingObj{binding}) + Expect(err).NotTo(HaveOccurred()) + Expect(finished).To(BeFalse()) + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, binding)).Should(Succeed()) + + Expect(k8sClient.Get(ctx, types.NamespacedName{Name: approvalRequestName, Namespace: namespace}, approvalRequest)).Should(Succeed()) + Expect(approvalRequest.GetLabels()[placementv1beta1.TargetUpdatingStageNameLabel]).To(Equal(placementv1beta1.UpdateRunDeleteStageLabelValue)) + approvalRequestStatus := approvalRequest.GetApprovalRequestStatus() + meta.SetStatusCondition(&approvalRequestStatus.Conditions, metav1.Condition{ + Type: string(placementv1beta1.ApprovalRequestConditionApproved), + Status: metav1.ConditionTrue, + ObservedGeneration: approvalRequest.GetGeneration(), + Reason: "Approved", + Message: "approved", + }) + Expect(k8sClient.Status().Update(ctx, approvalRequest)).Should(Succeed()) + + finished, _, err = r.executeDeleteStage(ctx, updateRun, []placementv1beta1.BindingObj{binding}) + Expect(err).NotTo(HaveOccurred()) + Expect(finished).To(BeFalse()) + Eventually(func() error { + err := k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, binding) + if apierrors.IsNotFound(err) { + return nil + } + return fmt.Errorf("binding get error = %v, want not found", err) + }, timeout, interval).Should(Succeed(), "binding should be deleted after approval") +} + func validateBindingState(ctx context.Context, binding *placementv1beta1.ClusterResourceBinding, resourceSnapshotName string, updateRun *placementv1beta1.ClusterStagedUpdateRun, clusterStatus *placementv1beta1.ClusterUpdatingStatus) { Eventually(func() error { if err := k8sClient.Get(ctx, types.NamespacedName{Name: binding.Name}, binding); err != nil { diff --git a/pkg/controllers/updaterun/execution_test.go b/pkg/controllers/updaterun/execution_test.go index c65d8fb1f..e6ea4a563 100644 --- a/pkg/controllers/updaterun/execution_test.go +++ b/pkg/controllers/updaterun/execution_test.go @@ -26,6 +26,7 @@ import ( "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -1233,6 +1234,7 @@ func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) { wantErrMsg: fmt.Sprintf("error returned by the API server: clusterapprovalrequests.placement.kubernetes-fleet.io \"%s\" not found", approvalRequestName), }, } + for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { objects := []client.Object{tt.updateRun} @@ -1265,6 +1267,160 @@ func TestCheckBeforeStageTasksStatus_NegativeCases(t *testing.T) { } } +func TestCheckAfterStageTasksStatus(t *testing.T) { + const ( + updateRunName = "test-update-run" + stageName = "test-stage" + ) + waitTime := time.Minute + now := time.Now() + tests := []struct { + name string + tasks []placementv1beta1.StageTask + taskStatuses []placementv1beta1.StageTaskStatus + waitStarted time.Time + wantPassed bool + wantPositiveWait bool + }{ + { + name: "unset tasks pass immediately", + wantPassed: true, + }, + { + name: "approved approval-only task passes", + tasks: []placementv1beta1.StageTask{{Type: placementv1beta1.StageTaskTypeApproval}}, + taskStatuses: []placementv1beta1.StageTaskStatus{{ + Type: placementv1beta1.StageTaskTypeApproval, + ApprovalRequestName: fmt.Sprintf(placementv1beta1.AfterStageApprovalTaskNameFmt, updateRunName, stageName), + Conditions: []metav1.Condition{{ + Type: string(placementv1beta1.StageTaskConditionApprovalRequestApproved), + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }}, + }}, + waitStarted: now, + wantPassed: true, + }, + { + name: "timed-wait-only task waits", + tasks: []placementv1beta1.StageTask{{Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &metav1.Duration{Duration: waitTime}}}, + taskStatuses: []placementv1beta1.StageTaskStatus{{Type: placementv1beta1.StageTaskTypeTimedWait}}, + waitStarted: now, + wantPositiveWait: true, + }, + { + name: "timed-wait-only task passes after wait", + tasks: []placementv1beta1.StageTask{{Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &metav1.Duration{Duration: waitTime}}}, + taskStatuses: []placementv1beta1.StageTaskStatus{{Type: placementv1beta1.StageTaskTypeTimedWait}}, + waitStarted: now.Add(-2 * waitTime), + wantPassed: true, + }, + { + name: "approval before timed wait still waits", + tasks: []placementv1beta1.StageTask{ + {Type: placementv1beta1.StageTaskTypeApproval}, + {Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &metav1.Duration{Duration: waitTime}}, + }, + taskStatuses: []placementv1beta1.StageTaskStatus{ + { + Type: placementv1beta1.StageTaskTypeApproval, + ApprovalRequestName: fmt.Sprintf(placementv1beta1.AfterStageApprovalTaskNameFmt, updateRunName, stageName), + Conditions: []metav1.Condition{{ + Type: string(placementv1beta1.StageTaskConditionApprovalRequestApproved), + Status: metav1.ConditionTrue, + ObservedGeneration: 1, + }}, + }, + {Type: placementv1beta1.StageTaskTypeTimedWait}, + }, + waitStarted: now, + wantPositiveWait: true, + }, + { + name: "timed wait before approval still waits", + tasks: []placementv1beta1.StageTask{ + {Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &metav1.Duration{Duration: waitTime}}, + {Type: placementv1beta1.StageTaskTypeApproval}, + }, + taskStatuses: []placementv1beta1.StageTaskStatus{ + {Type: placementv1beta1.StageTaskTypeTimedWait}, + { + Type: placementv1beta1.StageTaskTypeApproval, + ApprovalRequestName: fmt.Sprintf(placementv1beta1.AfterStageApprovalTaskNameFmt, updateRunName, stageName), + }, + }, + waitStarted: now.Add(-2 * waitTime), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + scheme := runtime.NewScheme() + if err := placementv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme() error = %v", err) + } + r := &Reconciler{Client: fake.NewClientBuilder().WithScheme(scheme).Build()} + updateRun := &placementv1beta1.ClusterStagedUpdateRun{ + ObjectMeta: metav1.ObjectMeta{Name: updateRunName, Generation: 1}, + } + stage := &placementv1beta1.StageConfig{Name: stageName, AfterStageTasks: tt.tasks} + stageStatus := &placementv1beta1.StageUpdatingStatus{ + StageName: stageName, + AfterStageTaskStatus: tt.taskStatuses, + Conditions: []metav1.Condition{{ + Type: string(placementv1beta1.StageUpdatingConditionProgressing), + Status: metav1.ConditionFalse, + ObservedGeneration: 1, + LastTransitionTime: metav1.NewTime(tt.waitStarted), + }}, + } + + gotPassed, gotWait, err := r.checkAfterStageTasksStatus(context.Background(), stage, stageStatus, updateRun) + if err != nil { + t.Fatalf("checkAfterStageTasksStatus() error = %v", err) + } + if gotPassed != tt.wantPassed { + t.Errorf("checkAfterStageTasksStatus() passed = %v, want %v", gotPassed, tt.wantPassed) + } + if (gotWait > 0) != tt.wantPositiveWait { + t.Errorf("checkAfterStageTasksStatus() wait = %v, wantPositiveWait %v", gotWait, tt.wantPositiveWait) + } + }) + } +} + +func TestExecuteDeleteStageWithoutTasks(t *testing.T) { + scheme := runtime.NewScheme() + if err := placementv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme() error = %v", err) + } + binding := &placementv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: placementv1beta1.ResourceBindingSpec{ + TargetCluster: "cluster-1", + }, + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(binding).Build() + r := &Reconciler{Client: fakeClient} + updateRun := &placementv1beta1.ClusterStagedUpdateRun{ + ObjectMeta: metav1.ObjectMeta{Name: "test-update-run", Generation: 1}, + Status: placementv1beta1.UpdateRunStatus{ + UpdateStrategySnapshot: &placementv1beta1.UpdateStrategySpec{}, + DeletionStageStatus: &placementv1beta1.StageUpdatingStatus{ + StageName: placementv1beta1.UpdateRunDeleteStageName, + Clusters: []placementv1beta1.ClusterUpdatingStatus{{ClusterName: "cluster-1"}}, + }, + }, + } + + if _, _, err := r.executeDeleteStage(context.Background(), updateRun, []placementv1beta1.BindingObj{binding}); err != nil { + t.Fatalf("executeDeleteStage() error = %v", err) + } + if err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(binding), &placementv1beta1.ClusterResourceBinding{}); !apierrors.IsNotFound(err) { + t.Fatalf("executeDeleteStage() binding get error = %v, want not found", err) + } +} + func TestGenerateStuckClustersString(t *testing.T) { tests := []struct { name string diff --git a/pkg/controllers/updaterun/initialization.go b/pkg/controllers/updaterun/initialization.go index 7a3217462..2c27f87d0 100644 --- a/pkg/controllers/updaterun/initialization.go +++ b/pkg/controllers/updaterun/initialization.go @@ -282,9 +282,15 @@ func (r *Reconciler) generateStagesByStrategy( updateStrategySpec := updateStrategy.GetUpdateStrategySpec() updateRunStatus.UpdateStrategySnapshot = updateStrategySpec - // Remove waitTime from the updateRun status for BeforeStageTask and AfterStageTask for type Approval. + // Remove waitTime from the updateRun status for Approval tasks. removeWaitTimeFromUpdateRunStatus(updateRun) + if err := validateAfterStageTask(updateStrategySpec.DeleteStageTasks); err != nil { + klog.ErrorS(err, "Failed to validate the delete stage tasks", "updateStrategy", strategyKey, "updateRun", updateRunRef) + invalidDeleteStageErr := controller.NewUserError(fmt.Errorf("the delete stage tasks are invalid, updateStrategy: `%s`, err: %s", strategyKey, err.Error())) + return fmt.Errorf("%w: %s", errValidationFailed, invalidDeleteStageErr.Error()) + } + // Compute the update stages. if err := r.computeRunStageStatus(ctx, scheduledBindings, updateRun); err != nil { return err @@ -305,6 +311,15 @@ func (r *Reconciler) generateStagesByStrategy( StageName: placementv1beta1.UpdateRunDeleteStageName, Clusters: toBeDeletedClusters, } + if len(updateStrategySpec.DeleteStageTasks) > 0 { + updateRunStatus.DeletionStageStatus.AfterStageTaskStatus = make([]placementv1beta1.StageTaskStatus, len(updateStrategySpec.DeleteStageTasks)) + for i, task := range updateStrategySpec.DeleteStageTasks { + updateRunStatus.DeletionStageStatus.AfterStageTaskStatus[i].Type = task.Type + if task.Type == placementv1beta1.StageTaskTypeApproval { + updateRunStatus.DeletionStageStatus.AfterStageTaskStatus[i].ApprovalRequestName = fmt.Sprintf(placementv1beta1.DeleteStageApprovalTaskNameFmt, updateRun.GetName()) + } + } + } return nil } diff --git a/pkg/controllers/updaterun/initialization_integration_test.go b/pkg/controllers/updaterun/initialization_integration_test.go index f208fc67a..c75646c47 100644 --- a/pkg/controllers/updaterun/initialization_integration_test.go +++ b/pkg/controllers/updaterun/initialization_integration_test.go @@ -697,7 +697,11 @@ var _ = Describe("Updaterun initialization tests", func() { }) It("Should generate the cluster update stage in the status as expected", func() { - By("Creating a clusterStagedUpdateStrategy") + By("Creating a clusterStagedUpdateStrategy with delete stage tasks") + updateStrategy.Spec.DeleteStageTasks = []placementv1beta1.StageTask{ + {Type: placementv1beta1.StageTaskTypeApproval}, + {Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &metav1.Duration{Duration: time.Minute}}, + } Expect(k8sClient.Create(ctx, updateStrategy)).To(Succeed()) By("Creating a new clusterStagedUpdateRun") @@ -1171,6 +1175,15 @@ func generateInitializedStatus( }, } populateStageTaskStatuses(status, updateRun.Name, updateStrategy.Spec.Stages) + if len(updateStrategy.Spec.DeleteStageTasks) > 0 { + status.DeletionStageStatus.AfterStageTaskStatus = make([]placementv1beta1.StageTaskStatus, len(updateStrategy.Spec.DeleteStageTasks)) + for i, task := range updateStrategy.Spec.DeleteStageTasks { + status.DeletionStageStatus.AfterStageTaskStatus[i].Type = task.Type + if task.Type == placementv1beta1.StageTaskTypeApproval { + status.DeletionStageStatus.AfterStageTaskStatus[i].ApprovalRequestName = fmt.Sprintf(placementv1beta1.DeleteStageApprovalTaskNameFmt, updateRun.Name) + } + } + } return status } diff --git a/test/apis/placement/v1beta1/api_validation_integration_test.go b/test/apis/placement/v1beta1/api_validation_integration_test.go index 3357999e8..25c2a0029 100644 --- a/test/apis/placement/v1beta1/api_validation_integration_test.go +++ b/test/apis/placement/v1beta1/api_validation_integration_test.go @@ -2721,6 +2721,58 @@ var _ = Describe("Test placement v1beta1 API validation", func() { Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("Too many: 3: must have at most 2 items")) }) + It("Should deny creation of ClusterStagedUpdateStrategy with more than 2 DeleteStageTasks", func() { + strategy := placementv1beta1.ClusterStagedUpdateStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(updateRunStrategyNameTemplate, GinkgoParallelProcess()), + }, + Spec: placementv1beta1.UpdateStrategySpec{ + Stages: []placementv1beta1.StageConfig{{Name: fmt.Sprintf(updateRunStageNameTemplate, GinkgoParallelProcess(), 1)}}, + DeleteStageTasks: []placementv1beta1.StageTask{ + {Type: placementv1beta1.StageTaskTypeApproval}, + {Type: placementv1beta1.StageTaskTypeApproval}, + {Type: placementv1beta1.StageTaskTypeTimedWait, WaitTime: &metav1.Duration{Duration: time.Second * 10}}, + }, + }, + } + err := hubClient.Create(ctx, &strategy) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create updateRunStrategy call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) + Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("Too many: 3: must have at most 2 items")) + }) + + It("Should deny creation of ClusterStagedUpdateStrategy with duplicate DeleteStageTasks", func() { + strategy := placementv1beta1.ClusterStagedUpdateStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(updateRunStrategyNameTemplate, GinkgoParallelProcess()), + }, + Spec: placementv1beta1.UpdateStrategySpec{ + Stages: []placementv1beta1.StageConfig{{Name: fmt.Sprintf(updateRunStageNameTemplate, GinkgoParallelProcess(), 1)}}, + DeleteStageTasks: []placementv1beta1.StageTask{{Type: placementv1beta1.StageTaskTypeApproval}, {Type: placementv1beta1.StageTaskTypeApproval}}, + }, + } + err := hubClient.Create(ctx, &strategy) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create updateRunStrategy call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) + Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("deleteStageTasks cannot have two tasks of the same type")) + }) + + It("Should deny creation of ClusterStagedUpdateStrategy with invalid DeleteStageTasks waitTime", func() { + strategy := placementv1beta1.ClusterStagedUpdateStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(updateRunStrategyNameTemplate, GinkgoParallelProcess()), + }, + Spec: placementv1beta1.UpdateStrategySpec{ + Stages: []placementv1beta1.StageConfig{{Name: fmt.Sprintf(updateRunStageNameTemplate, GinkgoParallelProcess(), 1)}}, + DeleteStageTasks: []placementv1beta1.StageTask{{Type: placementv1beta1.StageTaskTypeTimedWait}}, + }, + } + err := hubClient.Create(ctx, &strategy) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create updateRunStrategy call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) + Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("DeleteStageTaskType is TimedWait, waitTime is required")) + }) + It("Should deny creation of ClusterStagedUpdateStrategy with AfterStageTask of type Approval with waitTime specified", func() { strategy := placementv1beta1.ClusterStagedUpdateStrategy{ ObjectMeta: metav1.ObjectMeta{ From 00d38c1bf058fcada089118f7b8984accace98f3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:19:34 +0000 Subject: [PATCH 3/5] fix: avoid reevaluating delete gates after start Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com> --- pkg/controllers/updaterun/execution.go | 32 +++++++++--------- pkg/controllers/updaterun/execution_test.go | 37 +++++++++++++++++++++ 2 files changed, 54 insertions(+), 15 deletions(-) diff --git a/pkg/controllers/updaterun/execution.go b/pkg/controllers/updaterun/execution.go index cf89770af..4c768a9aa 100644 --- a/pkg/controllers/updaterun/execution.go +++ b/pkg/controllers/updaterun/execution.go @@ -349,24 +349,26 @@ func (r *Reconciler) executeDeleteStage( updateRunRef := klog.KObj(updateRun) updateRunStatus := updateRun.GetUpdateRunStatus() existingDeleteStageStatus := updateRunStatus.DeletionStageStatus - deleteStage := &placementv1beta1.StageConfig{ - Name: placementv1beta1.UpdateRunDeleteStageName, - AfterStageTasks: updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks, - } + if existingDeleteStageStatus.StartTime == nil { + deleteStage := &placementv1beta1.StageConfig{ + Name: placementv1beta1.UpdateRunDeleteStageName, + AfterStageTasks: updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks, + } - markUpdateRunWaiting(updateRun, fmt.Sprintf(condition.UpdateRunWaitingMessageFmt, "after-stage", existingDeleteStageStatus.StageName)) - markStageUpdatingWaiting(existingDeleteStageStatus, updateRun.GetGeneration(), "Waiting for delete stage tasks to complete") - approved, waitTime, err := r.checkAfterStageTasksStatus(ctx, deleteStage, existingDeleteStageStatus, updateRun) - if err != nil { - return false, 0, err - } - if !approved { - if waitTime < 0 { - waitTime = stageUpdatingWaitTime + markUpdateRunWaiting(updateRun, fmt.Sprintf(condition.UpdateRunWaitingMessageFmt, "after-stage", existingDeleteStageStatus.StageName)) + markStageUpdatingWaiting(existingDeleteStageStatus, updateRun.GetGeneration(), "Waiting for delete stage tasks to complete") + approved, waitTime, err := r.checkAfterStageTasksStatus(ctx, deleteStage, existingDeleteStageStatus, updateRun) + if err != nil { + return false, 0, err } - return false, waitTime, nil + if !approved { + if waitTime < 0 { + waitTime = stageUpdatingWaitTime + } + return false, waitTime, nil + } + markUpdateRunProgressing(updateRun) } - markUpdateRunProgressing(updateRun) existingDeleteStageClusterMap := make(map[string]*placementv1beta1.ClusterUpdatingStatus, len(existingDeleteStageStatus.Clusters)) for i := range existingDeleteStageStatus.Clusters { diff --git a/pkg/controllers/updaterun/execution_test.go b/pkg/controllers/updaterun/execution_test.go index e6ea4a563..c05ae86c0 100644 --- a/pkg/controllers/updaterun/execution_test.go +++ b/pkg/controllers/updaterun/execution_test.go @@ -1421,6 +1421,43 @@ func TestExecuteDeleteStageWithoutTasks(t *testing.T) { } } +func TestExecuteDeleteStageDoesNotReevaluateTasksAfterStart(t *testing.T) { + scheme := runtime.NewScheme() + if err := placementv1beta1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme() error = %v", err) + } + binding := &placementv1beta1.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "binding"}, + Spec: placementv1beta1.ResourceBindingSpec{TargetCluster: "cluster-1"}, + } + fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(binding).Build() + r := &Reconciler{Client: fakeClient} + startTime := metav1.Now() + updateRun := &placementv1beta1.ClusterStagedUpdateRun{ + ObjectMeta: metav1.ObjectMeta{Name: "test-update-run", Generation: 1}, + Status: placementv1beta1.UpdateRunStatus{ + UpdateStrategySnapshot: &placementv1beta1.UpdateStrategySpec{ + DeleteStageTasks: []placementv1beta1.StageTask{{ + Type: placementv1beta1.StageTaskTypeTimedWait, + WaitTime: &metav1.Duration{Duration: time.Hour}, + }}, + }, + DeletionStageStatus: &placementv1beta1.StageUpdatingStatus{ + StageName: placementv1beta1.UpdateRunDeleteStageName, + StartTime: &startTime, + Clusters: []placementv1beta1.ClusterUpdatingStatus{{ClusterName: "cluster-1"}}, + }, + }, + } + + if _, _, err := r.executeDeleteStage(context.Background(), updateRun, []placementv1beta1.BindingObj{binding}); err != nil { + t.Fatalf("executeDeleteStage() error = %v", err) + } + if err := fakeClient.Get(context.Background(), client.ObjectKeyFromObject(binding), &placementv1beta1.ClusterResourceBinding{}); !apierrors.IsNotFound(err) { + t.Fatalf("executeDeleteStage() binding get error = %v, want not found", err) + } +} + func TestGenerateStuckClustersString(t *testing.T) { tests := []struct { name string From 908d930208e13f6689c2ba172560fea8ab0d4ac1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:24:25 +0000 Subject: [PATCH 4/5] fix: stop safely while delete stage is gated Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com> --- apis/placement/v1beta1/stageupdate_types.go | 1 + ...etes-fleet.io_clusterstagedupdateruns.yaml | 4 ++++ ...leet.io_clusterstagedupdatestrategies.yaml | 4 ++++ ....kubernetes-fleet.io_stagedupdateruns.yaml | 4 ++++ ...netes-fleet.io_stagedupdatestrategies.yaml | 4 ++++ pkg/controllers/updaterun/stop.go | 2 +- pkg/controllers/updaterun/stop_test.go | 4 ++-- .../api_validation_integration_test.go | 19 +++++++++++++++++++ 8 files changed, 39 insertions(+), 3 deletions(-) diff --git a/apis/placement/v1beta1/stageupdate_types.go b/apis/placement/v1beta1/stageupdate_types.go index 53231214a..c1a0f72b0 100644 --- a/apis/placement/v1beta1/stageupdate_types.go +++ b/apis/placement/v1beta1/stageupdate_types.go @@ -274,6 +274,7 @@ type UpdateStrategySpec struct { // +kubebuilder:validation:XValidation:rule="!(self.size() == 2 && self[0].type == self[1].type)",message="deleteStageTasks cannot have two tasks of the same type" // +kubebuilder:validation:XValidation:rule="!self.exists(e, e.type == 'Approval' && has(e.waitTime))",message="DeleteStageTaskType is Approval, waitTime is not allowed" // +kubebuilder:validation:XValidation:rule="!self.exists(e, e.type == 'TimedWait' && !has(e.waitTime))",message="DeleteStageTaskType is TimedWait, waitTime is required" + // +kubebuilder:validation:XValidation:rule="!self.exists(e, e.type == 'TimedWait' && has(e.waitTime) && duration(e.waitTime) <= duration('0s'))",message="DeleteStageTaskType is TimedWait, waitTime must be greater than zero" DeleteStageTasks []StageTask `json:"deleteStageTasks,omitempty"` } diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml index 2bc41a455..c3f55d028 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml @@ -3275,6 +3275,10 @@ spec: rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' - message: DeleteStageTaskType is TimedWait, waitTime is required rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime must be + greater than zero + rule: '!self.exists(e, e.type == ''TimedWait'' && has(e.waitTime) + && duration(e.waitTime) <= duration(''0s''))' stages: description: Stage specifies the configuration for each update stage. diff --git a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml index d7003798e..8f1949ff3 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml @@ -407,6 +407,10 @@ spec: rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' - message: DeleteStageTaskType is TimedWait, waitTime is required rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime must be greater + than zero + rule: '!self.exists(e, e.type == ''TimedWait'' && has(e.waitTime) + && duration(e.waitTime) <= duration(''0s''))' stages: description: Stage specifies the configuration for each update stage. items: diff --git a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml index df0ab065d..69ab0ca44 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml @@ -2195,6 +2195,10 @@ spec: rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' - message: DeleteStageTaskType is TimedWait, waitTime is required rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime must be + greater than zero + rule: '!self.exists(e, e.type == ''TimedWait'' && has(e.waitTime) + && duration(e.waitTime) <= duration(''0s''))' stages: description: Stage specifies the configuration for each update stage. diff --git a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml index a3854c780..7d863df7c 100644 --- a/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml +++ b/config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml @@ -267,6 +267,10 @@ spec: rule: '!self.exists(e, e.type == ''Approval'' && has(e.waitTime))' - message: DeleteStageTaskType is TimedWait, waitTime is required rule: '!self.exists(e, e.type == ''TimedWait'' && !has(e.waitTime))' + - message: DeleteStageTaskType is TimedWait, waitTime must be greater + than zero + rule: '!self.exists(e, e.type == ''TimedWait'' && has(e.waitTime) + && duration(e.waitTime) <= duration(''0s''))' stages: description: Stage specifies the configuration for each update stage. items: diff --git a/pkg/controllers/updaterun/stop.go b/pkg/controllers/updaterun/stop.go index 17c878dc3..6cda8f618 100644 --- a/pkg/controllers/updaterun/stop.go +++ b/pkg/controllers/updaterun/stop.go @@ -204,7 +204,7 @@ func (r *Reconciler) stopDeleteStage( if allDeletingClustersDeleted { markStageUpdatingStopped(updateRunStatus.DeletionStageStatus, updateRun.GetGeneration()) } - return len(toBeDeletedBindings) == 0, nil + return allDeletingClustersDeleted, nil } // markUpdateRunStopping marks the update run as stopping in memory. diff --git a/pkg/controllers/updaterun/stop_test.go b/pkg/controllers/updaterun/stop_test.go index 499e1111d..f410ea533 100644 --- a/pkg/controllers/updaterun/stop_test.go +++ b/pkg/controllers/updaterun/stop_test.go @@ -613,7 +613,7 @@ func TestStopDeleteStage(t *testing.T) { }, }, { - name: "cluster not marked as deleting and binding not deleting", + name: "cluster waiting on delete stage tasks should stop without deleting its binding", updateRun: &placementv1beta1.ClusterStagedUpdateRun{ ObjectMeta: metav1.ObjectMeta{ Name: "test-updaterun", @@ -640,7 +640,7 @@ func TestStopDeleteStage(t *testing.T) { }, }, }, - wantFinished: false, + wantFinished: true, wantError: nil, wantProgressCond: metav1.Condition{ Type: string(placementv1beta1.StageUpdatingConditionProgressing), diff --git a/test/apis/placement/v1beta1/api_validation_integration_test.go b/test/apis/placement/v1beta1/api_validation_integration_test.go index 25c2a0029..7e1d73d61 100644 --- a/test/apis/placement/v1beta1/api_validation_integration_test.go +++ b/test/apis/placement/v1beta1/api_validation_integration_test.go @@ -2773,6 +2773,25 @@ var _ = Describe("Test placement v1beta1 API validation", func() { Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("DeleteStageTaskType is TimedWait, waitTime is required")) }) + It("Should deny creation of ClusterStagedUpdateStrategy with a non-positive DeleteStageTasks waitTime", func() { + strategy := placementv1beta1.ClusterStagedUpdateStrategy{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf(updateRunStrategyNameTemplate, GinkgoParallelProcess()), + }, + Spec: placementv1beta1.UpdateStrategySpec{ + Stages: []placementv1beta1.StageConfig{{Name: fmt.Sprintf(updateRunStageNameTemplate, GinkgoParallelProcess(), 1)}}, + DeleteStageTasks: []placementv1beta1.StageTask{{ + Type: placementv1beta1.StageTaskTypeTimedWait, + WaitTime: &metav1.Duration{}, + }}, + }, + } + err := hubClient.Create(ctx, &strategy) + var statusErr *k8sErrors.StatusError + Expect(errors.As(err, &statusErr)).To(BeTrue(), fmt.Sprintf("Create updateRunStrategy call produced error %s. Error type wanted is %s.", reflect.TypeOf(err), reflect.TypeOf(&k8sErrors.StatusError{}))) + Expect(statusErr.ErrStatus.Message).Should(MatchRegexp("DeleteStageTaskType is TimedWait, waitTime must be greater than zero")) + }) + It("Should deny creation of ClusterStagedUpdateStrategy with AfterStageTask of type Approval with waitTime specified", func() { strategy := placementv1beta1.ClusterStagedUpdateStrategy{ ObjectMeta: metav1.ObjectMeta{ From a3ed2226edae8ea831cb9164d8da20278a75f9d1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 13 Aug 2026 00:24:48 +0000 Subject: [PATCH 5/5] chore: complete delete stage task breadcrumb Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com> --- .../2026-08-12-2353-delete-stage-tasks.md | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md b/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md index eda6d3631..30aa639e3 100644 --- a/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md +++ b/.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md @@ -14,13 +14,20 @@ Allow staged update strategies to gate the implicit deletion stage with the exis ## Success Criteria -- [ ] Unset delete-stage tasks preserve immediate deletion. -- [ ] Approval and timed waits can independently gate deletion. -- [ ] Approval and timed waits run concurrently and both must pass. -- [ ] Cluster-scoped and namespaced runs retain bindings until their gate passes. -- [ ] Generated API and CRD artifacts are current. -- [ ] Targeted tests and repository quality checks pass. +- [x] Unset delete-stage tasks preserve immediate deletion. +- [x] Approval and timed waits can independently gate deletion. +- [x] Approval and timed waits run concurrently and both must pass. +- [x] Cluster-scoped and namespaced runs retain bindings until their gate passes. +- [x] Generated API and CRD artifacts are current. +- [x] Targeted tests and available repository quality checks pass. ## Approval Implementation was explicitly requested in the issue task. + +## Implementation Notes + +- Delete-stage approvals reuse the after-stage task machinery and labels. +- The delete stage uses a label-safe value for approval requests while retaining its canonical status/spec stage name. +- Gates run only before deletion starts; stopping while gated leaves bindings intact. +- Documentation updates are deferred to the separate documentation repository.