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

machines: Move from direct machines.json manipulation to D-Bus calls, move to /etc/cockpit/machines/*.json #5880

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@martinpitt
Member

martinpitt commented Feb 13, 2017

The first two commits implement the first item in the Finalize machines.json format trello card. I. e. it keeps /var/lib/machines.json for now, but moves the parsing/updating of that from the web frontend (cockpit.file) to a D-Bus interface.

The third commit moves the single /var/lib/machines.json to /etc/cockpit/machines/*.json. I kept that separately as it was historically developed that way, but it also makes it easier to separately review the move to D-Bus and the move to multiple files.

  • glib 2.44 is not available on all targets yet, rewrite g_auto* stuff to use manual cleanup
  • fix integration test regressions
  • drop XXX debug messages
  • fix memleaks (aside from leak in json-glib, added suppression )
  • add unit tests for new D-Bus interface
  • move to /etc/cockpit/machines/*.json
  • fix Update() call to only write the delta
  • change Update() call to take the destination file name instead of hardcoding it
  • tests for multiple machines/*.json files
  • update documentation
  • try to factorize merge_config(), the outer and inner loop are structurally similar
  • aggregate bursts of inotify events into one parse/property update callback

@martinpitt martinpitt self-assigned this Feb 13, 2017

@dperpeet

This comment has been minimized.

Show comment
Hide comment
@dperpeet

dperpeet Feb 13, 2017

Member

I suggest using a different prefix than machines - we have a package with that name that works with virtual machines.

Member

dperpeet commented Feb 13, 2017

I suggest using a different prefix than machines - we have a package with that name that works with virtual machines.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 13, 2017

Member

@dperpeet: good point; what about hosts? By "prefix" I assume you mean the D-Bus object/interfaces and the new source files.

Member

martinpitt commented Feb 13, 2017

@dperpeet: good point; what about hosts? By "prefix" I assume you mean the D-Bus object/interfaces and the new source files.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 13, 2017

Member

At least Debian 8 has an older glib (2.42), and the centos-7 build is unhappy too (although there's no useful error message). So I suppose I'll go back to manual unreffing for now. Too bad, this is quite nice :-)

Member

martinpitt commented Feb 13, 2017

At least Debian 8 has an older glib (2.42), and the centos-7 build is unhappy too (although there's no useful error message). So I suppose I'll go back to manual unreffing for now. Too bad, this is quite nice :-)

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 13, 2017

Member

Next version: I reverted the glib version bump and did the unreffing manually, and fixed the JsonNode reffing/counting to fix the test regressions (test suite now passes for me locally). But I'll only tick the task list box for that once the automatic results confirm.

Member

martinpitt commented Feb 13, 2017

Next version: I reverted the glib version bump and did the unreffing manually, and fixed the JsonNode reffing/counting to fix the test regressions (test suite now passes for me locally). But I'll only tick the task list box for that once the automatic results confirm.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 13, 2017

Member

note keeping: apparently on debian-8 and centos-7 json-glib is rather old, and does not offer some important API yet: I need to see how to get around that. This is hidden from the CI build logs, so putting it here:

src/bridge/cockpitdbusmachines.c:62:5: error: implicit declaration of function 'json_node_ref' [-Werror=implicit-function-declaration]
     result = json_node_ref (result);
     ^
src/bridge/cockpitdbusmachines.c:62:12: warning: assignment makes pointer from integer without a cast
     result = json_node_ref (result);
            ^
src/bridge/cockpitdbusmachines.c: In function 'build_machines':
src/bridge/cockpitdbusmachines.c:77:3: error: implicit declaration of function 'json_node_unref' [-Werror=implicit-function-declaration]
   json_node_unref (machines);
   ^
src/bridge/cockpitdbusmachines.c: In function 'update_machine':
src/bridge/cockpitdbusmachines.c:104:7: error: unknown type name 'JsonObjectIter'
       JsonObjectIter iter;
       ^
src/bridge/cockpitdbusmachines.c:110:7: error: implicit declaration of function 'json_object_iter_init' [-Werror=implicit-function-declaration]
       json_object_iter_init (&iter, json_node_get_object (info));
       ^
src/bridge/cockpitdbusmachines.c:111:7: error: implicit declaration of function 'json_object_iter_next' [-Werror=implicit-function-declaration]
       while (json_object_iter_next (&iter, &prop_name, &prop_node))
       ^
cc1: some warnings being treated as errors
Makefile:4107: recipe for target 'src/bridge/libcockpit_bridge_a-cockpitdbusmachines.o' failed

Update: fixed

Member

martinpitt commented Feb 13, 2017

note keeping: apparently on debian-8 and centos-7 json-glib is rather old, and does not offer some important API yet: I need to see how to get around that. This is hidden from the CI build logs, so putting it here:

src/bridge/cockpitdbusmachines.c:62:5: error: implicit declaration of function 'json_node_ref' [-Werror=implicit-function-declaration]
     result = json_node_ref (result);
     ^
src/bridge/cockpitdbusmachines.c:62:12: warning: assignment makes pointer from integer without a cast
     result = json_node_ref (result);
            ^
src/bridge/cockpitdbusmachines.c: In function 'build_machines':
src/bridge/cockpitdbusmachines.c:77:3: error: implicit declaration of function 'json_node_unref' [-Werror=implicit-function-declaration]
   json_node_unref (machines);
   ^
src/bridge/cockpitdbusmachines.c: In function 'update_machine':
src/bridge/cockpitdbusmachines.c:104:7: error: unknown type name 'JsonObjectIter'
       JsonObjectIter iter;
       ^
src/bridge/cockpitdbusmachines.c:110:7: error: implicit declaration of function 'json_object_iter_init' [-Werror=implicit-function-declaration]
       json_object_iter_init (&iter, json_node_get_object (info));
       ^
src/bridge/cockpitdbusmachines.c:111:7: error: implicit declaration of function 'json_object_iter_next' [-Werror=implicit-function-declaration]
       while (json_object_iter_next (&iter, &prop_name, &prop_node))
       ^
cc1: some warnings being treated as errors
Makefile:4107: recipe for target 'src/bridge/libcockpit_bridge_a-cockpitdbusmachines.o' failed

Update: fixed

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 13, 2017

Member

The Debianish tests fail because there /var/lib/cockpit is root:root 755 and thus the bridge of an admin user can't write to it. This works in RPMish environments as there the directory is root:wheel 775. While that should be fixed, I don't think we can/should rely on that, so we need a similar "run this through a root bridge" facility than the "superuser": "try" flag we used for cockpit.file.

Update: fixed

Member

martinpitt commented Feb 13, 2017

The Debianish tests fail because there /var/lib/cockpit is root:root 755 and thus the bridge of an admin user can't write to it. This works in RPMish environments as there the directory is root:wheel 775. While that should be fixed, I don't think we can/should rely on that, so we need a similar "run this through a root bridge" facility than the "superuser": "try" flag we used for cockpit.file.

Update: fixed

@martinpitt martinpitt changed the title from WIP: machines: Move from direct machines.json manipulation to D-Bus calls to WIP: machines: Move from direct machines.json manipulation to D-Bus calls, move to /etc/cockpit/machines/*.json Feb 17, 2017

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 17, 2017

Member

I now pushed the "move to /etc/cockpit/machines.d/*.json" changes and fixed the worst integration test fallout. Let's see what remains.

Member

martinpitt commented Feb 17, 2017

I now pushed the "move to /etc/cockpit/machines.d/*.json" changes and fixed the worst integration test fallout. Let's see what remains.

@martinpitt martinpitt removed the needswork label Feb 20, 2017

@martinpitt martinpitt changed the title from WIP: machines: Move from direct machines.json manipulation to D-Bus calls, move to /etc/cockpit/machines/*.json to machines: Move from direct machines.json manipulation to D-Bus calls, move to /etc/cockpit/machines/*.json Feb 20, 2017

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 20, 2017

Member

I fixed the remaining issues, and I believe this is now ready for reviewing. All integration tests succeed for me locally, but on the infra I keep getting (unrelated) failures from the usual suspects.

Member

martinpitt commented Feb 20, 2017

I fixed the remaining issues, and I believe this is now ready for reviewing. All integration tests succeed for me locally, but on the infra I keep getting (unrelated) failures from the usual suspects.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
{
static gchar *path = NULL;
if (path == NULL)
path = g_build_filename (g_getenv ("COCKPIT_CONFIG_DIR") ?: "/etc/cockpit", "machines.d", NULL);

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

I don't think we want a new env var for this.

Config is currently discovered like this. https://github.com/cockpit-project/cockpit/blob/master/src/common/cockpitconf.c#L156. I think we probably should use the same logic.

@petervo

petervo Feb 20, 2017

Contributor

I don't think we want a new env var for this.

Config is currently discovered like this. https://github.com/cockpit-project/cockpit/blob/master/src/common/cockpitconf.c#L156. I think we probably should use the same logic.

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

Thanks, fixed locally in my branch.

@martinpitt

martinpitt Feb 20, 2017

Member

Thanks, fixed locally in my branch.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
path = g_file_get_path (file);
if (g_str_has_suffix (path, ".json"))
g_debug ("on_machines_changed: event type %i on %s", event_type, path);

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

minor nit, probably should use {} here to match the else clause

@petervo

petervo Feb 20, 2017

Contributor

minor nit, probably should use {} here to match the else clause

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

Fixed in my local branch.

@martinpitt

martinpitt Feb 20, 2017

Member

Fixed in my local branch.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
if (G_UNLIKELY (res != 0 && res != GLOB_NOMATCH))
g_error ("glob %s failed with return code %i", glob_str, res);
/* also read /var/lib/cockpit/machines.json for backwards compat */
conf_glob.gl_pathv[0] = g_build_filename (g_getenv ("COCKPIT_DATA_DIR") ?: "/var/lib/cockpit", "machines.json", NULL);

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

Should we migrate this file once on start up and not have to read it every time.

@petervo

petervo Feb 20, 2017

Contributor

Should we migrate this file once on start up and not have to read it every time.

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

Migrating the file is part of https://github.com/cockpit-project/cockpit/wiki/Config-format-for-known-machines-and-ssh-keys, this PR just does not implement this (yet). I wasn't completely sure if we can just do that, but I suppose we don't need to be concerned about downgrading -bridge after the migration?

However, it might happen that the migration fails because /etc/cockpit/machines.d/ cannot be created or written to for some reason. So we need to keep the code that reads /var/ for some time at least.

This seemed hairy enough to be in a separate PR (this doesn't address known_hosts either yet), but if you prefer I'll fold it in here.

@martinpitt

martinpitt Feb 20, 2017

Member

Migrating the file is part of https://github.com/cockpit-project/cockpit/wiki/Config-format-for-known-machines-and-ssh-keys, this PR just does not implement this (yet). I wasn't completely sure if we can just do that, but I suppose we don't need to be concerned about downgrading -bridge after the migration?

However, it might happen that the migration fails because /etc/cockpit/machines.d/ cannot be created or written to for some reason. So we need to keep the code that reads /var/ for some time at least.

This seemed hairy enough to be in a separate PR (this doesn't address known_hosts either yet), but if you prefer I'll fold it in here.

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

Seems to me that migrating the file can be done separately from anything to do with known hosts. But yes, handling the no permissions case properly is a concern. I'm ok with leaving this for a follow up or a remotectl command.

@petervo

petervo Feb 20, 2017

Contributor

Seems to me that migrating the file can be done separately from anything to do with known hosts. But yes, handling the no permissions case properly is a concern. I'm ok with leaving this for a follow up or a remotectl command.

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

I created a work item for that in the Trello card.

@martinpitt

martinpitt Feb 20, 2017

Member

I created a work item for that in the Trello card.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
}
g_free (path);
machines = build_machines ();

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

It would be nice to not have to rebuild this from scratch everytime.

@petervo

petervo Feb 20, 2017

Contributor

It would be nice to not have to rebuild this from scratch everytime.

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

This is the callback when /etc/cockpit/machines.d/ changes, so here we have to rebuild it. Parsing a few small JSON files is rather cheap, I don't want to introduce complex heuristics about which files we don't need to re-parse and keep a per-file partial struct. This would be very involved and error prone.

@martinpitt

martinpitt Feb 20, 2017

Member

This is the callback when /etc/cockpit/machines.d/ changes, so here we have to rebuild it. Parsing a few small JSON files is rather cheap, I don't want to introduce complex heuristics about which files we don't need to re-parse and keep a per-file partial struct. This would be very involved and error prone.

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

I see the issue, i'm just a little concerned about performance if it goes beyond just a couple files. WDYT about adding some throttling to minimize the number of times it gets run or at least ensure that we don't have multiple runs going at the same time?

@petervo

petervo Feb 20, 2017

Contributor

I see the issue, i'm just a little concerned about performance if it goes beyond just a couple files. WDYT about adding some throttling to minimize the number of times it gets run or at least ensure that we don't have multiple runs going at the same time?

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

See measurements below. I. e. the thing to optimize is the number of D-Bus calls/property updates, not the parsing. But as we want a reactive UI, I don't think we can just ignore updates for some time after one update got sent, as we can't predict whether the files will change again after that dead time.

@martinpitt

martinpitt Feb 20, 2017

Member

See measurements below. I. e. the thing to optimize is the number of D-Bus calls/property updates, not the parsing. But as we want a reactive UI, I don't think we can just ignore updates for some time after one update got sent, as we can't predict whether the files will change again after that dead time.

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

It wouldn't be ignoring events, just batching events so that it's not one run per each event when there are multiple events received in quick succession.

@petervo

petervo Feb 20, 2017

Contributor

It wouldn't be ignoring events, just batching events so that it's not one run per each event when there are multiple events received in quick succession.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
g_return_val_if_fail (property_name != NULL, NULL);
if (g_str_equal (property_name, "Machines"))
return build_machines ();

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

I don't think we want to be rebuilding this on every property call. We should probably mantain a cache of what we've loaded. That might also help with the post monitoring actions.

@petervo

petervo Feb 20, 2017

Contributor

I don't think we want to be rebuilding this on every property call. We should probably mantain a cache of what we've loaded. That might also help with the post monitoring actions.

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

Agreed, machines_get_property() can use a cache as we have an inotify watch in place. Will work on this tomorrow.

@martinpitt

martinpitt Feb 20, 2017

Member

Agreed, machines_get_property() can use a cache as we have an inotify watch in place. Will work on this tomorrow.

// FIXME: investigate re-using the proxy from Loader (runs in different frame/scope)
var bridge = cockpit.dbus(null, { bus: "internal", "superuser": "try" });
var mod = bridge.call("/machines", "cockpit.Machines", "Update", [ "99-webui.json", host, values_variant ])

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

I'm not 100% convinced by having the dashboard write to only one file. But if everyone else in on board with that I'm ok with it.

@petervo

petervo Feb 20, 2017

Contributor

I'm not 100% convinced by having the dashboard write to only one file. But if everyone else in on board with that I'm ok with it.

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

I wondered about that as well in the recent email thread (direct link to that message) .

@mvollmer convinced me that it's better to write a dashboard specific file than to modify the original files, as that might conflict with files shipped by packages or managed by puppet & friends.

@martinpitt

martinpitt Feb 20, 2017

Member

I wondered about that as well in the recent email thread (direct link to that message) .

@mvollmer convinced me that it's better to write a dashboard specific file than to modify the original files, as that might conflict with files shipped by packages or managed by puppet & friends.

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

That's a pretty good reason. And I think one that we can live with.

A bigger decision is the implementation of multiple machines per file, rather than enforcing one machine per file. However I think you've made the right call here too.

@stefwalter

stefwalter Feb 22, 2017

Contributor

That's a pretty good reason. And I think one that we can live with.

A bigger decision is the implementation of multiple machines per file, rather than enforcing one machine per file. However I think you've made the right call here too.

}
static void
machines_method_call (GDBusConnection *connection,

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

Were you thinking the corresponding remotectl command for this would autogenerate the file name? Or expect that as user input. Or should we be doing some autogeneration here?

@petervo

petervo Feb 20, 2017

Contributor

Were you thinking the corresponding remotectl command for this would autogenerate the file name? Or expect that as user input. Or should we be doing some autogeneration here?

This comment has been minimized.

@martinpitt

martinpitt Feb 20, 2017

Member

I thought ATM remotectl only managed the certificate, not machines.json entries? Adding a CLI is indeed a work item in the Trello card but hasn't been discussed/designed yet. My first version had the 99-webui.json file name hardcoded in the bridge, but I changed it into an explicit D-Bus argument so that it only needs to be hardcoded in the frontend and thus the D-Bus API is flexible to write other files, for a CLI API to supply the "pre-seeded" files.

I think a CLI for adding a machine should encourage to specify a file name as well, maybe defaulting to "10-.json"?

@martinpitt

martinpitt Feb 20, 2017

Member

I thought ATM remotectl only managed the certificate, not machines.json entries? Adding a CLI is indeed a work item in the Trello card but hasn't been discussed/designed yet. My first version had the 99-webui.json file name hardcoded in the bridge, but I changed it into an explicit D-Bus argument so that it only needs to be hardcoded in the frontend and thus the D-Bus API is flexible to write other files, for a CLI API to supply the "pre-seeded" files.

I think a CLI for adding a machine should encourage to specify a file name as well, maybe defaulting to "10-.json"?

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

remotectl currently only manages certs but long term we want to be able to add machines with it. Having a 10-{{hostname}}.json default sounds good to me.

@petervo

petervo Feb 20, 2017

Contributor

remotectl currently only manages certs but long term we want to be able to add machines with it. Having a 10-{{hostname}}.json default sounds good to me.

This comment has been minimized.

@petervo

petervo Feb 20, 2017

Contributor

Just mainly want to make sure we've thought about the cli use cases before finalizing the dbus call params.

@petervo

petervo Feb 20, 2017

Contributor

Just mainly want to make sure we've thought about the cli use cases before finalizing the dbus call params.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 20, 2017

Member

Wrt. performance concerns: I did some timing with 20 files (the maximum number of hosts that we allow):

  struct timespec before, after;
  clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &before);
  for (unsigned i = 0; i < 10000; ++i)
  {
      GVariant *v = build_machines ();
      g_variant_unref (v);
  }
  clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &after);
  gint64 delta = (after.tv_sec - before.tv_sec) * 1000000000 + (after.tv_nsec - before.tv_nsec);
  g_warning("time: %" G_GINT64_FORMAT " ns\n", delta);
$ for i in `seq 20`; do echo "{\"host$i\": {\"address\": \"1.2.3.4\"}}" | sudo tee /etc/cockpit/machines.d/01-host$i.json; done

which takes between 3.42 and 3.46 s on my system (so the clock is fairly reliable, ran it ten times). I. e. one build_machine() call takes about 0.34 ms. Even on a slow ARM system it shouldn't take more than a few ms. Also, we don't run concurrent threads, so these don't run simultaneously.

So I don't think we need to over-optimize this. I'll still add a cache for get_property(), but the overhead of funneling this through D-Bus and into JS will outweigh the actual parsing by a lot.

In practical terms these files won't change a lot anyway, and the dashboard only ever reads the property once (at program start) and then just listens to inotify changes.

Member

martinpitt commented Feb 20, 2017

Wrt. performance concerns: I did some timing with 20 files (the maximum number of hosts that we allow):

  struct timespec before, after;
  clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &before);
  for (unsigned i = 0; i < 10000; ++i)
  {
      GVariant *v = build_machines ();
      g_variant_unref (v);
  }
  clock_gettime(CLOCK_PROCESS_CPUTIME_ID, &after);
  gint64 delta = (after.tv_sec - before.tv_sec) * 1000000000 + (after.tv_nsec - before.tv_nsec);
  g_warning("time: %" G_GINT64_FORMAT " ns\n", delta);
$ for i in `seq 20`; do echo "{\"host$i\": {\"address\": \"1.2.3.4\"}}" | sudo tee /etc/cockpit/machines.d/01-host$i.json; done

which takes between 3.42 and 3.46 s on my system (so the clock is fairly reliable, ran it ten times). I. e. one build_machine() call takes about 0.34 ms. Even on a slow ARM system it shouldn't take more than a few ms. Also, we don't run concurrent threads, so these don't run simultaneously.

So I don't think we need to over-optimize this. I'll still add a cache for get_property(), but the overhead of funneling this through D-Bus and into JS will outweigh the actual parsing by a lot.

In practical terms these files won't change a lot anyway, and the dashboard only ever reads the property once (at program start) and then just listens to inotify changes.

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Feb 20, 2017

Contributor

That limit is not going to hold in the future, I don't think we should be basing decisions on that limit.

It's not simultaneous runs that i'm concerned about as much as the function just being queued to run again and again in quick succession. Think of a cron or CM process that comes in and regularly touches stuff. Or even a simple rm *-hostname.json and there are 10 files that match. That will result in this function running 10x.

Contributor

petervo commented Feb 20, 2017

That limit is not going to hold in the future, I don't think we should be basing decisions on that limit.

It's not simultaneous runs that i'm concerned about as much as the function just being queued to run again and again in quick succession. Think of a cron or CM process that comes in and regularly touches stuff. Or even a simple rm *-hostname.json and there are 10 files that match. That will result in this function running 10x.

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 21, 2017

Member

Indeed, I'll aggregate bursts of inotify events into just one parsing/D-Bus property update. Added a TODO item into the description.

Member

martinpitt commented Feb 21, 2017

Indeed, I'll aggregate bursts of inotify events into just one parsing/D-Bus property update. Added a TODO item into the description.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Feb 21, 2017

bridge: read /etc/cockpit/machines.d/*.json machine definitions
Cockpit plugins, other packages, admins, VM management software, or
config management systems like Ansible/puppet/cloud-init might want to
pre-configure machines for cockpit. This is currently racy as each of
those and cockpit itself have to open and rewrite
/var/lib/cockpit/machines.json. Also, this should be treated as
configuration, not state.

So move the remote machines configuration to /etc/cockpit/machines.d/
and read all *.json files in that directory. Later files override
settings from earlier ones, i. e. they can add new hosts, add new
properties to existing hosts, or change existing properties. The D-Bus
Update() method now expects a file name to update, and the dashboard
writes into "99-webui.json".

Pre-create /etc/cockpit/machines.d in our install and thus our packages,
as otherwise we can't watch it for changes; and the bridge that does the
watching is running as user, so it does not have the privileges to
create the directory by itself.

Adjust the integration tests to the changed paths.

Closes #5880
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 21, 2017

Member

I pushed an update which now aggregates bursts of inotify events into just one round of parsing *.json and sending PropertiesChanged, and fixes the little things Peter found.

Notably it does not yet cache the machines property if you do an explicit Get() call -- I have a patch for this, but it is broken both in terms of tests (as a Get call now gets an obsolete value, one needs to retry a few times) and in terms of valgrind (as keeping proper refcounts is a bit ugly), and IMHO it also doesn't buy much really: Our UI only calls Get() once ever (when the bridge is started), and I'm not sure whether this is actually the right thing to do for the API in general -- if you write a file and then call Get(), it's not completely unreasonable to expect that you get the updated properties. With caching you would need to wait until the delayed inotify update kicks in. I keep this open as a todo item above for now.

Member

martinpitt commented Feb 21, 2017

I pushed an update which now aggregates bursts of inotify events into just one round of parsing *.json and sending PropertiesChanged, and fixes the little things Peter found.

Notably it does not yet cache the machines property if you do an explicit Get() call -- I have a patch for this, but it is broken both in terms of tests (as a Get call now gets an obsolete value, one needs to retry a few times) and in terms of valgrind (as keeping proper refcounts is a bit ugly), and IMHO it also doesn't buy much really: Our UI only calls Get() once ever (when the bridge is started), and I'm not sure whether this is actually the right thing to do for the API in general -- if you write a file and then call Get(), it's not completely unreasonable to expect that you get the updated properties. With caching you would need to wait until the delayed inotify update kicks in. I keep this open as a todo item above for now.

@martinpitt martinpitt removed the needswork label Feb 21, 2017

@petervo

This comment has been minimized.

Show comment
Hide comment
@petervo

petervo Feb 21, 2017

Contributor

I think that makes sense for now.

@stefwalter do you have any comments on the API, specifically having all hosts as one property as well as dashboard only writing to one file.

Contributor

petervo commented Feb 21, 2017

I think that makes sense for now.

@stefwalter do you have any comments on the API, specifically having all hosts as one property as well as dashboard only writing to one file.

@stefwalter

I don't think writing to only one file is a problem. In fact it does cleanly solve a number of issues:

  • Migration is as easy as a symlink or just reading the old path. The file format doesn't change.
  • Solves the mythical use cases we had about merging information from multiple callers of remotectl or multiple drop ins sourced from various places.

I was a little less keen on having one big property ... but again only for the mythical use cases of having many hosts and type-head UI to filter them.

However after thinking about this I believe that if/when we do get around to implementing such use cases we would implement additional methods/properties on the API in order to filtering on the bridge side. When we do implement those cases we could simply add a second Overflow property that indicates that only some machines (eg: recently used machines) are in the Machines property. In addition we would implement a method to do the type-ahead style searching.

Because this is theoretical and backwards compatible I think it's fine to have the current API.

I do believe the PropertiesChanged needs an fix for performance. Due to the way CockpitDBusCache works, using the invalidated later half of the PropertiesChanged signature works really well for us. The property will only be loaded if there's an active consumer.

I did some review of code style as well.

// FIXME: investigate re-using the proxy from Loader (runs in different frame/scope)
var bridge = cockpit.dbus(null, { bus: "internal", "superuser": "try" });
var mod = bridge.call("/machines", "cockpit.Machines", "Update", [ "99-webui.json", host, values_variant ])

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

That's a pretty good reason. And I think one that we can live with.

A bigger decision is the implementation of multiple machines per file, rather than enforcing one machine per file. However I think you've made the right call here too.

@stefwalter

stefwalter Feb 22, 2017

Contributor

That's a pretty good reason. And I think one that we can live with.

A bigger decision is the implementation of multiple machines per file, rather than enforcing one machine per file. However I think you've made the right call here too.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
/* counts the number of file change events that have not yet gotten a PropertiesChanged signal*/
static guint pending_updates;
static const char*

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: This should have a space before *

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: This should have a space before *

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
}
static int
glob_err_func (const char *epath, int eerrno)

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: Various arguments are on their own lines prefix with spaces as indentation.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: Various arguments are on their own lines prefix with spaces as indentation.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
return n;
}
static JsonNode*

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: space before asterisk

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: space before asterisk

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
{
result = json_parser_get_root (parser);
/* root is NULL if the file is empty */
if (result != NULL) {

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: brace goes on a new line.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: brace goes on a new line.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
g_assert (JSON_NODE_HOLDS_OBJECT(cur_props));
json_object_foreach_member (json_node_get_object (info), update_machine_property, json_node_get_object (cur_props));
}
else

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: Multiline block needs braces.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: Multiline block needs braces.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
g_variant_get (parameters, "(&s&s@a{sv})", &filename, &hostname, &info_v);
info_json = json_gvariant_serialize (info_v);
/* g_debug ("Updating %s for machine %s: %s", filename, hostname, g_variant_print (info_v, TRUE)); */

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

g_debug() statements can remain uncommented. They're useful later with G_MESSAGES_DEBUG

@stefwalter

stefwalter Feb 22, 2017

Contributor

g_debug() statements can remain uncommented. They're useful later with G_MESSAGES_DEBUG

This comment has been minimized.

@martinpitt

martinpitt Feb 23, 2017

Member

I left this commented as it causes a memleak (g_variant_print() result needs to be freed and thus also needs a new variable) and it's relatively expensive. I'll uncomment it but leave out the value of the gvariant, it was mostly just necessary for developing the correct disassembling of the variant.

@martinpitt

martinpitt Feb 23, 2017

Member

I left this commented as it causes a memleak (g_variant_print() result needs to be freed and thus also needs a new variable) and it's relatively expensive. I'll uncomment it but leave out the value of the gvariant, it was mostly just necessary for developing the correct disassembling of the variant.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
/* reset pending counter before we do any actual work, to avoid races */
pending_updates = 0;
machines = get_machines ();

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

I'm not against using a property for Machines ... but we should be using the invalidated parameter of PropertiesChanged instead of automatically doing this expensive operation each time ... even if nobody's listening.

@stefwalter

stefwalter Feb 22, 2017

Contributor

I'm not against using a property for Machines ... but we should be using the invalidated parameter of PropertiesChanged instead of automatically doing this expensive operation each time ... even if nobody's listening.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
static void
on_machines_changed (GFileMonitor *monitor,

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: Function arguments aren't lined up elsewhere in the C code.

@stefwalter

stefwalter Feb 22, 2017

Contributor

Style: Function arguments aren't lined up elsewhere in the C code.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
GVariant *machines, *signal_value;
GVariantBuilder builder;
GError *error = NULL;

This comment has been minimized.

@stefwalter

stefwalter Feb 22, 2017

Contributor

We need to check if pending_updates is zero here. If it is then this should just return immediately without generating more PropertiesChanged events.

@stefwalter

stefwalter Feb 22, 2017

Contributor

We need to check if pending_updates is zero here. If it is then this should just return immediately without generating more PropertiesChanged events.

This comment has been minimized.

@martinpitt

martinpitt Feb 23, 2017

Member

on_machines_changed() already does that:

      if (pending_updates++ == 0)
        g_timeout_add (100, update_properties, user_data);

I can certainly add the check again here, but I'm afraid I'm missing what that would buy?

@martinpitt

martinpitt Feb 23, 2017

Member

on_machines_changed() already does that:

      if (pending_updates++ == 0)
        g_timeout_add (100, update_properties, user_data);

I can certainly add the check again here, but I'm afraid I'm missing what that would buy?

This comment has been minimized.

@stefwalter

stefwalter Feb 23, 2017

Contributor

Oh ok. As long as we're not scheduling multiple update_properties() timeouts.

@stefwalter

stefwalter Feb 23, 2017

Contributor

Oh ok. As long as we're not scheduling multiple update_properties() timeouts.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Feb 23, 2017

bridge: read /etc/cockpit/machines.d/*.json machine definitions
Cockpit plugins, other packages, admins, VM management software, or
config management systems like Ansible/puppet/cloud-init might want to
pre-configure machines for cockpit. This is currently racy as each of
those and cockpit itself have to open and rewrite
/var/lib/cockpit/machines.json. Also, this should be treated as
configuration, not state.

So move the remote machines configuration to /etc/cockpit/machines.d/
and read all *.json files in that directory. Later files override
settings from earlier ones, i. e. they can add new hosts, add new
properties to existing hosts, or change existing properties. The D-Bus
Update() method now expects a file name to update, and the dashboard
writes into "99-webui.json".

Pre-create /etc/cockpit/machines.d in our install and thus our packages,
as otherwise we can't watch it for changes; and the bridge that does the
watching is running as user, so it does not have the privileges to
create the directory by itself.

Adjust the integration tests to the changed paths.

Closes #5880

@martinpitt martinpitt removed the needswork label Feb 23, 2017

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 23, 2017

Member

Pushed with addressing all of Stef's comments (only invalidating the property, stylistic fixes, g_error etc.)

Member

martinpitt commented Feb 23, 2017

Pushed with addressing all of Stef's comments (only invalidating the property, stylistic fixes, g_error etc.)

stefwalter added a commit to stefwalter/cockpit that referenced this pull request Feb 24, 2017

Makefile: Fix up install-tests target
Fix up the install-tests target. And always build the
cockpit-tests RPM subpackage. We want to start shipping
real tests in here.

Closes #5880
@stefwalter

Another pass of reviews. Sorry for missing these first time around.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
if (!JSON_NODE_HOLDS_OBJECT (delta_props))
{
g_warning ("%s: host name definition %s does not contain a JSON object, ignoring", path, hostname);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from non-bridge source.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from non-bridge source.

{
/* just log the error for debugging */
if (eerrno != ENOENT)
g_warning ("%s: cannot read: %s", epath, g_strerror (eerrno));

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from a non-bridge source.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from a non-bridge source.

This comment has been minimized.

@martinpitt

martinpitt Feb 24, 2017

Member

I think this actually should be a g_warning() as it's really unexpected that glob() itself fails -- it means that someone made /etc/cockpit/machines.d/ inaccessible, so this should really stand out IMHO.

@martinpitt

martinpitt Feb 24, 2017

Member

I think this actually should be a g_warning() as it's really unexpected that glob() itself fails -- it means that someone made /etc/cockpit/machines.d/ inaccessible, so this should really stand out IMHO.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
}
else
{
g_warning ("%s: does not contain a JSON object, ignoring", path);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from a non-bridge source.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from a non-bridge source.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
else
{
if (error->code != G_FILE_ERROR_NOENT)
g_warning ("%s: invalid JSON: %s", path, error->message);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from a non-bridge source.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message() since these files can originate from a non-bridge source.

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
&error);
if (error != NULL)
{
g_critical ("failed to send PropertiesChanged signal: %s", error->message);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This can happen during shutdown when the DBus connection is closed. I don't think it's a g_critical() unless we account for this:

src/bridge/cockpitdbuscache.c:      g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED))
@stefwalter

stefwalter Feb 24, 2017

Contributor

This can happen during shutdown when the DBus connection is closed. I don't think it's a g_critical() unless we account for this:

src/bridge/cockpitdbuscache.c:      g_error_matches (error, G_IO_ERROR, G_IO_ERROR_CLOSED))
Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
if (!JSON_NODE_HOLDS_VALUE (prop_node))
{
g_warning ("%s: host name definition %s: property %s does not contain a simple value, ignoring", path, hostname, propname);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message().

@stefwalter

stefwalter Feb 24, 2017

Contributor

This should be a g_message().

Show outdated Hide outdated src/bridge/cockpitdbusmachines.c
static guint pending_updates;
static const char *
machines_json_dir (void)

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Isn't the expectation with XDG style conf dirs that we load all files from all the directories? This seems to choose the last directory and only operate on that. Am I wrong here (on either count)?

@stefwalter

stefwalter Feb 24, 2017

Contributor

Isn't the expectation with XDG style conf dirs that we load all files from all the directories? This seems to choose the last directory and only operate on that. Am I wrong here (on either count)?

This comment has been minimized.

@martinpitt

martinpitt Feb 24, 2017

Member

Agreed, will rework it to do that (makes the globbing a bit more complicated, but shrug).

@martinpitt

martinpitt Feb 24, 2017

Member

Agreed, will rework it to do that (makes the globbing a bit more complicated, but shrug).

This comment has been minimized.

@martinpitt

martinpitt Feb 24, 2017

Member

Ah, it would not only affect the globbing, but we'd also need multiple file monitors and we would still need to pick one that we use for writing into. This is a lot of production code complication just to support tests, so as discussed on IRC, I'll move this back to the original proposal of a single dir env var, but call it $COCKPIT_TEST_CONFIG_DIR in line with the existing $COCKPIT_TEST_SERVER_PORT.

@martinpitt

martinpitt Feb 24, 2017

Member

Ah, it would not only affect the globbing, but we'd also need multiple file monitors and we would still need to pick one that we use for writing into. This is a lot of production code complication just to support tests, so as discussed on IRC, I'll move this back to the original proposal of a single dir env var, but call it $COCKPIT_TEST_CONFIG_DIR in line with the existing $COCKPIT_TEST_SERVER_PORT.

Show outdated Hide outdated src/ws/test-server.c
g_setenv ("XDG_DATA_HOME", SRCDIR "/src/bridge/mock-resource/home", TRUE);
g_setenv ("XDG_DATA_DIRS", SRCDIR "/src/bridge/mock-resource/system", TRUE);
g_setenv ("XDG_CONFIG_DIRS", config_dir, TRUE);
g_setenv ("COCKPIT_DATA_DIR", data_dir, TRUE);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Is this variable added just for unit testing? If so I would rely on integration tests and not worry about yet more environment variables.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Is this variable added just for unit testing? If so I would rely on integration tests and not worry about yet more environment variables.

This comment has been minimized.

@martinpitt

martinpitt Feb 24, 2017

Member

Yes, just for verifying that /var/lib/cockpit/machines.json still gets read. But I can turn this into an integration test. I'm going to add one for adding the migration from /var to /etc anyway (as soon as this one lands), so after that this wouldn't add much extra overhead.

@martinpitt

martinpitt Feb 24, 2017

Member

Yes, just for verifying that /var/lib/cockpit/machines.json still gets read. But I can turn this into an integration test. I'm going to add one for adding the migration from /var to /etc anyway (as soon as this one lands), so after that this wouldn't add much extra overhead.

Show outdated Hide outdated src/ws/test-server.c
@@ -875,5 +888,13 @@ main (int argc,
g_free (bridge_argv);
g_free (guid);
/* clean up temporary config/data dirs */
rm_rf_argv[2] = config_dir;
g_spawn_sync(NULL, rm_rf_argv, NULL, 0, NULL, NULL, NULL, NULL, NULL, NULL);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Space needed after function name.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Space needed after function name.

Show outdated Hide outdated src/ws/test-server.c
g_spawn_sync(NULL, rm_rf_argv, NULL, 0, NULL, NULL, NULL, NULL, NULL, NULL);
g_free (config_dir);
rm_rf_argv[2] = data_dir;
g_spawn_sync(NULL, rm_rf_argv, NULL, 0, NULL, NULL, NULL, NULL, NULL, NULL);

This comment has been minimized.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Space needed after function name.

@stefwalter

stefwalter Feb 24, 2017

Contributor

Space needed after function name.

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Feb 24, 2017

bridge: read /etc/cockpit/machines.d/*.json machine definitions
Cockpit plugins, other packages, admins, VM management software, or
config management systems like Ansible/puppet/cloud-init might want to
pre-configure machines for cockpit. This is currently racy as each of
those and cockpit itself have to open and rewrite
/var/lib/cockpit/machines.json. Also, this should be treated as
configuration, not state.

So move the remote machines configuration to /etc/cockpit/machines.d/
and read all *.json files in that directory. Later files override
settings from earlier ones, i. e. they can add new hosts, add new
properties to existing hosts, or change existing properties. The D-Bus
Update() method now expects a file name to update, and the dashboard
writes into "99-webui.json".

Pre-create /etc/cockpit/machines.d in our install and thus our packages,
as otherwise we can't watch it for changes; and the bridge that does the
watching is running as user, so it does not have the privileges to
create the directory by itself.

Adjust the integration tests to the changed paths, and add a new one
that verifies that the old path still gets read (that will be extended
later to also cover the migration).

Closes #5880
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 24, 2017

Member

Re-pushed with addressing @stefwalter's points, thanks for the review! This now moves the handling of the old file in /var from a unit into an integration test which gets rid of $COCKPIT_DATA_DIR, does the warning → message downgrade for most issues (except for glob() error, see above), unrefs the file monitor at bridge shutdown (busy work IMHO, but shrug), and goes back from multi-dir $XDG_CONFIG_DIRS to single-dir $COCKPIT_TEST_CONFIG_DIR (as above).

Member

martinpitt commented Feb 24, 2017

Re-pushed with addressing @stefwalter's points, thanks for the review! This now moves the handling of the old file in /var from a unit into an integration test which gets rid of $COCKPIT_DATA_DIR, does the warning → message downgrade for most issues (except for glob() error, see above), unrefs the file monitor at bridge shutdown (busy work IMHO, but shrug), and goes back from multi-dir $XDG_CONFIG_DIRS to single-dir $COCKPIT_TEST_CONFIG_DIR (as above).

changes made

@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 26, 2017

Member

The atomic failures seem real:

$ test/verify/check-docker-storage TestDockerStorage.testDevmapper
[...]
Error: failed to call cockpit.Machines.Update():  Failed to create file '/etc/cockpit/machines.d/99-webui.json.1T45VY': No such file or directory

$ test/verify/check-multi-machine-key
[...]
Error: failed to call cockpit.Machines.Update():  Failed to create file '/etc/cockpit/machines.d/99-webui.json.U2SNWY': No such file or directory

I'll fix those.

test/verify/check-shutdown-restart fails as well, I suppose for a similar reason.

Member

martinpitt commented Feb 26, 2017

The atomic failures seem real:

$ test/verify/check-docker-storage TestDockerStorage.testDevmapper
[...]
Error: failed to call cockpit.Machines.Update():  Failed to create file '/etc/cockpit/machines.d/99-webui.json.1T45VY': No such file or directory

$ test/verify/check-multi-machine-key
[...]
Error: failed to call cockpit.Machines.Update():  Failed to create file '/etc/cockpit/machines.d/99-webui.json.U2SNWY': No such file or directory

I'll fix those.

test/verify/check-shutdown-restart fails as well, I suppose for a similar reason.

@stefwalter

This comment has been minimized.

Show comment
Hide comment
@stefwalter

stefwalter Feb 26, 2017

Contributor

In addition to g_mkdir_with_parents this likely also needs /etc/cockpit/machines.d/ to be owned by the cockpit-bridge subpackage, not cockpit-ws.

Contributor

stefwalter commented Feb 26, 2017

In addition to g_mkdir_with_parents this likely also needs /etc/cockpit/machines.d/ to be owned by the cockpit-bridge subpackage, not cockpit-ws.

martinpitt added some commits Feb 8, 2017

bridge: Add /machines object to represent/update machines.json
As a prerequisite for reorganizing machines.json, add a new /machines
D-Bus object to the bridge which enumerates the configured machines in a
"Machines" property and offers an Update method to add/update properties
of a machine.

Add new test-machines.js test to cover the new D-Bus interface:
translating file contents into the D-Bus property, updating properties,
and adding hosts.

The "invalid data types" test triggers a memleak in json-glib
(https://bugzilla.gnome.org/show_bug.cgi?id=778674), so add a
suppression file for it.
lib: Move from direct machines.json manipulation to D-Bus calls
Use the bridge's new /machines D-Bus object to get/update information
about machines. This abstracts away the configuration format from the
frontend, and allows the bridge to move to more elaborate config storage
such as merging/updating an entire tree of json files.

Drop the obsolete tracking of the cockpit.file tag of machines.json.
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 26, 2017

Member

Right. I need the directory pre-created in the package as otherwise we can't watch it for changes, and the (initially) unprivileged bridge can't create it either. I even mentioned that in the commit log, but forgot to put the new dir into the package, duh..

Member

martinpitt commented Feb 26, 2017

Right. I need the directory pre-created in the package as otherwise we can't watch it for changes, and the (initially) unprivileged bridge can't create it either. I even mentioned that in the commit log, but forgot to put the new dir into the package, duh..

martinpitt added a commit to martinpitt/cockpit that referenced this pull request Feb 26, 2017

bridge: read /etc/cockpit/machines.d/*.json machine definitions
Cockpit plugins, other packages, admins, VM management software, or
config management systems like Ansible/puppet/cloud-init might want to
pre-configure machines for cockpit. This is currently racy as each of
those and cockpit itself have to open and rewrite
/var/lib/cockpit/machines.json. Also, this should be treated as
configuration, not state.

So move the remote machines configuration to /etc/cockpit/machines.d/
and read all *.json files in that directory. Later files override
settings from earlier ones, i. e. they can add new hosts, add new
properties to existing hosts, or change existing properties. The D-Bus
Update() method now expects a file name to update, and the dashboard
writes into "99-webui.json".

Pre-create /etc/cockpit/machines.d in our install and thus our packages,
as otherwise we can't watch it for changes; and the bridge that does the
watching is running as user, so it does not have the privileges to
create the directory by itself.

Adjust the integration tests to the changed paths, and add a new one
that verifies that the old path still gets read (that will be extended
later to also cover the migration).

Closes #5880
@martinpitt

This comment has been minimized.

Show comment
Hide comment
@martinpitt

martinpitt Feb 26, 2017

Member

Fixed with this relative diff, the fedora-atomic tests now pass for me locally.

Member

martinpitt commented Feb 26, 2017

Fixed with this relative diff, the fedora-atomic tests now pass for me locally.

bridge: read /etc/cockpit/machines.d/*.json machine definitions
Cockpit plugins, other packages, admins, VM management software, or
config management systems like Ansible/puppet/cloud-init might want to
pre-configure machines for cockpit. This is currently racy as each of
those and cockpit itself have to open and rewrite
/var/lib/cockpit/machines.json. Also, this should be treated as
configuration, not state.

So move the remote machines configuration to /etc/cockpit/machines.d/
and read all *.json files in that directory. Later files override
settings from earlier ones, i. e. they can add new hosts, add new
properties to existing hosts, or change existing properties. The D-Bus
Update() method now expects a file name to update, and the dashboard
writes into "99-webui.json".

Pre-create /etc/cockpit/machines.d in our install and thus our packages,
as otherwise we can't watch it for changes; and the bridge that does the
watching is running as user, so it does not have the privileges to
create the directory by itself.

Adjust the integration tests to the changed paths, and add a new one
that verifies that the old path still gets read (that will be extended
later to also cover the migration).

Closes #5880

@martinpitt martinpitt removed the needswork label Feb 26, 2017

stefwalter added a commit that referenced this pull request Feb 27, 2017

lib: Move from direct machines.json manipulation to D-Bus calls
Use the bridge's new /machines D-Bus object to get/update information
about machines. This abstracts away the configuration format from the
frontend, and allows the bridge to move to more elaborate config storage
such as merging/updating an entire tree of json files.

Drop the obsolete tracking of the cockpit.file tag of machines.json.

Closes #5880
Reviewed-by: Stef Walter <stefw@redhat.com>

stefwalter added a commit that referenced this pull request Feb 27, 2017

bridge: read /etc/cockpit/machines.d/*.json machine definitions
Cockpit plugins, other packages, admins, VM management software, or
config management systems like Ansible/puppet/cloud-init might want to
pre-configure machines for cockpit. This is currently racy as each of
those and cockpit itself have to open and rewrite
/var/lib/cockpit/machines.json. Also, this should be treated as
configuration, not state.

So move the remote machines configuration to /etc/cockpit/machines.d/
and read all *.json files in that directory. Later files override
settings from earlier ones, i. e. they can add new hosts, add new
properties to existing hosts, or change existing properties. The D-Bus
Update() method now expects a file name to update, and the dashboard
writes into "99-webui.json".

Pre-create /etc/cockpit/machines.d in our install and thus our packages,
as otherwise we can't watch it for changes; and the bridge that does the
watching is running as user, so it does not have the privileges to
create the directory by itself.

Adjust the integration tests to the changed paths, and add a new one
that verifies that the old path still gets read (that will be extended
later to also cover the migration).

Closes #5880
Reviewed-by: Stef Walter <stefw@redhat.com>

@martinpitt martinpitt deleted the martinpitt:machines branch Feb 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment