Add sysroot.bootloader repo config key (fix running bootloader when no deployments exist) by rfairley · Pull Request #1814 · ostreedev/ostree

rfairley · 2019-02-08T22:58:07Z

The sysroot.bootloader key is used to configure the bootloader
that OSTree uses when deploying a sysroot. Having this key
allows specifying behavior not to use the default bootloader
backend code, which is preferable when creating a first
deployment from the sysroot (#1774).

As of now, the key can take the values "auto" or "none". If
the key is not given, the value defaults to "auto".

"auto" causes _ostree_sysroot_query_bootloader() to be used
when writing a new deployment, which is the original behavior
that dynamically detects which bootloader to use.

"none" avoids querying the bootloader dynamically. The BLS
config fragments are still written to
sysroot/boot/loader/entries for use by higher-level software.

More values can be supported in future to specify a single
bootloader, different behavior for the bootloader code, or
a list of bootloaders to try.

Resolves: #1774

cgwalters · 2019-02-11T16:33:12Z

Related: #1719

I need to dive into this more; not entirely sure it's right (but it doesn't seem necessarily wrong either). There's risk here in changing the grub backend.

I know it's more work but if we can get FCOS systems into having a static grub config (i.e. ostree only writes BLS snippets, use the grub BLS plugin) the whole architecture is much clearer.

rfairley · 2019-02-11T17:28:59Z

Thanks @cgwalters - just pushed a change. new_deployments seems to hold the pending deployment as the first entry - so I think this is the data that needs to be passed in order for grub2-mkconfig to use the pending deployment.

There's risk here in changing the grub backend.

Agreed on the risk - I think now this is a question of "could this change break existing systems that might have been relying on the existing deployment being used rather than the pending".

I'm interested in taking a look at using the BLS plugin in OSTree - I need to read into it more, but now that I've had a look at the bootloader code a bit better, I think I can see what would need to be changed.

rfairley · 2019-02-13T21:49:15Z

As part of this, going to start on an implementation for writing the BLS config, and a way to configure the bootloader (something like #1801 (comment)).

One way after brief discussion is to have OSTree always write BLS configs, and ship with a config bootloader=auto to use the auto-detecting bootloader functionality currently in place. This can be configured to bootloader=none to not use the auto-detecting functionality (only BLS configs will be written).

cgwalters · 2019-02-13T21:51:21Z

One way after brief discussion is to have OSTree always write BLS configs

That's what ostree has done since the very start. The BLS configs are canonical. But at the time RHEL7's grub didn't speak BLS (actually no bootloaders did really) so I added the concept of a "bootloader backend" to ostree as a temporary hack.

libostree never reads the grub config - it reads the BLS configs it writes as the source of truth.

rfairley · 2019-02-13T22:11:06Z

That's what ostree has done since the very start

Ahh that's clear to me now, thanks!

# ls /boot/loader/entries/ostree-
ostree-1-fedora-atomic.conf  ostree-2-fedora-atomic.conf

So what's needed is really having a switch in the bootloader backend to use or not use it (at least for the grub2-mkconfig part), and a way to configure that switch (through bootloader=)?

rfairley · 2019-02-14T21:47:45Z

Dropped the edits to the grub2 bootloader code, and started on adding a sysroot.bootloader key to the ostree repo config.

WIP, with the following left to do:

Errors are given due to the [sysroot] group not existing in the default repo/config files. Looking for a way to make that group optional.
Update the docs.
Add tests
- ostree admin tests with sysroot.bootloader=none
- test first deployment passes with sysroot.bootloader=none
- unit tests for any util functions that were added

Right now I'm testing with the following commands:

mkdir sysroot && ostree admin init-fs sysroot
ostree config set sysroot.bootloader none --repo=sysroot/ostree/repo
ostree pull-local --repo=sysroot/ostree/repo /sysroot/ostree/repo
ostree admin os-init osname --sysroot sysroot
ref=$(ostree refs --repo sysroot/ostree/repo)
ostree admin deploy "$ref" --sysroot sysroot --os osname -v 2>err.txt
cat err.txt | grep "Using bootloader" # This shows which bootloader configuration was set and which bootloader was actually used
ls sysroot/boot/loader/entries/ # Shows the BLS entries were created

On cat err.txt | grep "Using bootloader" :

# cat err.txt | grep "Using bootloader"
OT: Using bootloader configuration: none
OT: Using bootloader: (none)

Repeating the previous commands with ostree config set sysroot.bootloader auto --repo=sysroot/ostree/repo (or not adding this line at all in the config file to get default behavior), and the line mkdir -p sysroot/boot/grub2 && touch sysroot/boot/grub2/grub.cfg before deploying, the following is shown:

# cat err.txt | grep "Using bootloader"
OT: Using bootloader configuration: auto
OT: Using bootloader: OstreeBootloaderGrub2

jlebon

Just various things I noticed from a quick review!

src/libostree/ostree-repo.c

src/libostree/ostree-sysroot-deploy.c

Prep commit as part of ostreedev#1814, where a new `sysroot` group is added to the repo config. This allows falling back to a default key value if the specified group does not exist in the config file.

rfairley · 2019-02-15T23:42:09Z

I'm thinking bd082ad is a clean way to make the sysroot group optional by extending the existing API to give a default value if the group is not found.

So far, the core group uses this API, aside from a case where we don't handle the error: https://github.com/ostreedev/ostree/blob/master/src/libostree/ostree-repo.c#L2928.

A problem I see with this is not giving the user an error if for some reason [core] is removed from the config file. Config files where the key exists but the group [core] does not would still have default values applied (wherever ot_keyfile_get_value_with_default is used). This shouldn't be a problem if the config is done through ostree config set though, and I think the documentation showing [core] being needed in the file is sufficient.

rfairley · 2019-02-15T23:48:09Z

Hmm - seeing the tests for keyfile utils https://github.com/ostreedev/ostree/blob/master/tests/test-keyfile-utils.c#L126, probably best to keep the API as it is and instead only make the [sysroot] section optional (by checking if the group exists in reload_sysroot_config and just returning success if it doesn't exist).

Rename ot_keyfile_get_string_as_list() to ot_keyfile_get_string_list_with_separator_choice() which expresses more clearly why the function is needed. Also shorten the function comment.

Add ot_keyfile_get_value_with_default_group_optional() which allows getting values from keys where the group is optional in the config file. This is preparatory to add the sysroot.bootloader repo config key, where the sysroot group is optional.

The sysroot.bootloader key configures the bootloader that OSTree uses when deploying a sysroot. Having this key allows specifying behavior not to use the default bootloader backend code, which is preferable when creating a first deployment from the sysroot (ostreedev#1774). As of now, the key can take the values "auto" or "none". If the key is not given, the value defaults to "auto". "auto" causes _ostree_sysroot_query_bootloader() to be used when writing a new deployment, which is the original behavior that dynamically detects which bootloader to use. "none" avoids querying the bootloader dynamically. The BLS config fragments are still written to sysroot/boot/loader/entries for use by higher-level software. More values can be supported in future to specify a single bootloader, different behavior for the bootloader code, or a list of bootloaders to try. Resolves: ostreedev#1774

rfairley · 2019-03-01T20:34:11Z

Updated, and fixed the commit messages!
Diff from last review: https://github.com/ostreedev/ostree/compare/1ee554d05a506d18eccbea0d69bf356be15fba9d..2c3cd83a913d395b500b4528f78073838a2d43ab

rfairley · 2019-03-01T20:37:27Z

Also split 5b5093f into its own commit (previously added tests for sysroot.bootloader there and did this edit in passing, but later removed the tests).

jlebon · 2019-03-01T20:48:05Z

Awesome, LGTM!
@rh-atomic-bot r+ 2c3cd83

rh-atomic-bot · 2019-03-01T20:48:11Z

⌛ Testing commit 2c3cd83 with merge 790a50e...

Closes: #1814 Approved by: jlebon

Rename ot_keyfile_get_string_as_list() to ot_keyfile_get_string_list_with_separator_choice() which expresses more clearly why the function is needed. Also shorten the function comment. Closes: #1814 Approved by: jlebon

Add ot_keyfile_get_value_with_default_group_optional() which allows getting values from keys where the group is optional in the config file. This is preparatory to add the sysroot.bootloader repo config key, where the sysroot group is optional. Closes: #1814 Approved by: jlebon

The sysroot.bootloader key configures the bootloader that OSTree uses when deploying a sysroot. Having this key allows specifying behavior not to use the default bootloader backend code, which is preferable when creating a first deployment from the sysroot (#1774). As of now, the key can take the values "auto" or "none". If the key is not given, the value defaults to "auto". "auto" causes _ostree_sysroot_query_bootloader() to be used when writing a new deployment, which is the original behavior that dynamically detects which bootloader to use. "none" avoids querying the bootloader dynamically. The BLS config fragments are still written to sysroot/boot/loader/entries for use by higher-level software. More values can be supported in future to specify a single bootloader, different behavior for the bootloader code, or a list of bootloaders to try. Resolves: #1774 Closes: #1814 Approved by: jlebon

rh-atomic-bot · 2019-03-01T21:14:33Z

💔 Test failed - status-atomicjenkins

jlebon · 2019-03-01T21:20:32Z

@rh-atomic-bot retry

rh-atomic-bot · 2019-03-01T21:20:38Z

⌛ Testing commit 2c3cd83 with merge 21ebc7d...

Closes: #1814 Approved by: jlebon

Rename ot_keyfile_get_string_as_list() to ot_keyfile_get_string_list_with_separator_choice() which expresses more clearly why the function is needed. Also shorten the function comment. Closes: #1814 Approved by: jlebon

Add ot_keyfile_get_value_with_default_group_optional() which allows getting values from keys where the group is optional in the config file. This is preparatory to add the sysroot.bootloader repo config key, where the sysroot group is optional. Closes: #1814 Approved by: jlebon

The sysroot.bootloader key configures the bootloader that OSTree uses when deploying a sysroot. Having this key allows specifying behavior not to use the default bootloader backend code, which is preferable when creating a first deployment from the sysroot (#1774). As of now, the key can take the values "auto" or "none". If the key is not given, the value defaults to "auto". "auto" causes _ostree_sysroot_query_bootloader() to be used when writing a new deployment, which is the original behavior that dynamically detects which bootloader to use. "none" avoids querying the bootloader dynamically. The BLS config fragments are still written to sysroot/boot/loader/entries for use by higher-level software. More values can be supported in future to specify a single bootloader, different behavior for the bootloader code, or a list of bootloaders to try. Resolves: #1774 Closes: #1814 Approved by: jlebon

rh-atomic-bot · 2019-03-01T21:47:57Z

☀️ Test successful - status-atomicjenkins
Approved by: jlebon
Pushing 21ebc7d to master...

dustymabe · 2019-03-01T22:21:39Z

❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️ ❤️

AdrianVovk · 2019-03-02T03:22:48Z

👍 Great work!

rfairley · 2019-03-04T13:42:56Z

Thanks :)

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch from a4c4d69 to 234b07b Compare February 11, 2019 17:17

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch from 234b07b to 307b5cc Compare February 11, 2019 20:08

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch 2 times, most recently from e5bb2c0 to dd38a31 Compare February 14, 2019 21:36

rfairley changed the title ~~[WIP] grub2: generate config from pending deployment when no deployments exist~~ [WIP] Add sysroot.bootloader repo config key (fix running bootloader when no deployments exist) Feb 14, 2019

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch 5 times, most recently from a71ade1 to e068e86 Compare February 14, 2019 22:13

jlebon reviewed Feb 15, 2019

View reviewed changes

src/libostree/ostree-repo.c Outdated Show resolved Hide resolved

src/libostree/ostree-repo.c Show resolved Hide resolved

src/libostree/ostree-repo.c Outdated Show resolved Hide resolved

src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved

rfairley mentioned this pull request Feb 15, 2019

WIP: remove anaconda coreos/coreos-assembler#311

Closed

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch 2 times, most recently from 98b2788 to ee14d6d Compare February 15, 2019 23:17

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch 7 times, most recently from 288fb49 to d6ab97c Compare February 21, 2019 19:55

Robert Fairley and others added 3 commits March 1, 2019 15:29

libotutil: Rename to ot_keyfile_get_string_list_with_separator_choice()

c6980b8

Rename ot_keyfile_get_string_as_list() to ot_keyfile_get_string_list_with_separator_choice() which expresses more clearly why the function is needed. Also shorten the function comment.

rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch from c76fb65 to 2c3cd83 Compare March 1, 2019 20:29

rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019

ostree-grub-generator: Comment wording fix

b94e51b

Closes: #1814 Approved by: jlebon

rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019

tests/test-config: Remove extra space

9b1930d

Closes: #1814 Approved by: jlebon

jlebon added the homu/approved label Mar 1, 2019

rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019

tests/test-config: Remove extra space

52987b5

Closes: #1814 Approved by: jlebon

rh-atomic-bot closed this in b0af96c Mar 1, 2019

rfairley deleted the rfairley-bootloader-use-pending-deployment-pr branch March 1, 2019 21:51

rfairley restored the rfairley-bootloader-use-pending-deployment-pr branch March 1, 2019 21:51

rfairley mentioned this pull request Mar 14, 2019

ostree admin deploy fails if there are no existing deployments when a bootloader is detected #1774

Closed

Comments

Conversation

rfairley commented Feb 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Feb 11, 2019

Uh oh!

rfairley commented Feb 11, 2019

Uh oh!

rfairley commented Feb 13, 2019

Uh oh!

cgwalters commented Feb 13, 2019

Uh oh!

rfairley commented Feb 13, 2019

Uh oh!

rfairley commented Feb 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rfairley commented Feb 15, 2019

Uh oh!

rfairley commented Feb 15, 2019

Uh oh!

rfairley commented Mar 1, 2019

Uh oh!

rfairley commented Mar 1, 2019

Uh oh!

jlebon commented Mar 1, 2019

Uh oh!

rh-atomic-bot commented Mar 1, 2019

Uh oh!

rh-atomic-bot commented Mar 1, 2019

Uh oh!

jlebon commented Mar 1, 2019

Uh oh!

rh-atomic-bot commented Mar 1, 2019

Uh oh!

rh-atomic-bot commented Mar 1, 2019

Uh oh!

dustymabe commented Mar 1, 2019

Uh oh!

AdrianVovk commented Mar 2, 2019

Uh oh!

rfairley commented Mar 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rfairley commented Feb 8, 2019 •

edited

Loading

rfairley commented Feb 14, 2019 •

edited

Loading