Merge lp://staging/~fwereade/pyjuju/upstartify-services into lp://staging/pyjuju

Note that ~600 lines of these diffs are purely cloud-init test data changes: the runcmds now write out upstart config files to bring the MA/PA up, rather than invoking them directly, and this makes for many lines of diff but relatively little interesting content. So it shouldn't be quite as bad as it looks :).

+1. Agreed about the diffs, pretty easy to grok this branch. Just one small thing I found:

[1]

+ def mock_start(self):
+ self.check_call(("/sbin/start", "juju-wordpress-0"))
+ self.mocker.result(None)

Should be self.mocker.result(0)

+
+ def mock_destroy(self):
+ self.check_call(("/sbin/stop", "juju-wordpress-0"))
+ self.mocker.result(None)

Ditto

[0]

machine/unit -> _get_container no longer uses zookeeper_hosts

[1]

machine/unit -> AgentProcessProtocol has been obsoleted.

it would be good to have the upstart config capture stderr/stdout to a file location, for debugging output, which the agentprocessprotocol would do as a python exception.

[2]

for local dev the user should also be set, ideally to that of the user for things that don't need root (ie. storage server, zk server).

[3]
+ # Reuse the lxc flag for tests needing sudo
+ test_start_stop.skip = test_namespacing.skip = run_lxc_tests()

These flags should be set physically next to the test the value is set on.

[5]

i'm a little concerned by the branch in that we're no longer running processes to a large extent, instead substituting mocking, so effectively decreasing the unit test real world coverage.

specifically most of the service unit tests that would actually launch a real process, have been given over for mocks. those tests where valuable in discovering problems with pythonpath or parent directories in the past.

i'd recommend additional skip enabled tests for the machine unit deployment.

Its seems like several unit deployment tests where removed that still need testing, specifically around container directory creation/destruction.

Ready for re-review, I think.

The many tedious changes to test data files in juju.providers can (probably;)) be safely ignored.

More detail:

UpstartService.start now checks that the job's pid is stable (5 checks over 0.4s), and raises a ServiceError if it isn't (including, where available, the stdout/stderr written out from the exec line in the job conf).

Local provider file storage now runs with user's uid/gid.

UnitMachineDeployment is now functionally tested (kind of, anyway; it's using dummy agent, as before).

+1, and approving again with just the following minors. Also this should have the real process coverage that Kapil raised that was finding issues around paths and parent dirs.

[2]

-def run_lxc_tests():
+def skip_sudo_tests():
     if os.environ.get("TEST_LXC"):
- return None
- return "TEST_LXC=1 to include lxc tests"
+ return False
+ return "TEST_LXC=1 to include lxc tests (and others which use sudo)"

Maybe we should rename this env var so it reflects the more general usage?

[3]

+def uses_sudo(f):
+ f.skip = skip_sudo_tests()
+ return f

Simple but nice decorator!

[4]

+def _sleep(secs):
+ d = Deferred()
+ reactor.callLater(secs, d.callback, None)
+ return d
+

Use juju.lib.twistutils.sleep instead

[5]

+ @inlineCallbacks
+ def _trash_output(self):
+ if os.path.exists(self.output_path):
+ yield self._call("rm", self.output_path)
+

Should be no need to have a single file removal to be done async; just use os.unlink as used elsewhere.

Changes look good. One oddity is that i'm seeing console output in the test runner when running the sudo upstart tests

ie.

TEST_SUDO=1 ./test juju/providers/local/tests/test_files.py
[sudo] password for kapil:
juju.providers.local.tests.test_files
  FileStorageTest
    test_get_url ... [OK]
  WebFileStorageTest
    test_capture_errors ... juju-borken-file-storage start/running, process 32115
juju-borken-file-storage stop/waiting
                                               [OK]
    test_namespacing ... juju-ns2-file-storage start/running, process 32136
juju-ns1-file-storage start/running, process 32149
juju-ns2-file-storage stop/waiting
juju-ns1-file-storage stop/waiting
                                                  [OK]
    test_start_invalid_directory ... [OK]
    test_start_missing_args ... [OK]
    test_start_stop ... juju-ns1-file-storage start/running, process 32178
juju-ns1-file-storage stop/waiting
                                                   [OK]
    test_upstart ... [OK]

-------------------------------------------------------------------------------
Ran 7 tests in 5.451s

[7] This can break the local provider due to juju/machine/unit.py using sys.executable instead of /usr/bin/python.. ie. its in the container, so the reference point to the binary is unambiguous, else it breaks with a virtualenv.

[2] Makes sense; I was vaguely worried that other things might have grown to depend on it being TEST_LXC, but if anyone experiences problems the fix should not be too hard to figure out ;)

[3] Thanks :)

[4] Ah, cool, thank you.

[5] _call inserts "sudo"s where necessary, and so works in (test) situations where a mere unlink would fall over.

[6?] Heh, I'll sort that out.

[7] Cheers.