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

[Newell Jensen]

LP: #1820984 -- Local boot a device if it tries PXE booting from MAAS.

[Blake Rouse (blake-rouse)]

Code path looks good, but need to fix log message.

review: Needs Fixing

[Andres Rodriguez (andreserl)]

review: Needs Fixing

[Andres Rodriguez (andreserl)]

A quick q, for the devices we are managing or tracking in MAAS, do we /always/ create dhcp hostmaps ?

[Newell Jensen (newell-jensen)]

> A quick q, for the devices we are managing or tracking in MAAS, do we /always/
> create dhcp hostmaps ?

No, I do not seeing any hostmaps in dhcpd.conf for devices. I updated the log message to use warning instead of info as well as the unit test.

[Andres Rodriguez (andreserl)]

@Newell,

Can you provide the output of how things show up in both maas.log and rackd.log ? Looking at the bug report, the user was looking at rackd.log and i think we should also have such message in rackd.log (and in maas.log too).

review: Needs Fixing

[Andres Rodriguez (andreserl)]

another comment inline.

[Andres Rodriguez (andreserl)]

see inline

review: Needs Fixing

[Newell Jensen (newell-jensen)]

> @Newell,
>
> Can you provide the output of how things show up in both maas.log and
> rackd.log ? Looking at the bug report, the user was looking at rackd.log and i
> think we should also have such message in rackd.log (and in maas.log too).

Currently, there is not output from maas.log when booting a device from MAAS. Only current output that we get for this is in rackd.log as shown here:

https://paste.ubuntu.com/p/zcqDPm9yY9/

[Newell Jensen (newell-jensen)]

Andres and Blake, do you guys both agree that this should be logged in both maas.log and rackd.log? If not, which one?

[Blake Rouse (blake-rouse)]

Having the log in both places is acceptable.

[Blake Rouse (blake-rouse)]

Looks good. Thanks for all the fixes and working through this.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1820984 lp:~newell-jensen/maas/+git/maas into -b master lp:~maas-committers/maas

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

diff --git a/src/maasserver/models/node.py b/src/maasserver/models/node.py
index 10af042..0e7f4c5 100644
--- a/src/maasserver/models/node.py
+++ b/src/maasserver/models/node.py
@@ -1351,6 +1351,9 @@ class Node(CleanSave, TimestampedModel):
     @property
     def ephemeral_deployment(self):
         """Return if node is set to ephemeral deployment."""
+        # Devices should always local boot.
+        if self.is_device:
+            return False
         return self.is_diskless or self.ephemeral_deploy
 
     def retrieve_storage_layout_issues(
@@ -4205,14 +4208,10 @@ class Node(CleanSave, TimestampedModel):
         """
         Return a suitable "purpose" for this boot, e.g. "install".
         """
-        # XXX: allenap bug=1031406 2012-07-31: The boot purpose is
-        # still in flux. It may be that there will just be an
-        # "ephemeral" environment and an "install" environment, and
-        # the differing behaviour between, say, enlistment and
-        # commissioning - both of which will use the "ephemeral"
-        # environment - will be governed by varying the preseed or PXE
-        # configuration.
-        if self.status in COMMISSIONING_LIKE_STATUSES:
+        if self.status == NODE_STATUS.DEFAULT and self.is_device:
+            # Always local boot a device.
+            return "local"
+        elif self.status in COMMISSIONING_LIKE_STATUSES:
             # It is commissioning or disk erasing. The environment (boot
             # images, kernel options, etc for erasing is the same as that
             # of commissioning.
diff --git a/src/maasserver/models/tests/test_node.py b/src/maasserver/models/tests/test_node.py
index 2a721ae..548659c 100644
--- a/src/maasserver/models/tests/test_node.py
+++ b/src/maasserver/models/tests/test_node.py
@@ -1051,6 +1051,10 @@ class TestNode(MAASServerTestCase):
         node = factory.make_Node(ephemeral_deploy=False)
         self.assertFalse(node.ephemeral_deployment)
 
+    def test_ephemeral_deployment_checks_is_device(self):
+        device = factory.make_Device()
+        self.assertFalse(device.ephemeral_deployment)
+
     def test_system_id_is_a_valid_znum(self):
         node = factory.make_Node()
         self.assertThat(
@@ -3990,6 +3994,9 @@ class TestNode(MAASServerTestCase):
             ("local", {"status": NODE_STATUS.DEPLOYING, "netboot": False}),
             ("local", {"status": NODE_STATUS.DEPLOYED}),
             ("poweroff", {"status": NODE_STATUS.RETIRED}),
+            ("local", {
+                "status": NODE_STATUS.DEFAULT,
+                "node_type": NODE_TYPE.DEVICE}),
         ]
         node = factory.make_Node()
         mock_get_boot_images_for = self.patch(
diff --git a/src/maasserver/rpc/boot.py b/src/maasserver/rpc/boot.py
index 4cbd7ab..e15c817 100644
--- a/src/maasserver/rpc/boot.py
+++ b/src/maasserver/rpc/boot.py
@@ -39,6 +39,7 @@ from maasserver.third_party_drivers import get_third_party_driver
 from maasserver.utils.orm import transactional
 from maasserver.utils.osystems import validate_hwe_kernel
 from provisioningserver.events import EVENT_TYPES
+from provisioningserver.logger import get_maas_logger
 from provisioningserver.rpc.exceptions import BootConfigNoResponse
 from provisioningserver.utils.network import get_source_address
 from provisioningserver.utils.twisted import (
@@ -48,6 +49,9 @@ from provisioningserver.utils.twisted import (
 from provisioningserver.utils.url import splithost
 
 
+maaslog = get_maas_logger("rpc.boot")
+
+
 DEFAULT_ARCH = 'i386'
 
 
@@ -293,7 +297,7 @@ def get_final_boot_purpose(machine, arch, purpose):
 def get_config(
         system_id, local_ip, remote_ip, arch=None, subarch=None, mac=None,
         hardware_uuid=None, bios_boot_method=None):
-    """Get the booting configration for the a machine.
+    """Get the booting configration for a machine.
 
     Returns a structure suitable for returning in the response
     for :py:class:`~provisioningserver.rpc.region.GetBootConfig`.
@@ -408,6 +412,16 @@ def get_config(
 
         # Early out if the machine is booting local.
         if purpose == 'local':
+            if machine.is_device:
+                # Log that we are setting to local boot for a device.
+                maaslog.warning(
+                    "Device %s with MAC address %s is PXE booting; "
+                    "instructing the device to boot locally." % (
+                        machine.hostname, mac))
+                # Set the purpose to 'local-device' so we can log a message
+                # on the rack.
+                purpose = 'local-device'
+
             return {
                 "system_id": machine.system_id,
                 "arch": arch,
diff --git a/src/maasserver/rpc/tests/test_boot.py b/src/maasserver/rpc/tests/test_boot.py
index 1501348..a0bfd50 100644
--- a/src/maasserver/rpc/tests/test_boot.py
+++ b/src/maasserver/rpc/tests/test_boot.py
@@ -16,6 +16,7 @@ from maasserver.enum import (
     INTERFACE_TYPE,
     IPADDRESS_TYPE,
     NODE_STATUS,
+    NODE_TYPE,
 )
 from maasserver.models import (
     Config,
@@ -261,6 +262,47 @@ class TestGetConfig(MAASServerTestCase):
             "http_boot": True,
         }, config)
 
+    def test__changes_purpose_to_local_device_for_device(self):
+        rack_controller = factory.make_RackController()
+        local_ip = factory.make_ip_address()
+        remote_ip = factory.make_ip_address()
+        device = self.make_node_with_extra(
+            status=NODE_STATUS.DEPLOYED, netboot=False,
+            node_type=NODE_TYPE.DEVICE)
+        device.boot_cluster_ip = local_ip
+        device.save()
+        mac = device.get_boot_interface().mac_address
+        self.patch_autospec(boot_module, 'event_log_pxe_request')
+        maaslog = self.patch(boot_module, 'maaslog')
+        config = get_config(
+            rack_controller.system_id, local_ip, remote_ip, mac=mac,
+            query_count=8)
+        self.assertEquals({
+            "system_id": device.system_id,
+            "arch": device.split_arch()[0],
+            "subarch": device.split_arch()[1],
+            "osystem": '',
+            "release": '',
+            "kernel": '',
+            "initrd": '',
+            "boot_dtb": '',
+            "purpose": 'local-device',
+            "hostname": device.hostname,
+            "domain": device.domain.name,
+            "preseed_url": ANY,
+            "fs_host": local_ip,
+            "log_host": local_ip,
+            "log_port": 5247,
+            "extra_opts": '',
+            "http_boot": True,
+        }, config)
+        self.assertThat(
+            maaslog.warning,
+            MockCalledOnceWith(
+                "Device %s with MAC address %s is PXE booting; "
+                "instructing the device to boot locally." % (
+                    device.hostname, mac)))
+
     def test__purpose_local_to_xinstall_for_ephemeral_deployment(self):
         # A diskless node is one that it is ephemerally deployed.
         rack_controller = factory.make_RackController()
diff --git a/src/provisioningserver/rackdservices/tests/test_tftp.py b/src/provisioningserver/rackdservices/tests/test_tftp.py
index 7b58405..ebb7934 100644
--- a/src/provisioningserver/rackdservices/tests/test_tftp.py
+++ b/src/provisioningserver/rackdservices/tests/test_tftp.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 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).
 
 """Tests for the maastftp Twisted plugin."""
@@ -379,8 +379,6 @@ class TestTFTPBackend(MAASTestCase):
 
     @inlineCallbacks
     def test_get_boot_method_reader_uses_same_client(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters()
         fake_params = fake_kernel_params._asdict()
@@ -450,8 +448,6 @@ class TestTFTPBackend(MAASTestCase):
 
     @inlineCallbacks
     def test_get_boot_method_reader_uses_different_clients(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters()
         fake_params = fake_kernel_params._asdict()
@@ -524,8 +520,6 @@ class TestTFTPBackend(MAASTestCase):
 
     @inlineCallbacks
     def test_get_boot_method_reader_grabs_new_client_on_lost_conn(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters()
         fake_params = fake_kernel_params._asdict()
@@ -602,8 +596,6 @@ class TestTFTPBackend(MAASTestCase):
 
     @inlineCallbacks
     def test_get_boot_method_reader_returns_rendered_params(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters()
         fake_params = fake_kernel_params._asdict()
@@ -657,8 +649,6 @@ class TestTFTPBackend(MAASTestCase):
 
     @inlineCallbacks
     def test_get_boot_method_reader_returns_rendered_params_for_local(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters(
             purpose="local", label="local")
@@ -699,9 +689,51 @@ class TestTFTPBackend(MAASTestCase):
             backend, kernel_params=fake_kernel_params, **params_with_ip))
 
     @inlineCallbacks
+    def test_get_boot_method_reader_returns_rendered_params_local_device(self):
+        # Fake kernel configuration parameters, as returned from the RPC call.
+        fake_kernel_params = make_kernel_parameters(
+            purpose="local", label="local")
+        fake_params = fake_kernel_params._asdict()
+        del fake_params["label"]
+
+        # Set purpose to `local-device` as this is what will be passed on.
+        fake_params["purpose"] = "local-device"
+
+        # Stub RPC call to return the fake configuration parameters.
+        client = Mock()
+        client.localIdent = factory.make_name("system_id")
+        client.return_value = succeed(fake_params)
+        client_service = Mock()
+        client_service.getClientNow.return_value = succeed(client)
+
+        # get_boot_method_reader() takes a dict() of parameters and returns an
+        # `IReader` of a PXE configuration, rendered by
+        # `PXEBootMethod.get_reader`.
+        backend = TFTPBackend(
+            self.make_dir(), client_service)
+
+        # Stub get_reader to return the render parameters.
+        method = PXEBootMethod()
+        fake_render_result = factory.make_name("render").encode("utf-8")
+        render_patch = self.patch(method, "get_reader")
+        render_patch.return_value = BytesReader(fake_render_result)
+
+        # Get the rendered configuration, which will actually be a JSON dump
+        # of the render-time parameters.
+        params_with_ip = dict(fake_params)
+        params_with_ip['remote_ip'] = factory.make_ipv4_address()
+        reader = yield backend.get_boot_method_reader(method, params_with_ip)
+        self.addCleanup(reader.finish)
+        self.assertIsInstance(reader, BytesReader)
+        output = reader.read(10000)
+
+        # The result has been rendered by `method.get_reader`.
+        self.assertEqual(fake_render_result, output)
+        self.assertThat(method.get_reader, MockCalledOnceWith(
+            backend, kernel_params=fake_kernel_params, **params_with_ip))
+
+    @inlineCallbacks
     def test_get_boot_method_reader_returns_no_image(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters(label='no-such-image')
         fake_params = fake_kernel_params._asdict()
@@ -745,8 +777,6 @@ class TestTFTPBackend(MAASTestCase):
 
     @inlineCallbacks
     def test_get_boot_method_reader_rendered_parms_for_depoyed_ephemeral(self):
-        # Fake configuration parameters, as discovered from the file path.
-        fake_params = {"mac": factory.make_mac_address("-")}
         # Fake kernel configuration parameters, as returned from the RPC call.
         fake_kernel_params = make_kernel_parameters(
             purpose="local", label="local", osystem="caringo")
diff --git a/src/provisioningserver/rackdservices/tftp.py b/src/provisioningserver/rackdservices/tftp.py
index 0512b10..6a919fe 100644
--- a/src/provisioningserver/rackdservices/tftp.py
+++ b/src/provisioningserver/rackdservices/tftp.py
@@ -1,4 +1,4 @@
-# Copyright 2012-2016 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).
 
 """Twisted Application Plugin for the MAAS TFTP server."""
@@ -36,6 +36,7 @@ from provisioningserver.rpc.region import (
     MarkNodeFailed,
 )
 from provisioningserver.utils import (
+    network,
     tftp,
     typed,
 )
@@ -236,10 +237,20 @@ class TFTPBackend(FilesystemSynchronousBackend):
         except Exception:
             pass
 
+        # Check to see if the we are PXE booting a device.
+        if params["purpose"] == "local-device":
+            mac = network.find_mac_via_arp(remote_ip)
+            log.info(
+                "Device %s with MAC address %s is PXE booting; "
+                "instructing the device to boot locally." % (
+                    params["hostname"], mac))
+            # Set purpose back to local now that we have the message logged.
+            params["purpose"] = "local"
+
         system_id = params.pop("system_id", None)
         if params["purpose"] == "local" and not is_ephemeral:
-            # Local purpose doesn't use a boot image so jsut set the label
-            # to "local", but this value will no be used.
+            # Local purpose doesn't use a boot image so just set the label
+            # to "local".
             params["label"] = "local"
             return params
         else: