Add update-target-stage gate annotation#1913
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ciecierski The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
OpenStackControlPlane CRD Size Report
Threshold reference
|
stuggi
left a comment
stuggi
left a comment
There was a problem hiding this comment.
I think we might want to add webhook validation for the annotation values, like validating the annotation value in the ValidateUpdate webhook on OpenStackVersion . If someone sets target-stage=tyop, the webhook rejects the update immediately with a clear error, rather than silently ignoring it and not would run a full update.
The OpenStackVersion already has a webhook. Adding a check like:
if stage, ok := r.Annotations[MinorUpdateTargetStageAnnotation]; ok {
validStages := map[string]bool{...}
if !validStages[stage] {
return Forbidden("invalid target stage")
}
}
|
Build failed (check pipeline). Post ❌ openstack-k8s-operators-content-provider FAILURE in 7m 30s |
ciecierski
force-pushed
the
staged-update
branch
2 times, most recently
from
49cecea to
1ed02a9
Compare
ciecierski
changed the title
|
/test functional |
1 similar comment
|
/test functional |
|
What happens if...
Stage D's condition would be reset to |
We have two alternatives either let user set annotation for stage C and let user fix it with fixed oc patch. Or as you mentioned set webook to block user from making this kind of harmless mistake(setting annotations with stage C won't rollback updated containers in stage D) . |
|
/retest-required |
ciecierski
force-pushed
the
staged-update
branch
3 times, most recently
from
fae4854 to
6bda39d
Compare
Introduce a core.openstack.org/update-target-stage annotation on OpenStackVersion. When set, the update controller completes all stages up to and including the named stage, marks the next stage as blocked (FalseCondition/RequestedReason), and pauses reconciliation. Removing the annotation or advancing it to a later stage resumes the update. Includes stage-name constants, the gated-message format string, controller logic for all seven stages, functional tests for block/resume/ advance scenarios, webhooks and updated operator documentation. AI-assisted: Cursor (Claude Sonnet 4.6 by Anthropic)
| annotationField := "metadata.annotations[" + MinorUpdateTargetStageAnnotation + "]" | ||
| if stage == "" { | ||
| return apierrors.NewForbidden( | ||
| schema.GroupResource{ | ||
| Group: GroupVersion.WithKind("OpenStackVersion").Group, | ||
| Resource: GroupVersion.WithKind("OpenStackVersion").Kind, | ||
| }, resourceName, &field.Error{ | ||
| Type: field.ErrorTypeForbidden, | ||
| Field: annotationField, | ||
| BadValue: stage, | ||
| Detail: "Annotation value must not be empty. Remove the annotation or set a valid stage name", | ||
| }, | ||
| ) | ||
| } | ||
| if !IsValidMinorUpdateTargetStage(stage) { | ||
| return apierrors.NewForbidden( | ||
| schema.GroupResource{ | ||
| Group: GroupVersion.WithKind("OpenStackVersion").Group, | ||
| Resource: GroupVersion.WithKind("OpenStackVersion").Kind, | ||
| }, resourceName, &field.Error{ | ||
| Type: field.ErrorTypeForbidden, | ||
| Field: annotationField, | ||
| BadValue: stage, | ||
| Detail: fmt.Sprintf( | ||
| "Invalid target stage %q. Must be one of: %s", | ||
| stage, | ||
| strings.Join(ValidMinorUpdateTargetStages(), ", "), | ||
| ), | ||
| }, | ||
| ) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
This could perhaps be simplified to just use L208-224, changing the error message to "Invalid target stage '%q'. Must be one of: %s",. The single-quotes around the bad value, whether empty or just an invalid stage name, would highlight the problem for the user. Just a thought, and certainly not a blocking issue.
| return v.Spec.TargetVersion != *v.Status.DeployedVersion | ||
| } | ||
|
|
||
| func minorUpdateTargetStageFromAnnotations(annotations map[string]string) (string, bool) { |
There was a problem hiding this comment.
This is duplicate logic found in [1]. We should just use/call [1] instead and get rid of this function.
| @@ -59,7 +60,6 @@ func (r *OpenStackVersion) Default() { | |||
| // ValidateCreate validates the OpenStackVersion on creation | |||
| func (r *OpenStackVersion) ValidateCreate(ctx context.Context, c goClient.Client) (admission.Warnings, error) { | |||
There was a problem hiding this comment.
While we wouldn't be expecting a user to create an OpenStackVersion with the staged annotation, they could, in theory. Should we therefore handle ValidateCreate as well for this concern?
3 participants
Introduce the core.openstack.org/update-target-stage annotation on OpenStackVersion. When set, the minor update controller completes all stages up to and including the named stage, marks the next stage as blocked (FalseCondition/RequestedReason), and pauses reconciliation. Removing the annotation or advancing it to a later stage resumes the update. Includes stage-name constants, the gated-message format string, controller logic for all seven stages, webhook validation and functional tests for block/resume/ advance scenarios, and updated operator documentation.
AI-assisted: Cursor (Claude Sonnet 4.6 by Anthropic)