Merge ~newell-jensen/maas:fix-1827149 into maas:master

[Newell Jensen]

LP: #1827149 -- Create bridges by default when deploying a machine and install_kvm is set to True.

[Blake Rouse (blake-rouse)]

review: Needs Information

8bd7f96... by Newell Jensen on 2019-05-13

Review updates.

35994df... by Newell Jensen on 2019-05-13

Update node.start unit test to check if install_kvm is being set.

[Newell Jensen (newell-jensen)]

Changed node.start to take install_kvm. Yes, the call to node._create_acquired_bridges is a noop if it has already been called and nothing has changed with regards to the interfaces since the last call was made.

[Blake Rouse (blake-rouse)]

Thanks for the fixes, looks good.

review: Approve

diff --git a/src/maasserver/api/machines.py b/src/maasserver/api/machines.py
index be156d0..ab5ab67 100644
--- a/src/maasserver/api/machines.py
+++ b/src/maasserver/api/machines.py
@@ -684,6 +684,21 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
             system_id=system_id, user=request.user,
             perm=NodePermission.edit)
         options = get_allocation_options(request)
+        # Deploying a node requires re-checking for EDIT permissions.
+        if not request.user.has_perm(NodePermission.edit, machine):
+            raise PermissionDenied()
+        # Deploying with 'install_rackd' requires ADMIN permissions.
+        if (options.install_rackd and not
+                request.user.has_perm(NodePermission.admin, machine)):
+            raise PermissionDenied()
+        # Deploying with 'install_kvm' requires ADMIN permissions.
+        if (options.install_kvm and not
+                request.user.has_perm(NodePermission.admin, machine)):
+            raise PermissionDenied()
+        if options.install_kvm and (
+                machine.ephemeral_deployment or options.ephemeral_deploy):
+            raise MAASAPIBadRequest(
+                "Cannot install KVM host for ephemeral deployments.")
         if machine.status == NODE_STATUS.READY:
             with locks.node_acquire:
                 if machine.owner is not None and machine.owner != request.user:
@@ -701,21 +716,6 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
             raise NodeStateViolation(
                 "Can't deploy a machine that is in the '{}' state".format(
                     NODE_STATUS_CHOICES_DICT[machine.status]))
-        # Deploying a node requires re-checking for EDIT permissions.
-        if not request.user.has_perm(NodePermission.edit, machine):
-            raise PermissionDenied()
-        # Deploying with 'install_rackd' requires ADMIN permissions.
-        if (options.install_rackd and not
-                request.user.has_perm(NodePermission.admin, machine)):
-            raise PermissionDenied()
-        # Deploying with 'install_kvm' requires ADMIN permissions.
-        if (options.install_kvm and not
-                request.user.has_perm(NodePermission.admin, machine)):
-            raise PermissionDenied()
-        if options.install_kvm and (
-                machine.ephemeral_deployment or options.ephemeral_deploy):
-            raise MAASAPIBadRequest(
-                "Cannot install KVM host for ephemeral deployments.")
         if not machine.distro_series and not series:
             series = Config.objects.get_config('default_distro_series')
         Form = get_machine_edit_form(request.user)
@@ -728,8 +728,6 @@ class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
             form.set_hwe_kernel(hwe_kernel=hwe_kernel)
         if options.install_rackd:
             form.set_install_rackd(install_rackd=options.install_rackd)
-        if options.install_kvm:
-            form.set_install_kvm(install_kvm=options.install_kvm)
         if options.ephemeral_deploy:
             form.set_ephemeral_deploy(
                 ephemeral_deploy=options.ephemeral_deploy)
diff --git a/src/maasserver/api/nodes.py b/src/maasserver/api/nodes.py
index e0229db..ae33551 100644
--- a/src/maasserver/api/nodes.py
+++ b/src/maasserver/api/nodes.py
@@ -16,6 +16,10 @@ import bson
 from django.db.models import Prefetch
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404
+from formencode.validators import (
+    Int,
+    StringBool,
+)
 from maasserver.api.support import (
     admin_method,
     AnonymousOperationsHandler,
@@ -905,7 +909,21 @@ class PowerMixin:
         if user_data is not None:
             user_data = b64decode(user_data)
         try:
-            node.start(request.user, user_data=user_data, comment=comment)
+            # These parameters are passed in the request from
+            # maasserver.api.machines.deploy when powering on
+            # the node for deployment.
+            install_kvm = get_optional_param(
+                request.POST, 'install_kvm',
+                default=False, validator=StringBool)
+            bridge_stp = get_optional_param(
+                request.POST, 'bridge_stp', default=None,
+                validator=StringBool)
+            bridge_fd = get_optional_param(
+                request.POST, 'bridge_fd', default=None, validator=Int)
+            node.start(
+                request.user, user_data=user_data, comment=comment,
+                install_kvm=install_kvm, bridge_stp=bridge_stp,
+                bridge_fd=bridge_fd)
         except StaticIPAddressExhaustion:
             # The API response should contain error text with the
             # system_id in it, as that is the primary API key to a node.
diff --git a/src/maasserver/api/tests/test_machine.py b/src/maasserver/api/tests/test_machine.py
index d4a218e..6eb2cd6 100644
--- a/src/maasserver/api/tests/test_machine.py
+++ b/src/maasserver/api/tests/test_machine.py
@@ -678,7 +678,8 @@ class TestMachineAPI(APITestCase.ForUser):
                 'comment': comment,
             })
         self.assertThat(machine_start, MockCalledOnceWith(
-            self.user, user_data=ANY, comment=comment))
+            self.user, user_data=ANY, comment=comment,
+            install_kvm=ANY, bridge_stp=ANY, bridge_fd=ANY))
 
     def test_POST_deploy_handles_missing_comment(self):
         machine = factory.make_Node(
@@ -698,7 +699,8 @@ class TestMachineAPI(APITestCase.ForUser):
                 'distro_series': distro_series,
             })
         self.assertThat(machine_start, MockCalledOnceWith(
-            self.user, user_data=ANY, comment=None))
+            self.user, user_data=ANY, comment=None,
+            install_kvm=ANY, bridge_stp=ANY, bridge_fd=ANY))
 
     def test_POST_deploy_doesnt_reset_power_options_bug_1569102(self):
         self.become_admin()
diff --git a/src/maasserver/forms/__init__.py b/src/maasserver/forms/__init__.py
index 021acdd..2341511 100644
--- a/src/maasserver/forms/__init__.py
+++ b/src/maasserver/forms/__init__.py
@@ -874,11 +874,6 @@ class MachineForm(NodeForm):
         self.is_bound = True
         self.data['install_rackd'] = install_rackd
 
-    def set_install_kvm(self, install_kvm=False):
-        """Sets whether to deploy the rack alongside this machine."""
-        self.is_bound = True
-        self.data['install_kvm'] = install_kvm
-
     def set_ephemeral_deploy(self, ephemeral_deploy=False):
         """Sets whether to deploy this machine ephemerally."""
         self.is_bound = True
@@ -916,7 +911,6 @@ class MachineForm(NodeForm):
             'min_hwe_kernel',
             'hwe_kernel',
             'install_rackd',
-            'install_kvm',
             'ephemeral_deploy',
             'commission'
         )
diff --git a/src/maasserver/forms/tests/test_machine.py b/src/maasserver/forms/tests/test_machine.py
index 9a79406..84be9bf 100644
--- a/src/maasserver/forms/tests/test_machine.py
+++ b/src/maasserver/forms/tests/test_machine.py
@@ -50,7 +50,6 @@ class TestMachineForm(MAASServerTestCase):
                 'min_hwe_kernel',
                 'hwe_kernel',
                 'install_rackd',
-                'install_kvm',
                 'ephemeral_deploy',
                 'commission',
             ], list(form.fields))
@@ -370,7 +369,6 @@ class TestAdminMachineForm(MAASServerTestCase):
                 'min_hwe_kernel',
                 'hwe_kernel',
                 'install_rackd',
-                'install_kvm',
                 'ephemeral_deploy',
                 'cpu_count',
                 'memory',
diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 10af042..40af61c 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -4388,10 +4388,16 @@ class Node(CleanSave, TimestampedModel):
         return NODE_STATUS_CHOICES_DICT[self.status]
 
     @transactional
-    def start(self, user, user_data=None, comment=None):
+    def start(
+            self, user, user_data=None, comment=None, install_kvm=None,
+            bridge_stp=None, bridge_fd=None):
         if not user.has_perm(NodePermission.edit, self):
             # You can't start a node you don't own unless you're an admin.
             raise PermissionDenied()
+        # Set install_kvm if not already set.
+        if not self.install_kvm and install_kvm:
+            self.install_kvm = True
+            self.save()
         event = EVENT_TYPES.REQUEST_NODE_START
         allow_power_cycle = False
         # If status is ALLOCATED, this start is actually for a deployment.
@@ -4401,6 +4407,9 @@ class Node(CleanSave, TimestampedModel):
         if self.status == NODE_STATUS.ALLOCATED:
             event = EVENT_TYPES.REQUEST_NODE_START_DEPLOYMENT
             allow_power_cycle = True
+            if self.install_kvm:
+                self._create_acquired_bridges(
+                    bridge_stp=bridge_stp, bridge_fd=bridge_fd)
         # Bug #1630361: Make sure that there is a maas_facing_server_address in
         # the same address family as our configured interfaces.
         # Every node in a real system has a rack controller, but many tests do
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 2a721ae..cc7b45e 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -7108,6 +7108,23 @@ class TestNode_Start(MAASTransactionServerTestCase):
         node.start(user)
         self.assertEquals(NODE_STATUS.DEPLOYING, node.status)
 
+    def test__creates_acquired_bridges_for_install_kvm(self):
+        user = factory.make_User()
+        node = self.make_acquired_node_with_interface(
+            user, power_type="manual")
+        bridge_stp = factory.pick_bool()
+        bridge_fd = random.randint(0, 500)
+        node.start(
+            user, install_kvm=True, bridge_stp=bridge_stp, bridge_fd=bridge_fd)
+        node = reload_object(node)
+        bridge = BridgeInterface.objects.get(node=node)
+        interface = node.interface_set.first()
+        self.assertEquals(NODE_STATUS.DEPLOYING, node.status)
+        self.assertEquals(bridge.mac_address, interface.mac_address)
+        self.assertEquals(bridge.params['bridge_stp'], bridge_stp)
+        self.assertEquals(bridge.params['bridge_fd'], bridge_fd)
+        self.assertTrue(node.install_kvm)
+
     def test__doesnt_change_broken(self):
         user = factory.make_User()
         node = self.make_acquired_node_with_interface(
diff --git a/src/maasserver/node_action.py b/src/maasserver/node_action.py
index e969138..32239ec 100644
--- a/src/maasserver/node_action.py
+++ b/src/maasserver/node_action.py
@@ -450,9 +450,7 @@ class Deploy(NodeAction):
         if self.node.owner is None:
             with locks.node_acquire:
                 try:
-                    bridge_all = True if install_kvm else False
-                    self.node.acquire(
-                        self.user, token=None, bridge_all=bridge_all)
+                    self.node.acquire(self.user, token=None)
                 except ValidationError as e:
                     raise NodeActionError(e)
         if install_kvm:
diff --git a/src/maasserver/tests/test_node_action.py b/src/maasserver/tests/test_node_action.py
index 8365260..7b3e256 100644
--- a/src/maasserver/tests/test_node_action.py
+++ b/src/maasserver/tests/test_node_action.py
@@ -724,8 +724,8 @@ class TestDeployAction(MAASServerTestCase):
         Deploy(node, user, request).execute(**extra)
         self.expectThat(mock_get_curtin_config, MockCalledOnceWith(ANY, node))
         # Make sure we set bridge_all when we acquire the Node.
-        self.expectThat(mock_node_acquire, MockCalledOnceWith(
-            user, token=None, bridge_all=True))
+        self.expectThat(
+            mock_node_acquire, MockCalledOnceWith(user, token=None))
         self.expectThat(mock_node_start, MockCalledOnceWith(user))
         # Make sure ubuntu/bionic is selected if KVM pod deployment has been
         # selected.