External control plane topology #744

csrwng · 2021-04-19T22:13:16Z

Add support for indicating whether a control plane is external through the ControlPlaneTopology field

csrwng · 2021-04-19T22:14:13Z

/cc @openshift/team-hypershift-maintainers @derekwaynecarr

dhellmann · 2021-04-23T14:57:49Z

I think this is a 4.9 release priority, so I'm adding the label to make it show up that way in the weekly newsletter.

/priority important-soon

eranco74 · 2021-04-25T11:56:38Z

enhancements/external-control-plane-topology.md

+
+## Proposal
+
+- Add `External` as an additional option to the `TopologyMode` type for `Infrastructure` 


This should replace the annotation, right?

annotations: include.release.openshift.io/ibm-cloud-managed: "true"

@eranco74 this is not going to take the place of cluster profiles. The cluster profile is for the CVO to decide which manifests to include/exclude or to set appropriate node selectors for operators. The controlPlaneTopology value is for components to adjust their behavior once they're running.

enhancements/external-control-plane-topology.md

csrwng · 2021-04-26T12:57:32Z

/cc @mfojtik @s-urbaniak @spadgett @dmage @Miciah
Please let me know if there's additional folks that should review this.

csrwng · 2021-04-26T12:57:46Z

/retest

mrunalp · 2021-04-26T16:08:14Z

enhancements/external-control-plane-topology.md

+- Allow expressing a new type of control plane topology in the Infrastructure resource inside an
+  OCP cluster.
+
+- Provide operators/components that change their behavior based on whether the control plane is external or


Are the list of components that need to be changed captured somewhere?

Currently it's components that have been modified to work with the IBMCloud platform type, will add a list with corresponding PRs

@mrunalp the list of components are useful, but really this is more saying 'there will never be a role=master' host in this cluster where set to External

wking · 2021-04-26T16:56:57Z

enhancements/external-control-plane-topology.md

+
+- Provide a design for running OCP with an external control plane.
+
+- Describe how the `controlPlaneTopology` field will be set for an external control plane deployment.


This enhancement is talking about the value it will be set to. Are you saying that we will not specify the actor which sets the value?

Yes, I didn't think it was relevant to explain that Hypershift or IBM ROKS toolkit do their own bootstrapping and will set the value appropriately. But if you think it's relevant I can add it to the doc.

I'm fine leaving it out. But maybe rephrase this line to:

Describe the mechanics of populating the controlPlaneTopology field. This enhancement only introduces the new value.

or some such to make it more clear what portion is out of scope.

@wking for clarification, the externalized control plane manager is responsible for projecting this value into the end-user cluster data plane.

dhellmann · 2021-04-26T17:20:04Z

There's a bug in the lint script that means the fact that this enhancement is missing a bunch of sections isn't caught by the automation. That bug is being fixed in #752.

The output of the script when I run it locally is:

enhancements/external-control-plane-topology.md missing "### User Stories"
enhancements/external-control-plane-topology.md missing "### Risks and Mitigations"
enhancements/external-control-plane-topology.md missing "## Design Details"
enhancements/external-control-plane-topology.md missing "### Test Plan"
enhancements/external-control-plane-topology.md missing "### Graduation Criteria"
enhancements/external-control-plane-topology.md missing "#### Dev Preview -> Tech Preview"
enhancements/external-control-plane-topology.md missing "#### Tech Preview -> GA"
enhancements/external-control-plane-topology.md missing "#### Removing a deprecated feature"
enhancements/external-control-plane-topology.md missing "### Upgrade / Downgrade Strategy"
enhancements/external-control-plane-topology.md missing "### Version Skew Strategy"
enhancements/external-control-plane-topology.md missing "## Implementation History"
enhancements/external-control-plane-topology.md missing "## Drawbacks"
enhancements/external-control-plane-topology.md missing "## Alternatives"

wking · 2021-04-26T17:36:23Z

enhancements/external-control-plane-topology.md

+
+Existing components such as Console and Monitoring that use platform type of `IBMCloud` to modify their behavior
+for external control planes would need to switch to `controlPlaneTopology` to determine whether the control plane
+is external.


Is the test plan for this "CI jobs with an external control plane pass e2e without blowing up"? Do we have CI coverage for things like whatever console twiddles based on this (looks like maybe just dashboard availability)? Or is the expectation that we make a best-effort attempt to transition existing IBMCloud consumers and then fix any we miss (or which slip in later) as we stumble across them?

Yes, I didn't think it was relevant to explain that Hypershift or IBM ROKS toolkit do their own bootstrapping and will set the value appropriately. But if you think it's relevant I can add it to the doc.

Sorry answer to the wrong question :)

The test plan for IBM Cloud is that QE on their side will verify that we have not regressed in the tweaks that have been made based on the IBMCloud platform. On our side, we don't yet have automated CI for Hypershift setups, but it's something that we're working towards.

wking · 2021-04-26T17:38:10Z

Now that #752 has landed, checking vs. this PR:

/test markdownlint

wking · 2021-04-26T17:44:07Z

CI cluster flake: the build src failed after 49s with reason OutOfMemoryKilled.

/retest

wking · 2021-04-26T18:28:18Z

openshift/release#18081 should remove the memory limit we were overshooting.

/retest

csrwng · 2021-04-26T18:52:02Z

@dhellmann the lint job passed, should I have seen an error about the missing sections?

dhellmann · 2021-04-26T19:42:39Z

@dhellmann the lint job passed, should I have seen an error about the missing sections?

The lint job uses a script in the repo, so you'll need to rebase to see the errors.

CI apparently checks out the base branch and merges the pull request branch before running tests [1]: INFO[2021-04-26T18:28:37Z] Resolved source https://github.com/openshift/enhancements to master@42b44d6e, merging: openshift#744 d8c0ef8 ... So instead of using the local 'master' branch (which includes that new merge), use origin/master to calculate changed files. [1]: https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_enhancements/744/pull-ci-openshift-enhancements-master-markdownlint/1386749042607788032#1:build-log.txt%3A3

wking · 2021-04-26T23:04:34Z

#755 landed.

/test markdownlint

wking · 2021-04-27T15:55:42Z

#756 has landed.

/test markdownlint

wking · 2021-04-27T16:34:46Z

Prow didn't notice.

/test markdownlint

wking · 2021-04-27T16:47:50Z

Hooray (I guess ;):

INFO[2021-04-27T16:40:00Z] enhancements/external-control-plane-topology.md missing "### User Stories" 
INFO[2021-04-27T16:40:00Z] enhancements/external-control-plane-topology.md missing "### Risks and Mitigations" 
INFO[2021-04-27T16:40:00Z] enhancements/external-control-plane-topology.md missing "## Design Details" 
...

csrwng · 2021-04-27T16:49:12Z

Will update later today :)

derekwaynecarr

It looks like all comments were addressed, and this is needed to differentiate Standalone OCP on IBM Cloud from Managed Control Planes on IBM Cloud (or other infrastructures).

/approve
/lgtm

derekwaynecarr · 2021-05-04T17:57:21Z

enhancements/external-control-plane-topology.md

+- Allow expressing a new type of control plane topology in the Infrastructure resource inside an
+  OCP cluster.
+
+- Provide operators/components that change their behavior based on whether the control plane is external or


@mrunalp the list of components are useful, but really this is more saying 'there will never be a role=master' host in this cluster where set to External

derekwaynecarr · 2021-05-04T17:58:52Z

enhancements/external-control-plane-topology.md

+
+- Provide a design for running OCP with an external control plane.
+
+- Describe how the `controlPlaneTopology` field will be set for an external control plane deployment.


@wking for clarification, the externalized control plane manager is responsible for projecting this value into the end-user cluster data plane.

openshift-ci-robot · 2021-05-04T18:00:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2021-05-04T19:53:02Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-05-04T20:29:00Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-05-04T22:09:58Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2021-05-04T22:24:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

dhellmann · 2021-05-05T00:38:14Z

/hold

The lint job is failing because this document is missing a bunch of the required headers from the template. Please restore those sections and put something like N/A if they do not apply to this design.

+ hack/template-lint.sh
enhancements/external-control-plane-topology.md missing "### User Stories"
enhancements/external-control-plane-topology.md missing "### Risks and Mitigations"
enhancements/external-control-plane-topology.md missing "## Design Details"
enhancements/external-control-plane-topology.md missing "### Test Plan"
enhancements/external-control-plane-topology.md missing "### Graduation Criteria"
enhancements/external-control-plane-topology.md missing "#### Dev Preview -> Tech Preview"
enhancements/external-control-plane-topology.md missing "#### Tech Preview -> GA"
enhancements/external-control-plane-topology.md missing "#### Removing a deprecated feature"
enhancements/external-control-plane-topology.md missing "### Upgrade / Downgrade Strategy"
enhancements/external-control-plane-topology.md missing "### Version Skew Strategy"
enhancements/external-control-plane-topology.md missing "## Implementation History"
enhancements/external-control-plane-topology.md missing "## Drawbacks"
enhancements/external-control-plane-topology.md missing "## Alternatives"

s-urbaniak · 2021-05-05T08:40:00Z

enhancements/external-control-plane-topology.md

+
+Existing components such as Console and Monitoring that use platform type of `IBMCloud` to modify their behavior
+for external control planes would need to switch to `controlPlaneTopology` to determine whether the control plane
+is external.


cc @simonpasquier

openshift-ci · 2021-05-18T14:17:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [derekwaynecarr]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

csrwng · 2021-05-18T14:49:18Z

@dhellmann I've added the missing sections

dhellmann · 2021-05-18T14:54:25Z

/hold cancel
/lgtm

openshift-ci-robot requested review from dhellmann and smarterclayton April 19, 2021 22:13

openshift-ci-robot requested a review from derekwaynecarr April 19, 2021 22:14

openshift-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Apr 23, 2021

eranco74 reviewed Apr 25, 2021

View reviewed changes

sttts reviewed Apr 26, 2021

View reviewed changes

enhancements/external-control-plane-topology.md Show resolved Hide resolved

deads2k reviewed Apr 26, 2021

View reviewed changes

enhancements/external-control-plane-topology.md Outdated Show resolved Hide resolved

csrwng force-pushed the external_cp branch from aa970f8 to d8c0ef8 Compare April 26, 2021 12:55

openshift-ci-robot requested review from dmage, mfojtik, Miciah, s-urbaniak and spadgett April 26, 2021 12:57

mrunalp reviewed Apr 26, 2021

View reviewed changes

wking reviewed Apr 26, 2021

View reviewed changes

derekwaynecarr approved these changes May 4, 2021

View reviewed changes

openshift-ci-robot assigned derekwaynecarr May 4, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2021

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2021

s-urbaniak reviewed May 5, 2021

View reviewed changes

External control plane topology

46da245

csrwng force-pushed the external_cp branch from d8c0ef8 to 46da245 Compare May 18, 2021 14:15

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label May 18, 2021

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 18, 2021

openshift-ci bot assigned dhellmann May 18, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 18, 2021

openshift-merge-robot merged commit dd37ee5 into openshift:master May 18, 2021


		## Proposal

		- Add `External` as an additional option to the `TopologyMode` type for `Infrastructure`


		- Provide a design for running OCP with an external control plane.

		- Describe how the `controlPlaneTopology` field will be set for an external control plane deployment.

External control plane topology #744

External control plane topology #744

Conversation

csrwng commented Apr 19, 2021

csrwng commented Apr 19, 2021

dhellmann commented Apr 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csrwng commented Apr 26, 2021

csrwng commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann commented Apr 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Apr 26, 2021

wking commented Apr 26, 2021

wking commented Apr 26, 2021

csrwng commented Apr 26, 2021

dhellmann commented Apr 26, 2021

wking commented Apr 26, 2021

wking commented Apr 27, 2021

wking commented Apr 27, 2021

wking commented Apr 27, 2021

csrwng commented Apr 27, 2021

derekwaynecarr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 4, 2021

openshift-bot commented May 4, 2021

openshift-bot commented May 4, 2021

openshift-bot commented May 4, 2021

openshift-bot commented May 4, 2021

dhellmann commented May 5, 2021

Choose a reason for hiding this comment

openshift-ci bot commented May 18, 2021

csrwng commented May 18, 2021

dhellmann commented May 18, 2021

wking Apr 28, 2021 •

edited

Loading