Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add update support for network block in vcd_vapp_vm, allow force guest customization #310

Merged
merged 34 commits into from
Aug 22, 2019

Conversation

Didainius
Copy link
Collaborator

@Didainius Didainius commented Aug 12, 2019

Closed #299 Closes #300
This PR goes on top of https://github.com/vmware/go-vcloud-director/pull/229/files and adds two features:

  • Allows updating network configuration for vcd_vapp_vm
  • Introduces customization block with one field forcewhich allows to force guest customization on next terraform apply` (causing a VM to reboot).

Testing features:

  • To shorten test suite length some tests will be run in parallel. A new make command make seqtestacc is introduced to force running tests sequentially

Note The customization block was introduced as there is already demand to make other customization features available as the GUI has in "Guest customization" tab. The idea is to group all customization settings under this block in future.

@ghost ghost added size/XL and removed size/XXL labels Aug 12, 2019
@ghost ghost added size/XXL and removed size/XL labels Aug 14, 2019
vcd/resource_vcd_vapp_vm.go Outdated Show resolved Hide resolved
This field works as a flag and triggers force customization when `true` during an update
(`terraform apply`) every time. It never complains about a change in statefile. Can be used when guest customization
is needed after VM configuration (e.g. NIC change, customization options change, etc.) and then set back to `false`.
**Note.** It will not have effect when `power_on` field is set to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should throw warning from the code? @lvirbalas

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed this with Dainius Yesterday. Yes, we need, and ideally during plan, if possible, because it will rebooting VM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a Terraform validator which emits warning.
Here's how the terraform plan looks like (only when customization.force=true):

...
      + template_name                  = "LB"
      + vapp_name                      = "lb-test-web"
      + vdc                            = "tf_vdc"

      + customization {
          + force = true
        }
    }

Plan: 2 to add, 0 to change, 0 to destroy.

Warning: Using 'true' value for field 'vcd_vapp_vm.customization.force' will reboot VM on every 'terraform apply' operation



  on main.tf line 62, in resource "vcd_vapp_vm" "web":
  62: resource "vcd_vapp_vm" "web" {



------------------------------------------------------------------------

Note: You didn't specify an "-out" parameter to save this plan, so Terraform
can't guarantee that exactly these actions will be performed if
"terraform apply" is subsequently run.

When doing terraform apply however these warnings are not shown. I think I'm ok with that, but need other opinions.

The thing I like about using built in validator for warning is that it prints very nicely and points out location automatically while if I add a print to stdout manually I can't control it so nicely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding output for terraform validate as well:

$  terraform validate

Warning: Using 'true' value for field 'vcd_vapp_vm.customization.force' will reboot VM on every 'terraform apply' operation



  on main.tf line 62, in resource "vcd_vapp_vm" "web":
  62: resource "vcd_vapp_vm" "web" {


Success! The configuration is valid, but there were some validation warnings as shown above.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect. Does this get printed with apply without --auto-approve option too?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. That is the problem as I mentioned above.

When doing terraform apply however these warnings are not shown. I think I'm ok with that, but need other opinions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can write warning on apply phase using functionality from upload resource if we want that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I could, but the somewhat less nice thing is that it would be "somewhere in the middle" while the warning in validator prints it very clearly.

@ghost ghost added size/XL and removed size/XXL labels Aug 21, 2019
@ghost ghost added size/XXL and removed size/XL labels Aug 21, 2019
@Didainius
Copy link
Collaborator Author

The tests have become quite lengthy, especially with this PR so I'm experimenting with parallelizing test runs using resource.ParallelTest. During initial testing it looks as if it saves ±30mins (takes 100 mins instead of 133mins) with current commit.

@@ -106,6 +106,65 @@ resource "vcd_vapp_vm" "web2" {

```


## Example forced customization workflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about moving this sample to the ## Customization section below? This way it would appear contextually there where it's needed the most.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I somehow forced myself into thinking that we put examples on top. Should be better now.

Copy link
Contributor

@dataclouder dataclouder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Collaborator

@lvirbalas lvirbalas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A good example of how seemingly small request becomes a huge PR. LGTM now!

@Didainius Didainius merged commit e17b96c into vmware:master Aug 22, 2019
@Didainius Didainius deleted the vm-network-update branch August 22, 2019 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants