OCPEDGE-2280: mutable topology - #2008
Conversation
jeff-roche
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jeff-roche: This pull request references OCPEDGE-2280 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
jeff-roche
changed the title
438e03c to
98a1ba4
Compare
openshift-ci
Bot
assigned
dgoodwin,
dusk125,
eggfoobar,
jaypoulz,
jerpeter1,
JoelSpeed,
patrickdillon,
sdodson and
tjungblu
brandisher
left a comment
brandisher
left a comment
There was a problem hiding this comment.
I'm missing a "why" statement covering why a day 2, out-of-payload operator is the right choice for this. The CVO section towards the bottom hints at the why a bit but more explicit detail is needed.
With that in mind, I haven't reviewed the EP fully because I don't understand why this is the approach we're taking. The assessment of CVO seems very light and not enough to exclude that as a potential option to meet the goals.
|
[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 |
@brandisher I've added a new paragraph under the |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
🤖 Generated with Claude Code
There are significant portions of this proposal that assume behaviour of OpenShift that either doesn't exist, or doesn't work in the way proposed. I'm assuming here that this is hallucination of Claude?
The EP as it stands today doesn't actually make sense for implementation. It also doesn't align with what I thought we had agreed on the architecture call.
Has anyone tried to manually take a cluster and scale up and manually transition from a single replica to multiple replicas? IMO this is the most important next step for this project
What I thought we had agreed:
- To scale from SNO to HA, the user must create two new control plane nodes and join them to the cluster
- On HighlyAvailable topology - KAS, KCM, etcd, etc all get scheduled automatically as static pods on these nodes - I don't see anything that prevents this based on if it's a SNO cluster today, this needs to be checked (it probably should)
- MCO still serves ignition for control plane nodes on SNO, so user needs to create the control plane nodes somehow to ignite from here
- New fields are added to the infrastructure spec to allow the user to say "I intend for this cluster to be HA going forward"
- A controller is added to cluster config operator
- This checks that the precondition of having additional control plane nodes in the cluster is met
- Once the precondition is met, it updates the status to reflect spec
- Operators now react to the change in status and transition from single to HA
- etcd operator promotes learners to full members, quorum goes from 1->3 (I don't know if this guard is in place today, we should add if not)
- KAS/KCM - no change, it already scheduled new KAS/kCM pods
- Others - Those that previously deploy a single replica of their operand now move to 2 replicas, other changes might be needed on a per operator basis, I was expecting those details in the EP but don't see them yet
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
I know the scope is limited to baremetal/platform:none, but I know there is interest for mutable topologies in cloud platforms as well so as much as appropriate I would to ensure the design leaves a path forward for those cloud platforms.
Also, like the other enhancement I don't see any mention of mastersSchedulable which affects the calculation for infrastructureTopology. How is the mastersSchedulable field handled/taken into account for this solution?
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
This one looks directionally correct 👍
|
Big update coming next week to realign this with CCO instead of a dedicated operator, add some more technical detail around the flow, and address masters schedulable. Thank you everyone for the quick and thorough reviews, I believe we are rapidly converging on a solid solution! |
a8d48b3 to
22b3682
Compare
|
Are there limitations for a SNO to TNF transition ? TNF requires BMC/Redfish so if the SNO bare metal hardware does not have it, does it block the transition? I could see this being a problem trying to match hardware in general (BMC firmware versions, vendor types, etc. ). |
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
This is much better than the previous iteration. I still fee like there's some disconnect between the new and old stuff, some stuff may still be hanging over from the previous iteration that doesn't quite make sense now, PTAL at my comments
Address review feedback from brandisher, JoelSpeed, zaneb, patrickdillon, DanielFroehlich, and dhensel-rh across 6 categories: API design: - Rename desiredTopology to desiredControlPlaneTopology - Make field empty by default (installer does not populate) - Replace CEL validation with DesiredTopologyMode named type - Drop ValidatingAdmissionPolicy for status field protection - Document spec-to-status mapping (CP topology, infra topology, mastersSchedulable) - Add worker node precondition check (compact clusters only) Workflow: - Clarify node-driven vs topology-driven operator reactions - CLI returns immediately; monitoring is separate - Failure handling uses standard K8s retry pattern (no spec reset) - Transition conditions on CCO ClusterOperator status (not infra CR) - Cancel semantics: only before status update (step 9) - CEO etcd scaling is independent (unsafe scaling path), not orchestrated by transition controller Accuracy: - CEO rollback replaced with manual quorum-restore.sh throughout - "type alias" corrected to "named type" (Go terminology) - Upgradeable=False blocks upgrades, not all version changes - "30+ operators" claim removed (uncited) - mastersSchedulable clarified as unchanged for SNO to HA compact Scope: - IBI clusters excluded (non-goal + topology considerations) - Platform:none rationale expanded (edge computing context) - Baremetal risk reframed as future scope - Backup compatibility added as open question Content: - Concrete failure examples (quorum loss, node readiness, operator reconciliation) - SLO dimensions defined in GA graduation criteria - Operational guidance updated (no availability guarantee, backup recommendation) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
| The overall orchestration differs from bootstrapping: bootstrapping uses a temporary bootstrap member that is later removed before the cluster reaches steady state, while a Day 2 transition adds permanent members to a running production cluster. Critically, the 2-voter intermediate state (steps 4–5 below) is unique to Day 2 transitions — it does not occur during bootstrapping. | ||
|
|
||
| 1. **Starting state**: 1 etcd voting member (quorum=1) | ||
| 2. CEO adds an etcd learner on the second control-plane node |
There was a problem hiding this comment.
The CEO uses learners regardless, the "unsafe" aspect is that the CEO doesn't check for quorum or member health on SNO, it just adds the new member as a learner and goes through the normal promotion process.
Address review feedback from brandisher, JoelSpeed, zaneb, patrickdillon, DanielFroehlich, and dhensel-rh across 6 categories: API design: - Rename desiredTopology to desiredControlPlaneTopology - Make field empty by default (installer does not populate) - Replace CEL validation with DesiredTopologyMode named type - Drop ValidatingAdmissionPolicy for status field protection - Document spec-to-status mapping (CP topology, infra topology, mastersSchedulable) - Add worker node precondition check (compact clusters only) Workflow: - Clarify node-driven vs topology-driven operator reactions - CLI returns immediately; monitoring is separate - Failure handling uses standard K8s retry pattern (no spec reset) - Transition conditions on CCO ClusterOperator status (not infra CR) - Cancel semantics: only before status update (step 9) - CEO etcd scaling is independent (unsafe scaling path), not orchestrated by transition controller Accuracy: - CEO rollback replaced with manual quorum-restore.sh throughout - "type alias" corrected to "named type" (Go terminology) - Upgradeable=False blocks upgrades, not all version changes - "30+ operators" claim removed (uncited) - mastersSchedulable clarified as unchanged for SNO to HA compact Scope: - IBI clusters excluded (non-goal + topology considerations) - Platform:none rationale expanded (edge computing context) - Baremetal risk reframed as future scope - Backup compatibility added as open question Content: - Concrete failure examples (quorum loss, node readiness, operator reconciliation) - SLO dimensions defined in GA graduation criteria - Operational guidance updated (no availability guarantee, backup recommendation) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7be83f9 to
0aab1bd
Compare
JoelSpeed
left a comment
JoelSpeed
left a comment
There was a problem hiding this comment.
Lets talk next week about conditions and reporting because I'm still not sure if what you say you want to report is possible, or at least, I'm not following what feeds into those conditions and their true/false states
|
From the oc point of view, EP looks good to me. |
|
@ardaguclu @JoelSpeed if either of you don't have any issues with the enhancement proposal as is, can you lgtm? |
WalkthroughThe proposal defines a feature-gated SNO-to-three-node compact HA transition on ChangesMutable topology transition
Estimated code review effort: 2 (Simple) | ~15 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Administrator
participant TopologyCLI
participant InfrastructureAPI
participant CCOTransitionController
participant Etcd
participant ClusterOperators
Administrator->>TopologyCLI: Request topology transition
TopologyCLI->>InfrastructureAPI: Set controlPlaneTopology
InfrastructureAPI->>CCOTransitionController: Reconcile desired topology
CCOTransitionController->>Etcd: Scale members sequentially
Etcd-->>CCOTransitionController: Report member and quorum status
CCOTransitionController->>ClusterOperators: Wait for operator health
ClusterOperators-->>CCOTransitionController: Report cluster readiness
CCOTransitionController->>InfrastructureAPI: Update topology conditions and status
🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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.
Inline comments:
In `@enhancements/topologies/mutable-topology.md`:
- Around line 532-534: Update the CLI interaction test description to verify
that `oc adm transition topology` patches `spec.controlPlaneTopology` and
returns immediately, without requiring progress monitoring. Remove monitoring
from this test and add a separate status-monitoring test only where the
corresponding UX is defined.
- Around line 188-200: The transition design must persist an explicit active
phase or generation through post-transition validation, rather than relying on
spec/status inequality as the reconciliation trigger. Update the documented
controller flow around the topology status update,
`TopologyTransitionCompleted`, and `TopologyTransitionFailed` to define durable
retry state across CCO restarts, and specify when each terminal condition is
cleared when a new retry begins.
- Around line 655-659: Update the “etcd Scaling Failures” resolution guidance to
remove restoring the pre-transition snapshot as a supported fallback. Keep
quorum restoration via standard disaster recovery procedures, and mention
snapshot restoration only as an unvalidated investigation item pending testing
and documentation.
- Line 280: Update the RBAC paragraph in mutable-topology.md to reference the
correct infrastructure field path, spec.controlPlaneTopology, and remove the
extra spec segment while preserving the surrounding access-control guidance.
- Around line 507-513: Renumber the Open Questions entries in the mutable
topology document so the sequence is contiguous and unambiguous. Update the
visible items currently numbered 2 through 5, adding the missing item only if
another question exists immediately before this excerpt; otherwise start this
section at 1 and preserve the existing question text.
- Around line 160-176: Reorder the documented workflow so the CLI admits the
transition and CCO marks it in progress before any additional control-plane
nodes join, preventing CEO from scaling etcd prematurely. Update the
preconditions and sequencing around the transition controller and node-driven
reactions, and define cancellation/recovery behavior for nodes added before
admission or after an abandoned transition.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ad219a06-3919-4200-912f-e211e840d0fa
📒 Files selected for processing (1)
enhancements/topologies/mutable-topology.md
d057722 to
4e0ac27
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@enhancements/topologies/mutable-topology.md`:
- Around line 323-329: Reconcile the topology transition workflow with the etcd
scaling contract across the documented workflow, conditions, component
responsibilities, and tests. Choose either CCO-owned orchestration with
admission and an explicit CEO/CCO handshake before scaling, or treat CEO’s
completed three-member scaling as an external prerequisite and remove CCO’s
scaling coordination and failure guarantees; apply the chosen contract
consistently to the transition admission and progress conditions.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b6220e9d-40e2-4137-8f9e-d8a52290b279
📒 Files selected for processing (1)
enhancements/topologies/mutable-topology.md
Introduce the Mutable Topology enhancement, which replaces the previous Adaptable Topology proposal. Instead of a new topology enum that all operators must interpret, this approach uses a dedicated operator (OTTO) to orchestrate transitions between existing fixed topology modes. Initial scope: SNO to HA compact on platform: none. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> enhancements/topologies: revise mutable topology proposal to base on CCO Move the topology transition controller from a standalone operator (OTTO) into cluster-config-operator. CCO owns the config.openshift.io API group and infrastructure CR lifecycle, making it the natural home. Key design decisions: - desiredTopology initialized by installer to match controlPlaneTopology (no kubebuilder default — value is cluster-specific) - Controller triggers on desiredTopology != status.controlPlaneTopology - On failure, controller resets desiredTopology to current topology - Upgrade blocked via Upgradeable=False during transitions - Condition types: TopologyTransitionProgressing, Completed, Failed - Per-operator topology audit required for Dev Preview entry Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Add additional reviewers for mutable topology Finalized reviewers for everyone who participated in the final review. feat: adding SNO to HA compact pre/transition/post steps Signed-off-by: Jeff Roche <jeroche@redhat.com>
4e0ac27 to
eb958cf
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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.
Inline comments:
In `@enhancements/topologies/mutable-topology.md`:
- Around line 24-25: Update the tracking metadata for OCPEDGE-2280 to use the
branch’s expected target version, 5.0.* or openshift-5.0.*, or document in the
enhancement why it intentionally targets openshift-4.22.
- Around line 160-175: Update the transition workflow so CCO admits the topology
transition and sets the CCO ClusterOperator Upgradeable=False before
administrator-joined nodes trigger CEO’s 1→2→3 etcd scaling. Ensure the
transition controller’s precondition and progress handling cover this ordering,
and apply the same sequencing correction to the corresponding Failure Handling
workflow.
- Around line 334-340: Define one shared node-role predicate specifying the
exact labels, taints, and count for the three compact control-plane/worker
nodes, then reuse it in preflight checks, post-transition validation, and
transition tests. Update the existing control-plane-only and worker-inclusive
checks so all workflow stages enforce the same role configuration.
- Around line 261-268: Update the transition status contract around
TopologyTransitionControllerProgressing and
TopologyTransitionControllerUpgradeable to use the same condition types
referenced by the workflow and support procedures, including an explicit failed
state. Define the status lifecycle for pending, rejected, admitted, retrying,
cancelled, and completed transitions, and document that CVO observes CCO’s
upgrade-blocking Upgradeable condition while CCO reconciles ClusterOperator
status to preserve it.
🪄 Autofix
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5e3c539c-b87b-45f6-9998-6cc46b375bbd
📒 Files selected for processing (1)
enhancements/topologies/mutable-topology.md
|
@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. |
17 participants
Summary
Introduces the Mutable Topology enhancement proposal, which enables OpenShift clusters to transition between topology modes as a Day 2 operation. This replaces the previous Adaptable Topology proposal.
Key Design Decisions
spec.desiredTopologyon the Infrastructure CR, validates preconditions, coordinates the transition across operators, and updates topology status fields when complete. CCO was chosen over CVO, CEO, and MCO (and over a standalone operator) because it owns theconfig.openshift.ioAPI group and the Infrastructure CR lifecycle. See Alternatives in the proposal for the full placement analysis.TopologyModevalues (SingleReplica,HighlyAvailable, etc.). Operators continue reacting to fixed topology values they already understand. Transition complexity is concentrated in a single controller rather than distributed across 30+ operators.spec.desiredTopologyexpresses administrator intent;status.controlPlaneTopologyreflects observed state. Mirrors theoc adm upgradepattern (patch spec, controller does the work).MutableTopologygate progresses through DevPreview → TechPreview → GA. Controller is not registered when the gate is disabled (zero runtime overhead).Scope
platform: noneoc adm transition topology HighlyAvailableUpgradeable=Falsewhile a transition is in progressWhat Changed (Revision History)
The proposal was revised to base the controller in CCO rather than proposing a dedicated standalone operator (OTTO). Key changes from the prior revision:
Upgradeable=Falseenforcement during transitions to prevent concurrent upgradesOut of Scope
platform: baremetal— pending keepalived resolution🤖 Generated with Claude Code
Summary by CodeRabbit
oc adm transition topologycommand.