Project

General

Profile

Actions

Idea #9953

closed

dockercleaner config file & systemd unit

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

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Story points:
0.5

Subtasks 2 (0 open2 closed)

Task #10049: config fileResolvedTom Clegg09/23/2016Actions
Task #10070: Review 9953-dockercleaner-configResolvedNico César09/28/2016Actions
Actions #1

Updated by Peter Amstutz over 7 years ago

  • Tracker changed from Bug to Idea
Actions #2

Updated by Tom Morris over 7 years ago

  • Target version set to 2016-09-28 sprint
Actions #3

Updated by Tom Clegg over 7 years ago

  • Assigned To set to Tom Clegg
Actions #4

Updated by Tom Clegg over 7 years ago

  • Status changed from New to In Progress
Actions #5

Updated by Tom Clegg over 7 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.

Actions #6

Updated by Nico César over 7 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?

Actions #7

Updated by Nico César over 7 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?

Actions #8

Updated by Nico César over 7 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 '/' 

...?

Actions #9

Updated by Tom Clegg over 7 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

Actions #10

Updated by Nico César over 7 years ago

test 30fb9bf

LGTM ready to merge

Actions #11

Updated by Radhika Chippada over 7 years ago

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

Updated by Tom Clegg over 7 years ago

  • Story points set to 0.5
Actions #13

Updated by Tom Clegg over 7 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:9cb3552d964d162d6eacff40435650440bc6f692.

Actions

Also available in: Atom PDF