Merge ~bjornt/maas:power-driver-metrics-3.2 into maas:3.2

[Björn Tillenius]

Remove unused code from Node.set_power_config().

There was code that checked for self.bmc, even though it couldn't really
be None, except for in unit tests.

A lot of tests passed in "" as the power type, expecting to get a node
without a BMC configured. But in reality, that created a BMC with power
type "", which is invalid.

(cherry picked from commit eb055f00437974e4a36ea2969002b2830f81af9f)

Add stats for power drivers.

Also added an attribute to BMC, so that we can track whether the BMC was
auto-detected in commissioning, or whether the user created it manually.

(cherry picked from commit ab360b09b24e750d106e07778574e72a8dfc99fc)

[Björn Tillenius (bjornt)]

Self-approve backport.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b power-driver-metrics-3.2 lp:~bjornt/maas/+git/maas into -b 3.2 lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

UNIT TESTS
-b power-driver-metrics-3.2 lp:~bjornt/maas/+git/maas into -b 3.2 lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/12818/console
COMMIT: 652a123f80edcdf5bd815eefc0781e3ab4fad8ff

review: Needs Fixing

[MAAS Lander (maas-lander)]

LANDING
-b power-driver-metrics-3.2 lp:~bjornt/maas/+git/maas into -b 3.2 lp:~maas-committers/maas

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

[MAAS Lander (maas-lander)]

UNIT TESTS
-b power-driver-metrics-3.2 lp:~bjornt/maas/+git/maas into -b 3.2 lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: f3f7d9906acff90e81b3ae9ece6a0e2f171eb1a4

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b power-driver-metrics-3.2 lp:~bjornt/maas/+git/maas into -b 3.2 lp:~maas-committers/maas

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

diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index fa8983d..f77e344 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -385,7 +385,7 @@ class WithPowerTypeMixin:
             power_parameters["certificate"] = cert.certificate_pem()
             power_parameters["key"] = cert.private_key_pem()
 
-        if type_changed or initial_parameters:
+        if type_changed or initial_parameters and power_type:
             machine.set_power_config(power_type, power_parameters)
 
     def clean(self):
diff --git a/src/maasserver/forms/tests/test_controller.py b/src/maasserver/forms/tests/test_controller.py
index 904501f..61385f7 100644
--- a/src/maasserver/forms/tests/test_controller.py
+++ b/src/maasserver/forms/tests/test_controller.py
@@ -55,6 +55,7 @@ class TestControllerForm(MAASServerTestCase):
         power_parameters_field = factory.make_string()
         form = ControllerForm(
             data={
+                "power_type": "ipmi",
                 "power_parameters_field": power_parameters_field,
                 "power_parameters_skip_check": "true",
             },
diff --git a/src/maasserver/forms/tests/test_machine.py b/src/maasserver/forms/tests/test_machine.py
index cb7d6f2..089587b 100644
--- a/src/maasserver/forms/tests/test_machine.py
+++ b/src/maasserver/forms/tests/test_machine.py
@@ -572,7 +572,7 @@ class TestAdminMachineForm(MAASServerTestCase):
         self.assertEqual(power_type, node.power_type)
 
     def test_AdminMachineForm_needs_interface_for_power_types(self):
-        node = factory.make_Node(power_type="")
+        node = factory.make_Node(power_type=None)
         arch = make_usable_architecture(self)
         form = AdminMachineForm(
             data={
diff --git a/src/maasserver/migrations/maasserver/0276_bmc_autodetect_metric.py b/src/maasserver/migrations/maasserver/0276_bmc_autodetect_metric.py
new file mode 100644
index 0000000..d9ac5a0
--- /dev/null
+++ b/src/maasserver/migrations/maasserver/0276_bmc_autodetect_metric.py
@@ -0,0 +1,27 @@
+# Generated by Django 2.2.12 on 2022-05-24 13:08
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("maasserver", "0275_interface_children"),
+    ]
+
+    operations = [
+        # All existing rows should have NULL values
+        migrations.AddField(
+            model_name="bmc",
+            name="created_by_commissioning",
+            field=models.BooleanField(default=None, editable=False, null=True),
+        ),
+        # All new rows should default to False.
+        migrations.AlterField(
+            model_name="bmc",
+            name="created_by_commissioning",
+            field=models.BooleanField(
+                default=False, editable=False, null=True
+            ),
+        ),
+    ]
diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py
index 5fda784..7baf693 100644
--- a/src/maasserver/models/bmc.py
+++ b/src/maasserver/models/bmc.py
@@ -289,6 +289,14 @@ class BMC(CleanSave, TimestampedModel):
         default=None,
         editable=False,
     )
+    created_by_commissioning = BooleanField(
+        # We allow None, since before 3.2 we didn't track this, and we
+        # don't know whether the BMC was created manually or
+        # automatically by commissioning.
+        null=True,
+        default=False,
+        editable=False,
+    )
 
     def __str__(self):
         return "{} ({})".format(
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 2a01c4b..2543ae2 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -1374,7 +1374,12 @@ class Node(CleanSave, TimestampedModel):
     def is_commissioning(self):
         return self.status not in (NODE_STATUS.DEPLOYED, NODE_STATUS.DEPLOYING)
 
-    def set_power_config(self, power_type, power_params):
+    def set_power_config(
+        self,
+        power_type,
+        power_params,
+        from_commissioning=False,
+    ):
         """Update the power configuration for a node.
 
         If power_type is not changed, this will update power parameters for the
@@ -1383,6 +1388,7 @@ class Node(CleanSave, TimestampedModel):
         """
         from maasserver.models.bmc import BMC
 
+        created_by_commissioning = from_commissioning
         old_bmc = self.bmc
         chassis, bmc_params, node_params = BMC.scope_power_parameters(
             power_type, power_params
@@ -1396,27 +1402,27 @@ class Node(CleanSave, TimestampedModel):
                     power_type=power_type, power_parameters=bmc_params
                 ).first()
                 if existing_bmc and existing_bmc.id != self.bmc_id:
-                    if self.bmc:
-                        # Point all nodes using old BMC at the new one.
-                        for node in self.bmc.node_set.exclude(id=self.id):
-                            node.bmc = existing_bmc
-                            node.save()
+                    # Point all nodes using old BMC at the new one.
+                    for node in self.bmc.node_set.exclude(id=self.id):
+                        node.bmc = existing_bmc
+                        node.save()
                     self.bmc = existing_bmc
                 elif not existing_bmc:
-                    if self.bmc:
-                        self.bmc.power_parameters = bmc_params
-                    else:
-                        self.bmc = BMC.objects.create(
-                            power_type=power_type, power_parameters=bmc_params
-                        )
+                    self.bmc.power_parameters = bmc_params
                     self.bmc.save()
         elif chassis:
             self.bmc, _ = BMC.objects.get_or_create(
-                power_type=power_type, power_parameters=bmc_params
+                power_type=power_type,
+                power_parameters=bmc_params,
+                defaults={
+                    "created_by_commissioning": created_by_commissioning
+                },
             )
         else:
             self.bmc = BMC.objects.create(
-                power_type=power_type, power_parameters=bmc_params
+                power_type=power_type,
+                power_parameters=bmc_params,
+                created_by_commissioning=created_by_commissioning,
             )
             self.bmc.save()
 
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 7a468c0..987e03f 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1641,7 +1641,7 @@ class TestNode(MAASServerTestCase):
         self.assertEqual("new-hostname", node.hostname)
 
     def test_get_effective_power_type_raises_if_not_set(self):
-        node = factory.make_Node(power_type="")
+        node = factory.make_Node(power_type=None)
         self.assertRaises(UnknownPowerType, node.get_effective_power_type)
 
     def test_get_effective_power_type_reads_node_field(self):
@@ -1706,7 +1706,7 @@ class TestNode(MAASServerTestCase):
         self.assertEqual("pxe", params["boot_mode"])
 
     def test_get_effective_power_info_is_False_for_unset_power_type(self):
-        node = factory.make_Node(power_type="")
+        node = factory.make_Node(power_type=None)
         self.assertEqual(
             (False, False, False, False, None, None),
             node.get_effective_power_info(),
@@ -3338,7 +3338,9 @@ class TestNode(MAASServerTestCase):
 
     def test_start_commissioning_errors_for_unconfigured_power_type(self):
         node = factory.make_Node(
-            interface=True, status=NODE_STATUS.NEW, power_type=""
+            interface=True,
+            status=NODE_STATUS.NEW,
+            power_type=None,
         )
         admin = factory.make_admin()
         self.assertRaises(UnknownPowerType, node.start_commissioning, admin)
@@ -4026,7 +4028,9 @@ class TestNode(MAASServerTestCase):
 
     def test_start_testing_errors_for_unconfigured_power_type(self):
         node = factory.make_Node(
-            interface=True, status=NODE_STATUS.DEPLOYED, power_type=""
+            interface=True,
+            status=NODE_STATUS.DEPLOYED,
+            power_type=None,
         )
         admin = factory.make_admin()
         self.assertRaises(UnknownPowerType, node.start_testing, admin)
@@ -5673,7 +5677,7 @@ class TestNode(MAASServerTestCase):
             status=random.choice(
                 [NODE_STATUS.READY, NODE_STATUS.BROKEN, NODE_STATUS.DEPLOYED]
             ),
-            power_type="",
+            power_type=None,
         )
         self.assertRaises(
             UnknownPowerType, node.start_rescue_mode, factory.make_admin()
@@ -6179,14 +6183,76 @@ class TestNodePowerParameters(MAASServerTestCase):
 
     def test_power_parameters_are_stored(self):
         parameters = dict(user="tarquin", address="10.1.2.3")
-        node = factory.make_Node(power_type="", power_parameters=parameters)
+        node = factory.make_Node(
+            power_type="ipmi", power_parameters=parameters
+        )
         node.save()
         node = reload_object(node)
         self.assertEqual(parameters, node.power_parameters)
 
     def test_power_parameters_default(self):
-        node = factory.make_Node(power_type="")
+        node = factory.make_Node(power_type=None)
         self.assertEqual({}, node.power_parameters)
+        self.assertIsNone(node.bmc)
+
+    def test_power_parameters_from_commissioning_not_new(self):
+        node = factory.make_Node(power_type="virsh")
+        node.set_power_config("ipmi", {}, from_commissioning=True)
+        self.assertTrue(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_not_from_commissioning_not_new(self):
+        node = factory.make_Node(power_type="virsh")
+        node.set_power_config("ipmi", {}, from_commissioning=False)
+        self.assertIsNotNone(node.bmc.created_by_commissioning)
+        self.assertFalse(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_from_commissioning_not_new_chassis(self):
+        node = factory.make_Node(power_type="virsh")
+        node.set_power_config("redfish", {}, from_commissioning=True)
+        self.assertTrue(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_not_from_commissioning_not_new_chassis(self):
+        node = factory.make_Node(power_type="virsh")
+        node.set_power_config("redfish", {}, from_commissioning=False)
+        self.assertIsNotNone(node.bmc.created_by_commissioning)
+        self.assertFalse(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_from_commissioning_new(self):
+        node = factory.make_Node(power_type=None)
+        node.set_power_config("ipmi", {}, from_commissioning=True)
+        self.assertTrue(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_not_from_commissioning_new(self):
+        node = factory.make_Node(power_type=None)
+        node.set_power_config("ipmi", {}, from_commissioning=False)
+        self.assertIsNotNone(node.bmc.created_by_commissioning)
+        self.assertFalse(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_from_commissioning_new_chassis(self):
+        node = factory.make_Node(power_type=None)
+        node.set_power_config("redfish", {}, from_commissioning=True)
+        self.assertTrue(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_not_from_commissioning_new_chassis(self):
+        node = factory.make_Node(power_type=None)
+        node.set_power_config("redfish", {}, from_commissioning=False)
+        self.assertIsNotNone(node.bmc.created_by_commissioning)
+        self.assertFalse(node.bmc.created_by_commissioning)
+
+    def test_power_parameters_from_commissioning_same(self):
+        node = factory.make_Node(
+            power_type="ipmi",
+            power_parameters={"power_address": factory.make_ipv4_address()},
+        )
+        old_value = random.choice([None, True, False])
+        node.bmc.created_by_commissioning = old_value
+        node.bmc.save()
+        node.set_power_config(
+            "ipmi",
+            {"power_address": factory.make_ipv6_address()},
+            from_commissioning=True,
+        )
+        self.assertEqual(node.bmc.created_by_commissioning, old_value)
 
     def test_power_type_and_bmc_power_parameters_stored_in_bmc(self):
         node = factory.make_Node(power_type="hmc")
diff --git a/src/maasserver/rpc/tests/test_nodes.py b/src/maasserver/rpc/tests/test_nodes.py
index 0d4bc07..1132db8 100644
--- a/src/maasserver/rpc/tests/test_nodes.py
+++ b/src/maasserver/rpc/tests/test_nodes.py
@@ -353,14 +353,12 @@ class TestListClusterNodesPowerParameters(MAASServerTestCase):
     # Those tests have been left there for now because they also check
     # that the return values are being formatted correctly for RPC.
 
-    def make_Node(self, power_type=None, power_state_queried=None, **kwargs):
+    def make_Node(self, power_state_queried=None, **kwargs):
         if power_state_queried is None:
             # Ensure that this node was last queried at least 5 minutes ago.
             power_state_queried = now() - timedelta(minutes=randint(6, 16))
         node = factory.make_Node(
-            power_type=power_type,
-            power_state_queried=power_state_queried,
-            **kwargs
+            power_state_queried=power_state_queried, **kwargs
         )
         return node
 
@@ -452,10 +450,11 @@ class TestListClusterNodesPowerParameters(MAASServerTestCase):
         rack = factory.make_RackController()
         node_queryable = self.make_Node(bmc_connected_to=rack)
 
-        factory.make_Device(power_type="")
-        factory.make_Device(power_type="")
+        factory.make_Device(power_type=None)
+        factory.make_Device(power_type=None)
         factory.make_Device(
-            power_type="", power_state_queried=(now() - timedelta(minutes=10))
+            power_type=None,
+            power_state_queried=(now() - timedelta(minutes=10)),
         )
 
         power_parameters = list_cluster_nodes_power_parameters(rack.system_id)
diff --git a/src/maasserver/rpc/tests/test_regionservice_calls.py b/src/maasserver/rpc/tests/test_regionservice_calls.py
index 2843f66..fbb553b 100644
--- a/src/maasserver/rpc/tests/test_regionservice_calls.py
+++ b/src/maasserver/rpc/tests/test_regionservice_calls.py
@@ -549,10 +549,10 @@ class TestRegionProtocol_ListNodePowerParameters(
                 }
             )
 
-        # Create a node with an invalid power type (i.e. the empty string).
+        # Create a node with an invalid power type.
         # This will not be reported by the call to ListNodePowerParameters.
         yield deferToDatabase(
-            self.create_node, power_type="", power_state_updated=None
+            self.create_node, power_type="invalid", power_state_updated=None
         )
 
         response = yield call_responder(
diff --git a/src/maasserver/stats.py b/src/maasserver/stats.py
index 4f6d960..f2596cc 100644
--- a/src/maasserver/stats.py
+++ b/src/maasserver/stats.py
@@ -25,6 +25,7 @@ from maasserver.enum import (
     NODE_TYPE,
 )
 from maasserver.models import (
+    BMC,
     BootResourceFile,
     Config,
     Fabric,
@@ -408,6 +409,31 @@ def get_tls_configuration_stats():
     return {"tls_enabled": True, "tls_cert_validity_days": validity_days}
 
 
+def get_bmc_stats():
+    stats = (
+        BMC.objects.all()
+        .values("power_type")
+        .annotate(
+            auto_detected=count_of(created_by_commissioning=True),
+            user_created=count_of(created_by_commissioning__exact=False),
+            unknown=count_of(created_by_commissioning__exact=None),
+        )
+    )
+    result = {
+        # BMCs that were auto-detected and created by commissoning.
+        "auto_detected": {},
+        # BMCs that the user manually created.
+        "user_created": {},
+        # Before 3.2 we didn't track whether the BMC was auto-detected.
+        "unknown": {},
+    }
+    for entry in stats:
+        for key in result.keys():
+            if entry[key]:
+                result[key][entry["power_type"]] = entry[key]
+    return result
+
+
 def get_maas_stats():
     # TODO
     # - architectures
@@ -457,6 +483,7 @@ def get_maas_stats():
         "vmcluster": get_vmcluster_stats(),
         "storage_layouts": get_storage_layouts_stats(),
         "tls_configuration": get_tls_configuration_stats(),
+        "bmcs": get_bmc_stats(),
     }
 
 
diff --git a/src/maasserver/testing/factory.py b/src/maasserver/testing/factory.py
index dc72f91..62bbd01 100644
--- a/src/maasserver/testing/factory.py
+++ b/src/maasserver/testing/factory.py
@@ -380,7 +380,7 @@ class Factory(maastesting.factory.Factory):
         zone=None,
         networks=None,
         sortable_name=False,
-        power_type=None,
+        power_type="virsh",
         power_parameters=None,
         power_state=None,
         power_state_updated=undefined,
@@ -420,8 +420,6 @@ class Factory(maastesting.factory.Factory):
             status = NODE_STATUS.DEFAULT
         if zone is None:
             zone = Zone.objects.get_default_zone()
-        if power_type is None:
-            power_type = "virsh"
         if power_state is None:
             power_state = self.pick_enum(POWER_STATE)
         if power_state_updated is undefined:
@@ -449,7 +447,7 @@ class Factory(maastesting.factory.Factory):
             cpu_speed=random.randint(1000, 5000),
             **kwargs,
         )
-        if bmc is None:
+        if bmc is None and power_type:
             # These setters will overwrite the BMC, so don't use them if the
             # BMC was specified.
             node.set_power_config(power_type, power_parameters or {})
diff --git a/src/maasserver/tests/test_stats.py b/src/maasserver/tests/test_stats.py
index 5abc2cc..a11b2d4 100644
--- a/src/maasserver/tests/test_stats.py
+++ b/src/maasserver/tests/test_stats.py
@@ -21,6 +21,7 @@ from maasserver.enum import (
 )
 from maasserver.forms import AdminMachineForm
 from maasserver.models import (
+    BMC,
     BootResourceFile,
     Config,
     Fabric,
@@ -31,6 +32,7 @@ from maasserver.models import (
     VLAN,
 )
 from maasserver.stats import (
+    get_bmc_stats,
     get_brownfield_stats,
     get_custom_images_deployed_stats,
     get_custom_images_uploaded_stats,
@@ -456,6 +458,11 @@ class TestMAASStats(MAASServerTestCase):
                 "tls_cert_validity_days": None,
                 "tls_enabled": False,
             },
+            "bmcs": {
+                "auto_detected": {},
+                "user_created": {"lxd": 1, "virsh": 2},
+                "unknown": {},
+            },
         }
         self.assertEqual(stats, expected)
 
@@ -628,6 +635,11 @@ class TestMAASStats(MAASServerTestCase):
                 "tls_cert_validity_days": None,
                 "tls_enabled": False,
             },
+            "bmcs": {
+                "auto_detected": {},
+                "user_created": {},
+                "unknown": {},
+            },
         }
         self.assertEqual(get_maas_stats(), expected)
 
@@ -1047,6 +1059,36 @@ class TestGetSubnetsUtilisationStats(MAASServerTestCase):
         )
 
 
+class TestGetBMCStats(MAASServerTestCase):
+    def test_get_bmc_stats_no_bmcs(self):
+        self.assertEqual(0, BMC.objects.all().count())
+        self.assertEqual(
+            {
+                "auto_detected": {},
+                "user_created": {},
+                "unknown": {},
+            },
+            get_bmc_stats(),
+        )
+
+    def test_get_bmc_stats_with_bmcs(self):
+        factory.make_BMC(power_type="redfish", created_by_commissioning=True)
+        factory.make_BMC(power_type="ipmi", created_by_commissioning=False)
+        factory.make_BMC(power_type="lxd", created_by_commissioning=None)
+        self.assertEqual(
+            {
+                "auto_detected": {"redfish": 1},
+                "user_created": {
+                    "ipmi": 1,
+                },
+                "unknown": {
+                    "lxd": 1,
+                },
+            },
+            get_bmc_stats(),
+        )
+
+
 class TestStatsService(MAASTestCase):
     """Tests for `ImportStatsService`."""
 
diff --git a/src/metadataserver/api.py b/src/metadataserver/api.py
index 5c4ad54..2de6c83 100644
--- a/src/metadataserver/api.py
+++ b/src/metadataserver/api.py
@@ -374,7 +374,9 @@ def store_node_power_parameters(node, request):
             **node.instance_power_parameters,
             **power_parameters,
         }
-    node.set_power_config(power_type, power_parameters)
+    node.set_power_config(
+        power_type, power_parameters, from_commissioning=True
+    )
     node.save()
 
 
diff --git a/src/metadataserver/tests/test_api.py b/src/metadataserver/tests/test_api.py
index a020b2e..6bfd733 100644
--- a/src/metadataserver/tests/test_api.py
+++ b/src/metadataserver/tests/test_api.py
@@ -4166,7 +4166,7 @@ class TestStoreNodeParameters(APITestCase.ForUser):
     def test_power_type_set_with_parameters(self):
         # When power_type is valid, and power_parameters is valid JSON, both
         # fields are set on the node, and the node is saved.
-        power_type = factory.pick_power_type()
+        power_type = factory.pick_power_type(but_not=[self.node.power_type])
         power_parameters = {"foo": [1, 2, 3]}
         self.request.POST = {
             "power_type": power_type,
@@ -4175,6 +4175,7 @@ class TestStoreNodeParameters(APITestCase.ForUser):
         store_node_power_parameters(self.node, self.request)
         self.assertEqual(power_type, self.node.power_type)
         self.assertEqual(power_parameters, self.node.power_parameters)
+        self.assertTrue(self.node.bmc.created_by_commissioning)
         self.save.assert_called_once_with()
 
     def test_power_type_set_with_invalid_parameters(self):