Bug 1808971: Machine status shows "running" when an instance was terminated #575

Danil-Grigorev · 2020-04-29T13:31:02Z

Setting machine instance state to unknown on machine failure

openshift-ci-robot · 2020-04-29T13:31:09Z

@Danil-Grigorev: This pull request references Bugzilla bug 1808971, 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 ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1808971: Machine status shows "running" when an instance was terminated

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.

alexander-demicev

This PR seems to duplicate provider logic https://github.com/openshift/cluster-api-provider-aws/blob/master/pkg/actuators/machine/reconciler.go#L335

alexander-demicev · 2020-04-29T13:52:27Z

This can break the consistency because we set same annotation in 2 different places. However, the idea of setting this in one place is nice.

enxebre · 2020-04-29T13:55:32Z

pkg/controller/machine/controller.go

@@ -426,9 +426,18 @@ func (r *ReconcileMachine) setPhase(machine *machinev1.Machine, phase string, er
 		machine.Status.LastUpdated = &now
 		if phase == phaseFailed && errorMessage != "" {
 			machine.Status.ErrorMessage = &errorMessage
+			if machine.Annotations == nil {
+				machine.Annotations = map[string]string{}


you could save the if:

annotations := m.GetAnnotations() annotations["key"] = "value" m.SetAnnotations(annotations)

This will cause a panic on Annotations being nil, especially in tests. I'm not sure how to avoid nil check here.

I'm missing where exactly would this panic? I'd expect GetAnnotations to give you an empty struct slice, may be I'm wrong. I'm not totally sure what the API machinery defaulting will return here. You're right the check is safe.

The check is needed, apimachinery will happily return a nil map
https://github.com/kubernetes/apimachinery/blob/7ae370969693753028c74c0c876eee17ddb1b15b/pkg/apis/meta/v1/meta.go#L165

enxebre · 2020-04-29T13:55:49Z

pkg/controller/machine/controller.go

+			if machine.Annotations == nil {
+				machine.Annotations = map[string]string{}
+			}
+			machine.Annotations[MachineInstanceStateAnnotationName] = "Unknown"


can we put this into a constant?

enxebre · 2020-04-29T13:56:58Z

pkg/controller/machine/controller.go

+			if machine.Annotations == nil {
+				machine.Annotations = map[string]string{}
+			}
+			machine.Annotations[MachineInstanceStateAnnotationName] = "Unknown"


I think we should only set unknown here https://github.com/openshift/machine-api-operator/pull/575/files#diff-b96ce5bf8686fb02d855188000ae1cb4R299-R304

Here is another place, where the machine will get into Failed pahse, and the VM would not be monitored/updated anymore:

machine-api-operator/pkg/controller/machine/controller.go

Lines 313 to 316 in 46410db

if isInvalidMachineConfigurationError(err) {

if err := r.setPhase(m, phaseFailed, err.Error()); err != nil {

return reconcile.Result{}, err

}

I was thinking to handle it generically.

make sense, in the other scenario the provider actually have a chance to set a valid state as it goes failed so might good to leave it. Happy to hear others thoughts.

JoelSpeed · 2020-04-30T14:22:36Z

pkg/controller/machine/controller.go

@@ -426,9 +426,18 @@ func (r *ReconcileMachine) setPhase(machine *machinev1.Machine, phase string, er
 		machine.Status.LastUpdated = &now
 		if phase == phaseFailed && errorMessage != "" {
 			machine.Status.ErrorMessage = &errorMessage
+			if machine.Annotations == nil {
+				machine.Annotations = map[string]string{}


The check is needed, apimachinery will happily return a nil map
https://github.com/kubernetes/apimachinery/blob/7ae370969693753028c74c0c876eee17ddb1b15b/pkg/apis/meta/v1/meta.go#L165

JoelSpeed · 2020-04-30T14:26:30Z

pkg/controller/machine/controller.go

+			if err := r.Client.Patch(context.Background(), machine, baseToPatch); err != nil {
+				klog.Errorf("Failed to update machine %q: %v", machine.GetName(), err)
+				return err
+			}
 		}
 		if err := r.Client.Status().Patch(context.Background(), machine, baseToPatch); err != nil {


Can you check to see if there are conflict errors coming through now? Can't remember if Patch needs an up to date resource version or not for the patch call to succeed

JoelSpeed · 2020-04-30T14:35:52Z

pkg/controller/machine/controller_test.go

@@ -382,7 +382,7 @@ func TestSetPhase(t *testing.T) {
 		t.Fatal(err)
 	}
 	if *lastUpdated != *got.Status.LastUpdated {
-		t.Errorf("Expected: %v, got: %v", *lastUpdated, *got.Status.LastUpdated)
+		t.Errorf("Expected: %v, got: %v", *lastUpdated, got.Status.LastUpdated)


Why is this changed? I don't see how it relates 🤔

JoelSpeed · 2020-04-30T14:36:30Z

pkg/controller/machine/controller_test.go

@@ -401,12 +403,21 @@ func TestSetPhase(t *testing.T) {
 	}
 	// validate persisted object
 	if expecterErrorMessage != *got.Status.ErrorMessage {
-		t.Errorf("Expected: %v, got: %v", expecterErrorMessage, *got.Status.ErrorMessage)
+		t.Errorf("Expected: %v, got: %v", expecterErrorMessage, got.Status.ErrorMessage)


Same as above, why is this deref changed?

JoelSpeed · 2020-04-30T14:37:38Z

pkg/controller/machine/controller_test.go

+		t.Fatal("Got phase nil")
+	}
+	if *got.Status.Phase != phaseFailed {
+		t.Errorf("Got: %v, expected: %v", *got.Status.Phase, phaseRunning)


The rest of the assertions around here are this way around

Suggested change

t.Errorf("Got: %v, expected: %v", *got.Status.Phase, phaseRunning)

t.Errorf("Expected: %v, got: %v", phaseFailed, *got.Status.Phase)

Oh I missed the fact that all those fields are pointers :D Thanks for the tip, will revert it.

Bug 1808971: Machine status shows "running" when an instance was terminated

openshift-ci-robot · 2020-05-05T14:49:20Z

@Danil-Grigorev: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws-scaleup-rhel7	`c38b02f`	link	`/test e2e-aws-scaleup-rhel7`
ci/prow/e2e-azure-operator	`c38b02f`	link	`/test e2e-azure-operator`
ci/prow/e2e-azure	`c38b02f`	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-06T07:36:12Z

pkg/controller/machine/controller.go

+			if machine.Annotations == nil {
+				machine.Annotations = map[string]string{}
+			}
+			machine.Annotations[MachineInstanceStateAnnotationName] = unknownInstanceState


this might cause skew if the actuators set something in the provider.Status. We might consolidate to consolidate that.

enxebre · 2020-05-06T07:36:24Z

/approve
@JoelSpeed PTAL

openshift-ci-robot · 2020-05-06T07:36:45Z

[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-06T07:50:56Z

@Danil-Grigorev: All pull requests linked via external trackers have merged: openshift/machine-api-operator#575. Bugzilla bug 1808971 has been moved to the MODIFIED state.

In response to this:

Bug 1808971: Machine status shows "running" when an instance was terminated

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 added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium 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 29, 2020

openshift-ci-robot requested review from enxebre and michaelgugino April 29, 2020 13:31

alexander-demicev reviewed Apr 29, 2020

View reviewed changes

enxebre reviewed Apr 29, 2020

View reviewed changes

Danil-Grigorev force-pushed the set-unknown-vm-state-on-failure branch 3 times, most recently from c10a93c to 96bb756 Compare April 30, 2020 10:20

JoelSpeed reviewed Apr 30, 2020

View reviewed changes

Danil-Grigorev force-pushed the set-unknown-vm-state-on-failure branch 3 times, most recently from c1656b4 to 1de9a9b Compare May 5, 2020 12:51

Setting machine instance state to unknown on machine failure

c38b02f

Bug 1808971: Machine status shows "running" when an instance was terminated

Danil-Grigorev force-pushed the set-unknown-vm-state-on-failure branch from 1de9a9b to c38b02f Compare May 5, 2020 12:53

Danil-Grigorev requested review from enxebre, JoelSpeed and alexander-demicev May 5, 2020 12:54

enxebre reviewed May 6, 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 6, 2020

JoelSpeed reviewed May 6, 2020

View reviewed changes

openshift-ci-robot assigned JoelSpeed May 6, 2020

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

openshift-merge-robot merged commit 67083c6 into openshift:master May 6, 2020

openshift-ci-robot mentioned this pull request May 14, 2020

Bug 1808971: Machine status shows "running" when an instance was terminated openshift/cluster-api-provider-gcp#89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1808971: Machine status shows "running" when an instance was terminated #575

Bug 1808971: Machine status shows "running" when an instance was terminated #575

Danil-Grigorev commented Apr 29, 2020

openshift-ci-robot commented Apr 29, 2020

alexander-demicev left a comment

alexander-demicev commented Apr 29, 2020

enxebre Apr 29, 2020

Danil-Grigorev Apr 29, 2020

enxebre Apr 30, 2020 •

edited

JoelSpeed Apr 30, 2020

enxebre Apr 29, 2020

enxebre Apr 29, 2020

Danil-Grigorev Apr 29, 2020

enxebre Apr 30, 2020

JoelSpeed Apr 30, 2020

JoelSpeed Apr 30, 2020

JoelSpeed Apr 30, 2020

JoelSpeed Apr 30, 2020

JoelSpeed Apr 30, 2020

Danil-Grigorev May 5, 2020

openshift-ci-robot commented May 5, 2020

enxebre May 6, 2020

enxebre commented May 6, 2020

openshift-ci-robot commented May 6, 2020

JoelSpeed left a comment

openshift-ci-robot commented May 6, 2020

	if isInvalidMachineConfigurationError(err) {
	if err := r.setPhase(m, phaseFailed, err.Error()); err != nil {
	return reconcile.Result{}, err
	}

	t.Errorf("Got: %v, expected: %v", *got.Status.Phase, phaseRunning)
	t.Errorf("Expected: %v, got: %v", phaseFailed, *got.Status.Phase)

Bug 1808971: Machine status shows "running" when an instance was terminated #575

Bug 1808971: Machine status shows "running" when an instance was terminated #575

Conversation

Danil-Grigorev commented Apr 29, 2020

openshift-ci-robot commented Apr 29, 2020

alexander-demicev left a comment

Choose a reason for hiding this comment

alexander-demicev commented Apr 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Apr 30, 2020 • edited

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

openshift-ci-robot commented May 5, 2020

Choose a reason for hiding this comment

enxebre commented May 6, 2020

openshift-ci-robot commented May 6, 2020

JoelSpeed left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented May 6, 2020

enxebre Apr 30, 2020 •

edited