Allocate Ironic port when node exists and has no port allocated.

[s3rj1k]

This PR fixes issues below:

• While node is created in Ironic DB and some misfortune happens to Ironic pod or DB pod, Ironic does not allocate port created node, this in turn breaks inspection with Ironic erroring on absence of allocated port, operator does not handle this case and stuck in erroring reconcile loop until back-off timer is triggered.

When node has no allocated port to it, Ironic returns this error

No lookup attributes were found, inspector won't be able to find it after introspection, consider creating ironic ports or providing an IPMI address

IPMI address part is just wrong, BMC creds are checked in Registering state and are validated, also they where validated manually just in case.

After checking sources that return this error in: https://github.com/openstack/ironic-inspector/blob/stable/victoria/ironic_inspector/introspect.py#L102-L119
We can clearly see that this error is indeed because of not allocated port to node (no MAC), this also is verified by this unix pipe magic for specified host.

kubectl get bmh worker-1 -o jsonpath='{.spec.bootMACAddress}' | xargs -rI{} --verbose openstack baremetal port show --address '{}' -f value --fields node_uuid | xargs -rI{} --verbose openstack baremetal node show {} -f value --fields name

[s3rj1k]

/assign @stbenjam

[metal3-io-bot]

Hi @s3rj1k. Thanks for your PR.

I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[s3rj1k]

cc: @dhellmann Can you please also look at this, thanks.

[dtantsur]

I was under impression that we always create a port before inspection, otherwise e.g. Redfish inspection would not work.

[s3rj1k]

@dtantsur We allocate port only when node was not created before, for existing node we expect that port was created.
This PR fixes this exact edge case when node was created but for some unknown reason port was not allocated for that node.

[dtantsur]

/ok-to-test
/test-integration

[s3rj1k]

/test-integration

[s3rj1k]

@dtantsur All tests are green :)

[s3rj1k]

@stbenjam @dhellmann @dtantsur Are we safe to merge?

[zaneb]

@s3rj1k can you describe what the actual change is in the first patch? Git makes the diff really hard to interpret but after going through it as closely as I can using a visual diff tool, I was unable it find any change in logic. It looks like a straight refactor (although arguably an improvement in readability). The commit message only says that something was broken and is now fixed but I can't figure out what. What's the actual code path that is different?

[s3rj1k]

@zaneb Yea, its a diff mess

So basically I am introducing var introspectionStatusNotFound bool flag and refactoring an if case to make it more readable

doing this

	status, err := introspection.GetIntrospectionStatus(p.inspector, ironicNode.UUID).Extract()
	if err != nil {
		if _, isNotFound := err.(gophercloud.ErrDefault404); isNotFound {
			introspectionStatusNotFound = true
			p.log.Info("inspection result not found")
		} else {
			result, err = transientError(errors.Wrap(err, "failed to extract hardware inspection status"))
			return
		}
	}

	if introspectionStatusNotFound { ... }

Old case code is mostly unchanged in { ... }, this fixes an issue with forced introspection, before, because of complicated flow logic it was hard to spot.

if nodes.ProvisionState(ironicNode.ProvisionState) == nodes.InspectFail && !force {

in before PR never gets executed

[s3rj1k]

@zaneb Did you get a chance to look at this again?

[zaneb]

I'm not following how that code fails to get executed:
If GetIntrospectionStatus() on line 653 returns a 404 Not Found then
err will be non-nil on line 654
isNotFound will be true on line 655
If the node's provision state is InspectFail, we'll hit the default case of the switch on line 661
The block starting at line 662 will execute or not depending on whether the force flag is set.

AFAICT the logic is the same after the patch, only the line numbers change:
If GetIntrospectionStatus() on line 655 returns a 404 Not Found then
isNotFound will be true on line 657
introspectionStatusNotFound will be true on line 666
If the node's provision state is InspectFail, we'll hit the default case of the switch on line 672
The block starting at line 673 will execute or not depending on whether the force flag is set.

There's no difference.

On closer inspection (no pun intended) there does appear to be a bug here though: I'm pretty sure I meant to put a return statement after err = nil on line 669. There'd be no point setting it to nil when we're going to fall through and overwrite it anyway, so that seems like a mistake on my part. Your patch doesn't change that though.

[s3rj1k]

@zaneb I could split it out into two different PRs, I noticed some work around Introspection in #607 so after this PR seems like it would be needed to recheck first part of my PR.

So that do you think? Leaving only a second part of a patch sounds good?

[s3rj1k]

@zaneb, so I've removed first part of PR, currently it only deals with Ironic port creation, hoping this would help in getting this merged.

[s3rj1k]

/test-integration

[s3rj1k]

@dtantsur Can you please take a look at this PR, hopefully approve it :)

[zaneb]

I'm pretty sure I meant to put a return statement after err = nil on line 669.

I opened #840 to fix this.

So that do you think? Leaving only a second part of a patch sounds good?

Sounds good to me, but this leaves only the part I don't understand well enough to review.

[s3rj1k]

Sounds good to me, but this leaves only the part I don't understand well enough to review.

I've highlighted @dtantsur on rebased PR, I think he could help with Ironic internals

[s3rj1k]

/test-integration

[s3rj1k]

@zaneb Can you please take another look, PR is rebased

[s3rj1k]

@zaneb @stbenjam kindly remainder, PR waits review, thanks.

[s3rj1k]

/cc @dtantsur
/cc @stbenjam

[s3rj1k]

/retest

[zaneb]

This looks correct, but expensive.

[s3rj1k]

/test-integration

[s3rj1k]

/recheck

[zaneb]

I remain worried about the extra overhead of this on every reconcile, but it doesn't seem avoidable.
/approve

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: s3rj1k, zaneb

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 [zaneb]

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

[s3rj1k]

/test-integration

[s3rj1k]

/test-integration

[dtantsur]

/lgtm