r/virtual_machine: Fix template UUID lookup for older vSphere versions #377
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Historically, the FindByUuid method under SearchIndex did not support looking up VMs marked as template. However, at some point in time, this was changed (possibly vSphere 6.5, based on bug reports), and any mention of the UUID search not supporting templates was removed from the API documentation, without a mention of when support for templates was introduced. Since this is the method that we use to look up both VMs and templates, this means that templates have not been functional for vSphere <= 6.0 users since the resource was re-written in 1.0. There has been a workaround in that the source template is just converted back to virtual machine, but this update fixes it so that on versions of vSphere older than 6.5, we fall back to using ContainerView with a filter on the UUID. This works for all versions of vSphere. We keep FindByUuid for 6.5 as it may possibly be more direct path.
vancluever
force-pushed
the
b-uuid-template-old-vsphere
branch
from
18c73ab to
0adde0b
Compare
|
Tests pass. We unfortunately can't test the |
paultyng
suggested changes
Jan 30, 2018
| func virtualMachineFromSearchIndex(client *govmomi.Client, uuid string) (object.Reference, error) { | ||
| log.Printf("[DEBUG] Using SearchIndex to look up UUID %q", uuid) | ||
| search := object.NewSearchIndex(client.Client) | ||
| ctx, cancel := context.WithTimeout(context.Background(), provider.DefaultAPITimeout) |
There was a problem hiding this comment.
I think it would be preferable to pass the ctx in and declare the Background up as high in the callstack as possible, in this case at lease up in FromUUID, to make switching to StopContext down the road easier.
| defer func() { | ||
| dctx, dcancel := context.WithTimeout(context.Background(), provider.DefaultAPITimeout) | ||
| defer dcancel() | ||
| v.Destroy(dctx) |
There was a problem hiding this comment.
If you don't care about the error, explicitly discard it, ie: _ = v.Destroy so it doesn't look like an oversight.
* Consolidated contexts (this will help streamline things when we add in support for exposing the provider contexts). * Added in error handling for the ContainerView destroy call. We don't expect this to error, so we panic if it does - this ensures it passes both errcheck and gas, which I am using now that I've adopted gometalinter. This may or may not be the standard going forward.
paultyng
approved these changes
Jan 31, 2018
paultyng
reviewed
Jan 31, 2018
|
|
||
| defer func() { | ||
| if err = v.Destroy(ctx); err != nil { | ||
| panic(err) |
There was a problem hiding this comment.
if you would rather not panic here, you could just wrap the body in an inner func, and do this afterwards, but seems fine to me as is, as its probably an edge.
For the destroy of the container view.
|
wewt wewt! When does |
|
@johnjelinek it's out now! |
ghost
locked and limited conversation to collaborators
# for free
to subscribe to this conversation on GitHub.
Already have an account?
#.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Historically, the
FindByUuidmethod underSearchIndexdid not supportlooking up VMs marked as template. However, at some point in time, this
was changed (possibly vSphere 6.5, based on bug reports), and any
mention of the UUID search not supporting templates was removed from the
API documentation, without a mention of when support for templates was
introduced. Since this is the method that we use to look up both VMs and
templates, this means that templates have not been functional for
vSphere <= 6.0 users since the resource was re-written in 1.0.
There has been a workaround in that the source template is just
converted back to virtual machine, but this update fixes it so that on
versions of vSphere older than 6.5, we fall back to using
ContainerViewwith a filter on the UUID. This works for all versions of vSphere. We
keep
FindByUuidfor 6.5 as it may possibly be more direct path.Fixes #271