Changing the default behaviour of the CAPBM to request hard reboot

[rdoxenham]

This change adds an additional mode to the reboot annotation that
forces all nodes sent for remediation, e.g. via a MachineHealthCheck,
to be forcefully rebooted rather than defaulting to a soft reboot
first, as it is today. The primary drive behind this change is to
enable quicker recovery of workloads, e.g. for high-availability
use cases, and by defaulting to forced hard reboot we can enable
functionality very close to fencing. This change shouldn't impact
any other non-remediation reboot requests, as the hard reboot
functionality only takes place when the mode=hard annotation is
applied to the node.

All of the work on the BMO can be found in the link below. Whilst
we depend on this PR to have a complete solution, we don't have a
hard dependency on them merging together.

BMO PR: metal3-io/baremetal-operator#795

[n1r1]

/lgtm

[n1r1]

Hmmm actually I'm not sure this is okay.
I know you've tested it and it worked but I still find it weird that the value is set as part of the annotation key.
Even if it works it could be misleading. For example we have the following line:

baremetalhost.Annotations[requestPowerOffAnnotation] = ""

and also:

_, exists = baremetalhost.Annotations[requestPowerOffAnnotation]

So I would feel much more comfortable to have the value set in this line

[rdoxenham]

Hmmm actually I'm not sure this is okay.
I know you've tested it and it worked but I still find it weird that the value is set as part of the annotation key.
Even if it works it could be misleading. For example we have the following line:

baremetalhost.Annotations[requestPowerOffAnnotation] = ""

and also:

_, exists = baremetalhost.Annotations[requestPowerOffAnnotation]

So I would feel much more comfortable to have the value set in this line

Thanks @n1r1 - made the changes, you're absolutely right here.

[n1r1]

thanks @rdoxenham
WDYT about adding this to the tests as well?

should be easy by adding a simple check here or inside the previous if statement

[rdoxenham]

thanks @rdoxenham
WDYT about adding this to the tests as well?

should be easy by adding a simple check here or inside the previous if statement

Thanks - I put it into the previous if statement. In reality, it assigns the poweredOffForRemediation prior to requestPowerOffAnnotation but I don't think it matters in this instance. Let me know.

[n1r1]

thanks @rdoxenham
WDYT about adding this to the tests as well?
should be easy by adding a simple check here or inside the previous if statement

Thanks - I put it into the previous if statement. In reality, it assigns the poweredOffForRemediation prior to requestPowerOffAnnotation but I don't think it matters in this instance. Let me know.

ah sorry, I was pointing to the wrong line 🤦

You're right, I meant to add the check where we actually add the requestPowerOffAnnotation annotation.
I think it worth checking that the reboot mode was set correctly, so if someone changes this in the future, test will fail.

So maybe adding inside this if or after it, something like:

if host.Annotations[requestPowerOffAnnotation] != `{"mode":"hard"}` {
     t.Log("...")
     t.Fail()
}

[rdoxenham]

thanks @rdoxenham
WDYT about adding this to the tests as well?
should be easy by adding a simple check here or inside the previous if statement

Thanks - I put it into the previous if statement. In reality, it assigns the poweredOffForRemediation prior to requestPowerOffAnnotation but I don't think it matters in this instance. Let me know.

ah sorry, I was pointing to the wrong line

You're right, I meant to add the check where we actually add the requestPowerOffAnnotation annotation.
I think it worth checking that the reboot mode was set correctly, so if someone changes this in the future, test will fail.

So maybe adding inside this if or after it, something like:

if host.Annotations[requestPowerOffAnnotation] != `{"mode":"hard"}` {
     t.Log("...")
     t.Fail()
}

Thanks @n1r1, added that test too.

[n1r1]

Thanks.
Since you moved from string to a struct in BMO PR, I guess it makes sense to use it here as well instead of {"mode":"hard"}

[rdoxenham]

Thanks.
Since you moved from string to a struct in BMO PR, I guess it makes sense to use it here as well instead of {"mode":"hard"}

@n1r1 sure, can do... forgive the ignorance, where's best to define the struct?

[n1r1]

Thanks.
Since you moved from string to a struct in BMO PR, I guess it makes sense to use it here as well instead of {"mode":"hard"}

@n1r1 sure, can do... forgive the ignorance, where's best to define the struct?

It was already defined in BMO, so you don't need to re-define it here.
The actuator here is already imports the relevant file:

bmh "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"

You'll probably need to revendor, but I think this will have to wait until that PR merges and backported to openshift/BMO, but I'm not sure

[rdoxenham]

Thanks.
Since you moved from string to a struct in BMO PR, I guess it makes sense to use it here as well instead of {"mode":"hard"}

@n1r1 sure, can do... forgive the ignorance, where's best to define the struct?

It was already defined in BMO, so you don't need to re-define it here.
The actuator here is already imports the relevant file:

bmh "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"

You'll probably need to revendor, but I think this will have to wait until that PR merges and backported to openshift/BMO, but I'm not sure

Oh sweet, I didn't realise you imported that. Perfect... I'll need to wait until it lands, yes. Thx!

[rdoxenham]

@n1r1 let me know if this meets your expectations. Thanks!

[dhellmann]

I opened #140 to remove the CRD generation entirely, since I don't think we're using those files at all.

[rdoxenham]

/test e2e-metal-ipi
/test e2e-metal-ipi-ovn-ipv6

[hardys]

lgtm, one small nit re the test adjustments, I think one of those landed in the hard-reboot commit, which was initially confusing when reviewing each commit individually. Not blocking on it, but may be worth fixing if you need to rebase or address any other comments.

/lgtm

[rdoxenham]

/test e2e-metal-ipi-upgrade

[hardys]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys, n1r1, rdoxenham

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [hardys]

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

[rdoxenham]

/cherry-pick release-4.7

[openshift-cherrypick-robot]

@rdoxenham: new pull request created: #143

Details

In response to this:

/cherry-pick release-4.7

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.

[zaneb]

We're ignoring any error in marshalling here... it seems like we should at least log it.

[rdoxenham]

@zaneb I don't disagree, but I figured it would be safe given that it's our own struct that we're marshalling, not data that we're getting from a client/user.

[zaneb]

Why delete this comment? Without it we will be carrying this code forever.

[rdoxenham]

@zaneb perhaps a misunderstanding on my behalf, but as we revendored with a new update of the BMO code, these states no longer existed, and therefore seemed pointless to keep the comment given that in that code metal3-io/baremetal-operator#388 had landed. I can follow up with another PR to reinstate the comment if you feel it necessary.

[zaneb]

@rdoxenham The comment applied to all 3 states, including StateRegistering. So the code below is now redundant but there's no reminder to us that it can be deleted.