(admi) API inconsistency start/shutdown, can we still fix this?

[Tom Zander]

The admin API has two commands that I'd really like to be consistent.

> admin.vm.Start.
This one waits until the VM is fully started.

> admin.vm.Shutdown
This one returns immediately.

This inconsistency is rather annoying and leaves me to connect to the
event-feed to get a final "it shut down" information.
I would like them both to wait until finished.
I would argue that the inconsistency is a bug.


Additionally, shutting down is certainly possible to fail (or, timeout), which would call for a 'kill'.
I forsee having to write a lot more code to handle that case with the current API.
Ideally "Shutdown" would not return immediately, but would return either on
a) completion (success)
b) timeout or failure (as an exception).

So, my question is if this can still be made consistent, which would make my app using the API simpler.

Second question; what is the general idea around backwards compatibility of the APIs?

ps. I started documenting the API here;

https://github.com/QubesController/qubes-api-cpp-lib/blob/master/Qubes.cpp#L29
--
Tom Zander
Blog: https://zander.github.io
Vlog: https://vimeo.com/channels/tomscryptochannel

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Mon, Jan 22, 2018 at 03:52:18PM +0100, 'Tom Zander' via qubes-devel wrote:
> The admin API has two commands that I'd really like to be consistent.
>
> > admin.vm.Start.
> This one waits until the VM is fully started.
>
> > admin.vm.Shutdown
> This one returns immediately.
>
> This inconsistency is rather annoying and leaves me to connect to the
> event-feed to get a final "it shut down" information.
> I would like them both to wait until finished.
> I would argue that the inconsistency is a bug.
>
>
> Additionally, shutting down is certainly possible to fail (or, timeout), which would call for a 'kill'.
> I forsee having to write a lot more code to handle that case with the current API.

This is indeed true, see qvm-shutdown tool:
https://github.com/QubesOS/qubes-core-admin-client/blob/master/qubesadmin/tools/qvm_shutdown.py

> Ideally "Shutdown" would not return immediately, but would return either on
> a) completion (success)
> b) timeout or failure (as an exception).

This require per-VM shutdown timeout. This means new property:
`shutdown_timeout` (we already have `qrexec_timeout`).

> So, my question is if this can still be made consistent, which would make my app using the API simpler.

Well, it's a bit late... But I think the benefits may still worth changing
it right now. The alternative is to keep the old behaviour, then add a new
call like admin.vm.ShutdownAndWait (see below about compat). I'm
slightly in favor of the latter option, to not risk breaking things just
before release. But if you think it still worth changing it for
admin.vm.Shutdown, and could help adjusting existing clients, I think we
can do it.

Changing API side (qubes-core-admin repo) is trivial. Less trivial is
changing client side: qvm-shutdown and GUI
(qubes-dbus/qubesdbus/models.py and
qubes-desktop-linux-manager/qui/tray/domains.py). Do you want to take care of any of it?

> Second question; what is the general idea around backwards compatibility of the APIs?

We start keeping backward compatibility at final release. Changes can be
either introduced in backward compatible way (like, new _optional_
arguments), or - if impossible - create new call and keep the old one.

> ps. I started documenting the API here;
>
> https://github.com/QubesController/qubes-api-cpp-lib/blob/master/Qubes.cpp#L29

You may want to look here:
https://www.qubes-os.org/doc/admin-api/

And here:
https://dev.qubes-os.org/projects/qubes-core-admin/en/latest/

Here for example are most events documented:
https://dev.qubes-os.org/projects/qubes-core-admin/en/latest/qubes-vm/qubesvm.html

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----

iQEzBAEBCAAdFiEEhrpukzGPukRmQqkK24/THMrX1ywFAlpmAmsACgkQ24/THMrX
1yxfUgf/YrGSqKfVKe3YF1xbXLpjG1ayrM45kQQjivp31TxxtCiAbKWuJRpue9w2
MFsnWAkSahINxmoQ0jm/578kiAIGapDsl2zx+uVZz+NGLs7xpCf3MJ3EF7K7F1jm
O+hXycagJER5kmBYbwCoOwZEXJOJ2OkJxeqVbwiUlyVE117j4w9jE+UC7RdlZY8a
75qPwinfLRZ8SAi8OgKC0BQMA8hRyN+EUD574F/Z3IXZjSj0KpdugMOmEhJmz0A+
gYzJVj90sAoWyzbOqmv4lQcT0a/q0ah1ZpSgqBgjtNPxxbWgWbx/UWAKzZQcZnqm
/5BZ3OS/LVYOZA4BPDPR67PaEOpfsQ==
=vdEu
-----END PGP SIGNATURE-----

[Tom Zander]

On Monday, 22 January 2018 16:25:31 CET Marek Marczykowski-Górecki wrote:
> > So, my question is if this can still be made consistent, which would
> > make my app using the API simpler.
> Well, it's a bit late... But I think the benefits may still worth changing
> it right now.

I guess it depends on weather the latest RC will be the final or not :)

> The alternative is to keep the old behaviour, then add a
> new call like admin.vm.ShutdownAndWait (see below about compat). I'm
> slightly in favor of the latter option, to not risk breaking things just
> before release. But if you think it still worth changing it for
> admin.vm.Shutdown, and could help adjusting existing clients, I think we
> can do it.

I am not in a position to understand the amount of users of the API, just
the qvm-shutdown and the qubes-manager as far as I can tell. I'll obviously
take care of my qubes-controller project.

As python is my least favourite language to program in, I have very little
experience with it and have absolutely no idea how to do threading /
blocking apis etc. using python.
So I'm not sure I can be of much help, and I try to keep my Qubes hacking
confined to weekends only :)

It would be great if anyone else can help out making this API consistent.

As an alternative, adding an 'waitForFinished' argument added to
admin.vm.Shutdown may allow the current API to be updated without
introducing a new top-level command.

Thinking further along those lines;
it should be rather trivial to pass 'waitForFinished\0false' in each of the
using APIs allowing the default value of that optional argument to be set to
'true' even in the upcoming release.