Story #9953

dockercleaner config file & systemd unit

Added by Peter Amstutz over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
09/23/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Subtasks

Task #10049: config fileResolvedTom Clegg

Task #10070: Review 9953-dockercleaner-configResolvedNico César

Associated revisions

Revision 9cb3552d
Added by Tom Clegg over 3 years ago

Merge branch '9953-dockercleaner-config' closes #9953

History

#1 Updated by Peter Amstutz over 3 years ago

  • Tracker changed from Bug to Story

#2 Updated by Tom Morris over 3 years ago

  • Target version set to 2016-09-28 sprint

#3 Updated by Tom Clegg over 3 years ago

  • Assigned To set to Tom Clegg

#4 Updated by Tom Clegg over 3 years ago

  • Status changed from New to In Progress

#5 Updated by Tom Clegg over 3 years ago

This branch ended up with some unexpected tangential improvements.

90b70c2 9953: Update build scripts to Go 1.7.1.
→ Comes with some performance improvements.

9b53747 9953: Install python data files to /usr/share/... not /usr/data/share/...
→ Mostly this just affects where license files get installed. It also affects libpam-arvados, which now comes with two copies of the relevant file: new /usr/lib/security/libpam_arvados.py and old /usr/data/lib/security/libpam_arvados.py. We can drop the old one after existing sites have had a chance to update their configs.

d602a34 9953: Ignore non-container events (volume, network) and events with no status, instead of crashing.
→ Even though we specify an older API, new versions of docker emit some events that could crash docker-cleaner (e.g., by not having a "status" key) or should have been ignored (e.g., "Type" indicates it's not a container event). These events are ignored now.

#6 Updated by Nico César over 3 years ago

Tom Clegg wrote:

This branch ended up with some unexpected tangential improvements.

90b70c2 9953: Update build scripts to Go 1.7.1.
→ Comes with some performance improvements.

does it work well in every distro?

#7 Updated by Nico César over 3 years ago

test 4d5de0e464c1de110de46588f3193c4677ac719c

there is the following comment on the scripts

+# NOTE: This package name detection will only work on Debian.
+# If this prerm script ever starts doing work on Red Hat,
+# we'll need to adapt this code accordingly.

is that relevant anymore?

#8 Updated by Nico César over 3 years ago

if there is a syntax error in the config

+    with open(args.config, 'r') as f:
+        config.update(json.load(f))

will make an exception

+def main(arguments=sys.argv[1:]):
+    config = load_config(arguments)
+    setup_logging(config)
+    try:
+        run(config, docker.Client(version='1.14'))
+    except KeyboardInterrupt:
+        sys.exit(1)

and a traceback blows up in your face

How hard is to make a human readable error like...

Error in '/etc/arvados/foo/foo.json' line 39: unexpected character '/' 

...?

#9 Updated by Tom Clegg over 3 years ago

AFAIK the Go 1.7 improvements are distro-agnostic, and I'm assuming it still builds distro-agnostic binaries as usual. (Not sure which point you are wondering about.)

AFAIK the prerm/postinst scripts are still as Debian-specific as they were when we added that comment (they assume $0 corresponds to the package name).

Eliminated stack trace for expected config-loading errors like ENOEXIST, EPERM, and json-parsing:

$ arvados-docker-cleaner --config <(echo '{"true": falsey}')
error reading config file /dev/fd/63: Expecting ',' delimiter: line 1 column 15 (char 14)
$ echo $?
1

30fb9bf

#10 Updated by Nico César over 3 years ago

test 30fb9bf

LGTM ready to merge

#11 Updated by Radhika Chippada over 3 years ago

  • Target version changed from 2016-09-28 sprint to 2016-10-12 sprint

#12 Updated by Tom Clegg over 3 years ago

  • Story points set to 0.5

#13 Updated by Tom Clegg over 3 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:9cb3552d964d162d6eacff40435650440bc6f692.

Also available in: Atom PDF