WIP govc: Add virtual machine vApp configuration options #991
Conversation
|
I think I've gotten as far as I'm going to be able to get on this for the week (I have to turn my attention to TF stuff now). This is functional, the govc commands work (tested them against our lab), the simulator code has been added but still needs to be tested, and the rest of the tests need to be in, which all in should hopefully not be a huge work commitment. |
|
Awesome thanks @vancluever ! I will review by tomorrow. |
There was a problem hiding this comment.
Looking good @vancluever , just a few minor comments. Happy to do a closer review and try it out when you're ready.
|
|
||
| // enum is a value store that validates against a set of strings. The | ||
| // validation is performed at set-time. | ||
| type enum struct { |
There was a problem hiding this comment.
Nice. We can make use of these in some of the other commands too. At some point I'd like to ditch our custom CLI framework for something else, but not in a rush to do that.
govc/vm/vapp/configure.go
Outdated
| return err | ||
| } | ||
|
|
||
| if err := task.Wait(ctx); err != nil { |
There was a problem hiding this comment.
Could use cmd.ProgressLogger here.
govc/vm/vapp/configure.go
Outdated
| } | ||
|
|
||
| func init() { | ||
| cli.Register("vm.vapp.configure", &configure{}) |
There was a problem hiding this comment.
Other Reconfigure_Task based commands are named "change", vm.change, cluster.change, etc. I wouldn't object to changing that name (no pun intended) in the future, but would prefer to keep it consistent for now.
govc/vm/vapp/property_add_edit.go
Outdated
| return err | ||
| } | ||
|
|
||
| if err := task.Wait(ctx); err != nil { |
simulator/virtual_machine_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func testBoolPtr(b bool) *bool { |
|
Thanks @dougm! Yeah I'm just trying to get coverage on the tests right now, balancing work/life at the same time. Hopefully will have more later this week. Thanks for the note on the type helpers - I totally missed those - might be able to clean up some TF code with that too 🙂 |
|
Hey all, just wanted to mention I haven't forgotten about this, it just wasn't as urgent for our process as it was a while ago now that hashicorp/terraform-provider-vsphere#303 went live, and my personal time has been quite limited as of late. I still want to complete this though, and hopefully will have the spare time to do it soon! |
|
@vancluever totally understand, take your time man. |
|
Hey @vancluever any chance you will be able to finish this off soon? As you saw from hashicorp/terraform-provider-vsphere#489 I need to turn off vApp options on my templates (which get respun with security updates relatively frequently) and would love to be able to use govc to do that. Thanks :) |
|
@bpoland I've been slowly working through my backlog of personal things, so hopefully I'll have a chance to finish this off soon! So yeah, I will be trying sometime in the next couple of weeks to close this off. |
|
Ok thanks @vancluever for this and all your work on the vsphere provider -- it is much appreciated! |
|
Hey everyone! I've started to work on this again - I'm just starting with addressing @dougm's review comments and finishing up the vApp property tests on the simulator, which should allow me to make progress on finishing up the rest of the simulator stuff soon. After that, I will move on to the rest of the testing stuff. Thanks! |
|
Simulator tests are finally done! Moving on to bats tests now, in addition to looping back on an interface for adding EULAs. I added it to |
This subcommand removes vApp configuration from a virtual machine. The current reasoning: Terraform currently lacks vApp configuration support. This is being worked on in hashicorp/terraform-provider-vsphere#303, but in the meantime I'm currently working on some code (not necessarily related to the work going on in the provider) that needs to leverage guestinfo in a CoreOS template. Working with these attributes in extraConfig is not possible if vApp configuration is present. This is functional and has been manually tested, but I'm going to look at adding the rest of the functionality to govc so that vApp properties can be largely manipulated within govc itself. This will require work to both govc and the simulator. Reference: #574
Adding the following commands: * vm.vapp.info, displaying general vApp and product details * vm.vapp.property.info, displaying information for select properties * vm.vapp.configure, allowing the configuration of all vApp options except for properties (these are coming in a single CRUD-style fashion). Also, extended the flag helpers by adding the following: * String helpers for optional string pointers and string slices * Enumerated helpers for when validation is needed on string and string slice values
Basically working off of the same workflow, so the same behaviour can be used for both operations with some slight divergences. Basically all that remains before working with the simulator and other tests (namely for the flag stuff) is deletion of properties (vm.vapp.property.delete).
The enumeration for the supported IP protocols example is invalid, make sure the capitalization was correct as it would not work in that form (govc would kick back an error).
Missed this as I was using goland for a little bit which lacks the ability to easily wrap comment lines on the fly, or else I just don't know how to do it like I do in vim. :P
This completes the vApp property configuration part of this work - all that remains is the tests and simulator work.
This adds virtual machine vApp configuration support to the simulator, supporting all fields that we currently allow with govc (which is pretty much everything except EULA and unrecognized OVF sections). Tests still need to be created for this, and tests for the govc functionality still need to be added to the bats tests.
For the property stuff - moving from the bottom up, after the property operations are done will move on to the product config and then general options.
* vm.vapp.configure is now vm.vapp.change, to match the rest of the VM reconfigure commands in govc. * Output for modification operations now uses ProgressLogger for better output. * Adjusted bool pointer generation in various points in the simulator to use types.NewBool.
Add the top-level property tests for edit and remove. This should complete the full path test and allow us to move on to adding the additional support to the simulator, namely the product config and general option sections.
This provides the coverage for the remaining vApp property support for virtual machines in the simulator. In addition to that, EULA support has been added in both the simulator and govc. I'm thinking about making the CLI support a bit better though as right now it's currently just a command line argument, which may be unwieldy with larger EULAs, in addition to the fact that there's no CLI support right now for *viewing* EULAs.
Some of these panic right now due to issues with fetching vApp configuration in the simulator when the configuration is unset - namely the output (nil) is not being sent in a format that the property readers in vim25/mo seem to like. Still trying to figure this out.
vm.log.Printf was removed a while ago - changing this to log.Printf instead to bring it in line with the other instances of logging.
|
@vancluever hi Chris, still interested in completing this PR? |
|
@frapposelli I think the truth of the matter is that I've had very little personal time to devote to non-work related projects lately, so I probably won't have the bandwidth to do this anytime soon. I've actually been meaning to post a message about this for a while. If anyone wants to take it over, I'm totally fine with that. Honestly the last things left to do to fix/add are the two remaining tasks described by the original comment (tests), and then it should be good to go. |
|
Would still love to see this implemented - extremely useful for distros and cloud-images that take advantage of the Currently solutions like ClusterAPI for vSphere have to use custom cloud-init datasources to work around this meaning we have to build baked images for them, rather than letting users roll their own. |
|
@mylesagray is there a related CAPV issue open for this? |
|
Any plans to get this completed? I have a use case where I need to add vApp properties to a bunch of VMs as part of a Openshift cluster deployment on vSphere. |
|
When can we expect this to be completed? |
|
This will be amazing for my VM builds. I recently replaced packer by using the GOVC commands, except the one thing missing is modifying vApp options. |
|
+1 |
embano1
changed the title
|
@dougm this PR is 3y old but there's still interest in the functionality. Since the PR itself is likely outdated, I suggest closing it and let interested users raise this in an issue for further discussion. |
|
Hey all, I'm closing this to clear out my old PR backlog. Will leave the repository open for now, but if anyone wants to carry on my work please clone it sooner rather than later as I may end up deleting my local fork. |
|
@vancluever I just went to clone it now but your fork is gone. Sorry we hadn't been able to pick up where you left off, but it still might be possible to if you can still make your changes available in some form if you still have it locally. Even patch in a gist would be good. Thanks! |
|
@dougm I thought I still had this repo, but apparently I deleted it in a security cleanup over a month ago. Sorry 😞 I really meant to keep it. I'm going to keep this bookmarked if I find it on an older workstation I'll make sure to re-push it. Really sorry again! |
|
The PRs content was still available in branch |
|
Thank you very much @janitha!!! Good to know this trick too just in case this happens again. Thanks again! |
|
Is it possible to to remove all vapp properties from a VM without this PR? |
This PR (still WIP) adds/aims to add sub-commands to manage vApp settings on an individual virtual machine, useful for things like OVF authoring or if you want to use TF in its current state with OVAs that support things like cloud-init, ie: CoreOS - this allows you to remove vApp config so you can manage
guestinfodirectly. Of course, you could just use this to manage the VM directly too.This also adds some flag helpers to the
flagspackage, namely having to do with handling optional string values, string slices, and enumerations where validation is necessary (it doesn't look like vSphere properly kicks back an error on some improper vApp configuration options, like if you use lower case IPv4/IPv6 enumerations in the supported IP protocols). I didn't end up using some of these but I figured they'd be useful and will add tests to make sure they are properly accounted for.Things present:
vm.vapp.info)vm.vapp.configure)vm.vapp.property.info)vm.vapp.property.add)vm.vapp.property.edit)vm.vapp.remove)Things still remaining as of the evening of Jan 9th:
vm.vapp.property.remove)Related: #574