-
Notifications
You must be signed in to change notification settings - Fork 35
feat: support tasks on the staged update delete stage #813
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
212698b
1d37f23
00d38c1
908d930
a3ed222
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| # 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 | ||
|
|
||
| - [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. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: extra line before this. follow the file structure. |
||
| DeleteStageApprovalTaskNameFmt = "%s-after-delete-stage" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
| ) | ||
|
|
||
| var ( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -266,6 +266,16 @@ 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" | ||
| // +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"` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as above. Why can't we have something generic and not specific to the Delete stage? |
||
| } | ||
|
|
||
| // ClusterStagedUpdateStrategyList contains a list of StagedUpdateStrategy. | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question as above about being generic or not? Could we use an interface pattern for this?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a suggestion. Doesn't mean that we should use. |
||
| if updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks[i].Type == placementv1beta1.StageTaskTypeApproval { | ||
| updateRunStatus.UpdateStrategySnapshot.DeleteStageTasks[i].WaitTime = nil | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extra line before this. follow the file structure.