OCPEDGE-2408, OCPEDGE-2410: Add AdaptableTopology feature gate and enum to Infrastructure API#2820
OCPEDGE-2408, OCPEDGE-2410: Add AdaptableTopology feature gate and enum to Infrastructure API#2820jeff-roche wants to merge 1 commit intoopenshift:masterfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@jeff-roche: This pull request references OCPEDGE-2408 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 "5.0.0" version, but no target version was set. This pull request references OCPEDGE-2410 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 "5.0.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. |
openshift-ci-robot
added
the
jira/valid-reference
|
Hello @jeff-roche! Some important instructions when contributing to openshift/api: |
📝 WalkthroughWalkthroughThis pull request introduces a new 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.11.4)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
Bot
added
the
size/XXL
|
[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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml (1)
1055-1116:⚠️ Potential issue | 🟡 MinorRemove "Adaptable" mode descriptions from OKD, Default, and TechPreviewNoUpgrade variants' enums mismatch.
The OKD, Default, and TechPreviewNoUpgrade CRD variants describe both
controlPlaneTopologyandinfrastructureTopologyas supporting an'Adaptable'mode, but do not includeAdaptablein the actual enum values. This creates confusing documentation for users—they will read about a feature in the description that they cannot actually set in those variants.The CustomNoUpgrade and DevPreviewNoUpgrade variants correctly include both the description and enum entry for
Adaptable. Consider aligning the OKD/Default/TechPreviewNoUpgrade variants to remove the'Adaptable'mode description blurbs to match their enums.Minor issue — does not affect validation, only user-facing docs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml` around lines 1055 - 1116, The descriptions for controlPlaneTopology and infrastructureTopology in the OKD/Default/TechPreviewNoUpgrade CRD variants mention an 'Adaptable' mode that is not present in their enum lists; remove the sentences referencing the 'Adaptable' behavior from the description blocks for controlPlaneTopology and infrastructureTopology in those variants so the prose matches the enum, leaving the enum entries unchanged; note that CustomNoUpgrade and DevPreviewNoUpgrade already include both the description and enum for Adaptable, so do not modify those variants.payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml (1)
1132-1139:⚠️ Potential issue | 🟡 MinorKeep TechPreview docs aligned with the rejected enum values.
This TechPreviewNoUpgrade CRD documents
Adaptable, but both enums still reject it. Since the PR objective says TechPreview must not includeAdaptable, these generated descriptions should not advertise it here; otherwise users see an option the API will reject.📝 Proposed generated-output fix
- The 'Adaptable' mode indicates the cluster dynamically adjusts control-plane behavior based on - current node count. enum: - HighlyAvailable - HighlyAvailableArbiter - SingleReplica - DualReplica - External @@ - The 'Adaptable' mode indicates the cluster dynamically adjusts infrastructure behavior based on - current node count. NOTE: External topology mode is not applicable for this field. enum: - HighlyAvailable - SingleReplicaPlease apply this through the source markers/codegen path so regeneration does not reintroduce the mismatch.
Also applies to: 1176-1182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml` around lines 1132 - 1139, The CRD description for TechPreviewNoUpgrade incorrectly mentions "Adaptable" even though the enum list (values: HighlyAvailable, HighlyAvailableArbiter, SingleReplica, DualReplica, External) rejects it; update the codegen/source marker that generates the description (the TechPreviewNoUpgrade CRD docstring generator) to remove any reference to "Adaptable" so the generated text matches the enum, and apply the same change for the second occurrence corresponding to the block around lines 1176-1182; regenerate the manifest to ensure the description and the enum values are consistent.payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml (1)
1346-1407:⚠️ Potential issue | 🟡 MinorDescription mentions
Adaptablebut the OKD enum does not accept it.The description text added at lines 1357-1358 (controlPlaneTopology) and 1401-1402 (infrastructureTopology) tells users that an
Adaptablemode exists, but theenumlists for this OKD variant (lines 1360-1364 and 1405-1406) do not includeAdaptable. That's consistent withAdaptableTopologybeing aDevPreviewNoUpgradegate that OKD does not enable — but it means a user reading this CRD will seeAdaptabledocumented and then get an enum-validation rejection when they try to set it.Since descriptions on the embedded
infrastatus field come straight from the Go doc comment inconfig/v1/types_infrastructure.goand are not feature-gate-filtered, the mismatch will also appear in theDefaultandTechPreviewNoUpgradevariants. Consider either:
- scoping the
Adaptablesentences in the Go doc comment to note they only apply when theAdaptableTopologyfeature gate is enabled, or- accepting this as a known quirk of non-gated description text and confirming docs consumers understand the enum is authoritative.
Worth confirming this is intentional for the OKD payload before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml` around lines 1346 - 1407, The description mentions an "Adaptable" topology mode but the OKD CRD enums for controlPlaneTopology and infrastructureTopology (symbols: controlPlaneTopology, infrastructureTopology, and the documented Adaptable mode) don't include it, causing a mismatch; fix by either updating the source Go doc comment that feeds these CRD descriptions (e.g., the comment near the types that define controlPlaneTopology/infrastructureTopology) to explicitly state that "Adaptable" is only valid when the AdaptableTopology feature gate is enabled, or adjust the OKD CRD description to remove or qualify the Adaptable sentences so they clearly indicate they are conditional on the AdaptableTopology gate (ensure the change targets the doc comment that generates the CRD description or the OKD-specific CRD templating path so the enum remains authoritative).payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml (1)
1425-1432:⚠️ Potential issue | 🟡 MinorDon’t document
Adaptablein the TechPreview CRD unless the enum allows it.This
TechPreviewNoUpgrademanifest still rejectsAdaptablebecause both enums omit it, but the descriptions now advertise it. That makes generated OpenAPI/kubectl explaindocs misleading for a value that should be DevPreview-only per the PR objective.Please remove these lines from non-DevPreview generated CRDs, or gate the generated description alongside the enum overlay.
Proposed generated-manifest shape
The 'HighlyAvailableArbiter' mode indicates that the control plane will consist of 2 control-plane nodes that run conventional services and 1 smaller sized arbiter node that runs a bare minimum of services to maintain quorum. - The 'Adaptable' mode indicates the cluster dynamically adjusts control-plane behavior based on - current node count. enum: - HighlyAvailable - HighlyAvailableArbiter @@ The 'SingleReplica' mode will be used in single-node deployments and the operators should not configure the operand for highly-available operation - The 'Adaptable' mode indicates the cluster dynamically adjusts infrastructure behavior based on - current node count. NOTE: External topology mode is not applicable for this field. enum: - HighlyAvailableAlso applies to: 1469-1475
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml` around lines 1425 - 1432, The TechPreviewNoUpgrade CRD still mentions the 'Adaptable' mode in its description while the enum arrays in this manifest (the enum values under the control-plane replicaMode field in the TechPreviewNoUpgrade CRD) do not include 'Adaptable'; update the manifest so docs and schema match by either removing the descriptive lines that reference 'Adaptable' from the TechPreviewNoUpgrade CRD or gating that description behind the DevPreview overlay, or alternatively add 'Adaptable' to the enum only in the DevPreview/Dev-only CRD; make the change in the description block and the enum values near the replicaMode field (the enum entries "- HighlyAvailable" ... "- External") and apply the same fix to the other occurrence noted (around the 1469-1475 area).
🧹 Nitpick comments (1)
config/v1/tests/infrastructures.config.openshift.io/AdaptableTopology.yaml (1)
7-16: Consider adding negative test coverage forAdaptable.The
onCreatecase only asserts that an empty spec is accepted, which doesn't exercise anything specific to theAdaptableTopologygate — it would pass regardless of this PR. Similarly, theonUpdatecases only cover the happy path. Consider adding:
- An
onUpdatecase asserting thatstatus.controlPlaneTopology: Adaptableis rejected without the feature gate (covered implicitly by other test files, but worth confirming).- An
onUpdatecase asserting that an invalid topology value (e.g.,Bogus) is rejected under this gate, to guard against the enum being accidentally widened.Not blocking — existing coverage via
DualReplica.yaml/HighlyAvailableArbiter.yamlpatterns likely exists; just flagging for parity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/v1/tests/infrastructures.config.openshift.io/AdaptableTopology.yaml` around lines 7 - 16, Add negative onUpdate test cases to AdaptableTopology.yaml to exercise the AdaptableTopology gate: add one onUpdate case that sets status.controlPlaneTopology: Adaptable and expects rejection when the feature gate is not enabled (name it e.g. "Reject Adaptable without feature gate"), and add another onUpdate case that sets status.controlPlaneTopology: Bogus (or other invalid enum) and expects rejection even when the AdaptableTopology gate is enabled (name it e.g. "Reject invalid topology value"), ensuring the test bodies reference status.controlPlaneTopology and the gate toggle used elsewhere so they fail when validation should block the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_infrastructure.go`:
- Around line 110-113: The FeatureGateAwareEnum annotation with
featureGate=AdaptableTopology incorrectly includes "DualReplica"; update the
annotation on the types_infrastructure.go struct (the line starting with
+openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=...)
to remove "DualReplica" so that the AdaptableTopology-only overlay only adds
"Adaptable" (i.e., ensure the enum list mirrors the second field's pattern and
does not include DualReplica).
In `@features/features.go`:
- Line 752: The call to
enhancementPR("https://github.com/openshift/enhancements/pull/XXXX") contains a
placeholder URL that 404s; update that enhancementPR invocation to point to the
real enhancement PR (replace the URL with the actual pull request URL/number)
or, if there is no enhancement PR, remove the placeholder call and use
legacyFeatureGateWithoutEnhancement in its place so the feature entry is valid;
look for the enhancementPR(...) expression in the features.go entry and change
it accordingly.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yaml`:
- Around line 1066-1067: The CRD description strings that state "The 'Adaptable'
mode indicates the cluster dynamically adjusts control-plane behavior based on
current node count." (and the similar text at the second occurrence) advertise a
topology value that this Default CRD's enum validation does not accept; remove
these two description lines or modify them to explicitly state that "Adaptable"
is feature-gated/unavailable in this CRD, and ensure the CRD's enum (the
topology enum declared in this Default CRD) and its description are consistent
so the docs do not advertise an unsupported value.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yaml`:
- Around line 1357-1358: The Default CRD currently mentions "Adaptable" in the
topology mode descriptions even though the Default CRD's topology enum does not
include "Adaptable"; update the Default CRD description text in
controllerconfigs-Default.crd.yaml to either remove the "Adaptable" wording or
wrap the sentence with the same enum-overlay gating used elsewhere so the
description only appears when the topology enum includes "Adaptable". Locate the
topology description blocks (the human-readable string mentioning "Adaptable"
under the topology/mode fields) and either delete the "Adaptable" sentence or
conditionally include it via the overlay mechanism so the Default CRD no longer
advertises a mode it rejects.
---
Outside diff comments:
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yaml`:
- Around line 1055-1116: The descriptions for controlPlaneTopology and
infrastructureTopology in the OKD/Default/TechPreviewNoUpgrade CRD variants
mention an 'Adaptable' mode that is not present in their enum lists; remove the
sentences referencing the 'Adaptable' behavior from the description blocks for
controlPlaneTopology and infrastructureTopology in those variants so the prose
matches the enum, leaving the enum entries unchanged; note that CustomNoUpgrade
and DevPreviewNoUpgrade already include both the description and enum for
Adaptable, so do not modify those variants.
In
`@payload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yaml`:
- Around line 1132-1139: The CRD description for TechPreviewNoUpgrade
incorrectly mentions "Adaptable" even though the enum list (values:
HighlyAvailable, HighlyAvailableArbiter, SingleReplica, DualReplica, External)
rejects it; update the codegen/source marker that generates the description (the
TechPreviewNoUpgrade CRD docstring generator) to remove any reference to
"Adaptable" so the generated text matches the enum, and apply the same change
for the second occurrence corresponding to the block around lines 1176-1182;
regenerate the manifest to ensure the description and the enum values are
consistent.
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yaml`:
- Around line 1346-1407: The description mentions an "Adaptable" topology mode
but the OKD CRD enums for controlPlaneTopology and infrastructureTopology
(symbols: controlPlaneTopology, infrastructureTopology, and the documented
Adaptable mode) don't include it, causing a mismatch; fix by either updating the
source Go doc comment that feeds these CRD descriptions (e.g., the comment near
the types that define controlPlaneTopology/infrastructureTopology) to explicitly
state that "Adaptable" is only valid when the AdaptableTopology feature gate is
enabled, or adjust the OKD CRD description to remove or qualify the Adaptable
sentences so they clearly indicate they are conditional on the AdaptableTopology
gate (ensure the change targets the doc comment that generates the CRD
description or the OKD-specific CRD templating path so the enum remains
authoritative).
In
`@payload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yaml`:
- Around line 1425-1432: The TechPreviewNoUpgrade CRD still mentions the
'Adaptable' mode in its description while the enum arrays in this manifest (the
enum values under the control-plane replicaMode field in the
TechPreviewNoUpgrade CRD) do not include 'Adaptable'; update the manifest so
docs and schema match by either removing the descriptive lines that reference
'Adaptable' from the TechPreviewNoUpgrade CRD or gating that description behind
the DevPreview overlay, or alternatively add 'Adaptable' to the enum only in the
DevPreview/Dev-only CRD; make the change in the description block and the enum
values near the replicaMode field (the enum entries "- HighlyAvailable" ... "-
External") and apply the same fix to the other occurrence noted (around the
1469-1475 area).
---
Nitpick comments:
In `@config/v1/tests/infrastructures.config.openshift.io/AdaptableTopology.yaml`:
- Around line 7-16: Add negative onUpdate test cases to AdaptableTopology.yaml
to exercise the AdaptableTopology gate: add one onUpdate case that sets
status.controlPlaneTopology: Adaptable and expects rejection when the feature
gate is not enabled (name it e.g. "Reject Adaptable without feature gate"), and
add another onUpdate case that sets status.controlPlaneTopology: Bogus (or other
invalid enum) and expects rejection even when the AdaptableTopology gate is
enabled (name it e.g. "Reject invalid topology value"), ensuring the test bodies
reference status.controlPlaneTopology and the gate toggle used elsewhere so they
fail when validation should block the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c86170c6-c79d-4207-88be-d6bd0f27e642
⛔ Files ignored due to path filters (42)
config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AdaptableTopology+DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AdaptableTopology.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.featuregated-crd-manifests/infrastructures.config.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**config/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AAA_ungated.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AWSEuropeanSovereignCloudInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AdaptableTopology+DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AdaptableTopology.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureClusterHostedDNSInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/AzureDualStackInstall.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/DyanmicServiceEndpointIBMCloud.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/NutanixMultiSubnets.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/OnPremDNSRecords.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereHostVMGroupZonal.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**machineconfiguration/v1/zz_generated.featuregated-crd-manifests/controllerconfigs.machineconfiguration.openshift.io/VSphereMultiNetworks.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*openapi/openapi.jsonis excluded by!openapi/**
📒 Files selected for processing (23)
config/v1/tests/infrastructures.config.openshift.io/AdaptableTopology.yamlconfig/v1/types_infrastructure.gofeatures.mdfeatures/features.gopayload-manifests/crds/0000_10_config-operator_01_infrastructures-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-Default.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-OKD.crd.yamlpayload-manifests/crds/0000_10_config-operator_01_infrastructures-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-Default.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-OKD.crd.yamlpayload-manifests/crds/0000_80_machine-config_01_controllerconfigs-TechPreviewNoUpgrade.crd.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yamltools/codegen/pkg/manifestmerge/generator.go
|
/retest |
jaypoulz
left a comment
jaypoulz
left a comment
There was a problem hiding this comment.
Just a few notes of things I thought looked funny. Going to ping @eggfoobar for a second pair of eyes.
jeff-roche
force-pushed
the
adaptable-topology
branch
from
07a5f04 to
24e6766
Compare
Adds the AdaptableTopology feature gate scoped to SelfManaged clusters at DevPreviewNoUpgrade. Introduces the Adaptable enum value for both controlPlaneTopology and infrastructureTopology fields, gated behind the AdaptableTopology feature gate. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jeff-roche
force-pushed
the
adaptable-topology
branch
from
24e6766 to
ac64c29
Compare
openshift-ci
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/v1/types_infrastructure.go`:
- Around line 108-112: Add the missing compound FeatureGateAwareEnum marker so
the ControlPlaneTopology enum preserves "Adaptable" when both gates are enabled:
update the feature-gate annotations on ControlPlaneTopology (TopologyMode) to
include a compound marker with featureGate=AdaptableTopology+DualReplica and
enum that includes
HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External
(matching the AdaptableTopology overlay), ensuring the compound marker is
present alongside the existing single-gate markers so overlays applied in sort
order do not drop Adaptable under dual-gate profiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a64e1e46-9094-4c6c-a8ce-81717a72d082
⛔ Files ignored due to path filters (2)
config/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*machineconfiguration/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*
📒 Files selected for processing (12)
config/v1/tests/infrastructures.config.openshift.io/AdaptableTopology.yamlconfig/v1/types_infrastructure.gofeatures.mdfeatures/features.gopayload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yamlpayload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
✅ Files skipped from review due to trivial changes (9)
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-Default.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-TechPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-TechPreviewNoUpgrade.yaml
- features.md
- features/features.go
- config/v1/tests/infrastructures.config.openshift.io/AdaptableTopology.yaml
- payload-manifests/featuregates/featureGate-4-10-SelfManagedHA-DevPreviewNoUpgrade.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-DevPreviewNoUpgrade.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- payload-manifests/featuregates/featureGate-4-10-Hypershift-OKD.yaml
- payload-manifests/featuregates/featureGate-4-10-Hypershift-Default.yaml
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;External | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;External | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External | ||
| // +optional | ||
| ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether a compound AdaptableTopology+DualReplica marker exists anywhere
# in the repo, and inspect all FeatureGateAwareEnum markers on controlPlaneTopology.
echo "=== Compound marker search (any compound) ==="
rg -nP 'FeatureGateAwareEnum:featureGate=[A-Za-z0-9]+\+[A-Za-z0-9]+'
echo
echo "=== All FeatureGateAwareEnum markers in types_infrastructure.go ==="
rg -nP 'FeatureGateAwareEnum' config/v1/types_infrastructure.go
echo
echo "=== Generated CRDs: check Adaptable presence under DevPreview / CustomNoUpgrade ==="
fd -t f 'infrastructures.*DevPreview.*\.yaml|infrastructures.*CustomNoUpgrade.*\.yaml' \
| xargs -I{} sh -c 'echo "--- {} ---"; rg -n "Adaptable|DualReplica" "{}" | head -40'
echo
echo "=== Integration test profiles exercised by AdaptableTopology.yaml ==="
fd -t f 'AdaptableTopology.yaml' | xargs -I{} sh -c 'echo "--- {} ---"; cat "{}"'Repository: openshift/api
Length of output: 3994
Missing compound AdaptableTopology+DualReplica marker for controlPlaneTopology causes Adaptable to be absent from generated CRD under dual-gate profiles.
The PR description states a compound FeatureGateAwareEnum:featureGate=AdaptableTopology+DualReplica marker should be added for controlPlaneTopology. However, the code at lines 108–110 shows only three markers (baseline, DualReplica, AdaptableTopology), with no compound marker present anywhere in the repository.
This is a functional bug: under Server-Side Apply with Force=true, single-gate overlays apply in sort order and each fully replaces the previous enum. Because the DualReplica overlay (line 109) does not include Adaptable, it overwrites the AdaptableTopology overlay's enum when both DevPreview gates are enabled (e.g., in CustomNoUpgrade or DevPreviewNoUpgrade profiles). Generated CRDs for these profiles confirm that Adaptable is missing from controlPlaneTopology's enum. The integration test only exercises the AdaptableTopology gate alone, not the "both gates enabled" case, so this regression is not caught.
Add the compound marker:
Diff
// +openshift:validation:FeatureGateAwareEnum:featureGate=DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;External
// +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External
+ // +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology+DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External
// +optional
ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"`📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;External | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;External | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External | |
| // +optional | |
| ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"` | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;External | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;External | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology+DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External | |
| // +optional | |
| ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/v1/types_infrastructure.go` around lines 108 - 112, Add the missing
compound FeatureGateAwareEnum marker so the ControlPlaneTopology enum preserves
"Adaptable" when both gates are enabled: update the feature-gate annotations on
ControlPlaneTopology (TopologyMode) to include a compound marker with
featureGate=AdaptableTopology+DualReplica and enum that includes
HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External
(matching the AdaptableTopology overlay), ensuring the compound marker is
present alongside the existing single-gate markers so overlays applied in sort
order do not drop Adaptable under dual-gate profiles.
There was a problem hiding this comment.
@jeff-roche There is a consequence of how things get generated which means we need to add an extra annotation for situations like this where two FeatureGates exist for the same value.
This is what's missing, but the suggestion is wrong, you'll want to add this annotation under adaptable you added, this will generate a DualReplica+AdaptableTopology DevPreview file, which you'll want to add the tests for both DualReplica and AdaptableTopology.
+openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External
+openshift:validation:FeatureGateAwareEnum:requiredFeatureGate=DualReplica;AdaptableTopology,enum=HighlyAvailable;SingleReplica;DualReplica;Adaptable;External
Feel free to peruse this PR where we originally added Arbiter and DualReplica.
Locally you can run make integration to validate the unit tests won't fail.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
@jeff-roche: The following tests failed, say
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. |
eggfoobar
left a comment
eggfoobar
left a comment
There was a problem hiding this comment.
Looking good @jeff-roche we're just missing some small additions and tests, you'll need to add some extra files to resolve the verify, I think those are manually added if I remember right.
This is the location of the machine config tests, feel free to copy and modify the DualReplica one.
- MachineConfig location: https://github.com/openshift/api/tree/master/machineconfiguration/v1/tests/controllerconfigs.machineconfiguration.openshift.io
Some of the other stuff should have been generated as well, after you add the machine config stuff, try running the generate again PROTO_OPTIONAL=1 make update, we're missing some CustomNoUPgrade CRDs changes.
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;External | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=DualReplica,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;External | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External | ||
| // +optional | ||
| ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"` |
There was a problem hiding this comment.
@jeff-roche There is a consequence of how things get generated which means we need to add an extra annotation for situations like this where two FeatureGates exist for the same value.
This is what's missing, but the suggestion is wrong, you'll want to add this annotation under adaptable you added, this will generate a DualReplica+AdaptableTopology DevPreview file, which you'll want to add the tests for both DualReplica and AdaptableTopology.
+openshift:validation:FeatureGateAwareEnum:featureGate=AdaptableTopology,enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;DualReplica;Adaptable;External
+openshift:validation:FeatureGateAwareEnum:requiredFeatureGate=DualReplica;AdaptableTopology,enum=HighlyAvailable;SingleReplica;DualReplica;Adaptable;External
Feel free to peruse this PR where we originally added Arbiter and DualReplica.
Locally you can run make integration to validate the unit tests won't fail.
4 participants
Summary
AdaptableTopologyfeature gate (DevPreviewNoUpgrade) andAdaptableTopologyModeenum constant to the Infrastructure APIFeatureGateAwareEnummarkers on bothcontrolPlaneTopologyandinfrastructureTopologyto acceptAdaptablewhen the feature gate is enabledDetails
Adaptable Topology enables clusters to dynamically adjust control-plane and infrastructure behavior based on current node count. This PR adds the foundational API types — the feature gate definition, the enum constant, and the validation markers that gate the new value behind
DevPreviewNoUpgrade.A compound
AdaptableTopology+DualReplicamarker is required forcontrolPlaneTopologybecause both feature gates are enabled in DevPreview and the CRD composition uses Server-Side Apply withForce=true(last overlay wins). The codegen sort fix (first commit) ensures compound overlays are always applied after all single-gate overlays.Test plan
AdaptableTopology.yamlintegration tests pass (controlPlaneTopology and infrastructureTopology both acceptAdaptablewhen gated)DualReplica.yamlandHighlyAvailableArbiter.yamltests unaffectedAdaptablein both topology enumsAdaptablemake verifypasses🤖 Generated with Claude Code