Merge lp:~hazmat/pyjuju/var-run-mass-restart into lp:pyjuju

[Kapil Thangavelu]

Agents play nice on run dir race.

Previously a reboot of several agents could cause a race around
the creation of the /var/run/juju dir, even with an existence check,
instead just try/except and handle win or lose the race.

https://codereview.appspot.com/8583048/

[Kapil Thangavelu (hazmat)]

Reviewers: mp+158980_code.launchpad.net,

Message:
Please take a look.

Description:
Agents play nice on run dir race.

Previously a reboot of several agents could cause a race around
the creation of the /var/run/juju dir, even with an existence check,
instead just try/except and handle win or lose the race.

https://code.launchpad.net/~hazmat/juju/var-run-mass-restart/+merge/158980

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8583048/

Affected files:
   A [revision details]
   M juju/agents/base.py

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: juju/agents/base.py
=== modified file 'juju/agents/base.py'
--- juju/agents/base.py 2013-02-01 16:53:27 +0000
+++ juju/agents/base.py 2013-04-15 17:20:47 +0000
@@ -32,8 +32,14 @@

  def save_client_id(path, client_id):
      parent = os.path.dirname(path)
- if not os.path.exists(parent):
+ # On reboot the run dir is wiped, with multiple agents
+ # we get a race trying to create the directory, hence try/except
+ # instead of checking to see if dir exists first.
+ try:
          os.makedirs(parent)
+ except OSError, e:
+ if e.errno != 17:
+ raise
      with open(path, "w") as f:
          f.write(yaml.dump(client_id))
      os.chmod(path, stat.S_IRUSR | stat.S_IWUSR)

[Benjamin Saller (bcsaller)]

LGTM w/trivial

https://codereview.appspot.com/8583048/diff/1/juju/agents/base.py
File juju/agents/base.py (right):

https://codereview.appspot.com/8583048/diff/1/juju/agents/base.py#newcode41
juju/agents/base.py:41: if e.errno != 17:
Could you use errno.EEXIST here rather than the magic number?

https://codereview.appspot.com/8583048/

[Kapil Thangavelu (hazmat)]

done thanks.

On Mon, Apr 15, 2013 at 10:55 AM, Benjamin Saller <
<email address hidden>> wrote:

> LGTM w/trivial
>
>
> https://codereview.appspot.com/8583048/diff/1/juju/agents/base.py
> File juju/agents/base.py (right):
>
> https://codereview.appspot.com/8583048/diff/1/juju/agents/base.py#newcode41
> juju/agents/base.py:41: if e.errno != 17:
> Could you use errno.EEXIST here rather than the magic number?
>
> https://codereview.appspot.com/8583048/
>
> --
> https://code.launchpad.net/~hazmat/juju/var-run-mass-restart/+merge/158980
> You are the owner of lp:~hazmat/juju/var-run-mass-restart.
>

[Kapil Thangavelu (hazmat)]

*** Submitted:

Agents play nice on run dir race.

Previously a reboot of several agents could cause a race around
the creation of the /var/run/juju dir, even with an existence check,
instead just try/except and handle win or lose the race.

R=bcsaller
CC=
https://codereview.appspot.com/8583048

https://codereview.appspot.com/8583048/

=== modified file 'juju/agents/base.py'
--- juju/agents/base.py	2013-02-01 16:53:27 +0000
+++ juju/agents/base.py	2013-04-15 17:23:28 +0000
@@ -32,8 +32,14 @@
 
 def save_client_id(path, client_id):
     parent = os.path.dirname(path)
-    if not os.path.exists(parent):
+    # On reboot the run dir is wiped, with multiple agents
+    # we get a race trying to create the directory, hence try/except
+    # instead of checking to see if dir exists first.
+    try:
         os.makedirs(parent)
+    except OSError, e:
+        if e.errno != 17:
+            raise
     with open(path, "w") as f:
         f.write(yaml.dump(client_id))
     os.chmod(path, stat.S_IRUSR | stat.S_IWUSR)