Merge ~raharper/curtin:fix/dname-warning-missing-serial into curtin:master

[Ryan Harper]

dname: relax dname req for disk serial/wwn presence for compatibility

Allow disks without serial/wwn values to have dnames and do not raise
a RuntimeError exception which prevented legacy KVM pods from deploying
as their VM configuration lacked disk serial numbers. Per discussion
in the bug, prefer compatibility with these systems, allow dname to remain
unstable and provide a warning in the install log but do not fail
deployment. Update dname match attributes to: ID_WWN_WITH_EXTENSION,
ID_WWN, ID_SERIAL, ID_SERIAL_SHORT.

LP: #1735839

[Ryan Harper (raharper)]

Started vmtest run on this:

https://jenkins.ubuntu.com/server/job/curtin-vmtest-devel-debug/143/console

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2443c3613a2d1338b53fbbdcefeb4ac12994fa4d
https://jenkins.ubuntu.com/server/job/curtin-ci/1173/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1173
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1173
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1173
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1173

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/1173/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

+1 assuming we get a valid vmtest run on this.

review: Approve

[Ryan Harper (raharper)]

Vmtest ran, 2 tests failed, but were due to high load on jenkins:

Ran 4535 tests in 47305.518s

FAILED (SKIP=716, errors=2)
Fri, 08 Feb 2019 05:30:58 +0000: vmtest end [1] in 47310s

I re-ran those two tests (CosmicTestMultipathBasic, CosmicTestMirrorboot)
which passed.

Ran 22 tests in 311.488s

OK
Fri, 08 Feb 2019 15:14:56 +0000: vmtest end [0] in 317s

[Server Team CI bot (server-team-bot)]

Commit message lints:
 - Line #0 has 3 too many characters. Line starts with: "dname: relax dname requirement"...

review: Needs Fixing

[Server Team CI bot (server-team-bot)]

review: Approve (continuous-integration)

diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
index dce3924..150bf82 100644
--- a/curtin/commands/block_meta.py
+++ b/curtin/commands/block_meta.py
@@ -242,12 +242,14 @@ def make_dname_byid(path, error_msg=None, info=None):
             "Disk tag udev rules are only for disks, %s has devtype=%s" %
             (error_msg, devtype))
 
-    byid_keys = ['ID_SERIAL', 'ID_WWN_WITH_EXTENSION']
+    byid_keys = ['ID_WWN_WITH_EXTENSION', 'ID_WWN',
+                 'ID_SERIAL', 'ID_SERIAL_SHORT']
     present = [k for k in byid_keys if info.get(k)]
     if not present:
-        raise RuntimeError(
+        LOG.warning(
             "Cannot create disk tag udev rule for %s, "
             "missing 'serial' or 'wwn' value" % error_msg)
+        return []
 
     return [[compose_udev_equality('ENV{%s}' % k, info[k]) for k in present]]
 
diff --git a/tests/unittests/test_make_dname.py b/tests/unittests/test_make_dname.py
index 76c7b28..a088660 100644
--- a/tests/unittests/test_make_dname.py
+++ b/tests/unittests/test_make_dname.py
@@ -132,7 +132,7 @@ class TestMakeDname(CiTestCase):
         block_meta.make_dname('iscsi1', self.storage_config)
         mock_util.ensure_dir.assert_called_with(self.rules_d)
         self.assertTrue(mock_log.debug.called)
-        both_rules = (id_rule_identifiers + wwn_rule_identifiers)
+        both_rules = (wwn_rule_identifiers + id_rule_identifiers)
         mock_util.write_file.assert_called_with(
             self.rule_file.format(res_dname),
             self._content(
@@ -157,10 +157,9 @@ class TestMakeDname(CiTestCase):
 
         # disk with no PT_UUID
         disk = 'disk_noid'
-        with self.assertRaises(RuntimeError):
-            block_meta.make_dname(disk, self.storage_config)
-            mock_log.warning.assert_called_with(warning_msg.format(disk))
-            self.assertFalse(mock_util.write_file.called)
+        block_meta.make_dname(disk, self.storage_config)
+        mock_log.warning.assert_called_with(warning_msg.format(disk))
+        self.assertFalse(mock_util.write_file.called)
 
         mock_util.subp.side_effect = self._make_mock_subp_blkid(
             '', self.trusty_blkid)
@@ -308,8 +307,7 @@ class TestMakeDnameById(CiTestCase):
         """test dname_byid raises RuntimeError on device without ID or WWN."""
         mypath = "/dev/" + self.random_string()
         m_udev.return_value = {'DEVTYPE': 'disk'}
-        with self.assertRaises(RuntimeError):
-            block_meta.make_dname_byid(mypath)
+        self.assertEqual([], block_meta.make_dname_byid(mypath))
 
     @mock.patch('curtin.commands.block_meta.udevadm_info')
     def test_udevinfo_not_called_if_info_provided(self, m_udev):
@@ -355,7 +353,7 @@ class TestMakeDnameById(CiTestCase):
             block_meta.make_dname_byid(mypath, info=info))
 
     def test_disk_with_both_id_wwn(self):
-        """test dname_byid returns rules with both ID_SERIAL and ID_WWN"""
+        """test dname_byid returns rules with both ID_WWN_* and ID_SERIAL"""
         mypath = "/dev/" + self.random_string()
         myserial = self.random_string()
         mywwn = self.random_string()
@@ -363,9 +361,46 @@ class TestMakeDnameById(CiTestCase):
                 'ID_WWN_WITH_EXTENSION': mywwn,
                 'DEVNAME': mypath}
         self.assertEqual(
-            [['ENV{ID_SERIAL}=="%s"' % myserial,
-              'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn]],
+            [[
+                'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn,
+                'ENV{ID_SERIAL}=="%s"' % myserial,
+            ]],
+            block_meta.make_dname_byid(mypath, info=info))
+
+    def test_disk_with_short_ids(self):
+        """test dname_byid returns rules w/ both ID_WWN and ID_SERIAL_SHORT."""
+        mypath = "/dev/" + self.random_string()
+        myserial = self.random_string()
+        mywwn = self.random_string()
+        info = {'DEVTYPE': 'disk', 'ID_SERIAL_SHORT': myserial,
+                'ID_WWN': mywwn,
+                'DEVNAME': mypath}
+        self.assertEqual(
+            [[
+                'ENV{ID_WWN}=="%s"' % mywwn,
+                'ENV{ID_SERIAL_SHORT}=="%s"' % myserial,
+            ]],
             block_meta.make_dname_byid(mypath, info=info))
 
+    def test_disk_with_all_ids(self):
+        """test dname_byid returns rules w/ all WWN and SERIAL values."""
+        mypath = "/dev/" + self.random_string()
+        myserial_short = self.random_string()
+        myserial = myserial_short + "_" + myserial_short
+        mywwn = self.random_string()
+        mywwn_ext = mywwn + "_" + mywwn
+        info = {'DEVTYPE': 'disk', 'ID_SERIAL_SHORT': myserial_short,
+                'ID_SERIAL': myserial,
+                'ID_WWN': mywwn,
+                'ID_WWN_WITH_EXTENSION': mywwn_ext,
+                'DEVNAME': mypath}
+        self.assertEqual(
+            [[
+                'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn_ext,
+                'ENV{ID_WWN}=="%s"' % mywwn,
+                'ENV{ID_SERIAL}=="%s"' % myserial,
+                'ENV{ID_SERIAL_SHORT}=="%s"' % myserial_short,
+            ]],
+            block_meta.make_dname_byid(mypath, info=info))
 
 # vi: ts=4 expandtab syntax=python