Merge lp:~bcsaller/pyjuju/lxc-killpid into lp:pyjuju

[Benjamin Saller]

Properly destroy local provider environment

Stop using temp dir to hold pid
Properly pass juju-directory on destory-environment

https://codereview.appspot.com/6460118/

[Benjamin Saller (bcsaller)]

Reviewers: mp+120497_code.launchpad.net,

Message:
Please take a look.

Description:
Properly destroy local provider environment

Stop using temp dir to hold pid
Properly pass juju-directory on destory-environment

https://code.launchpad.net/~bcsaller/juju/lxc-killpid/+merge/120497

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M juju/lib/service.py
   M juju/providers/local/__init__.py
   M juju/providers/local/agent.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/lib/service.py
=== modified file 'juju/lib/service.py'
--- juju/lib/service.py 2012-08-03 12:28:29 +0000
+++ juju/lib/service.py 2012-08-21 04:52:21 +0000
@@ -8,14 +8,18 @@

  def _check_call(args, env=None, output_path=None):
- if not output_path:
- output_path = os.devnull
+ if not output_path or not os.access(output_path, os.W_OK):
+ f = open(os.devnull, "a")
+ else:
+ f = open(output_path, "a")

- with open(output_path, "a") as f:
+ try:
          return subprocess.check_call(
              args,
              stdout=f.fileno(), stderr=f.fileno(),
              env=env)
+ finally:
+ f.close()

  def _cat(filename, use_sudo=False):
@@ -43,20 +47,11 @@
          self._description = None
          self._environ = None
          self._command = None
- self._output_path = None
+ self.output_path = None

          self._pid_path = pidfile
          self._pid = None

- @property
- def output_path(self):
- if self._output_path is not None:
- return self._output_path
- return "/tmp/%s.output" % self._name
-
- @output_path.setter
- def output_path(self, path):
- self._output_path = path

      def set_description(self, description):
          self._description = description
@@ -78,7 +73,7 @@

      @inlineCallbacks
      def _trash_output(self):
- if os.path.exists(self.output_path):
+ if self.output_path and os.path.exists(self.output_path):
              # Just using os.unlink will fail when we're running TEST_SUDO
              # tests which hit this code path (because root will own
              # self.output_path)
@@ -115,14 +110,12 @@
      def destroy(self):
          if (yield self.is_running()):
              yield self._call("kill", self.get_pid())
- yield self._trash_output()
+ yield self._trash_output()

      def get_pid(self):
          if self._pid != None:
              return self._pid

- if not os.path.exists(self._pid_path):
- return None
          r, data = _cat(self._pid_path, use_sudo=self._use_sudo)
          if r != 0:
              return None

Index: juju/providers/local/__init__.py
=== modified file 'juju/providers/local/__init__.py'
--- juju/providers/local/__init__.py 2012-07-20 17:28:17 +0000
+++ juju/providers/local/__in...

[Kapil Thangavelu (hazmat)]

On 2012/08/21 05:00:11, bcsaller wrote:
> Please take a look.

this was one of the original review items of the previous branch.
https://codereview.appspot.com/6404052/patch/2001/3004

in general please wait till the branch is actually marked approved
instead of relying on +1/lgtm in a review before merging.

https://codereview.appspot.com/6460118/

[Kapil Thangavelu (hazmat)]

https://codereview.appspot.com/6460118/diff/1/juju/lib/service.py
File juju/lib/service.py (right):

https://codereview.appspot.com/6460118/diff/1/juju/lib/service.py#newcode112
juju/lib/service.py:112: yield self._call("kill", self.get_pid())
having seen wrap-around pid behavior where this refers to an entirely
different process, this makes me wary.

https://codereview.appspot.com/6460118/diff/1/juju/lib/service.py#newcode118
juju/lib/service.py:118:
what happens to the stop/destroy case if the pidfile doesn't exist?

https://codereview.appspot.com/6460118/

[Kapil Thangavelu (hazmat)]

this implementation never quite worked for non global installation of libraries, as it passes env vars via a dict with sudo -e, which won't work for pythonpath. See the original implementation in the file history, the env vars must be passed on the cli, not via process env.

review: Needs Fixing

[Kapil Thangavelu (hazmat)]

i'm taking over this branch since it seems to have been abandoned and it should get merged for the next released. i found and fixed a half-dozen issues with it, lots of hardcoded path/ports, broken behavior wrt to env vars, invalid checks for file access, etc.

Unmerged revisions

564. By Benjamin Saller on 2012-08-21

properly kill daemons, properly pass juju_dir on destroy-environment/container destruction

=== modified file 'juju/lib/service.py'
--- juju/lib/service.py	2012-08-03 12:28:29 +0000
+++ juju/lib/service.py	2012-08-21 05:01:09 +0000
@@ -8,14 +8,18 @@
 
 
 def _check_call(args, env=None, output_path=None):
-    if not output_path:
-        output_path = os.devnull
+    if not output_path or not os.access(output_path, os.W_OK):
+        f = open(os.devnull, "a")
+    else:
+        f = open(output_path, "a")
 
-    with open(output_path, "a") as f:
+    try:
         return subprocess.check_call(
             args,
             stdout=f.fileno(), stderr=f.fileno(),
             env=env)
+    finally:
+        f.close()
 
 
 def _cat(filename, use_sudo=False):
@@ -43,20 +47,11 @@
         self._description = None
         self._environ = None
         self._command = None
-        self._output_path = None
+        self.output_path = None
 
         self._pid_path = pidfile
         self._pid = None
 
-    @property
-    def output_path(self):
-        if self._output_path is not None:
-            return self._output_path
-        return "/tmp/%s.output" % self._name
-
-    @output_path.setter
-    def output_path(self, path):
-        self._output_path = path
 
     def set_description(self, description):
         self._description = description
@@ -78,7 +73,7 @@
 
     @inlineCallbacks
     def _trash_output(self):
-        if os.path.exists(self.output_path):
+        if self.output_path and os.path.exists(self.output_path):
             # Just using os.unlink will fail when we're running TEST_SUDO
             # tests which hit this code path (because root will own
             # self.output_path)
@@ -115,14 +110,12 @@
     def destroy(self):
         if (yield self.is_running()):
             yield self._call("kill", self.get_pid())
-        yield self._trash_output()
+            yield self._trash_output()
 
     def get_pid(self):
         if self._pid != None:
             return self._pid
 
-        if not os.path.exists(self._pid_path):
-            return None
         r, data = _cat(self._pid_path, use_sudo=self._use_sudo)
         if r != 0:
             return None

=== modified file 'juju/providers/local/__init__.py'
--- juju/providers/local/__init__.py	2012-07-20 17:28:17 +0000
+++ juju/providers/local/__init__.py	2012-08-21 05:01:09 +0000
@@ -172,7 +172,8 @@
 
         # Stop the machine agent
         log.debug("Stopping machine agent...")
-        agent = ManagedMachineAgent(self._qualified_name)
+        agent = ManagedMachineAgent(self._qualified_name,
+                                    juju_directory=self._directory)
         yield agent.stop()
 
         # Stop the storage server

=== modified file 'juju/providers/local/agent.py'
--- juju/providers/local/agent.py	2012-08-03 10:55:21 +0000
+++ juju/providers/local/agent.py	2012-08-21 05:01:09 +0000
@@ -1,18 +1,10 @@
+import os
 import sys
-import tempfile
 
 from juju.lib.service import TwistedDaemonService
 from juju.providers.common.cloudinit import get_default_origin, BRANCH
 
 
-def get_temp_filename(basename):
-    if basename:
-        basename += "-"
-
-    fd, fn = tempfile.mkstemp(prefix=basename, suffix=".pid")
-    return fn
-
-
 class ManagedMachineAgent(object):
 
     agent_module = "juju.agents.machine"
@@ -51,7 +43,10 @@
         if public_key:
             env["JUJU_PUBLIC_KEY"] = public_key
 
-        pidfile = get_temp_filename(juju_unit_namespace)
+        pidfile = os.path.join(juju_directory,
+                               "%s-machine-agent.pid" % (
+                                       juju_unit_namespace))
+
         self._service = TwistedDaemonService(
             "juju-%s-machine-agent" % juju_unit_namespace,
             pidfile,