Merge lp:~gmb/maas-test/import-images into lp:maas-test

[Graham Binns]

This branch makes importing PXE files a setup step for the MAAS fixture. It updates the import_pxe_files config directly rather than using `export` because the latter didn't persist across SSH sessions.

[lp:~gmb/maas-test/import-images]

50. By Graham Binns on 2013-11-18

Added a todo.

[Gavin Panella (allenap)]

Nice. A few very minor comments.

[1]

+        # latter doesn't seem to work stick across SSH sessions. I don't

I assume "work stick" is a northern expression? :)

[2]

+        # know whether that's because # we're doing it over SSH and not

Extra #

[3]

+        expected_pxe_config_snippet = (
+            '\n'
+            'RELEASES="precise"\n'
+            'ARCHES="amd64/generic i386/generic"\n'
+            '\n')

This would be easier to read as:

        expected_pxe_config_snippet = dedent("""\

            RELEASES="precise"
            ARCHES="amd64/generic i386/generic"

            """

[4]

+        self.assertEqual(
+            expected_command, kvm_fixture.run_command.mock_calls[0])

I guess it's somewhat important that the config call is the first. Can
you note this here, so that this doesn't get changed to using
<mock>.assert_any_call()?

[5]

+        self.assertEqual(
+            mock.call([u'sudo', u'maas-import-pxe-files'], check_call=True),
+            kvm_fixture.run_command.mock_calls[-1])

Similar comment about ordering here.

review: Approve

[Raphaël Badin (rvb)]

Looks nice, but I've got a question, see [1].

[0]

32 === modified file 'maastest/maasfixture.py'
33 --- maastest/maasfixture.py 2013-11-15 14:14:29 +0000
34 +++ maastest/maasfixture.py 2013-11-18 09:48:39 +0000
[...]
39 - def import_maas_images(self, admin_user, api_key):
40 + def import_maas_images(self):

It's really fine to have 'RELEASES="precise" and 'ARCHES="amd64/generic i386/generic" hardcoded from now but it would be nice to have them hardcoded in maastest/main.py and passed to MAASFixture() (same as the parameters 'series' and 'architecture' used by KVMFixture). This way, when we will make them configurable, the change will be minimal. This is really just a detail but might be worth doing in a follow-up branch; this way, all the parameters (even those that are hardcoded for now) will be available at the highest level (i.e. in maastest/main.py) and we will have a nice list of all the config values in one place instead of hardcoded values scattered throughout the code.

[1]

50 + # Update the import_pxe_files config. The reason that we do this
51 + # rather than just `export`ing RELEASES and ARCHES is that the
52 + # latter doesn't seem to work stick across SSH sessions. I don't
53 + # know whether that's because # we're doing it over SSH and not
54 + # using shell=True on the Popen calls in run_command, but this,
55 + # on the other hand, definitely does work.

I'm a bit surprised by that, since we define "DEBIAN_FRONTEND=noninteractive" when we install packages in the VM (see maastest/maasfixture.py). Env variables are not carried over by sudo by default but how about defining the variables *inside* the command executed by sudo… something like:
sudo RELEASES="precise" ARCHES="amd64/generic i386/generic" maas-import-pxe-files
?

review: Needs Information

[lp:~gmb/maas-test/import-images]

51. By Graham Binns on 2013-11-18

Include config in command.

[Graham Binns (gmb)]

On 18 November 2013 13:19, Raphaël Badin <email address hidden> wrote:
>
> [0]
>
> 32 === modified file 'maastest/maasfixture.py'
> 33 --- maastest/maasfixture.py 2013-11-15 14:14:29 +0000
> 34 +++ maastest/maasfixture.py 2013-11-18 09:48:39 +0000
> [...]
> 39 - def import_maas_images(self, admin_user, api_key):
> 40 + def import_maas_images(self):
>
> It's really fine to have 'RELEASES="precise" and 'ARCHES="amd64/generic i386/generic" hardcoded from now but it would be nice to have them hardcoded in maastest/main.py and passed to MAASFixture() (same as the parameters 'series' and 'architecture' used by KVMFixture). This way, when we will make them configurable, the change will be minimal. This is really just a detail but might be worth doing in a follow-up branch; this way, all the parameters (even those that are hardcoded for now) will be available at the highest level (i.e. in maastest/main.py) and we will have a nice list of all the config values in one place instead of hardcoded values scattered throughout the code.

Agreeed. I'll do a follow-up.

> [1]
>
> 50 + # Update the import_pxe_files config. The reason that we do this
> 51 + # rather than just `export`ing RELEASES and ARCHES is that the
> 52 + # latter doesn't seem to work stick across SSH sessions. I don't
> 53 + # know whether that's because # we're doing it over SSH and not
> 54 + # using shell=True on the Popen calls in run_command, but this,
> 55 + # on the other hand, definitely does work.
>
> I'm a bit surprised by that, since we define "DEBIAN_FRONTEND=noninteractive" when we install packages in the VM (see maastest/maasfixture.py). Env variables are not carried over by sudo by default but how about defining the variables *inside* the command executed by sudo… something like:
> sudo RELEASES="precise" ARCHES="amd64/generic i386/generic" maas-import-pxe-files

WFM, I'll do that. It's not that defining the variables in the shell
doesn't work, it's that `export...` doesn't work — the DEBIAN_FRONTEND
variable is defined as part of the command executed by sudo, hence it
works fine.

[Raphaël Badin (rvb)]

> WFM, I'll do that. It's not that defining the variables in the shell
> doesn't work, it's that `export...` doesn't work — the DEBIAN_FRONTEND
> variable is defined as part of the command executed by sudo, hence it
> works fine.

Great, the code will be even simpler this way.

review: Approve

[lp:~gmb/maas-test/import-images]

52. By Graham Binns on 2013-11-18

Fixing quotes.

[Raphaël Badin (rvb)]

[2]

49 + logging.info("Importing MAAS PXE files.")

and

8 + logging.info(
9 + "Configuring network interface on virtual machine %s",
10 + self.name)

Btw, I've tried to put "..." at the end of the comments that indicate that an operation is being performed. Let's keep that convention here.

[lp:~gmb/maas-test/import-images]

53. By Graham Binns on 2013-11-18

Test fixes and shiznit.

=== modified file 'maastest/kvmfixture.py'
--- maastest/kvmfixture.py	2013-11-15 15:48:07 +0000
+++ maastest/kvmfixture.py	2013-11-18 16:04:03 +0000
@@ -250,12 +250,18 @@
         # Configure the bridged network if self.direct_interface is not
         # None.
         if self.direct_interface is not None:
+            logging.info(
+                "Configuring network interface on virtual machine %s...",
+                 self.name)
             network_config_snippet = make_network_interface_config(
                 'eth1', self.direct_ip, self.direct_network)
             self.run_command(
                 ["sudo", "tee", "-a", "/etc/network/interfaces"],
                 input=network_config_snippet, check_call=True)
             self.run_command(["sudo", "ifup", "eth1"], check_call=True)
+            logging.info(
+                "Done configuring network interface on virtual machine %s",
+                self.name)
 
     def destroy(self):
         """Destroy the virtual machine."""
@@ -302,6 +308,7 @@
         return retcode, stdout, stderr
 
 
+
 KVM_TEMPLATE = """
   <domain type='kvm'>
     <os>

=== modified file 'maastest/maasfixture.py'
--- maastest/maasfixture.py	2013-11-15 14:14:29 +0000
+++ maastest/maasfixture.py	2013-11-18 16:04:03 +0000
@@ -143,7 +143,7 @@
 
         return username, password
 
-    def import_maas_images(self, admin_user, api_key):
+    def import_maas_images(self):
         """Import boot images into the MAAS instance.
 
         This download gigabytes of data to the virtual machine's disk.
@@ -152,7 +152,17 @@
         :param admin_user: Login name for the MAAS admin user.
         :param api_key: Admin user's API key.
         """
-        # TODO: Actually implement.
+        logging.info("Importing MAAS PXE files...")
+        # TODO: ARCHES and RELEASES should come from command-line
+        # parameters.
+        self.kvm_fixture.run_command([
+            u'sudo',
+            u'RELEASES=precise',
+            u'ARCHES=amd64/generic i386/generic',
+            u'maas-import-pxe-files',
+            ],
+            check_call=True)
+        logging.info("Done importing MAAS PXE Files.")
 
     def setUp(self):
         """Install and configure MAAS inside the virtual machine.
@@ -174,7 +184,7 @@
         self.admin_api_key = api_key
         self.admin_maas_client = maas_client
         # MAAS setup.
-        self.import_maas_images(admin_user, api_key)
+        self.import_maas_images()
         if self.kvm_fixture.direct_ip is not None:
             self.check_cluster_connected()
             self.configure_cluster()

=== modified file 'maastest/tests/test_maasfixture.py'
--- maastest/tests/test_maasfixture.py	2013-11-15 11:57:00 +0000
+++ maastest/tests/test_maasfixture.py	2013-11-18 16:04:03 +0000
@@ -430,3 +430,17 @@
                 maas_fixture.check_cluster_connected.mock_calls,
                 maas_fixture.configure_cluster.mock_calls,
             ])
+
+    def test_import_mmas_images_imports_pxe_images(self):
+        kvm_fixture = self.make_kvm_fixture()
+        self.patch(kvm_fixture, 'run_command', mock.MagicMock())
+        maas_fixture = maasfixture.MAASFixture(kvm_fixture)
+        maas_fixture.import_maas_images()
+        self.assertEqual(
+            mock.call([
+                u'sudo',
+                u'RELEASES=precise',
+                u'ARCHES=amd64/generic i386/generic',
+                u'maas-import-pxe-files',
+            ], check_call=True),
+            kvm_fixture.run_command.mock_calls[-1])

=== modified file 'packages.txt'
--- packages.txt	2013-11-14 11:13:37 +0000
+++ packages.txt	2013-11-18 16:04:03 +0000
@@ -14,5 +14,6 @@
 python3-six
 python3-testresources
 python3-testtools
+qemu-kvm
 uvtool
 uvtool-libvirt