Skip to content
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

bootloader: Add a zipl bootloader backend #1937

Merged
merged 1 commit into from
Oct 16, 2019

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Oct 14, 2019

zipl is a bit special in that it parses the BLS config files
directly but we need to run the command to update the "boot block".

Hence, we're not generating a separate config file like the other
backends. Instead, extend the bootloader interface with a post_bls_sync
method that is run in the same place we swap the boot/loader symlink.

We write a "stamp file" in /boot that says we need to run this command.
The reason we use stamp file is to prevent the case where the system is
interrupted after BLS file is updated, but before zipl is triggered,
then zipl boot records are not updated.
This opens the door to making things eventually-consistent/reconcilable
by later adding a systemd unit to run zipl if we're interrupted via
a systemd unit - I think we should eventually take this approach
everywhere rather than requiring /boot/loader to be a symlink.

Author: Colin Walters walters@verbum.org
Tested-by: Tuan Hoang tmhoang@linux.ibm.com
Co-Authored-By: Tuan Hoang tmhoang@linux.ibm.com

@cgwalters
Copy link
Member Author

(Only compile tested!)

@Prashanth684
Copy link

(Only compile tested!)

i'd like to test these changes locally as part of creating an fcos/rhcos image on s390x. any idea how i can do that ?

@cgwalters
Copy link
Member Author

i'd like to test these changes locally as part of creating an fcos/rhcos image on s390x. any idea how i can do that ?

See https://github.com/coreos/coreos-assembler/blob/master/README-devel.md#using-overrides

@vtolstov
Copy link

can you share link to this kind of bootloader - zipl ?

@cgwalters
Copy link
Member Author

can you share link to this kind of bootloader - zipl ?

See https://github.com/ibm-s390-tools/s390-tools/ for https://en.wikipedia.org/wiki/Linux_on_z_Systems

@tuan-hoang1
Copy link
Collaborator

tuan-hoang1 commented Oct 15, 2019

Given we make *out_is_active = FALSE; and specifically set sysroot.bootloader zipl in create_disk.sh, I think we still need this, right ?

diff --git a/src/libostree/ostree-repo.c b/src/libostree/ostree-repo.c
index b654aff2..94116290 100644
--- a/src/libostree/ostree-repo.c
+++ b/src/libostree/ostree-repo.c
@@ -3185,7 +3185,7 @@ reload_sysroot_config (OstreeRepo          *self,
      * https://github.com/ostreedev/ostree/issues/1719
      * https://github.com/ostreedev/ostree/issues/1801
      */
-    if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none")))
+    if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none") || g_str_equal (bootloader, "zipl")))
       return glnx_throw (error, "Invalid bootloader configuration: '%s'", bootloader);
 
     g_free (self->bootloader);

I thought "auto" should cover when we specifically set bootloader, but seems not likely as rpm-ostreed reported
Oct 15 16:40:28 coreos rpm-ostree[1360]: error: Couldn't start daemon: Error setting up sysroot: Invalid bootloader configuration: 'zipl'

@cgwalters
Copy link
Member Author

if (!(g_str_equal (bootloader, "auto") || g_str_equal (bootloader, "none") || g_str_equal (bootloader, "zipl")))

Oh right. Thanks, I merged that bit and added the other bootloaders too for completeness. This bit is ugly...need to rewrite the bootloader stuff, but let's do the easy change for now.

@tuan-hoang1
Copy link
Collaborator

To summarize:
Colin's 542f550 commit : add a "zipl" check in libostree/ostree-repo.c
Colin's 5646da4 commit : add _ostree_bootloader_zipl_new() to create bootloader object for zipl case.
My last 2 commits : add a comment to make it clearer for why we do _ostree_bootloader_zipl_new() instead inside libostree/ostree-sysroot-deploy.c. Also update git commit message with some more details.

Colin @cgwalters, thanks a lot for quick turnaround ! This works perfectly well. I think we are good to go now.

@tuan-hoang1 tuan-hoang1 changed the title WIP: Add a zipl bootloader backend bootloader: Add a zipl bootloader backend Oct 16, 2019
@tuan-hoang1
Copy link
Collaborator

Looks like there was some permission issue in ostree source tree ? I pulled your remote tree and make a branch on my local repo, maybe that's an issue ? Let me clone your tree then ...

@cgwalters
Copy link
Member Author

Repushed to retrigger CI.

src/libostree/ostree-repo.c Outdated Show resolved Hide resolved
zipl is a bit special in that it parses the BLS config files
directly *but* we need to run the command to update the "boot block".

Hence, we're not generating a separate config file like the other
backends.  Instead, extend the bootloader interface with a `post_bls_sync`
method that is run in the same place we swap the `boot/loader` symlink.

We write a "stamp file" in `/boot` that says we need to run this command.
The reason we use stamp file is to prevent the case where the system is
interrupted after BLS file is updated, but before zipl is triggered,
then zipl boot records are not updated.
This opens the door to making things eventually-consistent/reconcilable
by later adding a systemd unit to run `zipl` if we're interrupted via
a systemd unit - I think we should eventually take this approach
everywhere rather than requiring `/boot/loader` to be a symlink.

Author: Colin Walters <walters@verbum.org>
Tested-by: Tuan Hoang <tmhoang@linux.ibm.com>
Co-Authored-By: Tuan Hoang <tmhoang@linux.ibm.com>
@jlebon
Copy link
Member

jlebon commented Oct 16, 2019

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants