Implemented AttachISO task #4
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,19 +17,43 @@ limitations under the License. | |
| package vspheretasks | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "github.com/golang/glog" | ||
| "github.com/pborman/uuid" | ||
| "io/ioutil" | ||
| "k8s.io/kops/pkg/apis/kops" | ||
| "k8s.io/kops/pkg/model" | ||
| "k8s.io/kops/upup/pkg/fi" | ||
| "k8s.io/kops/upup/pkg/fi/cloudup/vsphere" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| "runtime" | ||
| "strings" | ||
| ) | ||
|
|
||
| // AttachISO represents the cloud-init ISO file attached to a VMware VM | ||
| //go:generate fitask -type=AttachISO | ||
| type AttachISO struct { | ||
|
There was a problem hiding this comment. Can you please explain why did you have to remove dependency on VirtualMachine task and add it explicitly via GetDependencies method? It would be nice if we can utilize the dependency graph of topological sort, rather than having to implement explicit GetDependencies method for various vsphere tasks. Also, it would make more sense to get the vm name from VirtualMachine, as opposed to setting it in various tasks that need it, eg: AttachISO and VMPowerOn. There was a problem hiding this comment. VirtualMachine is a task. I wanted to add more members to AttachISO. The way the reflection based task dependency works is that it thinks that every field in the task is a dependency. For example, when I added InstanceGroup and BootstratScript fields in AttachISO task, the reflection based task dependency thinks that InstanceGroup and BootstratScript are tasks and fails complaining that they are not tasks. Basically if the tasks structs are simple, then the reflection based task dependency management is useful but in our case it is not simple. So, I had to explicitly call out the task dependency which is also fine. Basically there are 2 ways of expressing task dependency -
There was a problem hiding this comment. Hmm... I don't see this happening for slightly complex aws tasks: There was a problem hiding this comment. I believe I have found the problem that you are talking about. This is the code responsible for ignoring or accounting a dependency for reflect.Struct https://github.com/vmware/kops/blob/master/upup/pkg/fi/topological_sort.go#L98 The problem is that current code identifies only those structs that are listed in the if-else structure. The structs that you are adding to your task, like InstanceGroup and BootstratScript don't end up getting processed by last else-block and hence the error "Unhandled type for...". This if-else structure is not the ideal way of defining dependencies. But implementing HasDepenency method is also bit of a redundant work. I would recommend maintaining a set of types, like InstanceGroup and BootScript, which we don't want to include as a dependency. Check that in the above discussed if-else block. |
||
| Name *string | ||
|
There was a problem hiding this comment. I see that you have updated code to use VM.Name. Do you still need this string 'Name' field? There was a problem hiding this comment. 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. |
||
| VM *VirtualMachine | ||
| IG *kops.InstanceGroup | ||
| BootstrapScript *model.BootstrapScript | ||
| } | ||
|
|
||
| var _ fi.HasName = &AttachISO{} | ||
| var _ fi.HasDependencies = &AttachISO{} | ||
|
|
||
| func (o *AttachISO) GetDependencies(tasks map[string]fi.Task) []fi.Task { | ||
| var deps []fi.Task | ||
| vmCreateTask := tasks["VirtualMachine/"+*o.VM.Name] | ||
| if vmCreateTask == nil { | ||
| glog.Fatalf("Unable to find create VM task %s dependency for AttachISO %s", *o.VM.Name, *o.Name) | ||
| } | ||
| deps = append(deps, vmCreateTask) | ||
| return deps | ||
| } | ||
|
|
||
| // GetName returns the Name of the object, implementing fi.HasName | ||
| func (o *AttachISO) GetName() *string { | ||
|
|
@@ -56,7 +80,83 @@ func (_ *AttachISO) CheckChanges(a, e, changes *AttachISO) error { | |
| return nil | ||
| } | ||
|
|
||
| func (_ *AttachISO) RenderVSphere(t *vsphere.VSphereAPITarget, a, e, changes *AttachISO) error { | ||
| startupScript, err := changes.BootstrapScript.ResourceNodeUp(changes.IG) | ||
| startupStr, err := startupScript.AsString() | ||
| if err != nil { | ||
| return fmt.Errorf("error rendering startup script: %v", err) | ||
| } | ||
| dir, err := ioutil.TempDir("", *changes.VM.Name) | ||
| defer os.RemoveAll(dir) | ||
|
|
||
| isoFile := createISO(changes, startupStr, dir) | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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. |
||
| err = t.Cloud.UploadAndAttachISO(changes.VM.Name, isoFile) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func createUserData(startupStr string, dir string) { | ||
| // Update the startup script to add the extra spaces for | ||
| // indentation when copied to the user-data file. | ||
| strArray := strings.Split(startupStr, "\n") | ||
| for i, str := range strArray { | ||
| if len(str) > 0 { | ||
| strArray[i] = " " + str | ||
| } | ||
| } | ||
| startupStr = strings.Join(strArray, "\n") | ||
|
|
||
| data := strings.Replace(userDataTemplate, "$SCRIPT", startupStr, -1) | ||
| userDataFile := filepath.Join(dir, "user-data") | ||
| 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) | ||
|
There was a problem hiding this comment. Errors from createUserData and createMetaData are not getting propagated to createISO. How do you plan on handling them? There was a problem hiding this comment. They fail the kops command. See the usage of glog.Fatalf method in error conditions in those methods. |
||
| } | ||
| return | ||
| } | ||
|
|
||
| func createMetaData(dir string, vmName string) { | ||
| data := strings.Replace(metaDataTemplate, "$INSTANCE_ID", uuid.NewUUID().String(), -1) | ||
| data = strings.Replace(data, "$LOCAL_HOST_NAME", vmName, -1) | ||
|
|
||
| glog.V(4).Infof("Meta data file content: %s", string(data)) | ||
|
|
||
| metaDataFile := filepath.Join(dir, "meta-data") | ||
| if err := ioutil.WriteFile(metaDataFile, []byte(data), 0644); err != nil { | ||
| glog.Fatalf("Unable to write meta-data into file %s", metaDataFile) | ||
| } | ||
| return | ||
| } | ||
|
|
||
| func createISO(changes *AttachISO, startupStr string, dir string) string { | ||
| createUserData(startupStr, dir) | ||
| createMetaData(dir, *changes.VM.Name) | ||
|
|
||
| isoFile := filepath.Join(dir, *changes.VM.Name+".iso") | ||
| var cmd *exec.Cmd | ||
|
|
||
| switch os := runtime.GOOS; os { | ||
| case "darwin": | ||
| cmd = exec.Command("mkisofs", "-o", isoFile, "-volid", "cidata", "-joliet", "-rock", dir) | ||
| case "linux": | ||
| cmd = exec.Command("genisoimage", "-o", isoFile, "-volid", "cidata", "-joliet", "-rock", dir) | ||
| default: | ||
| glog.Fatalf("Cannot generate ISO file %s. Unsupported operation system!!!", isoFile) | ||
| } | ||
| var out bytes.Buffer | ||
| cmd.Stdout = &out | ||
| var stderr bytes.Buffer | ||
| cmd.Stderr = &stderr | ||
|
|
||
| err := cmd.Run() | ||
| if err != nil { | ||
| glog.Fatalf("Error %s occurred while executing command %+v", err, cmd) | ||
|
There was a problem hiding this comment. Not needed as err not being nil is fatal and will stop kops. |
||
| } | ||
| glog.V(4).Infof("genisoimage std output : %s\n", out.String()) | ||
| glog.V(4).Infof("genisoimage std error : %s\n", stderr.String()) | ||
| return isoFile | ||
|
There was a problem hiding this comment. Please save command name in a variable and use that for logging. It could be genisoimage or mkisofs. |
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait we would need this builder for PKI model...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on who checks in first, one of us can add it!