Skip to content

installconfig: separate control plane and compute machine pools #1157

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 2 commits into from
Feb 15, 2019
Merged

installconfig: separate control plane and compute machine pools #1157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ccdc32e
installconfig: separate control plane and compute machine pools
staebler Jan 30, 2019
a159db3
types: bump InstallConfig version to v1beta3
staebler Feb 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/asset/ignition/bootstrap/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (a *Bootstrap) Files() []*asset.File {

// getTemplateData returns the data to use to execute bootstrap templates.
func (a *Bootstrap) getTemplateData(installConfig *types.InstallConfig) (*bootstrapTemplateData, error) {
etcdEndpoints := make([]string, installConfig.MasterCount())
etcdEndpoints := make([]string, *installConfig.ControlPlane.Replicas)
for i := range etcdEndpoints {
etcdEndpoints[i] = fmt.Sprintf("https://%s-etcd-%d.%s:2379", installConfig.ObjectMeta.Name, i, installConfig.BaseDomain)
}
Expand Down
9 changes: 4 additions & 5 deletions pkg/asset/ignition/machine/master_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/installconfig"
Expand All @@ -30,11 +31,9 @@ func TestMasterGenerate(t *testing.T) {
Region: "us-east",
},
},
Machines: []types.MachinePool{
{
Name: "master",
Replicas: func(x int64) *int64 { return &x }(3),
},
ControlPlane: &types.MachinePool{
Name: "master",
Copy link
Member

@derekwaynecarr derekwaynecarr Feb 12, 2019

Choose a reason for hiding this comment

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

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

Copy link
Member

@derekwaynecarr derekwaynecarr Feb 12, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@staebler staebler Feb 13, 2019

Choose a reason for hiding this comment

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

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

Replicas: pointer.Int64Ptr(3),
},
},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/installconfig/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (a *InstallConfig) Generate(parents asset.Parents) error {

a.Config = &types.InstallConfig{
TypeMeta: metav1.TypeMeta{
APIVersion: "v1beta2",
APIVersion: types.InstallConfigVersion,
},
ObjectMeta: metav1.ObjectMeta{
Name: clusterName.ClusterName,
Expand Down
27 changes: 14 additions & 13 deletions pkg/asset/installconfig/installconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"github.com/golang/mock/gomock"
"github.com/stretchr/testify/assert"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"

"github.com/openshift/installer/pkg/asset"
"github.com/openshift/installer/pkg/asset/mock"
Expand Down Expand Up @@ -74,14 +75,14 @@ func TestInstallConfigGenerate_FillsInDefaults(t *testing.T) {
},
},
},
Machines: []types.MachinePool{
{
Name: "master",
Replicas: func(x int64) *int64 { return &x }(3),
},
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: pointer.Int64Ptr(3),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: func(x int64) *int64 { return &x }(3),
Replicas: pointer.Int64Ptr(3),
},
},
Platform: types.Platform{
Expand All @@ -104,7 +105,7 @@ func TestInstallConfigLoad(t *testing.T) {
{
name: "valid InstallConfig",
data: `
apiVersion: v1beta2
apiVersion: v1beta3
metadata:
name: test-cluster
baseDomain: test-domain
Expand Down Expand Up @@ -133,14 +134,14 @@ pullSecret: "{\"auths\":{\"example.com\":{\"auth\":\"authorization value\"}}}"
},
},
},
Machines: []types.MachinePool{
{
Name: "master",
Replicas: func(x int64) *int64 { return &x }(3),
},
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: pointer.Int64Ptr(3),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: func(x int64) *int64 { return &x }(3),
Replicas: pointer.Int64Ptr(3),
},
},
Platform: types.Platform{
Expand Down
18 changes: 4 additions & 14 deletions pkg/asset/machines/master.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ import (
"github.com/openshift/installer/pkg/asset/machines/libvirt"
"github.com/openshift/installer/pkg/asset/machines/openstack"
"github.com/openshift/installer/pkg/asset/rhcos"
"github.com/openshift/installer/pkg/types"
awstypes "github.com/openshift/installer/pkg/types/aws"
libvirttypes "github.com/openshift/installer/pkg/types/libvirt"
nonetypes "github.com/openshift/installer/pkg/types/none"
Expand Down Expand Up @@ -78,7 +77,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
var err error
machines := []machineapi.Machine{}
ic := installconfig.Config
pool := masterPool(ic.Machines)
pool := ic.ControlPlane
switch ic.Platform.Name() {
case awstypes.Name:
mpool := defaultAWSMachinePoolPlatform()
Expand All @@ -93,7 +92,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
mpool.Zones = azs
}
pool.Platform.AWS = &mpool
machines, err = aws.Machines(clusterID.ClusterID, ic, &pool, string(*rhcosImage), "master", "master-user-data")
machines, err = aws.Machines(clusterID.ClusterID, ic, pool, string(*rhcosImage), "master", "master-user-data")
if err != nil {
return errors.Wrap(err, "failed to create master machine objects")
}
Expand All @@ -103,7 +102,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
mpool.Set(ic.Platform.Libvirt.DefaultMachinePlatform)
mpool.Set(pool.Platform.Libvirt)
pool.Platform.Libvirt = &mpool
machines, err = libvirt.Machines(clusterID.ClusterID, ic, &pool, "master", "master-user-data")
machines, err = libvirt.Machines(clusterID.ClusterID, ic, pool, "master", "master-user-data")
if err != nil {
return errors.Wrap(err, "failed to create master machine objects")
}
Expand All @@ -115,7 +114,7 @@ func (m *Master) Generate(dependencies asset.Parents) error {
mpool.Set(pool.Platform.OpenStack)
pool.Platform.OpenStack = &mpool

machines, err = openstack.Machines(clusterID.ClusterID, ic, &pool, string(*rhcosImage), "master", "master-user-data")
machines, err = openstack.Machines(clusterID.ClusterID, ic, pool, string(*rhcosImage), "master", "master-user-data")
if err != nil {
return errors.Wrap(err, "failed to create master machine objects")
}
Expand Down Expand Up @@ -230,12 +229,3 @@ func (m *Master) StructuredMachines() ([]machineapi.Machine, error) {

return machines, nil
}

func masterPool(pools []types.MachinePool) types.MachinePool {
for idx, pool := range pools {
if pool.Name == "master" {
return pools[idx]
}
}
return types.MachinePool{}
}
2 changes: 1 addition & 1 deletion pkg/asset/machines/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func (w *Worker) Generate(dependencies asset.Parents) error {
}

ic := installconfig.Config
pool := workerPool(ic.Machines)
pool := workerPool(ic.Compute)
Copy link
Member

@wking wking Jan 30, 2019

Choose a reason for hiding this comment

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

This can be:

for _, pool := range ic.Compute {

couldn't it?

Copy link
Contributor Author

@staebler staebler Jan 31, 2019

Choose a reason for hiding this comment

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

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

switch ic.Platform.Name() {
case awstypes.Name:
mpool := defaultAWSMachinePoolPlatform()
Expand Down
2 changes: 1 addition & 1 deletion pkg/asset/manifests/operators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func (m *Manifests) generateBootKubeManifests(dependencies asset.Parents) []*ass
rootCA,
)

etcdEndpointHostnames := make([]string, installConfig.Config.MasterCount())
etcdEndpointHostnames := make([]string, *installConfig.Config.ControlPlane.Replicas)
for i := range etcdEndpointHostnames {
etcdEndpointHostnames[i] = fmt.Sprintf("%s-etcd-%d", installConfig.Config.ObjectMeta.Name, i)
}
Expand Down
43 changes: 15 additions & 28 deletions pkg/types/defaults/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
libvirtdefaults "github.com/openshift/installer/pkg/types/libvirt/defaults"
nonedefaults "github.com/openshift/installer/pkg/types/none/defaults"
openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults"
"k8s.io/utils/pointer"
)

var (
Expand Down Expand Up @@ -43,39 +42,27 @@ func SetInstallConfigDefaults(c *types.InstallConfig) {
},
}
}
numberOfMasters := int64(3)
numberOfWorkers := int64(3)
defaultReplicaCount := int64(3)
if c.Platform.Libvirt != nil {
numberOfMasters = 1
numberOfWorkers = 1
defaultReplicaCount = 1
}
if len(c.Machines) == 0 {
c.Machines = []types.MachinePool{
{
Name: "master",
Replicas: &numberOfMasters,
},
if c.ControlPlane == nil {
c.ControlPlane = &types.MachinePool{
Replicas: &defaultReplicaCount,
}
}
c.ControlPlane.Name = "master"
if len(c.Compute) == 0 {
c.Compute = []types.MachinePool{
{
Name: "worker",
Replicas: &numberOfWorkers,
Replicas: &defaultReplicaCount,
},
}
} else {
for i := range c.Machines {
switch c.Machines[i].Name {
case "master":
if c.Machines[i].Replicas == nil {
c.Machines[i].Replicas = &numberOfMasters
}
case "worker":
if c.Machines[i].Replicas == nil {
c.Machines[i].Replicas = &numberOfWorkers
}
default:
if c.Machines[i].Replicas == nil {
c.Machines[i].Replicas = pointer.Int64Ptr(0)
}
}
}
for i, p := range c.Compute {
if p.Replicas == nil {
c.Compute[i].Replicas = &defaultReplicaCount
Copy link
Member

@wking wking Jan 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@staebler staebler Jan 31, 2019

Choose a reason for hiding this comment

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

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?

}
}
switch {
Expand Down
34 changes: 17 additions & 17 deletions pkg/types/defaults/installconfig_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"testing"

"github.com/stretchr/testify/assert"
"k8s.io/utils/pointer"

"github.com/openshift/installer/pkg/ipnet"
"github.com/openshift/installer/pkg/types"
Expand All @@ -15,7 +16,6 @@ import (
nonedefaults "github.com/openshift/installer/pkg/types/none/defaults"
"github.com/openshift/installer/pkg/types/openstack"
openstackdefaults "github.com/openshift/installer/pkg/types/openstack/defaults"
"k8s.io/utils/pointer"
)

func defaultInstallConfig() *types.InstallConfig {
Expand All @@ -31,14 +31,14 @@ func defaultInstallConfig() *types.InstallConfig {
},
},
},
Machines: []types.MachinePool{
{
Name: "master",
Replicas: func(x int64) *int64 { return &x }(3),
},
ControlPlane: &types.MachinePool{
Name: "master",
Replicas: pointer.Int64Ptr(3),
},
Compute: []types.MachinePool{
{
Name: "worker",
Replicas: func(x int64) *int64 { return &x }(3),
Replicas: pointer.Int64Ptr(3),
},
},
}
Expand All @@ -56,10 +56,8 @@ func defaultLibvirtInstallConfig() *types.InstallConfig {
c.Networking.MachineCIDR = libvirtdefaults.DefaultMachineCIDR
c.Platform.Libvirt = &libvirt.Platform{}
libvirtdefaults.SetPlatformDefaults(c.Platform.Libvirt)
for i, m := range c.Machines {
m.Replicas = func(x int64) *int64 { return &x }(1)
c.Machines[i] = m
}
c.ControlPlane.Replicas = pointer.Int64Ptr(1)
c.Compute[0].Replicas = pointer.Int64Ptr(1)
return c
}

Expand Down Expand Up @@ -172,16 +170,18 @@ func TestSetInstallConfigDefaults(t *testing.T) {
}(),
},
{
name: "Machines present",
name: "Compute present",
config: &types.InstallConfig{
Machines: []types.MachinePool{{Name: "test-machine"}},
Compute: []types.MachinePool{{Name: "test-compute"}},
},
expected: func() *types.InstallConfig {
c := defaultInstallConfig()
c.Machines = []types.MachinePool{{
Name: "test-machine",
Replicas: pointer.Int64Ptr(0),
}}
c.Compute = []types.MachinePool{
{
Name: "test-compute",
Replicas: pointer.Int64Ptr(3),
},
}
return c
}(),
},
Expand Down
24 changes: 8 additions & 16 deletions pkg/types/installconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

const (
// InstallConfigVersion is the version supported by this package.
InstallConfigVersion = "v1beta2"
InstallConfigVersion = "v1beta3"
)

var (
Expand Down Expand Up @@ -47,11 +47,14 @@ type InstallConfig struct {
// Networking defines the pod network provider in the cluster.
*Networking `json:"networking,omitempty"`

// Machines is the list of MachinePools that need to be installed.
// ControlPlane is the configuration for the machines that comprise the
// control plane.
// +optional
// 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"`
Copy link
Member

@wking wking Feb 14, 2019

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

@staebler staebler Feb 14, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Member

@wking wking Feb 14, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@staebler staebler Feb 14, 2019

Choose a reason for hiding this comment

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

Fair enough. I will bump up to v1beta3.

Copy link
Contributor

@abhinavdahiya abhinavdahiya Feb 15, 2019

Choose a reason for hiding this comment

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

Why is this omitempty? and nilable.

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

Copy link
Member

@wking wking Feb 15, 2019

Choose a reason for hiding this comment

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

Because the installer can fill in the default.

Copy link
Contributor Author

@staebler staebler Feb 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@abhinavdahiya abhinavdahiya Feb 15, 2019

Choose a reason for hiding this comment

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

😕 sure


// Compute is the list of compute MachinePools that need to be installed.
// +optional
Compute []MachinePool `json:"compute,omitempty"`

// Platform is the configuration for the specific platform upon which to
// perform the installation.
Expand All @@ -61,17 +64,6 @@ type InstallConfig struct {
PullSecret string `json:"pullSecret"`
}

// MasterCount returns the number of replicas in the master machine pool,
// defaulting to one if no machine pool was found.
func (c *InstallConfig) MasterCount() int {
for _, m := range c.Machines {
if m.Name == "master" && m.Replicas != nil {
return int(*m.Replicas)
}
}
return 1
}

// Platform is the configuration for the specific platform upon which to perform
// the installation. Only one of the platform configuration should be set.
type Platform struct {
Expand Down
2 changes: 2 additions & 0 deletions pkg/types/machinepools.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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".
Copy link
Member

@wking wking Jan 30, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@staebler staebler Jan 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@staebler staebler Jan 31, 2019

Choose a reason for hiding this comment

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

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.

// For the compute machine pools, the only valid name is "worker".
Copy link
Member

@wking wking Jan 30, 2019

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@staebler staebler Jan 31, 2019

Choose a reason for hiding this comment

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

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

Name string `json:"name"`

// Replicas is the count of machines for this machine pool.
Expand Down
Loading