WIP: Start of work on daemon

[petervo]

This pull request is mainly to gather feedback on using a dbus daemon to back the upgrade,
rebase, rollback and status commands.

The daemon ensures that only one OSTree related action that mutates data can run at once. Methods that mutate data return immediately, and fire signals for progress and when complete.

I'd like to get feedback on the general structure and approach as well as of course any defects or best (project?) practice recomendations.

Besides review, there are still quite a few features missing. A few that I know of.

1. The rpm-ostree command needs to reworked to use the dbus api for upgrade,
   rebase, rollback and status.
2. The rpm diff methods need to be implemented. I'm imagining that they
   will return more information, even if the command line output doesn't
   change.
3. Remote management.
4. Safety. I've seen some weirdness when running multiple destructive
   commands on top of each other. I didn't look into it too much but I need
   to look at preventing this in the daemon or the ostree library itself.

[cgwalters]

Related: #44

[mbarnes]

You might want to take a look at how the UDisks2 D-Bus API handles long-running tasks through its Job object and see if it's applicable here. It uses progress properties rather than spamming the bus with signals, and also has a Cancel method.

http://udisks.freedesktop.org/docs/latest/gdbus-org.freedesktop.UDisks2.Job.html#gdbus-interface-org-freedesktop-UDisks2-Job.top_of_page

[petervo]

I did look over that when working on this.

To me signals felt a little more natural and flexible but if you think that a lot of properties would be better I'm open to that. In practice i think the amount of activity on the bus will be similar either way.

The other difference is jobs are a different interface published on a different path, because we really only want one running at any given time, I'm not sure that is as important here. Are there other advantages to doing it that way? Either way cancel could be easily added as a method on a job or the sysroot interface.

[mbarnes]

Having looked over the design a little closer now, yeah maybe the UDisks2 approach is a bit overkill here. Mainly just wanted to put it out there so you're aware of it.

[petervo]

Do you want to weigh in on properties vs signals. I'm good either way. I'll also add a cancel method to sysroot.

[mbarnes]

If I were writing a GObject or GTK+ based client for this interface I would prefer property change notifications over repetitive signals for things like progress updates, mainly for the ability to directly bind GObject properties and not have to write a bunch of trivial signal handlers. For example, you could bind GtkProgressBar::fraction to some equivalent D-Bus property and be done with it. But I'm not sure if that's even applicable here. So I guess no strong opinion at the moment.

Also I'm not entirely clear on what the data dictionary in progress signals might contain, or what clients are supposed to do with it.

[petervo]

That makes sense.

Right now the data dictionary contains the values that were used to generate the status strings that the rpm-ostree command currently outputs to the console. I was trying to both generate the same strings and output the actual values for clients that care. I'll think about what a property version would look like..

[matthiasclasen]

Signals are broadcast to everybody on the bus. That already makes properties preferable, imo.

[cgwalters]

Sorry about the long delay in commenting. This intersects somewhat with #117

Although I would say that we're only doing the daemon for client side operations - server side composes would be a separate thing, right?

The rpm diff operations fall in the middle there - both want them. We should have the daemon use the shared library code.

A major reason I put off the daemon thing for a while is trying to answer the question of: where does PackageKit fit in the future architecture? If PackageKit were to gain rpm-ostree awareness it'd be awkward to have a daemon talking to a daemon (among other things auditing gets lost).

On the other hand, PackageKit is attempting to abstract over package systems which rpm-ostree isn't exactly one. /cc @hughsie

Where did the daemon skeleton code come from btw? It looks like cockpit?

[cgwalters]

Also BTW another reason I put off the daemon question is that it's harder to make Ctrl-C do what one expects...see e.g. moby/moby#3654

[petervo]

Yes the plan is daemon only for client side operations. That would include some rpm diff operations, I put that off but yes using the shared library code sound like the right thing to do..

I had switch to other things for a while there, but I'm almost done re-factoring based on earlier feed back and integrating with the command line commands. I've also added a cancel method so that should make Ctrl-C easier to deal with. I'll push once it's ready.

[petervo]

@cgwalters Going to push an update here later today, using properties instead of signals for tracking pull progress. I'm not sure that's really the best of way doing it though, it makes handling them and parsing into output on the client side a little racy with emission rate limiting and proxy caching potentially effecting things.

@matthiasclasen property changes are emitted as signals on the bus so I'm not sure that really changes things one way or another.

[petervo]

Just updating the PR with the latest code, there is still work to do but this includes the progress tracked by properties changes (I'm not totally sold on it) and a cancel method as well as the start of the client implementation.

The cancel method is dependent on ostree to actually cancel the running task, there is a bit of a delay esp when pulling. Right now I have the client wait for the cancel to actually happen before closing we could change that, or look for ways to make the actual operation cancel faster.

[hughsie]

fwiw, I don't think it makes sense for PackageKit to handle anything other than packages, so I think this proposal is sound, and probably a prerequisite of integration with gnome-software.

[cgwalters]

Ok so on the signals vs properties...honestly it's been too long since I've written DBus APIs =/ I think to a degree this should be driven by whoever's writing the clients.

But re @matthiasclasen - signals definitely can be directed to particular destinations - g_dbus_connection_emit_signal() has a const gchar *destination_bus_name. And even without specific direction, the whole idea behind the AddMatch infrastructure is to avoid every process waking up for every signal.

At a higher level, I think the "agent" pattern has been used in projects like bluez where clients call a "Register" API and then the daemon emits signals to just that client as a destination. The service watches the bus name of the client and cleans up when it goes away.

But I would defer to someone who actively maintains a nontrivial DBus API today for best practices. I think Cockpit is probably one of the most active providers and consumers now, so personally I'd be happy to defer to what stefw thinks.

I know for sure we have things do fix in the ostree/rpm-ostree core like concurrency and improved status output.

[petervo]

For something like cockpit where you'd want to show the system status regardless of who actually triggered the command you would always want all the output and wouldn't want signals to only be targeted at whoever triggered the command. Of course when running a command from the command line you don't want to see any output from other possibly running commands, but since we are guaranteed with this API to only have one job running and emitting updates at any given time, this client can check if it's command was triggered successfully and if so display any output. This is what the code in this PR does.

All of this applies with both signals and properties, the main advantage of signals as i see is ordering and the main disadvantage is possibly more boilerplate type code required in the client.

[stefwalter]

The problem with properties is that they can be cached and various change notifications can be squashed. If the plan is to transfer progress, and require that each iteration of that progress hit the screen (such as line output) then signals are a clear win here.

In realmd we emit "Diagnostic" signals targetted at the caller of a given method. When the caller wants to show diagnostic progress style output, they subscribe to those signals.

We also have a nice cancellation model, where the caller passes in an optional cancellation/operation id to the various methods. Not only can any operation be cancelled via a global Cancel() DBus method, but the "Diagnostic" signal also includes this cancellation/operation id as its first argument. In the case where a caller has multiple operations going on, it's trivial to subscribe with 'arg0=xxx' support (or just filter the signals themselves). But this may not be relevant here.

In summary, If displaying each item of progress (such as log lines) is a requirement, I would posit that properties here would be a poor choice.

That said, in Cockpit we have worked around all sorts of nasty DBus API issues, and I don't think this is a blocker either way.

[cgwalters]

Getting this merged will be a high priority for the next cycle. Can we do two things:

• Move the branch under the primary repository so that it's easier for multiple people to collaborate on it
• Break it up as much as possible into separate commits; e.g. if there are preparatory cleanups let's try to land those first

[cgwalters]

I'm not entirely sure about moving things like diffs to be DBus API. Conceptually they're read-only. It would mean it'd be possible to get a crash if one was doing a background update and typing rpm-ostree db diff at these same time...but I'm not really sure how locking could work even if we had a daemon.

We could certainly simplify things significantly if only write operations went through the daemon - this would be similar to what dconf does.

I'm also a bit uncertain about generic sysroot support in the daemon. The main user of a non-default sysroot today is Anaconda - but using it this way would mean having the daemon run in the installer. I had rather liked how the sysroot support worked out for Anaconda without a daemon in the middle. But OTOH as long as the command line keeps working it doesn't need to use the daemon...

None of the above is saying anything must change...I'm just thinking out loud.

[stefwalter]

How would clients like Cockpit perform read operations (such as diff), if not via an API? Or are you thinking that there are two APIs one for reading and another for writing? Is there a good reason for that complexity?

[petervo]

I think being able to get diffs from the api is fairly important for clients to be able to see what would be changed. We don't have to support arbitrary db diff operations though. Can we be sure that the boot root won't change even with updates? If so we can have the dbus api only compare heads at a tmp location similar to the way that check diff works now. Or is that still going to have issues? We also can take the hammer approach make the diff calls acquire the lock making sure no updates are running until they are done.

Having the command line not use the daemon reintroduces the potential for unsafe concurrent operations. Of course even with the daemon someone can do that (with a little more difficulty) using ostree directly.

[cgwalters]

For diffs and other read-only operations (e.g. rpm -qa), I was thinking cockpit would link to librpmostree. Would that be a problem?

[petervo]

One of cockpits goals at the moment is to do as much as possible in the browser with as little server side 'glue' code as possible. So while linking is a possibility it does kind of go against the direction the project is headed. See the whole 'expect' discussion from a while back.

Wouldn't we still have the same concurrency issues though even if using the linked library.

[stefwalter]

For diffs and other read-only operations (e.g. rpm -qa), I was thinking cockpit would link to librpmostree. Would that be a problem?

Cockpit runs as javascript in a web browser. We can't link to a C binary API. What we would need to do is wrap it in a DBus API or REST API that we can call from from our javascript. This is exactly the work @petervo is doing.

[cgwalters]

Executive summary:

• Cockpit needs rich APIs like diffs that are remotable
• Colin has concerns about amount of data
• We could likely special case the local client case and optimize (pass read-only file descriptor for rpmdb or so)

    <petervo> walters, i could be missing something but from what i'm seeing the rpm specific information, change logs and from repos isn't represented and can't be gotten from RpmOstreePackage api
    <petervo> but maybe i'm looking at the wrong branch?
    <walters> petervo, currently...
    <petervo> good to know that's the plan, I guess we need to sort out if we want the dbus api to support this anyways before i go too much further down this path
    <stefw> walters, has there been a misunderstanding in what petervo is working on?
    <stefw> not sure how we would load a C library into a web browser. wrapping it into an API we can call is what this is all about.
    <stefw> in addition solving concurrency
    <walters> i'm just uncertain about having rpm-ostree expose everything via dbus in addition to being a shared library/commandline
    <stefw> alright, i guess we should wait on this work until we can figure this out properly then
    <stefw> because we can't load a shared library into the web browser
    <stefw> we need an API, DBus, REST, or socket based (ideally text on the socket)
    <stefw> or a command line API, that is machine parseable
    <stefw> or something like that
    <stefw> petervo, ^^\
    <stefw> :S
    <walters> i think everyone (hopefully) agrees there should be a daemon, so i don't see this as blocked exactly
    <stefw> yeah, but the reason petervo is writing it, is so that there can be an API that we can call
    <walters> s/API/remotable API/
    <stefw> yes, a remotable API
    <stefw> things like satellite are going to want a remotable API too
    <stefw> when you're working at the level of system management and configuration
    <stefw> C APIs aren't
    <stefw> they're implementation details
    <stefw> look at systemd, sure, it has C APIs
    <stefw> but it's real configuration, management and control apis are all remotable DBus APIs
    <stefw> look at Docker
    <stefw> if it had exposed a C API
    <stefw> ... there wouldn't be a docker eco-system
    <stefw> if it had exposed a C API instead of the REST API that it exposes ... there wouldn't be a docker eco-system
    <stefw> anyway, if the daemon is just an implementation detail, and we're left to screenscrape the command line ... then i guess that's what we have to do
    <stefw> hell we manage to screenscrape passwd
    <stefw> but it's unfortunate to design a new system like that
    <stefw> anyway, i'm sure there's lots to think about and consider here.
    <walters> i understand that...but remoting all the rpm data through dbus 1) feels ugly 2) has been done before in PackageKit
    <stefw> real world apis feel ugly
    <stefw> such is life
    <stefw> but if you feel you need to give this more consideration ... then please do
    <walters> an important aspect here is that at least ostree is happy to run as non-root
    <stefw> this is not about ostree right, this is about rpmostree
    <walters> basically backing scripts
    <stefw> anyway, you can also choose to have a machine readable command line API
    <stefw> like git
    <stefw> all very good possibilities
    <stefw> if you would prefer to write something like that ... once it's ready we can write a ui for it
    <stefw> DBus really is the best way to write an API though
    <walters> remotable API
    <stefw> like i said
    <stefw> at the level we're working at, all APIs are remotable
    <stefw> ones that aren't really are OS internal building blocks
    <stefw> you can't use a C API from a privileged container
    <walters> i'm not so sure, you can run `rpm -qa` as an unprivileged user
    <stefw> that's not a C API
    <walters> similarly, `rpm-ostree db diff` works as non-root today
    <stefw> that's a command line API
    <stefw> which *is* remotable
    <walters> rpm *does* have a C API
    <stefw> yeah, and it's an implementation detail
    <stefw> when it comes to configuration and management software
    <stefw> you can't load a C library from the host, and link to it from a process running in a container
    <stefw> not to mention a browser, or another host
    <stefw> anyway, one can call anything an API, including internal C functions and processor registers
    <stefw> i guess what we're talking about is APIs consumable by software not running in the tree
    <stefw> not running in the atomic host
    <stefw> so yes, remotable
    <stefw> given that atomic is completely locked down to adding software, this is *vital*
    <walters> disregarding the remotable aspect for a moment, certainly librpmostree could be installed into a container
    <stefw> Atomic Host APIs that are not remotable are irrelevant to software not running as part of the host
    <stefw> then it's essentially other software operating on the same data
    <walters> (and it's definitely possible to link to host libraries...there are versioning concerns etc., but those also apply to a degree to non-containerized software too)
    <stefw> yeah, except for there are decades of tooling to help with this in a normal machine environment
    <stefw> in atomic host, there's nothing
    <stefw> anyway, for cockpit it's irrelevant anyway
    <stefw> we can't load librpmostree into the browser
    <stefw> we communicate with remotable APIs from the browser
    <stefw> the cockpit-bridge is just that, a bridge for letting the browser talk to remotable APIs
    <stefw> that includes files in the file system, sockets, http/rest apis, dbus apis, spawning processes
    <stefw> there's a few corners where the lines get blurry, and we call getpwnam or something like that
    <stefw> but that's it
    <stefw> by the way, satellite will be in a similar situation ...
    <stefw> i gotta run, but will be back later
    <stefw> we should setup a brainstorming session about this
    <stefw> so we can see if we can get the work on a UI back on track, and figure out how it'll happen
    <walters> ok let me circle back again a bit, i'm not arguing against a daemon...maybe the commandline client can avoid schlepping the entire rpm database over dbus if it's local only or something
    <stefw> yeah, makes sense ... in addition it seems that certain operations could happen directly
    <stefw> asking the daemon to lock
    <stefw> or something like that
    <stefw> such as compose
    <stefw> or db diff
    <stefw> things that aren't exposed in the remotable API
    <walters> this "data" aspect seems to be a substantial difference from other components managed via cockpit
    <walters> true?
    <stefw> i didn't understand that
    <stefw> we certainly would never load an entire rpm database into cockpit
    <walters> amount of data exposed via the management api
    <stefw> we would want teh tresults of an rpm diff
    <stefw> and not perform it in the browser
    <stefw> we would expect the remotable API to perform that sorta processing, before handing off the results to the caller
    <walters> the current xz compressed Fedora Packages XML is 40MB
    <stefw> why would the caller of the API want that?
    <stefw> petervo, ^^
    <stefw> we would never want to load that
    <walters> are there any plans for cockpit/PackageKit integration?
    <stefw> yes, thetre are discussions, but until it's usable on the server, they're on ice
    <stefw> petervo knows more
    <walters> ok
    <stefw> so that's why we've been focusing on the rpmostree stuff
    <petervo> the plan was never to load all the packages, just get the diffs
    <stefw> because it's a solid use case
    <stefw> anyway ... gotta run ... good discussion ... i'll be back later
    <stefw> feel free to continue without me :)
    <walters> stefw, mind if i paste this back to the github?
    <stefw> sure
    <walters> cool, thanks
    <petervo> packagekit isn't ready for server side atm, too many dependencies and unlike this it's not always clear what needs to be done after an upgrade

[mbarnes]

I don't recall if this was discussed in our meeting, but this method and the allowance for multiple Sysroot interfaces seems like overkill to me at this point. Is there a use case for this method now?

[petervo]

I was trying to keep parity with the command line, if the command line is going to use the dbus api to run it's commands and continue to support that option then this needs to support it as well.

On the other hand, we could drop that option from the command line for the sub commands that use dbus or decide that if that option is provided it talks to libostree directly, though i wouldn't be thrilled about having 2 code paths. I'm happy to defer to whatever you think is better.

As a side note, i've been using this to run rpm-ostree from a SPC, but that's neither here nor there for what we want long term.

[mbarnes]

Apart from a couple comments above, the initial D-Bus interfaces look okay to me. Now that I understand the requirements better, I agree that signals are more appropriate than individual properties for progress reporting.

It's generally considered good practice to version your bus name and interfaces in case you have to change or extend them in the future. e.g. org.projectatomic.rpmostree1 But if these interface are only to be consumed by Cockpit in an atomic host environment then maybe not necessary. Just mentioning it.

[petervo]

Rebased again since @cgwalters asked to have the cli not get diffs directly instead of use dbus. Also added the monitor changes. Locking changes still need to be worked out, and as a result of that we need to add someway to track jobs with signals I'm thinking a job ID on the signals, though there are other options, @cgwalters lmk if you want me to go with that or discuss further.

[cgwalters]

Argh, sorry about that. ostreedev/ostree#113 should fix the locking there.

[cgwalters]

Ok, let's try to avoid too many more stalls and get this merged today. But I'd still like to figure out if we can have the flexibility to evolve the API going forward.

[mbarnes]

I'm 👍 overall on the branch, fwiw. Been refraining from nit picking too much since it would just prolong this further. All parties seem to be in agreement on the D-Bus API - the rest is just implementation details that can be fixed as needed in master.

[petervo]

There are at least 3 issues here that probably should be resolved before this makes it in.

• diff output cleanup
• less racy daemon shutdown
• ostree locking

I have ostree locking done on my machine, however it breaks both client implementations i currently have as there is no longer a way for a client to tell if signals it is receiving were triggered by it's call or not. So we either need job ids or change the current flow. I haven't started on the other two, I'm hoping to spend some time on it this afternoon.

[cgwalters]

I would say let's not try to have the daemon shut down if it's racy, in this first cut. (I'm wondering if we can make this better if we avoid the message bus activation and have the CLI consistently use a private bus connection like systemd does (and NetworkManager supports)? But this can hopefully come later.)

Regarding locking and client IDs...I'll look at this.

[cgwalters]

https://github.com/projectatomic/rpm-ostree/tree/daemon is a new branch based on some discussion Peter, Matthew and I had.

What I pushed so far is a "stub daemon" that compiles, and a new proposed API XML.

The major points from the discussion I recall were:

• Have a Transaction object like PackageKit, with at most one running at a time
• Move to having high level API methods that match the CLI such as Upgrade and Rollback, rather than having each client reimplement the lower level logic. We think we can do this will still allowing clients like Cockpit to have fine-grained details
• Matthew noticed a major simplification by having the "OSName" as an object type

[petervo]

We need a way to get checksum, version, time stamp and signature data for each commit and any upgrade targets. Propose adding GetDeploymentDetails and GetCachedUpdateDetails as methods on the Os interface.

[petervo]

@cgwalters @mbarnes is there any segment of this safe for me to work on without duplicating effort?

[cgwalters]

Agree adding GetDeploymentDetails() GetCachedUpdateDetails.

Please go ahead and just push stuff!

[mbarnes]

Do we need to know what method a transaction object is executing?

Thinking a Method string that's named after the method that started the transaction.

<interface name="org.projectatomic.rpmostree1.Transaction">
  ...
  <!-- Upgrade / Rollback / etc. -->
  <property name="Method" type="s" access="read"/>
  ...
</interface>

[mbarnes]

Just as a smoke test, I ran rpm-ostreed as a normal user and witnessed it silently fail to acquire its system bus name and just sit there.

Would you guys mind if I rework this to handle bus name acquisition in main.c instead of daemon.c, and terminate when it loses its bus name? I have some code I wrote for https://github.com/fleet-commander that I can just drop in here.

[cgwalters]

No objections...and it would likely be really worth making it easy to run the daemon as a regular user against a non-root sysroot. That's how the ostree tests work, and while it's not perfect, you get a lot of test coverage that way.

[petervo]

Non root users should run with peering instead of acquiring a bus name. Having the daemon terminate if it fails to ever acquire the name i think is fine, having it terminate immediately if it loses it might not be so good.

[mbarnes]

We could give it a use count. +1 while holding the bus name, +1 while a transaction is in progress, terminate when the count drops to 0.

[mbarnes]

I wrote a little factory function for transaction objects, hoping it proves useful.
06e3dca

[mbarnes]

I pushed a half-finished Upgrade() method to the branch just so I'm not doing all this in secret.

The CLI layer is getting rather ugly. The commit message has some discussion points.

690a4a3

[petervo]

Can non / sysroots can use peering mode with the daemon. That's how we talked about doing it before, is there a reason not to do that?

For check-diff on the client side can't we do DownloadUpdateRpmDiff. Then if we want to parse and output the diffs directly we can, but that way clients just observing are kept up to date with what is going on and there is less code duplication.

[mbarnes]

My understanding was peering mode was primarily for daemon testing. I guess the rpm-ostree CLI could be made to use peering mode -- although I'm not sure what the use case is, and I'm also not clear on how activation would work. Seems there's some pieces missing. Do we need a --dbus-peer option on the client end?

[petervo]

As i understood it peering mode was initially planned for anaconda or really any time you wanted to use a non standard sysroot. The cli shouldn't really use it on the standard sysroot as that would negate the benefits of having a shared daemon.

Yes, the old code had the peer option, see: https://github.com/petervo/rpm-ostree/blob/wip-daemon/src/app/rpmostree-builtin-upgrade.c#L47 and https://github.com/petervo/rpm-ostree/blob/wip-daemon/src/app/rpmostree-dbus-helpers.c#L119

[petervo]

Replaced by #157