🐛 Allow setting force_persistent_boot_device: <value> for deploy_interface: direct

[Cellebyte]

What this PR does / why we need it:

It allows configuring the force_persistent_boot_device: <value> when deploy_interface: direct is used.
It uses the same variable as for deploy_interface: ramdisk.

I did not rename the current env var LIVE_ISO_FORCE_PERSISTENT_BOOT_DEVICE to be backwards compatible.
It introduces a new environment variable DIRECT_DEPLOY_FORCE_PERSISTENT_BOOT_DEVICE which can be toggled when using the direct interface.
The default is the same behaviour as before, so people need to actively opt-in.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #2613

[metal3-io-bot]

Hi @Cellebyte. 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.

Details

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-sigs/prow repository.

[tuminoid]

/ok-to-test

[Cellebyte]

@tuminoid is this error in the test related to my change?

[tuminoid]

@tuminoid is this error in the test related to my change?

Not sure, do not have time right now to dig into. So, let's rerun and if it fails to the same, then very likely yes :)
/retest

[Cellebyte]

@tuminoid thank you for rerunning, seems to look good :)

[tuminoid]

@tuminoid thank you for rerunning, seems to look good :)

The prow keywords work for anyone after initial ok-to-test btw. Just in case you need to make some revisions and want/need to run some tests.

[Cellebyte]

Good to know :) THX

[tuminoid]

/cc @lentzi90 @Rozzii @elfosardo @dtantsur

PTAL

[lentzi90]

I don't know this well enough to review properly, just noting that this may count as ✨ instead of 🐛 maybe?

[Cellebyte]

@lentzi90 I am not sure if it counts as ✨ .
I think I need to still add tests for ther kernel URL ramdisk URL part :)

[Cellebyte]

/retest

[Cellebyte]

So the tests are added. And everything is green :)

[tuminoid]

So the tests are added. And everything is green :)

Yay :) Can you squash the commit please.

[Cellebyte]

I can :) Will do that today :)

[Cellebyte]

/retest

[tuminoid]

/cc @zaneb @dtantsur @elfosardo
Let's get more eyes on this meanwhile.

[Cellebyte]

Can this be merged?

[zaneb]

If people rely on the behaviour, it could break hosts using deploy_interface: direct after this MR is implemented as it is currently drafted.

We do currently rely on this behaviour in OpenShift. So I assume that this change will break us and 100% of other people who are using the environment variable (if any).

In the bug you said that "you could also run a Live ISO like system using the direct deploy_interface" but could means you also could not do that, so setting it in an environment variable where it affects every BMH sounds like almost certainly the wrong way to configure it. For live ISO, users opt in to the configuration by selecting the live-iso image format. (TBH I have no idea why it is even configurable and not baked in to live-iso handling.)

[Cellebyte]

I mean I could also add another environment variable which configures it for the direct interface. This would not break it for users which use both ways for their BMHs and only want to set it for live-iso.

[Rozzii]

/hold cancel
Removing the hold because 0.11 has been released.

I mean I could also add another environment variable which configures it for the direct interface. This would not break it for users which use both ways for their BMHs and only want to set it for live-iso.

This sounds reasonable IMO.

[Rozzii]

The code looks good but it will be reworked to provide separately configurable persistent boot device behavior.

[Cellebyte]

@Rozzii I will update this PR to accomodate for it :)

[Cellebyte]

PR is updated to accomodate for the feedback.

[Cellebyte]

@Rozzi can this one be merged?

[MahnoorAsghar]

I think the PR description needs to be updated about renaming the LIVE_ISO_FORCE_PERSISTENT_BOOT_DEVICE env variable, which is now backwards-compatible

[Rozzii]

/approve
Sorry for the late reply.

[metal3-io-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rozzii

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

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

[Cellebyte]

@zaneb is this now ok to merge? :)

[zaneb]

This seems correctly-named and shouldn't break anybody now.

[Cellebyte]

Can this be merged into the v0.12 + main?