Bug OCPBUGS-98696: Embed IP_OPTIONS kargs in provisioning ISO#182
Bug OCPBUGS-98696: Embed IP_OPTIONS kargs in provisioning ISO#182jacob-anders wants to merge 2 commits into
Conversation
In OCP 4.22, the installer reuses the bootstrap VM's live ISO as the provisioning ISO served to target bare metal nodes. The bootstrap ISO has console=ttyS0 embedded in its kargs area, which causes lockups on hardware that requires specific serial port configuration (baud rate, parity, etc). Additionally, ip=dhcp — previously embedded by the now-removed copy-metal/extract-machine-os.sh step — is missing. Pass the IP_OPTIONS environment variable (e.g. "ip=dhcp") as kernel arguments to NewRHCOSStreamReader when serving ISO images. The isoeditor replaces the entire kargs placeholder area in the ISO's GRUB config, so console=ttyS0 is overwritten with ip=dhcp, restoring parity with the 4.21 provisioning ISO kernel command line. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jacob-anders The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughChangesThe ISO image path now stores configured kernel arguments and passes them to the RHCOS stream reader during Ignition insertion, with a fallback to reader creation without kernel arguments. ISO kernel arguments
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant NewImageHandler
participant baseIso
participant RHCOSStreamReader
NewImageHandler->>baseIso: construct with IP options
baseIso->>RHCOSStreamReader: create reader with kernel argument bytes
RHCOSStreamReader-->>baseIso: return reader or error
baseIso->>RHCOSStreamReader: retry without kernel arguments on error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
/cc @zaneb @jacob-anders you need to attach a bug number instead of |
jacob-anders
changed the title
openshift-ci-robot
added
jira/severity-important
|
@jacob-anders: This pull request references Jira Issue OCPBUGS-98696, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Done. I was making some cluster-bot builds for 5.0 and 4.22 so wanted to make sure I remember which is which, can tidy this up now. |
|
/retest |
When the source ISO already has kargs embedded (e.g. console=ttyS0 set by the installer's createLiveVolume), the COREOS_KARG_EMBED_AREA regex no longer matches, causing NewRHCOSStreamReader to fail. This prevented ICC from serving the provisioning ISO at all, breaking node provisioning. Retry without kargs modification so provisioning continues to work even when the embed area cannot be rewritten. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/retest-required |
|
/test e2e-metal-ipi-ovn-ipv4 |
|
/test e2e-metal-ipi-serial-ipv4 |
|
/retest-required |
|
/test e2e-metal-ipi-ovn-ipv6 |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/test e2e-metal-ipi-ovn-ipv4 |
|
/test e2e-metal-ipi-virtualmedia |
1 similar comment
|
/test e2e-metal-ipi-virtualmedia |
|
/retest-required |
1 similar comment
|
/retest-required |
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
I think your diagnosis is correct: I neglected to add a mechanism for setting the IP_OPTIONS when I stopped using copy-metal. Then later the console=ttyS0 arg was added and the IPA image inherited it.
However, a couple of problems with this fix:
The isoeditor replaces the entire kargs placeholder area in the ISO's GRUB config, so
console=ttyS0is overwritten withip=dhcp
Claude is lying to you here, this is made up. The embed area as detected by assisted-image-service starts after any existing args. So arguments are only appended.
When the source ISO already has kargs embedded (e.g. console=ttyS0 set
by the installer's createLiveVolume), the COREOS_KARG_EMBED_AREA regex
no longer matches, causing NewRHCOSStreamReader to fail.
This commit effectively undoes the effect of the previous one, it's saying that the fix always fails so we will always fall back to the previous code. So if it's right then this PR can have no effect.
Most likely the reason for the failure is that NewRHCOSStreamReader (as used in the installer, not here) does not automatically re-add the newline at the end of the kernel args, unlike some of the other (cough better-designed) APIs for kargs in assisted-image-service.
Finally, looking at the original commit that added the IP_OPTIONS to i-c-c:
The IP_OPTIONS we need to use for the provisioned node (which has to
pull its Ignition from the external network) are not necessarily the
same as the ones used for starting IPA (which has to talk to ironic via
the provisioning network if it exists).
So the IP_OPTIONS we have here are the wrong ones - they're the ones that the cluster node should get, but we want the ones that IPA should get. Previously the bootstrap was setting it to ${PROVISIONING_IP_OPTIONS} in the ISO, whereas i-c-c gets ${EXTERNAL_IP_OPTIONS}.
Sooo... this is really 2 bugs:
- Failing to set the
ip=karg. The easiest way to handle this would be to set it in the bootstrap VM, since I'm pretty sure the bootstrap should have the same setting as the IPAs we are booting (i.e.${PROVISIONING_IP_OPTIONS})? - Presence of the
console=ttyS0option breaking some hardware. There's no easy way to overwrite this. TBH it's probably easiest to just add the baud/parity data to it.
| } | ||
| reader, err := isoeditor.NewRHCOSStreamReader(biso.filename, ignition, nil, kargsBytes) | ||
| if err != nil && kargsBytes != nil { | ||
| // The kargs embed area may already be occupied (e.g. the installer |
There was a problem hiding this comment.
I don't understand this. The embed area calculated by NewRHCOSStreamReader() by definition begins after the bit that is occupied. There's no reason to think this ought to fail unless the image is corrupt or in an unexpected format. Which I'd rather know about in CI than have it start silently failing to set the kargs.
| switch osImage.kind { | ||
| case imageKindISO: | ||
| isoFiles[key] = newBaseIso(filePath) | ||
| isoFiles[key] = newBaseIso(filePath, envInputs.IpOptions) |
There was a problem hiding this comment.
If we were overwriting the existing kargs, we would also need to pass the FIPS mode here; that is another thing that was set in copy-iso. However, at the moment that will already be set for us in the ISO used to create the bootstrap VM so we are ok.
| reader, err := isoeditor.NewRHCOSStreamReader(biso.filename, ignition, nil, kargsBytes) | ||
| if err != nil && kargsBytes != nil { | ||
| // The kargs embed area may already be occupied (e.g. the installer | ||
| // embedded console= params into this ISO). Fall back to serving the |
There was a problem hiding this comment.
Handling this case is the whole point of the PR according to Claude, so weird to me that it would have us ignore it.
|
@jacob-anders: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
Thank you for your review, Zane. Claude misled me a fair bit and since I work less with these parts of the code it was hard for me to realise this.
I have to say I do like the idea of just adding those two fixes to the bootstrap and retesting. I will disregard existing changes try this and report back. |
|
This direction was a dead end, we took a different approach in openshift/installer#10702 and this seems to resolve the issue. Closing this PR. |
4 participants
In OCP 4.22, the installer reuses the bootstrap VM's live ISO as the provisioning ISO served to target bare metal nodes. The bootstrap ISO has console=ttyS0 embedded in its kargs area, which causes lockups on hardware that requires specific serial port configuration (baud rate, parity, etc). Additionally, ip=dhcp — previously embedded by the now-removed copy-metal/extract-machine-os.sh step — is missing.
Pass the IP_OPTIONS environment variable (e.g. "ip=dhcp") as kernel arguments to NewRHCOSStreamReader when serving ISO images. The isoeditor replaces the entire kargs placeholder area in the ISO's GRUB config, so console=ttyS0 is overwritten with ip=dhcp, restoring parity with the 4.21 provisioning ISO kernel command line.
Summary by CodeRabbit
New Features
Bug Fixes