Merge lp:~rvb/maas-test/apache-timeout into lp:maas-test

[Raphaël Badin]

Try to restart apache2 a couple of time before giving up.

[Jeroen T. Vermeulen (jtv)]

It's a shame we can't get to the root cause right now, but it does look as if it may be in the Apache package somewhere...

A few notes as always:

.

An alternative way to make tests ignore the "rewrite configuration" command might be to extract that command into a separate function, and have the test patch out that function. Probably not worth it here though, but it's a recurring problem when invoking shell commands.

.

I think test_configure_default_maas_url_fails_apache_cannot_restart is missing an "if" in the name. Should still just barely fit on a line!

.

Perhaps make_exception() could decode as UTF-8? It's a reasonable guess, and text that isn't UTF-8 is highly unlikely to decode cleanly as UTF-8. The disadvantage would be that non-ASCII errors might be handled well only as long as they were in UTF-8, which might end up hiding mistakes from us or confronting people far away with more difficult errors than we see.

review: Approve

[lp:~rvb/maas-test/apache-timeout]

98. By Raphaël Badin on 2013-12-17

Review fixes.

[Raphaël Badin (rvb)]

> It's a shame we can't get to the root cause right now, but it does look as if
> it may be in the Apache package somewhere...
>
> A few notes as always:
>
> .
>
> An alternative way to make tests ignore the "rewrite configuration" command
> might be to extract that command into a separate function, and have the test
> patch out that function. Probably not worth it here though, but it's a
> recurring problem when invoking shell commands.

Yeah, I thought about that but keeping the rewrite of the conf and the restart of apache under the same roof (i.e. in the same method) seemed to make more sense to me rather that refactor it for the sake of testing.

> I think test_configure_default_maas_url_fails_apache_cannot_restart is missing
> an "if" in the name. Should still just barely fit on a line!

Right, done.

> Perhaps make_exception() could decode as UTF-8? It's a reasonable guess, and
> text that isn't UTF-8 is highly unlikely to decode cleanly as UTF-8. The
> disadvantage would be that non-ASCII errors might be handled well only as long
> as they were in UTF-8, which might end up hiding mistakes from us or
> confronting people far away with more difficult errors than we see.

Good point, fixed.

=== modified file 'maastest/maasfixture.py'
--- maastest/maasfixture.py	2013-11-26 22:48:30 +0000
+++ maastest/maasfixture.py	2013-12-17 14:16:31 +0000
@@ -122,8 +122,16 @@
             'http://%s/MAAS' % self.kvm_fixture.direct_ip)
         self.kvm_fixture.run_command(
             ['sudo'] + rewrite_command, check_call=True)
-        self.kvm_fixture.run_command(
-            ['sudo', 'service', 'apache2', 'restart'], check_call=True)
+        # Restarting apache2 sometimes times out: retry the command a
+        # couple of times before giving up.  See bug #1260363.
+        args = ['sudo', 'service', 'apache2', 'restart']
+        for retry in utils.retries(delay=10, timeout=60):
+            retcode, stdout, stderr = self.kvm_fixture.run_command(
+                args, check_call=False)
+            if retcode == 0:
+                break
+        else:
+            raise utils.make_exception(args, retcode, stdout, stderr)
 
     def query_api_key(self, username):
         """Return the API key for the given MAAS user."""

=== modified file 'maastest/tests/test_maasfixture.py'
--- maastest/tests/test_maasfixture.py	2013-11-26 22:43:41 +0000
+++ maastest/tests/test_maasfixture.py	2013-12-17 14:16:31 +0000
@@ -20,6 +20,7 @@
 from random import randint
 from subprocess import check_call
 from textwrap import dedent
+from itertools import chain, repeat
 
 from apiclient.maas_client import MAASClient
 from fixtures import TempDir
@@ -28,6 +29,7 @@
     maasfixture,
     utils,
     )
+from six import text_type
 from maastest.maas_enums import NODEGROUPINTERFACE_MANAGEMENT
 import mock
 import netaddr
@@ -196,13 +198,85 @@
         maas_fixture = self.make_maas_fixture(fixture)
         maas_fixture.configure_default_maas_url()
 
-        self.assertEqual([
-            mock.call(['sudo'] + rewrite_command, check_call=True),
-            mock.call(
-                ['sudo', 'service', 'apache2', 'restart'], check_call=True),
+        self.assertEqual(
+            [
+                mock.call(['sudo'] + rewrite_command, check_call=True),
+                mock.call(
+                    ['sudo', 'service', 'apache2', 'restart'],
+                    check_call=False),
             ],
             fixture.run_command.mock_calls)
 
+    def test_configure_default_maas_url_retries_if_restart_fails(self):
+        self.patch(kvmfixture, 'run_command', mock.MagicMock())
+        fixture = self.make_kvm_fixture()
+        # Configure fixture.run_command to simulate a transient failure when
+        # restarting apache.
+        returns = [
+            # Simluate response for the 'config rewrite' command.
+            (0, 'stdout', 'stderr'),
+            # Simulate first response for the 'apache2 restart' command:
+            # failure.
+            (1, 'stdout', 'an error!'),
+            # Simulate second response for the 'apache2 restart' command:
+            # success.
+            (0, 'stdout', 'stderr'),
+        ]
+
+        def side_effect(*args, **kwargs):
+            return returns.pop(0)
+
+        fixture.run_command = mock.MagicMock(side_effect=side_effect)
+        fixture.direct_ip = '127.99.88.77'
+
+        self.patch(
+            utils, 'retries', mock.MagicMock(return_value=[(0, 1), (0, 1)]))
+        maas_fixture = self.make_maas_fixture(fixture)
+
+        maas_fixture.configure_default_maas_url()
+
+        # The first call to fixture.run_command is the rewrite
+        # configuration command, ignore it since it's tested elsewhere
+        # and we're only testing that the 'apache2 restart' command is
+        # retried when it fails here.
+        run_command_calls = fixture.run_command.mock_calls[1:]
+        self.assertEqual(
+            [
+                # 2 'apache restart' calls.
+                mock.call(
+                    ['sudo', 'service', 'apache2', 'restart'],
+                    check_call=False),
+                mock.call(
+                    ['sudo', 'service', 'apache2', 'restart'],
+                    check_call=False),
+            ],
+            run_command_calls)
+
+    def test_configure_default_maas_url_fails_if_apache_cannot_restart(self):
+        self.patch(kvmfixture, 'run_command', mock.MagicMock())
+        fixture = self.make_kvm_fixture()
+        error_msg = self.getUniqueString()
+        # Configure fixture.run_command to simulate a failure when
+        # restarting apache.
+        returns = chain([
+            # Simluate response for the 'config rewrite' command.
+            (0, 'stdout', 'stderr')],
+            repeat((1, 'stdout', error_msg)))
+
+        def side_effect(*args, **kwargs):
+            return returns.next()
+
+        fixture.run_command = mock.MagicMock(side_effect=side_effect)
+        fixture.direct_ip = '127.99.88.77'
+
+        self.patch(
+            utils, 'retries', mock.MagicMock(return_value=[(0, 1), (0, 1)]))
+        maas_fixture = self.make_maas_fixture(fixture)
+
+        error = self.assertRaises(
+            Exception, maas_fixture.configure_default_maas_url)
+        self.assertIn(error_msg, text_type(error))
+
     def test_create_maas_admin_creates_admin(self):
         fixture = self.make_kvm_fixture()
 

=== modified file 'maastest/tests/test_utils.py'
--- maastest/tests/test_utils.py	2013-12-06 07:47:07 +0000
+++ maastest/tests/test_utils.py	2013-12-17 14:16:31 +0000
@@ -13,6 +13,7 @@
 __all__ = []
 
 import os.path
+from pipes import quote
 import platform
 from subprocess import PIPE
 from textwrap import dedent
@@ -26,6 +27,7 @@
 from six import text_type
 import testtools
 from testtools.matchers import (
+    ContainsAll,
     MatchesRegex,
     MatchesStructure,
     )
@@ -100,6 +102,21 @@
         self.assertEqual(
             (0, b'', input_string), (retcode, stderr, stdout))
 
+    def test_make_exception_contains_details(self):
+        args = ['ls', '/']
+        retcode = 58723
+        stdout = self.getUniqueString()
+        stderr = self.getUniqueString()
+        exception = utils.make_exception(args, retcode, stdout, stderr)
+        self.assertThat(
+            text_type(exception),
+            ContainsAll([
+                stdout,
+                stderr,
+                str(retcode),
+                " ".join(quote(arg) for arg in args),
+            ]))
+
 
 class TestBinaryContent(testtools.TestCase):
     """Tests for `binary_content`."""

=== modified file 'maastest/utils.py'
--- maastest/utils.py	2013-12-06 07:47:07 +0000
+++ maastest/utils.py	2013-12-17 14:16:31 +0000
@@ -49,18 +49,23 @@
         return f.read()
 
 
+def make_exception(args, retcode, stdout, stderr):
+    """Create an exception from the information about a failed command."""
+    cmd = " ".join(quote(arg) for arg in args)
+    return Exception(
+        "Command '%s' failed (%d):\n%s\n%s" % (
+            cmd, retcode,
+            stdout.decode('utf8', errors='replace'),
+            stderr.decode('utf8', errors='replace')))
+
+
 def run_command(args, input=None, check_call=False):
     """A wrapper to Popen to run commands in the command-line."""
     process = Popen(args, stdout=PIPE, stderr=PIPE, stdin=PIPE, shell=False)
     stdout, stderr = process.communicate(input)
     retcode = process.returncode
     if check_call and retcode != 0:
-        cmd = " ".join(quote(arg) for arg in args)
-        raise Exception(
-            "Command '%s' failed (%d):\n%s\n%s" % (
-                cmd, retcode,
-                stdout.decode('ascii'),
-                stderr.decode('ascii')))
+        raise make_exception(args, retcode, stdout, stderr)
     return retcode, stdout, stderr