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

Add sysroot.bootloader repo config key (fix running bootloader when no deployments exist) #1814

Conversation

rfairley
Copy link
Member

@rfairley rfairley commented Feb 8, 2019

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
Copy link
Member

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 rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch from a4c4d69 to 234b07b Compare February 11, 2019 17:17
@rfairley
Copy link
Member Author

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 rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch from 234b07b to 307b5cc Compare February 11, 2019 20:08
@rfairley
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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 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 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
Copy link
Member Author

rfairley commented Feb 14, 2019

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

@rfairley 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
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just various things I noticed from a quick review!

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 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 pushed a commit to rfairley/ostree that referenced this pull request Feb 15, 2019
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
Copy link
Member Author

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
Copy link
Member Author

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).

@rfairley 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
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 rfairley force-pushed the rfairley-bootloader-use-pending-deployment-pr branch from c76fb65 to 2c3cd83 Compare March 1, 2019 20:29
@rfairley
Copy link
Member Author

rfairley commented Mar 1, 2019

@rfairley
Copy link
Member Author

rfairley commented Mar 1, 2019

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
Copy link
Member

jlebon commented Mar 1, 2019

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

@rh-atomic-bot
Copy link

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

rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
Closes: #1814
Approved by: jlebon
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
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
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
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
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
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
Copy link

💔 Test failed - status-atomicjenkins

@jlebon
Copy link
Member

jlebon commented Mar 1, 2019

@rh-atomic-bot retry

@rh-atomic-bot
Copy link

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

rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
Closes: #1814
Approved by: jlebon
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
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
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
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
rh-atomic-bot pushed a commit that referenced this pull request Mar 1, 2019
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
Copy link

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

@rfairley rfairley deleted the rfairley-bootloader-use-pending-deployment-pr branch March 1, 2019 21:51
@rfairley rfairley restored the rfairley-bootloader-use-pending-deployment-pr branch March 1, 2019 21:51
@dustymabe
Copy link
Contributor

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

@AdrianVovk
Copy link

👍 Great work!

@rfairley
Copy link
Member Author

rfairley commented Mar 4, 2019

Thanks :)

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

Successfully merging this pull request may close these issues.

6 participants