curtin

Merge lp:~wesley-wiedenmeier/curtin/1562249 into lp:~curtin-dev/curtin/trunk

  1. 1562249
  2. Merge into trunk
Proposed by Wesley Wiedenmeier
Status: Merged
Merged at revision: 406
Proposed branch: lp:~wesley-wiedenmeier/curtin/1562249
Merge into: lp:~curtin-dev/curtin/trunk
Diff against target: 702 lines (+398/-94)
7 files modified
curtin/block/__init__.py (+63/-15)
curtin/commands/block_meta.py (+58/-76)
examples/tests/basic.yaml (+1/-1)
examples/tests/basic_scsi.yaml (+1/-1)
tests/unittests/test_block.py (+73/-0)
tests/unittests/test_make_dname.py (+200/-0)
tests/vmtests/test_basic.py (+2/-1)
To merge this branch: bzr merge lp:~wesley-wiedenmeier/curtin/1562249
Reviewer Review Type Date Requested Status
Server Team CI bot continuous-integration Approve
Ryan Harper (community) Needs Fixing
Review via email: mp+298482@code.launchpad.net

Description of the change

In block_meta, always use block.sys_block_path when finding information in sysfs, and in block.sys_block_path, properly find kname of device path given as /dev/cciss/cXdX

Note that this has not been tested yet as the hardware required is not supported by qemu

To post a comment you must log in.
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:403
https://server-team-jenkins.canonical.com/job/curtin-ci/285/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/282/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/285/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

  - cciss devices have a path at /dev/cciss!c0d0 as well as /dev/cciss/c0d0

Is this true for precise -> yakkety?

Otherwise, it looks good. Thanks for the unittest. Can you add a unittest for the
/dev/cciss/c0d0 case as well? What I mean is will get_blk return value
always be a cciss!c0d0 ? or would it ever return cciss/c0d0 ?

Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

I believe that the 'proper' address for cciss devices is in /dev/cciss/ but the /dev/cciss!c0d0 path may exist as a link created by udev rules, although I think there is a chance it may not exist on some systems. From the curtin config provided in the bug report though, it looks like MAAS is doing the right thing and giving curtin the /dev/cciss/c0d0 path instead of the /dev/cciss!c0d0 path, so it will work fine even if the /dev/cciss!c0d0 path does not exist.

The code as it stands will return the same /sys/class/block/ path whether given /dev/cciss/c0d0 or /dev/cciss!c0d0.

The unittest for cciss tests for both the /dev/cciss!c0d0 and /dev/cciss/c0d0 cases. However, some of the assumptions about device kname made elsewhere may be incorrect (block.dev_short() will not work right on /dev/cciss/c0d0) I'll go through and fix everywhere we get a disk kname

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:406
https://server-team-jenkins.canonical.com/job/curtin-ci/293/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/290/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/293/rebuild

review: Approve (continuous-integration)
Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

I fixed the issue with cciss partition knames and added a unittest. There is a new version of curtin building in my ppa right now, and I will have that copied over into trusty/xenial/yakkety soon. I think that this is the last place where the differences between cciss naming and regular naming will be an issue, so hopefully this version should install correctly.

Revision history for this message
Ryan Harper (raharper) wrote :

Let's see if this fixes the bug and if so, I'll pull and merge.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:407
https://server-team-jenkins.canonical.com/job/curtin-ci/294/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/291/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/294/rebuild

review: Approve (continuous-integration)
Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

I updated the cciss fix based on the cloud-init-output log here:
http://paste.ubuntu.com/18737231/

I believe I can write a vmtest for this by slightly modifying the environment curtin is running under, I am going to attempt that before merging, but I want to see if this passes vmtests first so I'm marking it ready for review again

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:414
https://server-team-jenkins.canonical.com/job/curtin-ci/297/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/294/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/297/rebuild

review: Approve (continuous-integration)
lp:~wesley-wiedenmeier/curtin/1562249 updated
415. By Wesley Wiedenmeier

For make_dname, use rule file name based on rule name not id

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:415
https://server-team-jenkins.canonical.com/job/curtin-ci/298/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/295/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/298/rebuild

review: Approve (continuous-integration)
lp:~wesley-wiedenmeier/curtin/1562249 updated
416. By Wesley Wiedenmeier

In block_meta.make_dname, sanitize dname input, replacing all special chars
with a '-', so that the system does not attempt to create a link in
/dev/disk/by-dname with a special character

417. By Wesley Wiedenmeier

Also allow underscore in dname

418. By Wesley Wiedenmeier

Add verification to vmtests that dnames with special chars will be sanitized

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:418
https://server-team-jenkins.canonical.com/job/curtin-ci/300/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/297/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/300/rebuild

review: Approve (continuous-integration)
lp:~wesley-wiedenmeier/curtin/1562249 updated
419. By Wesley Wiedenmeier

In block_meta.make_dname, use LOG.warning instead of depricated LOG.warn, and
raise a ValueError if instructed to run on a storage config entry with a type
not supported by make_dname

420. By Wesley Wiedenmeier

Added make_dname unittests

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:420
https://server-team-jenkins.canonical.com/job/curtin-ci/301/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/298/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/301/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

Looks mostly good. I think we should remove a lot of the background comments in those functions, there're useful in the merge but new readers of the code wouldn't know if dev_short was used before or not.

Some more comments inline

review: Needs Fixing
lp:~wesley-wiedenmeier/curtin/1562249 updated
421. By Wesley Wiedenmeier

cleaned up fixes to block and block_meta

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:421
https://server-team-jenkins.canonical.com/job/curtin-ci/304/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/304/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/304/rebuild

review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:421
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/9/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/305/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/9/rebuild

review: Approve (continuous-integration)
Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

Replied to some of the diff comments (forgot to save them earlier)

lp:~wesley-wiedenmeier/curtin/1562249 updated
422. By Wesley Wiedenmeier

Merge from trunk

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:422
https://server-team-jenkins.canonical.com/job/curtin-ci/323/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/342/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/323/rebuild

review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:422
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/28/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/343/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/28/rebuild

review: Approve (continuous-integration)
Revision history for this message
Christian Ehrhardt  (paelzer) wrote :

Hi,
a few inline comments from reading the MP for further review.

lp:~wesley-wiedenmeier/curtin/1562249 updated
423. By Wesley Wiedenmeier

Removed unneeded and unused call to os.path.normpath in
get_blockdev_for_partition()

424. By Wesley Wiedenmeier

Multipath devices with partitions on them that have paths in the form
/dev/mapper/mpathX have a 'p' before their partitions. Raw dm devices that have
partitions on them must also have a 'p' before the partition number or the
naming scheme would not work, so add both to block.partition_kname

425. By Wesley Wiedenmeier

Added test cases for mpath and dm devices in block.partition_kname

Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

Hi,

Thanks for reviewing, I updated the mp to include the fixes you suggested.

Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:425
https://server-team-jenkins.canonical.com/job/curtin-ci/331/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/358/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci/331/rebuild

review: Approve (continuous-integration)
Revision history for this message
Server Team CI bot (server-team-bot) wrote :

PASSED: Continuous integration, rev:425
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/36/
Executed test runs:
    None: https://server-team-jenkins.canonical.com/job/generic-update-mp/359/console

Click here to trigger a rebuild:
https://server-team-jenkins.canonical.com/job/curtin-ci-ppc64/36/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ryan Harper (raharper) wrote :

One failure to look at.

======================================================================
FAIL: test_dname (vmtests.test_basic.XenialTestScsiBasic)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/rharper/work/bzr/curtin/merge-trunk.fix-lp1562249/tests/vmtests/__init__.py", line 788, in test_dname
    self.assertIn(link, contents)
AssertionError: 'main_disk_with_in---valid--dname-part1' not found in ['main_disk', 'main_disk-part1', 'main_disk-part2', 'pnum_disk', 'pnum_disk-part1', 'pnum_disk-part10']

----------------------------------------------------------------------
Ran 653 tests in 10257.701s

FAILED (failures=1)

lp:~wesley-wiedenmeier/curtin/1562249 updated
426. By Wesley Wiedenmeier

Update name for main disk in basic_scsi.yaml to match that in basic.yaml so
that cls.disks_to_check is same between them

Revision history for this message
Wesley Wiedenmeier (wesley-wiedenmeier) wrote :

I just updated the scsi test file with the modified name for the main disk, so that should pass now

Revision history for this message
Ryan Harper (raharper) wrote :

Excellent! All else passed, so merging.

On Sat, Jul 30, 2016 at 5:57 PM, Wesley Wiedenmeier <
<email address hidden>> wrote:

> I just updated the scsi test file with the modified name for the main
> disk, so that should pass now
> --
> https://code.launchpad.net/~wesley-wiedenmeier/curtin/1562249/+merge/298482
> You are reviewing the proposed merge of
> lp:~wesley-wiedenmeier/curtin/1562249 into lp:curtin.
>

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'curtin/block/__init__.py'
2--- curtin/block/__init__.py 2016-07-11 19:06:24 +0000
3+++ curtin/block/__init__.py 2016-07-30 22:43:12 +0000
4@@ -59,14 +59,63 @@
5 return '/dev/' + devname
6
7
8+def path_to_kname(path):
9+ """
10+ converts a path in /dev or a path in /sys/block to the device kname,
11+ taking special devices and unusual naming schemes into account
12+ """
13+ # if path given is a link, get real path
14+ # only do this if given a path though, if kname is already specified then
15+ # this would cause a failure where the function should still be able to run
16+ if os.path.sep in path:
17+ path = os.path.realpath(path)
18+ # using basename here ensures that the function will work given a path in
19+ # /dev, a kname, or a path in /sys/block as an arg
20+ dev_kname = os.path.basename(path)
21+ # cciss devices need to have 'cciss!' prepended
22+ if path.startswith('/dev/cciss'):
23+ dev_kname = 'cciss!' + dev_kname
24+ LOG.debug("path_to_kname input: '{}' output: '{}'".format(path, dev_kname))
25+ return dev_kname
26+
27+
28+def kname_to_path(kname):
29+ """
30+ converts a kname to a path in /dev, taking special devices and unusual
31+ naming schemes into account
32+ """
33+ # if given something that is already a dev path, return it
34+ if os.path.exists(kname) and is_valid_device(kname):
35+ path = kname
36+ LOG.debug("kname_to_path input: '{}' output: '{}'".format(kname, path))
37+ return os.path.realpath(path)
38+ # adding '/dev' to path is not sufficient to handle cciss devices and
39+ # possibly other special devices which have not been encountered yet
40+ path = os.path.realpath(os.sep.join(['/dev'] + kname.split('!')))
41+ # make sure path we get is correct
42+ if not (os.path.exists(path) and is_valid_device(path)):
43+ raise OSError('could not get path to dev from kname: {}'.format(kname))
44+ LOG.debug("kname_to_path input: '{}' output: '{}'".format(kname, path))
45+ return path
46+
47+
48+def partition_kname(disk_kname, partition_number):
49+ """Add number to disk_kname prepending a 'p' if needed"""
50+ for dev_type in ['nvme', 'mmcblk', 'cciss', 'mpath', 'dm']:
51+ if disk_kname.startswith(dev_type):
52+ partition_number = "p%s" % partition_number
53+ break
54+ return "%s%s" % (disk_kname, partition_number)
55+
56+
57 def sys_block_path(devname, add=None, strict=True):
58 toks = ['/sys/class/block']
59 # insert parent dev if devname is partition
60 (parent, partnum) = get_blockdev_for_partition(devname)
61 if partnum:
62- toks.append(dev_short(parent))
63+ toks.append(path_to_kname(parent))
64
65- toks.append(dev_short(devname))
66+ toks.append(path_to_kname(devname))
67
68 if add is not None:
69 toks.append(add)
70@@ -172,21 +221,20 @@
71
72
73 def get_blockdev_for_partition(devpath):
74+ # normalize path
75+ rpath = os.path.realpath(devpath)
76+
77 # convert an entry in /dev/ to parent disk and partition number
78 # if devpath is a block device and not a partition, return (devpath, None)
79-
80- # input of /dev/vdb or /dev/disk/by-label/foo
81- # rpath is hopefully a real-ish path in /dev (vda, sdb..)
82- rpath = os.path.realpath(devpath)
83-
84- bname = os.path.basename(rpath)
85- syspath = "/sys/class/block/%s" % bname
86-
87+ base = '/sys/class/block'
88+
89+ # input of /dev/vdb, /dev/disk/by-label/foo, /sys/block/foo,
90+ # /sys/block/class/foo, or just foo
91+ syspath = os.path.join(base, path_to_kname(devpath))
92+
93+ # don't need to try out multiple sysfs paths as path_to_kname handles cciss
94 if not os.path.exists(syspath):
95- syspath2 = "/sys/class/block/cciss!%s" % bname
96- if not os.path.exists(syspath2):
97- raise ValueError("%s had no syspath (%s)" % (devpath, syspath))
98- syspath = syspath2
99+ raise ValueError("%s had no syspath (%s)" % (devpath, syspath))
100
101 ptpath = os.path.join(syspath, "partition")
102 if not os.path.exists(ptpath):
103@@ -594,7 +642,7 @@
104 offsets = [0, -zero_size]
105 is_block = is_block_device(path)
106 if not (is_block or os.path.isfile(path)):
107- raise ValueError("%s: not an existing file or block device")
108+ raise ValueError("%s: not an existing file or block device", path)
109
110 if partitions and is_block:
111 ptdata = sysfs_partition_data(path)
112
113=== modified file 'curtin/commands/block_meta.py'
114--- curtin/commands/block_meta.py 2016-07-27 15:35:28 +0000
115+++ curtin/commands/block_meta.py 2016-07-30 22:43:12 +0000
116@@ -28,7 +28,7 @@
117 import glob
118 import os
119 import platform
120-import re
121+import string
122 import sys
123 import tempfile
124 import time
125@@ -129,42 +129,10 @@
126 return "mbr"
127
128
129-def block_find_sysfs_path(devname):
130- # return the path in sys for device named devname
131- # support either short name ('sda') or full path /dev/sda
132- # sda -> /sys/class/block/sda
133- # sda1 -> /sys/class/block/sda/sda1
134- if not devname:
135- raise ValueError("empty devname provided to find_sysfs_path")
136-
137- sys_class_block = '/sys/class/block/'
138- basename = os.path.basename(devname)
139- # try without parent blockdevice, then prepend parent
140- paths = [
141- os.path.join(sys_class_block, basename),
142- os.path.join(sys_class_block,
143- re.split('[\d+]', basename)[0], basename),
144- ]
145-
146- # find path to devname directory in sysfs
147- devname_sysfs = None
148- for path in paths:
149- if os.path.exists(path):
150- devname_sysfs = path
151-
152- if devname_sysfs is None:
153- err = ('No sysfs path to device:'
154- ' {}'.format(devname_sysfs))
155- LOG.error(err)
156- raise ValueError(err)
157-
158- return devname_sysfs
159-
160-
161 def get_holders(devname):
162 # Look up any block device holders.
163 # Handle devices and partitions as devnames (vdb, md0, vdb7)
164- devname_sysfs = block_find_sysfs_path(devname)
165+ devname_sysfs = block.sys_block_path(devname)
166 if devname_sysfs:
167 holders = os.listdir(os.path.join(devname_sysfs, 'holders'))
168 LOG.debug("devname '%s' had holders: %s", devname, ','.join(holders))
169@@ -236,9 +204,8 @@
170
171 if os.path.exists(os.path.join(sys_block_path, "md")):
172 # md device
173- block_dev = os.path.join("/dev/", os.path.split(sys_block_path)[-1])
174- # if these fail its okay, the array might not be assembled and thats
175- # fine
176+ block_dev_kname = block.path_to_kname(sys_block_path)
177+ block_dev = block.kname_to_path(block_dev_kname)
178 mdadm.mdadm_stop(block_dev)
179 mdadm.mdadm_remove(block_dev)
180
181@@ -265,14 +232,6 @@
182 raise OSError('Failed to find device at path: %s', devpath)
183
184
185-def determine_partition_kname(disk_kname, partition_number):
186- for dev_type in ["nvme", "mmcblk"]:
187- if disk_kname.startswith(dev_type):
188- partition_number = "p%s" % partition_number
189- break
190- return "%s%s" % (disk_kname, partition_number)
191-
192-
193 def determine_partition_number(partition_id, storage_config):
194 vol = storage_config.get(partition_id)
195 partnumber = vol.get('number')
196@@ -304,6 +263,18 @@
197 return partnumber
198
199
200+def sanitize_dname(dname):
201+ """
202+ dnames should be sanitized before writing rule files, in case maas has
203+ emitted a dname with a special character
204+
205+ only letters, numbers and '-' and '_' are permitted, as this will be
206+ used for a device path. spaces are also not permitted
207+ """
208+ valid = string.digits + string.ascii_letters + '-_'
209+ return ''.join(c if c in valid else '-' for c in dname)
210+
211+
212 def make_dname(volume, storage_config):
213 state = util.load_command_environment()
214 rules_dir = os.path.join(state['scratch'], "rules.d")
215@@ -321,7 +292,7 @@
216 # we may not always be able to find a uniq identifier on devices with names
217 if not ptuuid and vol.get('type') in ["disk", "partition"]:
218 LOG.warning("Can't find a uuid for volume: {}. Skipping dname.".format(
219- dname))
220+ volume))
221 return
222
223 rule = [
224@@ -346,11 +317,24 @@
225 volgroup_name = storage_config.get(vol.get('volgroup')).get('name')
226 dname = "%s-%s" % (volgroup_name, dname)
227 rule.append(compose_udev_equality("ENV{DM_NAME}", dname))
228- rule.append("SYMLINK+=\"disk/by-dname/%s\"" % dname)
229+ else:
230+ raise ValueError('cannot make dname for device with type: {}'
231+ .format(vol.get('type')))
232+
233+ # note: this sanitization is done here instead of for all name attributes
234+ # at the beginning of storage configuration, as some devices, such as
235+ # lvm devices may use the name attribute and may permit special chars
236+ sanitized = sanitize_dname(dname)
237+ if sanitized != dname:
238+ LOG.warning(
239+ "dname modified to remove invalid chars. old: '{}' new: '{}'"
240+ .format(dname, sanitized))
241+
242+ rule.append("SYMLINK+=\"disk/by-dname/%s\"" % sanitized)
243 LOG.debug("Writing dname udev rule '{}'".format(str(rule)))
244 util.ensure_dir(rules_dir)
245- with open(os.path.join(rules_dir, volume), "w") as fp:
246- fp.write(', '.join(rule))
247+ rule_file = os.path.join(rules_dir, '{}.rules'.format(sanitized))
248+ util.write_file(rule_file, ', '.join(rule))
249
250
251 def get_path_to_storage_volume(volume, storage_config):
252@@ -368,9 +352,9 @@
253 partnumber = determine_partition_number(vol.get('id'), storage_config)
254 disk_block_path = get_path_to_storage_volume(vol.get('device'),
255 storage_config)
256- (base_path, disk_kname) = os.path.split(disk_block_path)
257- partition_kname = determine_partition_kname(disk_kname, partnumber)
258- volume_path = os.path.join(base_path, partition_kname)
259+ disk_kname = block.path_to_kname(disk_block_path)
260+ partition_kname = block.partition_kname(disk_kname, partnumber)
261+ volume_path = block.kname_to_path(partition_kname)
262 devsync_vol = os.path.join(disk_block_path)
263
264 elif vol.get('type') == "disk":
265@@ -419,13 +403,15 @@
266 # block devs are in the slaves dir there. Then, those blockdevs can be
267 # checked against the kname of the devs in the config for the desired
268 # bcache device. This is not very elegant though
269- backing_device_kname = os.path.split(get_path_to_storage_volume(
270- vol.get('backing_device'), storage_config))[-1]
271+ backing_device_path = get_path_to_storage_volume(
272+ vol.get('backing_device'), storage_config)
273+ backing_device_kname = block.path_to_kname(backing_device_path)
274 sys_path = list(filter(lambda x: backing_device_kname in x,
275 glob.glob("/sys/block/bcache*/slaves/*")))[0]
276 while "bcache" not in os.path.split(sys_path)[-1]:
277 sys_path = os.path.split(sys_path)[0]
278- volume_path = os.path.join("/dev", os.path.split(sys_path)[-1])
279+ bcache_kname = block.path_to_kname(sys_path)
280+ volume_path = block.kname_to_path(bcache_kname)
281 LOG.debug('got bcache volume path {}'.format(volume_path))
282
283 else:
284@@ -485,11 +471,11 @@
285 if info.get('wipe') and info.get('wipe') != "none":
286 # The disk has a lable, clear all partitions
287 mdadm.mdadm_assemble(scan=True)
288- disk_kname = os.path.split(disk)[-1]
289- syspath_partitions = list(
290+ disk_sysfs_path = block.sys_block_path(disk)
291+ sysfs_partitions = list(
292 os.path.split(prt)[0] for prt in
293- glob.glob("/sys/block/%s/*/partition" % disk_kname))
294- for partition in syspath_partitions:
295+ glob.glob(os.path.join(disk_sysfs_path, '*', 'partition')))
296+ for partition in sysfs_partitions:
297 clear_holders(partition)
298 with open(os.path.join(partition, "dev"), "r") as fp:
299 block_no = fp.read().rstrip()
300@@ -497,7 +483,7 @@
301 os.path.join("/dev/block", block_no))
302 block.wipe_volume(partition_path, mode=info.get('wipe'))
303
304- clear_holders("/sys/block/%s" % disk_kname)
305+ clear_holders(disk_sysfs_path)
306 block.wipe_volume(disk, mode=info.get('wipe'))
307
308 # Create partition table on disk
309@@ -552,13 +538,12 @@
310
311 disk = get_path_to_storage_volume(device, storage_config)
312 partnumber = determine_partition_number(info.get('id'), storage_config)
313-
314- disk_kname = os.path.split(
315- get_path_to_storage_volume(device, storage_config))[-1]
316+ disk_kname = block.path_to_kname(disk)
317+ disk_sysfs_path = block.sys_block_path(disk)
318 # consider the disks logical sector size when calculating sectors
319 try:
320- prefix = "/sys/block/%s/queue/" % disk_kname
321- with open(prefix + "logical_block_size", "r") as f:
322+ lbs_path = os.path.join(disk_sysfs_path, 'queue', 'logical_block_size')
323+ with open(lbs_path, 'r') as f:
324 l = f.readline()
325 logical_block_size_bytes = int(l)
326 except:
327@@ -576,17 +561,14 @@
328 extended_part_no = determine_partition_number(
329 key, storage_config)
330 break
331- partition_kname = determine_partition_kname(
332- disk_kname, extended_part_no)
333- previous_partition = "/sys/block/%s/%s/" % \
334- (disk_kname, partition_kname)
335+ pnum = extended_part_no
336 else:
337 pnum = find_previous_partition(device, info['id'], storage_config)
338- LOG.debug("previous partition number for '%s' found to be '%s'",
339- info.get('id'), pnum)
340- partition_kname = determine_partition_kname(disk_kname, pnum)
341- previous_partition = "/sys/block/%s/%s/" % \
342- (disk_kname, partition_kname)
343+
344+ LOG.debug("previous partition number for '%s' found to be '%s'",
345+ info.get('id'), pnum)
346+ partition_kname = block.partition_kname(disk_kname, pnum)
347+ previous_partition = os.path.join(disk_sysfs_path, partition_kname)
348 LOG.debug("previous partition: {}".format(previous_partition))
349 # XXX: sys/block/X/{size,start} is *ALWAYS* in 512b value
350 previous_size = util.load_file(os.path.join(previous_partition,
351@@ -1043,7 +1025,7 @@
352
353 if cache_device:
354 # /sys/class/block/XXX/YYY/
355- cache_device_sysfs = block_find_sysfs_path(cache_device)
356+ cache_device_sysfs = block.sys_block_path(cache_device)
357
358 if os.path.exists(os.path.join(cache_device_sysfs, "bcache")):
359 LOG.debug('caching device already exists at {}/bcache. Read '
360@@ -1068,7 +1050,7 @@
361 ensure_bcache_is_registered(cache_device, target_sysfs_path)
362
363 if backing_device:
364- backing_device_sysfs = block_find_sysfs_path(backing_device)
365+ backing_device_sysfs = block.sys_block_path(backing_device)
366 target_sysfs_path = os.path.join(backing_device_sysfs, "bcache")
367 if not os.path.exists(os.path.join(backing_device_sysfs, "bcache")):
368 util.subp(["make-bcache", "-B", backing_device])
369
370=== modified file 'examples/tests/basic.yaml'
371--- examples/tests/basic.yaml 2016-04-04 16:03:32 +0000
372+++ examples/tests/basic.yaml 2016-07-30 22:43:12 +0000
373@@ -7,7 +7,7 @@
374 ptable: msdos
375 model: QEMU HARDDISK
376 path: /dev/vdb
377- name: main_disk
378+ name: main_disk_with_in/\&valid@#dname
379 wipe: superblock
380 grub_device: true
381 - id: sda1
382
383=== modified file 'examples/tests/basic_scsi.yaml'
384--- examples/tests/basic_scsi.yaml 2016-06-09 18:11:53 +0000
385+++ examples/tests/basic_scsi.yaml 2016-07-30 22:43:12 +0000
386@@ -6,7 +6,7 @@
387 type: disk
388 ptable: msdos
389 wwn: '0x39cc071e72c64cc4'
390- name: main_disk
391+ name: main_disk_with_in/\&valid@#dname
392 wipe: superblock
393 grub_device: true
394 - id: sda1
395
396=== modified file 'tests/unittests/test_block.py'
397--- tests/unittests/test_block.py 2016-07-11 19:20:01 +0000
398+++ tests/unittests/test_block.py 2016-07-30 22:43:12 +0000
399@@ -162,6 +162,17 @@
400 self.assertEqual('/sys/class/block/foodev/md/b',
401 block.sys_block_path("foodev", "/md/b", strict=False))
402
403+ @mock.patch('curtin.block.get_blockdev_for_partition')
404+ @mock.patch('os.path.exists')
405+ def test_cciss_sysfs_path(self, m_os_path_exists, m_get_blk):
406+ m_os_path_exists.return_value = True
407+ m_get_blk.return_value = ('cciss!c0d0', None)
408+ self.assertEqual('/sys/class/block/cciss!c0d0',
409+ block.sys_block_path('/dev/cciss/c0d0'))
410+ m_get_blk.return_value = ('cciss!c0d0', 1)
411+ self.assertEqual('/sys/class/block/cciss!c0d0/cciss!c0d0p1',
412+ block.sys_block_path('/dev/cciss/c0d0p1'))
413+
414
415 class TestWipeFile(TestCase):
416 def __init__(self, *args, **kwargs):
417@@ -239,4 +250,66 @@
418 found = util.load_file(trgfile)
419 self.assertEqual(data, found)
420
421+
422+class TestBlockKnames(TestCase):
423+ """Tests for some of the kname functions in block"""
424+ def test_determine_partition_kname(self):
425+ part_knames = [(('sda', 1), 'sda1'),
426+ (('vda', 1), 'vda1'),
427+ (('nvme0n1', 1), 'nvme0n1p1'),
428+ (('mmcblk0', 1), 'mmcblk0p1'),
429+ (('cciss!c0d0', 1), 'cciss!c0d0p1'),
430+ (('dm-0', 1), 'dm-0p1'),
431+ (('mpath1', 2), 'mpath1p2')]
432+ for ((disk_kname, part_number), part_kname) in part_knames:
433+ self.assertEqual(block.partition_kname(disk_kname, part_number),
434+ part_kname)
435+
436+ @mock.patch('curtin.block.os.path.realpath')
437+ def test_path_to_kname(self, mock_os_realpath):
438+ mock_os_realpath.side_effect = lambda x: os.path.normpath(x)
439+ path_knames = [('/dev/sda', 'sda'),
440+ ('/dev/sda1', 'sda1'),
441+ ('/dev////dm-0/', 'dm-0'),
442+ ('vdb', 'vdb'),
443+ ('/dev/mmcblk0p1', 'mmcblk0p1'),
444+ ('/dev/nvme0n0p1', 'nvme0n0p1'),
445+ ('/sys/block/vdb', 'vdb'),
446+ ('/sys/block/vdb/vdb2/', 'vdb2'),
447+ ('/dev/cciss/c0d0', 'cciss!c0d0'),
448+ ('/dev/cciss/c0d0p1/', 'cciss!c0d0p1'),
449+ ('/sys/class/block/cciss!c0d0p1', 'cciss!c0d0p1'),
450+ ('nvme0n1p4', 'nvme0n1p4')]
451+ for (path, expected_kname) in path_knames:
452+ self.assertEqual(block.path_to_kname(path), expected_kname)
453+ if os.path.sep in path:
454+ mock_os_realpath.assert_called_with(path)
455+
456+ @mock.patch('curtin.block.os.path.exists')
457+ @mock.patch('curtin.block.os.path.realpath')
458+ @mock.patch('curtin.block.is_valid_device')
459+ def test_kname_to_path(self, mock_is_valid_device, mock_os_realpath,
460+ mock_exists):
461+ kname_paths = [('sda', '/dev/sda'),
462+ ('sda1', '/dev/sda1'),
463+ ('/dev/sda', '/dev/sda'),
464+ ('cciss!c0d0p1', '/dev/cciss/c0d0p1'),
465+ ('/dev/cciss/c0d0', '/dev/cciss/c0d0'),
466+ ('mmcblk0p1', '/dev/mmcblk0p1')]
467+
468+ mock_exists.return_value = True
469+ mock_os_realpath.side_effect = lambda x: x.replace('!', '/')
470+ # first call to is_valid_device needs to return false for nonpaths
471+ mock_is_valid_device.side_effect = lambda x: x.startswith('/dev')
472+ for (kname, expected_path) in kname_paths:
473+ self.assertEqual(block.kname_to_path(kname), expected_path)
474+ mock_is_valid_device.assert_called_with(expected_path)
475+
476+ # test failure
477+ mock_is_valid_device.return_value = False
478+ mock_is_valid_device.side_effect = None
479+ for (kname, expected_path) in kname_paths:
480+ with self.assertRaises(OSError):
481+ block.kname_to_path(kname)
482+
483 # vi: ts=4 expandtab syntax=python
484
485=== added file 'tests/unittests/test_make_dname.py'
486--- tests/unittests/test_make_dname.py 1970-01-01 00:00:00 +0000
487+++ tests/unittests/test_make_dname.py 2016-07-30 22:43:12 +0000
488@@ -0,0 +1,200 @@
489+from unittest import TestCase
490+import mock
491+
492+import textwrap
493+import uuid
494+
495+from curtin.commands import block_meta
496+
497+
498+class TestMakeDname(TestCase):
499+ state = {'scratch': '/tmp/null'}
500+ rules_d = '/tmp/null/rules.d'
501+ rule_file = '/tmp/null/rules.d/{}.rules'
502+ storage_config = {
503+ 'disk1': {'type': 'disk', 'id': 'disk1', 'name': 'main_disk'},
504+ 'disk1p1': {'type': 'partition', 'id': 'disk1p1', 'device': 'disk1'},
505+ 'disk2': {'type': 'disk', 'id': 'disk2',
506+ 'name': 'in_valid/name!@#$% &*(+disk'},
507+ 'disk2p1': {'type': 'partition', 'id': 'disk2p1', 'device': 'disk2'},
508+ 'md_id': {'type': 'raid', 'id': 'md_id', 'name': 'mdadm_name'},
509+ 'md_id2': {'type': 'raid', 'id': 'md_id2', 'name': 'mdadm/name'},
510+ 'lvol_id': {'type': 'lvm_volgroup', 'id': 'lvol_id', 'name': 'vg1'},
511+ 'lpart_id': {'type': 'lvm_partition', 'id': 'lpart_id',
512+ 'name': 'lpartition1', 'volgroup': 'lvol_id'},
513+ 'lpart2_id': {'type': 'lvm_partition', 'id': 'lpart2_id',
514+ 'name': 'lvm part/2', 'volgroup': 'lvol_id'},
515+ }
516+ disk_blkid = textwrap.dedent("""
517+ DEVNAME=/dev/sda
518+ PTUUID={}
519+ PTTYPE=dos""")
520+ part_blkid = textwrap.dedent("""
521+ DEVNAME=/dev/sda1
522+ UUID=f3e6efc2-d586-4b35-a681-dffb987c66fd
523+ TYPE=ext2
524+ PARTUUID={}""")
525+ trusty_blkid = ""
526+
527+ def _make_mock_subp_blkid(self, ident, blkid_out):
528+
529+ def subp_blkid(cmd, capture=False, rcs=None, retries=None):
530+ return (blkid_out.format(ident), None)
531+
532+ return subp_blkid
533+
534+ def _formatted_rule(self, identifiers, target):
535+ rule = ['SUBSYSTEM=="block"', 'ACTION=="add|change"']
536+ rule.extend(['ENV{%s}=="%s"' % ident for ident in identifiers])
537+ rule.append('SYMLINK+="disk/by-dname/{}"'.format(target))
538+ return ', '.join(rule)
539+
540+ @mock.patch('curtin.commands.block_meta.LOG')
541+ @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
542+ @mock.patch('curtin.commands.block_meta.util')
543+ def test_make_dname_disk(self, mock_util, mock_get_path, mock_log):
544+ disk_ptuuid = str(uuid.uuid1())
545+ mock_util.subp.side_effect = self._make_mock_subp_blkid(
546+ disk_ptuuid, self.disk_blkid)
547+ mock_util.load_command_environment.return_value = self.state
548+ rule_identifiers = [
549+ ('DEVTYPE', 'disk'),
550+ ('ID_PART_TABLE_UUID', disk_ptuuid)
551+ ]
552+
553+ # simple run
554+ res_dname = 'main_disk'
555+ block_meta.make_dname('disk1', self.storage_config)
556+ mock_util.ensure_dir.assert_called_with(self.rules_d)
557+ self.assertTrue(mock_log.debug.called)
558+ self.assertFalse(mock_log.warning.called)
559+ mock_util.write_file.assert_called_with(
560+ self.rule_file.format(res_dname),
561+ self._formatted_rule(rule_identifiers, res_dname))
562+
563+ # run invalid dname
564+ res_dname = 'in_valid-name----------disk'
565+ block_meta.make_dname('disk2', self.storage_config)
566+ self.assertTrue(mock_log.warning.called)
567+ mock_util.write_file.assert_called_with(
568+ self.rule_file.format(res_dname),
569+ self._formatted_rule(rule_identifiers, res_dname))
570+
571+ @mock.patch('curtin.commands.block_meta.LOG')
572+ @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
573+ @mock.patch('curtin.commands.block_meta.util')
574+ def test_make_dname_failures(self, mock_util, mock_get_path, mock_log):
575+ mock_util.subp.side_effect = self._make_mock_subp_blkid(
576+ '', self.trusty_blkid)
577+ mock_util.load_command_environment.return_value = self.state
578+
579+ warning_msg = "Can't find a uuid for volume: {}. Skipping dname."
580+
581+ # disk with no PT_UUID
582+ block_meta.make_dname('disk1', self.storage_config)
583+ mock_log.warning.assert_called_with(warning_msg.format('disk1'))
584+ self.assertFalse(mock_util.write_file.called)
585+
586+ # partition with no PART_UUID
587+ block_meta.make_dname('disk1p1', self.storage_config)
588+ mock_log.warning.assert_called_with(warning_msg.format('disk1p1'))
589+ self.assertFalse(mock_util.write_file.called)
590+
591+ @mock.patch('curtin.commands.block_meta.LOG')
592+ @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
593+ @mock.patch('curtin.commands.block_meta.util')
594+ def test_make_dname_partition(self, mock_util, mock_get_path, mock_log):
595+ part_uuid = str(uuid.uuid1())
596+ mock_util.subp.side_effect = self._make_mock_subp_blkid(
597+ part_uuid, self.part_blkid)
598+ mock_util.load_command_environment.return_value = self.state
599+
600+ rule_identifiers = [
601+ ('DEVTYPE', 'partition'),
602+ ('ID_PART_ENTRY_UUID', part_uuid),
603+ ]
604+
605+ # simple run
606+ res_dname = 'main_disk-part1'
607+ block_meta.make_dname('disk1p1', self.storage_config)
608+ mock_util.ensure_dir.assert_called_with(self.rules_d)
609+ self.assertTrue(mock_log.debug.called)
610+ self.assertFalse(mock_log.warning.called)
611+ mock_util.write_file.assert_called_with(
612+ self.rule_file.format(res_dname),
613+ self._formatted_rule(rule_identifiers, res_dname))
614+
615+ # run invalid dname
616+ res_dname = 'in_valid-name----------disk-part1'
617+ block_meta.make_dname('disk2p1', self.storage_config)
618+ self.assertTrue(mock_log.warning.called)
619+ mock_util.write_file.assert_called_with(
620+ self.rule_file.format(res_dname),
621+ self._formatted_rule(rule_identifiers, res_dname))
622+
623+ @mock.patch('curtin.commands.block_meta.mdadm')
624+ @mock.patch('curtin.commands.block_meta.LOG')
625+ @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
626+ @mock.patch('curtin.commands.block_meta.util')
627+ def test_make_dname_raid(self, mock_util, mock_get_path, mock_log,
628+ mock_mdadm):
629+ md_uuid = str(uuid.uuid1())
630+ mock_mdadm.mdadm_query_detail.return_value = {'MD_UUID': md_uuid}
631+ mock_util.load_command_environment.return_value = self.state
632+ rule_identifiers = [('MD_UUID', md_uuid)]
633+
634+ # simple
635+ res_dname = 'mdadm_name'
636+ block_meta.make_dname('md_id', self.storage_config)
637+ self.assertTrue(mock_log.debug.called)
638+ self.assertFalse(mock_log.warning.called)
639+ mock_util.write_file.assert_called_with(
640+ self.rule_file.format(res_dname),
641+ self._formatted_rule(rule_identifiers, res_dname))
642+
643+ # invalid name
644+ res_dname = 'mdadm-name'
645+ block_meta.make_dname('md_id2', self.storage_config)
646+ self.assertTrue(mock_log.warning.called)
647+ mock_util.write_file.assert_called_with(
648+ self.rule_file.format(res_dname),
649+ self._formatted_rule(rule_identifiers, res_dname))
650+
651+ @mock.patch('curtin.commands.block_meta.LOG')
652+ @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
653+ @mock.patch('curtin.commands.block_meta.util')
654+ def test_make_dname_lvm_partition(self, mock_util, mock_get_path,
655+ mock_log):
656+ mock_util.load_command_environment.return_value = self.state
657+
658+ # simple
659+ res_dname = 'vg1-lpartition1'
660+ rule_identifiers = [('DM_NAME', res_dname)]
661+ block_meta.make_dname('lpart_id', self.storage_config)
662+ self.assertTrue(mock_log.debug.called)
663+ self.assertFalse(mock_log.warning.called)
664+ mock_util.write_file.assert_called_with(
665+ self.rule_file.format(res_dname),
666+ self._formatted_rule(rule_identifiers, res_dname))
667+
668+ # with invalid name
669+ res_dname = 'vg1-lvm-part-2'
670+ rule_identifiers = [('DM_NAME', 'vg1-lvm part/2')]
671+ block_meta.make_dname('lpart2_id', self.storage_config)
672+ self.assertTrue(mock_log.warning.called)
673+ mock_util.write_file.assert_called_with(
674+ self.rule_file.format(res_dname),
675+ self._formatted_rule(rule_identifiers, res_dname))
676+
677+ def test_sanitize_dname(self):
678+ unsanitized_to_sanitized = [
679+ ('main_disk', 'main_disk'),
680+ ('main-disk', 'main-disk'),
681+ ('main/disk', 'main-disk'),
682+ ('main disk', 'main-disk'),
683+ ('m.a/i*n# d~i+sk', 'm-a-i-n---d-i-sk'),
684+ ]
685+ for (unsanitized, sanitized) in unsanitized_to_sanitized:
686+ self.assertEqual(block_meta.sanitize_dname(unsanitized), sanitized)
687+
688+# vi: ts=4 expandtab syntax=python
689
690=== modified file 'tests/vmtests/test_basic.py'
691--- tests/vmtests/test_basic.py 2016-06-23 14:26:39 +0000
692+++ tests/vmtests/test_basic.py 2016-07-30 22:43:12 +0000
693@@ -13,7 +13,8 @@
694 conf_file = "examples/tests/basic.yaml"
695 extra_disks = ['128G', '128G', '4G']
696 nvme_disks = ['4G']
697- disk_to_check = [('main_disk', 1), ('main_disk', 2)]
698+ disk_to_check = [('main_disk_with_in---valid--dname', 1),
699+ ('main_disk_with_in---valid--dname', 2)]
700 collect_scripts = [textwrap.dedent("""
701 cd OUTPUT_COLLECT_D
702 blkid -o export /dev/vda > blkid_output_vda

Subscribers

People subscribed via source and target branches