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: |
📝 WalkthroughWalkthroughThis change introduces a new 🚥 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
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
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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.
| // 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. | ||
| // 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:Enum=MachineAPI;ClusterAPI | ||
| // +openshift:enable:FeatureGate=MachineAPIMigration | ||
| // +optional | ||
| SynchronizedAPI SynchronizedAPI `json:"synchronizedAPI,omitempty"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full MachineSetStatus struct around the synchronizedAPI field
fd -e "types_machineset.go" --type f | head -1 | xargs wc -lRepository: openshift/api
Length of output: 58
🏁 Script executed:
# Get context around the synchronizedAPI field and check for validation rules
cat -n machine/v1beta1/types_machineset.go | sed -n '110,190p'Repository: openshift/api
Length of output: 5548
🏁 Script executed:
# Search for all XValidation rules in the MachineSetStatus area
rg -nP 'XValidation|Validation' machine/v1beta1/types_machineset.go -B 2 -A 2Repository: openshift/api
Length of output: 1219
🏁 Script executed:
# Check the Machine CRD (types_machine.go) for synchronizedAPI immutability patterns
fd -e "types_machine.go" --type f | head -1 | xargs wc -lRepository: openshift/api
Length of output: 58
🏁 Script executed:
# Search for synchronizedAPI in Machine CRD with validation context
rg -nP 'synchronizedAPI' machine/v1beta1/types_machine.go -B 5 -A 5Repository: openshift/api
Length of output: 1597
Add immutability validation to prevent clearing synchronizedAPI (and authoritativeAPI) once set.
The synchronizedAPI field in MachineSetStatus is missing an immutability validation rule that already exists in the corresponding MachineStatus field in the Machine CRD. The Machine CRD enforces this constraint with:
+kubebuilder:validation:XValidation:rule="oldSelf == ''|| self != ''",message="The synchronizedAPI field must not be set empty once it has a value."
The same is true for the authoritativeAPI field in MachineSetStatus (line 169), which also lacks this immutability protection despite its presence in the Machine CRD. Both fields should have the same immutability constraint applied to align with the PR objective: "once set the field cannot be cleared."
🤖 Prompt for AI Agents
In @machine/v1beta1/types_machineset.go around lines 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's find the files containing MachineStatus and MachineSetStatus
echo "=== Finding type definition files ==="
fd -e go -x grep -l "type MachineStatus struct\|type MachineSetStatus struct" {} \;
# Let's look for the synchronizedAPI field in both types
echo -e "\n=== Searching for synchronizedAPI field ==="
rg -n "SynchronizedAPI" --type=go -B 5 -A 1
# Check specifically in the types files
echo -e "\n=== Checking MachineSet status file ==="
cat -n machine/v1beta1/types_machineset.go | sed -n '165,185p'
# Find and check the Machine status file
echo -e "\n=== Finding Machine status file ==="
fd -e go "types_machine.go" -x cat -n {} \; | head -100Repository: openshift/api
Length of output: 9581
Add XValidation rule to match MachineStatus field validation.
The synchronizedAPI field in MachineSetStatus is missing the immutability validation rule present in MachineStatus. MachineStatus includes:
+kubebuilder:validation:XValidation:rule="oldSelf == ''|| self != ''",message="The synchronizedAPI field must not be set empty once it has a value."
This rule prevents the field from being unset once it has been assigned a value. Add this XValidation rule to MachineSetStatus to maintain consistency and enforce the same state transition constraints across both types.
🤖 Prompt for AI Agents
In @machine/v1beta1/types_machineset.go around lines 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.
| "synchronizedAPI": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "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. 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.", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the directory structure and look for machine-related type files
find . -type f -name "*.go" | grep -E "(types_machine|types_machineset)" | head -20Repository: openshift/api
Length of output: 170
🏁 Script executed:
#!/bin/bash
# Search for SynchronizedAPI type definition and any enum markers
rg -i "synchronizedapi" --type go -B 2 -A 5Repository: openshift/api
Length of output: 10448
🏁 Script executed:
#!/bin/bash
# Search for kubebuilder validation enum markers related to synchronizedAPI or any migration-related fields
rg "kubebuilder:validation:Enum" --type go | head -30Repository: openshift/api
Length of output: 2565
🏁 Script executed:
#!/bin/bash
# Check if the generated OpenAPI file actually has Enum constraint anywhere
grep -A 5 -B 5 "synchronizedAPI" openapi/generated_openapi/zz_generated.openapi.go | head -40Repository: openshift/api
Length of output: 2435
The generated OpenAPI schema is missing the Enum constraint despite proper validation markers in the source.
The source types in machine/v1beta1/types_machineset.go and machine/v1beta1/types_machine.go correctly include // +kubebuilder:validation:Enum=MachineAPI;ClusterAPI, but the generated OpenAPI schema at lines 42610-42616 and 42802-42808 does not include an Enum field in the SchemaProps. The generated schema should include Enum: []interface{}{"MachineAPI", "ClusterAPI"} to enforce valid values at the OpenAPI documentation and client-side validation level.
Verify that the OpenAPI generation process properly translates kubebuilder validation enum markers into the generated schema's Enum field.
🤖 Prompt for AI Agents
In @openapi/generated_openapi/zz_generated.openapi.go around lines 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.
| "synchronizedAPI": { | ||
| SchemaProps: spec.SchemaProps{ | ||
| Description: "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. 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.", | ||
| Type: []string{"string"}, | ||
| Format: "", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.go" | grep -E "(types|machine)" | head -20Repository: openshift/api
Length of output: 720
🏁 Script executed:
# Look for the machine types file and enum validation markers
fd -type f -name "*.go" | xargs rg "synchronizedAPI" -B 5 -A 5 | head -100Repository: openshift/api
Length of output: 14076
🏁 Script executed:
# Search for kubebuilder validation markers related to synchronizedAPI
rg -i "synchronizedapi" -B 3 -A 3 --type goRepository: openshift/api
Length of output: 10969
🏁 Script executed:
# Check if Enum field exists in the OpenAPI schema for synchronizedAPI
rg -A 10 '"synchronizedAPI":' openapi/generated_openapi/zz_generated.openapi.go | grep -i "enum"Repository: openshift/api
Length of output: 39
🏁 Script executed:
# Check the authoritativeAPI field in the same OpenAPI file to see if it has an Enum constraint for comparison
rg -A 10 '"authoritativeAPI":' openapi/generated_openapi/zz_generated.openapi.go | head -30Repository: openshift/api
Length of output: 3402
🏁 Script executed:
# Let's check if there are ANY enum fields in the OpenAPI file to understand the pattern
rg -i "Enum:" openapi/generated_openapi/zz_generated.openapi.go | head -20Repository: openshift/api
Length of output: 1265
🏁 Script executed:
# Let's see the complete structure of synchronizedAPI in the MachineStatus to understand what's there
rg -A 15 'MachineStatus.*struct' machine/v1beta1/types_machine.go | grep -A 15 "synchronizedAPI" || rg -B 2 -A 12 '"synchronizedAPI":' openapi/generated_openapi/zz_generated.openapi.go | head -40Repository: openshift/api
Length of output: 2187
🏁 Script executed:
# Get exact line numbers for synchronizedAPI in the OpenAPI file
rg -n '"synchronizedAPI":' openapi/generated_openapi/zz_generated.openapi.goRepository: openshift/api
Length of output: 120
Add Enum constraint to OpenAPI schema for synchronizedAPI field.
The synchronizedAPI field in the OpenAPI schema is missing the Enum constraint despite having the proper +kubebuilder:validation:Enum=MachineAPI;ClusterAPI marker in the source type definition. The description documents valid values ("MachineAPI" and "ClusterAPI"), but the generated schema cannot enforce them. Other enum fields in this OpenAPI file use the Enum: []interface{}{...} structure, so the generation process should apply this pattern to synchronizedAPI as well.
🤖 Prompt for AI Agents
In @openapi/generated_openapi/zz_generated.openapi.go around lines 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.
|
/assign @JoelSpeed |
|
@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. |
| // 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
| // 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
|
|
||
| // 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
| // 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?
3 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