Settings daemon driver - take 2 #835

ricab · 2019-06-11T18:19:27Z

Fixes #776.

New attempt, using a central file for daemon settings and requiring admin privileges to set.

Saviq

Hey, that's exactly what we need, just a few polishing touches :)

src/platform/platform_linux.cpp

src/utils/settings.cpp

ricab

Thanks for the review @Saviq , here are my replies.

src/platform/platform_linux.cpp

src/utils/settings.cpp

src/platform/platform_linux.cpp

ricab · 2019-06-13T09:10:13Z

@Saviq I replied to your points, but github is not dealing well with the multiple reviews. You can see them in the files tab or in each original comment's thread in this page.

Saviq · 2019-06-13T10:36:43Z

@Saviq I replied to your points, but github is not dealing well with the multiple reviews. You can see them in the files tab or in each original comment's thread in this page.

Yeah I thought I was being smart by "reviewing" all the comments rather than commenting one-by-one. Seems Github outsmarted me by making a mess out of that.

ricab · 2019-06-13T11:37:47Z

Yeah I thought I was being smart by "reviewing" all the comments rather than commenting one-by-one. Seems Github outsmarted me by making a mess out of that.

That is what I did also. It usually works, but maybe not if both sides use that approach.

codecov · 2019-06-14T09:41:59Z

Codecov Report

Merging #835 into master will increase coverage by 0.11%.
The diff coverage is 35.29%.

@@            Coverage Diff             @@
##           master     #835      +/-   ##
==========================================
+ Coverage   68.74%   68.86%   +0.11%     
==========================================
  Files         191      191              
  Lines        6530     6545      +15     
==========================================
+ Hits         4489     4507      +18     
+ Misses       2041     2038       -3

Impacted Files	Coverage Δ
include/multipass/settings.h	`0% <ø> (ø)`	⬆️
src/platform/platform_linux.cpp	`80.64% <100%> (+46.02%)`	⬆️
src/utils/settings.cpp	`22.91% <8.33%> (-3.4%)`	⬇️
...c/platform/backends/shared/linux/backend_utils.cpp	`53.62% <0%> (+1.44%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8a3a571...f1a9331. Read the comment docs.

codecov · 2019-06-14T09:41:59Z

Codecov Report

Merging #835 into master will increase coverage by 0.04%.
The diff coverage is 43.85%.

@@            Coverage Diff            @@
##           master    #835      +/-   ##
=========================================
+ Coverage   68.75%   68.8%   +0.04%     
=========================================
  Files         192     192              
  Lines        6602    6635      +33     
=========================================
+ Hits         4539    4565      +26     
- Misses       2063    2070       +7

Impacted Files	Coverage Δ
include/multipass/settings.h	`0% <ø> (ø)`	⬆️
include/multipass/exceptions/settings_exceptions.h	`20% <0%> (ø)`	⬆️
src/client/cli/cmd/set.cpp	`93.54% <100%> (+8.36%)`	⬆️
src/platform/platform_linux.cpp	`69.23% <66.66%> (+31.13%)`	⬆️
src/utils/settings.cpp	`22% <7.69%> (-4.32%)`	⬇️
src/platform/backends/shared/linux/linux_process.h	`0% <0%> (-100%)`	⬇️
...tform/backends/qemu/qemu_virtual_machine_factory.h	`0% <0%> (ø)`	⬆️
...c/platform/backends/shared/linux/process_factory.h	`0% <0%> (ø)`	⬆️
src/platform/backends/qemu/dnsmasq_server.cpp	`90.69% <0%> (ø)`	⬆️
...orm/backends/qemu/qemu_virtual_machine_factory.cpp	`86.33% <0%> (ø)`	⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0409d1e...e1dd710. Read the comment docs.

ricab · 2019-06-18T19:17:11Z

@Saviq

I remembered why we discarded /root before: it is not generally readable and so would require sudo for multipass get. I therefore went back to the earlier agreement of /var/lib, which best matches the requirements I was given (as I understood them).

If you deem the /root-based path preferable – because it could be preserved when moving away from file-based communication and despite the added permission restriction – let me know, so I can change the current speculative implementation. In that event, please also clarify whether you prefer:

/root/.local/share, which corresponds to AppDataLocation, but is different from what is already used in the client
/root/.config, which corresponds to AppConfigLocation, matching the client but having /etc/xdg as fallback path (althought I don't see when, if ever, that would be returned by Qt).

If you want to stick with previous decisions despite newly found implications, or if you simply do not have a preference and/or just want me to decide, please also let me know that. If it were up to me (and excluding new findings) I would probably go with 2 above and consider sudo a temporary consequence of the file-based comm choice.

Saviq · 2019-06-19T09:04:34Z

I'm fine with sudo and 2. above - provided the file location will not change when we move to RPC.

8000

ricab · 2019-06-19T09:16:48Z

OK, I will make that change then.

ricab · 2019-06-19T13:00:23Z

Unfortunately I again stumbled upon QSettings' inability to distinguish absent from unreadable files. I now realized my previous attempt to circumvent this (with exists_but_unreadable) was not working in all cases. In particular, if the file was in a directory that could itself not be read (e.g. /root/.config), QFileInfo("/root/.config/multipassd/multipassd.conf").exists() would return false, even if the file was there. So, with the new path, we would happily return the default value on a multipass get without sudo. I did not think this would make for an acceptable UX:

$ sudo multipass set local.driver libvirt
$ multipass get local.driver
qemu

After some investigation I ended up solving this by checking errno directly after trying to manually open the file. Permissions checks would suffer from the same issue as above. Checking for parent permissions would be doing the OS's job. QFile.error did not distinguish our cases either (it just returned QFileDevice::OpenError). QFile::errorString did, but string matching would be unreliable, as the actual returned string is not part of any interface. After digging through Qt's code, I found errorString was just relying on errno, so I used that directly.

The remaining "dir case" is reported by QSettings itself, so we don't need to account for it explicitly.

Saviq

A handful of questions/nitpicks below.

snap/snapcraft.yaml

src/daemon/daemon_main.cpp

src/platform/platform_linux.cpp

Saviq · 2019-06-19T14:19:45Z

This could use a nicer error message:

$ multipass set local.driver libvirt
Could not read/write settings: access error

Just realized:

$ sudo multipass set local.driver=libvirt
Need exactly one option key and one value

The supported format for mutlipass set should be key=value. Doesn't have to be this PR, though.

This works, but shouldn't:

$ sudo multipass set local.driver hyperkit
$ sudo multipass set local.driver foobar
$ sudo multipass get local.driver       
foobar

ricab · 2019-06-19T15:37:54Z

nicer error message

Like what? Do you mean adding "try sudo" or something? Or just totally different message?

Just realized:

Yes, I thought the equals-sign was just a quirk of description, since we separate all positional arguments w/ space currently. I will change it if it really was intentional.

This works, but shouldn't:

Well, those will fail the daemon when it tries to restart (just like the env vars and snapctl). I thought it appropriate to maintain this behavior for now because it keeps the responsibility of semantics-checking in the consumer – the daemon, where it will naturally be when we move to RPC. Do you want a temporary mp::platform check being called from the client instead?

Saviq · 2019-06-19T15:48:30Z

Like what? Do you mean adding "try sudo" or something? Or just totally different message?

Yeah, something like that.

Yes, I thought the equals-sign was just a quirk of description, since we separate all positional arguments w/ space currently. I will change it if it really was intentional.

It was intentional so you can provide multiple key/value pairs without those getting confusing.

Well, those will fail the daemon when it tries to restart (just like the env vars and snapctl). I thought it appropriate to maintain this behavior for now because it keeps the responsibility of semantics-checking in the consumer – the daemon, where it will naturally be when we move to RPC. Do you want a temporary mp::platform check being called from the client instead?

Right, the problem is that the client then just says:

$ multipass find
find failed: cannot connect to the multipass socket
Please ensure multipassd is running and '/run/multipass_socket' is accessible

And setting a different value doesn't resolve that, since the daemon's dead by then and won't be restarted automatically.

ricab · 2019-06-19T15:53:15Z

I see. Alright, fixing pending items.

ricab · 2019-06-19T17:39:19Z

Only last couple of points missing (= and driver-value check).

ricab · 2019-06-21T19:16:07Z

OK, so those two items are in. But reading back #756 and comments above, there is actually a 3rd item that I did not push yet: supporting multiple key-value pairs in set. For that, I would need a strategy to handle invalid keys/values. I considered the following options (in increasing preference):

Process arguments in order and stop setting upon error.
Set as much as we can from the command line arguments, skipping failures and continuing with remaining arguments, showing all errors in the end.
Only set anything if all settings succeed - requires transactional setting API.

Option 1 would mean committing to an order or leaving the settings in an undefined state. Either way, it does not sound very user-friendly.

Option 2 would require bookkeeping shenanigans and clarifying what caused what in the output (think different error causes in different arguments, building error text in an iterating maner from multiple exception msgs and corresponding args...). This would probably feel clunky to the user (not to mention the programmer).

Both 1 and 2 suffer from other downsides as well:

the various options may be related, meaning the user does not want to set one without the other
easy to do something like multipass set key1=val1K ey2=val2 and get key1=val1K recorded (if there were no particular requirements on key1's value)
these approaches are not scalable if we want to accept plain YAML in the future

Option 3 is the cleaner one IMO. We only let the request go through if it is 100% correct. While this could be done by keeping former values to revert to in case of error, that could be a bit clumsy – there could be problems when trying to revert and things would be left in an undefined state.

Doing option 3 properly requires changing the current interface of the Settings class to receive multiple options at once. I have an implementation, which is a bit hackish as long as we save to separate files, but that would go away. It mostly works, though it would still need some work, particularly on tests. I say "mostly" for two reasons:

Since we use separate files, one of them needs to be written before the other. If there is a failure writing the second one, it was not transactional.
Because we need sudo for daemon settings, the following undesirable interaction sequence becomes likely:

$ multipass set client.primary_name=yoyo local.driver=libvirt
Unable to read/write settings: access error (consider `sudo`)
$ sudo multipass set client.primary_name=yoyo local.driver=libvirt
$ multipass set client.primary_name=asdf
Unable to read/write settings: access error (consider `sudo`)

Sudoing the second step above would create a root-owned client file. Client settings would not work without sudo anymore.

For this reason (reason 2 above), and because we have only two settings for the time being, I thing our best option is to allow only one argument in set until we move to RPC for daemon settings. However, we will still have to deal with problem 1, even then. If, in order to address that, we decide to disallow set on multiple entities simultaneously (i.e. clients/daemons), that could be done today. It would solve problem 2 today, albeit only in abstract: since we have no more than one option per entity at present, no multiple-arg set would be accepted.

ricab · 2019-06-21T19:28:49Z

PS: In either case, perhaps this PR can already be merged. I would then deal with the other platforms before moving on to further functionality.

Saviq · 2019-06-24T08:45:23Z

@ricab yeah, multi-set would be a later step towards full config story, I didn't mean it should happen in this PR, just that the <key>=<value> format is more friendly for that case.

Saviq · 2019-06-24T12:04:41Z

src/client/cli/cmd/set.cpp


    auto status = parser->commandParse(this);
    if (status == ParseCode::Ok)
    {
        const auto args = parser->positionalArguments();
-        if (args.count() == 2)
+        if (args.isEmpty())


If someone provides more than one, it will be silently dropped?

Good catch, I did not do this because I meant to evolve to multiple arguments and then forgot.

Saviq · 2019-06-24T12:18:02Z

include/multipass/exceptions/settings_exceptions.h

-    PersistentSettingsException(const QString& attempted_operation, const QString& failure)
-        : SettingsException{fmt::format("Could not {} settings: {} error", attempted_operation, failure)}
+    PersistentSettingsException(const QString& attempted_operation, const QString& detail)
+        : SettingsException{fmt::format("Unable to {} settings: {}", attempted_operation, detail)}


I won't block on this, but we'll have to move away from constructing the error message like this later.

I am not sure what the issue is. Note that attempted_operation could be an enum here, representing read and write ops, that we would then turn into string. Then detail is a separate phrase. How could I make this friendlier to translation? Perhaps an example of what problems this could cause would help me understand better.

Basically what I mean is that "Unable to format settings" may translate to something completely different than "Unable to access settings" depending on language.

Different languages work in very different ways, building a sentence from a set of strings is bound to break at one point or another. It's a discussion to have over beers, though :)

I'm just sensitive to this because I've translated a lot of software over the years and Polish is one of the more complex languages out there so it easily highlights problems in approaches like that.

I think I understand the idea. To avoid duplication, I think I would then try a more "square" way, e.g. "Unable to operate on settings. Operation: {}; Details: {}". But this might not feel so good for the user... Isn't this the sort of thing that one would do when preparing the code for translation? Preemptively duplicating messages in the code does not feel right to me either.

BTW, the operation here is just "read" or "read/write", "format/access" goes in the second param as a free string.

Saviq · 2019-06-24T12:18:53Z

src/utils/settings.cpp

-                                                                       : QStringLiteral("access")};
+        throw mp::PersistentSettingsException{
+            attempted_operation, status == QSettings::FormatError ? QStringLiteral("format error")
+                                                                  : QStringLiteral("access error (consider `sudo`)")};


How will we approach this for Windows, where sudo is not a thing?

Good point. How about "consider running this with an administrative role"? (Replacing it here and having the same msg in all platforms)

Works for me.

Saviq · 2019-06-24T12:21:05Z

src/client/cli/cmd/set.cpp

-            cerr << "Need exactly one option key and one value\n";
-            status = ParseCode::CommandLineError;
+            const auto keyval = args.at(0).split('=');
+            if (keyval.count() != 2 || keyval.contains(QStringLiteral("")))


Long-term we'll need to be smarter here, I can imagine some values including = characters. So either we split at the first one only, or require some escape sequences.

Not in this PR, though.

OK, leaving it for now. multipass config would be an alternative for those cases in the future.

ricab · 2019-06-25T10:20:09Z

I didn't mean it should happen in this PR

OK, the reasoning stands for when it should happen, and can perhaps elucidate whether it should happen.

Adds a platform-dependent derivation of the daemon-config directory.

So that "on-failure-restart policies" are enough to restart the daemon when the settings change.

Use a `/root/config`-based dir for the default daemon-settings location. Add the application name to that path, for consistency with the client.

To avoid `Settings::get` returning a default in that case.

Co-Authored-By: Michał Sawicz <michal@sawicz.net>

Currently failing (implementation pending).

Fails `set` command with unsupported driver. Makes previous tests pass.

Refactor a couple of cli client tests on `set` cmd.

Currently failing (implementation pending).

Makes previous tests pass.

To accommodate adding other unrelated parameterized tests.

So that it makes sense on other platforms.

Saviq

's good!

townsend2010

This looks good to me. I had one question below that needs answered before I approve:)

include/multipass/platform.h

townsend2010

Ok, all good.

bors r=Saviq, townsend2010

835: Settings daemon driver - take 2 r=Saviq,townsend2010 a=ricab Fixes #776. New attempt, using a central file for daemon settings and requiring admin privileges to set. Co-authored-by: Ricardo Abreu <ricab@ricabhome.org>

bors · 2019-06-26T13:19:35Z

Build failed

continuous-integration/travis-ci/push

ricab changed the title ~~Settings daemon driver take2~~ Settings daemon driver - take 2 Jun 11, 2019

ricab added no-merge and removed no-merge labels Jun 11, 2019

Saviq requested changes Jun 12, 2019

View reviewed changes

src/platform/platform_linux.cpp Outdated Show resolved Hide resolved

src/platform/platform_linux.cpp Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

ricab commented Jun 12, 2019

View reviewed changes

src/platform/platform_linux.cpp Outdated Show resolved Hide resolved

src/platform/platform_linux.cpp Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

ricab force-pushed the settings-daemon-driver-take2 branch from 0df7cf9 to 7c7d345 Compare June 12, 2019 16:59

Saviq requested changes Jun 13, 2019

View reviewed changes

src/utils/settings.cpp Outdated Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

src/utils/settings.cpp Outdated Show resolved Hide resolved

src/platform/platform_linux.cpp Show resolved Hide resolved

Saviq requested changes Jun 19, 2019

View reviewed changes

ricab force-pushed the settings-daemon-driver-take2 branch from 66c128d to 50da5ad Compare June 21, 2019 18:05

Saviq mentioned this pull request Jun 24, 2019

Switching between drivers unreliable #854

Closed

Saviq reviewed Jun 24, 2019

View reviewed changes

ricab force-pushed the settings-daemon-driver-take2 branch from 4ed3637 to 52fd7a0 Compare June 25, 2019 16:13

ricab and others added 19 commits June 25, 2019 17:15

[settings] Use platform-dependent config path

adb9e10

Adds a platform-dependent derivation of the daemon-config directory.

[daemon] Exit with EXIT_FAILURE on settings change

921bbb1

So that "on-failure-restart policies" are enough to restart the daemon when the settings change.

[settings] Update default dir of daemon config

bda6d55

Use a `/root/config`-based dir for the default daemon-settings location. Add the application name to that path, for consistency with the client.

[settings] Fix silent ignore of permission denied

b76ad25

To avoid `Settings::get` returning a default in that case.

[daemon] Update exit code on settings change (42)

39f4402

[settings] Use QDir for file path

5988e92

Co-Authored-By: Michał Sawicz <michal@sawicz.net>

[daemon] Drop split driver treatment in error msg

053c155

[settings] Improve error messages

0f6471e

[test] Test that unsupported drivers are rejected

80e629d

Currently failing (implementation pending).

[settings] Implement synchronous driver rejection

7330309

Fails `set` command with unsupported driver. Makes previous tests pass.

[test] Small test refactoring

b9617b5

Refactor a couple of cli client tests on `set` cmd.

[test] Change set cli tests to key=val format

f5fb45b

Currently failing (implementation pending).

[test] Test set rejects bad key-val format

9268774

Currently failing (implementation pending).

[cli] Implement <key>=<val> format handling in set

1a122b8

Makes previous tests pass.

[test] Extract parameterized tests to own class

c65a269

To accommodate adding other unrelated parameterized tests.

[test] Test wrong arg num in get/set cmds

b240fa0

[cli] Fix argument checking in set cmd

dab7ddd

[settings] Improve error message

e8e3605

So that it makes sense on other platforms.

[test] Fix number of arguments in mock expectation

62166a3

ricab force-pushed the settings-daemon-driver-take2 branch from 52fd7a0 to 62166a3 Compare June 25, 2019 16:20

Saviq approved these changes Jun 25, 2019

View reviewed changes

townsend2010 reviewed Jun 25, 2019

View reviewed changes

include/multipass/platform.h Show resolved Hide resolved

Merge branch 'master' into settings-daemon-driver-take2

e1dd710

townsend2010 approved these changes Jun 26, 2019

View reviewed changes

townsend2010 merged commit e1dd710 into master Jun 26, 2019

bors bot deleted the settings-daemon-driver-take2 branch June 26, 2019 13:20

townsend2010 mentioned this pull request Jun 26, 2019

[snap] /snap/multipass/x7/bin/launch-multipassd: mkdir: not found #863

Closed

Saviq mentioned this pull request Jun 27, 2019

[snap] migrate from snap set driver… to multipass set local.driver… #866

Closed

Settings daemon driver - take 2 #835

Settings daemon driver - take 2 #835

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!