Unified core #940

cgwalters · 2017-08-23T16:23:02Z

WIP for #729

May extract some of the prep to a separate PR soon.

cgwalters · 2017-08-23T16:23:29Z

rh-atomic-bot · 2017-08-30T16:08:10Z

☔ The latest upstream changes (presumably 3047513) made this pull request unmergeable. Please resolve the merge conflicts.

rh-atomic-bot · 2017-09-12T15:12:00Z

☔ The latest upstream changes (presumably f113fc5) made this pull request unmergeable. Please resolve the merge conflicts.

Lots of confusion in the codebase about this. The basic problem is that in *most* cases, our code doesn't care; it's conceptually operating on `/usr/etc`, which we could maintain as `/etc` and just rename it back at the very end. The exceptions though are the `/etc/passwd` handling and livefs. And of course libostree needs to handle `/usr/etc` vs `/etc` for config merging. I considered trying to keep things the other way, but while I think we have some ugly added here in this patch for things where we need to maintain an external view (`remove-files` and `remove-from-packages`, and boy am I glad we had tests for those), this ends up being mostly more consistent elsewhere. One thing that might help is to maintain a fd for it; but that'd be an even more invasive change. This also ends up rolling in some unified core prep from coreos#940 in the form of `rename_if_exists()` - basically for some minimal rootfs we may not have `/boot`, or for that matter potentially even `etc`. Prep for coreos#997

Lots of confusion in the codebase about this. The basic problem is that in *most* cases, our code doesn't care; it's conceptually operating on `/usr/etc`, which we could maintain as `/etc` and just rename it back at the very end. The exceptions though are the `/etc/passwd` handling and livefs. And of course libostree needs to handle `/usr/etc` vs `/etc` for config merging. I considered trying to keep things the other way, but while I think we have some ugly added here in this patch for things where we need to maintain an external view (`remove-files` and `remove-from-packages`, and boy am I glad we had tests for those), this ends up being mostly more consistent elsewhere. One thing that might help is to maintain a fd for it; but that'd be an even more invasive change. This also ends up rolling in some unified core prep from #940 in the form of `rename_if_exists()` - basically for some minimal rootfs we may not have `/boot`, or for that matter potentially even `etc`. Prep for #997 Closes: #997 Approved by: jlebon

cgwalters · 2017-11-14T18:33:04Z

OK, this is updated again against master. Required for rpm-ostree jigdo ♲📦. Still WIP...going to try to clean things up a bit.

cgwalters · 2017-11-14T23:14:50Z

Note to self, need to implement NODOCS, that's an immediate obvious diff between libdnf and ucore compose tree. Tentatively thinking of doing this at unpack time.

rh-atomic-bot · 2017-11-16T15:37:31Z

☔ The latest upstream changes (presumably 1240d8d) made this pull request unmergeable. Please resolve the merge conflicts.

cgwalters · 2017-11-16T19:58:21Z

OK, lifting WIP on this. I have been squashing issues in the unprivileged path for a while but I am not sure how many more remain, and none of those really block the jigdo work. So I think my vote is let's land this, it's just an --ex option, but already works well enough I think for people to test out.

cgwalters · 2017-11-16T19:59:54Z

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

+      if (!glnx_opendirat (AT_FDCWD, "/", TRUE, &host_rootfs_dfd, error))
+        return FALSE;
+      g_autoptr(OstreeSePolicy) sepolicy = ostree_sepolicy_new_at (host_rootfs_dfd, cancellable, error);
+      if (!sepolicy)


I should note this also means that --ex-unified-core requires having selinux-policy-targeted in the container/host.

Let's just print a notice instead and keep going? It would make it easier to test from a container. :)
Anyway if caching, then we only pay that relabel cost once, right?

One can install the policy in a container, that's what I've done. We could stumble on but it means the relabel actually rewrites every object. Seems easier to require people to install policy?

(That said of course this whole thing intersects with #971)

jlebon

This looks really nice. Surprisingly less gory than expected.
Just one real concern, though I'd like to give it a spin as well.

jlebon · 2017-11-16T21:14:53Z

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

+    {
+      if (!rpmostree_context_import (self->corectx, cancellable, error))
+        return FALSE;
+      if (!rpmostree_context_relabel (self->corectx, cancellable, error))


We don't need this relabel though, right? Otherwise IIUC, we will unnecessarily relabel the whole cache everytime the build env changes policy, even if the target tree didn't.

It seems you're right...I am not entirely sure what I was thinking there. Deleted in a fixup ⬇️

jlebon · 2017-11-16T21:28:44Z

tests/compose-tests/libbasic-test.sh

@@ -0,0 +1,41 @@
+# This used to live in test-basic.sh, but it's now shared with test-basic-unified.sh


Minor: any reason for formatting this as a lib? Seems cleaner as a basic-test.sh we just source directly like ostree's basic-test.sh. Not super opposed either way though.

I kept this as just to avoid too much churn but may revisit later.

jlebon · 2017-11-16T21:34:09Z

Looks like f26-compose is failing with:

+ runcompose --ex-unified-core --add-metadata-from-json metadata.json
+ rpm-ostree compose tree --repo /var/tmp/rpmostree-compose-cache-0/repo-build --cache-only --cachedir=/var/tmp/rpmostree-compose-cache-0/cache composedata/fedora-basic.json --ex-unified-core --add-metadata-from-json metadata.json
No previous commit for fedora/stable/x86_64/basic
error: With policy root '/proc/self/fd/18/etc/selinux/targeted': selabel_open(SELABEL_CTX_FILE): No such file or directory

cgwalters · 2017-11-17T14:05:08Z

bot, retest this please

jlebon · 2017-11-17T14:46:08Z

Looks good! Though seems like CI is now failing with:

+ kver=4.11.8
+ ostree --repo=/var/tmp/rpmostree-compose-cache-0/repo-build ls fedora/stable/x86_64/basic /usr/lib/modules/4.11.8/vmlinuz /usr/lib/modules/4.11.8/initramfs.img
error: No such file or directory: /usr/lib/modules/4.11.8
+ _cleanup_tmpdir
+ test -z ''
+ test -f /var/tmp/rpm-ostree-compose-test.86iBZK/.test
+ rm /var/tmp/rpm-ostree-compose-test.86iBZK -rf
FAIL

rh-atomic-bot · 2017-11-17T15:08:11Z

☔ The latest upstream changes (presumably 7ab8869) made this pull request unmergeable. Please resolve the merge conflicts.

Adding the 2nd test revealed this didn't actually work, will fix at some point.

It seems that libarchive ends up returning `getuid()` actually, possibly because the cpio doesn't actually have ownership information? Anyways, what we really want here is to set `0/0`, which is what happens for the `ex container` path via `OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS`. Prep for unified core 🌐.

In an unprivileged case, we can't do this on the real filesystem. For `ex container`, we want to completely ignore uid/gid. I added a test installing `httpd` which failed previously. TODO: For non-root `--ex-unified-core` we need to do it as a commit modifier.

jlebon · 2017-11-17T15:12:44Z

And even when run as uid 0, there are some bugs, though I'm not sure
of any showstoppers yet. For example, dracut's dracut-install calls
cp --preserve=xattrs which fails to copy the user.ostreemeta xattrs
from a checkout (it shouldn't be copying that anyways...)

Yup, definitely also hitting this now. Did you investigate why it happens? It looks like we're getting ENOTSUP, which is probably due to /tmp being on tmpfs. But yet AFAIK, we're using --dir /tmp when invoking bwrap and our rootfs is in the ostree repo tmpdir, right?

cgwalters · 2017-11-17T15:14:23Z

Yeah it's the user.ostreemeta xattr, and "no user. xattrs on tmpfs" combo. See dracutdevs/dracut@61c761b

cgwalters · 2017-11-17T15:16:16Z

(I suppose we could actually bind a host /var/tmp tmpdir?)

The "--ex" prefix here means it's an experimental option. A tremendous change here is that start to support non-uid 0, but there are various things to fix there; the unpacker for example needs to learn to set imported objects fully based on the rpmfi information (i.e. default to uid 0, since libarchive gives the current uid by default). And even when run as uid 0, there are some bugs, though I'm not sure of any showstoppers yet. For example, dracut's `dracut-install` calls `cp --preserve=xattrs` which fails to copy the `user.ostreemeta` xattrs from a checkout (it shouldn't be copying that anyways...) Nevertheless, the infrastructure behind this really helps (is almost a hard requirement for) the [jigdo effort](coreos#1081). Which is really only true due to SELinux - we need to import the packages, then generate the final tree to get the final policy, then use that policy to relabel all of the packages.

jlebon · 2017-11-17T16:24:14Z

Ahhh right, bwrap itself uses a tmpfs when creating the container root. Yeah, probably easier to just bind a /var/tmp tmpdir and drop the --tmpdir=/tmp argument. We can do that later though!

Seems like the unified test is still failing.

cgwalters · 2017-11-17T17:46:00Z

I am not sure what's going on, that kver is clearly wrong; must be something with the regexp? But I can't reproduce locally, and it sure seems to me the kernel version downloaded in the logs should be fine too. I added some more logging in a fixup and also reworked it to be a bit more robust. (I keep forgetting about grep -o)

cgwalters · 2017-11-17T18:07:25Z

Well, that fixed it anyways!

jlebon · 2017-11-17T18:59:30Z

@rh-atomic-bot r+ cb761dc

rh-atomic-bot · 2017-11-17T18:59:37Z

⚡ Test exempted: merge already tested.

It seems that libarchive ends up returning `getuid()` actually, possibly because the cpio doesn't actually have ownership information? Anyways, what we really want here is to set `0/0`, which is what happens for the `ex container` path via `OSTREE_REPO_COMMIT_MODIFIER_FLAGS_CANONICAL_PERMISSIONS`. Prep for unified core 🌐. Closes: #940 Approved by: jlebon

In an unprivileged case, we can't do this on the real filesystem. For `ex container`, we want to completely ignore uid/gid. I added a test installing `httpd` which failed previously. TODO: For non-root `--ex-unified-core` we need to do it as a commit modifier. Closes: #940 Approved by: jlebon

The "--ex" prefix here means it's an experimental option. A tremendous change here is that start to support non-uid 0, but there are various things to fix there; the unpacker for example needs to learn to set imported objects fully based on the rpmfi information (i.e. default to uid 0, since libarchive gives the current uid by default). And even when run as uid 0, there are some bugs, though I'm not sure of any showstoppers yet. For example, dracut's `dracut-install` calls `cp --preserve=xattrs` which fails to copy the `user.ostreemeta` xattrs from a checkout (it shouldn't be copying that anyways...) Nevertheless, the infrastructure behind this really helps (is almost a hard requirement for) the [jigdo effort](#1081). Which is really only true due to SELinux - we need to import the packages, then generate the final tree to get the final policy, then use that policy to relabel all of the packages. Closes: #940 Approved by: jlebon

cgwalters added the WIP label Aug 23, 2017

cgwalters mentioned this pull request Aug 24, 2017

unified core 🌐 migration #729

Open

cgwalters force-pushed the unified-core branch 2 times, most recently from 63f1485 to 6d79556 Compare August 30, 2017 12:41

cgwalters force-pushed the unified-core branch from 6d79556 to aabc023 Compare September 11, 2017 21:47

cgwalters force-pushed the unified-core branch from aabc023 to cfaaf81 Compare September 25, 2017 17:37

cgwalters mentioned this pull request Sep 25, 2017

Copy in additional files before postprocessing #997

Closed

cgwalters force-pushed the unified-core branch from cfaaf81 to 1aa4c9d Compare November 14, 2017 18:31

cgwalters mentioned this pull request Nov 14, 2017

rpm-ostree jigdo ♲📦 initial code drop #1103

Closed

cgwalters force-pushed the unified-core branch from 1aa4c9d to d192635 Compare November 14, 2017 20:48

cgwalters force-pushed the unified-core branch from d192635 to ee60d1f Compare November 16, 2017 19:54

cgwalters changed the title ~~WIP: Unified core~~ Unified core Nov 16, 2017

cgwalters commented Nov 16, 2017

View reviewed changes

cgwalters removed the WIP label Nov 16, 2017

jlebon reviewed Nov 16, 2017

View reviewed changes

cgwalters force-pushed the unified-core branch from ee60d1f to 3225aea Compare November 16, 2017 22:14

cgwalters added 3 commits November 17, 2017 10:12

tests/ex-container: Disable parallelism for now

a7c2fc9

Adding the 2nd test revealed this didn't actually work, will fix at some point.

cgwalters added 3 commits November 17, 2017 10:19

fixup! compose: Add --ex-unified-core

af03a02

fixup! compose: Add --ex-unified-core

281ea46

cgwalters force-pushed the unified-core branch from a8cf584 to 281ea46 Compare November 17, 2017 15:21

cgwalters added 2 commits November 17, 2017 12:31

fixup! compose: Add --ex-unified-core

c25b395

fixup! compose: Add --ex-unified-core

cb761dc

rh-atomic-bot closed this in 71d04a4 Nov 17, 2017

cgwalters mentioned this pull request Dec 2, 2017

compose/dracut: Use a host tmpdir for dracut #1126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unified core #940

Unified core #940

cgwalters commented Aug 23, 2017

cgwalters commented Aug 23, 2017

rh-atomic-bot commented Aug 30, 2017

rh-atomic-bot commented Sep 12, 2017

cgwalters commented Nov 14, 2017

cgwalters commented Nov 14, 2017

rh-atomic-bot commented Nov 16, 2017

cgwalters commented Nov 16, 2017

cgwalters Nov 16, 2017

jlebon Nov 16, 2017

cgwalters Nov 16, 2017

jlebon left a comment

jlebon Nov 16, 2017

cgwalters Nov 16, 2017

jlebon Nov 16, 2017

cgwalters Nov 17, 2017

jlebon commented Nov 16, 2017

cgwalters commented Nov 17, 2017

jlebon commented Nov 17, 2017

rh-atomic-bot commented Nov 17, 2017

jlebon commented Nov 17, 2017

cgwalters commented Nov 17, 2017 •

edited

cgwalters commented Nov 17, 2017

jlebon commented Nov 17, 2017

cgwalters commented Nov 17, 2017

cgwalters commented Nov 17, 2017

jlebon commented Nov 17, 2017

rh-atomic-bot commented Nov 17, 2017

		@@ -0,0 +1,41 @@
		# This used to live in test-basic.sh, but it's now shared with test-basic-unified.sh

Unified core #940

Unified core #940

Conversation

cgwalters commented Aug 23, 2017

cgwalters commented Aug 23, 2017

rh-atomic-bot commented Aug 30, 2017

rh-atomic-bot commented Sep 12, 2017

cgwalters commented Nov 14, 2017

cgwalters commented Nov 14, 2017

rh-atomic-bot commented Nov 16, 2017

cgwalters commented Nov 16, 2017

cgwalters Nov 16, 2017

Choose a reason for hiding this comment

jlebon Nov 16, 2017

Choose a reason for hiding this comment

cgwalters Nov 16, 2017

Choose a reason for hiding this comment

jlebon left a comment

Choose a reason for hiding this comment

jlebon Nov 16, 2017

Choose a reason for hiding this comment

cgwalters Nov 16, 2017

Choose a reason for hiding this comment

jlebon Nov 16, 2017

Choose a reason for hiding this comment

cgwalters Nov 17, 2017

Choose a reason for hiding this comment

jlebon commented Nov 16, 2017

cgwalters commented Nov 17, 2017

jlebon commented Nov 17, 2017

rh-atomic-bot commented Nov 17, 2017

jlebon commented Nov 17, 2017

cgwalters commented Nov 17, 2017 • edited

cgwalters commented Nov 17, 2017

jlebon commented Nov 17, 2017

cgwalters commented Nov 17, 2017

cgwalters commented Nov 17, 2017

jlebon commented Nov 17, 2017

rh-atomic-bot commented Nov 17, 2017

cgwalters commented Nov 17, 2017 •

edited