Allow default ignition template to be overridden #536
Conversation
asergeant01
requested review from
afritzler,
nagadeesh-nagaraja and
stefanhipfel
github-actions
bot
added
size/XL
documentation
nagadeesh-nagaraja
left a comment
nagadeesh-nagaraja
left a comment
There was a problem hiding this comment.
may be we can make sure that the changes will not break the existing workflows in metal-api?
asergeant01
force-pushed
the
default-ipxe-template
branch
from
321532c to
b9ff7d4
Compare
|
I got two questions, sorry if I am missing something obvious:
|
|
My use case is trying to run e2e locally with a virtualised environment. I Intercept redfish calls to start server (instead creates a qemu vm) and likewise with stop server (stops the qemu vm). This allows me to observe the pxe boot and the metal probe execution - however, there are obviously some modifications I need to make to the ignition file and currently there is no way to override the hardcoded default. This implementation should not change the default behaviour of using the hardcoded ignition template, but add the option to use one provided from a configmap. Regarding could it be mounted and read - absolutely if you think this is a better approach. Curious as to the benefits of this approach? |
nagadeesh-nagaraja
left a comment
nagadeesh-nagaraja
left a comment
There was a problem hiding this comment.
LGTM from my side. please check on others comment if this works :)
asergeant01
force-pushed
the
default-ipxe-template
branch
3 times, most recently
from
3ea6950 to
b234a2e
Compare
It would simply the code, because you only have to |
asergeant01
force-pushed
the
default-ipxe-template
branch
from
b234a2e to
f8de4dd
Compare
asergeant01
force-pushed
the
default-ipxe-template
branch
2 times, most recently
from
93c1551 to
4bca890
Compare
asergeant01
force-pushed
the
default-ipxe-template
branch
from
4bca890 to
772cee3
Compare
asergeant01
force-pushed
the
default-ipxe-template
branch
from
946f988 to
441cb68
Compare
afritzler
changed the title
| @@ -1,51 +0,0 @@ | |||
| // SPDX-FileCopyrightText: 2025 SAP SE or an SAP affiliate company and IronCore contributors | |||
There was a problem hiding this comment.
Why moving those helper into a new file?
There was a problem hiding this comment.
So this was my attempt at reducing the sporadic test failures by making the helper a test in itself so I'd have access to the k8sclient. I'm going to revert this.
| LABEL source_repository="https://github.com/ironcore-dev/metal-operator" | ||
| WORKDIR / | ||
| COPY --from=manager-builder /workspace/manager . | ||
| COPY config/manager/ignition-template.yaml /etc/metal-operator/ignition-template.yaml |
There was a problem hiding this comment.
Can't we just load the in-code var as a default - meaning leaving the existing behavior if no override file is provided?
There was a problem hiding this comment.
We could, however I think it's clearer for an end user that the discovery ignition is always loaded from a file. To me it reduces ambiguity, easier debugging and reduces code.
asergeant01
force-pushed
the
default-ipxe-template
branch
from
37e157a to
3dd31da
Compare
WalkthroughAdds file-based ignition template support: a CLI flag to set the template path, propagation into the reconciler, template rendering from disk, tests for file/template behaviors, a default ignition template file, Dockerfile copy of the template, and probe resilience improvements to continue on data-collection errors. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as Manager CLI
participant Reconciler as ServerReconciler
participant Controller as Server Controller
participant Engine as Ignition Engine
participant FS as File System
CLI->>Reconciler: start with discovery-ignition-path
Reconciler->>Controller: initialize with DiscoveryIgnitionPath
Controller->>Controller: prepare ignition config
Controller->>Engine: GenerateIgnitionDataFromFile(path, config)
Engine->>FS: ReadFile(path)
FS-->>Engine: template content
Engine->>Engine: render template with config
Engine-->>Controller: rendered ignition bytes / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/probe/probe.go (1)
42-50: Inconsistent error handling between network data and system info.The
collectNetworkData()call (lines 42-45) still returns errors immediately, whilecollectSystemInfoData()(lines 46-51) now logs errors and continues with empty data. This inconsistency creates unclear expectations about which data is required vs optional.Consider either:
- Making network interface collection tolerant like other collectors (if it's also optional)
- Documenting which data is required vs optional for server registration
internal/controller/server_controller.go (1)
711-728: Add validation ofDiscoveryIgnitionPathat operator startup.The ignition data generation correctly uses
GenerateIgnitionDataFromFile, which validates the path during reconciliation and returns a helpful error message. However,DiscoveryIgnitionPathis not validated at operator startup. Currently, the operator will only fail when the first Server resource attempts reconciliation if the file is missing or unreadable. Add an early check incmd/manager/main.goto verify the file exists before starting the reconciler, so failures are detected immediately at startup rather than during reconciliation.
🤖 Fix all issues with AI agents
In @config/manager/ignition-template.yaml:
- Around line 5-19: The docker-install.service unit is using Debian's apt-get
(ExecStart=/usr/bin/apt-get update and ExecStart=/usr/bin/apt-get install ...)
which will fail on Fedora CoreOS; update the template for FCOS by removing
apt-get commands and either (a) convert the unit to enable/use podman (e.g.,
replace docker-install.service with a podman-ready unit or enable an existing
podman service) or (b) document/implement an FCOS-compatible Docker installation
approach (rpm-ostree layering or image rebase) if Docker is mandatory;
specifically update the service named docker-install.service and its ExecStart
entries to reference podman or an FCOS installation method and clarify in the
template which OS variant the unit targets.
🧹 Nitpick comments (2)
config/manager/ignition-template.yaml (1)
30-32: Consider error handling for image pull failures.Line 32 will fail the service startup if the image cannot be pulled (e.g., in offline scenarios or if the registry is temporarily unavailable). If the operator should tolerate such scenarios and use a cached image, consider adding the
-prefix:- ExecStartPre=/usr/bin/docker pull {{.Image}} + ExecStartPre=-/usr/bin/docker pull {{.Image}}However, if you want to ensure the latest image is always used and fail fast when it's not available, the current approach is correct.
internal/controller/server_controller_test.go (1)
395-456: Consider reducing template duplication.The test creates a temporary file with a hardcoded template string (lines 402-445) that duplicates the content of
config/manager/ignition-template.yaml. This creates a maintenance burden as changes to the default template need to be synchronized across multiple locations.Consider either:
- Reading from the actual template file at
config/manager/ignition-template.yaml- Extracting the template to a test fixture
- Documenting that this intentionally tests a specific template version
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/chart/IGNITION-CUSTOMIZATION.mdis excluded by!**/dist/**dist/chart/templates/configmap/ignition-template.yamlis excluded by!**/dist/**dist/chart/templates/manager/manager.yamlis excluded by!**/dist/**dist/chart/values.yamlis excluded by!**/dist/**
📒 Files selected for processing (10)
DockerfileMakefilecmd/manager/main.goconfig/manager/ignition-template.yamlinternal/controller/server_controller.gointernal/controller/server_controller_test.gointernal/controller/suite_test.gointernal/ignition/default.gointernal/ignition/default_test.gointernal/probe/probe.go
🧰 Additional context used
🧬 Code graph analysis (4)
internal/controller/server_controller.go (1)
internal/ignition/default.go (2)
Config(14-19)GenerateIgnitionDataFromFile(22-33)
internal/ignition/default_test.go (1)
internal/ignition/default.go (2)
Config(14-19)GenerateIgnitionDataFromFile(22-33)
internal/probe/probe.go (7)
internal/api/registry/dmi.go (1)
DMI(6-10)internal/api/registry/cpu.go (1)
CPUInfo(6-13)internal/api/registry/lldp.go (1)
LLDP(9-11)internal/api/registry/block.go (1)
BlockDevice(6-22)internal/api/registry/memory.go (1)
MemoryDevice(6-21)internal/api/registry/nic.go (1)
NIC(6-14)internal/api/registry/pcidev.go (1)
PCIDevice(6-13)
internal/controller/server_controller_test.go (2)
internal/ignition/default.go (2)
GenerateIgnitionDataFromFile(22-33)Config(14-19)internal/controller/server_controller.go (2)
SSHKeyPairSecretPublicKeyName(54-54)ServerReconciler(79-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run tests
- GitHub Check: Run linter
- GitHub Check: Run test e2e
- GitHub Check: Run test chart
🔇 Additional comments (16)
Makefile (1)
64-64: LGTM: Quoting improves path handling robustness.The addition of quotes around tool binary paths across multiple Make targets is a good practice that ensures the build system handles paths with spaces or special characters correctly.
Also applies to: 68-69, 73-73, 84-84, 127-127, 131-131, 135-135, 148-148, 152-152, 161-161, 215-216, 226-227, 234-235, 239-240, 244-244, 248-248
internal/probe/probe.go (1)
46-95: Verify relationship to PR objectives.The changes making data collectors tolerant to failures appear unrelated to the PR's stated objective of allowing ignition template override. While the PR description mentions running e2e locally in virtualized environments, the connection between probe data collection tolerance and ignition template customization isn't clear.
Could this be a separate concern that should be in its own PR, or is there a specific reason probe resilience is needed for custom ignition templates?
Dockerfile (1)
42-42: LGTM: Ignition template correctly copied into container.The ignition template file is properly copied to the expected location
/etc/metal-operator/ignition-template.yaml, which aligns with the default flag value incmd/manager/main.go.internal/ignition/default_test.go (1)
1-131: LGTM: Comprehensive test coverage for file-based ignition generation.The test suite thoroughly covers:
- Successful generation from file templates with proper substitutions
- Error handling for non-existent files, invalid templates, and empty paths
- Both file-based and template-based rendering flows
- Proper resource cleanup with
DeferCleanupcmd/manager/main.go (2)
119-120: LGTM: CLI flag properly configured.The
discovery-ignition-pathflag has a clear description and a sensible default that matches the container path in the Dockerfile.
357-357: LGTM: Ignition path correctly wired into ServerReconciler.The
DiscoveryIgnitionPathis properly passed to theServerReconciler, completing the configuration chain from CLI flag to reconciler.internal/controller/suite_test.go (1)
195-196: LGTM!The initialization of
DiscoveryIgnitionPathis correct and aligns with the test directory structure.internal/controller/server_controller.go (1)
95-95: LGTM!The addition of
DiscoveryIgnitionPathfield to theServerReconcileris appropriate and follows the existing pattern.config/manager/ignition-template.yaml (1)
39-44: LGTM!The user configuration with template placeholders for password hash and SSH key is correct.
internal/ignition/default.go (2)
21-33: LGTM!The
GenerateIgnitionDataFromFilefunction properly validates the file path and handles errors appropriately. Error messages include the file path for debugging.
35-49: LGTM!The
generateIgnitionDataFromTemplatehelper function correctly handles template parsing and execution with appropriate error handling.internal/controller/server_controller_test.go (5)
197-206: LGTM!The test correctly uses
GenerateIgnitionDataFromFilewith the template file path, properly migrating from the previous in-memory approach.
858-896: LGTM!The test context setup with
BeforeEachis well-structured and reduces duplication across test cases.
898-957: LGTM!This test case provides good coverage of the custom ignition template feature, validating that custom templates are correctly rendered with expected content.
959-1014: LGTM!These test cases provide excellent coverage of error scenarios, ensuring the system fails gracefully with appropriate error messages when files are missing or templates are invalid.
1016-1127: LGTM!The default template and end-to-end tests provide comprehensive validation of the file-based ignition template feature, ensuring both default and custom scenarios work correctly.
| - name: docker-install.service | ||
| enabled: true | ||
| contents: |- | ||
| [Unit] | ||
| Description=Install Docker | ||
| Before=metalprobe.service | ||
| [Service] | ||
| Restart=on-failure | ||
| RestartSec=20 | ||
| Type=oneshot | ||
| RemainAfterExit=yes | ||
| ExecStart=/usr/bin/apt-get update | ||
| ExecStart=/usr/bin/apt-get install docker.io docker-cli -y | ||
| [Install] | ||
| WantedBy=multi-user.target |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Fedora CoreOS rpm-ostree package manager container runtime Docker Podman pre-installed
💡 Result:
Short answer
- Fedora CoreOS is managed with rpm-ostree (atomic OSTree images + optional layering). [1][2]
- You can layer packages with rpm-ostree or rebase to a custom FCOS image, but layering is intended for debugging/custom images and generally discouraged for production. Reboot is required for layered installs. [2][3]
- Podman (daemonless) is included and is the recommended user-facing container tool. [2]
- Docker/Moby is not the recommended runtime: FCOS has docker.socket activated (docker.service disabled by default) so the docker daemon can be socket-activated if needed, but mixing Docker and Podman is discouraged. Kubernetes/OKD use CRI-O on FCOS. [1][4]
Sources
[1] Fedora CoreOS FAQ / Using containers (Fedora Docs).
[2] Fedora CoreOS migration & OS-extensions / rpm-ostree docs (Fedora Docs & rpm-ostree).
[3] rpm-ostree layering tutorial (coreos.github.io).
[4] OKD / FCOS container runtime (CRI-O) documentation.
Fix package manager compatibility with FCOS.
Fedora CoreOS uses rpm-ostree for system management and does not include apt-get (which is for Debian/Ubuntu systems). The ExecStart commands in this service will fail during boot. FCOS comes with podman pre-installed and mixing Docker with Podman is discouraged on FCOS.
Either:
- Use
podmaninstead of Docker (recommended for FCOS) - Install Docker using FCOS-compatible methods (rpm-ostree layering or custom image rebase)
- Clarify if this template is intended for a different OS variant
🤖 Prompt for AI Agents
In @config/manager/ignition-template.yaml around lines 5 - 19, The
docker-install.service unit is using Debian's apt-get
(ExecStart=/usr/bin/apt-get update and ExecStart=/usr/bin/apt-get install ...)
which will fail on Fedora CoreOS; update the template for FCOS by removing
apt-get commands and either (a) convert the unit to enable/use podman (e.g.,
replace docker-install.service with a podman-ready unit or enable an existing
podman service) or (b) document/implement an FCOS-compatible Docker installation
approach (rpm-ostree layering or image rebase) if Docker is mandatory;
specifically update the service named docker-install.service and its ExecStart
entries to reference podman or an FCOS installation method and clarify in the
template which OS variant the unit targets.
3dd31da to
01673c8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/probe/probe.go (1)
42-44: Apply resilience pattern to network interface collection for consistency.Network interface collection failures still cause
Init()to fail and return an error, while all other data sources (system info, CPU, LLDP, storage, memory, NICs, PCI) gracefully continue with empty/default data. TheNetworkInterfacesfield is markedomitemptyin the API struct like all other fields, and no validation logic requires it. Consider handling errors the same way as other data sources: log the error and continue with an empty slice for consistency.
🤖 Fix all issues with AI agents
In @internal/controller/server_controller_test.go:
- Line 858: Rename the test context string in server_controller_test.go from
"ConfigMap-based Ignition Templates" to a name that reflects the file-based
implementation (e.g., "File-based Ignition Templates") by updating the
Context(...) invocation used around the related tests so the description matches
the actual file-based template behavior.
- Around line 881-882: Remove the deprecated UUID field assignment in the test
fixture: delete the `UUID: "38947555-7742-3448-3784-823347823834",` entry and
only set `SystemUUID` (e.g., `SystemUUID:
"38947555-7742-3448-3784-823347823834",`) so the test matches the controller
behavior in bmc_controller.go (where server.Spec.UUID is cleared) and relies
solely on `SystemUUID`.
🧹 Nitpick comments (1)
internal/controller/server_controller_test.go (1)
395-456: Consider: Template duplication in tests.The test creates a temporary file with an inline template (lines 402-445). This duplicates template content rather than using the actual template file from
config/manager/ignition-template.yaml.While this approach provides test isolation and allows testing specific template variations, it creates maintenance overhead if the template structure changes. Consider whether some tests could reference the actual template file for validation.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
dist/chart/IGNITION-CUSTOMIZATION.mdis excluded by!**/dist/**dist/chart/templates/configmap/ignition-template.yamlis excluded by!**/dist/**dist/chart/templates/manager/manager.yamlis excluded by!**/dist/**dist/chart/values.yamlis excluded by!**/dist/**
📒 Files selected for processing (10)
DockerfileMakefilecmd/manager/main.goconfig/manager/ignition-template.yamlinternal/controller/server_controller.gointernal/controller/server_controller_test.gointernal/controller/suite_test.gointernal/ignition/default.gointernal/ignition/default_test.gointernal/probe/probe.go
🚧 Files skipped from review as they are similar to previous changes (5)
- Dockerfile
- internal/controller/server_controller.go
- Makefile
- cmd/manager/main.go
- config/manager/ignition-template.yaml
🧰 Additional context used
🧬 Code graph analysis (3)
internal/probe/probe.go (8)
internal/api/registry/dmi.go (1)
DMI(6-10)internal/api/registry/cpu.go (1)
CPUInfo(6-13)cmd/metalprobe/main.go (2)
LLDPSyncInterval(24-24)LLDPSyncDuration(25-25)internal/api/registry/lldp.go (1)
LLDP(9-11)internal/api/registry/block.go (1)
BlockDevice(6-22)internal/api/registry/memory.go (1)
MemoryDevice(6-21)internal/api/registry/nic.go (1)
NIC(6-14)internal/api/registry/pcidev.go (1)
PCIDevice(6-13)
internal/ignition/default_test.go (1)
internal/ignition/default.go (2)
Config(14-19)GenerateIgnitionDataFromFile(22-33)
internal/controller/server_controller_test.go (2)
internal/ignition/default.go (2)
GenerateIgnitionDataFromFile(22-33)Config(14-19)internal/controller/server_controller.go (2)
SSHKeyPairSecretPublicKeyName(54-54)ServerReconciler(79-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run test chart
- GitHub Check: Run test e2e
- GitHub Check: Run tests
🔇 Additional comments (5)
internal/probe/probe.go (1)
46-95: Good resilience improvements.The error handling pattern allows the probe to continue with partial data when individual collection steps fail. This prevents complete initialization failure due to unavailable/inaccessible hardware information sources like DMI/SMBIOS, LLDP, etc.
The logging is clear and indicates which data sources were skipped.
internal/ignition/default.go (1)
21-49: LGTM: Clean file-based template rendering implementation.The code properly:
- Validates input (empty path check)
- Handles file read errors with descriptive messages
- Separates concerns (file I/O vs template rendering)
- Uses proper error wrapping
The helper function
generateIgnitionDataFromTemplateprovides good separation for testability.internal/ignition/default_test.go (1)
1-131: LGTM: Comprehensive test coverage.The test suite thoroughly covers:
- Successful template rendering from files
- Error paths (missing files, invalid templates, empty paths)
- Direct template rendering with the helper function
- Template parsing/execution errors
Proper use of temporary files with cleanup and clear test assertions.
internal/controller/suite_test.go (1)
195-196: LGTM: Proper test configuration for file-based ignition.The
DiscoveryIgnitionPathcorrectly points to the template file using a relative path, andDiscoveryTimeoutis appropriately shortened for faster test execution.internal/controller/server_controller_test.go (1)
197-206: LGTM: Updated to use file-based template rendering.The test now correctly uses
GenerateIgnitionDataFromFilewith the actual template file path, aligning with the new file-based approach.
| Eventually(Get(&bootConfig)).Should(Satisfy(apierrors.IsNotFound)) | ||
| }) | ||
|
|
||
| Context("ConfigMap-based Ignition Templates", func() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Update context name to reflect file-based implementation.
The context is named "ConfigMap-based Ignition Templates" but the implementation uses file-based templates. According to the PR discussion, the design shifted from ConfigMap to file-based approach after feedback.
📝 Update context name
- Context("ConfigMap-based Ignition Templates", func() {
+ Context("File-based Ignition Templates", func() {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Context("ConfigMap-based Ignition Templates", func() { | |
| Context("File-based Ignition Templates", func() { |
🤖 Prompt for AI Agents
In @internal/controller/server_controller_test.go at line 858, Rename the test
context string in server_controller_test.go from "ConfigMap-based Ignition
Templates" to a name that reflects the file-based implementation (e.g.,
"File-based Ignition Templates") by updating the Context(...) invocation used
around the related tests so the description matches the actual file-based
template behavior.
| UUID: "38947555-7742-3448-3784-823347823834", | ||
| SystemUUID: "38947555-7742-3448-3784-823347823834", |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for Server struct definition
ast-grep --pattern 'type Server struct {
$$$
}'Repository: ironcore-dev/metal-operator
Length of output: 2039
🏁 Script executed:
# Search for uses of Spec.UUID in the codebase
rg -nP --type=go 'Spec\.UUID\b' -A2 -B2Repository: ironcore-dev/metal-operator
Length of output: 522
🏁 Script executed:
# Get context around lines 881-882 in the test file
sed -n '870,895p' internal/controller/server_controller_test.goRepository: ironcore-dev/metal-operator
Length of output: 826
🏁 Script executed:
# Find ServerSpec definition
rg -nP --type=go 'type ServerSpec struct' -A 30 api/v1alpha1/server_types.goRepository: ironcore-dev/metal-operator
Length of output: 1465
🏁 Script executed:
# Search for UUID field usage (not Spec.UUID) to understand its purpose
rg -nP --type=go 'UUID\s*:' -B 2 -A 2 api/v1alpha1/server_types.go | head -50Repository: ironcore-dev/metal-operator
Length of output: 53
🏁 Script executed:
# Check if UUID field is referenced in any controller logic
rg -nP --type=go '\.UUID\b' internal/controller/ -B2 -A2Repository: ironcore-dev/metal-operator
Length of output: 1034
Remove the redundant UUID field assignment.
The UUID field is deprecated in favor of SystemUUID and is explicitly cleared by the controller (see bmc_controller.go line 251: server.Spec.UUID = ""). Only SystemUUID is used in the actual logic. Remove the UUID assignment on line 881 to match the controller's behavior.
🤖 Prompt for AI Agents
In @internal/controller/server_controller_test.go around lines 881 - 882, Remove
the deprecated UUID field assignment in the test fixture: delete the `UUID:
"38947555-7742-3448-3784-823347823834",` entry and only set `SystemUUID` (e.g.,
`SystemUUID: "38947555-7742-3448-3784-823347823834",`) so the test matches the
controller behavior in bmc_controller.go (where server.Spec.UUID is cleared) and
relies solely on `SystemUUID`.
Proposed Changes
The current default ignition is hardcoded. This change allows it to be optionally overridden by a ConfigMap.
Summary by CodeRabbit
New Features
Bug Fixes
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.