Handling configurable machine setup/installation #104

kcoronado · 2018-04-19T17:03:19Z

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):
#85

Special notes for your reviewer:
This PR was migrated from kubernetes-retired/kube-deploy#664.

Release note:

NONE

@kubernetes/kube-deploy-reviewers

- Rename GCEProviderConfig.Image field to OS - Remove call to preloaded script in generate-image - Create a machine-setup ConfigMap to mount the yaml file

k8s-ci-robot · 2018-04-19T17:03:33Z

Hi @kcoronado. Thanks for your PR.

I'm waiting for a kubernetes or kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

kcoronado · 2018-04-19T17:03:52Z

/assign @krousey @jessicaochen

jessicaochen · 2018-04-19T17:53:06Z

cloud/google/machineactuator.go

@@ -50,7 +54,12 @@ const (
 	ZoneAnnotationKey    = "gcp-zone"
 	NameAnnotationKey    = "gcp-name"



Hmmm, I think you may need to rebase. You are re-adding the uid logic variable that was removed in another change.

jessicaochen · 2018-04-19T17:56:08Z

cloud/google/machineactuator.go

+		} else {
+			return nil, fmt.Errorf("Instance %v %v %v is missing uid label.", config.Project, config.Zone, identifyingMachine.ObjectMeta.Name)
+		}
+	} else if uid != fmt.Sprintf("%v", machine.ObjectMeta.UID) {


Another section of re-added logic that was removed.

kcoronado · 2018-04-19T21:10:12Z

Sorry about that! I originally just copied and pasted the whole file from the old repo because I thought it was up to date. I went and double checked the diffs from this PR and the old PR and got rid of any additions that weren't in the old one (it was just the UID logic).

jessicaochen

lgtm

krousey · 2018-04-20T18:50:43Z

/ok-to-test

krousey · 2018-04-20T20:19:51Z

/test pull-cluster-api-test

krousey · 2018-04-20T20:39:41Z

cloud/google/machinesetup/config_types_test.go

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


kcoronado · 2018-04-20T21:58:19Z

I thought I ran the tests when I migrated but I guess not :\ I fixed the paths so it's passing now.

krousey · 2018-04-20T22:13:23Z

The problem is since you have the old repo in your go path, it was able to find it locally.

…

On Fri, Apr 20, 2018, 14:58 Katie Coronado ***@***.***> wrote: I thought I ran the tests when I migrated but I guess not :\ I fixed the paths so it's passing now. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#104 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJlmxotWzp0Cg-tuVv5cMg1_Z8WbotRks5tqln7gaJpZM4TcKMn> .

krousey · 2018-04-21T02:10:04Z

/lgtm

k8s-ci-robot · 2018-04-21T02:10:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kcoronado, krousey

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~~ [krousey]

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

pipejakob · 2018-04-24T22:38:42Z

cloud/google/machineactuator.go

+	if configListPath != "" {
+		configWatch, err = machinesetup.NewConfigWatch(configListPath)
+		if err != nil {
+			glog.Errorf("Error creating config watch: %v", err)


Was it deliberate that this is swallowing the error instead of returning it? If so, I think it's worth a comment why, because that's usually an anti-pattern and could use justification.

pipejakob · 2018-04-24T22:41:43Z

cloud/google/machineactuator.go

@@ -103,6 +112,15 @@ func NewMachineActuator(kubeadmToken string, machineClient client.MachineInterfa
 		}
 	}

+	// TODO: get rid of empty string check when we switch to the new bootstrapping method.


For each of these TODOs, it would be great to create a tracking issue with all the necessary context, and link to them in these comments. Otherwise, the actual meaning of "new bootstrapping method," and why an empty string check can be removed, etc. will slowly fade from people's memory.

pipejakob · 2018-04-24T22:44:33Z

cloud/google/machineactuator.go

+		return err
+	}
+	configMap := corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{Name: "machine-setup"},


This magic string is used in a few places. Is it possible to move it to a shared constant, and reference that instead?

actually run push images

Update Makefile that runs generates to use the generate boilerplate instead of the go boilerplate

Replace hardcoded startup scripts with machine setup configs

d4cc9e2

- Rename GCEProviderConfig.Image field to OS - Remove call to preloaded script in generate-image - Create a machine-setup ConfigMap to mount the yaml file

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 19, 2018

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

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

jessicaochen suggested changes Apr 19, 2018

View reviewed changes

Remove UID logic

46490ac

jessicaochen approved these changes Apr 19, 2018

View reviewed changes

kcoronado mentioned this pull request Apr 19, 2018

GCP: support VM images that come with container runtime pre-installed #80

Closed

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 20, 2018

krousey reviewed Apr 20, 2018

View reviewed changes

Fix import path in tests

231f71b

krousey approved these changes Apr 21, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 21, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 21, 2018

k8s-ci-robot merged commit 262113e into kubernetes-sigs:master Apr 21, 2018

pipejakob reviewed Apr 24, 2018

View reviewed changes

pipejakob mentioned this pull request Apr 24, 2018

Use kubenet instead of weavenet for GCP provider #107

Merged

karan mentioned this pull request Apr 26, 2018

Prototype of named machines for vsphere #110

Merged

kcoronado mentioned this pull request Apr 27, 2018

Bump GCE machine controller image #113

Merged

k4leung4 mentioned this pull request Apr 27, 2018

Correct the network configuration to allow the master to come up successfully #114

Merged

kcoronado mentioned this pull request May 1, 2018

Add/update documentation for machine setup configs #120

Merged

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#104 from chuckha/push

7e67053

actually run push images

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019

Merge pull request kubernetes-sigs#104 from SataQiu/ifx-20190809

5c77b24

Update Makefile that runs generates to use the generate boilerplate instead of the go boilerplate

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handling configurable machine setup/installation #104

Handling configurable machine setup/installation #104

kcoronado commented Apr 19, 2018

k8s-ci-robot commented Apr 19, 2018

kcoronado commented Apr 19, 2018

jessicaochen Apr 19, 2018

jessicaochen Apr 19, 2018

kcoronado commented Apr 19, 2018

jessicaochen left a comment

krousey commented Apr 20, 2018

krousey commented Apr 20, 2018

krousey Apr 20, 2018

kcoronado commented Apr 20, 2018

krousey commented Apr 20, 2018 via email

krousey commented Apr 21, 2018

k8s-ci-robot commented Apr 21, 2018

pipejakob Apr 24, 2018

pipejakob Apr 24, 2018

pipejakob Apr 24, 2018

		@@ -50,7 +54,12 @@ const (
		ZoneAnnotationKey = "gcp-zone"
		NameAnnotationKey = "gcp-name"

Handling configurable machine setup/installation #104

Handling configurable machine setup/installation #104

Conversation

kcoronado commented Apr 19, 2018

k8s-ci-robot commented Apr 19, 2018

kcoronado commented Apr 19, 2018

jessicaochen Apr 19, 2018

Choose a reason for hiding this comment

jessicaochen Apr 19, 2018

Choose a reason for hiding this comment

kcoronado commented Apr 19, 2018

jessicaochen left a comment

Choose a reason for hiding this comment

krousey commented Apr 20, 2018

krousey commented Apr 20, 2018

krousey Apr 20, 2018

Choose a reason for hiding this comment

kcoronado commented Apr 20, 2018

krousey commented Apr 20, 2018 via email

krousey commented Apr 21, 2018

k8s-ci-robot commented Apr 21, 2018

pipejakob Apr 24, 2018

Choose a reason for hiding this comment

pipejakob Apr 24, 2018

Choose a reason for hiding this comment

pipejakob Apr 24, 2018

Choose a reason for hiding this comment