Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #36691 - use 'connectefi scsi' by default #9810

Merged
merged 1 commit into from
Apr 23, 2024

Conversation

sbernhard
Copy link
Contributor

No description provided.

@theforeman-bot
Copy link
Member

Issues: #36691

@sbernhard
Copy link
Contributor Author

[test unit]

@sbernhard sbernhard force-pushed the fix_36691 branch 2 times, most recently from 3ea4dd2 to 5976312 Compare December 22, 2023 14:29
@sbernhard
Copy link
Contributor Author

Failed tests are unrelated to this PR.

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cross linking: #10118

@sbernhard sbernhard force-pushed the fix_36691 branch 3 times, most recently from ca2358c to d12176d Compare April 8, 2024 13:12
@sbernhard
Copy link
Contributor Author

Many tests failed but I think they are not related to this PR.

@jloeser
Copy link

jloeser commented Apr 15, 2024

JFYI: PR #10126 could make this connectefi scsi approach obsolete.

If #9864 gets merged, this will probably not work anymore because most distributions don't have GRUB2 connectefi module installed.

I would make connectefi scsi opt-in.

@jloeser
Copy link

jloeser commented Apr 15, 2024

There was the idea from @goarsna to let GRUB2 find out if connectefi module is supported during runtime. With lsmod we can see if a module is available (or not). But I couldn't find anything quickly parse output and end up with a if-condition.

@jloeser
Copy link

jloeser commented Apr 16, 2024

Having connectefi module not available and running the command does not harm (just an unknown command):

Failed to find fs: Unsupported
Fetching Netboot Image
error: ../../grub-core/script/function.c:119:can't find command `connectefi'.
next instruction ...
SecureBoot enabled?
y

GRUB config:

connectefi scsi
echo "next instruction ..."
echo "SecureBoot enabled?"
echo $lockdown
sleep 10

Means if we enable it by default and the module ...

  • is available, chainloading would work as expected
  • is not available, we end up as before with this error message - which doesn't hurt

Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a tests file from my PR here too to have some coverage?

Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nitpick and we're done

@sbernhard
Copy link
Contributor Author

sbernhard commented Apr 17, 2024

Looks like the tests are green. The other option with truthy did not work because the value can be scsi or pciroot, too

ShimShtein
ShimShtein previously approved these changes Apr 18, 2024
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be ok with this if it was verified by someone, but slightly concerned the test failures could be related

@ekohl
Copy link
Member

ekohl commented Apr 22, 2024

Having connectefi module not available and running the command does not harm (just an unknown command):

I get the impression our CI disagrees. It says Invalid command and fails with Out of Memory on a grub check. So either the grub check is incorrect, or it breaks on non-patched grub.

Co-Authored-by: Shimon Shtein <[email protected]>
Co-Authored-by: Ewoud Kohl van Wijngaarden <[email protected]>
@sbernhard
Copy link
Contributor Author

sbernhard commented Apr 22, 2024

Thank you very much @ekohl and grub-script-check
Forget a a closing " (double-quote).

17:38 $ LANG=C grub-script-check unit/foreman/renderer/snapshots/ProvisioningTemplate/PXEGrub2/PXEGrub2_default_local_boot.host4dhcp.snap.txt
error: out of memory.
error: syntax error.
error: Incorrect command.
error: syntax error.
Syntax error at line 223

@stejskalleos stejskalleos requested a review from ekohl April 23, 2024 07:05
@stejskalleos
Copy link
Contributor

All tests are green, @ekohl if you okay I'm okay to merge

@ekohl
Copy link
Member

ekohl commented Apr 23, 2024

Yay for CI. Verifying snapshots with external validator tools has proven useful.

@ekohl ekohl merged commit 7a77b3c into theforeman:develop Apr 23, 2024
51 checks passed
@Lennonka
Copy link

Lennonka commented Jun 24, 2024

I'm a bit worried this might not work as expected for another than global host parameter if any.

To my knowledge, the including templates (global default and local boot) are rendered once and the same config is deployed to all SmartProxies (AKA the Build PXE default button). In other words, the scope in which the snippet is rendered, is not a host, but it's the Foreman server itself.
Therefore, the templates are indifferent towards hosts, subnets, organizations..., I would expect. And if that's true, the unit test doesn't represent a valid test case and its results are inconclusive.

I'd be more than happy to be proven wrong.

@jloeser
Copy link

jloeser commented Jun 25, 2024

To my knowledge, the including templates (global default and local boot) are rendered once and the same config is deployed to all SmartProxies (AKA the Build PXE default button). In other words, the scope in which the snippet is rendered, is not a host, but it's the Foreman server itself.

Partly true. The "local boot" template is host scoped and gets rendered whenever a host's build state changes.

If you render non-host scoped global template (Build PXE default button), the connectefi command is absent. In this case it can happen that hosts cannot chainload from disk if they are not managed by Foreman (means there is no local boot template). Then one would need to set connectefi scsi manually (as before).

Two things here:

  1. Executing connectefi scsi does not harm, even if the command does not exist in GRUB2; in the best case, hosts will boot up successfully, or not (as before)
  2. We plan to get rid of chainloader command at all which would make this connectefi command completely obsolete. Idea is to simply load the configuration file (see Fixes #37562 - Fix local disk boot over network #10207)

@Lennonka
Copy link

Thank you so much for the clarification

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants