Skip to content
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

Add "transient" unlock #2103

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

cgwalters
Copy link
Member

I was thinking a bit more recently about the "live" changes
stuff coreos/rpm-ostree#639
(particularly since coreos/rpm-ostree#2060 )
and I realized reading the last debates in that issue that
there's really a much simpler solution; do exactly the same
thing we do for ostree admin unlock, except mount it read-only
by default.

Then, anything that wants to modify it does the same thing
libostree does for /sysroot and /boot as of recently; create
a new mount namespace and do the modifications there.

The advantages of this are numerous. First, we already have
all of the code, it's basically just plumbing through a new
entry in the state enumeration and passing MS_RDONLY into
the mount() system call.

"live" changes here also naturally don't persist, unlike what
we are currently doing in rpm-ostree.

@cgwalters
Copy link
Member Author

That said, with this I'm not sure if there's a sane way to get hardlinks from the base ostree to new content. overlayfs does a "copy up" on any change, and it's not supported to mutate the lowerdir AFAIK.

If I do go "underneath" the overlayfs mount and e.g. ln bash bash2, that appears just fine in ls /usr/bin/bash2 but again the docs say:

Changes to the underlying filesystems while part of a mounted overlay filesystem are not allowed. If the underlying filesystem is changed, the behavior of the overlay is undefined, though it will not result in a crash or deadlock.

Perhaps for "small scale" changes, having the new content live in RAM isn't very problematic.

@jlebon
Copy link
Member

jlebon commented May 26, 2020

Hmm, interesting idea.

That said, with this I'm not sure if there's a sane way to get hardlinks from the base ostree to new content. overlayfs does a "copy up" on any change, and it's not supported to mutate the lowerdir AFAIK.

So in other words, we'd only be able to run livefs once?

@cgwalters
Copy link
Member Author

So in other words, we'd only be able to run livefs once?

We can always inject whatever we want on the top of the overlay; but it's less efficient obviously.

Hmm...I need to investigate using metacopy=on.

@cgwalters
Copy link
Member Author

I think something new broke in CI here; rebased this one as a sanity check.

@cgwalters
Copy link
Member Author

OK I'm increasingly thinking this approach is good - it maximizes flexibility and is a small tweak on the existing code we have now. It will make implementing "live" updates obviously safe, and also be an alternative path for coreos/fedora-coreos-tracker#354 instead of /run/bin.

Let's get this in?

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me overall! And I think the idea of making livefs be a completely transient thing is in line with discussions we've had about redesigning it.

@@ -318,6 +318,8 @@ ostree_deployment_unlocked_state_to_string (OstreeDeploymentUnlockedState state)
return "hotfix";
case OSTREE_DEPLOYMENT_UNLOCKED_DEVELOPMENT:
return "development";
case OSTREE_DEPLOYMENT_UNLOCKED_TRANSIENT:
return "transient";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bikeshed/optional: kinda feels like "development" should be aliased to "transient" instead and this new mode would be "transient-readonly" or something? That would capture better the core difference between the two.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming is hard for sure...I'd like to avoid renaming the previous one in this PR. It does capture somewhat though the idea that it should mostly be for development - it's mostly about the convenience of just being able to run arbitrary commands to write to the rootfs.

This one though does support that "arbitrary" nature but OTOH I hope its uses will be more constrained in practice. In particular I'd like to try to build upon this and teach ostree how to record in a reliable way (ostree commits?) any changes made in this "underlay". So that would be more of the admin touch point, rather than just "some transient stuff happened".

g_autofree char *devpath_parent = dirname (g_strdup (devpath));

if (!glnx_shutil_mkdir_p_at (AT_FDCWD, devpath_parent, 0755, cancellable, error))
return FALSE;

if (!g_file_set_contents (devpath, "", 0, error))
if (!g_file_set_contents (devpath, unlock_ovldir, -1, error))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was that change on purpose?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, this provides a link between the deployment dir and its unlock dir, so in the future we can e.g. garbage collect them in ostree more accurately and not need systemd-tmpfiles to handle it asynchronously.

fatal "modified /usr"
fi
# But, we can affect it in a new mount namespace
unshare -m -- /bin/sh -c 'mount -o remount,rw /usr && date >'"${testfile}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can check here that the file now exists (and I guess a sanity-check at the top that the file doesn't exist already).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

I was thinking a bit more recently about the "live" changes
stuff coreos/rpm-ostree#639
(particularly since coreos/rpm-ostree#2060 )
and I realized reading the last debates in that issue that
there's really a much simpler solution; do exactly the same
thing we do for `ostree admin unlock`, except mount it read-only
by default.

Then, anything that wants to modify it does the same thing
libostree does for `/sysroot` and `/boot` as of recently; create
a new mount namespace and do the modifications there.

The advantages of this are numerous.  First, we already have
all of the code, it's basically just plumbing through a new
entry in the state enumeration and passing `MS_RDONLY` into
the `mount()` system call.

"live" changes here also naturally don't persist, unlike what
we are currently doing in rpm-ostree.
@jlebon
Copy link
Member

jlebon commented Aug 7, 2020

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cgwalters
Copy link
Member Author

Aug 07 19:20:31 qemu0 kola-runext-itest-payload-link.sh[1255]: Error: unable to pull registry.svc.ci.openshift.org/coreos/fedora:31: unable to pull image: Error initializing source docker://registry.svc.ci.openshift.org/coreos/fedora:31: Error reading manifest 31 in registry.svc.ci.openshift.org/coreos/fedora: error parsing HTTP 400 response body: unexpected end of JSON input: ""

Argh, need to implement coreos/coreos-assembler#1645

@openshift-merge-robot openshift-merge-robot merged commit 012af18 into ostreedev:master Aug 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants