ensure the boot mode is set in ironic before starting inspection #635

dhellmann · 2020-09-15T19:54:59Z

We were previously only updating the host status copy of the boot mode
when we entered the inspecting state or provisioning states. This change
extends that to set it correctly during the registration process, too,
so that when the node is created in ironic it is given the right mode.

We were only updating the boot mode in ironic during registration and
during provisioning. This change updates the mode in ironic before
starting provisioning.

metal3-io-bot · 2020-09-15T19:55:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann

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

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

dhellmann · 2020-09-16T14:03:12Z

/test-integration

dhellmann · 2020-09-16T19:52:04Z

/test-integration

hardys · 2020-09-17T14:20:08Z

pkg/controller/baremetalhost/host_state_machine.go

+			// it needs error handling logic that we can't support in
+			// this function.
+			if updateBootModeStatus(hsm.Host) {
+				info.log.Info("saving boot mode")


Not that we have to fix it in this PR, but this log output would potentially be more useful if it included the bootMode that we saved?

Sure, that makes sense. Done.

hardys · 2020-09-17T14:22:11Z

lgtm but needs a rebase

dhellmann · 2020-09-17T15:14:08Z

Rebased

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Move the call to update the boot mode in the status fields to one spot in the state machine where we always know the new state we're going to transition to. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Now that getUpdateOptsForNode() is being called before inspection, we may be using it when the image values are not set for the host. We don't want to pass empty values to ironic, so the logic is restructured to only set values associated with the image when there are values to send. The general structure of the function is reorganized at the same time to deal with values coming from the hardware profile first, right after the profile is fetched, and to handle all of the image values next. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

zaneb

Setting all of the Node options (except the image) prior to inspecting seems like a big change with unpredictable effects that could easily be avoided if our goal is just to fix this bug.

zaneb · 2020-09-17T16:22:17Z

pkg/provisioner/ironic/ironic.go

-		op = nodes.ReplaceOp
-		p.log.Info("updating image_source")
-	}
+	// instance_uuid


I'd have assumed we only want to set this when provisioning ('instance' here means Nova instance).

IIUC, ironic doesn't use any of these values until it needs them, so setting them early shouldn't hurt. They're reset before provisioning, as well.

zaneb · 2020-09-17T16:26:51Z

pkg/provisioner/ironic/ironic.go

+				// Update the node settings in ironic to ensure we
+				// have the boot mode configured, at least.
+				var updates nodes.UpdateOpts
+				updates, err = p.getUpdateOptsForNode(ironicNode)


This would seem a lot lower-risk if we just refactored the code for setting the boot mode option out of getUpdateOptsForNode() and only set that one here.

zaneb · 2020-09-17T16:27:54Z

pkg/provisioner/ironic/ironic.go

+	// cpu_arch
+	//
+	// FIXME(dhellmann): This should come from inspecting the
+	// host.


We're now calling this func before inspection, which will make it hard to change this.

It's not clear to me why we need to set this at all if it can come from inspection, but when we do drop the profile we would necessarily not set it here unless we did have inspection data.

zaneb · 2020-09-17T16:37:22Z

pkg/controller/baremetalhost/host_state_machine.go

+		// the API. That means we can safely update any status fields
+		// along with the state.
+		switch hsm.NextState {
+		case metal3v1alpha1.StateRegistering,


Is there any point doing this before Registering when it's not copied to Ironic until we reach Inspecting?

I don't think it's safe to do this before #388 is merged, because right now this could end up changing the status of a provisioned Host so that the status would no longer match what we actually provisioned with.

I can remove registering from the list here.

dhellmann · 2020-09-21T20:03:43Z

/hold

Let's see how #645 looks instead of this one.

metal3-io-bot · 2020-09-24T05:05:23Z

@dhellmann: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
gomod	`b0468b6`	link	`/test gomod`

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.

dhellmann · 2020-09-24T15:41:14Z

This is no longer needed.

metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 15, 2020

dhellmann changed the title ~~ensure the boot mode is set in ironic before starting inspection~~ WIP: ensure the boot mode is set in ironic before starting inspection Sep 15, 2020

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2020

dhellmann changed the title ~~WIP: ensure the boot mode is set in ironic before starting inspection~~ ensure the boot mode is set in ironic before starting inspection Sep 16, 2020

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2020

dhellmann changed the title ~~ensure the boot mode is set in ironic before starting inspection~~ WIP: ensure the boot mode is set in ironic before starting inspection Sep 16, 2020

metal3-io-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2020

dhellmann changed the title ~~WIP: ensure the boot mode is set in ironic before starting inspection~~ ensure the boot mode is set in ironic before starting inspection Sep 16, 2020

metal3-io-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2020

dhellmann requested a review from zaneb September 16, 2020 14:48

hardys reviewed Sep 17, 2020

View reviewed changes

dhellmann force-pushed the boot-mode-status-update branch from 709b641 to 5e503bb Compare September 17, 2020 15:13

dhellmann added 5 commits September 17, 2020 11:17

add unit tests for updateBootModeStatus

5b3b87d

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

ensure boot mode is set before registering the host

edcd7a9

Move the call to update the boot mode in the status fields to one spot in the state machine where we always know the new state we're going to transition to. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

ensure that the boot mode is set before starting inspection

b28845d

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

log the new boot mode when we update the status block

b0468b6

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the boot-mode-status-update branch from 5e503bb to b0468b6 Compare September 17, 2020 15:17

zaneb reviewed Sep 17, 2020

View reviewed changes

dhellmann mentioned this pull request Sep 21, 2020

ensure the boot mode is set in ironic before starting inspection #645

Merged

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 21, 2020

dhellmann closed this Sep 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure the boot mode is set in ironic before starting inspection #635

ensure the boot mode is set in ironic before starting inspection #635

dhellmann commented Sep 15, 2020

metal3-io-bot commented Sep 15, 2020

dhellmann commented Sep 16, 2020

dhellmann commented Sep 16, 2020

hardys Sep 17, 2020

dhellmann Sep 17, 2020

hardys commented Sep 17, 2020

dhellmann commented Sep 17, 2020

zaneb left a comment

zaneb Sep 17, 2020

dhellmann Sep 18, 2020

zaneb Sep 17, 2020

zaneb Sep 17, 2020

dhellmann Sep 18, 2020

zaneb Sep 17, 2020

dhellmann Sep 18, 2020

dhellmann commented Sep 21, 2020

metal3-io-bot commented Sep 24, 2020

dhellmann commented Sep 24, 2020

ensure the boot mode is set in ironic before starting inspection #635

ensure the boot mode is set in ironic before starting inspection #635

Conversation

dhellmann commented Sep 15, 2020

metal3-io-bot commented Sep 15, 2020

dhellmann commented Sep 16, 2020

dhellmann commented Sep 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardys commented Sep 17, 2020

dhellmann commented Sep 17, 2020

zaneb 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

dhellmann commented Sep 21, 2020

metal3-io-bot commented Sep 24, 2020

dhellmann commented Sep 24, 2020