provider/vsphere: added update function with support for vcpu and memory #6356

thetuxkeeper · 2016-04-26T16:40:30Z

Since not all VMs should be rebuild from scratch if we want to change memory or vcpus (cluster masters, ...), I added an update function supporting these two settings to change with a reboot.

stack72 · 2016-04-26T18:39:23Z

This LGTM but I would rather one of the VMWare specific people took a look as well

chrislovecnm · 2016-04-27T16:05:28Z

@thetuxkeeper looks good, but can we have a unit test ;) Let me know if you are not familiar with the unit testing framework. @stack72 / @phinze / @jen20 does this project have a write up on how to write unit / integrations tests?

Also please update the provider docs that update capabilities are in the earily stages ... Here is how you do it.

If you have any questions holler! And again thanks for the work!!!!!

thetuxkeeper · 2016-04-27T16:30:29Z

@chrislovecnm i'm not familiar with unit/integration tests. Additionally my PRs are the only thing I did with go, so I'd need some support here (I know only the basic language features).
I'll try to understand how to write tests with what I can find tomorrow. But it would be great if someone could help me :)

stack72 · 2016-04-27T16:33:49Z

Hi @thetuxkeeper

There is a section of the CONTRIBUTING doc that has this - https://github.com/hashicorp/terraform/blob/master/.github/CONTRIBUTING.md#writing-acceptance-tests

P.

chrislovecnm · 2016-04-27T16:47:47Z

@thetuxkeeper look at the integration tests that have been written https://github.com/hashicorp/terraform/blob/master/builtin/providers/vsphere/resource_vsphere_virtual_machine_test.go

Please ask questions if yah got them!!

dkalleg · 2016-04-28T00:01:36Z

Hi @thetuxkeeper , looks like were forcing a reboot here. Not all updates would require that, so my comment is regarding future-proofing the resourceVSphereVirtualMachineUpdate function. Whether that work is done in this initial update patch or subsequent patches that need conditional reboot is debatable.

dkalleg · 2016-04-28T00:03:50Z

builtin/providers/vsphere/resource_vsphere_virtual_machine.go

+	for !toolsRunning {
+		time.Sleep(1000 * time.Millisecond)
+		toolsRunning, err = vm.IsToolsRunning(context.TODO())
+	}


Why do we wait for vm tools? Is this redundant with the WaitForIP() call?

@dkalleg good catch. @thetuxkeeper what exactly are we doing here?

I think it's a left over and I forgot to review it in detail. It should be redundant with WaitForIP(), since the vsphere clients won't show IPs without running tools either.
I'll remove it.

thetuxkeeper · 2016-04-28T09:09:20Z

@chrislovecnm and @stack72 thanks for the links. I'll try to get as far as I can and come back if I need help with the tests
@dkalleg Thanks for reviewing! True, there's no way to prevent a reboot if the VM has to be reconfigured, but doesn't need a reboot (reservations, resource pool,...). I'll think about it.

thetuxkeeper · 2016-04-28T12:23:23Z

Now with tests for changing memory and vcpu. Is this the right way?
The tests passed with a minimal installed centos 7 template

stack72 · 2016-04-28T12:30:02Z

builtin/providers/vsphere/resource_vsphere_virtual_machine_test.go

+	var locationOpt string
+	var datastoreOpt string
+
+	if v := os.Getenv("VSPHERE_DATACENTER"); v != "" {


If these are the standard env vars that are required to run the resources outside of the tests, then they are not needed here - they are taken care of in provider_test.go

provider_test.go only checks for VSPHERE_USER, VSPHERE_PASSWORD and VSPHERE_SERVER, but not VSPHERE_DATACENTER.

how would datacenter usually be sent to the VM? Would it be created and then passed as a resource?

With a vSphere server you can maintain multiple vSphere data centers. Below the data centers you usually have clusters containing physical hosts and resource pools. So you have to specify the data center at creation time of the VM since it's part of the location of the VM. Using (offline?) Vmotion you can move the VM from one data center to another.

stack72 · 2016-04-28T12:34:26Z

Hi @thetuxkeeper

the tests look ok - did you manage to run them? can you paste the output of the tests?

something like:

make testacc TEST=./builtin/providers/vsphere TESTARGS='-run=TestAccVSphereVirtualMachine_update'

would be able to run them

Paul

thetuxkeeper · 2016-04-28T12:45:08Z

Output of TestAccVSphereVirtualMachine_updateMemory:

# make testacc TEST=./builtin/providers/vsphere TESTARGS='-run=TestAccVSphereVirtualMachine_updateMemory'
==> Checking that code complies with gofmt requirements...
/home/dpossmann/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/vsphere -v -run=TestAccVSphereVirtualMachine_updateMemory -timeout 120m
=== RUN   TestAccVSphereVirtualMachine_updateMemory
--- PASS: TestAccVSphereVirtualMachine_updateMemory (128.29s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/vsphere        128.302s

Output of TestAccVSphereVirtualMachine_updateVcpu:

# make testacc TEST=./builtin/providers/vsphere TESTARGS='-run=TestAccVSphereVirtualMachine_updateVcpu'  
==> Checking that code complies with gofmt requirements...
/home/dpossmann/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
TF_ACC=1 go test ./builtin/providers/vsphere -v -run=TestAccVSphereVirtualMachine_updateVcpu -timeout 120m
=== RUN   TestAccVSphereVirtualMachine_updateVcpu
--- PASS: TestAccVSphereVirtualMachine_updateVcpu (136.40s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/vsphere        136.404s

thetuxkeeper · 2016-04-29T09:39:14Z

I added the the reboot flag to be able to skip the reboot, but I haven't found a parameter that's easy implement.
Resource pool needs another task and data structure (relocation). I pushed the preparation for that to another branch (https://github.com/godmodelabs/terraform/tree/vsphere-update-relocation)

I'll rebase it to current master (memory reservation commit is blocking the clean merge) when the open questions/issues are resolved.

markpeek · 2016-04-29T22:25:34Z

builtin/providers/vsphere/resource_vsphere_virtual_machine.go

+
+	if d.HasChange("memory") {
+		configSpec.MemoryMB = int64(d.Get("memory").(int))
+		hasChanges = true


Need to set rebootRequired = true here and for vcpu.

Thanks! It's fixed.
Missed that when Iwas splitting the patch (relocation branch an this one).

chrislovecnm · 2016-05-03T03:41:41Z

@thetuxkeeper can you resolve the conflicts? Once that is done, ping me at @markpeek and we will do a final code review

thetuxkeeper · 2016-05-03T12:23:28Z

@chrislovecnm, @markpeek: I rebased it to the master and the conflicts are gone (perhaps a cached status since the rebase had none). Should I squash it to a single commit?

chrislovecnm · 2016-05-03T15:53:32Z

@thetuxkeeper a squash would be great, then we should get this merged!! First part of update support 😄

stack72 · 2016-05-03T15:58:29Z

Thanks for all the work here :) I will squash as part of the merge

chrislovecnm · 2016-05-03T16:02:06Z

@stack72 thanks Paul ...

…ory (hashicorp#6356) * added update function with support for vcpu and memory * waiting for vmware tools redundant with WaitForIP * proper error handling of PowerOn task * added test cases for update memory and vcpu * reboot flag

ghost · 2020-04-26T01:53:52Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

stack72 added enhancement provider/vsphere labels Apr 26, 2016

mixacha mentioned this pull request Apr 27, 2016

vSphere Provider - VM Update Capability #6341

Closed

dkalleg reviewed Apr 28, 2016
View reviewed changes

stack72 reviewed Apr 28, 2016
View reviewed changes

markpeek reviewed Apr 29, 2016
View reviewed changes

thetuxkeeper added 5 commits May 3, 2016 13:57

added update function with support for vcpu and memory

3047a05

waiting for vmware tools redundant with WaitForIP

43b7aff

proper error handling of PowerOn task

2e803fe

added test cases for update memory and vcpu

c25ceeb

reboot flag

dc553a4

stack72 merged commit bb73c74 into hashicorp:master May 3, 2016

bobtfish mentioned this pull request May 5, 2016

Rebase from hashicorp/terraform Yelp/terraform#15

Merged

thetuxkeeper deleted the vsphere-update branch May 9, 2016 09:34

ghost locked and limited conversation to collaborators Apr 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/vsphere: added update function with support for vcpu and memory #6356

provider/vsphere: added update function with support for vcpu and memory #6356

thetuxkeeper commented Apr 26, 2016

stack72 commented Apr 26, 2016

chrislovecnm commented Apr 27, 2016

thetuxkeeper commented Apr 27, 2016

stack72 commented Apr 27, 2016

chrislovecnm commented Apr 27, 2016

dkalleg commented Apr 28, 2016

dkalleg Apr 28, 2016

chrislovecnm Apr 28, 2016

thetuxkeeper Apr 28, 2016

thetuxkeeper commented Apr 28, 2016

thetuxkeeper commented Apr 28, 2016

stack72 Apr 28, 2016

thetuxkeeper Apr 28, 2016

stack72 Apr 28, 2016

stack72 Apr 28, 2016

thetuxkeeper Apr 28, 2016

stack72 commented Apr 28, 2016

thetuxkeeper commented Apr 28, 2016

thetuxkeeper commented Apr 29, 2016

markpeek Apr 29, 2016

thetuxkeeper May 2, 2016

chrislovecnm commented May 3, 2016

thetuxkeeper commented May 3, 2016

chrislovecnm commented May 3, 2016

stack72 commented May 3, 2016

chrislovecnm commented May 3, 2016

ghost commented Apr 26, 2020

provider/vsphere: added update function with support for vcpu and memory #6356

provider/vsphere: added update function with support for vcpu and memory #6356

Conversation

thetuxkeeper commented Apr 26, 2016

stack72 commented Apr 26, 2016

chrislovecnm commented Apr 27, 2016

thetuxkeeper commented Apr 27, 2016

stack72 commented Apr 27, 2016

chrislovecnm commented Apr 27, 2016

dkalleg commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thetuxkeeper commented Apr 28, 2016

thetuxkeeper commented Apr 28, 2016

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

stack72 commented Apr 28, 2016

thetuxkeeper commented Apr 28, 2016

thetuxkeeper commented Apr 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrislovecnm commented May 3, 2016

thetuxkeeper commented May 3, 2016

chrislovecnm commented May 3, 2016

stack72 commented May 3, 2016

chrislovecnm commented May 3, 2016

ghost commented Apr 26, 2020