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

provider/vsphere: wait for network enhanced #6377

Merged
merged 3 commits into from
May 17, 2016
Merged

provider/vsphere: wait for network enhanced #6377

merged 3 commits into from
May 17, 2016

Conversation

thetuxkeeper
Copy link
Contributor

I did some changes to the "wait for network" logic to make it more flexible and add the feature to wait for Guest.Net (takes ~30s longer than Summary.Guest.IpAddress in our environment)

  • boot_delay with default value
  • added network_wait_timeout which was previously hard coded to 600s. Changed the check for boot_delay to network_wait_timeout > 0
  • added wait_for_network_interfaces to make it possible to wait for Guest.Net (the network interfaces)
  • added the necessary logic to waitForNetworkingActive

I tried not to change the behavior, so wait_for_network_interfaces is set to false and boot_delay to 0 by default.

@stumbaumr
Copy link
Contributor

This solves #6174 for us by the way...

@@ -740,7 +770,7 @@ func resourceVSphereVirtualMachineDelete(d *schema.ResourceData, meta interface{
return nil
}

func waitForNetworkingActive(client *govmomi.Client, datacenter, name string) resource.StateRefreshFunc {
func waitForNetworkingActive(client *govmomi.Client, datacenter, name string, waitForNetworkInterfaces bool) resource.StateRefreshFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this would https://github.com/dougm/govmomi/blob/388df2f1d8cb5a0059850fdf10520df7923474ef/object/virtual_machine.go#L252 work?

I would rather use the API to wait for interfaces than maintain our own wait code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wasn't merged, when I started my work and we need this feature to start using terraform. But I think it could replace this change and additionally all WaitForIP and waitForNetworkingActive calls.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please update this PR to use this. We will also need a unit test 😄 Pretty please.

Copy link
Contributor

Choose a reason for hiding this comment

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

@thetuxkeeper btw this is a HUGE help addressing this. Much appreciated!!!

@chrislovecnm
Copy link
Contributor

chrislovecnm commented May 2, 2016

Appreciate the work!!

Do we have a use case where we would not wait for the nics? I am thinking that I would rather make it wait by default.

@Shruti29 @matt-deboer @benlangfeld @getstek thoughts?

@chrislovecnm
Copy link
Contributor

@thetuxkeeper might we have a unit test?

@@ -74,6 +74,8 @@ The following arguments are supported:
* `dns_suffixes` - (Optional) List of name resolution suffixes for the virtual network adapter
* `dns_servers` - (Optional) List of DNS servers for the virtual network adapter; defaults to 8.8.8.8, 8.8.4.4
* `network_interface` - (Required) Configures virtual network interfaces; see [Network Interfaces](#network-interfaces) below for details.
* `wait_for_network_interfaces` - (Optional) Specifies if we should wait for at least one network interface to appear and not only for the guest ip address
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add something around default behavior. Also a note that you may need this for DHCP ...

Copy link
Contributor Author

@thetuxkeeper thetuxkeeper May 2, 2016

Choose a reason for hiding this comment

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

I was thinking of giving the choice if you want to wait for the interfaces (wait_for_network_interfaces set to true) or if you only need the "WaitForIP" functionality (wait_for_network_interfaces 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.

I don't know of a use cases where we would not want the interfaces 😄 So I would default everything to true. But the docs need to spell this out clearly.

@thetuxkeeper
Copy link
Contributor Author

Will push some fixes since I forgot to let it build before pushing ...

@thetuxkeeper
Copy link
Contributor Author

thetuxkeeper commented May 3, 2016

@chrislovecnm : I tried to update the govmomi dependency to tag v0.6.1, but failed (using this guide https://github.com/hashicorp/terraform/blob/master/README.md#updating-a-dependency)

# godep update github.com/vmware/govmomi
godep: no packages can be updated

@chrislovecnm
Copy link
Contributor

chrislovecnm commented May 3, 2016

@thetuxkeeper let me take a look later today, or by tomorrow, at getting the update to work.

@jen20 @stack72 - we need to get govmomi updated to v0.6.1, and @thetuxkeeper is getting an error. How strong is your godep kungfu?

@chrislovecnm
Copy link
Contributor

@dkalleg can you and @thetuxkeeper guys work on another PR for the version bump? We should be able to get that in quickly.

@chrislovecnm
Copy link
Contributor

I am getting an error trying to update govmomi as well.

$ godep restore
godep: error restoring dep (github.com/aws/aws-sdk-go/service/cloudfront): Wanted to restore rev 6876e9922ff299adf36e43e04c94820077968b3b, already restored rev e7cf1e5986499eea7d4a87868f1eb578c8f2045a for another package in the repo
godep: Error restoring some deps. Aborting check.

@chrislovecnm
Copy link
Contributor

I filed an issue about the error: #6464

@thetuxkeeper
Copy link
Contributor Author

@chrislovecnm: I found the reason for the "no packages can be updated" message. You have to use godep update github.com/vmware/govmomi/... (with the ... like in the readme). I think this is the same problem: tools/godep#164
I'm quite new to go and I usually read ... as "etc" not as part of the command or as recursive operator. So the documentation is correct.

@chrislovecnm
Copy link
Contributor

@thetuxkeeper ... Are you cutting a separate PR?

@thetuxkeeper
Copy link
Contributor Author

thetuxkeeper commented May 4, 2016

@chrislovecnm : I started implementing some changes for v0.6.1 but haven't pushed it yet (will be a new PR). I ran into some problems with other commits.

  • reservation not implemented in deploy function, only create => unit tests fail
  • ipv6 somehow wants to update ("Computed" is not set and we get a link-local address even without dhcp) => unit tests fail ("not empty plan")

That's the current status and since all just happened a short while ago, there's no issue/PR yet.

@chrislovecnm
Copy link
Contributor

@thetuxkeeper can you mark with WIP? This depends on the gomomi upgrade ;)

@thetuxkeeper thetuxkeeper changed the title provider/vsphere: wait for network enhanced [WIP] provider/vsphere: wait for network enhanced May 6, 2016
@benlangfeld
Copy link

@chrislovecnm What still remains to be done here?

@chrislovecnm
Copy link
Contributor

@benlangfeld we need the govmomi upgrade merged. At the airport so I will check on it in a bit.

@stack72
Copy link
Contributor

stack72 commented May 12, 2016

@chrislovecnm what is the risk of merging the govmomi PR? We can do that as we are on Terrafor 0.7 and it won't be released for a while and we can check the tests

thoughts?

@chrislovecnm
Copy link
Contributor

@stack72 I asked @thetuxkeeper that question on #6479

Thanks Chris ... appreciate the advice 👍

- removed duplicate wait for network parts
@thetuxkeeper
Copy link
Contributor Author

Did a rebase onto master. Will run the acceptance tests later (leaving the office now)

@dkalleg
Copy link
Contributor

dkalleg commented May 17, 2016

Used this on Friday (after manually rebasing on my end) and works as advertised. Solves a common bug, I'm eager to see this merge. @chrislovecnm @stack72 any word on eta? thx.

@chrislovecnm
Copy link
Contributor

@thetuxkeeper we ready for a merge?

@stack72 @jen20 @phinze once @thetuxkeeper changes it from [WIP] can we get a merge?

@thetuxkeeper
Copy link
Contributor Author

Acceptance test results look good on my side. I'll remove [WIP] then please review :)

==> Checking that code complies with gofmt requirements...
/home/tux/go/bin/stringer
go generate $(go list ./... | grep -v /vendor/)
2016/05/17 19:01:33 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/vsphere -v  -timeout 120m
=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccVSphereFile_basic
Expected error received: cannot stat '[DS1] tf_file_test.vmdk': No such file
--- PASS: TestAccVSphereFile_basic (9.27s)
=== RUN   TestAccVSphereFile_renamePostCreation
Expected error received: cannot stat '[DS1] tf_test_file_moved.vmdk': No such file
Expected error received: cannot stat '[DS1] tf_test_file.vmdk': No such file
--- PASS: TestAccVSphereFile_renamePostCreation (15.48s)
=== RUN   TestAccVSphereFolder_basic
--- PASS: TestAccVSphereFolder_basic (6.18s)
=== RUN   TestAccVSphereFolder_nested
--- PASS: TestAccVSphereFolder_nested (27.85s)
=== RUN   TestAccVSphereFolder_dontDeleteExisting
--- PASS: TestAccVSphereFolder_dontDeleteExisting (7.37s)
=== RUN   TestAccVSphereVirtualMachine_basic
--- PASS: TestAccVSphereVirtualMachine_basic (104.96s)
=== RUN   TestAccVSphereVirtualMachine_diskInitType
--- PASS: TestAccVSphereVirtualMachine_diskInitType (99.72s)
=== RUN   TestAccVSphereVirtualMachine_dhcp
--- PASS: TestAccVSphereVirtualMachine_dhcp (89.75s)
=== RUN   TestAccVSphereVirtualMachine_custom_configs
--- PASS: TestAccVSphereVirtualMachine_custom_configs (90.49s)
=== RUN   TestAccVSphereVirtualMachine_createInExistingFolder
--- PASS: TestAccVSphereVirtualMachine_createInExistingFolder (89.80s)
=== RUN   TestAccVSphereVirtualMachine_createWithFolder
--- PASS: TestAccVSphereVirtualMachine_createWithFolder (98.80s)
=== RUN   TestAccVSphereVirtualMachine_createWithCdrom
--- PASS: TestAccVSphereVirtualMachine_createWithCdrom (93.50s)
=== RUN   TestAccVSphereVirtualMachine_createWithExistingVmdk
--- PASS: TestAccVSphereVirtualMachine_createWithExistingVmdk (62.83s)
=== RUN   TestAccVSphereVirtualMachine_updateMemory
--- PASS: TestAccVSphereVirtualMachine_updateMemory (161.11s)
=== RUN   TestAccVSphereVirtualMachine_updateVcpu
--- PASS: TestAccVSphereVirtualMachine_updateVcpu (154.21s)
=== RUN   TestAccVSphereVirtualMachine_ipv4Andipv6
--- PASS: TestAccVSphereVirtualMachine_ipv4Andipv6 (109.22s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/vsphere        1220.551s

@thetuxkeeper thetuxkeeper changed the title [WIP] provider/vsphere: wait for network enhanced provider/vsphere: wait for network enhanced May 17, 2016
@chrislovecnm
Copy link
Contributor

chrislovecnm commented May 17, 2016

@jen20 @stack72 @phinze good sirs ... Can we get a merge. This fixes about four bugs

Nice work @thetuxkeeper ... appreciated!!

@thetuxkeeper
Copy link
Contributor Author

@stack72: thanks! :)

@chrislovecnm
Copy link
Contributor

@thetuxkeeper that may have closed about four issues :) ... WOOP!!

ojongerius added a commit to atlassian/terraform that referenced this pull request May 18, 2016
…equire_full_window_and_locked

* upstream/master: (665 commits)
  merged createVirtualMachine and deployVirtualMachine to setupVirtualMachine (hashicorp#6659)
  Update CHANGELOG.md
  provider/fastly: add support for custom VCL configuration (supersedes hashicorp#6587) (hashicorp#6662)
  Remove CHANGELOG entry for backported 0.6.17 feature
  Update CHANGELOG.md
  provider/vsphere: wait for network enhanced (hashicorp#6377)
  Update CHANGELOG.md
  Update CHANGELOG.md
  Update CHANGELOG.md
  docs: clarify an internal-plugins header
  Fixes an vet error.
  Update CHANGELOG.md
  provider/aws: Support for Redshift Cluster encryption using a KMS key (hashicorp#6712)
  provider/aws: Randomize key names in KMS alias test
  Include the list of allowed values for AWS auto scaling group termination policies (hashicorp#6710)
  website: docs for azurerm custom images
  Update CHANGELOG.md
  Godeps: rm dup github.com/ryanuber/columnize
  Add note about paid training
  provider/aws: Fix crash in ElastiCache param group
  ...
cristicalin pushed a commit to cristicalin/terraform that referenced this pull request May 24, 2016
* - use WaitForNetIP
- removed duplicate wait for network parts

* gofmt fix

* fixes
@ghost
Copy link

ghost commented Apr 25, 2020

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.

@ghost ghost locked and limited conversation to collaborators Apr 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants