-
Notifications
You must be signed in to change notification settings - Fork 192
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
Package layering #107
Package layering #107
Conversation
I've been thinking about this more recently...obviously having to detect unsafe We wouldn't be using FUSE to run, only to build. |
@cgwalters sounds like a good idea; do you have experience with fuse filesystems? |
Another idea for handling this - write a LD_PRELOAD wrapper that catches open/openat. If the call is not O_RDONLY, then do the cp/mv trick and then run open on the newly created path. http://www.catonmat.net/blog/simple-ld-preload-tutorial-part-2/ fakeroot does something similar - http://fakeroot.alioth.debian.org/. |
@dbnicholson |
@cgwalters Definitely if you're trying to wrap any system call that can create a file, LD_PRELOAD is going to suck (been there before). However, this just has a single specific case to cover - don't write into files that have multiple hardlinks. I believe that could be covered just by catching open and openat. That said, if you can come up with a fuse implementation, that would be better. I'll probably try the preload hack to see what it would look like, anyway. |
Ok, I did this: https://github.com/cgwalters/rofiles-fuse Was quite easy. I think it's correct. Now to stick it in...ostree? rpm-ostree? Debating. |
Nice. I'd like it if it was in ostree itself. Then it could be used in checkout to take a commit, safely update files within it, and then make a new commit. I don't think that's possible currently. |
7c1d7ae
to
da1df77
Compare
No major changes, just rebased on master. |
da1df77
to
01f13c0
Compare
01f13c0
to
654f33b
Compare
Rebased on master. But still runs from the client - we need to have this code run from the daemon. |
654f33b
to
39fe5ec
Compare
TODO:
|
Ahhh, looks like ostree has a new dependency on yacc. Will update the test container. |
bot, retest this please |
Teaching So what we need to do is efficiently cache the unpacked RPM data we get from the tree itself. The interesting problem here is that RPMs only have SELinux labels applied at install time. If policy changes, we need to apply the new label. Which means we need a physical copy of the file. Representing each RPM as a commit tree itself is the obvious thing to do - then we store metadata such as the checksum of the SELinux policy that was used for labeling? Alternatively, we do "trust but verfiy" by unpacking the commit again, but then verifying the labels vs the current policy. |
0378099
to
4a9f20d
Compare
The next major item here is to move most of the logic into the Upgrader class so it can be shared with plain |
This builds upon the earlier prototype in https://github.com/cgwalters/atomic-pkglayer The `.origin` file says for a replicated installation: [origin] refspec=local:rhel-atomic-host/7/x86_64/standard If you then run `rpm-ostree pkg-add strace`, it will result in a new tree with: [origin] baserefspec=local:rhel-atomic-host/7/x86_64/standard [packages] requested=strace; Work still remaining here is to teach `rpm-ostree status` and `rpm-ostree upgrade` about this.
OK, I pushed some more commits on my package-layering branch. Commits before 19233b9 are mostly focused on cleaning up the original package-layering branch, while the ones after start the work on migrating to the RPM import model. Overview of the branch
There's still a lot left to do (quoting from the TODO I added to track them all :) ):
Will update as I check off these items. |
At a high level I am a fan of smaller commits...but I'm a little uncertain about this small. It feels like a lot of them could be rolled up as "libpriv: Rework core API to prepare for package layering merge" Then the commit message could include the commit messages from the relevant commits there. Maybe what I'm arguing for here is to split out internal changes not related to layering first, change the container bits to use those, then land layering... 😎 as a layer on top? Not a hard requirement from my POV, just if you agree. Though maybe there are shades here, as |
Yup, that sounds good to me! Will squash the trivial ones. WDYT of the actual pkg layering commit in general? |
OK, pushed to the branch again. Rebased on master, squashed some commits, and made some progress. We now actually check for the final set of RPMs to install in the deployment. The logic for this was a bit trickier than expected. E.g. when you do a In the end, this means that what goes in the deployment's origin's "requested" set is not necessarily the same as the actual set of pkgs we need to layer, which was my assumption previously. Next up is tackling the SELinux issues mentioned previously. Though it's definitely in a good enough state to test and play around with. |
What do you think about submitting PRs for things that can basically be cherry-picked to master? Like jlebon@2949cbe ? |
Pushed some new commits to rework the package layering logic a bit and added a In my initial attempt at adding The downside is that we now do slightly more work when doing multiple |
OK, took an initial stab at the SELinux code in the latest commit (jlebon/rpm-ostree@98b6747). We now pick up the sepolicy from the merge deployment and lookup the label as we unpack. We also record the policy csum in the commit metadata. It's still early, so the way these things are implemented might change (e.g. it'd be nice to push some of the work into ostree instead). Next up is cache validation based on the policy csum. |
Another SELinux issue here is how we deal with packages which bring their own pps. These are normally installed in %post, which we don't run. It seems like we'll have to be heuristicky here and assume that all pps the pkg adds to There's also a sort of convergence issue with this since we label stuff with the new sepolicy as we import the pkg. Since we'll only find out about the new pps and recompile after we're done unpacking, we might have to re-unpack and relabel the pkg. Maybe we can just do a quick scan of all the archive entries first to check for pps. Or maybe it would be easier to just unpack to a tmprootfs rather than directly committing from libarchive to ostree (maybe that will help us get rid of the libarchive-input-stream.c copy/paste as well?). And of course, yet another possible issue with this is #27. |
OK pushed some more commits on my branch. I had also rebased on master, though I think it's already out of sync. To summarize, we now mark the csum of the policy during pkg import. Then during overlay, if we see a package that requires relabeling (i.e. target policy csum != import csum), we check it out, relabel, and add a commit to the package branch. More in-depth testing is definitely required, but it works well so far. One interesting "issue" is that after writing the new deployment, since we call I added a couple of things in the TODO file to track pending items. Of those, these top three are probably hard reqs for a seamless user experience, so I'll be working on those now:
That said, I'd like to get what's there merged (but maybe with the pkg-* commands hidden) so that (1) it will be less of a pain to rebase everytime, and (2) it will be easier for others to test (e.g. we could probably announce it at least on atomic-devel). I can clean up and squash some commits soon if we want to go that way. |
The issue above made me think about whether we should keep unpacked packages in a child repo so that we have better control over them. It seems like it just makes sense to have |
Just to see what it would look like, I did this in jlebon@b914f30. We store the unpacked pkgs in a child repo at The advantages of this method are:
It should be an easy patch to revert down the line, so I'll just try it out this way and see what it gives. |
The packages with custom policies is indeed a nasty circular dependency. I don't even know how that works with rpm. Offhand I think librpm reloads its file context matching any time policy is loaded, but in this case it must have already laid out the files on disk. Oh wait, librpm is probably relying on transaction ordering here. This means we can't parallelize imports =( |
BTW sorry I didn't respond here earlier, I missed all of these notifications recently I think due to a flood of other notifications (kubernetes/contrib changed, the ADB people got very active). |
Child repo is OK by me. It's an interesting concept whether to model things like this as child repos or "prefixed branches" in a single repo. Offhand it seems the child repo is easier for management and avoids GC/locking issues, but reduces sharing. My instinct here is sharing is mostly useful if you expect things to be shared. For example if one has both a Debian and Fedora trees, they probably share very little at an actual binary level. But, an imported Fedora Docker base image probably shares a lot with the host system (assuming same major version). So that would argue for projectatomic/atomic#298 to use the primary repo, and not be a child repo. Hmm, actually I just realized...GC in parent repos is unaware of child repos. It should probably work a bit more like |
One hack that would probably get us pretty far is to require a marker on any packages that include selinux policy, where an initial definition of "marker" is "has |
There's two major pieces of work here. First is to allow layering "simple" packages like strace. Let's take a value of "simple" = "no %post". That covers things like It seems very reasonable to land that in master now. We can try to change things to error out if we detect a Then the branch continues for handling the fully general case of server-side treecompose, which has to handle everything. That fully general case also fixes things like installing a custom docker with new selinux policy. |
Hmm interesting, I didn't make that connection before, but that makes a lot of sense.
Yeah :(. The upside I guess is that it gives a way to handle this properly. We can use librpm to sort the pkgs to import (like we do during assembly), then during each unpacking use a flag to determine whether we have to recompile the policy before unpacking the next pkg. Oh wait, since we're only importing a subset of all the pkgs in the transaction, I wonder if rpmtsOrder() can fail sometimes.
Ouch, yeah that's an issue. Though I guess it doesn't affect us here since we never actually look up objects in the parent repo (other than the files we just checked out from it into the tmprootfs) and only really "push up".
OK that's a good way to break this up and get more early testers. Maybe we can allow but print a warning if we detect a Almost about to land a rework of the unpacker which does proper labeling of parent dirs. Afterwards I'll take a look at making libhif work with repo vars. I don't expect that to take too long, but if there's a blocking factor there, I'll just clean up my branch and get it ready for a merge. |
Pushed rpmostree-unpacker rework: jlebon@72ad56c
|
WDYT about pushing this to the upstream repo as package-layering so we can both push to it? |
@cgwalters Sounds good, pushed it. |
Pushed a major rework of the unpacker: 8baffc0
|
re hardlinks, oops! Hmm...i'm not sure about unpacking to a tmprootfs. That has a few problems, such as the fact that for e.g. setuid binaries they'll exist on the host temporarily. Also, it's harder to run as unprivileged - it means we'd try to chown files to uid 0 as non-root, etc. I think I'd be more in favor of dropping the unpack_to_dfd path, since it's mostly for debugging. Anyone who wants to unpack an rpm by hand already has For hardlink support...it sounds like you're right this is a bug in the ostree-libarchive bits. I think we'd have to do something like hold a reference to mtrees for each hardlink target, and when we find the actual content, update the target mtrees? |
Closing in favour of #289. |
This is a prototype patch series - it's taking https://github.com/cgwalters/atomic-pkglayer and moving it into the rpm-ostree core. However, as the last patch notes,
upgrade
andstatus
(andpkg-remove
) are not yet implemented.