Merge ~ltrager/maas:lp1888021 into maas:master

[Lee Trager]

LP: #1888021 - Read storage MODEL and MODEL_ENC from udev

Storage devices may have two model names. One is encoded to allow spaces
and other special characters, the other is not. lsblk in Xenial and Bionic
give the encoded model name while in Focal it gives the non-encode model
name. LXD gives the encoded model name in LXD 4.3+. Read and store both
from udev to ensure matching works.

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1888021 lp:~ltrager/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 5ee39f8396cbb7f1ac26e2818cbbb7c403a131de

review: Approve

[Alberto Donato (ack)]

+1

review: Approve

diff --git a/src/machine-resources/src/machine-resources/Gopkg.lock b/src/machine-resources/src/machine-resources/Gopkg.lock
index 52abaeb..fbd28e4 100644
--- a/src/machine-resources/src/machine-resources/Gopkg.lock
+++ b/src/machine-resources/src/machine-resources/Gopkg.lock
@@ -22,8 +22,8 @@
 [[projects]]
   branch = "master"
   name = "github.com/lxc/lxd"
-  packages = ["lxd/resources","shared","shared/api","shared/cancel","shared/ioprogress","shared/logger","shared/osarch","shared/units","shared/usbid","shared/version"]
-  revision = "44487eda6756305828ea8aaf7555225f61d1d292"
+  packages = ["lxd/include","lxd/resources","shared","shared/api","shared/cancel","shared/ioprogress","shared/logger","shared/osarch","shared/units","shared/usbid","shared/validate","shared/version"]
+  revision = "4c7cd25c6842f49f3a29468aa745ee97651d42bd"
 
 [[projects]]
   name = "github.com/mitchellh/go-homedir"
@@ -41,7 +41,7 @@
   branch = "master"
   name = "golang.org/x/sys"
   packages = ["internal/unsafeheader","unix","windows"]
-  revision = "ddb9806d33aed8dbaac1cd6f1cba58952e87f933"
+  revision = "0cf7623e9dbd9bc79267b27a7e6c5757ef550423"
 
 [[projects]]
   branch = "v2"
diff --git a/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py b/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py
index 68819e4..ea9def3 100755
--- a/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py
+++ b/src/metadataserver/user_data/templates/snippets/maas_run_remote_scripts.py
@@ -5,7 +5,7 @@
 #
 # Author: Lee Trager <[e-mail]>
 #
-# Copyright (C) 2017-2019 Canonical
+# Copyright (C) 2017-2020 Canonical
 #
 # This program is free software: you can redistribute it and/or modify
 # it under the terms of the GNU Affero General Public License as
@@ -582,6 +582,54 @@ class CustomNetworking:
             self._apply_ephemeral_netplan()
 
 
+def udev_decode(string):
+    """Decode udev encoded values
+
+    Inverse of
+    https://github.com/systemd/systemd/blob/master/src/basic/device-nodes.c#L22
+    Go version in LXD
+    https://github.com/lxc/lxd/blob/master/lxd/resources/utils.go#L106
+    """
+    ret = ""
+    i = 0
+    while i < len(string):
+        if string[i] == "\\" and i + 4 < len(string) and string[i + 1] == "x":
+            ret += chr(int(string[i + 2 : i + 4], 16))
+            i += 4
+        else:
+            ret += string[i]
+            i += 1
+    return ret.strip()
+
+
+def get_storage_model_from_udev(block_dev):
+    """Get the storage model name from udev
+
+    Storage devices may have two model names. One is encoded to allow
+    spaces and other special characters, the other is not. lsblk in Xenial
+    and Bionic give the encoded model name while in Focal it gives the
+    non-encoded model name. LXD gives the encoded model name in LXD 4.3+. Read
+    and store both from udev to ensure matching works.
+    """
+    udev_path = "/run/udev/data/b%s" % block_dev["MAJ:MIN"]
+    if not os.path.exists(udev_path):
+        # udev should always be available, if udev_path isn't found something
+        # changed. lsblk will still give a model name, try using that.
+        sys.stderr.write(
+            "WARNING: Unable to read block device data from "
+            "udev(%s)" % udev_path
+        )
+        block_dev["MODEL_ENC"] = block_dev["MODEL"]
+        return block_dev
+    with open(udev_path, "r") as f:
+        for line in f.readlines():
+            if line.startswith("E:ID_MODEL_ENC"):
+                block_dev["MODEL_ENC"] = udev_decode(line.split("=", 1)[1])
+            elif line.startswith("E:ID_MODEL"):
+                block_dev["MODEL"] = line.split("=", 1)[1].strip()
+    return block_dev
+
+
 # Cache the block devices so we only have to query once.
 _block_devices = None
 _block_devices_lock = Lock()
@@ -604,7 +652,7 @@ def get_block_devices():
                 "-d",
                 "-P",
                 "-o",
-                "NAME,MODEL,SERIAL",
+                "NAME,MODEL,SERIAL,MAJ:MIN",
             ]
             block_list = check_output(cmd, timeout=60).decode("utf-8")
         except TimeoutExpired:
@@ -626,7 +674,9 @@ def get_block_devices():
                 for token in tokens:
                     k, v = token.split("=", 1)
                     current_block[k] = v.strip()
-                block_devices.append(current_block)
+                block_devices.append(
+                    get_storage_model_from_udev(current_block)
+                )
             # LP:1732539 - Don't fill cache until all results are proceeded.
             _block_devices = block_devices
 
@@ -739,8 +789,8 @@ def parse_parameters(script, scripts_dir):
                 for blockdev in get_block_devices():
                     if (
                         model == blockdev["MODEL"]
-                        and serial == blockdev["SERIAL"]
-                    ):
+                        or model == blockdev.get("MODEL_ENC")
+                    ) and serial == blockdev["SERIAL"]:
                         value["path"] = value["input"] = (
                             "/dev/%s" % blockdev["NAME"]
                         )
diff --git a/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py b/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py
index e322ca3..5a6940b 100644
--- a/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py
+++ b/src/metadataserver/user_data/templates/snippets/tests/test_maas_run_remote_scripts.py
@@ -1,4 +1,4 @@
-# Copyright 2017-2019 Canonical Ltd.  This software is licensed under the
+# Copyright 2017-2020 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for maas_run_remote_scripts.py."""
@@ -15,8 +15,9 @@ import random
 import stat
 from subprocess import CalledProcessError, DEVNULL, PIPE, TimeoutExpired
 import tarfile
+from textwrap import dedent
 import time
-from unittest.mock import ANY, call, MagicMock
+from unittest.mock import ANY, call, MagicMock, mock_open
 from zipfile import ZipFile
 
 import yaml
@@ -39,6 +40,7 @@ from snippets.maas_run_remote_scripts import (
     download_and_extract_tar,
     get_block_devices,
     get_interfaces,
+    get_storage_model_from_udev,
     install_dependencies,
     output_and_send,
     output_and_send_scripts,
@@ -47,6 +49,7 @@ from snippets.maas_run_remote_scripts import (
     run_script,
     run_scripts,
     run_scripts_from_metadata,
+    udev_decode,
 )
 
 # Unused ScriptResult id, used to make sure number is always unique.
@@ -1581,19 +1584,25 @@ class TestParseParameters(MAASTestCase):
         maas_run_remote_scripts._block_devices = None
 
     def test_get_block_devices(self):
+        mock_get_storage_model_from_udev = self.patch(
+            maas_run_remote_scripts, "get_storage_model_from_udev"
+        )
+        mock_get_storage_model_from_udev.side_effect = lambda bd: bd
         expected_blockdevs = [
             {
                 "NAME": factory.make_name("NAME"),
                 "MODEL": factory.make_name("MODEL"),
                 "SERIAL": factory.make_name("SERIAL"),
+                "MAJ:MIN": factory.make_name("MAJ:MIN"),
             }
             for _ in range(3)
         ]
         mock_check_output = self.patch(maas_run_remote_scripts, "check_output")
         mock_check_output.return_value = "".join(
             [
-                'NAME="{NAME}" MODEL="{MODEL}" SERIAL="{SERIAL}"\n'.format(
-                    **blockdev
+                'NAME="{NAME}" MODEL="{MODEL}" SERIAL="{SERIAL}" '
+                'MAJ:MIN="{MAJ_MIN}"\n'.format(
+                    **blockdev, MAJ_MIN=blockdev["MAJ:MIN"]
                 )
                 for blockdev in expected_blockdevs
             ]
@@ -1601,6 +1610,7 @@ class TestParseParameters(MAASTestCase):
         maas_run_remote_scripts._block_devices = None
 
         self.assertItemsEqual(expected_blockdevs, get_block_devices())
+        self.assertTrue(mock_get_storage_model_from_udev.called)
 
     def test_get_block_devices_cached(self):
         block_devices = factory.make_name("block_devices")
@@ -1633,6 +1643,49 @@ class TestParseParameters(MAASTestCase):
 
         self.assertRaises(KeyError, get_block_devices)
 
+    def test_udev_decode(self):
+        self.assertEquals(
+            "QEMU HARDDISK", udev_decode("QEMU\x20HARDDISK\x20\x20\x20")
+        )
+
+    def test_get_storage_model_from_udev(self):
+        self.patch(
+            maas_run_remote_scripts.os.path, "exists"
+        ).return_value = True
+        self.patch(
+            maas_run_remote_scripts,
+            "open",
+            mock_open(
+                read_data=dedent(
+                    """\
+                    S:disk/by-id/scsi-SATA_QEMU_HARDDISK_QM00001
+                    E:ID_MODEL=QEMU_HARDDISK
+                    E:ID_MODEL_ENC=QEMU\x20HARDDISK\x20\x20\x20
+                    """
+                )
+            ),
+        )
+        self.assertDictEqual(
+            {
+                "MAJ:MIN": "8:0",
+                "MODEL": "QEMU_HARDDISK",
+                "MODEL_ENC": "QEMU HARDDISK",
+            },
+            get_storage_model_from_udev({"MAJ:MIN": "8:0"}),
+        )
+
+    def test_get_storage_model_from_udev_warns(self):
+        mock_stderr_write = self.patch(
+            maas_run_remote_scripts.sys.stderr, "write"
+        )
+        model = factory.make_name("model")
+        block_dev = {"MAJ:MIN": factory.make_name("MAJ:MIN"), "MODEL": model}
+        self.assertDictEqual(
+            {"MODEL_ENC": model, **block_dev},
+            get_storage_model_from_udev(block_dev),
+        )
+        self.assertThat(mock_stderr_write, MockCalledOnce())
+
     def test_get_interfaces(self):
         maas_run_remote_scripts._interfaces = None
         netplan_dir = self.useFixture(TempDirectory()).path
@@ -1728,6 +1781,10 @@ class TestParseParameters(MAASTestCase):
         self.assertThat(mock_listdir, MockNotCalled())
 
     def test_parse_parameters(self):
+        mock_get_storage_model_from_udev = self.patch(
+            maas_run_remote_scripts, "get_storage_model_from_udev"
+        )
+        mock_get_storage_model_from_udev.side_effect = lambda bd: bd
         scripts_dir = factory.make_name("scripts_dir")
         script = {
             "path": os.path.join("path_to", factory.make_name("script_name")),
@@ -1778,9 +1835,8 @@ class TestParseParameters(MAASTestCase):
         mock_check_output = self.patch(maas_run_remote_scripts, "check_output")
         mock_check_output.return_value = "".join(
             [
-                'NAME="{name}" MODEL="{model}" SERIAL="{serial}"\n'.format(
-                    **param["value"]
-                )
+                'NAME="{name}" MODEL="{model}" SERIAL="{serial}" '
+                'MAJ_MIN="8:0"\n'.format(**param["value"])
                 for param_name, param in script["parameters"].items()
                 if "storage" in param_name
             ]
@@ -1808,8 +1864,13 @@ class TestParseParameters(MAASTestCase):
             ],
             parse_parameters(script, scripts_dir),
         )
+        self.assertTrue(mock_get_storage_model_from_udev.called)
 
     def test_parse_parameters_argument_format(self):
+        mock_get_storage_model_from_udev = self.patch(
+            maas_run_remote_scripts, "get_storage_model_from_udev"
+        )
+        mock_get_storage_model_from_udev.side_effect = lambda bd: bd
         scripts_dir = factory.make_name("scripts_dir")
         script = {
             "path": os.path.join("path_to", factory.make_name("script_name")),
@@ -1853,9 +1914,8 @@ class TestParseParameters(MAASTestCase):
         mock_check_output = self.patch(maas_run_remote_scripts, "check_output")
         mock_check_output.return_value = "".join(
             [
-                'NAME="{name}" MODEL="{model}" SERIAL="{serial}"\n'.format(
-                    **param["value"]
-                )
+                'NAME="{name}" MODEL="{model}" SERIAL="{serial}" '
+                'MAJ:MIN="8:0"\n'.format(**param["value"])
                 for param_name, param in script["parameters"].items()
                 if "storage" in param_name
             ]
@@ -1889,6 +1949,7 @@ class TestParseParameters(MAASTestCase):
             ],
             parse_parameters(script, scripts_dir),
         )
+        self.assertTrue(mock_get_storage_model_from_udev.called)
 
     def test_parse_parameters_storage_value_all_raises_keyerror(self):
         scripts_dir = factory.make_name("scripts_dir")