Merge ~bjornt/maas:bug-1772906-fix-pod-update-api into maas:master

[Björn Tillenius]

LP: #1772906 - Pods can't be updated via the API without passing power_address

[Alberto Donato (ack)]

+1

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b bug-1772906-fix-pod-update-api lp:~bjornt/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/2904/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b bug-1772906-fix-pod-update-api lp:~bjornt/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/2906/consoleText

[MAAS Lander (maas-lander)]

LANDING
-b bug-1772906-fix-pod-update-api lp:~bjornt/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/2908/consoleText

[MAAS Lander (maas-lander)]

UNIT TESTS
-b bug-1772906-fix-pod-update-api lp:~bjornt/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/2911/console
COMMIT: aa1df742fc57ac9acb2b9254284ce89fda6ef091

review: Needs Fixing

f1a1cdf... by Björn Tillenius on 2018-05-24

Fix test failure.

diff --git a/src/maasserver/api/tests/test_pods.py b/src/maasserver/api/tests/test_pods.py
index fed3570..863df22 100644
--- a/src/maasserver/api/tests/test_pods.py
+++ b/src/maasserver/api/tests/test_pods.py
@@ -276,6 +276,26 @@ class TestPodAPI(APITestCase.ForUser, PodMixin):
         self.assertEqual(new_name, parsed_output['name'])
         self.assertEqual(discovered_pod.cores, parsed_output['total']['cores'])
 
+    def test_PUT_update_minimal(self):
+        self.become_admin()
+        pod_info = self.make_pod_info()
+        power_parameters = {
+            'power_address': pod_info['power_address'],
+            'power_pass': pod_info['power_pass'],
+        }
+        pod = factory.make_Pod(
+            pod_type=pod_info['type'], parameters=power_parameters)
+        new_name = factory.make_name('pool')
+        self.fake_pod_discovery()
+        response = self.client.put(get_pod_uri(pod), {
+            'name': new_name,
+        })
+        self.assertEqual(
+            http.client.OK, response.status_code, response.content)
+        pod.refresh_from_db()
+        self.assertEqual(new_name, pod.name)
+        self.assertEqual(power_parameters, pod.power_parameters)
+
     def test_refresh_requires_admin(self):
         pod = factory.make_Pod()
         response = self.client.post(get_pod_uri(pod), {
diff --git a/src/maasserver/forms/pods.py b/src/maasserver/forms/pods.py
index 654f5f6..89e3b2b 100644
--- a/src/maasserver/forms/pods.py
+++ b/src/maasserver/forms/pods.py
@@ -165,10 +165,14 @@ class PodForm(MAASModelForm):
         # selected type for the pod.
         if len(self.errors) == 0:
             driver_fields = get_driver_parameters_from_json(
-                self.drivers_orig, None, scope=SETTING_SCOPE.BMC)
+                self.drivers_orig, scope=SETTING_SCOPE.BMC)
             self.param_fields = (
                 driver_fields[self.cleaned_data['type']].field_dict)
             self.fields.update(self.param_fields)
+            if not self.is_new:
+                for key, value in self.instance.power_parameters.items():
+                    if key not in self.data:
+                        self.data[key] = value
             super(PodForm, self)._clean_fields()
 
     def clean(self):
diff --git a/src/maasserver/forms/tests/test_pods.py b/src/maasserver/forms/tests/test_pods.py
index 64bfae4..a567f31 100644
--- a/src/maasserver/forms/tests/test_pods.py
+++ b/src/maasserver/forms/tests/test_pods.py
@@ -85,7 +85,7 @@ def make_pod_with_hints():
     pod = factory.make_Pod(
         architectures=architectures, cores=random.randint(8, 16),
         memory=random.randint(4096, 8192),
-        cpu_speed=random.randint(2000, 3000))
+        cpu_speed=random.randint(2000, 3000), parameters={})
     for _ in range(3):
         pool = factory.make_PodStoragePool(pod)
     pod.default_storage_pool = pool
@@ -369,18 +369,22 @@ class TestPodForm(MAASTransactionServerTestCase):
 
     def test_updates_existing_pod_minimal(self):
         self.fake_pod_discovery()
-        pod_info = self.make_pod_info()
         zone = factory.make_Zone()
         pool = factory.make_ResourcePool()
         cpu_over_commit = random.randint(0, 10)
         memory_over_commit = random.randint(0, 10)
+        power_parameters = {
+            'power_address': 'qemu+ssh://1.2.3.4/system',
+            'power_pass': 'pass',
+        }
         orig_pod = factory.make_Pod(
-            pod_type=pod_info['type'], zone=zone, default_pool=pool,
+            pod_type='virsh', zone=zone, default_pool=pool,
             cpu_over_commit_ratio=cpu_over_commit,
-            memory_over_commit_ratio=memory_over_commit)
+            memory_over_commit_ratio=memory_over_commit,
+            parameters=power_parameters)
         new_name = factory.make_name("pod")
-        pod_info['name'] = new_name
-        form = PodForm(data=pod_info, request=self.request, instance=orig_pod)
+        form = PodForm(
+            data={'name': new_name}, request=self.request, instance=orig_pod)
         self.assertTrue(form.is_valid(), form._errors)
         pod = form.save()
         self.assertEqual(new_name, pod.name)
@@ -388,6 +392,8 @@ class TestPodForm(MAASTransactionServerTestCase):
         self.assertEqual(pool, pod.default_pool)
         self.assertEqual(cpu_over_commit, pod.cpu_over_commit_ratio)
         self.assertEqual(memory_over_commit, pod.memory_over_commit_ratio)
+        self.assertEqual(memory_over_commit, pod.memory_over_commit_ratio)
+        self.assertEqual(power_parameters, pod.power_parameters)
 
     def test_updates_existing_pod(self):
         discovered_pod, discovered_racks, failed_racks = (
@@ -533,7 +539,10 @@ class TestPodForm(MAASTransactionServerTestCase):
             self.fake_pod_discovery())
         zone = factory.make_Zone()
         pod_info = self.make_pod_info()
-        orig_pod = factory.make_Pod(zone=zone, pod_type=pod_info['type'])
+        power_parameters = {'power_address': pod_info['power_address']}
+        orig_pod = factory.make_Pod(
+            zone=zone, pod_type=pod_info['type'],
+            parameters=power_parameters)
         form = PodForm(data=pod_info, request=self.request, instance=orig_pod)
         pod = form.discover_and_sync_pod()
         self.assertThat(pod, MatchesStructure(
@@ -546,8 +555,8 @@ class TestPodForm(MAASTransactionServerTestCase):
             cpu_speed=Equals(discovered_pod.cpu_speed),
             zone=Equals(zone),
             power_type=Equals(pod_info['type']),
-            power_parameters=Equals({}),
-            ip_address=Is(None),
+            power_parameters=Equals(power_parameters),
+            ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
         routable_racks = [
             relation.rack_controller
@@ -571,8 +580,10 @@ class TestPodForm(MAASTransactionServerTestCase):
             pods_module.discover_pod.return_value)
         zone = yield deferToDatabase(factory.make_Zone)
         pod_info = yield deferToDatabase(self.make_pod_info)
+        power_parameters = {'power_address': pod_info['power_address']}
         orig_pod = yield deferToDatabase(
-            factory.make_Pod, zone=zone, pod_type=pod_info['type'])
+            factory.make_Pod, zone=zone, pod_type=pod_info['type'],
+            parameters=power_parameters)
         form = yield deferToDatabase(
             PodForm, data=pod_info, request=self.request, instance=orig_pod)
         pod = yield form.discover_and_sync_pod()
@@ -586,8 +597,8 @@ class TestPodForm(MAASTransactionServerTestCase):
             cpu_speed=Equals(discovered_pod.cpu_speed),
             zone=Equals(zone),
             power_type=Equals(pod_info['type']),
-            power_parameters=Equals({}),
-            ip_address=Is(None),
+            power_parameters=Equals(power_parameters),
+            ip_address=MatchesStructure(ip=Equals(pod_info['ip_address'])),
         ))
 
         def validate_rack_routes():
diff --git a/src/maasserver/models/tests/test_bmc.py b/src/maasserver/models/tests/test_bmc.py
index 9bc3cfb..84db146 100644
--- a/src/maasserver/models/tests/test_bmc.py
+++ b/src/maasserver/models/tests/test_bmc.py
@@ -1509,8 +1509,8 @@ class TestPodDelete(MAASTransactionServerTestCase):
         yield pod.async_delete()
         self.assertThat(
             client, MockCalledOnceWith(
-                DecomposeMachine, type=pod.power_type, context={},
-                pod_id=pod.id, name=pod.name))
+                DecomposeMachine, type=pod.power_type,
+                context=pod.power_parameters, pod_id=pod.id, name=pod.name))
         decomposable_machine = yield deferToDatabase(
             reload_object, decomposable_machine)
         delete_machine = yield deferToDatabase(reload_object, delete_machine)
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index 0204588..e5bd2b7 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -610,7 +610,10 @@ class Factory(maastesting.factory.Factory):
         if pod_type is None:
             pod_type = 'virsh'
         if parameters is None:
-            parameters = {}
+            parameters = {
+                'power_address': 'qemu+ssh://{}/system'.format(
+                    self.make_ip_address())
+            }
         pod = Pod(
             power_type=pod_type, power_parameters=parameters,
             ip_address=ip_address, **kwargs)
diff --git a/src/maasserver/websockets/handlers/tests/test_pod.py b/src/maasserver/websockets/handlers/tests/test_pod.py
index 7539a5a..d3a4621 100644
--- a/src/maasserver/websockets/handlers/tests/test_pod.py
+++ b/src/maasserver/websockets/handlers/tests/test_pod.py
@@ -9,14 +9,12 @@ import random
 from unittest.mock import MagicMock
 
 from crochet import wait_for
-from maasserver.enum import NODE_TYPE
 from maasserver.forms import pods
 from maasserver.forms.pods import PodForm
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import MAASTransactionServerTestCase
 from maasserver.utils.orm import reload_object
 from maasserver.utils.threads import deferToDatabase
-from maasserver.websockets.base import dehydrate_datetime
 from maasserver.websockets.handlers.pod import (
     ComposeMachineForm,
     PodHandler,
@@ -113,116 +111,22 @@ class TestPodHandler(MAASTransactionServerTestCase):
             cpu_speed=random.randint(1000, 3000), local_storage=0)
         return composed_machine, pod_hints
 
-    def dehydrate_storage_pool(self, pool):
-        used = pool.get_used_storage()
-        return {
-            'id': pool.pool_id,
-            'name': pool.name,
-            'type': pool.pool_type,
-            'path': pool.path,
-            'total': pool.storage,
-            'used': used,
-            'available': pool.storage - used,
-        }
-
-    def dehydrate_pod(self, pod, admin=True, for_list=True):
-        data = {
-            "id": pod.id,
-            "name": pod.name,
-            "cpu_speed": pod.cpu_speed,
-            "type": pod.power_type,
-            "ip_address": pod.ip_address,
-            "updated": dehydrate_datetime(pod.updated),
-            "created": dehydrate_datetime(pod.created),
-            "composed_machines_count": pod.node_set.filter(
-                node_type=NODE_TYPE.MACHINE).count(),
-            "total": {
-                "cores": pod.cores,
-                "memory": pod.memory,
-                'memory_gb': '%.1f' % (pod.memory / 1024.0),
-                "local_storage": pod.local_storage,
-                'local_storage_gb': '%.1f' % (pod.local_storage / (1024 ** 3)),
-                },
-            "used": {
-                "cores": pod.get_used_cores(),
-                "memory": pod.get_used_memory(),
-                'memory_gb': '%.1f' % (pod.get_used_memory() / 1024.0),
-                "local_storage": pod.get_used_local_storage(),
-                'local_storage_gb': '%.1f' % (
-                    pod.get_used_local_storage() / (1024 ** 3)),
-                },
-            "available": {
-                "cores": pod.cores - pod.get_used_cores(),
-                "memory": pod.memory - pod.get_used_memory(),
-                'memory_gb': '%.1f' % (
-                    (pod.memory - pod.get_used_memory()) / 1024.0),
-                "local_storage": (
-                    pod.local_storage - pod.get_used_local_storage()),
-                'local_storage_gb': '%.1f' % (
-                    (pod.local_storage - pod.get_used_local_storage()) / (
-                        (1024 ** 3))),
-                },
-            "default_pool": pod.default_pool.id,
-            "capabilities": pod.capabilities,
-            "architectures": pod.architectures,
-            "hints": {
-                'cores': pod.hints.cores,
-                'cpu_speed': pod.hints.cpu_speed,
-                'memory': pod.hints.memory,
-                'memory_gb': '%.1f' % (pod.hints.memory / 1024.0),
-                'local_storage': pod.hints.local_storage,
-                'local_storage_gb': '%.1f' % (
-                    pod.hints.local_storage / (1024 ** 3)),
-                'local_disks': pod.hints.local_disks,
-                'iscsi_storage': pod.hints.iscsi_storage,
-                'iscsi_storage_gb': '%.1f' % (
-                    pod.hints.iscsi_storage / (1024 ** 3)),
-                },
-            "tags": pod.tags,
-            "zone": pod.zone.id,
-            "cpu_over_commit_ratio": pod.cpu_over_commit_ratio,
-            "memory_over_commit_ratio": pod.memory_over_commit_ratio
-            }
-        if Capabilities.FIXED_LOCAL_STORAGE in pod.capabilities:
-            data['total']['local_disks'] = pod.local_disks
-            data['used']['local_disks'] = pod.get_used_local_disks()
-            data['available']['local_disks'] = (
-                pod.local_disks - pod.get_used_local_disks())
-        if Capabilities.ISCSI_STORAGE in pod.capabilities:
-            data['total']['iscsi_storage'] = pod.iscsi_storage
-            data['total']['iscsi_storage_gb'] = '%.1f' % (
-                pod.iscsi_storage / (1024 ** 3))
-            data['used']['iscsi_storage'] = pod.get_used_iscsi_storage()
-            data['used']['iscsi_storage_gb'] = '%.1f' % (
-                pod.get_used_iscsi_storage() / (1024 ** 3))
-            data['available']['iscsi_storage'] = (
-                pod.iscsi_storage - pod.get_used_iscsi_storage())
-            data['available']['iscsi_storage_gb'] = '%.1f' % (
-                (pod.iscsi_storage - pod.get_used_iscsi_storage()) / (
-                    1024 ** 3))
-        if admin:
-            data.update(pod.power_parameters)
-        if not for_list and pod.storage_pools.all():
-            data["storage_pools"] = [
-                self.dehydrate_storage_pool(pool)
-                for pool in pod.storage_pools.all()
-            ]
-            data["default_storage_pool"] = pod.default_storage_pool.pool_id
-        return data
-
     def test_get(self):
         admin = factory.make_admin()
         handler = PodHandler(admin, {})
         pod = self.make_pod_with_hints()
-        expected_data = self.dehydrate_pod(pod, for_list=False)
+        expected_data = handler.full_dehydrate(pod)
         result = handler.get({"id": pod.id})
+        self.assertItemsEqual(expected_data.keys(), result.keys())
+        for key in expected_data:
+            self.assertEqual(expected_data[key], result[key], key)
         self.assertThat(result, Equals(expected_data))
 
     def test_get_as_standard_user(self):
         user = factory.make_User()
         handler = PodHandler(user, {})
         pod = self.make_pod_with_hints()
-        expected_data = self.dehydrate_pod(pod, admin=False, for_list=False)
+        expected_data = handler.full_dehydrate(pod)
         result = handler.get({"id": pod.id})
         self.assertThat(result, Equals(expected_data))
 
@@ -230,7 +134,7 @@ class TestPodHandler(MAASTransactionServerTestCase):
         admin = factory.make_admin()
         handler = PodHandler(admin, {})
         pod = self.make_pod_with_hints()
-        expected_data = [self.dehydrate_pod(pod, for_list=True)]
+        expected_data = [handler.full_dehydrate(pod, for_list=True)]
         result = handler.list({"id": pod.id})
         self.assertThat(result, Equals(expected_data))
 
@@ -244,10 +148,13 @@ class TestPodHandler(MAASTransactionServerTestCase):
             PodForm, 'discover_and_sync_pod')
         mock_discover_and_sync_pod.return_value = succeed(pod)
         expected_data = yield deferToDatabase(
-            self.dehydrate_pod, pod, for_list=False)
+            handler.full_dehydrate, pod, for_list=False)
         observed_data = yield handler.refresh({"id": pod.id})
         self.assertThat(
             mock_discover_and_sync_pod, MockCalledOnceWith())
+        self.assertItemsEqual(expected_data.keys(), observed_data.keys())
+        for key in expected_data:
+            self.assertEqual(expected_data[key], observed_data[key], key)
         self.assertEqual(expected_data, observed_data)
 
     @wait_for_reactor