feat: support tasks on the staged update delete stage - #813
feat: support tasks on the staged update delete stage#813Yetkin Timocin (ytimocin) with Copilot wants to merge 5 commits into
Conversation
Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com>
Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com>
Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com>
Co-authored-by: ytimocin <5220939+ytimocin@users.noreply.github.com>
|
|
||
| // 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. |
There was a problem hiding this comment.
nit: extra line before this. follow the file structure.
|
|
||
| // 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. |
There was a problem hiding this comment.
nit: extra line before this. follow the file structure.
| // 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" |
There was a problem hiding this comment.
What is the difference between AfterStageApprovalTaskNameFmt and AfterStageApprovalTaskNameFmt? Why can't we have one generic? Names don't make that much sense.
AfterStageApprovalTaskNameFmt sounds generic like after any stage and that makes me think that Delete is also a stage, right?
| // +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"` |
There was a problem hiding this comment.
Same comment as above. Why can't we have something generic and not specific to the Delete stage?
| } | ||
| } | ||
| } | ||
| for i := range updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks { |
There was a problem hiding this comment.
Same question as above about being generic or not? Could we use an interface pattern for this?
There was a problem hiding this comment.
Just a suggestion. Doesn't mean that we should use.
There was a problem hiding this comment.
Pull request overview
Adds approval and timed-wait gates before staged-update deletion, supporting controlled workload decommissioning.
Changes:
- Adds validated
deleteStageTasksAPI fields and generated artifacts. - Gates cluster-scoped and namespaced binding deletion on configured tasks.
- Allows gated deletion stages to stop without deleting bindings.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
test/apis/placement/v1beta1/api_validation_integration_test.go |
Tests delete-task validation. |
pkg/controllers/updaterun/stop.go |
Corrects gated-stage stopping. |
pkg/controllers/updaterun/stop_test.go |
Tests stopping before deletion. |
pkg/controllers/updaterun/initialization.go |
Initializes delete-task status. |
pkg/controllers/updaterun/initialization_integration_test.go |
Verifies initialization. |
pkg/controllers/updaterun/execution.go |
Evaluates gates before deletion. |
pkg/controllers/updaterun/execution_test.go |
Tests task evaluation and deletion behavior. |
pkg/controllers/updaterun/execution_integration_test.go |
Tests approval gates for both scopes. |
pkg/controllers/updaterun/controller.go |
Sanitizes approval task snapshots. |
pkg/controllers/updaterun/controller_test.go |
Tests snapshot sanitization. |
config/crd/bases/placement.kubernetes-fleet.io_stagedupdatestrategies.yaml |
Adds namespaced strategy schema. |
config/crd/bases/placement.kubernetes-fleet.io_stagedupdateruns.yaml |
Adds namespaced snapshot schema. |
config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdatestrategies.yaml |
Adds cluster strategy schema. |
config/crd/bases/placement.kubernetes-fleet.io_clusterstagedupdateruns.yaml |
Adds cluster snapshot schema. |
apis/placement/v1beta1/zz_generated.deepcopy.go |
Deep-copies delete tasks. |
apis/placement/v1beta1/stageupdate_types.go |
Defines the new API field. |
apis/placement/v1beta1/commons.go |
Adds delete-stage constants. |
.github/.copilot/breadcrumbs/2026-08-12-2353-delete-stage-tasks.md |
Records implementation decisions. |
Files not reviewed (1)
- apis/placement/v1beta1/zz_generated.deepcopy.go: Generated file
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| updateRunRef := klog.KObj(updateRun) | ||
| updateRunStatus := updateRun.GetUpdateRunStatus() | ||
| existingDeleteStageStatus := updateRunStatus.DeletionStageStatus | ||
| if existingDeleteStageStatus.StartTime == nil { |
The implicit delete stage could remove workloads immediately, bypassing the Approval and TimedWait gates available to update stages. This prevented controlled decommissioning of clusters leaving a placement.
API
deleteStageTasksto both scoped update strategies.ApprovalandTimedWaittask types.Execution
Lifecycle