vCenter push capability #1929

thebsdbox · 2017-05-30T11:08:36Z

Re-factored the run code to create the push functionality.

Signed-off-by: Dan Finneran daniel.finneran@gmail.com

- What I did
Added the capability to push Linuxkit .iso images to VMware vCenter. It uses a either a -folder=<NAME> or will take the name of the image and create that folder to host the uploaded image.

- How I did it
Modified run_vcenter.go to allow push_vcenter.go to log in to vCenter and upload images without creating a full VM.

- How to verify it
Push functionality

./bin/linuxkit push vcenter -datastore=vSphereNFS -hostname=esxi01.fnnrn.me -folder=sharedLinuxKit2  ./vcenter.iso 
Uploading LinuxKit file [./vcenter.iso]

run functionality (ensuring changes haven't broken existing functionality.

./bin/linuxkit run vcenter -datastore=vSphereNFS -network=HomeNetwork -hostname=esxi01.fnnrn.me -powerOn -persistentSize=1024 ./vcenter.iso 
Creating new LinuxKit Virtual Machine
Uploading LinuxKit file [./vcenter.iso]
Adding ISO to the Virtual Machine
Adding a persistent disk to the Virtual Machine
Adding VM Networking
Powering on LinuxKit VM

- Description for the changelog

Added the functionality to push linuxkit images to VMware vCenter.

- A picture of a cute animal (not mandatory but encouraged)

justincormack · 2017-05-30T11:10:53Z

src/cmd/linuxkit/push_vcenter.go

+	newVM.vSphereHost = flags.String("hostname", os.Getenv("VMHOST"), "The Server that will host the image")
+	newVM.path = flags.String("path", "", "Path to a specific image")
+
+	newVM.vmName = flags.String("folder", "", "A Folder on the datastore to push the image too")


its confusing calling it "folder" and "vmName" - maybe call both folder?

Opted for vmFolder, as it makes sense in both use cases. With the run the vmFolder is both the name of the VM and the name of the folder that will be created to house the various VM files. With push the vmFolder is simply a folder created on a datastore that a will hold the VM iso file. Does that sound OK?

justincormack · 2017-05-30T11:11:56Z

src/cmd/linuxkit/push_vcenter.go

+	// Ensure an iso has been passed to the vCenter push Command
+	if strings.HasSuffix(*newVM.path, ".iso") {
+		// Used as a folder to hold the images that are pushed
+		if *newVM.vmName == "" {


there is no need for this to be in this condition, it can be unconditional below.

rn · 2017-05-30T13:38:54Z

src/cmd/linuxkit/push_vcenter.go

+	newVM.dsName = flags.String("datastore", os.Getenv("VMDATASTORE"), "The Name of the DataStore to host the image")
+	newVM.networkName = flags.String("network", os.Getenv("VMNETWORK"), "The VMware vSwitch the image will use")
+	newVM.vSphereHost = flags.String("hostname", os.Getenv("VMHOST"), "The Server that will host the image")
+	newVM.path = flags.String("path", "", "Path to a specific image")


Are these standard environment variables or made up? They seem to be inconsistently named and I would prefer if they had a common prefix...

rn · 2017-05-30T13:39:36Z

src/cmd/linuxkit/push_vcenter.go

+	flags := flag.NewFlagSet("vCenter", flag.ExitOnError)
+	invoked := filepath.Base(os.Args[0])
+
+	newVM.vCenterURL = flags.String("url", os.Getenv("VCURL"), "URL in the format of https://username:password@host/sdk")


Is this the URL to the vcenter host? Id so, mention it in the help message

rn · 2017-05-30T13:39:57Z

src/cmd/linuxkit/push_vcenter.go

+	invoked := filepath.Base(os.Args[0])
+
+	newVM.vCenterURL = flags.String("url", os.Getenv("VCURL"), "URL in the format of https://username:password@host/sdk")
+	newVM.dsName = flags.String("datastore", os.Getenv("VMDATASTORE"), "The Name of the DataStore to host the image")


nit: Name -> name

rn · 2017-05-30T13:41:28Z

src/cmd/linuxkit/push_vcenter.go

+
+	newVM.vCenterURL = flags.String("url", os.Getenv("VCURL"), "URL in the format of https://username:password@host/sdk")
+	newVM.dsName = flags.String("datastore", os.Getenv("VMDATASTORE"), "The Name of the DataStore to host the image")
+	newVM.networkName = flags.String("network", os.Getenv("VMNETWORK"), "The VMware vSwitch the image will use")


Is this the name of the network the VM will be placed on or the name of a vSwitch as the help message implies?

rn · 2017-05-30T13:41:47Z

src/cmd/linuxkit/push_vcenter.go

+	newVM.vCenterURL = flags.String("url", os.Getenv("VCURL"), "URL in the format of https://username:password@host/sdk")
+	newVM.dsName = flags.String("datastore", os.Getenv("VMDATASTORE"), "The Name of the DataStore to host the image")
+	newVM.networkName = flags.String("network", os.Getenv("VMNETWORK"), "The VMware vSwitch the image will use")
+	newVM.vSphereHost = flags.String("hostname", os.Getenv("VMHOST"), "The Server that will host the image")


nit: Server -> server

rn · 2017-05-30T13:43:47Z

src/cmd/linuxkit/push_vcenter.go

+	newVM.vmFolder = flags.String("folder", "", "A Folder on the datastore to push the image too")
+
+	flags.Usage = func() {
+		fmt.Printf("USAGE: %s push vcenter [options] [name]\n\n", invoked)


is name optional as the brackets imply? To be consistent with the qemu runner this should probably be called path

rn · 2017-05-30T13:45:45Z

src/cmd/linuxkit/push_vcenter.go

+
+	// Ensure an iso has been passed to the vCenter push Command
+	if !strings.HasSuffix(*newVM.path, ".iso") {
+		log.Fatalln("Ensure that an \".iso\" file is used as part of the path")


maybe rephrase: Please pass an ".iso" file as the path or somesuch.

riyazdf · 2017-05-30T22:30:38Z

src/cmd/linuxkit/push_vcenter.go

+	}
+
+	if err := flags.Parse(args); err != nil {
+		log.Fatalln("Unable to parse args")


should we include the error message? log.Fatalf("Unable to parse args: %v", err)?

We should probably standardise this across all of the backends?

Actually, looking through the source code as we use ExitOnError and the parse does the following:

case ExitOnError: os.Exit(2)

The log.fatalln(...) will never be called.

Does that look right @justincormack / @rneugeba / @riyazdf ?

As we have this same snippet in a lot of places.

Yes thats probably true, but the linter will complain if we don't catch the error I guess so it doesn't really matter. Just be consistent with elsewhere.

OK, i'll leave it as is for now.

got it - yeah we can probably standardize this with a custom error type (in a followup PR across backends) but this is good for now

riyazdf · 2017-05-30T22:34:02Z

src/cmd/linuxkit/push_vcenter.go

+	newVM.vSphereHost = flags.String("hostname", os.Getenv("VCHOST"), "The server that will host the image")
+	newVM.path = flags.String("path", "", "Path to a specific image")
+
+	newVM.vmFolder = flags.String("folder", "", "A Folder on the datastore to push the image too")


nit: can we lowercase to folder?

riyazdf · 2017-05-30T22:34:20Z

src/cmd/linuxkit/run_vcenter.go


-	newVM.vmName = flags.String("vmname", "", "Specify a name for virtual Machine")
+	newVM.vmFolder = flags.String("vmfolder", "", "Specify a name/folder for the virtual Machine to reside in")


nit: can we lowercase to machine?

riyazdf · 2017-05-30T22:35:25Z

src/cmd/linuxkit/run_vcenter.go

+	newVM.vCenterURL = flags.String("url", os.Getenv("VCURL"), "URL of VMware vCenter in the format of https://username:password@VCaddress/sdk")
+	newVM.dsName = flags.String("datastore", os.Getenv("VCDATASTORE"), "The name of the DataStore to host the VM")
+	newVM.networkName = flags.String("network", os.Getenv("VCNETWORK"), "The network label the VM will use")
+	newVM.vSphereHost = flags.String("hostname", os.Getenv("VCHOST"), "The server that will run the VM")


what is the reason for changing to VC*? Should we update the other flags like vmfolder? I'm not too familiar with vCenter so I'm not sure what makes most sense here :)

The VC**** environment variables are there to make it a bit more clearer when repeatedly deploying VMs. The credentials and labels for vCenter internals will stay the same, so we can store them elsewhere (env vars in this case), the rest of the flags we can pass as usual as there is some need for uniqueness. Also @rneugeba, suggested some format for the env variables so I opted for VC.

riyazdf · 2017-05-30T22:37:38Z

src/cmd/linuxkit/run_vcenter.go

+	// Parse URL from string
+	u, err := url.Parse(*newVM.vCenterURL)
+	if err != nil {
+		log.Fatalf("URL can't be parsed, ensure it is https://username:password/<address>/sdk")


might be helpful to include the err message here? log.Fatalf("URL can't be parsed, ensure it is https://username:password/<address>/sdk: %v", err)

riyazdf · 2017-05-30T22:38:02Z

src/cmd/linuxkit/run_vcenter.go

+	// Connect and log in to ESX or vCenter
+	c, err := govmomi.NewClient(ctx, u, true)
+	if err != nil {
+		log.Fatalln("Error logging into vCenter, check address and credentials")


same comment re: including error message: log.Fatalf("Error logging into vCenter, check address and credentials: %v", err)

riyazdf · 2017-05-30T22:40:00Z

src/cmd/linuxkit/run_vcenter.go

+	// Find one and only datacenter, not sure how VMware linked mode will work
+	dc, err := f.DefaultDatacenter(ctx)
+	if err != nil {
+		log.Fatalln("No Datacenter instance could be found inside of vCenter")


same comment re: including error message for this and subsequent errors

riyazdf · 2017-05-30T22:41:51Z

src/cmd/linuxkit/run_vcenter.go

@@ -248,7 +260,7 @@ func addVMDK(ctx context.Context, vm *object.VirtualMachine, dss *object.Datasto
 		log.Fatalf("Unable to find SCSI device from VM configuration\n%v", err)
 	}
 	// The default is to have all persistent disks named linuxkit.vmdk
-	disk := devices.CreateDisk(controller, dss.Reference(), dss.Path(fmt.Sprintf("%s/%s", *newVM.vmName, "linuxkit.vmdk")))
+	disk := devices.CreateDisk(controller, dss.Reference(), dss.Path(fmt.Sprintf("%s/%s", *newVM.vmFolder, "linuxkit.vmdk")))


should this become configurable? Can be a separate PR, but wondering if we should open a tracking issue

It was to begin with, but then it became apparent that uploading avmdk from workstation to server required a lot more steps (image conversion from thin to fat, meant uploading GBs of zeros etc.) As there is a 1:1 mapping inside of VMware vCenter VMs I opted to keep it simple and in the event the run requires a persistent disk they just get a new disk inside of their VM folder to use.

Happy to revisit this, it felt that uploading large existing disks or having to manipulate the disk controllers to support additional SCSI disks might be outside of what run should be doing :)

ah got it. Makes sense to keep it like this for now then, thank you for the explanation!

justincormack · 2017-05-31T13:40:23Z

src/cmd/linuxkit/run_vcenter.go

-		fmt.Printf("USAGE: %s run vcenter [options] [name]\n\n", invoked)
-		fmt.Printf("'name' specifies the full path of an image that will be ran\n")
+		fmt.Printf("USAGE: %s run vcenter [options] path\n\n", invoked)
+		fmt.Printf("'path' specifies the full path of an image that will be ran\n")


"that will be ran" reads weirdly, "to run" is better

Updating, i've used "to run" below so changing it keeps the language consistent.

riyazdf

LGTM after nits, thanks @thebsdbox!

riyazdf · 2017-05-31T17:20:32Z

src/cmd/linuxkit/push_vcenter.go

+	}
+
+	if err := flags.Parse(args); err != nil {
+		log.Fatalln("Unable to parse args")


got it - yeah we can probably standardize this with a custom error type (in a followup PR across backends) but this is good for now

riyazdf · 2017-05-31T17:21:01Z

src/cmd/linuxkit/run_vcenter.go

+	newVM.vCenterURL = flags.String("url", os.Getenv("VCURL"), "URL of VMware vCenter in the format of https://username:password@VCaddress/sdk")
+	newVM.dsName = flags.String("datastore", os.Getenv("VCDATASTORE"), "The name of the DataStore to host the VM")
+	newVM.networkName = flags.String("network", os.Getenv("VCNETWORK"), "The network label the VM will use")
+	newVM.vSphereHost = flags.String("hostname", os.Getenv("VCHOST"), "The server that will run the VM")


riyazdf · 2017-05-31T17:23:03Z

src/cmd/linuxkit/run_vcenter.go

+	// Find one and only datacenter, not sure how VMware linked mode will work
+	dc, err := f.DefaultDatacenter(ctx)
+	if err != nil {
+		log.Fatalln("No Datacenter instance could be found inside of vCenter %v", err)


nit: I think this should be log.Fatalf?

true :D fixed.

riyazdf · 2017-05-31T17:23:35Z

src/cmd/linuxkit/run_vcenter.go

@@ -248,7 +260,7 @@ func addVMDK(ctx context.Context, vm *object.VirtualMachine, dss *object.Datasto
 		log.Fatalf("Unable to find SCSI device from VM configuration\n%v", err)
 	}
 	// The default is to have all persistent disks named linuxkit.vmdk
-	disk := devices.CreateDisk(controller, dss.Reference(), dss.Path(fmt.Sprintf("%s/%s", *newVM.vmName, "linuxkit.vmdk")))
+	disk := devices.CreateDisk(controller, dss.Reference(), dss.Path(fmt.Sprintf("%s/%s", *newVM.vmFolder, "linuxkit.vmdk")))


ah got it. Makes sense to keep it like this for now then, thank you for the explanation!

Re-factored the `run` code to create the `push` functionality. Signed-off-by: Dan Finneran <daniel.finneran@gmail.com>

riyazdf

LGTM on green, thanks for the quick fix!

GordonTheTurtle added the status/0-triage label May 30, 2017

justincormack reviewed May 30, 2017

View reviewed changes

thebsdbox force-pushed the push_vcenter branch from fc2b17e to 20664bb Compare May 30, 2017 13:33

rn reviewed May 30, 2017

View reviewed changes

thebsdbox force-pushed the push_vcenter branch from 20664bb to 3c8591b Compare May 30, 2017 14:18

riyazdf reviewed May 30, 2017

View reviewed changes

thebsdbox force-pushed the push_vcenter branch from 3c8591b to c46052c Compare May 31, 2017 09:24

justincormack reviewed May 31, 2017

View reviewed changes

thebsdbox force-pushed the push_vcenter branch from c46052c to 33d0e0b Compare May 31, 2017 13:44

justincormack approved these changes May 31, 2017

View reviewed changes

riyazdf approved these changes May 31, 2017

View reviewed changes

vCenter push capability

6d9a3c5

Re-factored the `run` code to create the `push` functionality. Signed-off-by: Dan Finneran <daniel.finneran@gmail.com>

thebsdbox force-pushed the push_vcenter branch from 33d0e0b to 6d9a3c5 Compare May 31, 2017 20:04

riyazdf approved these changes May 31, 2017

View reviewed changes

riyazdf merged commit abb19f8 into linuxkit:master May 31, 2017

thebsdbox deleted the push_vcenter branch June 1, 2017 06:11

thebsdbox mentioned this pull request Jun 1, 2017

Proposal: LinuxKit command to push vmware.vmdk to remote ESXi datastore #1751

Closed


		newVM.vmName = flags.String("vmname", "", "Specify a name for virtual Machine")
		newVM.vmFolder = flags.String("vmfolder", "", "Specify a name/folder for the virtual Machine to reside in")

vCenter push capability #1929

vCenter push capability #1929

Conversation

thebsdbox commented May 30, 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

riyazdf May 31, 2017 • edited

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

riyazdf left a comment

Choose a reason for hiding this comment

riyazdf May 31, 2017 • edited

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

riyazdf left a comment

Choose a reason for hiding this comment

riyazdf May 31, 2017 •

edited

riyazdf May 31, 2017 •

edited