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 new ex initramfs-etc command #2170

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jul 24, 2020

This command allows users to cheaply inject configuration files in the
initramfs stage without having to regenerate the whole initramfs (or
even a new OSTree commit). This will be useful for configuring services
involved in bringing up the root block device.

$ echo 'hello world' > /etc/foobar
$ rpm-ostree ex initramfs-etc --track /etc/foobar
Staging deployment... done
Run "systemctl reboot" to start a reboot
$ rpm-ostree status
State: idle
Deployments:
  ostree://fedora:fedora/x86_64/coreos/testing-devel
                   Version: 32.20200716.dev.1 (2020-07-16T02:47:29Z)
                    Commit: 9a817d75bef81b955179be6e602d1e6ae350645b6323231a62ba2ee6e5b9644b
              GPGSignature: (unsigned)
              InitramfsEtc: /etc/foobar
                  Unlocked: development

● ostree://fedora:fedora/x86_64/coreos/testing-devel
                   Version: 32.20200716.dev.1 (2020-07-16T02:47:29Z)
                    Commit: 9a817d75bef81b955179be6e602d1e6ae350645b6323231a62ba2ee6e5b9644b
              GPGSignature: (unsigned)
$ reboot
(boot into rd.break)
sh-5.0# cat /etc/foobar
hello world

See the libostree side of this at:
ostreedev/ostree#2155

Lots more discussions in:
coreos/fedora-coreos-tracker#94

@openshift-ci-robot
Copy link
Collaborator

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
Copy link
Member Author

jlebon commented Jul 24, 2020

Requires: ostreedev/ostree#2155

Still need to write tests for this.

@cgwalters
Copy link
Member

How often do we expect this stuff to change? Following up to ostreedev/ostree#2155 (comment) - what if all we did is support multiple initrds. Then the CoreOS code might generate a secondary initramfs (in the initramfs!) and install it as /boot/coreos-rootmap-initramfs.img or so, and update the bootloader configuration to use it...and that's it.

The experience for the administrator of "I want to update my rootmap" is certainly uglier but then it's clear - the "source of truth" is that file in /boot and we don't need any complex infrastructure to copy bits from /etc into images.

@jlebon
Copy link
Member Author

jlebon commented Aug 13, 2020

Rebased!

Going to reply to this comment and ostreedev/ostree#2155 (comment) here.

Let's approach this from the other side. I.e. what do we want the UX to look like?

The major goals I'm trying to meet are the following:

  • Built into rpm-ostree: it should feel native to rpm-ostree; I think this command should replace rpm-ostree initramfs --enable whenever possible. Similarly, rpm-ostree status should show meaningful information about the state of the overlay.
  • The source of truth lives in /etc: users don't have to think about anything else, they can just edit the same files in /etc and it'll get updated on the next deployment. This is how rpm-ostree initramfs works as well. I think there might be use cases for having different configs in the real root and in the initrd, but we can extend the feature for that when we encounter it.
  • Generic: while the primary purpose is indeed for root device configuration, it shouldn't be designed with just that in mind. Configuring systemd and journald are good examples.
  • Rollback: config changes made to the overlay should be bound to a specific deployment so that users can rollback just as they would with rpm-ostree initramfs --enable. This argues for making it part of the bootcsum directory itself in /boot. (Edit: ended doing something a bit smarter here so we don't invalidate the bootcsum; see lib/deploy: Add support for overlay initrds  ostreedev/ostree#2155).

Do you agree with the above?

Implementation-wise, I agree with you generating a new commit isn't ideal. Actually, my first approach at this was to extend the staging API to take additional initrds, so then e.g. rpm-ostree would create the initrd somewhere in /run and pass that to ostree_sysroot_{deploy,stage}_tree_with_options. This works, though the disconnect between the bits which handle staging and the bits which actually copy files into /boot made it a non-trivial patch in comparison to dffc850 (#2155). But maybe it's worth it in the end so we avoid having to create a new commit.

Re. the overlay not changing often, yeah it's tempting to try to make optimizations on that assumption. OTOH, the sizes we're talking about are minuscule in comparison to the initrd itself. One thought I had on this topic was it'd be interesting to have another OSTree repo in /boot so that we can share kernels and initrds across bootcsums.

I think if we're in agreement on the wanted UX, we could land this (once we add tests) and iterate on the implementation? Though there's no real rush either. I'll brush up the alternative libostree patch I had which will avoid having to generate a new commit.

@jlebon
Copy link
Member Author

jlebon commented Aug 13, 2020

Updated as per ostreedev/ostree#2155 (comment)! Now with 100% less OSTree commit operations. (You can see in the updated sample output in #2170 (comment) that we just go straight to staging the deployment.)

@cgwalters
Copy link
Member

We aren't doing change detection, right? IOW:

$ rpm-ostree ex initramfs --track /etc/systemd.conf
$ vi /etc/systemd.conf
...
$ rpm-ostree upgrade
No upgrade available.

And when I reboot, I'll have the old config until at some point something else changed, right?

I'd be OK if we added rpm-ostree ex initramfs update or something...but, a root problem here is having two copies of the files. What if instead the interface was:

$ cd $(mktemp -d)
$ cp /etc/systemd.conf .
$ vi systemd.conf
$ rpm-ostree ex initramfs --add .

Which would be a really thin wrapper around the libostree API (in fact, we should probably expose that via ostree admin deploy initramfs --add or so).

I am still skeptical that these initramfs overlays will need to be updated often; if we discover later that we need to make it more ergonomic to do so, we could add rpm-ostree ex initramfs --extract which would extract the initramfs into a temporary directory (but, one could also just use a small shell script to do so), then rerun -add.

(Implicit a bit in this is the idea there's at most one overlay, which seems fine to me; supporting multiple makes everything messy)

}

static gboolean
generate_initramfs_overlay (GHashTable *initramfs_etc_files,
Copy link
Member

Choose a reason for hiding this comment

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

Just a note/optional: Feels like we could move some of this into Rust.

@jlebon
Copy link
Member Author

jlebon commented Aug 18, 2020

We aren't doing change detection, right?

Right, though that's a bug IMO. With dracut that's hard to do because we have no insight in what files get picked up. But for initramfs-etc we could create the cpio and compare checksums. (This PR doesn't do that yet... but I'll add it).

I am still skeptical that these initramfs overlays will need to be updated often

I don't disagree that in the common case they wouldn't be updated often. The new model in libostree reflects that (by not making it part of the bootcsum).

What if instead the interface was:

$ cd $(mktemp -d)
$ cp /etc/systemd.conf .
$ vi systemd.conf
$ rpm-ostree ex initramfs --add .

Hmm, perhaps. Though just directly pointing rpm-ostree at the files in /etc is a much nicer UX IMO. So with the change detection tweak mentioned above, you'd just have to edit /etc/systemd.conf then rpm-ostree initramfs-etc --sync (or just rpm-ostree upgrade). How does that sound?

@lucab
Copy link
Contributor

lucab commented Sep 17, 2020

One personal UX/design doubt on this point:

The source of truth lives in /etc: users don't have to think about anything else, they can just edit the same files in /etc and it'll get updated on the next deployment. This is how rpm-ostree initramfs works as well. I think there might be use cases for having different configs in the real root and in the initrd, but we can extend the feature for that when we encounter it.

I feel that the rest of the ecosystem is going in a direction where part of the source of truth (user overrides) lives in /etc. Those may not make sense taken in isolation without vendor defaults, i.e. the fragments in /usr.
Are we making the assumption that there is a symmetry in /usr between rootfs and initramfs? If so, is that assumption generally true in the current dracut+rpm world?

@jlebon
Copy link
Member Author

jlebon commented Sep 28, 2020

Are we making the assumption that there is a symmetry in /usr between rootfs and initramfs? If so, is that assumption generally true in the current dracut+rpm world?

I think if an app wants to rely on its /usr configs in the initrd, the appropriate dracut module should already be pulling it in (as well as the /etc config really). IOW, this isn't unique to the initramfs-etc workflow, or even to OSTree-based systems (it's equally relevant on other dracut-based OSes, even if the initrd happens to be built client-side).

jlebon added a commit to jlebon/ostree that referenced this pull request Oct 6, 2020
This is a basic `.packit.yaml` integration file which will allow us have
continuous builds of OSTree in cosa and upstream CI. If things go well,
we'll likely deploy this in other build tools like rpm-ostree.

Prompted by wanting to get ostreedev#2155 out to unblock
coreos/rpm-ostree#2170.
jlebon added a commit to jlebon/ostree that referenced this pull request Oct 6, 2020
This is a basic `.packit.yaml` integration file which will allow us have
continuous builds of OSTree in cosa and upstream CI. If things go well,
we'll likely deploy this in other build tools like rpm-ostree.

Prompted by wanting to get ostreedev#2155 out to unblock
coreos/rpm-ostree#2170.
jlebon added a commit to jlebon/fedora-coreos-config that referenced this pull request Oct 14, 2020
Get this in now so we get some early testing before releases next week.
Also because it unblocks coreos/rpm-ostree#2170.
@jlebon
Copy link
Member Author

jlebon commented Oct 19, 2020

Rebased this and restarted CI on it. Keeping as draft for now until I add tests, but the rest of the code should be ready for review!

@jlebon
Copy link
Member Author

jlebon commented Oct 21, 2020

Now ready for review and with a test!

@travier
Copy link
Member

travier commented Oct 22, 2020

It looks like this would fix the most common case for #1930.

This command allows users to cheaply inject configuration files in the
initramfs stage without having to regenerate the whole initramfs (or
even a new OSTree commit). This will be useful for configuring services
involved in bringing up the root block device.

```
$ echo 'hello world' > /etc/foobar
$ rpm-ostree ex initramfs-etc --track /etc/foobar
Staging deployment... done
Run "systemctl reboot" to start a reboot
$ rpm-ostree status
State: idle
Deployments:
  ostree://fedora:fedora/x86_64/coreos/testing-devel
                   Version: 32.20200716.dev.1 (2020-07-16T02:47:29Z)
                    Commit: 9a817d75bef81b955179be6e602d1e6ae350645b6323231a62ba2ee6e5b9644b
              GPGSignature: (unsigned)
              InitramfsEtc: /etc/foobar

● ostree://fedora:fedora/x86_64/coreos/testing-devel
                   Version: 32.20200716.dev.1 (2020-07-16T02:47:29Z)
                    Commit: 9a817d75bef81b955179be6e602d1e6ae350645b6323231a62ba2ee6e5b9644b
              GPGSignature: (unsigned)
$ reboot
(boot into rd.break)
sh-5.0# cat /etc/foobar
hello world
```

See the libostree side of this at:
ostreedev/ostree#2155

Lots more discussions in:
coreos/fedora-coreos-tracker#94

Closes: coreos#1930
@jlebon
Copy link
Member Author

jlebon commented Oct 23, 2020

Rebased on top of #2278!

It looks like this would fix the most common base for #1930.

Good catch, thanks! I added a Closes: #1930 footer to the commit message.

@jlebon
Copy link
Member Author

jlebon commented Oct 23, 2020

✔️ continuous-integration/jenkins/pr-merge — This commit looks good

🎉

@cgwalters
Copy link
Member

Two high level questions:

  1. Is the idea that this will be ex to allow us to work on using it in FCOS for complex roots, and then we lift ex once that seems to be working well, but before we stabilize the FCOS usage of it?

  2. I still don't much like the --force-sync thing - but presumably in most cases (as noted above) this will be for things that should very rarely change and so we don't need to worry about it?

@jlebon
Copy link
Member Author

jlebon commented Oct 28, 2020

  1. Is the idea that this will be ex to allow us to work on using it in FCOS for complex roots, and then we lift ex once that seems to be working well, but before we stabilize the FCOS usage of it?

Yes, exactly. We can refer users who need it to the new command and gather feedback there. Just playing it safe in case the UX shows clunkiness when used in more realistic situations.

  1. I still don't much like the --force-sync thing - but presumably in most cases (as noted above) this will be for things that should very rarely change and so we don't need to worry about it?

Yeah, it's not great, but I think it fills a UX need. Let's say you use rpm-ostree ex initramfs-etc --track /etc/foobar.conf, reboot, and then realize you need to make a change to foobar.conf (I can imagine someone iterating on this multiple times as they try different versions of the config to test/debug something).

You'd have to do some artificial operation just to create a deployment (e.g. doing rpm-ostree deploy for the same version you're on). Having a clearly labeled switch for this feels much more natural I think. (And on that note, rpm-ostree initramfs could also use a switch like this. I can add that to make them more similar... in which case, maybe the switch should just be called --force?).


GInputStream *cpio_output = g_subprocess_get_stdout_pipe (cpio);
GOutputStream *gzip_input = g_subprocess_get_stdin_pipe (gzip);
g_output_stream_splice_async (gzip_input, cpio_output,
Copy link
Member

@cgwalters cgwalters Oct 28, 2020

Choose a reason for hiding this comment

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

It'd be more efficient and simpler to just pass cpio's stdout as gzip's stdin, right?


const char *cpio_argv[] = {"cpio", "--create", "--format", "newc", "--quiet",
"--reproducible", "--null", NULL};
g_autoptr(GSubprocess) cpio = g_subprocess_newv ((const char *const*)cpio_argv,
Copy link
Member

Choose a reason for hiding this comment

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

https://developer.gnome.org/gio/stable/GSubprocess.html#g-subprocess-communicate also takes stdin as a buffer, so we wouldn't need to splice anything in combination with the below.

}

static gboolean
initramfs_etc_transaction_execute (RpmostreedTransaction *transaction,
Copy link
Member

Choose a reason for hiding this comment

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

I thought we'd been trending a bit towards glomming everything into Deploy so it could be done in one go? I guess it doesn't much matter for this case, just curious if you had a reason to make this separate. (It is clearer to read separately, but more duplicated code)

@cgwalters
Copy link
Member

OK I restarted CI on this and am OK merging as is, since it's ex we can fix things like the above in followups!

@cgwalters
Copy link
Member

/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

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.

None yet

6 participants