Merge ~newell-jensen/maas:lp1812509 into maas:master

[Newell Jensen]

LP: #1812509 -- Fix setting the power type for a node with BMCs that have matching power parameters.

[Newell Jensen (newell-jensen)]

To me this seems like a better fix than doing a data migration because if there are any machines with potentially matching power parameters (future power types), this fix will cover them as well.

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: 90036214afa07bca2b58a0dd935eabbe313167a6

review: Approve

[Björn Tillenius (bjornt)]

I need more context before I can review this. Which revision and migration caused this bug to happen?

I'd like to look more into this, since the solution looks more like a workaround than a proper fix.

[Newell Jensen (newell-jensen)]

> I need more context before I can review this. Which revision and migration
> caused this bug to happen?
>
> I'd like to look more into this, since the solution looks more like a
> workaround than a proper fix.

Before the migration, machines with a manual power type were all linked to a singleton BMC. The migration broke up this linked BMC and made it so all the machines had their own BMC. This causes an exception to be raised at this particular point in the code. If we introduced another migration to fix this issue, we would have to rearrange previous migrations, as this needs to be backported to 2.5. I agree with your comment on the XXX portion.

[Blake Rouse (blake-rouse)]

I think we need to fix this another way, because a manual power type is a unique BMC per machine. I understand that manual is in a unique situation where the power_parameters are the same for each machine.

I feel like there is another way to solve this. I feel like this is only an issue with the manual power provider anyway. Why not special case the manual power type (we already do that in many places) and do the correct thing at that point.

review: Needs Fixing

2992c82... by Newell Jensen on 2019-01-29

Handle the manual power type directly to create its BMC when setting the power type for a node.

8bb5145... by Newell Jensen on 2019-01-29

Remove code that is no longer needed.

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: 2992c82a4c0e0299e6871e37a7b5722b13aa9613

review: Approve

[MAAS Lander (maas-lander)]

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

STATUS: SUCCESS
COMMIT: 8bb51451bce93145d6ebaf8c77d41a6ef3ba404c

review: Approve

882bc59... by Newell Jensen on 2019-01-30

Add unit test to show that setting a manual power type machine to manual does not create an additional BMC.

11fc99c... by Newell Jensen on 2019-01-30

Fix unit test to use bmc id.

[Blake Rouse (blake-rouse)]

Looks good, thanks for the test.

review: Approve

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 998e02c..b2f558e 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Node objects."""
@@ -1223,8 +1223,13 @@ class Node(CleanSave, TimestampedModel):
             return
         if self.bmc is not None and self.bmc.power_type == power_type:
             return
-        self.bmc, _ = BMC.objects.get_or_create(
-            power_type=power_type, power_parameters=self.power_parameters)
+
+        if power_type == 'manual':
+            self.bmc = BMC.objects.create(
+                power_type='manual', power_parameters={})
+        else:
+            self.bmc, _ = BMC.objects.get_or_create(
+                power_type=power_type, power_parameters=self.power_parameters)
 
     @property
     def power_parameters(self):
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 361b6f8..3aa4169 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2018 Canonical Ltd.  This software is licensed under the
+# Copyright 2012-2019 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test maasserver models."""
@@ -4582,6 +4582,34 @@ class TestNodePowerParameters(MAASServerTestCase):
         self.assertEqual(node.power_type, node.bmc.power_type)
         self.assertEqual(ip_address, node.bmc.ip_address.ip)
 
+    def test_power_type_creates_new_bmc_for_manual_power_type(self):
+        ip_address = factory.make_ipv4_address()
+        bmc_parameters = dict(power_address=ip_address)
+        node_parameters = dict(server_name=factory.make_string())
+        parameters = {**bmc_parameters, **node_parameters}
+        node = factory.make_Node(
+            power_type='virsh', power_parameters=parameters)
+        self.assertFalse(BMC.objects.filter(power_type='manual'))
+        node.power_type = 'manual'
+        node.save()
+        node = reload_object(node)
+        self.assertEqual('manual', node.bmc.power_type)
+        self.assertEqual({}, node.bmc.power_parameters)
+        self.assertTrue(BMC.objects.filter(power_type='manual'))
+
+    def test_power_type_does_not_create_new_bmc_for_already_manual(self):
+        ip_address = factory.make_ipv4_address()
+        bmc_parameters = dict(power_address=ip_address)
+        node_parameters = dict(server_name=factory.make_string())
+        parameters = {**bmc_parameters, **node_parameters}
+        node = factory.make_Node(
+            power_type='manual', power_parameters=parameters)
+        bmc_id = node.bmc.id
+        node.power_type = 'manual'
+        node.save()
+        node = reload_object(node)
+        self.assertEqual(bmc_id, node.bmc.id)
+
     def test_power_parameters_are_stored_in_proper_scopes(self):
         node = factory.make_Node(power_type='virsh')
         bmc_parameters = dict(