-
Notifications
You must be signed in to change notification settings - Fork 269
Nvidia Bluefield ignition provider #2151
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new Ignition provider for Nvidia BlueField DPUs. The provider reads the Ignition config from the bootfifo device. The implementation is straightforward and correct. I've made a couple of suggestions to improve code style and maintainability: one regarding Go's idiomatic error handling, and another about package naming conventions for consistency with other providers in the project.
5161388 to
58bf0e3
Compare
|
CI is reporting: which I think wants you to update https://github.com/coreos/ignition/blob/main/docs/supported-platforms.md |
|
I'm testing this now. Can you squash all commits down into 1? |
8f808f4 to
a678803
Compare
|
All of the previous commits are now squashed into one :) |
|
I hit trouble here because the ignition/dracut/30ignition/module-setup.sh Line 117 in c43db71
ignition/internal/providers/qemu/qemu_fwcfg.go Lines 53 to 56 in c43db71
|
4dcca53 to
b79667c
Compare
|
I added the dracut generation step and it worked immediately on an RHCOS pullspec image, even without the modprobe section. I included the modprobe part afterward just to be safe. |
dracut/30ignition/module-setup.sh
Outdated
|
|
||
| # required by nvidiabluefield platform to read ignition file through bootfifo sysfs interface | ||
| if [[ ${DRACUT_ARCH:-$(uname -m)} == aarch64 ]]; then | ||
| instmods -c mlxbf-bootctl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think typically underscores are used here:
| instmods -c mlxbf-bootctl | |
| instmods -c mlxbf_bootctl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, are we sure this won't apply to x86_64 at some point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’ll likely stay aarch64 for the foreseeable future, given that current BlueField models are based on ARM SoCs.
It seems like this kernel module is not built on x86 at the major distros.
|
Can you update the release notes with an entry for this new platform? https://github.com/coreos/ignition/blob/main/docs/release-notes.md |
500e9a5 to
5d91b2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my perspective the changes make sense and are conforming to my expectation for adding this platform.
LGTM, thank you all for working through this.
My only nit is the commit message.
I would prefer it to look more like "providers/nvidiabluefield: add initial support" but not a blocker.
5d91b2a to
5314ede
Compare
5314ede to
3fbad83
Compare
|
I tested this in RHCOS by running a version of the make_bfb.sh with: and providing an Ignition config like: Basically this is the Ignition config for the install boot that drops down an installer config and then also the Ignition for the first boot delivered to Here's the outdated make_bfb.sh |
dustymabe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
nvidiabluefieldis an ignition provider for Nvidia BlueField DPUs.It uses the
bootfifodevice provided by themlxbf-bootctlkernel module to pull the ignition.Previous discussion is at issue #2148