Add `rpm-ostree ex rebuild` command #3340

jlebon · 2022-01-18T22:41:00Z

This command reads treefile dropins in /etc and applies it to the
system. Only the OSTree container flow is supported for now. For the
client-side flow, see #2326.

In the container flow, only packages is currently supported. But this
paves the way for supporting more derivation fields as well as making
currently "base" fields only become valid derivation fields too (by
promoting it from the BaseComposeConfigFields struct to
TreeComposeConfig).

What we're doing here is providing a "container" backend for derivation
treefiles, but #2326 will provide a "client" backend which uses the core
more fully. But the inputs themselves are the exact same, hence
providing symmetry between the two flows. (See also discussions about
this in coreos/fedora-coreos-tracker#1054).

This will also allow us to drop the microdnf dependency which will be
done in a following patch.

openshift-ci · 2022-01-18T22:41:01Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

jlebon · 2022-01-18T22:46:31Z

Sample output:

bash-5.1# rpm -q ltrace
package ltrace is not installed

bash-5.1# cat tmp.yaml
packages:
  - ltrace

bash-5.1# rpm-ostree compose container tmp.yaml
Enabled rpm-md repositories: fedora-cisco-openh264 fedora-modular updates-modular updates fedora updates-archive
Importing rpm-md... done
rpm-md repo 'fedora-cisco-openh264' (cached); generated: 2021-09-21T18:07:30Z solvables: 4
rpm-md repo 'fedora-modular' (cached); generated: 2021-10-26T05:08:36Z solvables: 1283
rpm-md repo 'updates-modular' (cached); generated: 2022-01-06T01:57:22Z solvables: 1371
rpm-md repo 'updates' (cached); generated: 2022-01-18T01:34:10Z solvables: 17188
rpm-md repo 'fedora' (cached); generated: 2021-10-26T05:31:27Z solvables: 65732
rpm-md repo 'updates-archive' (cached); generated: 2022-01-18T02:08:37Z solvables: 20988
Resolving dependencies... done
Will download: 1 package (140.0?kB)
Downloading from 'fedora'... done
Installing: ltrace-0.7.91-42.fc35.x86_64 (fedora)

bash-5.1# rpm -q ltrace
ltrace-0.7.91-42.fc35.x86_64

cgwalters · 2022-01-19T15:48:43Z

I find calling this compose container a bit confusing given #3238 which was actually about doing this outside a container.

Maybe...rpm-ostree configure /tmp.yaml? Per #2326 again there's no strong reason we can't support this as well "per machine" right? So supporting a generic configure verb would then lead naturally to having the CLI drop the yaml file in /etc/rpm-ostree/config.d/<hash of yaml>.yml and just invoking rpm-ostree rebuild or so.

cgwalters

Prep patches seem sane, let's split them to a separate PR and get them in.

cgwalters · 2022-01-19T15:41:13Z

src/libpriv/rpmostree-core-private.h

  gboolean is_system;
+  /* Whether we were created with new_container() */
+  gboolean is_container;


OK as is but I think we should switch to an enum at some point.

cgwalters · 2022-01-19T15:42:27Z

src/libpriv/rpmostree-core.cxx

@@ -154,10 +154,8 @@ set_rpm_macro_define (const char *key, const char *value)
 RpmOstreeContext *
 rpmostree_context_new_base (OstreeRepo   *repo)
 {
-  g_assert (repo != NULL);


This part is going to be...interesting. Heavy intersection with ostreedev/ostree-rs-ext#159

cgwalters · 2022-01-19T15:45:30Z

src/app/rpmostree-compose-builtin-tree.cxx

@@ -1661,3 +1661,93 @@ rpmostree_compose_builtin_extensions (int             argc,

  return TRUE;
 }
+
+static void
+state_action_changed_cb (DnfState       *state,


Hmm...I think we could actually just do this by default in rpmostree-core.cxx right? I believe we bypass this section of libdnf in the ostree-native case. That way it'd work out of the box in other code paths too.

jlebon · 2022-01-19T16:02:43Z

Prep patches split out in #3341.

cgwalters · 2022-01-19T16:41:11Z

Did you see #3330 btw? That was a much hackier version of this that avoids rpmostree-core.cxx. Clearly once this lands I think it does still make sense to use the same backend for the imperative path i.e. rpm-ostree install too.

cgwalters · 2022-01-20T14:45:47Z

I find calling this compose container a bit confusing given #3238 which was actually about doing this outside a container.

I was thinking about this more, and I realized even though we've been working on the same project for years now, we may view things differently here.

Today, rpm-ostree compose has two qualities: It's declarative, and it's about building a new thing, distinct from what's running; i.e. distinct from the "host" (or container).

To me, compose was always much more about the latter i.e. "cross builds" or "new base images" we might say. Having it be declarative was just an obviously necessary thing in order to have a sane build system.

But I realized perhaps you've seen it more this whole time as being the declarative frontend, and today it just happens to only support cross builds. Is that the case?

jlebon · 2022-01-20T15:38:16Z

I've tried a couple of different places for this command, but wasn't comfortable defining the UX for #2326 in the process, because I think there's still some fleshing out to do there. And the semantics are not exactly the same as what we're doing here.

The reason I ended up under rpm-ostree compose for this was because that command is purely associated with the server-side: the CoreOS layering work makes the distinction a bit fuzzy, but it's still more of a server-side operation, even if the server in this case is a container image build system that you control.

Of course, once we do have #2326 I think it makes sense to revisit and possibly unify the UX (or decide to leave it a separate command that just uses the same backend).

Anyway, I'm OK also adding an experimental rebuild command though it feels a bit early and will have to awkwardly error out in the !container case for now. So personally, leaning more towards keeping this somewhere under compose though open to a different name given #3238 (e.g. rpm-ostree compose livecontainer?).

Currently working on factoring out the container bits to share with install so we can drop the microdnf dep.

cgwalters · 2022-01-20T15:58:53Z

The reason I ended up under rpm-ostree compose for this was because that command is purely associated with the server-side: the CoreOS layering work makes the distinction a bit fuzzy, but it's still more of a server-side operation,

But see above - I was talking about compose being about cross builds, which also happens to be how one wants to do it on the server side.

It's worth noting here of course that quite commonly today rpm-ostree compose tree itself normally runs inside a container (or in the case of coreos-assembler in production as you know, a supermin VM which is more like a kata container).

To state this a different way, the fact that we're mutating the current root is what I'm keying in on.

Now, I don't want to block this! But I think this discussion isn't just bikeshedding - it's important to have a shared consensus and understanding of what these different CLI verbs mean, because it has a large impact on the system architecture.

jlebon · 2022-01-20T17:24:29Z

To state this a different way, the fact that we're mutating the current root is what I'm keying in on.

Right yup. I do agree that fact is more pertinent than the server/client distinction. I wouldn't read too much into the compose part. It's more about getting it out of where client-side users go looking.

Anyway, I've moved this to rpm-ostree ex rebuild now and made the command hidden. This turns into an opportunity to test the UX, although just as part of the container flow.

OK, got carried away a bit and also added support for reading dropins and completely ripping out microdnf. Can split some of this into separate PRs if preferred.

jlebon · 2022-01-20T17:25:39Z

Split out the first four into #3350.

cgwalters

Overall looks sane, a few comments.

cgwalters · 2022-01-20T19:16:00Z

Cargo.toml

@@ -78,6 +78,7 @@ tracing-subscriber = { version = "0.3", features = ["env-filter"] }
 tokio = { version = "1.15.0", features = ["full"] }
 xmlrpc = "0.15.1"
 termcolor = "1.1.2"
+glob = "0.3.0"


OK, glancing at the reverse dependencies this is at least a somewhat widely used crate. Notably, cargo uses it.

That said it was also last updated 3 years ago.

Yeah, coreos-installer uses it too, which is where I learned it from.

cgwalters · 2022-01-20T19:16:55Z

rust/src/treefile.rs

@@ -46,6 +47,9 @@ const INCLUDE_MAXDEPTH: u32 = 50;
 /// it's intended to be informative.
 const COMPOSE_JSON_PATH: &str = "usr/share/rpm-ostree/treefile.json";

+/// Globby path to client-side treefiles.
+const CLIENT_TREEFILES_GLOB: &str = "/etc/rpm-ostree/origin.d/*.yaml";


And in this case, it's not really clear to me that we're gaining very much with this crate glob versus just looping over the directory contents ourselves with if !name.ends_with(".yaml") { continue; }.

I initially wanted to use the ** bit, but agreed it's a bit overkill as is.

cgwalters · 2022-01-20T19:17:18Z

rust/src/treefile.rs

@@ -2260,3 +2272,20 @@ pub(crate) fn treefile_new_client(filename: &str, basearch: &str) -> CxxResult<B
    r.error_if_base()?;
    Ok(r)
 }
+
+/// Create a new client treefile using the treefile dropins in /etc/rpm-ostree/origin.d/.


cgwalters · 2022-01-20T21:01:26Z

src/libpriv/rpmostree-container.cxx

+{
+  // XXX: awkward: add a e.g. `treefile_new_client_from_fields()`
+  g_autofree char *joined = g_strjoinv ("\",\"", packages);
+  g_autofree char *treefile_s = g_strdup_printf ("{\"packages\": [\"%s\"]}", joined);


Eeek. I think today we can do treefile_new_empty() and then let's add a set_packages() API?

Let's keep that in a follow-up? I'd like to consider something more generic than per-field methods.

Add support for creating a client treefile based on the dropins in `/etc/rpm-ostree/origin.d`. This will currently be used only by the container flow, but could eventually also end up being used client-side as discussed in coreos#2326.

This command reads treefile dropins in `/etc` and applies it to the system. Only the OSTree container flow is supported for now. For the client-side flow, see coreos#2326. In the container flow, only `packages` is currently supported. But this paves the way for supporting more derivation fields as well as making currently "base" fields only become valid derivation fields too (by promoting it from the `BaseComposeConfigFields` struct to `TreeComposeConfig`). What we're doing here is providing a "container" backend for derivation treefiles, but coreos#2326 will provide a "client" backend which uses the core more fully. But the inputs themselves are the exact same, hence providing symmetry between the two flows. (See also discussions about this in coreos/fedora-coreos-tracker#1054). This will also allow us to drop the `microdnf` dependency which will be done in a following patch.

We know how to natively do package installs in containers now, so use that instead of relying on microdnf.

jlebon · 2022-01-21T17:59:24Z

OK, added a test for this now. Currently fighting issues with cosa locally so I didn't actually verify that it passes, but we'll see what CI says.

The only thing we use it for is clearing the cache. But now that we use the core, which uses a different cache directory, that command doesn't delete the right thing. And at that point, it's just simpler to directly delete it ourselves in the container flow. Though I think it could also make sense to add a `--no-cache` switch directly to `rebuild` so that container builders can just apply the config and clear the cache in one shot.

cgwalters

Awesome work here!

cgwalters · 2022-01-22T16:08:53Z

rust/src/treefile.rs

+    let mut tfs = iter_etc_treefiles()?.collect::<Result<Vec<PathBuf>>>()?;
+    tfs.sort(); // sort because order matters; later treefiles override earlier ones


Oh right, that's a trap. Hmm. Also an argument for having the API for loading config files return a Vec<> directly. But that can be a followup.

Or...I dunno actually, the more I think about this the more I think we shouldn't allow overrides at all? Allowing it just seems like it's most likely to be a footgun for users.

At least this isn't a problem yet right because we're only supporting packages which are always unionioned?

openshift-ci bot added the do-not-merge/work-in-progress label Jan 18, 2022

jlebon force-pushed the pr/treefile-derivation branch from ff113ca to 0ee1377 Compare January 18, 2022 22:44

jlebon force-pushed the pr/treefile-derivation branch from 0ee1377 to 04b5b1c Compare January 19, 2022 13:08

cgwalters reviewed Jan 19, 2022

View reviewed changes

jlebon mentioned this pull request Jan 19, 2022

Prep patches for #3340 #3341

Merged

jlebon force-pushed the pr/treefile-derivation branch from 04b5b1c to 688efa0 Compare January 20, 2022 17:24

cgwalters reviewed Jan 20, 2022

View reviewed changes

cgwalters mentioned this pull request Jan 20, 2022

WIP: Directly do container installs #3330

Closed

openshift-ci bot added the needs-rebase label Jan 21, 2022

jlebon force-pushed the pr/treefile-derivation branch from 688efa0 to 0ae9972 Compare January 21, 2022 02:27

openshift-ci bot removed the needs-rebase label Jan 21, 2022

jlebon changed the title ~~WIP: Add rpm-ostree compose container command~~ Add rpm-ostree ex rebuild command Jan 21, 2022

jmarrero mentioned this pull request Jan 21, 2022

config/fcos: Add extensions param and generate treefile with packages coreos/butane#304

Merged

jlebon added 2 commits January 21, 2022 12:58

app/pkg-builtins: Use container backend for installs

194d1a3

We know how to natively do package installs in containers now, so use that instead of relying on microdnf.

jlebon force-pushed the pr/treefile-derivation branch from 0ae9972 to 0e1a2ad Compare January 21, 2022 17:58

jlebon marked this pull request as ready for review January 21, 2022 17:58

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 21, 2022

jlebon force-pushed the pr/treefile-derivation branch from 0e1a2ad to 8202134 Compare January 21, 2022 18:22

jlebon mentioned this pull request Jan 21, 2022

rust: Fix cargo clippy warnings #3351

Merged

cgwalters mentioned this pull request Jan 21, 2022

Links with long paths cause an error when rebasing on a layered oci image. #3310

Closed

cgwalters approved these changes Jan 22, 2022

View reviewed changes

cgwalters merged commit 18a436e into coreos:main Jan 22, 2022

cgwalters mentioned this pull request Jan 27, 2022

Support kernel overrides in ex rebuild container flow #3364

Closed

jlebon mentioned this pull request Jan 31, 2022

Implement client side/boot time layering of extensions. coreos/butane#322

Closed

cgwalters mentioned this pull request Feb 4, 2022

Support declarative rebases #3403

Open

jlebon mentioned this pull request Mar 3, 2022

Support non-kernel overrides in ex rebuild container flow #3489

Closed

jlebon deleted the pr/treefile-derivation branch April 23, 2023 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `rpm-ostree ex rebuild` command #3340

Add `rpm-ostree ex rebuild` command #3340

jlebon commented Jan 18, 2022 •

edited

Loading

openshift-ci bot commented Jan 18, 2022

jlebon commented Jan 18, 2022

cgwalters commented Jan 19, 2022

cgwalters left a comment

cgwalters Jan 19, 2022

cgwalters Jan 19, 2022

cgwalters Jan 19, 2022

jlebon commented Jan 19, 2022

cgwalters commented Jan 19, 2022

cgwalters commented Jan 20, 2022

jlebon commented Jan 20, 2022

cgwalters commented Jan 20, 2022

jlebon commented Jan 20, 2022

jlebon commented Jan 20, 2022

cgwalters left a comment

cgwalters Jan 20, 2022

jlebon Jan 20, 2022

cgwalters Jan 20, 2022

jlebon Jan 20, 2022

cgwalters Jan 20, 2022

cgwalters Jan 20, 2022

jlebon Jan 20, 2022

jlebon commented Jan 21, 2022

cgwalters left a comment

cgwalters Jan 22, 2022

		let mut tfs = iter_etc_treefiles()?.collect::<Result<Vec<PathBuf>>>()?;
		tfs.sort(); // sort because order matters; later treefiles override earlier ones

Add rpm-ostree ex rebuild command #3340

Add rpm-ostree ex rebuild command #3340

Conversation

jlebon commented Jan 18, 2022 • edited Loading

openshift-ci bot commented Jan 18, 2022

jlebon commented Jan 18, 2022

cgwalters commented Jan 19, 2022

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Jan 19, 2022

cgwalters commented Jan 19, 2022

cgwalters commented Jan 20, 2022

jlebon commented Jan 20, 2022

cgwalters commented Jan 20, 2022

jlebon commented Jan 20, 2022

jlebon commented Jan 20, 2022

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Jan 21, 2022

cgwalters left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `rpm-ostree ex rebuild` command #3340

Add `rpm-ostree ex rebuild` command #3340

jlebon commented Jan 18, 2022 •

edited

Loading