Merge ~ack/maas:1904758-lxd-pod-refresh-fix into maas:master

[Alberto Donato]

LP: #1904758 - reuse existing VirtualMachine object when refreshing if one
already exists.

VirtualMachine objects are not deleted when the linked machine is removed.
This means when refreshing if a machine get re-added to maas, a VirtualMachine
entry for it already exists.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1904758-lxd-pod-refresh-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8728/console
COMMIT: d6d16aa5b60dacc6389a2668db70bc0d620b2dfd

review: Needs Fixing

[Alberto Donato (ack)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1904758-lxd-pod-refresh-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/8730/console
COMMIT: d6d16aa5b60dacc6389a2668db70bc0d620b2dfd

review: Needs Fixing

[Björn Tillenius (bjornt)]

review: Needs Information

[Alberto Donato (ack)]

jenkins: !test

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1904758-lxd-pod-refresh-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 5c4d65c836d17118ab1f58d74099562d52ae4e94

review: Approve

[Björn Tillenius (bjornt)]

+1 with a nitpick.

review: Approve

diff --git a/src/maasserver/models/bmc.py b/src/maasserver/models/bmc.py
index f0b4fca..5fe306b 100644
--- a/src/maasserver/models/bmc.py
+++ b/src/maasserver/models/bmc.py
@@ -1145,14 +1145,13 @@ class Pod(BMC):
 
             vm = getattr(existing_machine, "virtualmachine", None)
             if vm is None:
-                vm = VirtualMachine.objects.create(
+                vm, _ = VirtualMachine.objects.get_or_create(
                     identifier=existing_machine.instance_power_parameters[
                         "instance_name"
                     ],
                     bmc=self,
-                    machine=existing_machine,
                 )
-
+                vm.machine = existing_machine
             vm.memory = discovered_machine.memory
             vm.hugepages_backed = discovered_machine.hugepages_backed
             vm.pinned_cores = discovered_machine.pinned_cores
@@ -1411,6 +1410,19 @@ class Pod(BMC):
                 % (self.name, remove_machine.hostname)
             )
 
+        # delete entries for VirtualMachines if the corresponding VM has been
+        # removed on the host
+        existing_instance_names = set(
+            machine.power_parameters.get("instance_name")
+            for machine in discovered_machines
+        )
+        existing_instance_names.discard(None)
+        from maasserver.models.virtualmachine import VirtualMachine
+
+        VirtualMachine.objects.filter(bmc=self).exclude(
+            identifier__in=existing_instance_names
+        ).delete()
+
     def sync_storage_pools(self, discovered_storage_pools):
         """Sync the storage pools for the pod."""
         storage_pools_by_id = {
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index fa05273..5d506b4 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -3001,6 +3001,12 @@ class Node(CleanSave, TimestampedModel):
                 )
                 self.bmc.delete()
 
+            # XXX always delete the VM associated to the machine, if present.
+            # Ideally we should only delete VMs when the vM is decomposed.
+            # See LP:#1904758 for details
+            from maasserver.models.virtualmachine import VirtualMachine
+
+            VirtualMachine.objects.filter(machine_id=self.id).delete()
             super().delete(*args, **kwargs)
 
     def set_random_hostname(self):
diff --git a/src/maasserver/models/tests/test_bmc.py b/src/maasserver/models/tests/test_bmc.py
index 64db20d..249a093 100644
--- a/src/maasserver/models/tests/test_bmc.py
+++ b/src/maasserver/models/tests/test_bmc.py
@@ -1130,6 +1130,39 @@ class TestPod(MAASServerTestCase):
             Equals(len(discovered.machines)),
         )
 
+    def test_sync_for_lxd_pod_links_existing_vm(self):
+        discovered_machine = self.make_discovered_machine()
+        discovered_pod = self.make_discovered_pod(
+            machines=[discovered_machine]
+        )
+        instance_name = discovered_machine.power_parameters["instance_name"]
+
+        pod = factory.make_Pod(pod_type="lxd")
+        virtual_machine = factory.make_VirtualMachine(
+            identifier=instance_name, bmc=pod
+        )
+        self.patch(Machine, "start_commissioning")
+        pod.sync(discovered_pod, factory.make_User())
+        machine = Machine.objects.get(hostname=instance_name)
+        self.assertEqual(machine.virtualmachine, virtual_machine)
+
+    def test_sync_for_lxd_pod_removes_unknown_vms(self):
+        discovered_pod = self.make_discovered_pod(machines=[])
+        pod = factory.make_Pod(pod_type="lxd")
+        machine1 = factory.make_Machine(bmc=pod)
+        # a VM linked to a machine that's been removed on the VM host
+        factory.make_VirtualMachine(
+            identifier=machine1.hostname,
+            bmc=pod,
+            machine=machine1,
+        )
+        # a VM not linked to a machine that's been removed on the VM host
+        factory.make_VirtualMachine(bmc=pod)
+        self.patch(Machine, "start_commissioning")
+        pod.sync(discovered_pod, factory.make_User())
+        self.assertFalse(Machine.objects.exists())
+        self.assertFalse(VirtualMachine.objects.exists())
+
     def test_sync_pod_upgrades_default_storage_pool(self):
         discovered = self.make_discovered_pod(machines=[])
         discovered_default = discovered.storage_pools[2]
@@ -2081,7 +2114,10 @@ class TestPod(MAASServerTestCase):
             ),
         )
         vm = factory.make_VirtualMachine(
-            bmc=pod, machine=machine, hugepages_backed=False
+            identifier=machine.hostname,
+            bmc=pod,
+            machine=machine,
+            hugepages_backed=False,
         )
         discovered_interface = self.make_discovered_interface(
             mac_address=machine.interface_set.first().mac_address,
@@ -2091,6 +2127,8 @@ class TestPod(MAASServerTestCase):
         )
         discovered_machine.hugepages_backed = True
         discovered_machine.pinned_cores = [0, 1, 2]
+        discovered_machine.hostname = machine.hostname
+        discovered_machine.power_parameters["instance_name"] = machine.hostname
         discovered_pod = self.make_discovered_pod(
             machines=[discovered_machine]
         )
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 939976f..a5a5aa7 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1620,7 +1620,7 @@ class TestNode(MAASServerTestCase):
         node.delete()
         self.assertIsNotNone(reload_object(pod))
 
-    def test_delete_doesnt_delete_existing_virtual_machine(self):
+    def test_delete_deletes_existing_virtual_machine(self):
         pod = factory.make_Pod()
         machine = factory.make_Machine(
             bmc=pod, creation_type=NODE_CREATION_TYPE.PRE_EXISTING
@@ -1629,7 +1629,7 @@ class TestNode(MAASServerTestCase):
         vm.save()
         with post_commit_hooks:
             machine.delete()
-        self.assertIsNotNone(reload_object(vm))
+        self.assertIsNone(reload_object(vm))
 
     def test_delete_node_deletes_related_interface(self):
         node = factory.make_Node()