OCPEDGE-2746: Add MutableTopology feature gated infra spec.controlPlaneTopology#2891
OCPEDGE-2746: Add MutableTopology feature gated infra spec.controlPlaneTopology#2891jeff-roche wants to merge 5 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@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: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds an optional Suggested reviewers
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
[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. |
| // 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
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
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>
6bc4bb9 to
eaf0036
Compare
Signed-off-by: Jeff Roche <jeroche@redhat.com>
Signed-off-by: Jeff Roche <jeroche@redhat.com>
|
@jeff-roche: 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. |
…opology Signed-off-by: Jeff Roche <jeroche@redhat.com>
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