Handling configurable machine setup/installation #664

kcoronado · 2018-04-03T21:47:41Z

What this PR does / why we need it:
This PR modifies the GCE actuator to handle configurable Kubernetes versions, OS image, and startup scripts when creating clusters and starting up machines. It creates a new machine setup configmap that contains the different configurations that the GCE actuator can handle.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
#637

Special notes for your reviewer:

Release note:

@kubernetes/kube-deploy-reviewers

(work in progress)

kcoronado · 2018-04-03T21:58:40Z

/assign @krousey @jessicaochen

kcoronado · 2018-04-03T22:03:32Z

/cc @rsdcastro @karan @medinatiger @k4leung4

k8s-ci-robot · 2018-04-03T22:03:35Z

@kcoronado: GitHub didn't allow me to request PR reviews from the following users: k4leung4.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @rsdcastro @karan @medinatiger @k4leung4

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.

jessicaochen · 2018-04-04T16:53:48Z

/ok-to-test

krousey · 2018-04-04T21:00:18Z

cluster-api/cloud/google/machineactuator.go

@@ -104,6 +106,14 @@ func NewMachineActuator(kubeadmToken string, machineClient client.MachineInterfa
 		}
 	}

+	var configWatch *machinesetup.ConfigWatch
+	if configListPath != "" {


I don't think this check is necessary. machinesetup.NewConfigWatch should return an error if this is not a valid path to a file and this function should error-out approrpiately.

Create is the only command that has to pass a config list path, and then add and delete pass an empty string to the method. I put this check here to handle the empty string from add and delete, and create has a check to prevent an empty string. NewConfigWatch() checks that the path is valid. I can move the empty string check to NewConfigWatch() so it just returns nil (but no error) if it gets an empty string if that makes more sense.

Fair enough. Can we add TODOs to remove this when we switch to the new bootstrapping method. In that setup, we will be running a full-blown controller that will always need a config path.

So would add/delete also need to pass the config path when we switch over (and I should add TODOs there too)?

krousey · 2018-04-04T21:00:28Z

cluster-api/cloud/google/cmd/generate-image/main.go

@@ -42,6 +42,7 @@ var generateCmd = &cobra.Command{
 	},
 }

+// TODO(kcoronado): take a script file as a parameter instead of generating the preloaded script from the template.


Remove TODO?

krousey · 2018-04-04T21:01:06Z

cluster-api/cloud/google/machineactuator.go

@@ -153,20 +164,41 @@ func (gce *GCEClient) Create(cluster *clusterv1.Cluster, machine *clusterv1.Mach
 		return errors.New("invalid master configuration: missing Machine.Spec.Versions.Kubelet")
 	}

-	image, preloaded := gce.getImage(machine, config)
+	if gce.configWatch == nil {


Should this every really happen?

No, the checks in the create command and NewMachineActuator() should prevent this. I'll get rid of it.

krousey · 2018-04-04T21:15:05Z

cluster-api/cloud/google/machinesetup/config_types.go

+)
+
+type ConfigWatch struct {
+	Path string


I don't think this field should be public.

krousey · 2018-04-04T21:16:58Z

cluster-api/cloud/google/machineactuator.go

-				Cluster:   cluster,
-				Machine:   machine,
-				Preloaded: preloaded,
+			metadataParams{


Can we switch to just passing arguments directly as parameters. I think it would be better for the function to be explicit about which fields of this struct it's going to need.

I agree, I switched it.

krousey · 2018-04-04T21:21:16Z

cluster-api/cloud/google/machineactuator.go


 	// A full image path must match the regex format. If it doesn't, we'll assume it's just the image name and try to get it.
 	// If that doesn't work, we will fall back to a default base image.
 	matches := regexp.MustCompile("projects/(.+)/global/images/(family/)*(.+)").FindStringSubmatch(img)
 	if matches == nil {
-		// Only the image name was specified in config, so check if it is preloaded in the project specified in config.
+		// Only the image name was specified in config, so check if it exists in the project specified in config.


I think we can say that image path needs to be fully specified in the config now. And we can drop the project parameter as it's only needed for when the full path wasn't specified.

krousey · 2018-04-04T21:26:22Z

cluster-api/cloud/google/machinesetup/config_types.go

+	clustercommon "k8s.io/kube-deploy/cluster-api/pkg/apis/cluster/common"
+	clusterv1 "k8s.io/kube-deploy/cluster-api/pkg/apis/cluster/v1alpha1"
+	"k8s.io/kube-deploy/cluster-api/util"
+	"os"


These imports should be grouped differently. See https://github.com/golang/go/wiki/CodeReviewComments#imports

krousey · 2018-04-04T21:27:05Z

cluster-api/cloud/google/machinesetup/config_types.go

+	// map to the given Image and Metadata.
+	Params []ConfigParams `json:"machineParams"`
+
+	// This can either be a full projects path to an image/family,


Let's just have this be full path.

krousey · 2018-04-04T21:33:31Z

cluster-api/cloud/google/machinesetup/config_types.go

@@ -0,0 +1,118 @@
+package machinesetup


Needs copyright blurb

krousey · 2018-04-04T21:33:42Z

cluster-api/cloud/google/machinesetup/config_types_test.go

@@ -0,0 +1,243 @@
+package machinesetup


Needs copyright blurb

krousey · 2018-04-04T21:34:43Z

cluster-api/cloud/google/metadata.go

@@ -0,0 +1,117 @@
+/*
+Copyright 2017 The Kubernetes Authors.


https://i.imgur.com/RISobZt.png

krousey · 2018-04-04T21:35:57Z

cluster-api/cloud/google/metadata.go

+
+KUBELET_VERSION={{ .Machine.Spec.Versions.Kubelet }}
+export VERSION=v${KUBELET_VERSION}
+export ARCH=amd64


These two seem unnecessary. If the startup script needs them, it could be there.

krousey · 2018-04-04T21:37:07Z

cluster-api/gcp-deployer/machine_setup_configs.yaml

+      set -x
+
+      (
+        apt-get update


spacing nit

krousey · 2018-04-04T21:41:24Z

Also, @maisem, could you tell @kcoronado how to regenerate the provider config code for this change?

jessicaochen · 2018-04-04T21:13:33Z

cluster-api/cloud/google/machinesetup/config_types.go

+			}
+			foundRoles := true
+			for _, role := range params.Roles {
+				if !util.RoleContains(role, validParams.Roles) {


This ensures the matched config has at least all the roles in the param. However, it still matches even if the config has additional roles the param did not have.

Perhaps cover this in your unit tests?

I added a case to the test for it.

I probably should be more clear. Given my understanding of the design, this is probably a bug. As per the design, the config and the params should have the exact same roles. This is a case where there are different roles and yet the match still happens.

I originally planned to allow the configs to have more roles than the params because I intended for the config to support any combination of roles that it lists there. It would consolidate the configs so if you had multiple roles that can all use the same script, you wouldn't need a config for each combination. But I thought about it some more and it's kind of hard to reason about this situation since we only have a master and node role. I switched it to ensure it's an exact match to avoid confusion.

How would this work is validParams.Roles was Role1, Role2 and params.Roles was Role1, Role1? There should be validation against this at higher levels, but possibly also guard against this here.

I modified the check to account for this and added another case to the test.

jessicaochen · 2018-04-04T21:21:47Z

cluster-api/cloud/google/metadata.go

+	params.PodCIDR = getSubnet(params.Cluster.Spec.ClusterNetwork.Pods)
+	params.ServiceCIDR = getSubnet(params.Cluster.Spec.ClusterNetwork.Services)
+	params.MasterEndpoint = getEndpoint(&params.Cluster.Status.APIEndpoints[0])
+


(nit)
Perhaps the getEndpoint method should take a clusterv1.APIEndpoint instead of a *clusterv1.APIEndpoint?

jessicaochen · 2018-04-04T21:27:31Z

cluster-api/gcp-deployer/cmd/create.go


 	return d.CreateCluster(cluster, machines)
 }
 func init() {
 	createCmd.Flags().StringVarP(&co.Cluster, "cluster", "c", "", "cluster yaml file")
 	createCmd.Flags().StringVarP(&co.Machine, "machines", "m", "", "machine yaml file")
+	createCmd.Flags().StringVarP(&co.MachineSetup, "setup", "s", "", "machine setup configs yaml file")


(nit)
I would keep it as machinesetup or machinessetupconfig. setup by itself is a bit confusing.

jessicaochen · 2018-04-04T21:47:13Z

cluster-api/gcp-deployer/cmd/create.go


 	return d.CreateCluster(cluster, machines)
 }
 func init() {
 	createCmd.Flags().StringVarP(&co.Cluster, "cluster", "c", "", "cluster yaml file")
 	createCmd.Flags().StringVarP(&co.Machine, "machines", "m", "", "machine yaml file")
+	createCmd.Flags().StringVarP(&co.MachineSetup, "setup", "s", "", "machine setup configs yaml file")


Can we default to machine_setup_configs.yaml ?

We can do that, but there's no default for --machines or --cluster. Is that ok?

k8s-ci-robot · 2018-04-05T17:57:01Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kcoronado
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jessicaochen

Assign the PR to them by writing /assign @jessicaochen in a comment when ready.

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

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

kcoronado

Thanks for the review, I addressed all the comments.

kcoronado · 2018-04-04T23:34:39Z

cluster-api/cloud/google/machineactuator.go

@@ -153,20 +164,41 @@ func (gce *GCEClient) Create(cluster *clusterv1.Cluster, machine *clusterv1.Mach
 		return errors.New("invalid master configuration: missing Machine.Spec.Versions.Kubelet")
 	}

-	image, preloaded := gce.getImage(machine, config)
+	if gce.configWatch == nil {


No, the checks in the create command and NewMachineActuator() should prevent this. I'll get rid of it.

kcoronado · 2018-04-05T00:05:30Z

cluster-api/cloud/google/machineactuator.go

-				Cluster:   cluster,
-				Machine:   machine,
-				Preloaded: preloaded,
+			metadataParams{


I agree, I switched it.

kcoronado · 2018-04-05T17:32:09Z

cluster-api/cloud/google/machinesetup/config_types.go

+			}
+			foundRoles := true
+			for _, role := range params.Roles {
+				if !util.RoleContains(role, validParams.Roles) {


I added a case to the test for it.

kcoronado · 2018-04-05T17:35:14Z

cluster-api/gcp-deployer/cmd/create.go


 	return d.CreateCluster(cluster, machines)
 }
 func init() {
 	createCmd.Flags().StringVarP(&co.Cluster, "cluster", "c", "", "cluster yaml file")
 	createCmd.Flags().StringVarP(&co.Machine, "machines", "m", "", "machine yaml file")
+	createCmd.Flags().StringVarP(&co.MachineSetup, "setup", "s", "", "machine setup configs yaml file")


We can do that, but there's no default for --machines or --cluster. Is that ok?

kcoronado · 2018-04-05T17:47:20Z

cluster-api/cloud/google/machineactuator.go

@@ -104,6 +106,14 @@ func NewMachineActuator(kubeadmToken string, machineClient client.MachineInterfa
 		}
 	}

+	var configWatch *machinesetup.ConfigWatch
+	if configListPath != "" {


So would add/delete also need to pass the config path when we switch over (and I should add TODOs there too)?

krousey · 2018-04-06T16:18:02Z

cluster-api/cloud/google/metadata.go

+
+KUBELET_VERSION={{ .Machine.Spec.Versions.Kubelet }}
+VERSION=v${KUBELET_VERSION}
+ARCH=amd64


I'm not sure that ARCH should be in this. This probably belongs in the install scripts that need it.

I moved it to the scripts.

jessicaochen · 2018-04-06T17:18:46Z

cluster-api/cloud/google/machinesetup/config_types.go

+			}
+			foundRoles := true
+			for _, role := range params.Roles {
+				if !util.RoleContains(role, validParams.Roles) {


I probably should be more clear. Given my understanding of the design, this is probably a bug. As per the design, the config and the params should have the exact same roles. This is a case where there are different roles and yet the match still happens.

krousey · 2018-04-10T20:58:27Z

cluster-api/cloud/google/machinesetup/config_types.go

+			}
+			foundRoles := true
+			for _, role := range params.Roles {
+				if !util.RoleContains(role, validParams.Roles) {


How would this work is validParams.Roles was Role1, Role2 and params.Roles was Role1, Role1? There should be validation against this at higher levels, but possibly also guard against this here.

jessicaochen · 2018-04-11T21:13:23Z

cluster-api/cloud/google/machinesetup/config_types.go

+
+func rolesToMap(roles []clustercommon.MachineRole) map[clustercommon.MachineRole]int {
+	rolesMap := map[clustercommon.MachineRole]int{}
+	for _, role := range roles {


Interesting. Are we expecting multiple of the same role specified? Seems like a bug to have the same role multiple times. Perhaps we should have a check for it (not in this PR) in the apiserver.

Yeah it would be a bug or user error if there are duplicate roles. I asked Kris about it yesterday and he said we can add a check for it elsewhere later on.

I'm curious why roles was made a slice in the first place. @krousey

Others expressed interest in having more than Node and Master roles and also having machines serve multiple roles.

jessicaochen · 2018-04-11T21:19:28Z

cluster-api/cloud/google/machinesetup/config_types.go

+			if params.Versions != validParams.Versions {
+				continue
+			}
+			return &conf, nil


This returns the first match. What is the plan for making sure there are not multiple matches available in the config?

Would it be reasonable to check through all of them and return an error if multiple matches are found? And just print out the params so the user can find where the duplicates are in their configs.

Sounds Good.

I updated the check and added a test case for it.

karan

A few nits among other things.

karan · 2018-04-12T18:13:20Z

cluster-api/cloud/google/config/configtemplate.go

@@ -147,6 +149,7 @@ spec:
        args:
        - --kubeconfig=/etc/kubernetes/admin.conf
        - --token={{ .Token }}
+        - --config=/etc/machinesetup/machine_setup_configs.yaml


Call it this something other than config please. There are way too many things named config. Be specific here.

I named it machinesetup to be consistent with the flag in create.go.

karan · 2018-04-12T18:14:08Z

cluster-api/cloud/google/gceproviderconfig/types.go

@@ -27,5 +27,5 @@ type GCEProviderConfig struct {
 	Project     string `json:"project"`
 	Zone        string `json:"zone"`
 	MachineType string `json:"machineType"`
-	Image       string `json:"image"`
+	OS          string `json:"os"`


Is this OS image or OS name?

It should be the name, it gets mapped to the image in the machine setup configs. I added a comment.

karan · 2018-04-12T18:14:14Z

cluster-api/cloud/google/gceproviderconfig/v1alpha1/types.go

@@ -27,5 +27,5 @@ type GCEProviderConfig struct {
 	Project     string `json:"project"`
 	Zone        string `json:"zone"`
 	MachineType string `json:"machineType"`
-	Image       string `json:"image"`
+	OS          string `json:"os"`


Same as above.

karan · 2018-04-12T18:14:40Z

cluster-api/cloud/google/machineactuator.go

@@ -52,6 +56,8 @@ const (

 	UIDLabelKey       = "machine-crd-uid"
 	BootstrapLabelKey = "boostrap"
+
+	MachineSetupConfigsFilename = "machine_setup_configs.yaml"


Please add a comment about what this file contains and in what format.

karan · 2018-04-12T18:19:23Z

cluster-api/cloud/google/machineactuator.go

@@ -653,42 +687,29 @@ func (gce *GCEClient) handleMachineError(machine *clusterv1.Machine, err *apierr
 	return err
 }

-func (gce *GCEClient) getImage(machine *clusterv1.Machine, config *gceconfig.GCEProviderConfig) (image string, isPreloaded bool) {
+func (gce *GCEClient) getImagePath(img string) (imagePath string) {


This is a great method for testing with unit tests.

I agree, but it makes a call to the GCE API to check that the image is there and I was having trouble figuring out how to mock it (otherwise I would have added the test when I refactored this method in a previous PR). Rob is working on a PR (#688) to set up a fake client so I can write a unit test for this method once that gets merged.

karan · 2018-04-12T18:21:08Z

cluster-api/cloud/google/machinesetup/config_types.go

+	clusterv1 "k8s.io/kube-deploy/cluster-api/pkg/apis/cluster/v1alpha1"
+)
+
+type ConfigWatch struct {


Document this please

karan · 2018-04-12T18:21:12Z

cluster-api/cloud/google/machinesetup/config_types.go

+	path string
+}
+
+type ValidConfigs struct {


Document this please

karan · 2018-04-12T18:25:00Z

cluster-api/cloud/google/machinesetup/config_types.go

+	// map to the given Image and Metadata.
+	Params []ConfigParams `json:"machineParams"`
+
+	// The fully specified image path.


Does that include https://www.googleapis.com/compute/beta/projects/ubuntu-os-cloud/global/images/ubuntu-1604-xenial along with /projects/ubuntu-os-cloud/global/images/ubuntu-1604-xenial-v20180405?

It's whatever will be found by the Google cloud images.get or images.getFromFamily API calls, so I think it would have to be ubuntu-1604-xenial-v20180405. I added a comment with examples. If the image can't be found in the project, it will default to projects/ubuntu-os-cloud/global/images/family/ubuntu-1710 (but that has to be changed to ubuntu-1604-lts in a later PR).

karan · 2018-04-12T18:26:42Z

cluster-api/cloud/google/machinesetup/config_types.go

+
+func rolesToMap(roles []clustercommon.MachineRole) map[clustercommon.MachineRole]int {
+	rolesMap := map[clustercommon.MachineRole]int{}
+	for _, role := range roles {


I'm curious why roles was made a slice in the first place. @krousey

jessicaochen · 2018-04-12T22:05:16Z

cluster-api/cloud/google/machinesetup/config_types.go

+			if params.Versions != validParams.Versions {
+				continue
+			}
+			return &conf, nil


Sounds Good.

krousey

Other than the documentation additions requested, this looks good to me.

krousey · 2018-04-13T14:46:59Z

cluster-api/cloud/google/machinesetup/config_types.go

+
+func rolesToMap(roles []clustercommon.MachineRole) map[clustercommon.MachineRole]int {
+	rolesMap := map[clustercommon.MachineRole]int{}
+	for _, role := range roles {


Others expressed interest in having more than Node and Master roles and also having machines serve multiple roles.

karan · 2018-04-16T14:48:40Z

cluster-api/gcp-deployer/machine_setup_configs.yaml

@@ -0,0 +1,379 @@
+items:


I'm curious why this file isn't defined as a ConfigMap but just a YAML list of things? To someone just browsing the code, it would not be obvious that these key=value pairs are consumed as a ConfigMap is the cluster.

I clarified a few things with Kris offline. So in this case, the configmap is just the mechanism for getting the yaml file mounted into the pod as a volume, and the controller consumes the yaml file, not the configmap object. We don't want to work with the actual configmap API object in case the machine controller is ever run on a non-kubernetes cluster. If we changed the yaml file to be configmap keys and values where the key is a string constructed from the defining params, each of the keys would become a separate file and we'd have to pull down all the files from the directory and key off of a filename, so it's easier to just work with the single file.

If someone did want to edit the machine setup configs yaml while a cluster is running, they would do it through the machine-setup configmap, so in that sense it's not obvious that that's how you'd update it. I can update the readme with documentation on what the yaml file is and how you'd go about updating the yaml in a running cluster.

We don't want to work with the actual configmap API object in case the machine controller is ever run on a non-kubernetes cluster

Are we supporting that use case? When can the controller run if not in a k8s cluster (even bootstrapping will happen in a K8s cluster.

it's easier to just work with the single file.

That's fine. We can have everything in one file still. That's not my concern. My concern is that we are hiding this from the user and it's not obvious to them how to update these values. If this was using the ConfigMap object itself, they could simply kubectl apply it rather than mucking with documentation.

So to summarize - I'm not saying we need to split things, but rather the abstraction of ConfigMap be pulled up in this spec file rather than deep in code somewhere.

When can the controller run if not in a k8s cluster (even bootstrapping will happen in a K8s cluster.

Right now the gcp deployer is bootstrapping outside of a k8s cluster. In the future when we have real bootstrapping we can switch over to setting up a configmap from the start, but the machinesetup library is set up to handle a yaml file. If the APIServer ever goes down, we don't want to rely on the configmap object and instead work with the volume mounted yaml file.

Changing the yaml to a configmap definition would add a lot of extra logic to the gcp deployer since it would have to serialize and write out the contents to a file so the library can consume it. The gcp deployer is also just temporary until we have actual bootstrapping set up, so I don't think it's worth adding all the extra logic when it'll be replaced anyway.

it's not obvious to them how to update these values. If this was using the ConfigMap object itself, they could simply kubectl apply it rather than mucking with documentation.

Once the cluster is created and running, they can still use kubectl apply to update the configmap. If they want to edit the values before creating the cluster, they'd edit the yaml file the same way they can edit the machines or cluster yaml. Again, we can move to transparently setting up configmap in the future with real bootstrapping and it would be more obvious how to update, but I think for now just documenting what I said above would be enough.

I disagree about the extra logic concern you have. Currently the matching logic and the mounting logic are complex and opaque to the user. If anything, it will simplify the configmap a whole lot more. But I don't want to block this PR on that.

Please add your explanation as a comment and a TODO with an issue as a future improvement.

kcoronado

I addressed all the comments except for the unit tests for getImagePath() which is currently blocked.

kcoronado · 2018-04-12T21:47:12Z

cluster-api/cloud/google/config/configtemplate.go

@@ -147,6 +149,7 @@ spec:
        args:
        - --kubeconfig=/etc/kubernetes/admin.conf
        - --token={{ .Token }}
+        - --config=/etc/machinesetup/machine_setup_configs.yaml


I named it machinesetup to be consistent with the flag in create.go.

kcoronado · 2018-04-12T21:52:15Z

cluster-api/cloud/google/gceproviderconfig/types.go

@@ -27,5 +27,5 @@ type GCEProviderConfig struct {
 	Project     string `json:"project"`
 	Zone        string `json:"zone"`
 	MachineType string `json:"machineType"`
-	Image       string `json:"image"`
+	OS          string `json:"os"`


It should be the name, it gets mapped to the image in the machine setup configs. I added a comment.

kcoronado · 2018-04-13T18:18:20Z

cluster-api/cloud/google/machinesetup/config_types.go

+	// map to the given Image and Metadata.
+	Params []ConfigParams `json:"machineParams"`
+
+	// The fully specified image path.


It's whatever will be found by the Google cloud images.get or images.getFromFamily API calls, so I think it would have to be ubuntu-1604-xenial-v20180405. I added a comment with examples. If the image can't be found in the project, it will default to projects/ubuntu-os-cloud/global/images/family/ubuntu-1710 (but that has to be changed to ubuntu-1604-lts in a later PR).

kcoronado · 2018-04-13T22:43:31Z

cluster-api/cloud/google/machinesetup/config_types.go

+			if params.Versions != validParams.Versions {
+				continue
+			}
+			return &conf, nil


I updated the check and added a test case for it.

kcoronado · 2018-04-13T23:03:16Z

cluster-api/cloud/google/machineactuator.go

@@ -653,42 +687,29 @@ func (gce *GCEClient) handleMachineError(machine *clusterv1.Machine, err *apierr
 	return err
 }

-func (gce *GCEClient) getImage(machine *clusterv1.Machine, config *gceconfig.GCEProviderConfig) (image string, isPreloaded bool) {
+func (gce *GCEClient) getImagePath(img string) (imagePath string) {


I agree, but it makes a call to the GCE API to check that the image is there and I was having trouble figuring out how to mock it (otherwise I would have added the test when I refactored this method in a previous PR). Rob is working on a PR (#688) to set up a fake client so I can write a unit test for this method once that gets merged.

kcoronado · 2018-04-16T18:10:48Z

cluster-api/gcp-deployer/machine_setup_configs.yaml

@@ -0,0 +1,379 @@
+items:


I clarified a few things with Kris offline. So in this case, the configmap is just the mechanism for getting the yaml file mounted into the pod as a volume, and the controller consumes the yaml file, not the configmap object. We don't want to work with the actual configmap API object in case the machine controller is ever run on a non-kubernetes cluster. If we changed the yaml file to be configmap keys and values where the key is a string constructed from the defining params, each of the keys would become a separate file and we'd have to pull down all the files from the directory and key off of a filename, so it's easier to just work with the single file.

If someone did want to edit the machine setup configs yaml while a cluster is running, they would do it through the machine-setup configmap, so in that sense it's not obvious that that's how you'd update it. I can update the readme with documentation on what the yaml file is and how you'd go about updating the yaml in a running cluster.

jessicaochen

lgtm

jessicaochen · 2018-04-17T18:11:15Z

@krousey - Why does this change need migration. It seems to only touch cloud specific stuff (ie. stuff under cloud and the deployer)

kcoronado added 4 commits April 2, 2018 15:41

Rename GCEProviderConfig.Image field to OS

5cd6e74

Replace hardcoded startup script with machine setup configs.

6bc01dd

(work in progress)

Remove call to preloaded script in generate-image

242d193

Create/mount configmap for machine setup configs

9b9a8b9

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 3, 2018

kcoronado changed the title ~~Installation~~ Handling configurable machine setup/installation Apr 3, 2018

k8s-ci-robot assigned jessicaochen and krousey Apr 3, 2018

k8s-ci-robot requested review from rsdcastro, karan and medinatiger April 3, 2018 22:03

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 4, 2018

krousey reviewed Apr 4, 2018

View reviewed changes

krousey suggested changes Apr 4, 2018

View reviewed changes

jessicaochen suggested changes Apr 4, 2018

View reviewed changes

kcoronado added 6 commits April 4, 2018 16:43

fixup 6bc01dd Remove nil check for ConfigWatch

bd5d0ec

fixup 6bc01dd fix metadata method parameters

a2782a1

fixup 6bc01dd must use full image path in configs

dfc5f96

fixup 6bc01dd fix config_types.go

9c5614b

fixup 6bc01dd fix config_types_test.go

88ceb5d

fixup 9b9a8b9 fix config_types_test.go

c79b5a0

kcoronado added 3 commits April 5, 2018 10:41

fixup 6bc01dd fix create flag and default value

7e00e5b

fixup 6bc01dd fix spacing in configs file

1cc6a4d

fixup 6bc01dd add TODO

078e7e8

kcoronado commented Apr 5, 2018

View reviewed changes

fixup 6bc01dd fix env vars template bug

5c450a0

krousey reviewed Apr 6, 2018

View reviewed changes

jessicaochen suggested changes Apr 6, 2018

View reviewed changes

kcoronado added 2 commits April 10, 2018 11:09

fixup 6bc01dd roles must be exact match

e1d2b28

fixup 6bc01dd move ARCH env var to scripts

6eea28e

krousey suggested changes Apr 10, 2018

View reviewed changes

fixup 6bc01dd fix role matching check

66a419f

jessicaochen suggested changes Apr 11, 2018

View reviewed changes

karan reviewed Apr 12, 2018

View reviewed changes

fixup 6bc01dd rename gce-machine-controller flag

e829e94

jessicaochen suggested changes Apr 12, 2018

View reviewed changes

krousey approved these changes Apr 13, 2018

View reviewed changes

kcoronado added 2 commits April 13, 2018 11:29

fixup 6bc01dd add comments/documentation

c124da1

fixup 6bc01dd return error if >1 config match

c1c88ef

karan reviewed Apr 16, 2018

View reviewed changes

kcoronado commented Apr 16, 2018

View reviewed changes

krousey added the do-not-merge/needs-migration label Apr 16, 2018

jessicaochen approved these changes Apr 17, 2018

View reviewed changes

fixup 9b9a8b9 add TODO for improving configmap visibility

1cfe66b

rsdcastro mentioned this pull request Apr 19, 2018

Cloud Provider implementation for AWS kubernetes-sigs/cluster-api#59

Closed

kcoronado mentioned this pull request Apr 19, 2018

Handling configurable machine setup/installation kubernetes-sigs/cluster-api#104

Merged

kcoronado closed this Apr 24, 2018

Handling configurable machine setup/installation #664

Handling configurable machine setup/installation #664

Conversation

kcoronado commented Apr 3, 2018

kcoronado commented Apr 3, 2018

kcoronado commented Apr 3, 2018

k8s-ci-robot commented Apr 3, 2018

jessicaochen commented Apr 4, 2018

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krousey commented Apr 4, 2018

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

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

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 5, 2018

kcoronado left a 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

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

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

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

karan left a 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

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

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

Choose a reason for hiding this comment

krousey left a 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

Choose a reason for hiding this comment