Implemented AttachISO task #4

SandeepPissay · 2017-03-22T02:07:39Z

AttachISO task creates the user-data/meta-data cloud init files and creates cloud-init.iso file using "genisoimage" tool. It then uploads it to the datastore where the master/worker VM resides and inserts it into the cd-rom device of the master/worker VM. When the master/worker VM powers on, the cloud-init package in it runs the bootstrap script that downloads nodeup and runs it.

Testing done:

Tested end to end that the master and worker VMs executes the cloud-init script successfully.
2, "make ci" is successful.

…plete. This is partial implementation!

prashima · 2017-03-22T17:13:24Z

upup/pkg/fi/cloudup/vsphere/vsphere_cloud.go

+	}
+	p := soap.DefaultUpload
+	glog.V(2).Infof("Uploading ISO file %s to datastore %+v\n", isoFile, dsObj)
+	err = dsObj.UploadFile(ctx, isoFile, *vm+"/cloud-init.iso", &p)


Please define a constant for loose string. Also putting name construction in a separate method would be nice. We might have to reuse this name in case we need to do any further ISO related operations.

Nit: variable names like localIsoFile, dstIsoFile or something similar would help understand the method usage better. Also it might be useful to log the destination iso file name, along with local.

prashima · 2017-03-22T17:14:27Z

upup/pkg/fi/cloudup/vsphere/vsphere_cloud.go

+		return err
+	}
+
+	cdrom, err := devices.FindCdrom("")


Can you please explain with does empty string argument signifies here?

prashima · 2017-03-22T17:19:16Z

upup/pkg/fi/cloudup/vsphere/vsphere_cloud.go

+	if err != nil {
+		return err
+	}
+	iso := dsObj.Path(*vm + "/cloud-init.iso")


Nit: log here that the iso uploaded successfully would be helpful. Also same variable can be used for the destination iso file name here, as the one used for upload.

Here we are inserting the iso into the cd-rom. Will add a log.

prashima · 2017-03-22T17:24:19Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+write_files:
+  - content: |
+$SCRIPT
+    owner: root:root


Over the period this user data is going to become bigger and crowd this file. Let's keep this in a separate file where we only have cloud-init related strings specified.

prashima · 2017-03-22T17:37:43Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

@@ -57,6 +94,73 @@ func (_ *AttachISO) CheckChanges(a, e, changes *AttachISO) error {
 }

 func (_ *AttachISO) RenderVC(t *vsphere.VSphereAPITarget, a, e, changes *AttachISO) error {


VirtualMachine task has 'RenderVSphere' method defined on it, where as AttachISO and VMPowerOn tasks have 'RenderVC' method defined on it. Can you please fix it?

My understanding was that 'RenderVSphere' method name will be constructed and called using reflection, based on the target name. I am wondering how 'RenderVC' is getting invoked for AttachISO and VMPowerOn.

Task framework calls Render* method. I'll rename RenderVC method to RenderVSphere to keep it consistent with "VSphere" we have been using everywhere else.

prashima · 2017-03-22T17:53:33Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+	createMetaData(dir, *changes.VM)
+
+	isoFile := filepath.Join(dir, *changes.VM+".iso")
+	cmd := exec.Command("genisoimage", "-o", isoFile, "-volid", "cidata", "-joliet", "-rock", dir)


Please check for the availability of "genisonimage". Also, genisoimage is not available for mac. For VCS prototype I was using mkisofs, if genisoimage was not present. If both are not available we will have to fail with appropriate error.

I would need a mac to check how mkisofs works. I'll come to your place for some experiments. I'll enhance this code.

prashima · 2017-03-22T17:55:53Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+
+	err := cmd.Run()
+	if err != nil {
+		glog.Fatalf("Error %s occurred while executing command %+v", err, cmd)


Return here with nil?

Not needed as err not being nil is fatal and will stop kops.

prashima · 2017-03-22T17:56:37Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+	dir, err := ioutil.TempDir("", *changes.VM)
+	defer os.RemoveAll(dir)
+
+	isoFile := createISO(changes, startupStr, dir)


I see that createISO method is not returning any error. It would be good to either return an error or check isoFile to be not nil.

If there is an error in createISO we fail the execution by invoking glog.Fatalf method. Basically there is no point in continuing if they fail.

prashima · 2017-03-22T18:06:18Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+	}
+	startupStr = strings.Join(strArray, "\n")
+
+	data := strings.Replace(userData, "$SCRIPT", startupStr, -1)


It would be better to do this using go templates. If you don't want to make this change in this PR, please feel free to add an issue for improving this method and createMetaData to make use of template.

Filed an issue

prashima · 2017-03-22T18:07:24Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+	glog.V(4).Infof("User data file content: %s", data)
+
+	if err := ioutil.WriteFile(userDataFile, []byte(data), 0644); err != nil {
+		glog.Fatalf("Unable to write user-data into file %s", userDataFile)


Errors from createUserData and createMetaData are not getting propagated to createISO. How do you plan on handling them?

They fail the kops command. See the usage of glog.Fatalf method in error conditions in those methods.

SandeepPissay · 2017-03-23T00:44:35Z

Addressed review comments.

luomiao · 2017-03-23T02:16:24Z

upup/pkg/fi/cloudup/apply_cluster.go

-					KopsModelContext: modelContext,
-				}
-
-				l.Builders = append(l.Builders,


wait we would need this builder for PKI model...

Depending on who checks in first, one of us can add it!

prashima

LGTM.

Please address comments before merging.

prashima · 2017-03-23T16:39:44Z

upup/pkg/fi/cloudup/apply_cluster.go

@@ -482,6 +476,7 @@ func (c *ApplyClusterCmd) Run() error {
 		}

 		role := ig.Spec.Role
+		glog.V(4).Infof("Generating renderNodeUpConfig for %s", role)


Nit: Please move this log statement after if-block. If role is nil then anyways the program is exiting with error.

"Generating Nodeup config for role..." would be a more appropriate message.

I did not intend to checkin this. It was added for debugging. Will remove this log,

prashima · 2017-03-23T17:05:24Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

 )

 // AttachISO represents the cloud-init ISO file attached to a VMware VM
 //go:generate fitask -type=AttachISO
 type AttachISO struct {
-	Name *string
-	VM   *VirtualMachine
+	Name            *string


I see that you have updated code to use VM.Name. Do you still need this string 'Name' field?

I think every task struct need to have the task Name. This was added in my previous checkin and I remember that not having this field was having problems executing the tasks.

prashima · 2017-03-23T17:29:59Z

upup/pkg/fi/cloudup/vspheretasks/attachiso.go

+	}
+	glog.V(4).Infof("genisoimage std output : %s\n", out.String())
+	glog.V(4).Infof("genisoimage std error : %s\n", stderr.String())
+	return isoFile


Please save command name in a variable and use that for logging. It could be genisoimage or mkisofs.

luomiao · 2017-03-23T17:47:09Z

BTW can we compile with this PR's change on MacOS now?

…emplate

SandeepPissay · 2017-03-23T18:19:06Z

Compilation is never a problem. The issue was the tool used to generate ISO file. If we run kops on mac you would need mkisofs tool and it should be in your PATH env variable.

Fix the type of vSphere fields in CloudConfig and update auto-generated conversion files.

AttachISO task creates the user-data/meta-data cloud init files and creates cloud-init.iso file using "genisoimage" tool. It then uploads it to the datastore where the master/worker VM resides and inserts it into the cd-rom device of the master/worker VM. When the master/worker VM powers on, the cloud-init package in it runs the bootstrap script that downloads nodeup and runs it. Also removed redundant VirtualMachineModelBuilder that does nothing. Testing done: 1. Tested end to end that the master and worker VMs executes the cloud-init script successfully. 2, "make ci" is successful.

SandeepPissay added 2 commits March 21, 2017 18:16

Generating cloud-init file and uploading it to the VM home folder com…

121c791

…plete. This is partial implementation!

AttachISO task working end to end

e91b808

SandeepPissay requested review from luomiao and prashima March 22, 2017 02:07

prashima reviewed Mar 22, 2017

View reviewed changes

Addressed review comments

673db57

Removed redundant VirtualMachineModelBuilder that does nothing

503e74a

luomiao reviewed Mar 23, 2017

View reviewed changes

prashima approved these changes Mar 23, 2017

View reviewed changes

Addressed review comments and minor changes to cloud-init user-data t…

99c7427

…emplate

SandeepPissay merged commit 990c60f into vmware-archive:vsphere-develop Mar 23, 2017

prashima pushed a commit to prashima/kops that referenced this pull request Mar 28, 2017

Merge pull request vmware-archive#4 from luomiao/new-develop

5c1d24f

Fix the type of vSphere fields in CloudConfig and update auto-generated conversion files.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implemented AttachISO task #4

Implemented AttachISO task #4

SandeepPissay commented Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

prashima Mar 22, 2017

SandeepPissay Mar 22, 2017

SandeepPissay commented Mar 23, 2017

luomiao Mar 23, 2017

SandeepPissay Mar 23, 2017

prashima left a comment

prashima Mar 23, 2017

SandeepPissay Mar 23, 2017

prashima Mar 23, 2017

SandeepPissay Mar 23, 2017

prashima Mar 23, 2017

SandeepPissay Mar 23, 2017

luomiao commented Mar 23, 2017

SandeepPissay commented Mar 23, 2017

		@@ -57,6 +94,73 @@ func (_ AttachISO) CheckChanges(a, e, changes AttachISO) error {
		}

		func (_ AttachISO) RenderVC(t vsphere.VSphereAPITarget, a, e, changes *AttachISO) error {

Implemented AttachISO task #4

Implemented AttachISO task #4

Conversation

SandeepPissay commented Mar 22, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SandeepPissay commented Mar 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prashima 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

luomiao commented Mar 23, 2017

SandeepPissay commented Mar 23, 2017