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

daemon: Remove support for session bus #2854

Merged
merged 2 commits into from
May 31, 2021

Conversation

cgwalters
Copy link
Member

We haven't actually tested this in a long time, it's just
cruft now. Bigger picture we need to make it as ergonomic and
fast as possible to test in VMs.

(I think it likely would be worthwhile at some point having rpm-ostree run in a "stock
podman container" w/o systemd but that's a whole lot of work.)

Prep for some work on socket activation.

We haven't actually tested this in a long time, it's just
cruft now.  Bigger picture we need to make it as ergonomic and
fast as possible to test in VMs.

(I think it likely would be worthwhile at some point having rpm-ostree run in a "stock
podman container" w/o systemd but that's a whole lot of work.)

Prep for some work on socket activation.
@cgwalters cgwalters force-pushed the remove-peer-activation branch from 0a53e35 to 9b6bb0c Compare May 25, 2021 17:23
@cgwalters
Copy link
Member Author

OK it turned out to be not too bad to follow this thread even farther, so I pushed another commit here.

@@ -61,7 +61,7 @@ rpmostree_db_option_context_parse (GOptionContext *context,
argc, argv,
invocation,
cancellable,
NULL, NULL, NULL, NULL,
Copy link
Member Author

Choose a reason for hiding this comment

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

This NULL NULL NULL NULL thing screams bad API design too, so it's good to reduce it!

@cgwalters cgwalters force-pushed the remove-peer-activation branch from 9b6bb0c to 36237b9 Compare May 25, 2021 17:32
@cgwalters
Copy link
Member Author

/retest

lucab
lucab previously approved these changes May 27, 2021
src/app/rpmostree-builtin-status.cxx Outdated Show resolved Hide resolved
src/app/rpmostree-builtin-upgrade.cxx Outdated Show resolved Hide resolved
static GOptionEntry opt_entries[] =
{
{ "debug", 'd', 0, G_OPTION_ARG_NONE, &opt_debug, "Print debug information on stderr", NULL },
{ "sysroot", 0, 0, G_OPTION_ARG_STRING, &opt_sysroot, "Use system root SYSROOT (default: /)", "SYSROOT" },
Copy link
Member

Choose a reason for hiding this comment

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

Can we leave this one in? It's related to the peer D-Bus stuff, but still mostly independent, right?

There's a bunch of things related to this I'd like to explore. E.g. running rpm-ostree directly after coreos-installer install, running rpm-ostree from the initramfs, etc... I think also Anaconda may need this for the initramfs-etc work.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, this actually works today from a live boot:

[root@cosa-devsh ~]# coreos-installer install /dev/vda
[root@cosa-devsh ~]# mount /dev/disk/by-label/root /mnt/root
[root@cosa-devsh ~]# mount /dev/disk/by-label/boot /mnt/root/boot
[root@cosa-devsh ~]# rpm-ostree start-daemon --sysroot /mnt/root &
[root@cosa-devsh ~]# rpm-ostree status
State: idle
Deployments:
* ostree://fedora:fedora/x86_64/coreos/testing-devel
                   Version: 34.20210520.dev.0 (2021-05-20T21:45:14Z)
                    Commit: a53b7d233ef46c2e1ceb469c0ec8e441a5d67c03c6641630632e89c52f45a516
              GPGSignature: (unsigned)

We'd want to make that easier of course. Maybe just have coreos-installer automatically add a drop-in for rpm-ostreed.service or something. Not sure.

Actually doing a write operation like rpm-ostree ex initramfs-etc on it fails, but at least the base is there.

Copy link
Member Author

Choose a reason for hiding this comment

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

E.g. running rpm-ostree directly after coreos-installer install,

But the more we do that the more it weakens the Ignition story/focus.

running rpm-ostree from the initramfs, etc...

I am very skeptical of this one because any nontrivial use will need to configure rpm-ostree (think e.g. 3rd party repos or local mirroring) and then that gets into configuring the initramfs and...

I think most of the desire for rpm-ostree-in-initramfs has been obviated by the install -A model.

Also we don't have tests for --sysroot now. It might not be really hard with some scaffolding to to basically double our tests and do it though. Or I guess have some basic tests.

Copy link
Member

Choose a reason for hiding this comment

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

E.g. running rpm-ostree directly after coreos-installer install,

But the more we do that the more it weakens the Ignition story/focus.

Oh yes definitely. But I'm hoping to converge towards rpm-ostree ex initramfs-etc for solving bootstrapping problems which hard require pre-configuration. Notably networking and non-default multipath configuration.

running rpm-ostree from the initramfs, etc...

I am very skeptical of this one because any nontrivial use will need to configure rpm-ostree (think e.g. 3rd party repos or local mirroring) and then that gets into configuring the initramfs and...

I think most of the desire for rpm-ostree-in-initramfs has been obviated by the install -A model.

I haven't thought this through yet (and I've actually been meaning to experiment with this during a hack day), but as part of coreos/fedora-coreos-tracker#681, basically I was thinking we would match Ignition semantics and apply layered requests before we switchroot. I think this would be cleaner just doing it in a bwrap container though to avoid actually having to drag rpm-ostree into the initrd. So maybe we don't actually need --sysroot in that case. But yeah, configuration would definitely come from the real root.

The advantage over live-apply in the real root is that it removes any ambiguity wrt ordering and e.g. configuring the layered things via Ignition itself. It also allows us to enforce strict failure semantics like Ignition.

Also we don't have tests for --sysroot now. It might not be really hard with some scaffolding to to basically double our tests and do it though. Or I guess have some basic tests.

Yeah, I think some basic tests at least wouldn't be too hard. I can volunteer for that! :)

Copy link
Member Author

Choose a reason for hiding this comment

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

OK for now, I kept sysroot since it is orthogonal to removing the session bus bits.

That said, on this topic...my thought here is that we can avoid using DBus for these cases. Basically rpm-ostree ex initramfs-etc --sysroot=/mnt/sysroot ... would directly perform the operation in the client code.

It's much like where systemd ended up where e.g. systemctl can talk to pid1 over dbus, but also happily works offline directly.

Copy link
Member

Choose a reason for hiding this comment

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

That said, on this topic...my thought here is that we can avoid using DBus for these cases. Basically rpm-ostree ex initramfs-etc --sysroot=/mnt/sysroot ... would directly perform the operation in the client code.

I think that makes sense though likely will require a lot of refactoring to pull off. Definitely worth investigating though!

Supporting the session bus was a nice idea but we don't
have any tests for this anymore, and carrying it is
nontrivial overhead.  It used to only kind of work because
ostree ran well as non-root, but rpm-ostreed really requires
root nowadays.
@cgwalters cgwalters force-pushed the remove-peer-activation branch from 45c2ca8 to 4eb55ea Compare May 29, 2021 12:39
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.

Nice!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants