Merge lp:~jtv/maas-test/pidfiles-run into lp:maas-test

[Jeroen T. Vermeulen]

Move pidfiles into /run/maas-test.

As discussed with Julian, pidfiles belong in /run. As a simple solution to keeping the name meaningful ("/run/proxy.pid" would be a bit too generic) and keep room for other pidfiles we may want to add later, I created a directory /run/maas-test.

This wasn't quite as trivial as updating a path and making sure it's created, because existing tests also needed updating. Don't want those writing to /run!

Next: moving the logs into /var/log/maas-test.

Jeroen

[Julian Edwards (julian-edwards)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlLOPskACgkQWhGlTF8G/HfifACfdc01DXHhEyqZheAvuu9zPTi0
ExwAn33VNd2jDyz9z683Fq91dCHra8Xv
=E+N3
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'maastest/proxyfixture.py'
--- maastest/proxyfixture.py	2014-01-09 03:51:20 +0000
+++ maastest/proxyfixture.py	2014-01-09 06:08:12 +0000
@@ -58,7 +58,7 @@
 
     #TODO: port should be determined at runtime from the set of
     # available ports rather than being an arbitrary one as it is here.
-    def __init__(self, config_dir=None, port=42676):
+    def __init__(self, config_dir=None, port=42676, pidfile_dir=None):
         """Create a `LocalProxyFixture` object.
 
         :param url: The URL of the proxy to use. If None, LocalProxyFixture
@@ -68,15 +68,20 @@
             If URL is specified, this parameter is ignored.
         :type port: int
         :param config_dir: The directory in which to put the proxy's
-            config, pid file and cache.
+            configuration and cache.
         :type config_dir: string
+        :param pid_dir: Optional directory in which to write the proxy's
+            pidfile.  Will be created if not present.  Defaults to
+            `/run/maas-test`.
         """
         super(LocalProxyFixture, self).__init__()
         self.port = port
         if config_dir is None:
             config_dir = utils.DEFAULT_STATE_DIR
         self.config_dir = config_dir
-        self.pid_file = os.path.join(self.config_dir, 'proxy.pid')
+        if pidfile_dir is None:
+            pidfile_dir = utils.DEFAULT_PIDFILE_DIR
+        self.pid_file = os.path.join(pidfile_dir, 'proxy.pid')
         self._network_address = None
 
     def _get_config_arguments(self):
@@ -119,6 +124,11 @@
         # From this point, if further setup fails, we'll need to clean up.
         self.addCleanup(self.kill_running_proxy)
 
+        # Create pidfile directory if it did not already exist.
+        pidfile_dir = os.path.dirname(self.pid_file)
+        if not os.path.isdir(pidfile_dir):
+            os.makedirs(pidfile_dir, 0o755)
+
         self.start_proxy()
 
     def start_proxy(self):

=== modified file 'maastest/tests/test_proxyfixture.py'
--- maastest/tests/test_proxyfixture.py	2014-01-08 17:31:43 +0000
+++ maastest/tests/test_proxyfixture.py	2014-01-09 06:08:12 +0000
@@ -39,23 +39,52 @@
                 'netmask': '255.255.255.0',
             }]}
         self.patch(netifaces, 'ifaddresses', mock_ifaddresses)
-        self.temp_dir = self.useFixture(TempDir()).path
+
+    def make_dir(self):
+        """Create a temporary directory for the duration of this test."""
+        return self.useFixture(TempDir()).path
+
+    def make_fixture(self, port=None, config_dir=None, pidfile_dir=None):
+        """Create a `LocalProxyFixture`.
+
+        :param port: Optional port.  Defaults to a random choice.
+        :param config_dir: Optional configuration directory.  Defaults to a
+            temporary directory.
+        :param pidfile_dir: Optional pidfile directory.  Defaults to a
+            temporary directory.
+        """
+        if port is None:
+            port = self.get_random_port_number()
+        if config_dir is None:
+            config_dir = self.make_dir()
+        if pidfile_dir is None:
+            pidfile_dir = self.make_dir()
+        return LocalProxyFixture(
+            port=port, config_dir=config_dir, pidfile_dir=pidfile_dir)
 
     def get_random_port_number(self):
         return random.randint(1, 65535)
 
     def test_init_sets_properties(self):
         port = self.get_random_port_number()
-        proxy_fixture = LocalProxyFixture(port=port, config_dir=self.temp_dir)
+        config_dir = self.make_dir()
+        pidfile_dir = self.make_dir()
+        proxy_fixture = LocalProxyFixture(
+            port=port, config_dir=config_dir, pidfile_dir=pidfile_dir)
         self.assertEqual(port, proxy_fixture.port)
-        self.assertEqual(self.temp_dir, proxy_fixture.config_dir)
+        self.assertEqual(config_dir, proxy_fixture.config_dir)
         self.assertEqual(
-            os.path.join(self.temp_dir, 'proxy.pid'),
+            os.path.join(pidfile_dir, 'proxy.pid'),
             proxy_fixture.pid_file)
 
+    def test_creates_pidfile_dir(self):
+        pidfile_dir = os.path.join(self.make_dir(), "maas-test-pidfiles")
+        with self.make_fixture(pidfile_dir=pidfile_dir):
+            self.assertThat(pidfile_dir, DirExists())
+
     def test_get_url_returns_proxy_url(self):
         port = self.get_random_port_number()
-        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir, port=port)
+        proxy_fixture = self.make_fixture(port=port)
         self.assertEqual(
             "http://192.168.0.1:%s" % port,
             proxy_fixture.get_url())
@@ -64,9 +93,7 @@
         self.patch(LocalProxyFixture, 'create_cache_root', mock.MagicMock())
         self.patch(LocalProxyFixture, 'start_proxy', mock.MagicMock())
 
-        port = self.get_random_port_number()
-        config_dir = self.temp_dir
-        with LocalProxyFixture(config_dir=config_dir, port=port) as fixture:
+        with self.make_fixture() as fixture:
             pass
 
         self.assertEqual(
@@ -79,8 +106,7 @@
     def test_start_proxy_starts_proxy(self):
         mock_run_command = mock.MagicMock()
         self.patch(utils, 'run_command', mock_run_command)
-        proxy_fixture = LocalProxyFixture(
-            config_dir=self.temp_dir, port=self.get_random_port_number())
+        proxy_fixture = self.make_fixture()
 
         proxy_fixture.start_proxy()
 
@@ -91,28 +117,25 @@
             mock_run_command.mock_calls[-1])
 
     def test_create_cache_root_creates_directory(self):
-        proxy_fixture = LocalProxyFixture(
-            config_dir=self.temp_dir, port=self.get_random_port_number())
+        config_dir = self.make_dir()
+        proxy_fixture = self.make_fixture(config_dir=config_dir)
 
         proxy_fixture.create_cache_root()
         expected_cache_path = proxyfixture.DISK_CACHE_ROOT % {
-            'config_dir': self.temp_dir}
+            'config_dir': config_dir}
         self.assertThat(expected_cache_path, DirExists())
 
     def test_create_cache_root_ignores_existing_directory(self):
-        temp_dir = self.useFixture(TempDir()).path
-        self.patch(proxyfixture, 'DISK_CACHE_ROOT', temp_dir)
-        proxy_fixture = LocalProxyFixture(
-            config_dir=self.temp_dir, port=self.get_random_port_number())
+        disk_cache_root = self.make_dir()
+        self.patch(proxyfixture, 'DISK_CACHE_ROOT', disk_cache_root)
+        proxy_fixture = self.make_fixture()
 
         proxy_fixture.create_cache_root()
 
-        self.assertThat(temp_dir, DirExists())
+        self.assertThat(disk_cache_root, DirExists())
 
     def test_get_network_address(self):
-        proxy_fixture = LocalProxyFixture(
-            config_dir=self.temp_dir, port=self.get_random_port_number())
-        proxy_adddress = proxy_fixture._get_network_address()
+        proxy_adddress = self.make_fixture()._get_network_address()
         self.assertEqual('192.168.0.1', proxy_adddress.ip.format())
         self.assertEqual(24, proxy_adddress.prefixlen)
 
@@ -122,8 +145,7 @@
         mock_read_file = mock.MagicMock()
         mock_read_file.return_value = bytes(self.getUniqueInteger())
         self.patch(utils, 'read_file', mock_read_file)
-        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
-        proxy_fixture.kill_running_proxy()
+        self.make_fixture().kill_running_proxy()
         self.assertEqual(
             mock.call(['kill', mock_read_file.return_value], check_call=True),
             mock_run_command.mock_calls[0])
@@ -134,8 +156,7 @@
         mock_read_file = mock.MagicMock()
         mock_read_file.side_effect = Exception(self.getUniqueString())
         self.patch(utils, 'read_file', mock_read_file)
-        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
-        proxy_fixture.kill_running_proxy()
+        self.make_fixture().kill_running_proxy()
         mock_log_error.assert_has_calls([
             mock.call("Unable to kill proxy:"),
             mock.call(mock_read_file.side_effect.message)])
@@ -149,8 +170,7 @@
         mock_read_file = mock.MagicMock()
         mock_read_file.return_value = bytes(self.getUniqueInteger())
         self.patch(utils, 'read_file', mock_read_file)
-        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
-        proxy_fixture.kill_running_proxy()
+        self.make_fixture().kill_running_proxy()
         mock_log_error.assert_has_calls([
             mock.call("Unable to kill proxy:"),
             mock.call(mock_run_command.side_effect.message)])
@@ -163,8 +183,7 @@
         self.patch(utils, 'read_file', mock_read_file)
         mock_kill_running_proxy = mock.MagicMock()
 
-        port = self.get_random_port_number()
-        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir, port=port)
+        proxy_fixture = self.make_fixture()
         self.patch(
             proxy_fixture, 'kill_running_proxy', mock_kill_running_proxy)
 
@@ -176,5 +195,5 @@
         mock_exists = mock.MagicMock()
         mock_exists.return_value = True
         self.patch(os.path, 'exists', mock_exists)
-        proxy_fixture = LocalProxyFixture(config_dir=self.temp_dir)
+        proxy_fixture = self.make_fixture()
         self.assertRaises(Exception, proxy_fixture.setUp)

=== modified file 'maastest/utils.py'
--- maastest/utils.py	2014-01-09 03:51:20 +0000
+++ maastest/utils.py	2014-01-09 06:08:12 +0000
@@ -17,6 +17,7 @@
     "determine_vm_series",
     "determine_vm_architecture",
     "get_uri",
+    "DEFAULT_PIDFILE_DIR",
     "read_file",
     "retries",
     "run_command",
@@ -50,6 +51,9 @@
 # machine, and the http proxy cache.
 DEFAULT_STATE_DIR = '/var/cache/maas-test'
 
+# Default location for pidfiles.
+DEFAULT_PIDFILE_DIR = '/run/maas-test'
+
 
 def read_file(path):
     """Return a given file's contents, as `bytes`."""