Use supermin in unprivileged environments #190

jlebon · 2018-10-31T17:30:00Z

This is a rebased rework of #124 with some modifications:

We auto-detect if we have CAP_SYS_ADMIN and if not, fall back to using
supermin. My position is that both approaches will be in use in CI
contexts and that the privileged case is faster for local dev, where
iterating fast on the content will matter. I've also hopefully
implemented things in a way that maintains almost the exact same logic
build-wise between the two flows so there's not too much divergence.
Anyway, totally open to revisiting this if needed!
In the virtualized path, fetch now directly populates the qcow2
cache so that the split fetch/build approach keeps working as
expected.
We drop the repo-build/ repo since it's essentially also a cache and
duplicates content from the archive repo. This is also needed to
ensure that the pkgcache repo and the repo we commit into are both on
the same file system.
The supermin appliance is reused if already generated; the runvm
command just takes the command you want to run verbatim and plops it
into a file the appliance is already coded to check from.

Some other minor fixes:

We handle symlinked repos.
Split out supermin packages into a separate file.
Capture rc and bubble that up to the runvm caller.
Add virtio-rng device.

Originally based on a patch by:
Dusty Mabe [email protected]

jlebon · 2018-10-31T17:30:35Z

Marking as WIP because I still need to test this in a fully unprivileged pod in CentOS CI.

jlebon · 2018-10-31T17:34:51Z

Requires: ostreedev/ostree#1769

src/cmdlib.sh

cgwalters · 2018-10-31T18:07:42Z

src/cmd-init

@@ -116,4 +116,7 @@ mkdir -p cache
 mkdir -p builds
 mkdir -p tmp
 ostree --repo=repo init --mode=archive
-ostree --repo=repo-build init --mode=bare-user
+if ! has_privileges; then
+    LIBGUESTFS_BACKEND=direct qemu-img create -f qcow2 cache.qcow2 10G


The LIBGUESTFS_BACKEND bit here is a no-op, qemu-img isn't libguestfs. I think you meant the line below? But we do that globally now in libguestfs.sh right?

Ahh yeah, good catch. Fixed!

But we do that globally now in libguestfs.sh right?

Do you mean sourcing that by default in all the command scripts? I suppose we could, though we don't really need any of the other functions there.

src/cmdlib.sh

src/supermin-init-prelude.sh

src/cmdlib.sh

dustymabe · 2018-10-31T18:12:27Z

src/cmdlib.sh

+        touch "${vmbuilddir}/.done"
+    fi
+
+    echo "$@" > ${TMPDIR}/cmd.sh


in this case this works because ${TMPDIR} is under ${workdir} right?

I've actually seen issues running in openshift where having $TMPDIR be on the nfs mounted volume caused real issues for me so I changed it to not be exported: dustymabe@7ecba34

dustymabe · 2018-10-31T18:15:16Z

src/cmdlib.sh

+    local vmpreparedir=${workdir}/tmp/supermin.prepare
+    local vmbuilddir=${workdir}/tmp/supermin.build
+
+    # use REBUILDVM=1 if e.g. hacking on rpm-ostree/ostree and wanting to get


the problem with not building the vm every time is that it can get out of date (i.e. new deps needed, etc). This is probably fine for your local dev case, but not ideal for having to go do something in your CI. I converted my master branch to just rebuild it every time. I found it didn't really add that much extra time to the compose.

then again, if we are doing it for fetch and build then it starts to get a bit more heavyweight

Yeah, that's the thing. We're calling this now both on fetch and build. I'm open to dropping that logic if it causes problems, though offhand it's nice to be able to skip this also for the local dev case (where you're actually testing the unpriv path).

I think we could make fetch unprivileged without too much effort BTW.

I think we could make fetch unprivileged without too much effort BTW.

right, but if we are in "unpriv mode" we still need to run it in the VM so it can access the cache I think.

src/cmdlib.sh

dustymabe · 2018-10-31T18:23:55Z

thanks @jlebon

src/cmdlib.sh

Previously, we were limiting the target repo in unified mode to be a bare-user repo located on the same filesystem (see message of previous commit). This patch lifts this restriction by making a distinction between the *build repo* and the *final* target repo. To do this, we create a bare-user repo located near the pkgcache to take advantage of hardlinks and devino caching at commit time. And only after committing do we essentially `pull-local` into the final target repo. This of course allows us to avoid potentially pulling across the two filesystems file objects that are already present in the target repo. This will be used by coreos-assembler: coreos/coreos-assembler#190

Previously, we were limiting the target repo in unified mode to be a bare-user repo located on the same filesystem (see message of previous commit). This patch lifts this restriction by making a distinction between the *build repo* and the *final* target repo. To do this, we create a bare-user repo located near the pkgcache to take advantage of hardlinks and devino caching at commit time. And only after committing do we essentially `pull-local` into the final target repo. This of course allows us to avoid potentially pulling across the two filesystems file objects that are already present in the target repo. This will be used by coreos-assembler: coreos/coreos-assembler#190 Closes: coreos#1490

jlebon · 2018-11-05T20:30:54Z

This now works on top of coreos/rpm-ostree#1657, which greatly simplified things. I'm still testing this in CentOS CI (it's a pain to iterate on this in a truly unprivileged pod since it requires rebuilding the container each time), which will likely require cherry-picking some of Dusty's patches to work around libvirt wanting a valid user etc...

I kept the changes as a fixup! to make it easier to see though will squash it in before we're ready to go!

cgwalters · 2018-11-05T20:36:15Z

(it's a pain to iterate on this in a truly unprivileged pod since it requires rebuilding the container each time

I think it'd work to use oc rsh to copy in the binary, or set up an unprivileged pet container with a host bind mount etc.

Previously, we were limiting the target repo in unified mode to be a bare-user repo located on the same filesystem (see message of previous commit). This patch lifts this restriction by making a distinction between the *build repo* and the *final* target repo. To do this, we create a bare-user repo located near the pkgcache to take advantage of hardlinks and devino caching at commit time. And only after committing do we essentially `pull-local` into the final target repo. This of course allows us to avoid potentially pulling across the two filesystems file objects that are already present in the target repo. This will be used by coreos-assembler: coreos/coreos-assembler#190 Closes: #1490 Closes: #1657 Approved by: cgwalters

dustymabe · 2018-11-05T22:24:31Z

(it's a pain to iterate on this in a truly unprivileged pod since it requires rebuilding the container each time

I think it'd work to use oc rsh to copy in the binary, or set up an unprivileged pet container with a host bind mount etc.

I was using a sleep pod based on the coreos-assembler OCI image and then copying the scripts into a writable directory and modifying them and then running everything from there

dustymabe · 2018-11-05T22:24:53Z

Thanks @jlebon for the updated code. Will review it soon!

dustymabe · 2018-11-06T03:13:22Z

src/cmdlib.sh

+            ${manifest} "$@"
+
+    if ! grep -q '^# disable-unified-core' "${manifest}"; then
+        set - "$@" --unified-core


colin had told me that I could drop this check so I did in my fork. i.e. it is safe to assume anything using coreos-assembler will be using unified core in the future.

dustymabe · 2018-11-06T04:00:20Z

this looks mostly good to me. I think other issues can probably be resolved in follow up PRs. I'm going to run this locally in the morning too.

Makes for a scary message otherwise in the `ENOENT` case.

On the very first build, if the treecompose worked, but image generation failed, we would hit `${previous_image_genver}` not being defined (since there wasn't a completed previous build yet) while trying to come up with a new buildid.

This is a rebased rework of coreos#124 with some modifications: - We auto-detect if we have CAP_SYS_ADMIN and if not, fall back to using supermin. My position is that both approaches will be in use in CI contexts and that the privileged case is faster for local dev, where iterating fast on the content will matter. I've also hopefully implemented things in a way that maintains almost the exact same logic build-wise between the two flows so there's not too much divergence. Anyway, totally open to revisiting this if needed! - In the virtualized path, `fetch` now directly populates the qcow2 cache so that the split `fetch`/`build` approach keeps working as expected. - We drop the repo-build/ repo since it's essentially also a cache and duplicates content from the archive repo. This is also needed to ensure that the pkgcache repo and the repo we commit into are both on the same file system. - The supermin appliance is reused if already generated; the `runvm` command just takes the command you want to run verbatim and plops it into a file the appliance is already coded to check from. Some other minor fixes: - We handle symlinked repos. - Split out supermin packages into a separate file. - Capture rc and bubble that up to the `runvm` caller. - Add virtio-rng device. Originally based on a patch by: Dusty Mabe <[email protected]>

If you don't you get errors like this from libvirt: error: Cannot create user runtime directory '//.cache/libvirt': Permission denied

We don't want to do this because then subprocesses (like anything with libguestfs) will possibly fail on openshift with errors like: libguestfs: error: security: cached appliance # /srv/tmp/build/tmp/.guestfs-1000240000 is not owned by UID # 1000240000

Even though rpm-ostree now transparently supports both modes without having to do the bare-user dance, let's just drop it anyway so we can concentrate on a single way of doing things.

jlebon · 2018-11-06T14:43:51Z

OK, this is now tested working in CentOS CI! Lifting WIP!

(it's a pain to iterate on this in a truly unprivileged pod since it requires rebuilding the container each time

I think it'd work to use oc rsh to copy in the binary, or set up an unprivileged pet container with a host bind mount etc.

I was using a sleep pod based on the coreos-assembler OCI image and then copying the scripts into a writable directory and modifying them and then running everything from there

That doesn't work if you need a custom rpm-ostree though :) And supermin takes the binaries from the RPM paths.

I ended up running in a privileged pod to do my hacking but then for testing doing:

$ alias unpriv='sudo setpriv --reuid=1000240000 --regid 0 --inh-caps -all --ambient-caps -all --bounding-set -all --no-new-privs --groups 0,1000240000 env -u USER -u USERNAME HOME=/'
$ unpriv coreos-assembler ...

I also made sure to test with the /srv PVC mount point to make sure we play nice with NFS.

jlebon · 2018-11-06T14:45:51Z

coreos-assembler

+  # We need to make sure we set $HOME in the /etc/passwd file because
+  # if we don't libvirt will try to use `/` and we will get permission
+  # issues
+  export HOME="/var/tmp/${USER_NAME:-default}" && mkdir -p $HOME


@dustymabe I slightly modified your patch here to put it in /var/tmp instead of /tmp since the former is less likely to be on tmpfs.

sounds good

dustymabe · 2018-11-06T14:52:18Z

That doesn't work if you need a custom rpm-ostree though :) And supermin takes the binaries from the RPM paths.

ahh yes. when I was hacking I was just hacking on the coreos-assembler scripts

dustymabe · 2018-11-06T15:46:34Z

this is looking good to me. doing local testing now

jlebon · 2018-11-06T21:31:12Z

Alrighty, looks like it worked! http://artifacts.ci.centos.org/fedora-coreos/delete/builds/29.3/

dustymabe · 2018-11-06T21:37:47Z

@cgwalters can you confirm this isn't going to break the pipeline v2 stuff before we merge it?

cgwalters · 2018-11-06T21:39:21Z

Hmm. How about we do a release of the current git before merging as v0.3.1 or so, then start v0.4.0 after this merges?

jlebon · 2018-11-06T21:45:45Z

That sounds good to me. FWIW, since today the RHCOS pipeline is privileged, it shouldn't affect it. The major change though is that the repo-build/ dir is now unused. Hmm, maybe I should add a rm -rf repo-build in there for compatibility.

dustymabe · 2018-11-06T21:47:36Z

Hmm. How about we do a release of the current git before merging as v0.3.1 or so, then start v0.4.0 after this merges?

sounds good I'll run a v0.3.1 release and then merge this.

Hmm, maybe I should add a rm -rf repo-build in there for compatibility.

i wouldn't worry about it I don't think

It took me a while to debug why my newly built images had old ostree commits in them; the summary update was removed in coreos#190 Let's add it back in.

It took me a while to debug why my newly built images had old ostree commits in them; the summary update was removed in #190 Let's add it back in.

I found myself doing a lot of `init` operations when testing changes related to #190 and wanted a way to skip the download of the ISO/sha256sum. This change introduces a new flag (`--installerdir`) that will instruct the `init` command to look in a provided directory for the ISO and sha256sum. If the required files are found, they'll be copied to the `installer` directory for use later on. If the `--force` option is provided, it downloads the ISO/sha256sum regardless if the `--installerdir` flag is provided.

It took me a while to debug why my newly built images had old ostree commits in them; the summary update was removed in coreos#190 Let's add it back in.

jlebon mentioned this pull request Oct 31, 2018

WIP: run compose in small supermin VM #124

Closed

dustymabe self-requested a review October 31, 2018 17:50

cgwalters reviewed Oct 31, 2018

View reviewed changes

dustymabe requested changes Oct 31, 2018

View reviewed changes

dustymabe reviewed Oct 31, 2018

View reviewed changes

src/cmdlib.sh Outdated Show resolved Hide resolved

jlebon mentioned this pull request Nov 2, 2018

compose: Support all target repos in unified mode coreos/rpm-ostree#1657

Closed

cgwalters mentioned this pull request Nov 5, 2018

Fix 'shellcheck' complaints #193

Merged

jlebon force-pushed the pr/rework-supermin branch from 6e624f9 to 4c051fc Compare November 5, 2018 20:25

dustymabe reviewed Nov 6, 2018

View reviewed changes

jlebon and others added 7 commits November 6, 2018 09:05

cmd-build: Silence ostree rev-parse

d0f9aa0

Makes for a scary message otherwise in the `ENOENT` case.

cmd-build: Handle resuming builds on first build

d909410

On the very first build, if the treecompose worked, but image generation failed, we would hit `${previous_image_genver}` not being defined (since there wasn't a completed previous build yet) while trying to come up with a new buildid.

Support arbitrary UIDs for OpenShift

8c380d0

Set $HOME path in /etc/passwd on openshift

f302c82

If you don't you get errors like this from libvirt: error: Cannot create user runtime directory '//.cache/libvirt': Permission denied

Don't export TMPDIR variable

91d8f2a

We don't want to do this because then subprocesses (like anything with libguestfs) will possibly fail on openshift with errors like: libguestfs: error: security: cached appliance # /srv/tmp/build/tmp/.guestfs-1000240000 is not owned by UID # 1000240000

Drop support for non-unified core mode

18a0752

Even though rpm-ostree now transparently supports both modes without having to do the bare-user dance, let's just drop it anyway so we can concentrate on a single way of doing things.

jlebon force-pushed the pr/rework-supermin branch from 4c051fc to 18a0752 Compare November 6, 2018 14:42

jlebon changed the title ~~WIP: Use supermin in unprivileged environments~~ Use supermin in unprivileged environments Nov 6, 2018

jlebon commented Nov 6, 2018

View reviewed changes

dustymabe merged commit ffdbca4 into coreos:master Nov 6, 2018

dustymabe mentioned this pull request Nov 7, 2018

support compose with default docker/kube privileges coreos/rpm-ostree#1329

Open

cgwalters mentioned this pull request Nov 9, 2018

Adds qemu wrapper to disable pmu in cpu flags #199

Merged

cgwalters mentioned this pull request Nov 10, 2018

cmd-build: Update the summary again #204

Merged

cgwalters added a commit that referenced this pull request Nov 12, 2018

cmd-build: Update the summary again

5c9777d

It took me a while to debug why my newly built images had old ostree commits in them; the summary update was removed in #190 Let's add it back in.

miabbott mentioned this pull request Nov 12, 2018

init: add ability to use existing ISO/sha256sum #210

Merged

jlebon mentioned this pull request Nov 23, 2018

Support for rootless builds #236

Closed

This was referenced Nov 26, 2018

cmd-build: Update the summary again #238

Closed

Teach installer to specify OS version through oscontainer image URLs openshift/installer#281

Closed

change kola to run unprivileged coreos/mantle#956

Closed

jlebon deleted the pr/rework-supermin branch July 6, 2020 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use supermin in unprivileged environments #190

Use supermin in unprivileged environments #190

jlebon commented Oct 31, 2018

jlebon commented Oct 31, 2018

jlebon commented Oct 31, 2018

cgwalters Oct 31, 2018

jlebon Nov 5, 2018

dustymabe Oct 31, 2018

dustymabe Oct 31, 2018

dustymabe Oct 31, 2018

jlebon Nov 5, 2018

cgwalters Nov 5, 2018

dustymabe Nov 5, 2018

dustymabe commented Oct 31, 2018

jlebon commented Nov 5, 2018

cgwalters commented Nov 5, 2018

dustymabe commented Nov 5, 2018

dustymabe commented Nov 5, 2018

dustymabe Nov 6, 2018

dustymabe commented Nov 6, 2018

jlebon commented Nov 6, 2018

jlebon Nov 6, 2018

dustymabe Nov 6, 2018

dustymabe commented Nov 6, 2018

dustymabe commented Nov 6, 2018

jlebon commented Nov 6, 2018

dustymabe commented Nov 6, 2018

cgwalters commented Nov 6, 2018

jlebon commented Nov 6, 2018

dustymabe commented Nov 6, 2018

Use supermin in unprivileged environments #190

Use supermin in unprivileged environments #190

Conversation

jlebon commented Oct 31, 2018

jlebon commented Oct 31, 2018

jlebon commented Oct 31, 2018

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

dustymabe commented Oct 31, 2018

jlebon commented Nov 5, 2018

cgwalters commented Nov 5, 2018

dustymabe commented Nov 5, 2018

dustymabe commented Nov 5, 2018

Choose a reason for hiding this comment

dustymabe commented Nov 6, 2018

jlebon commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe commented Nov 6, 2018

dustymabe commented Nov 6, 2018

jlebon commented Nov 6, 2018

dustymabe commented Nov 6, 2018

cgwalters commented Nov 6, 2018

jlebon commented Nov 6, 2018

dustymabe commented Nov 6, 2018