-
Notifications
You must be signed in to change notification settings - Fork 93
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
s390x: call zipl using exact kernel, ramdisk and cmdline #1422
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.
this looks good to me, thanks for digging on this @nikita-dubrovskii
@prestist can you give this another look too?
docs/release-notes.md
Outdated
@@ -14,6 +14,7 @@ Minor changes: | |||
|
|||
Internal changes: | |||
|
|||
- zipl: specify exact kernel, ramdisk and cmdline instead of copying & modifying of bls config |
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.
Not sure if release notes is where we should put this info, but I'd be interested to note if we should mention #1422 here?
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.
As i remember without updating notes
CI == failure
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.
No yes. We do need a release notes entry.
My question is whether or not we should include a link in the release notes entry to coreos/fedora-coreos-tracker#1667
(sorry the link I pasted in #1422 (comment) was wrong. I should have linked coreos/fedora-coreos-tracker#1667)
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 don't think so. Commit message contains the link, but let's wait what @jlebon says about that
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.
ehh. doesn't need to wait. we can just ship it if everyone else is happy with it
let kernel = boot.join(&kernel[1..]); | ||
let initrd = boot.join(&initrd[1..]); |
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.
and because we have the boot -> .
symlink it doesn't matter if the ultimate path ends up being /boot/boot/ostree/foo/vmlinuz...
?
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.
yes, exactly, symlink does the trick
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.
Some minor details, but looks sane overall!
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.
Looks good to me, nothing major that I see needs to change. Agree with most of the nits.
There is no need to copy `boot/loader/entries` folder and to create temporary `zipl.conf` , instead we could parse bls config and tell `zipl` which image and disk to use. This also fixes an issue when images comes with `bootprefix`. Issue: coreos/fedora-coreos-tracker#1667
860d8b1
to
8f132f4
Compare
This is a second round of f5677a3 (reverted in db7bf08). Now that coreos-installer in rawhide has coreos/coreos-installer#1422 we can set sysroot.bootprefix to true again. We can't do it in the non-OSBuild configs because not all streams have the new coreos-installer yet. Here is the comment from the original commit: This setting will make it so that BLS config entries get prepended with /boot. OSTree already places a boot -> . symlink in the root of the boot filesystem prepending with /boot will always just work. For context see osbuild/osbuild#1566 (comment) This also allows for dropping one of the upstream OSBuild zipl stage patches.
This is a second round of f5677a3 (reverted in db7bf08). Now that coreos-installer in rawhide has coreos/coreos-installer#1422 we can set sysroot.bootprefix to true again. We can't do it in the non-OSBuild configs because not all streams have the new coreos-installer yet. Here is the comment from the original commit: This setting will make it so that BLS config entries get prepended with /boot. OSTree already places a boot -> . symlink in the root of the boot filesystem prepending with /boot will always just work. For context see osbuild/osbuild#1566 (comment) This also allows for dropping one of the upstream OSBuild zipl stage patches.
There is no need to copy
boot/loader/entries
folder and to create temporaryzipl.conf
, instead we could parse bls config and tellzipl
which image and disk to use. This also fixes an issue when images comes withbootprefix
.Issue: coreos/fedora-coreos-tracker#1667