Code review comment for lp:~bcsaller/pyjuju/lxc-killpid

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/__init__.py 2012-08-21 04:52:21 +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

Index: juju/providers/local/agent.py
=== 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 04:52:21 +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,