Merge ~newell-jensen/maas:virsh-storage-pools into maas:master

[Newell Jensen]

LP: #1742708 -- Add ability to define default storage pool for Virsh pods.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b virsh-storage-pools lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2180/console
COMMIT: 03cc1edfb2ad5619660df60b3fecc16ce1cc958c

review: Needs Fixing

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b virsh-storage-pools lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2221/consoleText

aaa4698... by Newell Jensen on 2018-03-29

Fix failing tests.

diff --git a/src/maasserver/api/tests/test_enlistment.py b/src/maasserver/api/tests/test_enlistment.py
index 2f03848..5fc5563 100644
--- a/src/maasserver/api/tests/test_enlistment.py
+++ b/src/maasserver/api/tests/test_enlistment.py
@@ -480,6 +480,7 @@ class SimpleUserLoggedInEnlistmentAPITest(APITestCase.ForUser):
     def test_POST_simple_user_can_set_power_type_and_parameters(self):
         new_power_address = factory.make_ip_address()  # XXX: URLs don't work.
         new_power_id = factory.make_name('power_id')
+        new_default_storage_pool = factory.make_name('default_pool')
         response = self.client.post(
             reverse('machines_handler'), {
                 'architecture': make_usable_architecture(self),
@@ -488,6 +489,7 @@ class SimpleUserLoggedInEnlistmentAPITest(APITestCase.ForUser):
                     {
                         "power_address": new_power_address,
                         "power_id": new_power_id,
+                        "default_storage_pool": new_default_storage_pool,
                     }),
                 'mac_addresses': ['AA:BB:CC:DD:EE:FF'],
                 })
@@ -500,6 +502,7 @@ class SimpleUserLoggedInEnlistmentAPITest(APITestCase.ForUser):
                 'power_pass': '',
                 'power_id': new_power_id,
                 'power_address': new_power_address,
+                'default_storage_pool': new_default_storage_pool,
             }, machine.power_parameters)
 
     def test_POST_returns_limited_fields(self):
@@ -614,6 +617,7 @@ class AdminLoggedInEnlistmentAPITest(APITestCase.ForAdmin):
         new_power_id = factory.make_name('power_id')
         new_power_address = factory.make_ipv4_address()
         new_power_pass = factory.make_name('power_pass')
+        new_default_storage_pool = factory.make_name('default_pool')
         response = self.client.post(
             reverse('machines_handler'), {
                 'architecture': make_usable_architecture(self),
@@ -621,6 +625,8 @@ class AdminLoggedInEnlistmentAPITest(APITestCase.ForAdmin):
                 'power_parameters_power_id': new_power_id,
                 'power_parameters_power_pass': new_power_pass,
                 'power_parameters_power_address': new_power_address,
+                'power_parameters_default_storage_pool': (
+                    new_default_storage_pool),
                 'mac_addresses': ['AA:BB:CC:DD:EE:FF'],
                 })
 
@@ -633,6 +639,7 @@ class AdminLoggedInEnlistmentAPITest(APITestCase.ForAdmin):
                 'power_id': new_power_id,
                 'power_pass': new_power_pass,
                 'power_address': new_power_address,
+                'default_storage_pool': new_default_storage_pool,
             },
             reload_object(machine).power_parameters)
 
diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index 65fd929..b143f22 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -1280,12 +1280,15 @@ class TestMachineAPI(APITestCase.ForUser):
         new_power_id = factory.make_name('power_id')
         new_power_pass = factory.make_name('power_pass')
         new_power_address = factory.make_ipv4_address()
+        new_default_storage_pool = factory.make_name('default_pool')
         response = self.client.put(
             self.get_machine_uri(machine),
             {
                 'power_parameters_power_id': new_power_id,
                 'power_parameters_power_pass': new_power_pass,
                 'power_parameters_power_address': new_power_address,
+                'power_parameters_default_storage_pool': (
+                    new_default_storage_pool),
             }
         )
 
@@ -1295,6 +1298,7 @@ class TestMachineAPI(APITestCase.ForUser):
                 'power_id': new_power_id,
                 'power_pass': new_power_pass,
                 'power_address': new_power_address,
+                'default_storage_pool': new_default_storage_pool,
             },
             reload_object(machine).power_parameters)
 
diff --git a/src/maasserver/forms/tests/test_pods.py b/src/maasserver/forms/tests/test_pods.py
index 3808839..6eb2805 100644
--- a/src/maasserver/forms/tests/test_pods.py
+++ b/src/maasserver/forms/tests/test_pods.py
@@ -94,6 +94,7 @@ class TestPodForm(MAASTransactionServerTestCase):
         pod_ip_adddress = factory.make_ipv4_address()
         pod_power_address = 'qemu+ssh://user@%s/system' % pod_ip_adddress
         pod_password = factory.make_name('password')
+        pod_default_storage_pool = factory.make_name('default_pool')
         pod_tags = [
             factory.make_name("tag")
             for _ in range(3)
@@ -104,6 +105,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             'type': pod_type,
             'power_address': pod_power_address,
             'power_pass': pod_password,
+            'default_storage_pool': pod_default_storage_pool,
             'ip_address': pod_ip_adddress,
             'tags': ",".join(pod_tags),
             'cpu_over_commit_ratio': pod_cpu_over_commit_ratio,
@@ -168,6 +170,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters=Equals({
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             }),
             ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
@@ -208,6 +211,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters=Equals({
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             }),
             ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
@@ -239,6 +243,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters=Equals({
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             }),
             ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
@@ -288,6 +293,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters=Equals({
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             }),
             ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
@@ -339,6 +345,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters={
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             })
         request = MagicMock()
         request.user = factory.make_User()
@@ -375,6 +382,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters=Equals({
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             }),
             ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
@@ -425,6 +433,7 @@ class TestPodForm(MAASTransactionServerTestCase):
             power_parameters=Equals({
                 'power_address': pod_info['power_address'],
                 'power_pass': pod_info['power_pass'],
+                'default_storage_pool': pod_info['default_storage_pool'],
             }),
             ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
diff --git a/src/maasserver/rpc/tests/test_nodes.py b/src/maasserver/rpc/tests/test_nodes.py
index c8034b1..f8f99ff 100644
--- a/src/maasserver/rpc/tests/test_nodes.py
+++ b/src/maasserver/rpc/tests/test_nodes.py
@@ -240,6 +240,7 @@ class TestCreateNode(MAASTransactionServerTestCase):
             'power_address': factory.make_ip_address(),  # XXX: URLs break.
             'power_pass': factory.make_name('power_pass'),
             'power_id': factory.make_name('power_id'),
+            'default_storage_pool': factory.make_name('default_pool'),
         }
 
         node = create_node(
diff --git a/src/maasserver/websockets/handlers/tests/test_machine.py b/src/maasserver/websockets/handlers/tests/test_machine.py
index 27bc195..b027541 100644
--- a/src/maasserver/websockets/handlers/tests/test_machine.py
+++ b/src/maasserver/websockets/handlers/tests/test_machine.py
@@ -1853,6 +1853,7 @@ class TestMachineHandler(MAASServerTestCase):
         power_id = factory.make_name('power_id')
         power_pass = factory.make_name('power_pass')
         power_address = factory.make_ipv4_address()
+        default_storage_pool = factory.make_name('default_pool')
         node_data["hostname"] = new_hostname
         node_data["architecture"] = new_architecture
         node_data["zone"] = {
@@ -1863,6 +1864,7 @@ class TestMachineHandler(MAASServerTestCase):
             'power_id': power_id,
             'power_pass': power_pass,
             'power_address': power_address,
+            'default_storage_pool': default_storage_pool,
         }
         updated_node = handler.update(node_data)
         self.expectThat(updated_node["hostname"], Equals(new_hostname))
@@ -1873,6 +1875,7 @@ class TestMachineHandler(MAASServerTestCase):
             'power_id': power_id,
             'power_pass': power_pass,
             'power_address': power_address,
+            'default_storage_pool': default_storage_pool,
         }))
 
     def test_update_adds_tags_to_node(self):
diff --git a/src/provisioningserver/drivers/pod/tests/test_virsh.py b/src/provisioningserver/drivers/pod/tests/test_virsh.py
index d73ebc3..44cd8d7 100644
--- a/src/provisioningserver/drivers/pod/tests/test_virsh.py
+++ b/src/provisioningserver/drivers/pod/tests/test_virsh.py
@@ -1416,12 +1416,29 @@ class TestVirshPodDriver(MAASTestCase):
             yield driver.discover(system_id, context)
 
     @inlineCallbacks
+    def test_discover_errors_on_incorrect_default_storage_pool(self):
+        driver = VirshPodDriver()
+        system_id = factory.make_name('system_id')
+        context = {
+            'power_address': factory.make_name('power_address'),
+            'power_pass': factory.make_name('power_pass'),
+            'default_storage_pool': factory.make_name('non-existent-pool')
+        }
+        mock_login = self.patch(virsh.VirshSSH, 'login')
+        mock_login.return_value = True
+        mock_run = self.patch(virsh.VirshSSH, 'run')
+        mock_run.return_value = SAMPLE_POOLLIST
+        with ExpectedException(virsh.VirshError):
+            yield driver.discover(system_id, context)
+
+    @inlineCallbacks
     def test_discover(self):
         driver = VirshPodDriver()
         system_id = factory.make_name('system_id')
         context = {
             'power_address': factory.make_name('power_address'),
-            'power_pass': factory.make_name('power_pass')
+            'power_pass': factory.make_name('power_pass'),
+            'default_storage_pool': 'default'
         }
         machines = [
             factory.make_name('machine')
@@ -1429,6 +1446,8 @@ class TestVirshPodDriver(MAASTestCase):
         ]
         mock_login = self.patch(virsh.VirshSSH, 'login')
         mock_login.return_value = True
+        mock_run = self.patch(virsh.VirshSSH, 'run')
+        mock_run.return_value = SAMPLE_POOLLIST
         mock_get_pod_resources = self.patch(
             virsh.VirshSSH, 'get_pod_resources')
         mock_get_pod_hints = self.patch(
@@ -1451,6 +1470,7 @@ class TestVirshPodDriver(MAASTestCase):
                 call(machines[1]),
                 call(machines[2])))
         self.expectThat(['virtual'], Equals(discovered_pod.tags))
+        self.expectThat(driver.default_storage_pool, Equals('default'))
 
     @inlineCallbacks
     def test_compose(self):
diff --git a/src/provisioningserver/drivers/pod/virsh.py b/src/provisioningserver/drivers/pod/virsh.py
index a32d935..afdca25 100644
--- a/src/provisioningserver/drivers/pod/virsh.py
+++ b/src/provisioningserver/drivers/pod/virsh.py
@@ -295,11 +295,16 @@ class VirshSSH(pexpect.spawn):
         machines = machines.strip().splitlines()
         return [m for m in machines if m.startswith(self.dom_prefix)]
 
-    def list_pools(self):
-        """Lists all pools in the pod."""
+    def list_pools(self, default_pool=None):
+        """Lists all pools in the pod.
+
+        If default_pool is set, only return this pool.
+        """
         keys = ['Name']
         output = self.run(['pool-list'])
         pools = self.get_column_values(output, keys)
+        if default_pool:
+            return [p[0] for p in pools if p[0] == default_pool]
         return [p[0] for p in pools]
 
     def list_machine_block_devices(self, machine):
@@ -372,10 +377,10 @@ class VirshSSH(pexpect.spawn):
         # Memory in MiB.
         return int(KiB / 1024)
 
-    def get_pod_pool_size_map(self, key):
+    def get_pod_pool_size_map(self, key, default_pool=None):
         """Return the mapping for a size calculation based on key."""
         pools = {}
-        for pool in self.list_pools():
+        for pool in self.list_pools(default_pool):
             output = self.run(['pool-info', pool]).strip()
             if output is None:
                 # Skip if cannot get more information.
@@ -445,6 +450,11 @@ class VirshSSH(pexpect.spawn):
         # name, that MAAS understands.
         return ARCH_FIX.get(arch, arch)
 
+    def check_default_storage_pool(self, default_pool):
+        """Check that default_pool exists."""
+        if not len(self.list_pools(default_pool)):
+            raise VirshError("Storage pool '%s' doesn't exist." % default_pool)
+
     def get_pod_resources(self):
         """Get the pod resources."""
         discovered_pod = DiscoveredPod(
@@ -596,17 +606,17 @@ class VirshSSH(pexpect.spawn):
             return False
         return True
 
-    def get_usable_pool(self, size):
+    def get_usable_pool(self, size, default_pool=None):
         """Return the pool that as enough space for `size`."""
-        pools = self.get_pod_pool_size_map("Available")
+        pools = self.get_pod_pool_size_map("Available", default_pool)
         for pool, available in pools.items():
             if size <= available:
                 return pool
         return None
 
-    def create_local_volume(self, size):
+    def create_local_volume(self, size, default_pool=None):
         """Create a local volume with `size`."""
-        usable_pool = self.get_usable_pool(size)
+        usable_pool = self.get_usable_pool(size, default_pool)
         if usable_pool is None:
             return None
         volume = str(uuid.uuid4())
@@ -718,7 +728,7 @@ class VirshSSH(pexpect.spawn):
             idx = (idx // 26) - 1
         return "vd" + name
 
-    def create_domain(self, request):
+    def create_domain(self, request, default_pool=None):
         """Create a domain based on the `request` with hostname.
 
         For now this just uses `get_best_network` to connect the interfaces
@@ -730,7 +740,7 @@ class VirshSSH(pexpect.spawn):
         created_disks = []
         for idx, disk in enumerate(request.block_devices):
             try:
-                disk_info = self.create_local_volume(disk.size)
+                disk_info = self.create_local_volume(disk.size, default_pool)
             except Exception:
                 self.cleanup_disks(created_disks)
                 raise
@@ -802,7 +812,11 @@ class VirshPodDriver(PodDriver):
         make_setting_field(
             'power_id', "Virsh VM ID", scope=SETTING_SCOPE.NODE,
             required=True),
+        make_setting_field(
+            'default_storage_pool', "Default storage pool (optional)",
+            required=False),
     ]
+    default_storage_pool = None
     ip_extractor = make_ip_extractor(
         'power_address', IP_EXTRACTOR_PATTERNS.URL)
 
@@ -903,6 +917,18 @@ class VirshPodDriver(PodDriver):
         """
         conn = yield self.get_virsh_connection(context)
 
+        # Check and set default storage pool.
+        self.default_storage_pool = context.get('default_storage_pool')
+        if self.default_storage_pool:
+            try:
+                yield deferToThread(
+                    conn.check_default_storage_pool, self.default_storage_pool)
+            except VirshError:
+                # Set the default_storage_pool to None since
+                # one wasn't found and raise the error.
+                self.default_storage_pool = None
+                raise
+
         # Discover pod resources.
         discovered_pod = yield deferToThread(conn.get_pod_resources)
 
@@ -930,7 +956,8 @@ class VirshPodDriver(PodDriver):
     def compose(self, system_id, context, request):
         """Compose machine."""
         conn = yield self.get_virsh_connection(context)
-        created_machine = yield deferToThread(conn.create_domain, request)
+        created_machine = yield deferToThread(
+            conn.create_domain, request, self.default_storage_pool)
         hints = yield deferToThread(conn.get_pod_hints)
         return created_machine, hints