Inspect all property changes before exiting WaitForKeyInExtraConfig #5938
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
sflxn
force-pushed
the
nonexistent_group
branch
from
6784243 to
2e7e599
Compare
emlin
reviewed
Aug 9, 2017
pkg/vsphere/vm/vm.go
Outdated
| err := vm.WaitForExtraConfig(ctx, waitFunc) | ||
| if err == nil && poweredOff != nil { | ||
| if err == nil && poweredOff != nil && detail == "" { |
There was a problem hiding this comment.
I think the check detail == "" can help to avoid powerdOff error if the value is already set by tether.
But what's the benefit to set exitEarly above? If you want, maybe you could just return at line 202, https://github.com/vmware/vic/pull/5938/files#diff-818b18702ad1cb5203eeaccc54071e09R202
|
Simpler fix would be to swap the property order here: https://github.com/vmware/vic/blob/master/pkg/vsphere/vm/vm.go#L176 So if we get both properties in the same update set, we examine the extraConfig before the powerState. |
|
Thanks Doug. I was wondering if I could do that also, but I wasn't sure if that actually affected the ordering. |
|
The test will need fixing too: modified pkg/vsphere/vm/vm_test.go
@@ -339,7 +339,6 @@ func TestWaitForKeyInExtraConfig(t *testing.T) {
opt := &types.OptionValue{Key: "foo", Value: "bar"}
obj := simulator.Map.Get(vm.Reference()).(*simulator.VirtualMachine)
- obj.Config.ExtraConfig = append(obj.Config.ExtraConfig, opt)
val, err := vm.WaitForKeyInExtraConfig(ctx, opt.Key)
@@ -347,6 +346,7 @@ func TestWaitForKeyInExtraConfig(t *testing.T) {
t.Error("expected error")
}
+ obj.Config.ExtraConfig = append(obj.Config.ExtraConfig, opt)
obj.Summary.Runtime.PowerState = types.VirtualMachinePowerStatePoweredOn
val, err = vm.WaitForKeyInExtraConfig(ctx, opt.Key) |
vburenin
reviewed
Aug 14, 2017
pkg/vsphere/vm/vm.go
Outdated
| @@ -198,30 +199,30 @@ func (vm *VirtualMachine) WaitForKeyInExtraConfig(ctx context.Context, key strin | |||
| if key == value.GetOptionValue().Key { | |||
| detail = value.GetOptionValue().Value.(string) | |||
| if detail != "" && detail != "<nil>" { | |||
| return true | |||
| exitEarly = true | |||
| } | |||
| break // continue the outer loop as we may have a powerState change too | |||
There was a problem hiding this comment.
break is not needed here.
vburenin
reviewed
Aug 14, 2017
pkg/vsphere/vm/vm.go
Outdated
| @@ -198,30 +199,30 @@ func (vm *VirtualMachine) WaitForKeyInExtraConfig(ctx context.Context, key strin | |||
| if key == value.GetOptionValue().Key { | |||
| detail = value.GetOptionValue().Value.(string) | |||
| if detail != "" && detail != "<nil>" { | |||
| return true | |||
| exitEarly = true | |||
There was a problem hiding this comment.
How does it help to iterate over all values? It literally looks like you just need to exist in place where you assign true.
sflxn
force-pushed
the
nonexistent_group
branch
2 times, most recently
from
1ad8165 to
808b23d
Compare
dougm
approved these changes
Aug 15, 2017
emlin
approved these changes
Aug 15, 2017
sflxn
force-pushed
the
nonexistent_group
branch
3 times, most recently
from
f6be904 to
07d9197
Compare
There are cases where a container starts up and shuts down very quickly. We treated this case as early termination because the tether failed to initialize. However, in quick to shut down containers, the tether may have initialized and written out return codes, but we read the power off before we read the codes. We now read the status code before the power state changes in WaitForKeyInExtraConfig. This should allow us to not misinterpret quick ending containers. Resolves vmware#5629
AngieCris
pushed a commit
to AngieCris/vic
that referenced
this pull request
Nov 20, 2017
…mware#5938) There are cases where a container starts up and shuts down very quickly. We treated this case as early termination because the tether failed to initialize. However, in quick to shut down containers, the tether may have initialized and written out return codes, but we read the power off before we read the codes. We now read the status code before the power state changes in WaitForKeyInExtraConfig. This should allow us to not misinterpret quick ending containers. Resolves vmware#5629
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
There are cases where a container starts up and shuts down very quickly. We
treated this case as early termination because the tether failed to initialize.
However, in quick to shut down containers, the tether may have initialized and
written out return codes, but we read the power off before we read the codes.
We now read the status code before the power state changes in WaitForKeyInExtraConfig.
This should allow us to not misinterpret quick ending containers.
Resolves #5629