fix NPE in vic-machine caused by vm invalid state error #2992

emlin · 2016-11-03T01:06:17Z

Fixes #2818
For another kind of invalid state vm, vsphere might return empty vm.Config property, so govmomi will panic for NPE. This is fixed in #2977
This PR did following changes to fix this kind of failure:

Override govmomi vm.Properties method in vm.go, and fix the vm if that's in invalid state, cause this function is not enclosed by waitForResult method.
Fix waitForResult method, to handle non invalid state error, if vm is already in invalid state. This might include RPC send error.
Update urfave/cli vendor code, cause it does not hide all error stack in latest version.

hmahmood · 2016-11-03T15:50:29Z

pkg/vsphere/vm/vm.go

+			break
+		}
+	}
+	var newps []string


You can just do (no need for newps)

if !contains { ps = append(ps, connectionStatus) }

this is because I need to double check properties, and the next time, I don't need to add new properties. So I kept the old variable.

Do we actually need to check? What happens if we append "summary.runtime.connectionState" every time, irrespective of whether it's present already?

It doesn't matter to query duplicated properties, but as we're trying to make minimum properties query each time, it will help to reduce the array length.
And I think this array length is really short based on our current coding practice, I assume it doesn't hurt to loop that array.

hmahmood · 2016-11-03T15:51:50Z

pkg/vsphere/vm/vm.go

+	}
+	var newps []string
+	if !contains {
+		newps = append(ps, connectionStatus)


You are checking for summary above, but then appending on connectionStatus here? What about if connectionStatus is present in ps.

Also, the constant names are less intuitive than the actual strings; makes the code a little less readable imo.

any suggestion on the name?

I would not use the constants.

hmahmood · 2016-11-03T15:53:37Z

vendor/manifest

@@ -888,6 +888,14 @@
 			"branch": "v1"
 		},
 		{
+			"importpath": "gopkg.in/urfave/cli.v1",
+			"repository": "https://gopkg.in/urfave/cli.v1",


Where are we using this?

I don't know. I just use gvt to fetch urfave/cli latest commit, but it includes this one dynamically

I searched the urfave/cli code, itself has this dependency, so I assume I cannot remove this directory simply.

Looks like we should be using "gopkg.in/urfave/cli.v1" as the import path in our own code to avoid the duplicate vendoring of this package. If the NPE fix doesn't depend on updating this dependency, maybe the vendor update + import path change could be done in a separate PR?

Not directly related, just to improve the capability to print out NPE. I can separate it.

dougm · 2016-11-04T16:28:45Z

pkg/vsphere/vm/vm.go

 	log.Debugf("Fix invalid state VM: %s", vm.Reference())
 	folders, err := vm.Session.Datacenter.Folders(ctx)
 	if err != nil {
 		log.Errorf("Unable to get vm folder: %s", err)
 		return err
 	}

-	properties := []string{"config.files", "summary.config", "summary.runtime", "resourcePool", "parentVApp"}
+	properties := []string{"summary.config", "summary.runtime", "resourcePool", "parentVApp"}


While you're in there, looks like we only use Runtime.Host here, maybe limit that to "summary.runtime.host".

hickeng · 2016-11-04T22:10:34Z

pkg/vsphere/vm/vm_test.go

@@ -190,6 +191,13 @@ func TestVM(t *testing.T) {
 		t.Errorf("Failed to find fixed vm: %s", err)
 	}
 	assert.Equal(t, vm.Reference(), newVM.Reference())
+
+	// VM properties
+	var ovm mo.VirtualMachine


This is exactly the sort of test where we should be using vcsim to return InvalidState - @dougm is there an easy way to do this fault injection?

Yes, we are doing fault injection in this test for example: https://github.com/vmware/vic/blob/master/pkg/vsphere/tasks/waiter_test.go#L376

The test also implements methods that are not implemented by the simulator (MarkAsTemplate), similar might be needed here since Register/Unregister aren't currently implemented by the simulator.

@dougm the registervm methods are folder method or vApp method, how to inject the folder or vApp in simulator?

Added Register/Unregister: #3068

emlin · 2016-11-09T16:26:31Z

Changed fixed vm condition in WaitForResult for #2958

dougm · 2016-11-09T18:44:56Z

pkg/vsphere/vm/vm_test.go

+
+	//	// Inject invalid connection state to vm
+	ref := simulator.Map.Get(vmo.Reference()).(*simulator.VirtualMachine)
+	ref.Summary.Config.VmPathName = "[LocalDS_0] ha-host_VM0/ha-host_VM0.vmx"


Looks like we need to set that field in vcsim. You can also use this for now, rather than the hardcoded string:
ref.Config.Files.VmPathName

dougm · 2016-11-09T18:45:14Z

lgtm

This method will overrid govmomi method, to workaround invalid state vm issue

Update fixVM condition as well

emlin · 2016-11-12T03:04:16Z

lgtm

vmwclabot added the cla-not-required label Nov 3, 2016

emlin changed the title ~~fix NPE in vic-machine caused by vm invalid state error~~ [full ci]fix NPE in vic-machine caused by vm invalid state error Nov 3, 2016

emlin force-pushed the 2818 branch from f875d3f to 5d30db2 Compare November 3, 2016 15:29

hmahmood reviewed Nov 3, 2016

View reviewed changes

emlin changed the title ~~[full ci]fix NPE in vic-machine caused by vm invalid state error~~ fix NPE in vic-machine caused by vm invalid state error Nov 3, 2016

emlin force-pushed the 2818 branch from a26c936 to cdaad61 Compare November 3, 2016 21:06

dougm reviewed Nov 4, 2016

View reviewed changes

emlin force-pushed the 2818 branch from cdaad61 to 339697d Compare November 4, 2016 17:05

hickeng reviewed Nov 4, 2016

View reviewed changes

emlin force-pushed the 2818 branch 3 times, most recently from 2308708 to b1e845d Compare November 9, 2016 16:09

emlin changed the title ~~fix NPE in vic-machine caused by vm invalid state error~~ [full ci]fix NPE in vic-machine caused by vm invalid state error Nov 9, 2016

dougm reviewed Nov 9, 2016

View reviewed changes

emlin changed the title ~~[full ci]fix NPE in vic-machine caused by vm invalid state error~~ fix NPE in vic-machine caused by vm invalid state error Nov 9, 2016

emlin force-pushed the 2818 branch from cd866a4 to 4faf51d Compare November 9, 2016 18:56

emlin added 4 commits November 11, 2016 20:15

fix vm if connection status is invalid

d0192e1

Add Properties method

a6f11c5

This method will overrid govmomi method, to workaround invalid state vm issue

address comments

b23c02c

Test fix vm in WaitForResult and Properties

6e3fa56

Update fixVM condition as well

emlin force-pushed the 2818 branch from 861caf7 to a550156 Compare November 12, 2016 02:31

fix test failure caused by session change

a550156

emlin merged commit 6719689 into vmware:master Nov 12, 2016

emlin deleted the 2818 branch November 14, 2016 15:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix NPE in vic-machine caused by vm invalid state error #2992

fix NPE in vic-machine caused by vm invalid state error #2992

emlin commented Nov 3, 2016

hmahmood Nov 3, 2016 •

edited

Loading

emlin Nov 3, 2016

hickeng Nov 4, 2016

emlin Nov 7, 2016

hmahmood Nov 3, 2016 •

edited

Loading

emlin Nov 3, 2016

hmahmood Nov 4, 2016

hmahmood Nov 3, 2016

emlin Nov 3, 2016

emlin Nov 3, 2016

dougm Nov 4, 2016

emlin Nov 4, 2016

dougm Nov 4, 2016

hickeng Nov 4, 2016

dougm Nov 4, 2016

emlin Nov 7, 2016

dougm Nov 8, 2016

emlin commented Nov 9, 2016

dougm Nov 9, 2016

emlin Nov 9, 2016

dougm commented Nov 9, 2016

emlin commented Nov 12, 2016

fix NPE in vic-machine caused by vm invalid state error #2992

fix NPE in vic-machine caused by vm invalid state error #2992

Conversation

emlin commented Nov 3, 2016

hmahmood Nov 3, 2016 • edited Loading

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

hmahmood Nov 3, 2016 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlin commented Nov 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dougm commented Nov 9, 2016

emlin commented Nov 12, 2016

hmahmood Nov 3, 2016 •

edited

Loading

hmahmood Nov 3, 2016 •

edited

Loading