bridge: Migrate /var/lib/cockpit/machines.json to /etc #5963

martinpitt · 2017-02-27T07:16:14Z

On startup, migrate the old machines.json file in /var to the new canonical location /etc/cockpit/machines.d/99-webui.json, so that the entire configuration is in the one documented place. Much of the code tries to handle the (rather unlikely) case that both the old /var/lib/cockpit/machines.json and the new /etc/cockpit/machines.d/99-webui.json already exist; this could happen when upgrading to the new version, the initial migration fails due to some weird permissions error (directory not writable by root), and then the user adds some new hosts. This can never be made fully robust by nature, but the code tries to cope as well as possible. Maybe this is also overengineered and we should simply not support that case, not sure.

We still need to keep reading the file in /var for some time as there is no guarantee that the migration actually works.

The second commit documents the file format. You can see what the generated result looks like at http://piware.de/tmp/feature-machines.html, except that there is a light blue box missing around the example (I didn't want to spend too much time figuring out which CSS to put where).

stefwalter · 2017-02-27T08:11:04Z

What is the goal of migrating? If the goal is documentation and discoverability, would just trying to create a symlink to /etc/cockpit/machines/95-migrated.json -> /var/lib/cockpit/machines.json accomplish the same purpose? Such a link could even be installed by newer bridge packages.

martinpitt · 2017-02-27T08:22:39Z

The goal is to have all the actual config files in the documented place, /etc/cockpit/machines.d/. We currently don't have a second GFileMonitor to watch for changes in /var (we could add that if necessary, though), but more importantly, an admin who backs up /etc/cockpit/ will miss half of their configuration. I was hoping that in a few releases we could actually drop the reading of the old file in /var/ (but that's of course a bit handwavy).

martinpitt · 2017-02-27T15:28:49Z

I think we can combine the ideas to simplify this:

If we can move /var/lib/cockpit/machines.json to /etc/cockpit/machines.d/99-webui.json (i. e. the latter doesn't exist yet and the dir is writable), we continue to do so. This is the 99% case, and it will result in a "nice" and legacy free file layout. This also avoids having a file in /var which will never change again, and an ever-growing delta against that in 99-webui.json.
If moving the file fails because 99-webui.json already exists, just create a 95-migrated.json -> /var/lib/cockpit/machines.json symlink. This is already a small corner case, and as nothing writes the file in /var any more we can still get away without GFileMonitor'ing it. The code for this would be a lot simpler than the merging fallback here.
If both of the above fail because we can't create/write into /etc/cockpit/machines.d/, we have some options:

Just print a g_warning() and give up -- we just don't support a non-writable /etc/cockpit/, we also need it for ws-certs.d/ and possibly other things. The user would lose the machines defined in /var.
Similar to 1., but keep the additional reading of /var/lib/cockpit/machines.json for a few months (or e. g. until after RHEL 7.4 got released), until the migration should have run pretty much everywhere.
Keep the additional reading of /var/lib/cockpit/machines.json for all eternity.

My gut feeling is that 2. is the right approach, this is what I was heading towards in this PR.

@stefwalter , opinions?

martinpitt · 2017-02-28T08:39:32Z

If moving the file fails because 99-webui.json already exists, just create a 95-migrated.json -> /var/lib/cockpit/machines.json symlink.

Actually, I think it's better to move the file rather than symlinking:

it will then be more regular and backup systems will actually contain the file contents rather than just the link
we can then remove the file in /var, so that we don't read it twice (first directly from the backwards compat code path, and then again as part of machines.d/)
we can cleanly finish the transition by removing the file in /var, or roll back by not changing anything at all.

martinpitt · 2017-02-28T09:01:12Z

Reworked like discussed above; this is simpler and more robust.

martinpitt · 2017-02-28T09:11:54Z

I added another commit on top to fix the unit tests when /var/lib/cockpit/machines.json exists (what @stefwalter pointed out yesterday).

martinpitt · 2017-02-28T15:22:22Z

@larskarlitski found out in the meeting that current master's dashboard doesn't work with the latest released bridge (error message about missing machines.Update() D-Bus function). I bumped the dependency now. During that I noticed that tools/min-base-version was broken and returned 0, due to the renaming of manifest.json to manifest.json.in. Fixed in this PR as well; let me know if you want a separate PR for the two topmost commits.

stefwalter · 2017-03-01T07:56:23Z

src/bridge/cockpitdbusmachines.c

+        }
+      else /* different g_file_move() error than EXISTS */
+        {
+          g_warning ("migration of %s to %s failed: %s", var_path, etc_path, error->message);


Does this fail to EPERM or EACCESS? If so then this is not a g_warning() ... in fact we should probably just ignore that failure quietly.

In particular, the caller only checks if we can access the destination, and no check whether we can remove stuff from the source.

At this point we already asserted g_access (machines_json_dir (), W_OK) (from the call below), so this could happen if /var/lib/cockpit/ isn't writable. So this really is a Should Not Happen™ situation, and if someone gets here they wedged up their system. So IMHO g_warning() is not too strong at all.

stefwalter · 2017-03-01T07:58:25Z

tools/min-base-version

@@ -28,14 +28,14 @@ from glob import glob
 proj_dir = os.path.dirname(os.path.dirname(os.path.realpath(sys.argv[0])))

 max_version = '0'
-for manifest in glob(os.path.join(proj_dir, 'pkg/*/manifest.json')):
+for manifest in glob(os.path.join(proj_dir, 'pkg/*/manifest.json.in')):


This is probably a follow up but we cannot conflate all the "requires" here. Certain distros don't ship all the packages at the same time. We need to start doing these individually or manually curating these. Such a change could be a follow up.

Right, let's handle that in a separate PR. Right now the dependency calculation is completely broken, so one way or another we need to fix this for 133.

tought about this in issue #5997, and I think I have a reasonably good solution for this (at least much better than what we have now). So I think this particular hunk could stay, but I'm happy to revert the bumping of the dependency in pkg/dashboard/manifest.json.in (i. e. remove the topmost commit here) in this PR, as it will not actually influence the dependency of -dashboard (that doesn't use the manifest dependencies, it's just hardcoded at "same version as -ws") and unecessarily bump the dependency of all "plugin" packages.

This should alleviate the dependency concerns but still keep the unbreaking of min-base-version.

martinpitt · 2017-03-01T11:10:23Z

Re-pushed with dropping the depenency bump and g_warning() → g_message()` and fixing the writability test.

Otherwise an existing /var/lib/cockpit/machines.json would fail the tests.

We moved machines.json into /etc and a directory because we want it to be a public API. Don't document the "avatar" property for now, as it does not really work right now.

martinpitt · 2017-03-01T12:46:23Z

Rebased to adjust to changes from #5990 (dropping inject_extras()).

martinpitt · 2017-03-01T13:14:43Z

Priority, this should be included in 133, otherwise the migration will be more complicated/less clean.

On startup, migrate the old machines.json file in /var to the new canonical location /etc/cockpit/machines.d/99-webui.json, so that the entire configuration is in the one documented place. If that already exists (unlikely, but maybe a previous migration attempt failed and then the user manually added a new machine), move it to 98-migrated.json instead. We still keep reading the file in /var for some time, to ensure the migration has run for everyone. Closes cockpit-project#5963

martinpitt · 2017-03-01T13:50:09Z

Adjusted once again, older glib versions (debian-8/centos-7) have a different error message, which caused a mismatch in the expected journal messages. I made this more general now.

martinpitt force-pushed the machines-migration branch from 09118ff to cb5e93f Compare February 27, 2017 07:17

stefwalter added the question Further information is requested label Feb 27, 2017

martinpitt force-pushed the machines-migration branch from cb5e93f to aad9a3e Compare February 28, 2017 09:00

martinpitt removed the question Further information is requested label Feb 28, 2017

martinpitt force-pushed the machines-migration branch 2 times, most recently from 992f53d to 305781f Compare February 28, 2017 16:38

martinpitt mentioned this pull request Mar 1, 2017

test: Avoid racily injecting code in dashboard tests #5990

Closed

stefwalter suggested changes Mar 1, 2017

View reviewed changes

stefwalter added the needswork label Mar 1, 2017

martinpitt force-pushed the machines-migration branch from 305781f to 621f378 Compare March 1, 2017 11:09

martinpitt removed the needswork label Mar 1, 2017

martinpitt force-pushed the machines-migration branch from 621f378 to 349f55d Compare March 1, 2017 11:17

martinpitt force-pushed the machines-migration branch from 349f55d to 0935dc3 Compare March 1, 2017 11:18

stefwalter approved these changes Mar 1, 2017

View reviewed changes

martinpitt added 2 commits March 1, 2017 13:40

bridge: Don't read /var/lib/cockpit/machines.json during unit tests

ee0958d

Otherwise an existing /var/lib/cockpit/machines.json would fail the tests.

doc: Document machine JSON format

bf86859

We moved machines.json into /etc and a directory because we want it to be a public API. Don't document the "avatar" property for now, as it does not really work right now.

martinpitt force-pushed the machines-migration branch from 0935dc3 to f545f8b Compare March 1, 2017 12:46

martinpitt added the release-blocker Targetted for next release label Mar 1, 2017

martinpitt force-pushed the machines-migration branch from f545f8b to 986eb08 Compare March 1, 2017 13:43

martinpitt force-pushed the machines-migration branch from 986eb08 to 7f891d9 Compare March 1, 2017 13:49

stefwalter closed this in eae85c6 Mar 1, 2017

martinpitt deleted the machines-migration branch March 1, 2017 21:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bridge: Migrate /var/lib/cockpit/machines.json to /etc #5963

bridge: Migrate /var/lib/cockpit/machines.json to /etc #5963

martinpitt commented Feb 27, 2017

stefwalter commented Feb 27, 2017

martinpitt commented Feb 27, 2017

martinpitt commented Feb 27, 2017

martinpitt commented Feb 28, 2017

martinpitt commented Feb 28, 2017

martinpitt commented Feb 28, 2017

martinpitt commented Feb 28, 2017

stefwalter Mar 1, 2017

stefwalter Mar 1, 2017

martinpitt Mar 1, 2017

stefwalter Mar 1, 2017

martinpitt Mar 1, 2017

martinpitt Mar 1, 2017

martinpitt commented Mar 1, 2017

martinpitt commented Mar 1, 2017

martinpitt commented Mar 1, 2017

martinpitt commented Mar 1, 2017

Navigation Menu

bridge: Migrate /var/lib/cockpit/machines.json to /etc #5963

bridge: Migrate /var/lib/cockpit/machines.json to /etc #5963

Conversation

martinpitt commented Feb 27, 2017

stefwalter commented Feb 27, 2017

martinpitt commented Feb 27, 2017

martinpitt commented Feb 27, 2017

martinpitt commented Feb 28, 2017

martinpitt commented Feb 28, 2017

martinpitt commented Feb 28, 2017

martinpitt commented Feb 28, 2017

stefwalter Mar 1, 2017

Choose a reason for hiding this comment

stefwalter Mar 1, 2017

Choose a reason for hiding this comment

martinpitt Mar 1, 2017

Choose a reason for hiding this comment

stefwalter Mar 1, 2017

Choose a reason for hiding this comment

martinpitt Mar 1, 2017

Choose a reason for hiding this comment

martinpitt Mar 1, 2017

Choose a reason for hiding this comment

martinpitt commented Mar 1, 2017

martinpitt commented Mar 1, 2017

martinpitt commented Mar 1, 2017

martinpitt commented Mar 1, 2017