Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

alexander-demicev · 2020-05-19T11:56:10Z

A follow-up for #585

PR adds similar logic for datastores and folder + unit test coverage increase

JoelSpeed · 2020-05-19T12:17:55Z

pkg/controller/vsphere/reconciler.go

+	var multipleFoundError *find.MultipleFoundError
+	var notFoundError *find.NotFoundError
+	if errors.As(vsphereError, &multipleFoundError) {
+		return machinecontroller.InvalidMachineConfiguration(multipleFoundMsg)
+	}
+
+	if errors.As(vsphereError, &notFoundError) {
+		return machinecontroller.InvalidMachineConfiguration(notFoundMsg)
+	}


Suggested change

var multipleFoundError *find.MultipleFoundError

var notFoundError *find.NotFoundError

if errors.As(vsphereError, &multipleFoundError) {

return machinecontroller.InvalidMachineConfiguration(multipleFoundMsg)

}

if errors.As(vsphereError, &notFoundError) {

return machinecontroller.InvalidMachineConfiguration(notFoundMsg)

}

var multipleFoundError *find.MultipleFoundError

if errors.As(vsphereError, &multipleFoundError) {

return machinecontroller.InvalidMachineConfiguration(multipleFoundMsg)

}

var notFoundError *find.NotFoundError

if errors.As(vsphereError, &notFoundError) {

return machinecontroller.InvalidMachineConfiguration(notFoundMsg)

}

JoelSpeed · 2020-05-19T12:22:11Z

pkg/controller/vsphere/reconciler_test.go

@@ -95,130 +96,234 @@ func TestClone(t *testing.T) {
 		},
 	}

+	// Set this value to 2 because it's the default number on machine in mocked environment


Suggested change

// Set this value to 2 because it's the default number on machine in mocked environment

// Set this value to 2 because it's the default number of machines in a mocked environment

JoelSpeed · 2020-05-19T12:24:10Z

pkg/controller/vsphere/reconciler_test.go

+		testCase              string
+		cloneVM               bool
+		expectedError         error
+		setupFailureCondition func()


Would be better to return an error from this func and then t.Fatal, at the moment you are calling t.Fatal on the whole test t, not on the subtest t

JoelSpeed · 2020-05-19T12:27:25Z

pkg/controller/vsphere/reconciler_test.go

+				vmsCount++
+				if vmsCount != len(vms) {
+					t.Errorf("Unexpected number of machines. Expected: %v, got: %v", vmsCount, len(vms))
+				}


This looks a bit odd, should there be a loop or something here?

What do you mean by loop? That part of test checks that instance was created after calling clone() by comparing the number of instances before and after the call.

So the loop here is the for _, tc := range testCases, I wonder if there is a way to check that the new VM was created without having a count specifically? This vmsCount is shared across multiple subtests, so if one of them fails for some reason, that could have a knock on effect for the other subtests, so we would potentially have difficulty tracking down the problem. Ideally each subtest should share as little as possible/nothing if possible, so there isn't dependencies between them.

Does clone return the newly created VM? Could we check that exists in the VMList instead?

JoelSpeed · 2020-05-20T13:10:33Z

pkg/controller/vsphere/reconciler.go

@@ -677,6 +675,20 @@ func setProviderStatus(taskRef string, condition vspherev1.VSphereMachineProvide
 	return nil
 }

+func handleVsphereError(multipleFoundMsg, notFoundMsg string, defaultError, vsphereError error) error {


I think elsewhere this is capitalised as VSphere, do we have a pattern or is it mixed everywhere?

right, it should be VSphere

JoelSpeed · 2020-05-20T13:14:19Z

pkg/controller/vsphere/reconciler_test.go

+				vmsCount++
+				if vmsCount != len(vms) {
+					t.Errorf("Unexpected number of machines. Expected: %v, got: %v", vmsCount, len(vms))
+				}


So the loop here is the for _, tc := range testCases, I wonder if there is a way to check that the new VM was created without having a count specifically? This vmsCount is shared across multiple subtests, so if one of them fails for some reason, that could have a knock on effect for the other subtests, so we would potentially have difficulty tracking down the problem. Ideally each subtest should share as little as possible/nothing if possible, so there isn't dependencies between them.

Does clone return the newly created VM? Could we check that exists in the VMList instead?

alexander-demicev · 2020-05-21T09:49:07Z

@JoelSpeed I updated the unit test. Instead of counting VMs it now checks that returned value is not empty, which should happen only if clone task was started successfully.

JoelSpeed

/approve

Thanks

openshift-ci-robot · 2020-05-21T09:58:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enxebre · 2020-05-21T10:06:47Z

pkg/controller/vsphere/reconciler_test.go

+			expectedError: errors.New("multiple resource pools found, specify one in config"),
+			setupFailureCondition: func() error {
+				// Create resource pools
+				defaultResourcePool, err := session.Finder.ResourcePool(context.Background(), "/DC0/host/DC0_C0/Resources")


is this magic value "/DC0/host/DC0_C0/Resources" completely arbitrary? may be add a comment explaining where is it coming from?

That's not a magic value, just the default value that 'simulator' has set. I had to debug this a bit.

enxebre · 2020-05-21T10:07:21Z

awesome @alexander-demichev, thanks!
/lgtm

openshift-bot · 2020-05-21T11:36:07Z

/retest

Please review the full test history for this PR and help us cut down flakes.

enxebre · 2020-05-21T18:43:58Z

/retitle Bug 1838504: [vSphere] Fail machine on invalid provider spec values

enxebre · 2020-05-21T18:44:12Z

/bugzilla refresh

openshift-ci-robot · 2020-05-21T18:44:13Z

@enxebre: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-05-21T18:44:17Z

@alexander-demichev: This pull request references Bugzilla bug 1838504, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1838504: [vSphere] Fail machine on invalid provider spec values

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

enxebre · 2020-05-21T18:44:58Z

/bugzilla refresh

openshift-ci-robot · 2020-05-21T18:45:00Z

@enxebre: This pull request references Bugzilla bug 1838504, which is invalid:

expected the bug to target the "4.5.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

enxebre · 2020-05-21T18:45:30Z

/bugzilla refres

enxebre · 2020-05-21T18:45:35Z

/bugzilla refresh

openshift-ci-robot · 2020-05-21T18:45:40Z

@enxebre: This pull request references Bugzilla bug 1838504, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-05-21T18:49:49Z

@alexander-demichev: All pull requests linked via external trackers have merged: openshift/machine-api-operator#593. Bugzilla bug 1838504 has been moved to the MODIFIED state.

In response to this:

Bug 1838504: [vSphere] Fail machine on invalid provider spec values

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from elmiko and michaelgugino May 19, 2020 11:56

JoelSpeed reviewed May 19, 2020

View reviewed changes

alexander-demicev force-pushed the errors branch from 6672ec4 to a00814b Compare May 20, 2020 09:13

JoelSpeed reviewed May 20, 2020

View reviewed changes

[vSphere] Fail machine on invalid provider spec value

656e287

alexander-demicev force-pushed the errors branch from a00814b to 656e287 Compare May 21, 2020 09:47

alexander-demicev changed the title ~~[vSphere] Fail machine on invalid provider spec values~~ Bug 1838504 - [vSphere] Fail machine on invalid provider spec values May 21, 2020

JoelSpeed approved these changes May 21, 2020

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2020

enxebre reviewed May 21, 2020

View reviewed changes

openshift-ci-robot assigned enxebre May 21, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 21, 2020

openshift-ci-robot changed the title ~~Bug 1838504 - [vSphere] Fail machine on invalid provider spec values~~ Bug 1838504: [vSphere] Fail machine on invalid provider spec values May 21, 2020

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label May 21, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 21, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label May 21, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label May 21, 2020

openshift-merge-robot merged commit 6dd4c3b into openshift:master May 21, 2020

alexander-demicev deleted the errors branch May 21, 2020 19:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

alexander-demicev commented May 19, 2020

JoelSpeed May 19, 2020

JoelSpeed May 19, 2020

JoelSpeed May 19, 2020

JoelSpeed May 19, 2020

alexander-demicev May 20, 2020

JoelSpeed May 20, 2020

JoelSpeed May 20, 2020

alexander-demicev May 21, 2020

JoelSpeed May 20, 2020

alexander-demicev commented May 21, 2020

JoelSpeed left a comment

openshift-ci-robot commented May 21, 2020

enxebre May 21, 2020

alexander-demicev May 21, 2020

enxebre commented May 21, 2020

openshift-bot commented May 21, 2020

enxebre commented May 21, 2020

enxebre commented May 21, 2020

openshift-ci-robot commented May 21, 2020

openshift-ci-robot commented May 21, 2020

enxebre commented May 21, 2020

openshift-ci-robot commented May 21, 2020

enxebre commented May 21, 2020

enxebre commented May 21, 2020

openshift-ci-robot commented May 21, 2020

openshift-ci-robot commented May 21, 2020

	// Set this value to 2 because it's the default number on machine in mocked environment
	// Set this value to 2 because it's the default number of machines in a mocked environment

Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

Bug 1838504: [vSphere] Fail machine on invalid provider spec values #593

Conversation

alexander-demicev commented May 19, 2020

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

alexander-demicev commented May 21, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre commented May 21, 2020

openshift-bot commented May 21, 2020

enxebre commented May 21, 2020

enxebre commented May 21, 2020

openshift-ci-robot commented May 21, 2020

openshift-ci-robot commented May 21, 2020

enxebre commented May 21, 2020

openshift-ci-robot commented May 21, 2020

enxebre commented May 21, 2020

enxebre commented May 21, 2020

openshift-ci-robot commented May 21, 2020

openshift-ci-robot commented May 21, 2020