installconfig: separate control plane and compute machine pools #1157

staebler · 2019-01-30T21:19:46Z

There should always be a control plane machine pool, so the declaration is moved from a generic machines field to a specific controlPlane field. The machines field contains only compute machines, so it is renamed to compute.

This is the first step in replacing references to master and worker with references to control plane and compute throughout the installer.

pkg/tfvars/tfvars.go

wking · 2019-01-30T21:49:55Z

pkg/asset/installconfig/installconfig.go

@@ -55,7 +57,7 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {

 	a.Config = &types.InstallConfig{
 		TypeMeta: metav1.TypeMeta{
-			APIVersion: "v1beta1",
+			APIVersion: "v1beta2",


I think we want a public constant for "the current version". For an example, see here and other changes in that PR.

abhinavdahiya · 2019-01-30T21:51:26Z

regarding fabb911 I think converters must exist in pkg/types as part of the api.

pkg/types/defaults/installconfig.go

wking · 2019-01-30T22:27:42Z

pkg/types/defaults/installconfig.go

+	}
+	for i, p := range c.Compute {
+		if p.Replicas == nil {
+			c.Compute[i].Replicas = &defaultReplicaCount


Do we want to default to 1 (on libvirt) or 3 (elsewhere) for compute pools we don't recognize? I think the previous zero default may be more appropriate as long as at least one compute pool has a non-zero replicas. I'm also fine revisiting the "we can pick a default for you" approach from #1146 and just erroring out if folks leave replicas unset.

At the moment, the only compute machine pool accepted by the validator is "worker". My expectation in the future is that the installer will not care what the name of the compute machine pools are. The installer will not treat the "worker" or "compute" machine pool any different than any other compute machine pool. Do you have different expectations?

wking · 2019-01-30T22:28:55Z

pkg/types/machinepools.go

@@ -9,6 +9,8 @@ import (
 // MachinePool is a pool of machines to be installed.
 type MachinePool struct {
 	// Name is the name of the machine pool.
+	// For the control plane machine pool, the name will always be "master".


Do we care what they name it? I'm fine leaving this up to the user.

Ultimately, we may not care what they name it. For now, (1) the installer does care, or (2) I don't care to rationalize about whether we really do care. As the rest of the changes from master and worker to control plane and compute play out, this restriction may be relaxed.

As a data point, we had this conversation in hive back when I was on that team. We could not come up with a compelling use case for the user to provide a custom name for the control plane machines.

wking · 2019-01-30T22:30:47Z

pkg/types/machinepools.go

@@ -9,6 +9,8 @@ import (
 // MachinePool is a pool of machines to be installed.
 type MachinePool struct {
 	// Name is the name of the machine pool.
+	// For the control plane machine pool, the name will always be "master".
+	// For the compute machine pools, the only valid name is "worker".


It would be nice to lift this restriction. All we're doing for compute nodes is pushing MachineSets which reference an installer-created worker IAM instance profile, right? We can do that regardless of the name we set in the MachineSet.

Yes, this restriction will be lifted. This is just the first step in a larger transition that will include that lifted restriction.

pkg/asset/installconfig/installconfig.go

pkg/typesv1beta1/doc.go

wking · 2019-01-30T22:52:44Z

pkg/asset/machines/worker.go

@@ -88,7 +88,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
 	}

 	ic := installconfig.Config
-	pool := workerPool(ic.Machines)
+	pool := workerPool(ic.Compute)


This can be:

for _, pool := range ic.Compute {

couldn't it?

Ultimately, yes. But baby steps. And the validator only accepts a single compute machine pool named "worker" at the moment anyway.

pkg/tfvars/tfvars.go

pkg/types/validation/installconfig.go

wking · 2019-01-30T23:00:34Z

I am super-excited about this :). CC @dgoodwin, @vrutkovs about the pending install-config schema change.

dgoodwin · 2019-01-31T12:03:52Z

Awesome, and looks like it matches openshift/hive#164 nicely. CC @abutcher

staebler · 2019-01-31T16:42:51Z

regarding fabb911 I think converters must exist in pkg/types as part of the api.

@abhinavdahiya Yes, I agree. I did not want to tackle doing long-term converters as part of this PR. I only wanted to put something in place temporarily so that current users such as hive and byo have a chance to switch over to v1beta2 without breaking. Once people have had a chance to make the switch, all the v1beta1 types and converted will be removed.

wking · 2019-01-31T16:54:25Z

@dgoodwin, @vrutkovs, do you need short-term compat with v1beta1, or can you bump your generators when you bump installer versions?

dgoodwin · 2019-01-31T17:01:46Z

I think we'll be ok, we'll adjust as soon as we vendor this in.

vrutkovs · 2019-01-31T17:33:58Z

@wking our GCP test was scrapped, scaleup test uses AWS cluster, so any option is fine by me

staebler · 2019-01-31T17:44:18Z

OK. If nobody needs the v1beta1 support, then I will remove it. Makes things easier.

staebler · 2019-02-01T18:40:14Z

/retest

staebler · 2019-02-12T16:24:41Z

/hold

derekwaynecarr · 2019-02-12T22:08:56Z

pkg/asset/ignition/machine/master_test.go

-					Replicas: func(x int64) *int64 { return &x }(3),
-				},
+			ControlPlane: &types.MachinePool{
+				Name:     "master",


is there a desire to eliminate the term "master" generally?

just asking as that is a broader change to push through.

Yes. The broader change of replacing the terms master and worker with control-plane and compute are tackled in #1227.

flaper87 · 2019-02-13T08:38:22Z

If you don't mind, I'd like to test this patch with OpenStack before it goes in. I'll wait for the next rebase to test it.

staebler · 2019-02-13T16:17:21Z

If you don't mind, I'd like to test this patch with OpenStack before it goes in. I'll wait for the next rebase to test it.

A test on OpenStack sounds wonderful. The PR has been rebased.

wking · 2019-02-14T19:36:37Z

pkg/types/installconfig.go

-	// Default on AWS and OpenStack is 3 masters and 3 workers.
-	// Default on Libvirt is 1 master and 1 worker.
-	Machines []MachinePool `json:"machines,omitempty"`
+	ControlPlane *MachinePool `json:"controlPlane,omitempty"`


We'll need to bump InstallConfigVersion for this. Possibly also with a temporary hack to get through CI (e.g. like the one being removed by #1251).

I wasn't sure whether the version needed to be bumped with every change. Or whether the version change covered the collections of API changes between beta1 and beta2.

We cut 0.12.0 with v1beta2, so I think we need another bump. But I'm fine combining future changes that land between releases.

Fair enough. I will bump up to v1beta3.

staebler · 2019-02-14T19:48:24Z

/hold cancel

The feature exception for this has been approved.

openshift-ci-robot · 2019-02-14T21:17:14Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-openstack	05cc74b5b542c91a77f78e0f3b09715a36b6c437	link	`/test e2e-openstack`
ci/prow/e2e-libvirt	05cc74b5b542c91a77f78e0f3b09715a36b6c437	link	`/test e2e-libvirt`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

There should always be a control plane machine pool, so the declaration is moved from a generic machines field to a specific controlPlane field. The machines field contains only compute machines, so it is renamed to compute.

The Machines field was replaced with ControlPlane and Compute fields, so the API version of InstallConfig needs to be bumped.

abhinavdahiya · 2019-02-15T17:45:42Z

pkg/types/installconfig.go

-	// Default on AWS and OpenStack is 3 masters and 3 workers.
-	// Default on Libvirt is 1 master and 1 worker.
-	Machines []MachinePool `json:"machines,omitempty"`
+	ControlPlane *MachinePool `json:"controlPlane,omitempty"`


Why is this omitempty? and nilable.

from the commit message There should always be a control plane machine pool

Because the installer can fill in the default.

It is omitempty and nilable because the user does not have to provide it. If it is omitted, then the installer will add default values to the InstallConfig used. The validator will ensure that the field is not nil in the final InstallConfig.

abhinavdahiya · 2019-02-15T17:49:59Z

/lgtm

openshift-ci-robot · 2019-02-15T17:50:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler

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~~ [abhinavdahiya,staebler]

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

… machines. In the cases where the replica count is not customizable, the controlPlane and compute have been removed to use the defaults for the machine pools. Update the apiVersion for install-config.yaml to v1beta3, which is the version that incorporates the controlPlane and compute fields. See openshift/installer#1157

machines is now split into compute and controlPlane See openshift/installer#1157

When we pivoted to v1beta3 in ccdc32e (installconfig: separate control plane and compute machine pools, 2019-01-29, openshift#1157), we dropped support for the old 'machines' JSON property. Instead of silently ignoring that property in v1beta2 configs, error out to avoid surprising users later when they notice us not picking up their machines configuration. Or internal consumers pivoted to v1beta3 in openshift/release@e1d729c6 (Modify install-config.yaml to use controlPlane and compute instead of machines, 2019-02-05, openshift/release#2787) and openshift/hive@3eda6d12 (Bump to installer master branch, 2019-02-21, openshift/hive#228).

After openshift#1157 there is no need to restrict what name is used for the controlPlane Machines or compute MachineSets. By removing this restriction, multiple compute MachineSets can be defined at install time.

openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 30, 2019

openshift-ci-robot requested review from abhinavdahiya and wking January 30, 2019 21:20

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019

abhinavdahiya reviewed Jan 30, 2019

View reviewed changes

pkg/tfvars/tfvars.go Outdated Show resolved Hide resolved