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

Proposed by Ryan Harper on 2019-02-05
Status: Merged
Approved by: Ryan Harper on 2019-02-08
Approved revision: 2443c3613a2d1338b53fbbdcefeb4ac12994fa4d
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/curtin:fix/dname-warning-missing-serial
Merge into: curtin:master
Diff against target: 116 lines (+49/-12)
2 files modified
curtin/commands/block_meta.py (+4/-2)
tests/unittests/test_make_dname.py (+45/-10)
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve on 2019-02-08
Chad Smith 2019-02-05 Approve on 2019-02-07
Review via email: mp+362752@code.launchpad.net

Commit message

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

To post a comment you must log in.
Chad Smith (chad.smith) wrote :

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

review: Approve
Ryan Harper (raharper) wrote :

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

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

review: Needs Fixing
review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/curtin/commands/block_meta.py b/curtin/commands/block_meta.py
2index dce3924..150bf82 100644
3--- a/curtin/commands/block_meta.py
4+++ b/curtin/commands/block_meta.py
5@@ -242,12 +242,14 @@ def make_dname_byid(path, error_msg=None, info=None):
6 "Disk tag udev rules are only for disks, %s has devtype=%s" %
7 (error_msg, devtype))
8
9- byid_keys = ['ID_SERIAL', 'ID_WWN_WITH_EXTENSION']
10+ byid_keys = ['ID_WWN_WITH_EXTENSION', 'ID_WWN',
11+ 'ID_SERIAL', 'ID_SERIAL_SHORT']
12 present = [k for k in byid_keys if info.get(k)]
13 if not present:
14- raise RuntimeError(
15+ LOG.warning(
16 "Cannot create disk tag udev rule for %s, "
17 "missing 'serial' or 'wwn' value" % error_msg)
18+ return []
19
20 return [[compose_udev_equality('ENV{%s}' % k, info[k]) for k in present]]
21
22diff --git a/tests/unittests/test_make_dname.py b/tests/unittests/test_make_dname.py
23index 76c7b28..a088660 100644
24--- a/tests/unittests/test_make_dname.py
25+++ b/tests/unittests/test_make_dname.py
26@@ -132,7 +132,7 @@ class TestMakeDname(CiTestCase):
27 block_meta.make_dname('iscsi1', self.storage_config)
28 mock_util.ensure_dir.assert_called_with(self.rules_d)
29 self.assertTrue(mock_log.debug.called)
30- both_rules = (id_rule_identifiers + wwn_rule_identifiers)
31+ both_rules = (wwn_rule_identifiers + id_rule_identifiers)
32 mock_util.write_file.assert_called_with(
33 self.rule_file.format(res_dname),
34 self._content(
35@@ -157,10 +157,9 @@ class TestMakeDname(CiTestCase):
36
37 # disk with no PT_UUID
38 disk = 'disk_noid'
39- with self.assertRaises(RuntimeError):
40- block_meta.make_dname(disk, self.storage_config)
41- mock_log.warning.assert_called_with(warning_msg.format(disk))
42- self.assertFalse(mock_util.write_file.called)
43+ block_meta.make_dname(disk, self.storage_config)
44+ mock_log.warning.assert_called_with(warning_msg.format(disk))
45+ self.assertFalse(mock_util.write_file.called)
46
47 mock_util.subp.side_effect = self._make_mock_subp_blkid(
48 '', self.trusty_blkid)
49@@ -308,8 +307,7 @@ class TestMakeDnameById(CiTestCase):
50 """test dname_byid raises RuntimeError on device without ID or WWN."""
51 mypath = "/dev/" + self.random_string()
52 m_udev.return_value = {'DEVTYPE': 'disk'}
53- with self.assertRaises(RuntimeError):
54- block_meta.make_dname_byid(mypath)
55+ self.assertEqual([], block_meta.make_dname_byid(mypath))
56
57 @mock.patch('curtin.commands.block_meta.udevadm_info')
58 def test_udevinfo_not_called_if_info_provided(self, m_udev):
59@@ -355,7 +353,7 @@ class TestMakeDnameById(CiTestCase):
60 block_meta.make_dname_byid(mypath, info=info))
61
62 def test_disk_with_both_id_wwn(self):
63- """test dname_byid returns rules with both ID_SERIAL and ID_WWN"""
64+ """test dname_byid returns rules with both ID_WWN_* and ID_SERIAL"""
65 mypath = "/dev/" + self.random_string()
66 myserial = self.random_string()
67 mywwn = self.random_string()
68@@ -363,9 +361,46 @@ class TestMakeDnameById(CiTestCase):
69 'ID_WWN_WITH_EXTENSION': mywwn,
70 'DEVNAME': mypath}
71 self.assertEqual(
72- [['ENV{ID_SERIAL}=="%s"' % myserial,
73- 'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn]],
74+ [[
75+ 'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn,
76+ 'ENV{ID_SERIAL}=="%s"' % myserial,
77+ ]],
78+ block_meta.make_dname_byid(mypath, info=info))
79+
80+ def test_disk_with_short_ids(self):
81+ """test dname_byid returns rules w/ both ID_WWN and ID_SERIAL_SHORT."""
82+ mypath = "/dev/" + self.random_string()
83+ myserial = self.random_string()
84+ mywwn = self.random_string()
85+ info = {'DEVTYPE': 'disk', 'ID_SERIAL_SHORT': myserial,
86+ 'ID_WWN': mywwn,
87+ 'DEVNAME': mypath}
88+ self.assertEqual(
89+ [[
90+ 'ENV{ID_WWN}=="%s"' % mywwn,
91+ 'ENV{ID_SERIAL_SHORT}=="%s"' % myserial,
92+ ]],
93 block_meta.make_dname_byid(mypath, info=info))
94
95+ def test_disk_with_all_ids(self):
96+ """test dname_byid returns rules w/ all WWN and SERIAL values."""
97+ mypath = "/dev/" + self.random_string()
98+ myserial_short = self.random_string()
99+ myserial = myserial_short + "_" + myserial_short
100+ mywwn = self.random_string()
101+ mywwn_ext = mywwn + "_" + mywwn
102+ info = {'DEVTYPE': 'disk', 'ID_SERIAL_SHORT': myserial_short,
103+ 'ID_SERIAL': myserial,
104+ 'ID_WWN': mywwn,
105+ 'ID_WWN_WITH_EXTENSION': mywwn_ext,
106+ 'DEVNAME': mypath}
107+ self.assertEqual(
108+ [[
109+ 'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn_ext,
110+ 'ENV{ID_WWN}=="%s"' % mywwn,
111+ 'ENV{ID_SERIAL}=="%s"' % myserial,
112+ 'ENV{ID_SERIAL_SHORT}=="%s"' % myserial_short,
113+ ]],
114+ block_meta.make_dname_byid(mypath, info=info))
115
116 # vi: ts=4 expandtab syntax=python

Subscribers

People subscribed via source and target branches