Merge lp:~mpontillo/maas/fix-deploy-auto-allocate--bug-1672363 into lp:~maas-committers/maas/trunk

[Mike Pontillo]

Allow an individual machine to be allocated and deployed from the API in one step.

[Blake Rouse (blake-rouse)]

Looks good.

review: Approve

=== modified file 'src/maasserver/api/machines.py'
--- src/maasserver/api/machines.py	2017-04-18 00:33:14 +0000
+++ src/maasserver/api/machines.py	2017-04-21 20:21:20 +0000
@@ -185,6 +185,21 @@
     return storage_layout, params
 
 
+def get_allocation_parameters(request):
+    """Returns shared parameters for deploy and allocate operations."""
+    comment = get_optional_param(request.POST, 'comment')
+    bridge_all = get_optional_param(
+        request.POST, 'bridge_all', default=False,
+        validator=StringBool)
+    bridge_stp = get_optional_param(
+        request.POST, 'bridge_stp', default=False,
+        validator=StringBool)
+    bridge_fd = get_optional_param(
+        request.POST, 'bridge_fd', default=0, validator=Int)
+    agent_name = request.data.get('agent_name', '')
+    return agent_name, bridge_all, bridge_fd, bridge_stp, comment
+
+
 class MachineHandler(NodeHandler, OwnerDataMixin, PowerMixin):
     """Manage an individual Machine.
 
@@ -360,6 +375,21 @@
         :param hwe_kernel: If present, this parameter specified the kernel to
             be used on the machine
         :type hwe_kernel: unicode
+        :param agent_name: An optional agent name to attach to the
+            acquired machine.
+        :type agent_name: unicode
+        :param bridge_all: Optionally create a bridge interface for every
+            configured interface on the machine. The created bridges will be
+            removed once the machine is released.
+            (Default: False)
+        :type bridge_all: boolean
+        :param bridge_stp: Optionally turn spanning tree protocol on or off
+            for the bridges created on every configured interface.
+            (Default: off)
+        :type bridge_stp: boolean
+        :param bridge_fd: Optionally adjust the forward delay to time seconds.
+            (Default: 15)
+        :type bridge_fd: integer
         :param comment: Optional comment for the event log.
         :type comment: unicode
 
@@ -377,14 +407,28 @@
         series = request.POST.get('distro_series', None)
         license_key = request.POST.get('license_key', None)
         hwe_kernel = request.POST.get('hwe_kernel', None)
-
+        # Acquiring a node requires VIEW permissions.
         machine = self.model.objects.get_node_or_404(
             system_id=system_id, user=request.user,
-            perm=NODE_PERMISSION.EDIT)
-
-        if machine.owner is None:
-            raise NodeStateViolation(
-                "Can't start machine: it hasn't been allocated.")
+            perm=NODE_PERMISSION.VIEW)
+        if machine.status == NODE_STATUS.READY:
+            with locks.node_acquire:
+                if machine.owner is not None and machine.owner != request.user:
+                    raise NodeStateViolation(
+                        "Can't allocate a machine belonging to another user.")
+                agent_name, bridge_all, bridge_fd, bridge_stp, comment = (
+                    get_allocation_parameters(request))
+                maaslog.info(
+                    "Request from user %s to acquire machine: %s (%s)",
+                    request.user.username, machine.fqdn, machine.system_id)
+                machine.acquire(
+                    request.user, get_oauth_token(request),
+                    agent_name=agent_name, comment=comment,
+                    bridge_all=bridge_all, bridge_stp=bridge_stp,
+                    bridge_fd=bridge_fd)
+        # Deploying a node requires re-checking for EDIT permissions.
+        if not request.user.has_perm(NODE_PERMISSION.EDIT, machine):
+            raise PermissionDenied()
         if not machine.distro_series and not series:
             series = Config.objects.get_config('default_distro_series')
         Form = get_machine_edit_form(request.user)
@@ -1355,18 +1399,13 @@
         # XXX AndresRodriguez 2016-10-27: If new params are added and are not
         # constraints, these need to be added to IGNORED_FIELDS in
         # src/maasserver/node_constraint_filter_forms.py.
-        comment = get_optional_param(request.POST, 'comment')
         input_constraints = str([
             param for param in request.data.lists() if param[0] != 'op'])
         maaslog.info(
             "Request from user %s to acquire a machine with constraints: %s",
             request.user.username, input_constraints)
-        bridge_all = get_optional_param(
-            request.POST, 'bridge_all', default=False, validator=StringBool)
-        bridge_stp = get_optional_param(
-            request.POST, 'bridge_stp', default=False, validator=StringBool)
-        bridge_fd = get_optional_param(
-            request.POST, 'bridge_fd', default=False, validator=Int)
+        agent_name, bridge_all, bridge_fd, bridge_stp, comment = (
+            get_allocation_parameters(request))
         verbose = get_optional_param(
             request.POST, 'verbose', default=False, validator=StringBool)
         dry_run = get_optional_param(
@@ -1434,7 +1473,6 @@
                         '(resolved to "%s")' % (
                             input_constraints, constraints))
                 raise NodesNotAvailable(message)
-            agent_name = request.data.get('agent_name', '')
             if not dry_run:
                 machine.acquire(
                     request.user, get_oauth_token(request),

=== modified file 'src/maasserver/api/tests/test_machine.py'
--- src/maasserver/api/tests/test_machine.py	2017-04-17 19:44:23 +0000
+++ src/maasserver/api/tests/test_machine.py	2017-04-21 20:21:20 +0000
@@ -598,6 +598,103 @@
         response_content = json_load_bytes(response.content)
         self.assertEquals('virsh', response_content['power_type'])
 
+    def test_POST_deploy_allocates_ready_machines(self):
+        self.patch(node_module.Node, "_start")
+        machine = factory.make_Node(
+            status=NODE_STATUS.READY, interface=True,
+            power_type='manual',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        request = {
+            'op': 'deploy',
+            'distro_series': distro_series,
+        }
+        response = self.client.post(self.get_machine_uri(machine), request)
+        self.assertEqual(http.client.OK, response.status_code)
+
+    def test_POST_deploy_rejects_node_owned_by_another_user(self):
+        self.patch(node_module.Node, "_start")
+        user2 = factory.make_User()
+        machine = factory.make_Node(
+            status=NODE_STATUS.READY, owner=user2, interface=True,
+            power_type='manual',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        request = {
+            'op': 'deploy',
+            'distro_series': distro_series,
+        }
+        response = self.client.post(self.get_machine_uri(machine), request)
+        self.assertEqual(http.client.CONFLICT, response.status_code)
+
+    def test_POST_deploy_passes_agent_name(self):
+        self.patch(node_module.Node, "_start")
+        machine = factory.make_Node(
+            status=NODE_STATUS.READY, interface=True,
+            power_type='manual',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        request = {
+            'op': 'deploy',
+            'distro_series': distro_series,
+            'agent_name': factory.make_name(),
+            'comment': factory.make_name(),
+        }
+        response = self.client.post(self.get_machine_uri(machine), request)
+        self.assertEqual(http.client.OK, response.status_code)
+        machine = reload_object(machine)
+        self.assertThat(machine.agent_name, Equals(request['agent_name']))
+
+    def test_POST_deploy_passes_comment_on_acquire(self):
+        self.patch(node_module.Node, "_start")
+        machine_method = self.patch(node_module.Machine, 'acquire')
+        machine = factory.make_Node(
+            status=NODE_STATUS.READY, owner=self.user, interface=True,
+            power_type='manual',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        request = {
+            'op': 'deploy',
+            'distro_series': distro_series,
+            'agent_name': factory.make_name(),
+            'comment': factory.make_name(),
+        }
+        response = self.client.post(self.get_machine_uri(machine), request)
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertThat(
+            machine_method, MockCalledOnceWith(
+                ANY, ANY, agent_name=ANY,
+                bridge_all=False, bridge_fd=False,
+                bridge_stp=False, comment=request['comment']))
+
+    def test_POST_deploy_passes_bridge_settings(self):
+        self.patch(node_module.Node, "_start")
+        machine_method = self.patch(node_module.Machine, 'acquire')
+        machine = factory.make_Node(
+            status=NODE_STATUS.READY, owner=self.user, interface=True,
+            power_type='manual',
+            architecture=make_usable_architecture(self))
+        osystem = make_usable_osystem(self)
+        distro_series = osystem['default_release']
+        request = {
+            'op': 'deploy',
+            'distro_series': distro_series,
+            'bridge_all': True,
+            'bridge_stp': True,
+            'bridge_fd': 7,
+        }
+        response = self.client.post(self.get_machine_uri(machine), request)
+        self.assertEqual(http.client.OK, response.status_code)
+        self.assertThat(
+            machine_method, MockCalledOnceWith(
+                ANY, ANY, agent_name=ANY,
+                bridge_all=True, bridge_fd=7,
+                bridge_stp=True, comment=None))
+
     def test_POST_release_releases_owned_machine(self):
         self.patch(node_module.Machine, '_stop')
         owned_statuses = [

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2017-04-18 00:33:14 +0000
+++ src/maasserver/models/node.py	2017-04-21 20:21:20 +0000
@@ -2497,7 +2497,7 @@
             self, user, token=None, agent_name='', comment=None,
             bridge_all=False, bridge_stp=None, bridge_fd=None):
         """Mark commissioned node as acquired by the given user and token."""
-        assert self.owner is None
+        assert self.owner is None or self.owner == user
         assert token is None or token.user == user
 
         self._create_acquired_filesystems()