Move tf-state to machine object, and remove file system dependency #264

karan · 2018-06-01T00:26:51Z

What this PR does / why we need it:

#224

Bake the terraform providers in the controller image.
Instead of storing the terraform config and state on disk, rely on the named machine config map and machine object. See below for a little more details.
Lots of other refactors for readability.

Special notes for your reviewer:

The only thing I have supported using this model, and tested is cluster create. Update will be done after we move to clusterctl and delete vsphere-deployer.
During bootstrap, the machineClient does not exist (we are not in K8s land). This means that to transfer the state for the master machine, we still need to scp the directory to master. This will be fixed after Working vsphere clusterctl example #263 -- in minikube, machineClient will always exist, so we can simply pivot the machine object and remove the volume mount entirely. Expect a follow-up PR.
The current flow now is that when we need to create a new machine, we create a staging directory /tmp/cluster-api/machines/$MACHINE_NAME/. After the machine is created, the tfstate is populated in the Machine object as an annotation. Then the staging directory is deleted.
There are a lot of other refactors I want to do, but let's leave those our of this PR.
I am not updating the image. That will happen after I integrate and test cluster creation with clusterctl (and jess's bootstrap change).

k8s-ci-robot · 2018-06-01T00:26:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karan

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

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

karan · 2018-06-01T00:29:40Z

/assign @mkjelland

/assign @kcoronado

mkjelland · 2018-06-01T15:39:29Z

cloud/vsphere/pods.go

-var machineControllerImage = "gcr.io/k8s-cluster-api/vsphere-machine-controller:0.0.1"
+
+//var machineControllerImage = "gcr.io/k8s-cluster-api/vsphere-machine-controller:0.0.2"
+var machineControllerImage = "gcr.io/karangoel-gke-1/vsphere-machine-controller:0.0.2-dev"


Did you intend to use leave the dev image uncommented?

I guess I can remove it although the image published is a non-dev image.

mkjelland · 2018-06-01T16:24:09Z

cloud/vsphere/machineactuator.go

+		return err
+	}
+
+	if verr := vc.validateMachine(machine, config); verr != nil {


Is there a reason to use a new error (verr) instead of using the previously declared err?

No -- legacy code.

mkjelland · 2018-06-01T16:27:32Z

cloud/vsphere/machineactuator.go

@@ -398,15 +466,17 @@ func (vc *VsphereClient) Update(cluster *clusterv1.Cluster, goalMachine *cluster
 	// This can only happen right after bootstrapping.
 	if goalMachine.ObjectMeta.Annotations == nil {
 		ip, _ := vc.GetIP(goalMachine)
-		glog.Info("Annotations do not exist. Populating existing state for bootstrapped machine.")
-		return vc.updateAnnotations(goalMachine, ip)
+		glog.Info("Annotations do not exist. This happens when for a newly bootstrapped machine.")


remove "when"

mkjelland · 2018-06-01T16:30:51Z

cloud/vsphere/machineactuator.go

+		return string(tfStateBytes), nil
+	}
+
+	return "", errors.New("could not get tfstatae")


typo: should be tfstate

mkjelland · 2018-06-01T16:43:24Z

cloud/vsphere/machineactuator.go

 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 	cmd.Run()

 	return nil
 }

-func (vc *VsphereClient) updateAnnotations(machine *clusterv1.Machine, masterEndpointIp string) error {
+func (vc *VsphereClient) updateAnnotations(machine *clusterv1.Machine, masterEndpointIp, tfState string) error {


Can you comment on why this is an annotation, as opposed to a field in machine status ProviderStatus? Do you see it moving there eventually, or is there a reason why you don't think it makes sense there?

I added a comment.

// We are storing these as annotations and not in Machine Status because that's intended for // "Provider-specific status" that will usually be used to detect updates. Additionally, // Status requires yet another version API resource which is too heavy to store IP and TF state.

mkjelland

lgtm, leaving for @kcoronado to add the lgtm label

kcoronado

/lgtm
The comments and PR description were helpful for me to understand what was going on, so thanks for that!

karan · 2018-06-01T18:32:32Z

Thanks for the reviews!

…ubernetes-sigs#264) * make the new broken type work, pass user, pass to tf * cleanup logs, bump image * Progress so far * make state in machine object work * Add and fix comments in vsphere provider

* add standalone esx support * move all glog to klog * Fixed machine provisioning on ESXi. - fixed boot sequence on some images (e.g. xenial) - fixed sudo on machines without DNS access - fixed cloud provider bootstrap - fixed rbac role preventing machine deletion - refactored templates.go and the esx cloning code Fixed boot sequence on some images by adding a serial port to allow random number initialization. This affect some images like Xenial. It currently adds a serial port to all machines if it doesn't already in the vm spec. Fixed sudo access for machines without DNS access, which for most development scenarios in nested ESXi on dev laptops. Fixed cloud provider bootstrapping on infrastructure that do not have cloud provider support (e.g. ESXi) issue kubernetes-sigs#177

karan added 4 commits May 31, 2018 14:12

make the new broken type work, pass user, pass to tf

7bbd5ac

cleanup logs, bump image

cfe1c99

Progress so far

3471e75

make state in machine object work

dc2b1ac

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 1, 2018

k8s-ci-robot assigned kcoronado and mkjelland Jun 1, 2018

karan mentioned this pull request Jun 1, 2018

[vSphere] Move terraform state to machine object, instead of file system #224

Closed

5 tasks

mkjelland reviewed Jun 1, 2018

View reviewed changes

Add and fix comments in vsphere provider

eed8b37

mkjelland approved these changes Jun 1, 2018

View reviewed changes

kcoronado approved these changes Jun 1, 2018

View reviewed changes

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

k8s-ci-robot merged commit f104e0e into kubernetes-sigs:master Jun 1, 2018

jessicaochen mentioned this pull request Jun 1, 2018

Working vsphere clusterctl example #263

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move tf-state to machine object, and remove file system dependency #264

Move tf-state to machine object, and remove file system dependency #264

karan commented Jun 1, 2018

k8s-ci-robot commented Jun 1, 2018

karan commented Jun 1, 2018

mkjelland Jun 1, 2018

karan Jun 1, 2018

mkjelland Jun 1, 2018

karan Jun 1, 2018

mkjelland Jun 1, 2018

karan Jun 1, 2018

mkjelland Jun 1, 2018

karan Jun 1, 2018

mkjelland Jun 1, 2018

karan Jun 1, 2018

mkjelland left a comment

kcoronado left a comment

karan commented Jun 1, 2018

Move tf-state to machine object, and remove file system dependency #264

Move tf-state to machine object, and remove file system dependency #264

Conversation

karan commented Jun 1, 2018

k8s-ci-robot commented Jun 1, 2018

karan commented Jun 1, 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

mkjelland left a comment

Choose a reason for hiding this comment

kcoronado left a comment

Choose a reason for hiding this comment

karan commented Jun 1, 2018