Bug 1829408: Reconcile machine power state annotation #578

alexander-demicev · 2020-04-30T10:45:16Z

Reconcile machine power state annotation

openshift-ci-robot · 2020-04-30T10:50:19Z

@alexander-demichev: This pull request references Bugzilla bug 1829408, 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:

Bug 1829408: Reconcile machine power state annotation

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-04T07:49:35Z

pkg/controller/vsphere/reconciler.go

@@ -333,6 +336,24 @@ func (r *Reconciler) reconcileNetwork(vm *virtualMachine) error {
 	return nil
 }

+func (r *Reconciler) reconcilePowerStateAnnontation(vm *virtualMachine) {


can we please have a unit for this func?

+1, would be good to see a couple of tests exercising this explicitly

enxebre · 2020-05-05T14:23:21Z

pkg/controller/vsphere/reconciler.go

+	powerState, err := vm.getPowerState()
+	if err != nil {
+		klog.V(3).Infof("%s: Failed to get power state: %v", r.machine.Name, err)
+		return


why are we ignoring errors here?
we should return error, then is up to the consumer what to do with it, e.g log and ignore it if there's a reason to do so

enxebre · 2020-05-05T14:23:56Z

pkg/controller/vsphere/reconciler_test.go

+		t.Errorf("Expected providerId: %s, got: %s", expectedProviderID, *reconciler.machine.Spec.ProviderID)
+	}
+
+	expectedPowerState := "poweredOn"


should this be a constant?

alexander-demicev · 2020-05-06T10:19:49Z

@enxebre All fixed

enxebre · 2020-05-07T10:38:37Z

/approve

JoelSpeed

Provided a couple of suggestions for potential rewording of some of this, WDYT?

JoelSpeed · 2020-05-07T10:42:16Z

pkg/controller/vsphere/reconciler_test.go

+	}
+
+	vm := &virtualMachine{
+		Context: context.TODO(),


Nit

Suggested change

Context: context.TODO(),

Context: context.Background(),

JoelSpeed · 2020-05-07T10:44:22Z

pkg/controller/vsphere/reconciler.go

@@ -333,6 +338,25 @@ func (r *Reconciler) reconcileNetwork(vm *virtualMachine) error {
 	return nil
 }

+func (r *Reconciler) reconcilePowerStateAnnontation(vm *virtualMachine) error {
+	if vm == nil {
+		return errors.New("VM can't be nil")


Suggested change

return errors.New("VM can't be nil")

return errors.New("provided VM is nil")

JoelSpeed · 2020-05-07T10:45:06Z

pkg/controller/vsphere/reconciler.go

@@ -233,6 +233,11 @@ func (r *Reconciler) reconcileMachineWithCloudState(vm *virtualMachine, taskRef
 		return err
 	}

+	klog.V(3).Infof("%v: reconciling powerstate annotation", r.machine.GetName())
+	if err := r.reconcilePowerStateAnnontation(vm); err != nil {
+		klog.V(3).Infof("%s: Failed to get power state, machine can be in process of deletion: %v", r.machine.Name, err)


Suggested change

klog.V(3).Infof("%s: Failed to get power state, machine can be in process of deletion: %v", r.machine.Name, err)

klog.V(3).Infof("%s: Failed to get power state (machine may be being deleted): %v", r.machine.Name, err)

enxebre · 2020-05-12T10:52:04Z

pkg/controller/vsphere/reconciler.go

@@ -233,6 +233,11 @@ func (r *Reconciler) reconcileMachineWithCloudState(vm *virtualMachine, taskRef
 		return err
 	}

+	klog.V(3).Infof("%v: reconciling powerstate annotation", r.machine.GetName())
+	if err := r.reconcilePowerStateAnnontation(vm); err != nil {
+		klog.V(3).Infof("%s: Failed to get power state (machine may be being deleted): %v", r.machine.Name, err)


why are we ignoring the error here? we should return an error.

shouldn't we?

openshift-ci-robot · 2020-05-12T13:45:40Z

@alexander-demichev: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure-operator	`5c34c0a`	link	`/test e2e-azure-operator`
ci/prow/e2e-azure	`5c34c0a`	link	`/test e2e-azure`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

enxebre · 2020-05-12T14:18:51Z

/approve

openshift-ci-robot · 2020-05-12T14:19:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

JoelSpeed

/lgtm

openshift-ci-robot · 2020-05-13T10:44:53Z

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

In response to this:

Bug 1829408: Reconcile machine power state annotation

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 JoelSpeed April 30, 2020 10:45

alexander-demicev changed the title ~~Reconcile machine power state annotation~~ Bug 1829408: Reconcile machine power state annotation Apr 30, 2020

openshift-ci-robot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Apr 30, 2020

enxebre reviewed May 4, 2020

View reviewed changes

alexander-demicev force-pushed the instancestate branch from 3fe06c0 to ebfe75a Compare May 5, 2020 12:00

enxebre reviewed May 5, 2020

View reviewed changes

alexander-demicev force-pushed the instancestate branch from ebfe75a to 0632565 Compare May 6, 2020 10:17

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

JoelSpeed reviewed May 7, 2020

View reviewed changes

alexander-demicev force-pushed the instancestate branch from 0632565 to 3f52adc Compare May 11, 2020 09:13

enxebre reviewed May 12, 2020

View reviewed changes

Reconcile machine power state annotation

5c34c0a

alexander-demicev force-pushed the instancestate branch from 3f52adc to 5c34c0a Compare May 12, 2020 11:35

JoelSpeed reviewed May 13, 2020

View reviewed changes

openshift-ci-robot assigned JoelSpeed May 13, 2020

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

openshift-merge-robot merged commit bcc666e into openshift:master May 13, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1829408: Reconcile machine power state annotation #578

Bug 1829408: Reconcile machine power state annotation #578

alexander-demicev commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 2020

enxebre May 4, 2020

JoelSpeed May 5, 2020

alexander-demicev May 5, 2020

enxebre May 5, 2020

enxebre May 5, 2020

alexander-demicev commented May 6, 2020

enxebre commented May 7, 2020

JoelSpeed left a comment

JoelSpeed May 7, 2020

alexander-demicev May 11, 2020

JoelSpeed May 7, 2020

alexander-demicev May 11, 2020

JoelSpeed May 7, 2020

alexander-demicev May 11, 2020

enxebre May 12, 2020

enxebre May 12, 2020

openshift-ci-robot commented May 12, 2020

enxebre commented May 12, 2020

openshift-ci-robot commented May 12, 2020

JoelSpeed left a comment

openshift-ci-robot commented May 13, 2020

	return errors.New("VM can't be nil")
	return errors.New("provided VM is nil")

	klog.V(3).Infof("%s: Failed to get power state, machine can be in process of deletion: %v", r.machine.Name, err)
	klog.V(3).Infof("%s: Failed to get power state (machine may be being deleted): %v", r.machine.Name, err)

Bug 1829408: Reconcile machine power state annotation #578

Bug 1829408: Reconcile machine power state annotation #578

Conversation

alexander-demicev commented Apr 30, 2020

openshift-ci-robot commented Apr 30, 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

alexander-demicev commented May 6, 2020

enxebre commented May 7, 2020

JoelSpeed left a 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

openshift-ci-robot commented May 12, 2020

enxebre commented May 12, 2020

openshift-ci-robot commented May 12, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 13, 2020