OCPEDGE-2746: Add MutableTopology feature gated infra spec.controlPlaneTopology#2891
OCPEDGE-2746: Add MutableTopology feature gated infra spec.controlPlaneTopology#2891jeff-roche wants to merge 3 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-roche: This pull request references OCPEDGE-2746 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. |
|
Hello @jeff-roche! Some important instructions when contributing to openshift/api: |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (8)
📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds an optional 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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.12.2)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 |
|
The linter failure doesn't make much sense to me, enum should be a better validator than string length. |
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum= | ||
| // +openshift:validation:FeatureGateAwareEnum:featureGate=MutableTopology,enum=HighlyAvailable;SingleReplica |
There was a problem hiding this comment.
Since the field is gated, you don't need to gate the validation
| // +openshift:validation:FeatureGateAwareEnum:featureGate="",enum= | |
| // +openshift:validation:FeatureGateAwareEnum:featureGate=MutableTopology,enum=HighlyAvailable;SingleReplica | |
| // +kubebuilder:validation:enum=HighlyAvailable;SingleReplica |
There was a problem hiding this comment.
So switching to kubebuilder validation appears to be less-secure (according to the integration tests). the feature gate aware enum successfully blocks invalid values in the integration tests whereas the standard kubebuilder validation does not.
There was a problem hiding this comment.
Ahh, my bad. That's because it's case sensitive, needs Enum not enum in that final segment
| // controlPlaneTopology expresses the desired topology configuration for control nodes. | ||
| // The 'HighlyAvailable' mode represents a "normal", 3 control node cluster. | ||
| // The 'SingleReplica' mode represents configuration where there is a single control node. | ||
| // If left blank, no change is required and no transitions will be triggered. |
There was a problem hiding this comment.
I'd prefer to explain here what happens when you set the different values. At the moment you're kind of defining the meaning but not saying what the observable changes are.
We also need to think about valid transitions. For example, once the spec is HighlyAvailable, and the status is also HighlyAvailable, moving spec back to SingleReplica wouldn't be supported and we should prevent that with a validation and explain it here
There was a problem hiding this comment.
Where would you suggest is the best place to prevent that? Current plan for the new CCO controller was only react to SNO->HA for now, do you think we need some sort of CEL validation to prevent changing from HA->SNO if status is HA?
There was a problem hiding this comment.
You can add a CEL rule to the infrastructure object to to prevent the spec change if status is HA yes, that shouldn't be super complex I don't think
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/v1/types_infrastructure.go (1)
59-66: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftEnhance field documentation to explain observable behavior and add transition validation.
The field documentation needs improvement in three areas:
Observable behavior: The comment defines what each mode represents but doesn't explain what observable changes occur when setting these values. For example, the status field (lines 108-115) explains that operators "should not configure the operand for highly-available operation" in SingleReplica mode. The spec field should similarly explain what happens when you request each topology.
Valid transitions: A past review comment indicates that certain transitions (e.g., HighlyAvailable → SingleReplica after status is set) may not be supported. If transition constraints exist, they must be:
- Documented in the field comment
- Enforced with
+kubebuilder:validation:XValidationCEL rulesRelationship to status: The comment should clarify how
spec.controlPlaneTopologyrelates tostatus.controlPlaneTopology, especially regarding the behavior when the spec field is omitted.As per coding guidelines, field relationships or constraints must be enforced with XValidation rules using CEL expressions, and all validation markers must be fully documented in field comments.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/v1/types_infrastructure.go` around lines 59 - 66, The controlPlaneTopology field comment needs enhancement in three areas: add documentation explaining what observable changes occur when each mode is set (e.g., what happens with HighlyAvailable vs SingleReplica), document any valid topology transitions and add kubebuilder:validation:XValidation CEL rules to enforce transition constraints (such as preventing HighlyAvailable to SingleReplica transitions after status is set), and clarify in the comment how the spec.controlPlaneTopology field relates to the status.controlPlaneTopology field including the behavior when the spec field is omitted. Update the comment block above the controlPlaneTopology field definition and add appropriate XValidation markers with CEL expressions to enforce any documented transition rules.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@config/v1/types_infrastructure.go`:
- Around line 59-66: The controlPlaneTopology field comment needs enhancement in
three areas: add documentation explaining what observable changes occur when
each mode is set (e.g., what happens with HighlyAvailable vs SingleReplica),
document any valid topology transitions and add
kubebuilder:validation:XValidation CEL rules to enforce transition constraints
(such as preventing HighlyAvailable to SingleReplica transitions after status is
set), and clarify in the comment how the spec.controlPlaneTopology field relates
to the status.controlPlaneTopology field including the behavior when the spec
field is omitted. Update the comment block above the controlPlaneTopology field
definition and add appropriate XValidation markers with CEL expressions to
enforce any documented transition rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f6a425df-2096-4cea-b08f-b935c6be042c
📒 Files selected for processing (1)
config/v1/types_infrastructure.go
openshift-ci
Bot
added
the
needs-rebase
Introduce the MutableTopology feature gate which enables spec.controlPlaneTopology on the Infrastructure resource, allowing cluster topology to be set to HighlyAvailable or SingleReplica. CRD manifests are now split per cluster profile (Hypershift, SelfManagedHA) so the field is only present in the appropriate profile/feature-set combinations. Includes integration tests verifying: - Accepted values when the gate is enabled (MutableTopology.yaml) - Field pruning when the gate is disabled (AAA_ungated.yaml) Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Jeff Roche <jeroche@redhat.com>
jeff-roche
force-pushed
the
controlPlaneTopologySpec
branch
from
6bc4bb9 to
eaf0036
Compare
openshift-ci
Bot
removed
the
needs-rebase
Signed-off-by: Jeff Roche <jeroche@redhat.com>
|
@jeff-roche: The following test 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. |
3 participants
Utilizes the MutableTopology feature gate which enables spec.controlPlaneTopology on the Infrastructure resource, allowing cluster topology to be set to HighlyAvailable or SingleReplica.
CRD manifests are now split per cluster profile (Hypershift, SelfManagedHA) so the field is only present in the appropriate profile/feature-set combinations.
Includes integration tests verifying:
Implements API changes needed for enhancements#2008