Merge lp:~vila/ubuntu-ci-services-itself/1289273-racy-config-file into lp:ubuntu-ci-services-itself

[Vincent Ladeuil]

Avoid races for rabbit mq config file by renaming the file once it's complete.

This fixes the race encountered around the rabbit worker config file.

I suspect the same kind of race occurred for the service file so I applied the same fix (Andy, what do you think ?).

I've been unable to setup a deployment for the last hours so I'm proposing anyway, if you have a working deployment...

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:338
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~vila/ubuntu-ci-services-itself/1289273-racy-config-file/+merge/209940/+edit-commit-message

http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/327/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/327/rebuild

review: Needs Fixing (continuous-integration)

[Andy Doan (doanac)]

It seems sane. Lets try and deploy it once though. The removal of the f.sync calls makes me nervous. I added that originally because I saw a bug. Its not the first time I've seen python code not seem to *really* close/flush a file to disk in time for an immediate call to subprocess that requires the file:

 http://git.linaro.org/lava/lava-dispatcher.git/blob/HEAD:/lava_dispatcher/device/sdmux.py#l53

Anyway - i think its sane, but I'd like to see it deploy for us

[lp:~vila/ubuntu-ci-services-itself/1289273-racy-config-file]

339. By Vincent Ladeuil on 2014-03-07

Keep the temp file ! os.fsync() restored, witnessed a 'No such file or directory' on the following os.rename() in 1 of the three deployed workers...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:339
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/332/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/332/rebuild

review: Approve (continuous-integration)

[lp:~vila/ubuntu-ci-services-itself/1289273-racy-config-file]

340. By Vincent Ladeuil on 2014-03-07

Re-remove os.fsync() as previous tests were broken.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:340
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/336/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/336/rebuild

review: Approve (continuous-integration)

[lp:~vila/ubuntu-ci-services-itself/1289273-racy-config-file]

341. By Vincent Ladeuil on 2014-03-08

juju status accepts regexps \o/

[Vincent Ladeuil (vila)]

I've now completed 2 successful full deployments without any errors.

On top of that, I've done 3 terminate-machine --force (all workers) / deploy.py (same code, so safe) runs without errors.

@Andy: I'm now convinced the patch does the right thing. The failures I saw on Friday were a different kind of race introduced by stupidly not using delete=False.

What happened with delete=True (the default) is that it succeeded "often" because the rename succeeded before the os really removed the tmp file.

[Vincent Ladeuil (vila)]

@Andy: I still keep my eyes opened about the service file.

[Evan (ev)]

82 + os.rename(f.name, path)

os.unlink(f.name)?

Otherwise looks good. +1

review: Approve

=== modified file 'TRICKS'
--- TRICKS	2014-02-28 19:22:57 +0000
+++ TRICKS	2014-03-08 10:02:17 +0000
@@ -225,3 +225,37 @@
 out, which will cause Juju to fail mid-deployment.
 
 $ for x in $(nova secgroup-list | tail -n +5 |awk '{ print $2 }' | grep -v '^$'); do nova secgroup-delete $x; done
+
+Making juju status useful again
+===============================
+
+Our deployment is now too big to use 'watch -n 5 juju status', but 'juju status' is smart:
+
+$ juju status bsb-worker
+environment: hp
+machines:
+  "21":
+    agent-state: started
+    agent-version: 1.17.4
+    dns-name: 15.125.100.252
+    instance-id: 305ddc5b-aa77-4d1f-9087-780f8335f6ba
+    instance-state: ACTIVE
+    series: precise
+    hardware: arch=amd64 cpu-cores=1 mem=1024M root-disk=10240M
+services:
+  bsb-worker:
+    charm: local:precise/rabbitmq-worker-0
+    exposed: false
+    relations:
+      amqp:
+      - rabbit
+    units:
+      bsb-worker/3:
+        agent-state: started
+        agent-version: 1.17.4
+        machine: "21"
+        public-address: 15.125.100.252
+
+So 'watch -n 5 juju status <regexp>' can be use to monitor a subset of the
+deployment and is useful learning tool about how machines, units and
+services comes up.

=== modified file 'charms/precise/rabbitmq-worker/hooks/hooks.py'
--- charms/precise/rabbitmq-worker/hooks/hooks.py	2014-03-04 16:12:49 +0000
+++ charms/precise/rabbitmq-worker/hooks/hooks.py	2014-03-08 10:02:17 +0000
@@ -6,6 +6,7 @@
 import shutil
 import subprocess
 import sys
+import tempfile
 import textwrap
 import charmhelpers.fetch
 
@@ -137,20 +138,26 @@
         'uid': config.get('uid', 'nobody'),
         'gid': config.get('gid', 'nogroup'),
     }
-    with open('/etc/init/%s.conf' % _service_name(config), 'w') as f:
+    path = '/etc/init/{}.conf'.format(_service_name(config))
+    with tempfile.NamedTemporaryFile('w', delete=False) as f:
         f.write(template.format(**params))
-        # need this so the file is ready for the subprocess call
-        f.flush()
-        os.fsync(f.fileno())
+    # Rename to final destination
+    os.rename(f.name, path)
 
 
 def _create_amqp_config(config):
-    with open(os.path.join(_service_dir(config), 'amqp_config.py'), 'w') as f:
+    path = os.path.join(_service_dir(config), 'amqp_config.py')
+    with tempfile.NamedTemporaryFile('w', delete=False) as f:
         f.write('# DO NOT EDIT. Generated by restish charm hook\n')
         f.write('AMQP_USER = "%s"\n' % config['amqp-user'])
         f.write('AMQP_VHOST = "%s"\n' % config['amqp-vhost'])
         f.write('AMQP_HOST = "%s"\n' % _relation_get('private-address'))
         f.write('AMQP_PASSWORD = "%s"\n' % _relation_get('password'))
+    # Now that the file is complete (and closed) rename it to its final
+    # destination. This avoids races where another process try to import that
+    # file as a python module (creating a .pyc) and ends up having close enough
+    # time stamps to fool python (which won't reload the completed file).
+    os.rename(f.name, path)
 
 
 def amqp_relation_joined(config):

=== modified file 'test_runner/tstrun/__init__.py'
--- test_runner/tstrun/__init__.py	2014-02-28 09:25:47 +0000
+++ test_runner/tstrun/__init__.py	2014-03-08 10:02:17 +0000
@@ -16,6 +16,9 @@
 import os
 import yaml
 
+
+HERE = os.path.abspath(os.path.dirname(__file__))
+
 # FIXME: Everything below sounds like config material, may be they should just
 # be moved into their own config.py file. -- vila 2014-02-05
 
@@ -27,7 +30,7 @@
 # data store require nova credentials.
 def get_auth_config(path=None):
     if path is None:
-        path = os.path.join(os.path.dirname(__file__), '../../unit_config')
+        path = os.path.abspath(os.path.join(HERE, '../../unit_config'))
     with open(path) as f:
         config = yaml.safe_load(f)
     return config