OCPCLOUD-2998: Add synchronizedAPI to Machine and MachineSet status #2642
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@RadekManak: This pull request references OCPCLOUD-2998 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @RadekManak! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughAdded a new exported string type 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)machine/v1beta1/types_machineset.go (1)
🔇 Additional comments (4)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
openshift-ci
bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @machine/v1beta1/types_machineset.go:
- Around line 171-179: Add the same immutability XValidation annotations used in
the Machine CRD to both MachineSetStatus.authoritativeAPI and
MachineSetStatus.synchronizedAPI so they cannot be cleared once set: apply
+kubebuilder:validation:XValidation:rule="oldSelf == '' || self !=
''",message="The <field> field must not be set empty once it has a value."
(replace <field> with authoritativeAPI and synchronizedAPI respectively) on the
corresponding fields (AuthoritativeAPI and SynchronizedAPI) to enforce the "once
set cannot be cleared" behavior.
- Around line 171-179: MachineSetStatus's synchronizedAPI field lacks the
XValidation immutability rule present on MachineStatus; add the same kubebuilder
XValidation annotation to the SynchronizedAPI field in MachineSetStatus so it
prevents clearing the field once set (use the rule: oldSelf == '' || self != ''
with the message "The synchronizedAPI field must not be set empty once it has a
value."), placing the annotation immediately above the SynchronizedAPI field
declaration so it applies to that field.
In @openapi/generated_openapi/zz_generated.openapi.go:
- Around line 42610-42616: The generated OpenAPI for the status.synchronizedAPI
string is missing the Enum entries even though the source types
(types_machineset.go / types_machine.go) include the kubebuilder enum marker;
confirm the exact marker `// +kubebuilder:validation:Enum=MachineAPI;ClusterAPI`
is placed on the field/type, upgrade/controller-gen to a version that supports
enum translation if needed, then re-run the openapi generation (controller-gen
or the repo's make generate) so the SchemaProps for synchronizedAPI (and the
other occurrence) include Enum: []interface{}{"MachineAPI","ClusterAPI"}; if
controller-gen is already current but still failing, check generator
config/hooks for filtering of validation tags and adjust so kubebuilder
validation markers are propagated into zz_generated.openapi.go.
- Around line 42802-42808: The OpenAPI schema for the synchronizedAPI property
is missing the Enum constraint; update the SchemaProps for "synchronizedAPI" in
zz_generated.openapi.go to include Enum: []interface{}{"MachineAPI",
"ClusterAPI"} (matching the +kubebuilder:validation:Enum marker) so the
generated schema enforces those values; ideally regenerate the OpenAPI with the
generator that respects kubebuilder tags, or directly add the Enum entry next to
Type/Format in the synchronizedAPI SchemaProps to match other enum fields.
🧹 Nitpick comments (2)
machine/v1beta1/types_machine.go (2)
415-415: LGTM! Validation logic correctly prevents clearing the field.The immutable-non-empty validation rule is implemented correctly and aligns with the PR requirements. The logic properly allows initial setting while preventing the field from being cleared once it has a value.
Minor style suggestion for consistency
For consistency with the validation rule on line 414, consider adding a space after the
||operator:- // +kubebuilder:validation:XValidation:rule="oldSelf == ''|| self != ''",message="The authoritativeAPI field must not be set empty once it has a value." + // +kubebuilder:validation:XValidation:rule="oldSelf == '' || self != ''",message="The authoritativeAPI field must not be set empty once it has a value."
420-429: LGTM! Well-documented field with correct validation constraints.The
synchronizedAPIfield is properly implemented with:
- Clear, comprehensive documentation explaining its purpose and lifecycle
- Correct enum validation allowing only stable states (MachineAPI, ClusterAPI) - appropriately excluding "Migrating"
- Immutable-non-empty validation preventing the field from being cleared once set
- Proper type safety using the custom
SynchronizedAPItype- Appropriate feature gating
The implementation correctly addresses all requirements from the PR description for migration cancellation support.
Minor style suggestion for consistency
For consistency with the validation rule on line 414, consider adding a space after the
||operator:- // +kubebuilder:validation:XValidation:rule="oldSelf == ''|| self != ''",message="The synchronizedAPI field must not be set empty once it has a value." + // +kubebuilder:validation:XValidation:rule="oldSelf == '' || self != ''",message="The synchronizedAPI field must not be set empty once it has a value."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (8)
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.featuregated-crd-manifests/machines.machine.openshift.io/MachineAPIMigration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machine/v1beta1/zz_generated.featuregated-crd-manifests/machinesets.machine.openshift.io/MachineAPIMigration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (4)
machine/v1beta1/types_machine.gomachine/v1beta1/types_machineset.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.go
🧰 Additional context used
🧬 Code graph analysis (1)
machine/v1beta1/types_machineset.go (1)
machine/v1beta1/types_machine.go (1)
SynchronizedAPI(188-188)
🔇 Additional comments (3)
machine/v1beta1/zz_generated.swagger_doc_generated.go (1)
629-629: LGTM: Generated documentation is consistent.The Swagger documentation entries for
synchronizedAPIin bothMachineStatus(line 629) andMachineSetStatus(line 733) are consistent and accurately reflect the field semantics described in the type definitions.Note: This is an auto-generated file (as indicated by line 11), so these entries will be regenerated if the source comments change.
Also applies to: 733-733
machine/v1beta1/types_machineset.go (1)
179-179: SynchronizedAPI type definition is correct.The type is properly defined in
machine/v1beta1/types_machine.gowith both required constants (MachineAPISynchronizedandClusterAPISynchronized) correctly typed and valued, matching the enum validation (Enum=MachineAPI;ClusterAPI) specified on the field.machine/v1beta1/types_machine.go (1)
188-196: LGTM! Clean type definition with clear constants.The new
SynchronizedAPItype and constants are well-structured and follow the existing pattern ofMachineAuthority. The string values correctly match the enum validation used in the field definition, and the documentation clearly explains each constant's purpose.
|
/assign @JoelSpeed |
machine/v1beta1/types_machine.go
Outdated
| // The migration controller sets `status.synchronizedAPI` to the value of the `status.authoritativeAPI` before it transitions to "Migrating". | ||
| // It is used to determine the source API of the migration. | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == ''|| self != ''",message="The synchronizedAPI field must not be set empty once it has a value." | ||
| // +kubebuilder:validation:Enum=MachineAPI;ClusterAPI |
There was a problem hiding this comment.
Probably better to add this on the type itself
machine/v1beta1/types_machine.go
Outdated
| // When set to Migrating, the migration controller is currently performing the handover of authority from one API to the other. | ||
| // +kubebuilder:validation:Enum=MachineAPI;ClusterAPI;Migrating | ||
| // +kubebuilder:validation:XValidation:rule="self == 'Migrating' || self == oldSelf || oldSelf == 'Migrating'",message="The authoritativeAPI field must not transition directly from MachineAPI to ClusterAPI or vice versa. It must transition through Migrating." | ||
| // +kubebuilder:validation:XValidation:rule="oldSelf == ''|| self != ''",message="The authoritativeAPI field must not be set empty once it has a value." |
There was a problem hiding this comment.
What's the motivation on this one? Empty isn't actually a valid value so it would be rejected anyway?
Were you trying to make sure it cannot be removed once set? If so, you need a CEL rule on the parent struct, not on the field itself
There was a problem hiding this comment.
I wanted to make sure a hypothetical error in the controller does not remove the value by accident. Removed this as it is not required.
There was a problem hiding this comment.
Having a rule to prevent it from being removed makes sense, it was just in the wrong place, lets add one to the parent struct that prevents this child from being removed in an update
machine/v1beta1/types_machine.go
Outdated
|
|
||
| // synchronizedAPI represents the API that is currently in sync with the state of the resource. | ||
| // Valid values are "MachineAPI" and "ClusterAPI". | ||
| // When omitted (empty value), the resource has not yet been reconciled by the migration controller. |
There was a problem hiding this comment.
Omitted doesn't actually mean an empty value, it means the key isn't present
machine/v1beta1/types_machine.go
Outdated
| // Valid values are "MachineAPI" and "ClusterAPI". | ||
| // When omitted (empty value), the resource has not yet been reconciled by the migration controller. | ||
| // The migration controller sets `status.synchronizedAPI` to the value of the `status.authoritativeAPI` before it transitions to "Migrating". | ||
| // It is used to determine the source API of the migration. |
There was a problem hiding this comment.
Source of the last successful synchronization wasn't it?
RadekManak
force-pushed
the
synchronizedAPI
branch
from
8d088e5 to
4019d49
Compare
openshift-merge-robot
added
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
machine/v1beta1/types_machine.go (1)
330-435: Missing validation to enforce "cannot be cleared once set" forstatus.synchronizedAPI.Both
MachineStatusandMachineSetStatusare missing validation to preventsynchronizedAPIfrom being cleared after it is initially set. The field is marked+optional, allowing it to be dropped on full-object updates. The PR objectives require this field to remain non-clearable once set.While
synchronizedGenerationhas appropriate FeatureGateAwareXValidation in place, an equivalent rule is needed forsynchronizedAPI:Proposed fix (FeatureGate-aware XValidation)
// MachineStatus defines the observed state of Machine // +openshift:validation:FeatureGateAwareXValidation:featureGate=MachineAPIMigration,rule="!has(oldSelf.synchronizedGeneration) || (has(self.synchronizedGeneration) && self.synchronizedGeneration >= oldSelf.synchronizedGeneration) || (oldSelf.authoritativeAPI == 'Migrating' && self.authoritativeAPI != 'Migrating')",message="synchronizedGeneration must not decrease unless authoritativeAPI is transitioning from Migrating to another value" +// +openshift:validation:FeatureGateAwareXValidation:featureGate=MachineAPIMigration,rule="!has(oldSelf.synchronizedAPI) || has(self.synchronizedAPI)",message="synchronizedAPI cannot be cleared once set" type MachineStatus struct {Apply the same fix to
MachineSetStatusintypes_machineset.go.
🧹 Nitpick comments (1)
machine/v1beta1/types_machine.go (1)
188-198: NewSynchronizedAPItype/enum is fine; consider constant naming alignment (optional).
The Enum + two constants are clear. Optionally, consider names that mirrorMachineAuthority*style (e.g.,SynchronizedAPIMachineAPI,SynchronizedAPIClusterAPI) for scan-ability.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (8)
machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machines-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.crd-manifests/0000_10_machine-api_01_machinesets-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machine/v1beta1/zz_generated.featuregated-crd-manifests/machines.machine.openshift.io/MachineAPIMigration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machine/v1beta1/zz_generated.featuregated-crd-manifests/machinesets.machine.openshift.io/MachineAPIMigration.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (4)
machine/v1beta1/types_machine.gomachine/v1beta1/types_machineset.gomachine/v1beta1/zz_generated.swagger_doc_generated.goopenapi/generated_openapi/zz_generated.openapi.go
🚧 Files skipped from review as they are similar to previous changes (1)
- machine/v1beta1/types_machineset.go
🔇 Additional comments (2)
machine/v1beta1/zz_generated.swagger_doc_generated.go (2)
617-631:MachineStatus.synchronizedAPIswagger doc looks consistent with the new field semantics.
Doc describes “last stable authoritativeAPI”, cancellation/restore intent, allowed values, and “omitted” behavior.
723-735:MachineSetStatus.synchronizedAPIswagger doc looks consistent with the new field semantics.
Same semantics as MachineStatus; wording and allowed values match the intended contract.
RadekManak
force-pushed
the
synchronizedAPI
branch
from
4019d49 to
fa48e18
Compare
openshift-merge-robot
removed
the
needs-rebase
|
Added the suggested changes and rewrote the description to be simpler and without implementation details. |
|
/lgtm This is fine as is, we may still want the removal protection thing we discussed but can follow up for that one |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@RadekManak: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
4 participants
Summary
This PR adds a new
synchronizedAPIstatus field to Machine and MachineSet resources to support migration cancellation in the Machine API Migration feature.New Field
status.synchronizedAPI: Tracks the API that was last successfully synchronized beforestatus.authoritativeAPItransitioned toMigrating. Valid values areMachineAPIandClusterAPI.Purpose
Currently, we determine the direction of migration by checking the value of
spec.authoritativeAPI. This works for the base case, but it doesn't work when a migration becomes stuck instatus.authoritativeAPI: Migratingand the user, to unblock the migration, changesspec.authoritativeAPIback to the original value.At that point, we need to reliably determine which direction we were trying to migrate when we got stuck. The
synchronizedAPIfield captures this information, allowing us to:spec.authoritativeAPImatchesstatus.synchronizedAPIwhile inMigratingstate)Without this field, once the user changes
spec.authoritativeAPIback, we lose the information about the original migration direction and cannot safely restore the system to its previous state.Behavior
status.synchronizedAPIto the currentstatus.authoritativeAPIvalue before transitioning toMigratingsynchronizedAPIremains unchanged, preserving the source APIsynchronizedAPIis updated to reflect the new authoritative APIspec.authoritativeAPIback to matchstatus.synchronizedAPIwhile inMigratingstate, the migration controller detects this as a cancellation request, unpauses the appropriate resources, and returns to the previous stateValidation
MachineAPI,ClusterAPIImplementation
The controller implementation is available at: openshift/cluster-capi-operator#376