Skip to content

Conversation

@hexfusion
Copy link
Contributor

@hexfusion hexfusion commented Sep 23, 2020

As OpenShift 4 extends its use cases additional workflows for how the bootstrapping of the cluster will evolve. For example, assisted-installer uses the openshift-installer to generate manifests and then independently manages the bootstrapping of the cluster after install complete. Because these alternative workflows require in many cases the cluster and operators to observe these changes clear signal is required.

This PR introduces an optional field to the InstallConfig InstallType. This type which will be embedded into the cluster-config-v1 ConfigMap in the kube-system namespace. I feel this change requires very little overhead for maintenance meanwhile providing very important signal to the cluster.

Other uses for this flag could include for example CRC single node cluster [1] and new future edge installations. This allows the logic for how these installs are managed to be handled by the cluster operators instead of additional knobs to the installer itself.

[1] openshift/enhancements#302

enhancement: openshift/enhancements#480
blocking 4.7 epic: https://issues.redhat.com/browse/ETCD-94

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign abhinavdahiya
You can assign the PR to them by writing /assign @abhinavdahiya in a comment when ready.

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

Details Needs approval from an approver in each of these files:

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

@hexfusion hexfusion force-pushed the add-installType branch 2 times, most recently from 70f252b to b06a994 Compare September 23, 2020 16:20
@hexfusion hexfusion changed the title [wip] *: add InstallType to InstallConfig *: add InstallType to InstallConfig Sep 23, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 23, 2020
@hexfusion
Copy link
Contributor Author

hexfusion commented Sep 23, 2020

// InstallType provides signal to the cluster that the installation will not follow the default installer workflow.
//
// +optional
InstallType string `json:"installType,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there's a more descriptive name to be found, and it should be an enum at least... seems like it will inevitably become mirror of PlatformType to some degree but with different lifecycle implications.

Is a raw enum going to be enough? (It wasn't for platform type.) What's the relationship between this type and and platform types? Which combinations are supported, and what are the clear semantics and cluster implications for each type and combination of type/platform? etc.

Would a vendor annotation on the installconfig make any sense as an alternative if we aren't able to more carefully specify these things right now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is a raw enum going to be enough? (It wasn't for platform type.) What's the relationship between this type and and platform types? Which combinations are supported, and what are the clear semantics and cluster implications for each type and combination of type/platform? etc.

I agree with you in general but the idea is that the logic and relationships that each operator would conclude should be based on the value. We could certainly make this more strict and only define explicitly supported types. Then extended the list as necessary.

Would a vendor annotation on the installconfig make any sense as an alternative if we aren't able to more carefully specify these things right now?

If no agreement can be made I am open to any reasonable signal.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(see: openshift/enhancements#480 (comment))

I agree that tying the install type to a platform doesn't make sense. The idea is to completely platform agnostic as the install type is not related to the platform it is performed on the platform. That was my intention in making the key at root level.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like there's a more descriptive name to be found, and it should be an enum at least

If you have a better name I am open to change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm most interested in clarity of semantics. Possible values, what they mean on their own, what they mean with respect to platform types.

For example, based on what I understand so far, here's a try:

// AssistedInstaller indicates the platform will assume an installation
// topology consisting of exactly 3 nodes which will be the only available
// nodes to complete cluster bootstrapping.
//
// The platform may relax certain high availability constraints to achieve
// bootstrapping under the constrained topology.
//
// This install type is only supported when the platform type is BareMetal.
type AssistedInstaller InstallType = "AssistedInstaller"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it needs to be even more specific in terms of the machine pools. Isn't the requirement for there to be a single master pool with 3 replicas and an empty worker pool? This should be unambiguous.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why? we let the user choose how many workers he adds - among all the discovered hardware, we auto-select 3 masters and all the rest become workers

Copy link
Contributor

@ironcladlou ironcladlou Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Is the absence of the dedicated bootstrap node the substantive difference? Again, we should figure out how to define it

Copy link
Contributor Author

@hexfusion hexfusion Sep 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worker pools are not assumed but unlike compact clusters workers are possible. @romfreiman keep me honest here.

@romfreiman
Copy link

@openshift-ci-robot
Copy link
Contributor

@hexfusion: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-ovirt a89f586 link /test e2e-ovirt
ci/prow/e2e-libvirt a89f586 link /test e2e-libvirt
ci/prow/e2e-metal-ipi a89f586 link /test e2e-metal-ipi
ci/prow/e2e-crc a89f586 link /test e2e-crc

Full PR test history. Your PR dashboard.

Details

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 kubernetes/test-infra repository. I understand the commands that are listed here.

@hardys
Copy link

hardys commented Sep 29, 2020

I'm looking at the ClusterProfile proposal and wondering if we have some overlapping requirements here - that's focussed on the CVO, but perhaps we should have a "2 master bootstrap" cluster profile that sets some data via a manifest, that the CEO can consume to influence the bootstrap behavior?

Also in the initial description it's mentioned that this requirement could include CRC/edge single node requirements, and we can't necessarily say all of those environments will be deployed via the assisted installer (could be UPI without the assisted workflow if customers are already using that approach and have their own automation in place, for example) so relying on a flag that defines the InstallType may prove inflexible in those cases?

@hardys
Copy link

hardys commented Sep 29, 2020

Also see #3986

@hexfusion
Copy link
Contributor Author

hexfusion commented Sep 29, 2020

I'm looking at the ClusterProfile proposal and wondering if we have some overlapping requirements here - that's focussed on the CVO, but perhaps we should have a "2 master bootstrap" cluster profile that sets some data via a manifest, that the CEO can consume to influence the bootstrap behavior?

The point of InstallType is that we are completely explicit about the intent of install to the cluster. To me "2 master bootstrap" is not clear it is a generalization. Where "AssistedInstaller" is very clear and explicit. How the signal is passed really is not so important to the cluster as what the signal means. The cluster needs to understand with certainty if the cluster will not look like it normally should why, and what it should expect.

Also in the initial description it's mentioned that this requirement could include CRC/edge single node requirements, and we can't necessarily say all of those environments will be deployed via the assisted installer (could be UPI without the assisted workflow if customers are already using that approach and have their own automation in place, for example) so relying on a flag that defines the InstallType may prove inflexible in those cases?

So what I mean is that the InstallType can include those install types once supported. As the key is root level it is not bound to any provider etc. This would encompass only installs that have a workflow that is nonstandard or on its own would be considered unsupported. So back to the "2 master bootstrap" this would require some feature of supporting 2 masters so that the shortcomings of 2 member clusters were resolved in some smart way. The feature would be defined as an InstallType.

To be clear I don't have super strong feeling on how the signal is passed but it needs to be clear.

type InstallType string

const (
  // AssistedInstaller what cluster shoudl expect is....
  type AssistedInstaller InstallType = "AssistedInstaller"
  type CodeReadyContainers InstallType = "CodeReadyContainers"
  type NewEdgeSomething InstallType = "NewEdgeSomething"
)

@jstuever
Copy link
Contributor

jstuever commented Oct 5, 2020

/cc @abhinavdahiya

@gbraad
Copy link

gbraad commented Oct 13, 2020

I'm looking at the ClusterProfile proposal and wondering if we have some overlapping requirements here - that's focussed on the CVO, but perhaps we should have a "2 master bootstrap" cluster profile that sets some data via a manifest, that the CEO can consume to influence the bootstrap behavior?

please find us on #cluster-profiles for further discussion ?

Also, as I understood, an InstallType that is specifc to "CodeReady Containers", etc is not actually wished for. The profiles should handle being able to handle the needed functionality for specific use-cases, like NonHA, etc

@hexfusion
Copy link
Contributor Author

@gbraad I joined your forum but had no response to a few questions I will schedule a meeting next week.

@romfreiman
Copy link

@hexfusion @gbraad please add me as well

@gbraad
Copy link

gbraad commented Nov 9, 2020

As far as I can see @hexfusion didn't respond on the internal at-reply since the 13th of Oct? Anyway, both @guillaumerose and @cfergeau are working on this. If you have any requests or questions, please schedule a meeting with them.

@hexfusion hexfusion closed this Nov 11, 2020
@hexfusion hexfusion deleted the add-installType branch November 11, 2020 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants