OCPEDGE-2280 OCPEDGE-2640: mutable topology - #2008
Conversation
|
@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. |
438e03c to
98a1ba4
Compare
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.
@brandisher I've added a new paragraph under the |
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
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
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. ). |
|
/jira refresh |
|
@jeff-roche: This pull request references OCPEDGE-2280 which is a valid jira issue. 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: This pull request references OCPEDGE-2280 which is a valid jira issue. This pull request references OCPEDGE-2640 which is a valid jira issue. 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. |
eb958cf to
a9bbfb9
Compare
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 105-109: Define InfrastructureStatus topology fields consistently
as either the converged observed state or the admitted target state. Prefer
retaining observed-state semantics: do not update controlPlaneTopology or
infrastructureTopology until validation and transition completion succeed, and
add a persisted transition phase for in-progress or failed transitions; revise
the affected wording and transition sequence accordingly.
- Around line 301-323: The supported-transition admission predicate must require
the current topology to be SingleReplica with platform none before applying the
listed preconditions. Also explicitly exclude HyperShift and IBI clusters,
either in this checklist or in the supported-transition matcher, so unsupported
three-node clusters cannot be admitted.
- Around line 325-330: The orchestration steps must make admission and the
topology status update conflict-safe: retain the Infrastructure CR
resourceVersion observed during the re-read, require the status update for
controlPlaneTopology and infrastructureTopology to compare against it, and
restart admission when the compare-and-swap conflicts. Do not commit status for
a request whose spec changed after admission.
- Around line 280-284: Clarify the upgrade policy in the MutableTopology
feature-gate lifecycle section: explicitly state whether clusters enabled during
Dev Preview or Tech Preview must move to the Default feature set before standard
upgrades after transition, or revise the policy so they remain on the applicable
no-upgrade feature set. Ensure this guidance is consistent with the stated
feature-set progression and pre-GA upgrade-testing requirement.
🪄 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: a59ababd-20cc-4758-89de-0da6d7e5cd07
📒 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>
a9bbfb9 to
938c10c
Compare
c6afce4 to
47ca7e1
Compare
Signed-off-by: Jeff Roche <jeroche@redhat.com>
47ca7e1 to
268645d
Compare
|
LGTM from the Workloads team perspective (cc @ardaguclu) with regard to changes in oc |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
c3d0090 to
36b3473
Compare
Signed-off-by: Jeff Roche <jeroche@redhat.com>
36b3473 to
77c4682
Compare
|
@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. |
|
/lgtm Reapplying final /lgtm post-addressing Prashanth's feedback. Thanks all! |
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.