Support kargs.d directories for default kargs (rebased) #2217
Conversation
Instead of splitting a string of kargs with a space as the separator, parse the string and allow spaces that are protected by quotes in the karg value. The next_arg() function is based on the implementation in the Linux kernel: https://github.com/torvalds/linux/blob/8a05452ca460b05c985eadc7b5a4f040f124463e/lib/cmdline.c#L204
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: agners The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @agners. Thanks for your PR. I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/ok-to-test |
|
This is really neat. Currently we enable in-tree kargs by having a file KARGS="console=/dev/ttyS0,115200 ..."with a shell script to do the deploy like this: eval "$(ostree cat "$commit_sha" /usr/lib/ostree-boot/kargs.env || true)"
KARGS_ARG="--karg-none $(/usr/bin/printf "--karg-append=%s " $KARGS)"
ostree admin deploy ... $KARGS_ARG "$commit_sha"but an ostree built-in solution would be a lot cleaner. |
Add the following config directories for setting default kernel arguments: /etc/ostree/kargs.d (host config) /usr/lib/ostree-boot/kargs.d (base config) These directories contain files whose contents consist of a karg snippet, which is a collection kernel parameter keys and values. Example of a snippet: ``` KEY=FOO KEYFLAG ANOTHERKEY=VALUE1 ANOTHERKEY=VALUE2 ``` Snippets are read in alphanumeric order of the karg snippet filename when generating the final kernel options from the host and base config directories. Ordering may be specified by prefixing a number, e.g. `/etc/ostree/kargs.d/4000_localhost`. The bootconfig key `ostree-kargs-generated-from-config` indicates whether the kargs were generated by ostree from the kargs.d directories (`true`), or copied from the previous deployment (`false`). Editing the kargs through the command line (e.g. `ostree admin deploy --karg=`) will set the flag to `false`, and kargs will be copied from the previous deployment for subsequent deployments. Also add support for `ostree admin instutil set-kargs` so that installer programs using this command (such as Anaconda) remain managed by the kargs.d directories from the first deployment. Closes: ostreedev#479 [rebased to master] Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
Instead of setting kernel argument at deploy time use the kargs.d mechanism. Currently, this mechanism is still a downstream change of OSTree, but has been proposed upstream: ostreedev/ostree#2217 Signed-off-by: Stefan Agner <stefan.agner@toradex.com>
|
Removed a TODO and simplified Also successfully tested this changes in our OE/meta-updater based environment (with changes to meta-updater, see https://github.com/agners/meta-updater/tree/dunfell-kargs.d). |
|
One concern I have about this PR which I feel like we should flesh out a bit first is how updating from older OSTree will work. At least in FCOS/RHCOS, kargs are currently all untracked. I think we'll need some sort of "adoption" scheme where untracked kargs get translated to Edit: what this PR is doing IIUC is to make it " |
|
+1 to using an "adoption scheme" where the merge deployment's kargs are translated to drop-ins in |
|
I haven't done a deep dive on this yet but the more I think about this what I'd propose is:
IOW we're keeping the idea that the BLS file is a standard that admins and tools can edit directly and not inventing something ostree specific. |
|
We're having some live discussion on this, other topics:
I think combining these we are going to be sure we handle the cases of both removing default kargs as well as appending. |
| gboolean past_quote = FALSE; | ||
| gboolean past_equals = FALSE; |
There was a problem hiding this comment.
Were these variables here meant for handling errors if the input string (args) weren't formatted properly? Otherwise, if we can assume that args is formatted properly, these two variables seem unnecessary.
| bootconfig_set_kargs(bootconfig, new_options, FALSE); | ||
| ostree_bootconfig_parser_set (bootconfig, "options", new_options); |
There was a problem hiding this comment.
Looks like bootconfig_set_kargs() already handles the subsequent ostree_bootconfig_parser_set()'s job.
| @@ -2824,34 +2837,250 @@ get_var_dfd (OstreeSysroot *self, | |||
| return glnx_opendirat (base_dfd, base_path, TRUE, ret_fd, error); | |||
| } | |||
|
|
|||
| /* Get a GFile* referring to the commit object at ref for the file at path. */ | |||
There was a problem hiding this comment.
For my understanding, does this mean "Get a GFile* that points to the file at path in the content for ref"?
|
|
||
| if (dir_exists) | ||
| { | ||
| g_autoptr(GFile) karg_file = get_file_from_repo (ostree_sysroot_repo (sysroot), |
There was a problem hiding this comment.
Would it be clearer to name karg_file something like karg_dir to clearly distinguish it from the children files in usr/lib/ostree-boot/kargs.d that contain the kargs?
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
nit: looks like a lot of new lines here 😄
| if (karg_contents && !g_tree_lookup (kargs_configs, karg_name)) | ||
| g_tree_insert (kargs_configs, g_strdup (karg_name), | ||
| g_steal_pointer (&karg_contents)); |
There was a problem hiding this comment.
The way this currently works, if there is a host configured karg in /etc/ostree/kargs.d, and the vendor adds the same karg at some point in the future during an update but with a different name for the karg file, then there will be a duplicates in the BLS entry.
Additionally, it is possible that there are multiple host configured kargs all in the same karg file, and when the vendor adds a subset of these kargs at some point during an update, we would have duplicates.
To deal with these two potential problems, perhaps we can make it so that each karg file must only have one karg and the karg KEY must be in the name of the karg file. However, then we would need to have some extra logic to ignore the number preceding the karg file or add the ordering info to the karg file.
|
Re: #2217 (comment), if we don't support |
Yeah agree, and thanks for bringing this up. I think having configuration in one place (the BLS file) just fundamentally conflicts with supporting all of the general corner cases of the systemd style "split configuration" model. My initial thought here is we just say that overriding vendor kargs isn't supported. I suspect most people who want this don't even have per-system configuration at all - they are making custom ostree builds that they control and just want a convenient mechanism to add/remove kargs at build time without needing to wrap But if we do want to support this, we could have Or the other approach is to take this problem upstream to the BLS spec; something like And |
|
One angle to look at this from then is: Would we use vendor kargs in CoreOS? I'm not sure. I guess one use case I can think of is for RHCOS we added random.trust_cpu=on while we waited for a kernel fix to be backported. After that's done we can safely remove the karg - so that's a perfect use case for vendor kargs. And in this case, there is absolutely no sane reason someone would want to turn that argument off. |
|
Taking the problem upstream to BLS spec with something like If we do user karg configuration through But in both cases, a "factory reset" doesn't seem to be easily doable unless we can reliably distinguish between user per-system configuration, vendor configuration, and everything in between (e.g. Ignition-related or platform-specific kargs in FCOS). |
|
Could we use a third directory However, for existing nodes, the "adoption stage" might be a little tricky since it will be difficult for OSTree to distinguish between what is "default" and what is user configured in the existing BLS entry. |
|
Since this is still being discussed, let's I've been thinking about this, and I have an idea which I think would resolve some of the problems mentioned earlier. The core issue is that by looking at One way we can easily clear this up is by introducing a separator karg like
So then, it becomes safe to upgrade to such an OSTree version because all the preexisting kargs are essentially inherited as "unmanaged". The idea here is that the source of truth for the managed part of the BLS lives in Now, for CoreOS at least, it's likely that we wouldn't make the kargs in |
|
(Had this comment ready earlier, but I split it out of #2217 (comment) for clarity.)
I think the concept of a "factory reset" in practice is going to have to live at a higher-level than OSTree because there are different grades possible (some discussions about that in coreos/fedora-coreos-tracker#399). And as you mentioned, a karg which looks like a "user karg" to OSTree might actually be a "integration/distro karg" (like the Ignition platform ID one). And similarly for files, you may not actually want to nuke all the files in |
|
Doesn't the per-device kernel live in the ostree commit anyway? |
Yes...are you saying that e.g. if one is building a custom kernel one can already bake in command line arguments via e.g. ? |
|
Having Reference for MCO kargs code: https://github.com/openshift/machine-config-operator/blob/034afd05b6e04fd1c8df9cb7110461ea06763b0b/pkg/daemon/update.go#L805 |
We don't have any per-device kargs because we have a different ostree commit for each device type, so kargs stored under |
It feels to me that allowing kargs to write as file in /etc/ostree/kargs.d brings additional complexity for MCO. Users can directly drop a file here using MacineConfig and whenever next time ostree kargs gets called kargs will be updated. While in the current approach, MCO allows adding additional kargs only through specifying value in kernelArguments in a MachineConifg. |
Right; this would be a new way to write kernel arguments via the MCO (include them in Ignition), however I was more thinking about this as a new backend for the current MCO kernel arguments logic. IOW rather than the MCO running That said...to make this whole thread even more twisty, this intersects with openshift/machine-config-operator#1190 and its underlying issue #2220 - fixing that would really be a prerequisite for managing kargs via |
|
So right now, I see two main proposals for how we can tackle this PR: Option 1: Managed vs Unmanaged Kargs Advantages:
Disadvantages:
Option 2: Just support Advantages:
Disadvantages:
I think Option 1 should be safe to do and would be useful for FCOS. It does add some complexity, but for distros that don't need the extra functionality, they can easily ignore it. Should we put this to a vote on how to proceed? |
|
@cgwalters I meant that having per-device kernel args shouldn't stop you from reading from /usr as the canonical location because the per-device kernel is in /usr anyways |
yes, that is my understanding as well.
It is not difficult to handle both cases but creates multiple source of truth to look for all users added kargs. Debugging kargs related issues can get trickier compared to today as we would need to look for any added file into /etc/ostree/kargs.d/ |
|
According to me, both options currently have too many disadvantages:
I think we need to stick to a single source of truth. Either it's the BLS config or the configuration in the real root ( |
|
So if there's both /usr/lib/ostree/kargs.d, and /etc/ostree/kargs.d:
Alternatively, maybe this logic can be opt-in (or out if rpm-ostree can handle disabling this functionality on upgrade via the repo config) or maybe only enabled for newly-created repos/deployments? |
|
After some discussion with the CoreOS team, we've decided that for the non embedded use case (e.g. FCOS), attempting to create a new OSTree way of shipping vendor kargs while supporting host configured kargs simultaneously would create many problems. For this PR, I believe it should be sufficient to just support |
|
/unhold |
|
@kelvinfan001 thanks for the update and sorry for the late reply. I no longer work in the project. Some feedback still from that use case's perspective:
That might work out yes. The initial idea was to separate distro provided kargs (e.g. OS supplier) and project provided kargs (e.g. embedded product team). The way I was thinking to implement it is that the product team uses I don't plan to work on this personally. If this approach is fine, I think whoever is working on it should open a new pull request with such an implementation. |
|
This feature would be useful for us (for the embedded use-case). Would this change still be of interest to everyone? Of course given the suggestions/changes from @kelvinfan001, last comment. It should be sufficient for our use case to only support the vendor-provided I just wanted to check if any other points or discussions came up related to this before I attempt to submit a new PR. For example I see FCOS implemented their own kargs system, but I assume this is specific to the FCOS use-case. |
|
Hi yes from my PoV it's totally fine to re-open/re-consider this PR. This also came up again related to coreos/enhancements#7 since there is a natural desire to embed kargs in these container builds. |
I'm debugging https://bugzilla.redhat.com/show_bug.cgi?id=2075126 and while I haven't verified this is the case, as far as I can tell from looking through the code and thinking about things, if we somehow fail to apply the expected kernel arguments (which can occur if `ostree-finalize-staged` fails) then we will (on the next boot) drop in to `validateOnDiskState()` which has for a long time checked that all the expected *files* exist and mark the update as complete. But we didn't check the kernel arguments. That can then cause later problems because in trying to apply further updates we'll ask rpm-ostree to try to remove kernel arguments that aren't actually present. Worse, often these kernel arguments are actually *quite important* and may even have security relevant properties (e.g. `nosmt`). Now...I am actually increasingly convinced that we *really* need to move opinionated kernel argument handling into ostree (and rpm-ostree). There's ye olde ostreedev/ostree#2217 and the solution may look something like that. Particularly now with the layering philosophy that it makes sense to support e.g. customizations dropping content in `/usr/lib` and such. For now though, validating that we didn't get the expected kargs should make things go Degraded, the same as if there was a file conflict. And *that* in turn should make it easier to debug failures. As of right now, it will appear that updates are complete, and then we'll only find out much later that the kargs are actually missing. And in turn, because kubelet spams the journal, any error messages from e.g. `ostree-finalize-staged.service` may be lost.
I'm debugging https://bugzilla.redhat.com/show_bug.cgi?id=2075126 and while I haven't verified this is the case, as far as I can tell from looking through the code and thinking about things, if we somehow fail to apply the expected kernel arguments (which can occur if `ostree-finalize-staged` fails) then we will (on the next boot) drop in to `validateOnDiskState()` which has for a long time checked that all the expected *files* exist and mark the update as complete. But we didn't check the kernel arguments. That can then cause later problems because in trying to apply further updates we'll ask rpm-ostree to try to remove kernel arguments that aren't actually present. Worse, often these kernel arguments are actually *quite important* and may even have security relevant properties (e.g. `nosmt`). Now...I am actually increasingly convinced that we *really* need to move opinionated kernel argument handling into ostree (and rpm-ostree). There's ye olde ostreedev/ostree#2217 and the solution may look something like that. Particularly now with the layering philosophy that it makes sense to support e.g. customizations dropping content in `/usr/lib` and such. For now though, validating that we didn't get the expected kargs should make things go Degraded, the same as if there was a file conflict. And *that* in turn should make it easier to debug failures. As of right now, it will appear that updates are complete, and then we'll only find out much later that the kargs are actually missing. And in turn, because kubelet spams the journal, any error messages from e.g. `ostree-finalize-staged.service` may be lost.
|
I was thinking about this more recently. ISTM what we really want to do for default kernel arguments is actually: attach them to the kernel. One can already actually build in default kernel arguments via So...here's a strawman proposal: we only scope in image build time; we don't try to support client side stuff with e.g. The implementation is a simple text file like When tools like ostree do an upgrade from A to B they should effectively perform a 3-way merge, similar to our handling of
or so? This seems pretty straightforward to implement and support. |
|
@martinezjavier what do you think about this? |
This is a rebased version of #1836. The rebase mainly required trivial conflict fixes. There was one larger conflict with https://github.com/ostreedev/ostree/commits/52a6224606e4a057b806225d93a2d0239a004723 which required a bit more in depth changes to preserve/adhere to the idea of that change. All unit tests run by
make checkare passing.We would like a method to set and update kernel arguments via vanilla OSTree updates. Besides this approach we were also considering using OSTree metadata. But it seems this approach with files in-tree is more flexible. Feedback welcome!
Add the following config directories for setting default kernel
arguments:
/etc/ostree/kargs.d (host config)
/usr/lib/ostree-boot/kargs.d (base config)
These directories contain files whose contents consist of a karg
snippet, which is a collection kernel parameter keys and values.
Example of a snippet:
Snippets are read in alphanumeric order of the karg snippet filename
when generating the final kernel options from the host and base config
directories. Ordering may be specified by prefixing a number, e.g.
/etc/ostree/kargs.d/4000_localhost.The bootconfig key
ostree-kargs-generated-from-configindicateswhether the kargs were generated by ostree from the kargs.d directories
(
true), or copied from the previous deployment (false). Editing thekargs through the command line (e.g.
ostree admin deploy --karg=) willset the flag to
false, and kargs will be copied from the previousdeployment for subsequent deployments.
Also add support for
ostree admin instutil set-kargsso that installerprograms using this command (such as Anaconda) remain managed by the
kargs.d directories from the first deployment.
Closes: #479
[rebased to master]
Signed-off-by: Stefan Agner stefan.agner@toradex.com