-
Notifications
You must be signed in to change notification settings - Fork 27
Conversation
cc @cgwalters for early review until the dependencies are ready |
# The retval code is still respected with having this if-else block. | ||
ExecStart=/bin/sh -c \ | ||
'mount -o remount,rw /boot && rm /boot/ignition.firstboot && \ | ||
if [[ $(uname -m) = s390x ]]; then zipl; fi' |
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.
We could ship this and be totally fine but...the perfectionist in me is bothered by the potential case here where the system is interrupted between rm /boot/ignition.firstboot
and running zipl
- we'd be in a situation where Ignition would rerun every boot until an upgrade (or something else ran zipl
). (Actually wait, have we even patched libostree to run zipl
?)
Anyways so I think we shoudl remove the stamp file after running zipl, i.e.:
ExecStart=/bin/sh -c 'mount -o remount,rw /boot && if [[ $(uname -m) = s390x ]]; then zipl; fi && rm /boot/ignition.firstboot '
or so?
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.
(Actually wait, have we even patched libostree to run zipl?)
: The only attempt I made to patch libostree for zipl support is to generate the config file - similar to this https://github.com/ostreedev/ostree/blob/master/src/libostree/ostree-bootloader-syslinux.c for boot/syslinux/syslinux.cfg
- which I think we don't need. Because zipl understands BLS and we would also ship a minimal zipl.conf (soon to be merged and backported). For other means outside generating the config, I'm not sure if we need to patch libostree for zipl. Please some light on it (either here or on IRC).
And agree with you about moving the rm
after the if
block, didn't really consider that details. Good 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.
zipl understands BLS, but the command still needs to be rerun after updating the BLS files, right? (Unlike grub2)
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 if I'm not wrong, s390x/zipl doesn't care if /boot/ignition.firstboot
exists or not - at least for now when we don't have grub-emu yet (as described in#84). AFAIU, /boot/ignition.firstboot
only makes sense for grub.
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.
zipl understands BLS, but the command still needs to be rerun after updating the BLS files, right? (Unlike grub2)
: Ah right, now I remember we once talked about the hook/trigger mechanism to invoke zipl after ostree updates new commits.
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.
Couldn't be better, working on it, quite urgent tbh. Many thanks, Colin.
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.
Ok @cgwalters @tuan-hoang1 the zipl backend works well on rhcos with cosa master. The other question i had is - would this work for rhcos-4.2 with cosa-4.2 (basically using anaconda) and how do we deal with setting the backend to zipl like we do in create_disk.sh?
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.
@Prashanth684 I think of 2 ways:
- Add
ostree config set bootloader
to anaconda patch at https://bugzilla.redhat.com/show_bug.cgi?id=1741908 and generate newupdates.img
. - Execute
ostree config set bootloader
ingf-anaconda-cleanup
.
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.
But that's another issue. Let's not hijack this PR :P
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.
Execute ostree config set bootloader in gf-anaconda-cleanup
I like doing this fix in cosa - changing Anaconda is much harder. It might also work to do it in a %post --nochroot
in image-base.ks
.
84452d1
to
f85f821
Compare
cc @barthy1 |
Friendly ping @ashcrow. |
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.
Minor typo, LGTM otherwise!
@@ -18,7 +18,13 @@ RemainAfterExit=yes | |||
# detected this file. Fail if we are unable to remove it, rather than risking | |||
# rerunning Ignition at next boot. | |||
MountFlags=slave | |||
ExecStart=/bin/sh -c 'mount -o remount,rw /boot && rm /boot/ignition.firstboot' | |||
# It is better to have a separate script to do this but it might be polutting |
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.
polluting
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.
Checked, thanks.
zipl records need to be updated, because ignition.firstboot is burned into target disk during coreos-installer As a short-term solution for: coreos#84 Depends on: ibm-s390-linux/s390-tools#71 ibm-s390-linux/s390-tools#74 Related: coreos/coreos-installer#61 coreos/coreos-assembler#780
f85f821
to
9133253
Compare
zipl records need to be updated, because ignition.firstboot is burned
into target disk during coreos-installer
As a short-term solution for:
#84
Depends on:
ibm-s390-linux/s390-tools#71
ibm-s390-linux/s390-tools#74
Related:
coreos/coreos-installer#61
coreos/coreos-assembler#780