curtin

Merge ~raharper/curtin:fix/dname_to_serial_wwn_if_available into curtin:master

  1. Git
  2. lp:~raharper/curtin
  3. fix/dname_to_serial_wwn_if_available
  4. Merge into master
Proposed by Ryan Harper
Status: Merged
Approved by: Ryan Harper
Approved revision: 7047f4ec13eda8a3adbcf1f5488cb6180a06171b
Merge reported by: Server Team CI bot
Merged at revision: not available
Proposed branch: ~raharper/curtin:fix/dname_to_serial_wwn_if_available
Merge into: curtin:master
Diff against target: 974 lines (+484/-63)
13 files modified
curtin/commands/block_meta.py (+74/-15)
curtin/udev.py (+37/-0)
doc/topics/storage.rst (+37/-2)
examples/tests/basic.yaml (+2/-0)
examples/tests/basic_scsi.yaml (+15/-0)
examples/tests/nvme.yaml (+2/-2)
examples/tests/nvme_bcache.yaml (+1/-1)
tests/unittests/helpers.py (+7/-0)
tests/unittests/test_make_dname.py (+172/-30)
tests/unittests/test_udev.py (+68/-0)
tests/vmtests/__init__.py (+55/-5)
tests/vmtests/test_basic.py (+8/-4)
tests/vmtests/test_nvme.py (+6/-4)
Reviewer Review Type Date Requested Status
Scott Moser (community) Approve
Server Team CI bot continuous-integration Approve
Chad Smith Pending
Review via email: mp+358116@code.launchpad.net

Commit message

dname: persistent names based on serial or wwn

Currently /dev/disk/by-dname symlinks are not created
if a device does not have a partition table or a super block.
For devices which do have a serial or wwn number, curtin will
append an additional rule to the disk rules file which includes
both the partition table/superblock UUID as well as rule matching
the device provided serial or wwn. This ensures that even if
a block device is wiped that the dname will continue to function.

Other changes:
- add method to discover udev database value of a block device
  ID_SERIAL value.
- refactor nvme vmtests to use 'serial' values
- don't hardcode nvme serials based on their enumerated index
- ensure we pass nvme serials to both install and boot phases
- update disks_to_check for basic and nvme to ensure we have a
  dname for the disk itself

LP: #1735839

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:43f42654b5f488e450f72e77473e801264772ee9
https://jenkins.ubuntu.com/server/job/curtin-ci/1103/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/1103
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/1103
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/1103
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=torkoal/1103

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

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

vmtest here:

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

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
7132714... by Ryan Harper

vmtest: fix PsuedoVMBaseClass, add test_dname_rules test

18175ca... by Ryan Harper

dname_byid: handle iscsi devices without serial, wwn value by lookup

In our vmtest, at least, the iscsi devices do not have a 'serial' or
'wwn' config attribute. This prevents dname rules from including a
rule that references a disk attribute. Allow the byid rule generator
to query udev info on that block device to determine if it has a
serial or wwn value to use.

0e4470d... by Ryan Harper

dname: handle disks without serial, wwn config by udevinfo

Restructure dname byid to look up the serial/wwn values if
present and use them, allowing for both values to be present.
This is needed for the vmtest iscsi cases where we do not set
a serial or wwn value in the config, but the devices to have
a value.

ada685e... by Ryan Harper

Drop disk_serial_to_udev_info_value method, use info directly

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

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

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

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

vmtest results showed issues with iscsi tests, so refactored how we get the serial/wwn values so they work with iscsi.

Restarted the vmtest run

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

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
c64b480... by Ryan Harper

Drop debug

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

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

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

review: Approve (continuous-integration)
~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
6b6931a... by Ryan Harper

vmtest: handle serial number with space matching when checking dname rules

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

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

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

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

forgot to fix multipath, fixed issue with spaces in serial values.

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

Revision history for this message
Scott Moser (smoser) wrote :

Commit message:
> Currently /dev/disk/by-dname symlinks are not created
> if a device does not have a partition table or a super block.

More clear as:

  Currently /dev/disk/by-dname symlinks are only created if
  a device has a partition table or a filesystem.

Revision history for this message
Scott Moser (smoser) wrote :

more review tomororw

Revision history for this message
Scott Moser (smoser) wrote :

one comment in line.

I had once envisioned thath we might just write a shell or other program
that interpreted config and dump that into the installed system.

either way, we're dumping files into the installed system that wont ever
be serviced, so its not a terrible suggestion.

my idea would have been to have more "config" like format in /etc/curtin/disks
and a udev rule that fired that read that data.

data format might look like this:
   name=mydisk1 serial=foo-serial

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
c3b5cf9... by Ryan Harper

block-meta: generate automatic -part%n partition rules for disks with dname

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

> one comment in line.
>
> I had once envisioned thath we might just write a shell or other program
> that interpreted config and dump that into the installed system.
>
> either way, we're dumping files into the installed system that wont ever
> be serviced, so its not a terrible suggestion.
>
> my idea would have been to have more "config" like format in /etc/curtin/disks
> and a udev rule that fired that read that data.
>
> data format might look like this:
> name=mydisk1 serial=foo-serial

I mentioned this idea at the product roadmap sprint and the discussion ended with the
understanding that the current unmanaged rules was the most ideal without a lot more work
centered around locally managing the configuration file and with what tool/cli would we do that.

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
d79733c... by Ryan Harper

dname_byid: raise exception error, simplify rule construction

Revision history for this message
Ryan Harper (raharper) :
Revision history for this message
Chad Smith (chad.smith) wrote :

Some nits and questions. thanks for this.

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

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

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

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

Thanks for review, fixing up most suggestions.

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
f91292a... by Ryan Harper

Add docstrings and small refactor to address feedback.

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

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

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

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

I have some questions inline.

One generall question though...
What happens if maas provides a dname for a device that does not have a wwn or id?
Will install fail?

That can be considered "correct" behavior, but I dont know if maas has been
sending dname (or allows the user to do so) in other cases. I'm not sure
that we can start failing an install when previously we just did
the wrong thing and dname didnt work.

Last, we'd like to add some doc on dname. Cove
 - behavior when a name is given to a partition
 - behavior when a name is given to a md or vg

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

> I have some questions inline.
>
> One generall question though...
> What happens if maas provides a dname for a device that does not have a wwn or
> id?
> Will install fail?

It already fails due to curtin not finding the disk specified by the 'serial' or 'wwn' attribute.
While maas *could* send path only; I don't think they do for real machines, and for kvm pods, they are using serial attributes as well.

worth having a sync with MAAS and ask what expectations they'd have and how they handle disks detected wtih no serial/wwn.

>
> That can be considered "correct" behavior, but I dont know if maas has been
> sending dname (or allows the user to do so) in other cases. I'm not sure
> that we can start failing an install when previously we just did
> the wrong thing and dname didnt work.

Agreed. I generally don't think dname is a fatal thing, but let's sync with MAAS on this.

>
>
> Last, we'd like to add some doc on dname. Cove
> - behavior when a name is given to a partition
> - behavior when a name is given to a md or vg

ACK, will add.

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

Thanks for the review, I'll update with docs and unittests.

Revision history for this message
Scott Moser (smoser) :
~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
e6b37d0... by Ryan Harper

block-meta: make_dname_byid refactor for easier unittesting, add unittests

9d1446e... by Ryan Harper

udev: refactor udevadm_info to use just --query=property and add unittests

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

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

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

review: Approve (continuous-integration)
~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
b6b9009... by Ryan Harper

Update docstrings for udevadm_info

Revision history for this message
Scott Moser (smoser) wrote :

some little things.

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
f129c88... by Ryan Harper

docs: update documentation around dname for disks and partitions

3189f59... by Ryan Harper

block_meta: update dname debug message to indicate rule creation, not writing

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

Thanks, fixing up

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
e0c3384... by Ryan Harper

unittest: make_dname, add test for passing info dict, use assertEqual

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

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

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

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) wrote :

At this point, I think this is probably fine.
I put together what I think are some test case cleanups at:
  http://paste.ubuntu.com/p/XkJ2KMW5x7/

I'd like you to pull them, but wont insist on it.

Also, I think we have a bug with multipath disks.
As it is, I'm pretty sure that ifyou put a name on a disk that was
a multipath disk, you'll get a symlink from /dev/disk/by-name/my-name -> /dev/diskX
when what we want would be /dev/mapper/mpathX

For example, on some of our power systems, /dev/sda and /dev/sdg are
multiple paths to the same disk. If we put a name on one of those, I think
we'd get either
  /dev/disk/by-name -> /dev/sda or /dev/disk/by-name -> /dev/sdg
but what we want is
  /dev/disk/by-name -> /dev/mapper/mpath0

I'm fine to leave that as it is for now, but I think ultimately we
will want to have some fix for that.

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

On Wed, Nov 28, 2018 at 11:20 AM Scott Moser <email address hidden> wrote:
>
> At this point, I think this is probably fine.
> I put together what I think are some test case cleanups at:
> http://paste.ubuntu.com/p/XkJ2KMW5x7/
>
> I'd like you to pull them, but wont insist on it.

I'll pull

>
> Also, I think we have a bug with multipath disks.
> As it is, I'm pretty sure that ifyou put a name on a disk that was
> a multipath disk, you'll get a symlink from /dev/disk/by-name/my-name -> /dev/diskX
> when what we want would be /dev/mapper/mpathX
>
> For example, on some of our power systems, /dev/sda and /dev/sdg are
> multiple paths to the same disk. If we put a name on one of those, I think
> we'd get either
> /dev/disk/by-name -> /dev/sda or /dev/disk/by-name -> /dev/sdg
> but what we want is
> /dev/disk/by-name -> /dev/mapper/mpath0
>
> I'm fine to leave that as it is for now, but I think ultimately we
> will want to have some fix for that.

https://bugs.launchpad.net/ubuntu/+source/curtin/+bug/1805673

>
>
> --
> https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/358116
> You are the owner of ~raharper/curtin:fix/dname_to_serial_wwn_if_available.

~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
68571d0... by Scott Moser

unittest: dname unittest cleanup

 - move test_udevinfo_not_called_if_info_provided
 - add test_udevinfo_called_if_info_not_provided
   if info is provided, udevinfo should not be called.
 - add '/dev/' prefix to "mypath" variables to make them more expected.
 - split test_disk_with_id_or_wwn into 2 tests
     test_disk_with_only_id
     test_disk_with_only_wwn
   also here use self.assertEqual as it makes it more clear
   what the expected result is.
 - make test_disk_with_both_id_wwn call with info=

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

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

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

review: Approve (continuous-integration)
~raharper/curtin:fix/dname_to_serial_wwn_if_available updated
7047f4e... by Ryan Harper

unittest: add test_udev tests

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

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

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

review: Approve (continuous-integration)
Revision history for this message
Scott Moser (smoser) :
review: Approve

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 ef0a48a..8decab5 100644
3--- a/curtin/commands/block_meta.py
4+++ b/curtin/commands/block_meta.py
5@@ -8,7 +8,8 @@ from curtin.log import LOG, logged_time
6 from curtin.reporter import events
7
8 from . import populate_one_subcmd
9-from curtin.udev import compose_udev_equality, udevadm_settle, udevadm_trigger
10+from curtin.udev import (compose_udev_equality, udevadm_settle,
11+ udevadm_trigger, udevadm_info)
12
13 import glob
14 import os
15@@ -221,12 +222,43 @@ def sanitize_dname(dname):
16 return ''.join(c if c in valid else '-' for c in dname)
17
18
19+def make_dname_byid(path, error_msg=None, info=None):
20+ """ Returns a list of udev equalities for a given disk path
21+
22+ :param path: string of a kernel device path to a block device
23+ :param error_msg: more information about path for log/errors
24+ :param info: dict of udevadm info key, value pairs of device specified by
25+ path.
26+ :returns: list of udev equalities (lists)
27+ :raises: ValueError if path is not a disk.
28+ :raises: RuntimeError if there is no serial or wwn.
29+ """
30+ error_msg = str(path) + ("" if not error_msg else " [%s]" % error_msg)
31+ if info is None:
32+ info = udevadm_info(path=path)
33+ devtype = info.get('DEVTYPE')
34+ if devtype != "disk":
35+ raise ValueError(
36+ "Disk tag udev rules are only for disks, %s has devtype=%s" %
37+ (error_msg, devtype))
38+
39+ byid_keys = ['ID_SERIAL', 'ID_WWN_WITH_EXTENSION']
40+ present = [k for k in byid_keys if info.get(k)]
41+ if not present:
42+ raise RuntimeError(
43+ "Cannot create disk tag udev rule for %s, "
44+ "missing 'serial' or 'wwn' value" % error_msg)
45+
46+ return [[compose_udev_equality('ENV{%s}' % k, info[k]) for k in present]]
47+
48+
49 def make_dname(volume, storage_config):
50 state = util.load_command_environment()
51 rules_dir = os.path.join(state['scratch'], "rules.d")
52 vol = storage_config.get(volume)
53 path = get_path_to_storage_volume(volume, storage_config)
54 ptuuid = None
55+ byid = None
56 dname = vol.get('name')
57 if vol.get('type') in ["partition", "disk"]:
58 (out, _err) = util.subp(["blkid", "-o", "export", path], capture=True,
59@@ -235,28 +267,41 @@ def make_dname(volume, storage_config):
60 if "PTUUID" in line or "PARTUUID" in line:
61 ptuuid = line.split('=')[-1]
62 break
63+ if vol.get('type') == 'disk':
64+ byid = make_dname_byid(path, error_msg="id=%s" % vol.get('id'))
65 # we may not always be able to find a uniq identifier on devices with names
66- if not ptuuid and vol.get('type') in ["disk", "partition"]:
67+ if (not ptuuid and not byid) and vol.get('type') in ["disk", "partition"]:
68 LOG.warning("Can't find a uuid for volume: {}. Skipping dname.".format(
69 volume))
70 return
71
72- rule = [
73+ matches = []
74+ base_rule = [
75 compose_udev_equality("SUBSYSTEM", "block"),
76 compose_udev_equality("ACTION", "add|change"),
77 ]
78 if vol.get('type') == "disk":
79- rule.append(compose_udev_equality('ENV{DEVTYPE}', "disk"))
80- rule.append(compose_udev_equality('ENV{ID_PART_TABLE_UUID}', ptuuid))
81+ if ptuuid:
82+ matches += [[compose_udev_equality('ENV{DEVTYPE}', "disk"),
83+ compose_udev_equality('ENV{ID_PART_TABLE_UUID}',
84+ ptuuid)]]
85+ for rule in byid:
86+ matches += [
87+ [compose_udev_equality('ENV{DEVTYPE}', "disk")] + rule]
88 elif vol.get('type') == "partition":
89- rule.append(compose_udev_equality('ENV{DEVTYPE}', "partition"))
90- dname = storage_config.get(vol.get('device')).get('name') + \
91- "-part%s" % determine_partition_number(volume, storage_config)
92- rule.append(compose_udev_equality('ENV{ID_PART_ENTRY_UUID}', ptuuid))
93+ # if partition has its own name, bind that to the existing PTUUID
94+ if dname:
95+ matches += [[compose_udev_equality('ENV{DEVTYPE}', "partition"),
96+ compose_udev_equality('ENV{ID_PART_ENTRY_UUID}',
97+ ptuuid)]]
98+ else:
99+ # disks generate dname-part%n rules automatically
100+ LOG.debug('No partition-specific dname')
101+ return
102 elif vol.get('type') == "raid":
103 md_data = mdadm.mdadm_query_detail(path)
104 md_uuid = md_data.get('MD_UUID')
105- rule.append(compose_udev_equality("ENV{MD_UUID}", md_uuid))
106+ matches += [[compose_udev_equality("ENV{MD_UUID}", md_uuid)]]
107 elif vol.get('type') == "bcache":
108 # bind dname to bcache backing device's dev.uuid as the bcache minor
109 # device numbers are not stable across reboots.
110@@ -265,12 +310,12 @@ def make_dname(volume, storage_config):
111 bcache_super = bcache.superblock_asdict(device=backing_dev)
112 if bcache_super and bcache_super['sb.version'].startswith('1'):
113 bdev_uuid = bcache_super['dev.uuid']
114- rule.append(compose_udev_equality("ENV{CACHED_UUID}", bdev_uuid))
115+ matches += [[compose_udev_equality("ENV{CACHED_UUID}", bdev_uuid)]]
116 bcache.write_label(sanitize_dname(dname), backing_dev)
117 elif vol.get('type') == "lvm_partition":
118 volgroup_name = storage_config.get(vol.get('volgroup')).get('name')
119 dname = "%s-%s" % (volgroup_name, dname)
120- rule.append(compose_udev_equality("ENV{DM_NAME}", dname))
121+ matches += [[compose_udev_equality("ENV{DM_NAME}", dname)]]
122 else:
123 raise ValueError('cannot make dname for device with type: {}'
124 .format(vol.get('type')))
125@@ -283,11 +328,25 @@ def make_dname(volume, storage_config):
126 LOG.warning(
127 "dname modified to remove invalid chars. old: '{}' new: '{}'"
128 .format(dname, sanitized))
129- rule.append("SYMLINK+=\"disk/by-dname/%s\"\n" % sanitized)
130- LOG.debug("Writing dname udev rule '{}'".format(str(rule)))
131+ content = ['# Written by curtin']
132+ for match in matches:
133+ rule = (base_rule + match +
134+ ["SYMLINK+=\"disk/by-dname/%s\"\n" % sanitized])
135+ LOG.debug("Creating dname udev rule '{}'".format(str(rule)))
136+ content.append(', '.join(rule))
137+
138+ if vol.get('type') == 'disk':
139+ for brule in byid:
140+ rule = (base_rule +
141+ [compose_udev_equality('ENV{DEVTYPE}', 'partition')] +
142+ brule +
143+ ['SYMLINK+="disk/by-dname/%s-part%%n"\n' % sanitized])
144+ LOG.debug("Creating dname udev rule '{}'".format(str(rule)))
145+ content.append(', '.join(rule))
146+
147 util.ensure_dir(rules_dir)
148 rule_file = os.path.join(rules_dir, '{}.rules'.format(sanitized))
149- util.write_file(rule_file, ', '.join(rule))
150+ util.write_file(rule_file, '\n'.join(content))
151
152
153 def get_poolname(info, storage_config):
154diff --git a/curtin/udev.py b/curtin/udev.py
155index 13d9cc5..106a7e7 100644
156--- a/curtin/udev.py
157+++ b/curtin/udev.py
158@@ -61,4 +61,41 @@ def udevadm_trigger(devices):
159 util.subp(['udevadm', 'trigger'] + list(devices))
160 udevadm_settle()
161
162+
163+def udevadm_info(path=None):
164+ """ Return a dictionary populated by properties of the device specified
165+ in the `path` variable via querying udev 'property' database.
166+
167+ :params: path: path to device, either /dev or /sys
168+ :returns: dictionary of key=value pairs as exported from the udev database
169+ :raises: ValueError path is None, ProcessExecutionError on exec error.
170+ """
171+ if not path:
172+ raise ValueError('Invalid path: "%s"' % path)
173+
174+ info_cmd = ['udevadm', 'info', '--query=property', path]
175+ output, _ = util.subp(info_cmd, capture=True)
176+
177+ # strip for trailing empty line
178+ info = {}
179+ for line in output.splitlines():
180+ if not line:
181+ continue
182+ # maxsplit=2 gives us key and remaininng part of line is value
183+ # py2.7 on Trusty doesn't have keyword, pass as argument
184+ key, value = line.split('=', 2)
185+ if not value:
186+ value = None
187+ if value:
188+ # devlinks is a list of paths separated by space
189+ # convert to a list for easy use
190+ if key == 'DEVLINKS':
191+ info[key] = value.split()
192+ else:
193+ # preserve spaces in values, to match udev database
194+ info[key] = value
195+
196+ return info
197+
198+
199 # vi: ts=4 expandtab syntax=python
200diff --git a/doc/topics/storage.rst b/doc/topics/storage.rst
201index 92fdbc3..f9a9fe8 100644
202--- a/doc/topics/storage.rst
203+++ b/doc/topics/storage.rst
204@@ -175,7 +175,15 @@ not need to be preserved.
205 If the ``name`` key is present, curtin will create a udev rule that makes a
206 symbolic link to the disk with the given name value. This makes it easy to find
207 disks on an installed system. The links are created in
208-``/dev/disk/by-dname/<name>``.
209+``/dev/disk/by-dname/<name>``. The udev rules will utilize two types of disk
210+metadata to construct the link. For disks with ``serial`` and/or ``wwn`` values
211+these will be used to ensure the name persists even if the contents of the disk
212+change. For legacy purposes, curtin also emits a rule utilizing metadata on
213+the disk contents, typically a partition UUID value, this also preserves these
214+links for disks which lack persistent attributes such as a ``serial`` or
215+``wwn``, typically found on virtualized environments where such values are left
216+unset.
217+
218 A link to each partition on the disk will also be created at
219 ``/dev/disk/by-dname/<name>-part<number>``, so if ``name: maindisk`` is set,
220 the disk will be at ``/dev/disk/by-dname/maindisk`` and the first partition on
221@@ -276,6 +284,17 @@ filesystem or be mounted anywhere on the system.
222 If the preserve flag is set to true, curtin will verify that the partition
223 exists and will not modify the partition.
224
225+**name**: *<name>*
226+
227+If the ``name`` key is present, curtin will create a udev rule that makes a
228+symbolic link to the partition with the given name value. The links are created
229+in ``/dev/disk/by-dname/<name>``.
230+
231+For partitions, the udev rule created relies upon disk contents, in this case
232+the partition entry UUID. This will remain in effect unless the underlying disk
233+on which the partition resides has the partition table modified or wiped.
234+
235+
236 **Config Example**::
237
238 - id: disk0-part1
239@@ -284,6 +303,7 @@ exists and will not modify the partition.
240 size: 8GB
241 device: disk0
242 flag: boot
243+ name: boot_partition
244
245 .. _format:
246
247@@ -504,6 +524,12 @@ scheme for Logical Volumes follows the pattern
248 with ``name`` *lv1* on a ``lvm_volgroup`` named *vg1* would have the path
249 ``/dev/disk/by-dname/vg1-lv1``.
250
251+.. note::
252+
253+ dname values for contructed devices (such as lvm) only remain persistent
254+ as long as the device metadata does not change. If users modify the device
255+ such that device metadata is changed then the udev rule may no longer apply.
256+
257 **volgroup**: *<volgroup id>*
258
259 The ``volgroup`` key specifies the ``id`` of the Volume Group in which to
260@@ -600,7 +626,9 @@ The ``name`` key specifies the name of the md device.
261 .. note::
262
263 Curtin creates a udev rule to create a link to the md device in
264- ``/dev/disk/by-dname/<name>`` using the specified name.
265+ ``/dev/disk/by-dname/<name>`` using the specified name. The dname
266+ symbolic link is only persistent as long as the raid metadata is
267+ not modifed or destroyed.
268
269 **raidlevel**: *0, 1, 5, 6, 10*
270
271@@ -677,6 +705,13 @@ reads/writes from the cache. None effectively disables bcache.
272 If the ``name`` key is present, curtin will create a link to the device at
273 ``/dev/disk/by-dname/<name>``.
274
275+.. note::
276+
277+ dname values for contructed devices (such as bcache) only remain persistent
278+ as long as the device metadata does not change. If users modify the device
279+ such that device metadata is changed then the udev rule may no longer apply.
280+
281+
282 **Config Example**::
283
284 - id: bcache0
285diff --git a/examples/tests/basic.yaml b/examples/tests/basic.yaml
286index f0ddc5d..71730c0 100644
287--- a/examples/tests/basic.yaml
288+++ b/examples/tests/basic.yaml
289@@ -26,6 +26,7 @@ storage:
290 number: 3
291 size: 1GB
292 device: sda
293+ name: swap
294 - id: sda1_root
295 type: format
296 fstype: ext4
297@@ -88,6 +89,7 @@ storage:
298 device: pnum_disk
299 flag: prep
300 wipe: zero
301+ name: prep
302 - id: pnum_disk_p3
303 type: partition
304 number: 10
305diff --git a/examples/tests/basic_scsi.yaml b/examples/tests/basic_scsi.yaml
306index 0ea8a51..aa62137 100644
307--- a/examples/tests/basic_scsi.yaml
308+++ b/examples/tests/basic_scsi.yaml
309@@ -25,14 +25,20 @@ storage:
310 number: 3
311 size: 1GB
312 device: sda
313+ name: swap
314 - id: sda1_root
315 type: format
316 fstype: ext4
317 volume: sda1
318+ label: 'cloudimg-rootfs'
319 - id: sda2_home
320 type: format
321 fstype: ext4
322 volume: sda2
323+ - id: sda3_swap
324+ type: format
325+ fstype: swap
326+ volume: sda3
327 - id: sda1_mount
328 type: mount
329 path: /
330@@ -46,6 +52,10 @@ storage:
331 wwn: '0x080258d13ea95ae5'
332 name: sparedisk
333 wipe: superblock
334+ - id: sparedisk_fat_fmt_id
335+ type: format
336+ fstype: fat32
337+ volume: sparedisk_id
338 - id: btrfs_disk_id
339 type: disk
340 wwn: '0x22dc58dc023c7008'
341@@ -78,8 +88,13 @@ storage:
342 device: pnum_disk
343 flag: prep
344 wipe: zero
345+ name: prep
346 - id: pnum_disk_p3
347 type: partition
348 number: 10
349 size: 1GB
350 device: pnum_disk
351+ - id: swap_mnt
352+ type: mount
353+ path: "none"
354+ device: sda3_swap
355diff --git a/examples/tests/nvme.yaml b/examples/tests/nvme.yaml
356index b2a1276..4cf7735 100644
357--- a/examples/tests/nvme.yaml
358+++ b/examples/tests/nvme.yaml
359@@ -44,7 +44,7 @@ storage:
360 device: main_disk_home
361 - id: nvme_disk
362 type: disk
363- path: /dev/nvme0n1
364+ serial: nvme-a
365 name: nvme_disk
366 wipe: superblock
367 ptable: gpt
368@@ -63,7 +63,7 @@ storage:
369 device: nvme_disk
370 - id: nvme_disk2
371 type: disk
372- path: /dev/nvme1n1
373+ serial: nvme-b
374 wipe: superblock
375 ptable: msdos
376 name: second_nvme
377diff --git a/examples/tests/nvme_bcache.yaml b/examples/tests/nvme_bcache.yaml
378index 2ee0ad3..4fefd94 100644
379--- a/examples/tests/nvme_bcache.yaml
380+++ b/examples/tests/nvme_bcache.yaml
381@@ -19,7 +19,7 @@ storage:
382 model: INTEL SSDPEDME400G4
383 name: nvme0n1
384 ptable: gpt
385- path: /dev/nvme0n1
386+ serial: nvme-a
387 type: disk
388 wipe: superblock
389 - device: sda
390diff --git a/tests/unittests/helpers.py b/tests/unittests/helpers.py
391index 58e068b..1268880 100644
392--- a/tests/unittests/helpers.py
393+++ b/tests/unittests/helpers.py
394@@ -5,7 +5,9 @@ import imp
395 import importlib
396 import mock
397 import os
398+import random
399 import shutil
400+import string
401 import tempfile
402 from unittest import TestCase
403
404@@ -67,6 +69,11 @@ class CiTestCase(TestCase):
405 return os.path.normpath(
406 os.path.abspath(os.path.sep.join((_dir, path))))
407
408+ def random_string(self, length=8):
409+ """ return a random lowercase string with default length of 8"""
410+ return ''.join(
411+ random.choice(string.ascii_lowercase) for _ in range(length))
412+
413
414 def dir2dict(startdir, prefix=None):
415 flist = {}
416diff --git a/tests/unittests/test_make_dname.py b/tests/unittests/test_make_dname.py
417index 2b92a88..76c7b28 100644
418--- a/tests/unittests/test_make_dname.py
419+++ b/tests/unittests/test_make_dname.py
420@@ -13,10 +13,16 @@ class TestMakeDname(CiTestCase):
421 state = {'scratch': '/tmp/null'}
422 rules_d = '/tmp/null/rules.d'
423 rule_file = '/tmp/null/rules.d/{}.rules'
424+ disk_serial = 'abcdefg'
425+ disk_wwn = '0x1234567890'
426 storage_config = {
427- 'disk1': {'type': 'disk', 'id': 'disk1', 'name': 'main_disk'},
428+ 'disk1': {'type': 'disk', 'id': 'disk1', 'name': 'main_disk',
429+ 'serial': disk_serial},
430+ 'disk_noid': {'type': 'disk', 'id': 'disk_noid', 'name': 'main_disk'},
431 'disk1p1': {'type': 'partition', 'id': 'disk1p1', 'device': 'disk1'},
432- 'disk2': {'type': 'disk', 'id': 'disk2',
433+ 'disk1p2': {'type': 'partition', 'id': 'disk1p2', 'device': 'disk1',
434+ 'name': 'custom-partname'},
435+ 'disk2': {'type': 'disk', 'id': 'disk2', 'wwn': disk_wwn,
436 'name': 'in_valid/name!@#$% &*(+disk'},
437 'disk2p1': {'type': 'partition', 'id': 'disk2p1', 'device': 'disk2'},
438 'md_id': {'type': 'raid', 'id': 'md_id', 'name': 'mdadm_name'},
439@@ -27,7 +33,8 @@ class TestMakeDname(CiTestCase):
440 'lpart2_id': {'type': 'lvm_partition', 'id': 'lpart2_id',
441 'name': 'lvm part/2', 'volgroup': 'lvol_id'},
442 'bcache1_id': {'type': 'bcache', 'id': 'bcache1_id',
443- 'name': 'my-cached-data'}
444+ 'name': 'my-cached-data'},
445+ 'iscsi1': {'type': 'disk', 'id': 'iscsi1', 'name': 'iscsi_disk1'}
446 }
447 bcache_super_show = {
448 'sb.version': '1 [backing device]',
449@@ -57,20 +64,37 @@ class TestMakeDname(CiTestCase):
450 rule.append('SYMLINK+="disk/by-dname/{}"\n'.format(target))
451 return ', '.join(rule)
452
453+ def _content(self, rules=[]):
454+ return "\n".join(['# Written by curtin'] + rules)
455+
456+ @mock.patch('curtin.commands.block_meta.udevadm_info')
457 @mock.patch('curtin.commands.block_meta.LOG')
458 @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
459 @mock.patch('curtin.commands.block_meta.util')
460- def test_make_dname_disk(self, mock_util, mock_get_path, mock_log):
461+ def test_make_dname_disk(self, mock_util, mock_get_path, mock_log,
462+ mock_udev):
463 disk_ptuuid = str(uuid.uuid1())
464 mock_util.subp.side_effect = self._make_mock_subp_blkid(
465 disk_ptuuid, self.disk_blkid)
466 mock_util.load_command_environment.return_value = self.state
467- rule_identifiers = [
468- ('DEVTYPE', 'disk'),
469- ('ID_PART_TABLE_UUID', disk_ptuuid)
470- ]
471+
472+ rule_identifiers = [('ID_PART_TABLE_UUID', disk_ptuuid)]
473+ id_rule_identifiers = [('ID_SERIAL', self.disk_serial)]
474+ wwn_rule_identifiers = [('ID_WWN_WITH_EXTENSION', self.disk_wwn)]
475+
476+ def _drule(devtype, match):
477+ return [('DEVTYPE', devtype)] + [m for m in match]
478+
479+ def drule(match):
480+ return _drule('disk', match)
481+
482+ def prule(match):
483+ return _drule('partition', match)
484
485 # simple run
486+ mock_udev.side_effect = (
487+ [{'DEVTYPE': 'disk', 'ID_SERIAL': self.disk_serial},
488+ {'DEVTYPE': 'disk', 'ID_WWN_WITH_EXTENSION': self.disk_wwn}])
489 res_dname = 'main_disk'
490 block_meta.make_dname('disk1', self.storage_config)
491 mock_util.ensure_dir.assert_called_with(self.rules_d)
492@@ -78,7 +102,13 @@ class TestMakeDname(CiTestCase):
493 self.assertFalse(mock_log.warning.called)
494 mock_util.write_file.assert_called_with(
495 self.rule_file.format(res_dname),
496- self._formatted_rule(rule_identifiers, res_dname))
497+ self._content(
498+ [self._formatted_rule(drule(rule_identifiers),
499+ res_dname),
500+ self._formatted_rule(drule(id_rule_identifiers),
501+ res_dname),
502+ self._formatted_rule(prule(id_rule_identifiers),
503+ "%s-part%%n" % res_dname)]))
504
505 # run invalid dname
506 res_dname = 'in_valid-name----------disk'
507@@ -86,12 +116,39 @@ class TestMakeDname(CiTestCase):
508 self.assertTrue(mock_log.warning.called)
509 mock_util.write_file.assert_called_with(
510 self.rule_file.format(res_dname),
511- self._formatted_rule(rule_identifiers, res_dname))
512+ self._content(
513+ [self._formatted_rule(drule(rule_identifiers),
514+ res_dname),
515+ self._formatted_rule(drule(wwn_rule_identifiers),
516+ res_dname),
517+ self._formatted_rule(prule(wwn_rule_identifiers),
518+ "%s-part%%n" % res_dname)]))
519
520+ # iscsi disk with no config, but info returns serial and wwn
521+ mock_udev.side_effect = (
522+ [{'DEVTYPE': 'disk', 'ID_SERIAL': self.disk_serial,
523+ 'DEVTYPE': 'disk', 'ID_WWN_WITH_EXTENSION': self.disk_wwn}])
524+ res_dname = 'iscsi_disk1'
525+ block_meta.make_dname('iscsi1', self.storage_config)
526+ mock_util.ensure_dir.assert_called_with(self.rules_d)
527+ self.assertTrue(mock_log.debug.called)
528+ both_rules = (id_rule_identifiers + wwn_rule_identifiers)
529+ mock_util.write_file.assert_called_with(
530+ self.rule_file.format(res_dname),
531+ self._content(
532+ [self._formatted_rule(drule(rule_identifiers), res_dname),
533+ self._formatted_rule(drule(both_rules), res_dname),
534+ self._formatted_rule(prule(both_rules),
535+ "%s-part%%n" % res_dname)]))
536+
537+ @mock.patch('curtin.commands.block_meta.udevadm_info')
538 @mock.patch('curtin.commands.block_meta.LOG')
539 @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
540 @mock.patch('curtin.commands.block_meta.util')
541- def test_make_dname_failures(self, mock_util, mock_get_path, mock_log):
542+ def test_make_dname_failures(self, mock_util, mock_get_path, mock_log,
543+ mock_udev):
544+ mock_udev.side_effect = ([{'DEVTYPE': 'disk'}, {'DEVTYPE': 'disk'}])
545+
546 mock_util.subp.side_effect = self._make_mock_subp_blkid(
547 '', self.trusty_blkid)
548 mock_util.load_command_environment.return_value = self.state
549@@ -99,10 +156,14 @@ class TestMakeDname(CiTestCase):
550 warning_msg = "Can't find a uuid for volume: {}. Skipping dname."
551
552 # disk with no PT_UUID
553- block_meta.make_dname('disk1', self.storage_config)
554- mock_log.warning.assert_called_with(warning_msg.format('disk1'))
555- self.assertFalse(mock_util.write_file.called)
556+ disk = 'disk_noid'
557+ with self.assertRaises(RuntimeError):
558+ block_meta.make_dname(disk, self.storage_config)
559+ mock_log.warning.assert_called_with(warning_msg.format(disk))
560+ self.assertFalse(mock_util.write_file.called)
561
562+ mock_util.subp.side_effect = self._make_mock_subp_blkid(
563+ '', self.trusty_blkid)
564 # partition with no PART_UUID
565 block_meta.make_dname('disk1p1', self.storage_config)
566 mock_log.warning.assert_called_with(warning_msg.format('disk1p1'))
567@@ -123,22 +184,15 @@ class TestMakeDname(CiTestCase):
568 ]
569
570 # simple run
571- res_dname = 'main_disk-part1'
572- block_meta.make_dname('disk1p1', self.storage_config)
573+ res_dname = 'custom-partname'
574+ block_meta.make_dname('disk1p2', self.storage_config)
575 mock_util.ensure_dir.assert_called_with(self.rules_d)
576 self.assertTrue(mock_log.debug.called)
577 self.assertFalse(mock_log.warning.called)
578 mock_util.write_file.assert_called_with(
579 self.rule_file.format(res_dname),
580- self._formatted_rule(rule_identifiers, res_dname))
581-
582- # run invalid dname
583- res_dname = 'in_valid-name----------disk-part1'
584- block_meta.make_dname('disk2p1', self.storage_config)
585- self.assertTrue(mock_log.warning.called)
586- mock_util.write_file.assert_called_with(
587- self.rule_file.format(res_dname),
588- self._formatted_rule(rule_identifiers, res_dname))
589+ self._content(
590+ [self._formatted_rule(rule_identifiers, res_dname)]))
591
592 @mock.patch('curtin.commands.block_meta.mdadm')
593 @mock.patch('curtin.commands.block_meta.LOG')
594@@ -158,7 +212,8 @@ class TestMakeDname(CiTestCase):
595 self.assertFalse(mock_log.warning.called)
596 mock_util.write_file.assert_called_with(
597 self.rule_file.format(res_dname),
598- self._formatted_rule(rule_identifiers, res_dname))
599+ self._content(
600+ [self._formatted_rule(rule_identifiers, res_dname)]))
601
602 # invalid name
603 res_dname = 'mdadm-name'
604@@ -166,7 +221,8 @@ class TestMakeDname(CiTestCase):
605 self.assertTrue(mock_log.warning.called)
606 mock_util.write_file.assert_called_with(
607 self.rule_file.format(res_dname),
608- self._formatted_rule(rule_identifiers, res_dname))
609+ self._content(
610+ [self._formatted_rule(rule_identifiers, res_dname)]))
611
612 @mock.patch('curtin.commands.block_meta.LOG')
613 @mock.patch('curtin.commands.block_meta.get_path_to_storage_volume')
614@@ -183,7 +239,8 @@ class TestMakeDname(CiTestCase):
615 self.assertFalse(mock_log.warning.called)
616 mock_util.write_file.assert_called_with(
617 self.rule_file.format(res_dname),
618- self._formatted_rule(rule_identifiers, res_dname))
619+ self._content(
620+ [self._formatted_rule(rule_identifiers, res_dname)]))
621
622 # with invalid name
623 res_dname = 'vg1-lvm-part-2'
624@@ -192,7 +249,8 @@ class TestMakeDname(CiTestCase):
625 self.assertTrue(mock_log.warning.called)
626 mock_util.write_file.assert_called_with(
627 self.rule_file.format(res_dname),
628- self._formatted_rule(rule_identifiers, res_dname))
629+ self._content(
630+ [self._formatted_rule(rule_identifiers, res_dname)]))
631
632 @mock.patch('curtin.commands.block_meta.LOG')
633 @mock.patch('curtin.commands.block_meta.bcache')
634@@ -213,7 +271,8 @@ class TestMakeDname(CiTestCase):
635 self.assertFalse(mock_log.warning.called)
636 mock_util.write_file.assert_called_with(
637 self.rule_file.format(res_dname),
638- self._formatted_rule(rule_identifiers, res_dname))
639+ self._content(
640+ [self._formatted_rule(rule_identifiers, res_dname)]))
641
642 def test_sanitize_dname(self):
643 unsanitized_to_sanitized = [
644@@ -226,4 +285,87 @@ class TestMakeDname(CiTestCase):
645 for (unsanitized, sanitized) in unsanitized_to_sanitized:
646 self.assertEqual(block_meta.sanitize_dname(unsanitized), sanitized)
647
648+
649+class TestMakeDnameById(CiTestCase):
650+
651+ @mock.patch('curtin.commands.block_meta.udevadm_info')
652+ def test_bad_path(self, m_udev):
653+ """test dname_byid raises ValueError on invalid path."""
654+ mypath = None
655+ with self.assertRaises(ValueError):
656+ block_meta.make_dname_byid(mypath)
657+
658+ @mock.patch('curtin.commands.block_meta.udevadm_info')
659+ def test_non_disk(self, m_udev):
660+ """test dname_byid raises ValueError on DEVTYPE != 'disk'"""
661+ mypath = "/dev/" + self.random_string()
662+ m_udev.return_value = {'DEVTYPE': 'not_a_disk'}
663+ with self.assertRaises(ValueError):
664+ block_meta.make_dname_byid(mypath)
665+
666+ @mock.patch('curtin.commands.block_meta.udevadm_info')
667+ def test_disk_with_no_id_wwn(self, m_udev):
668+ """test dname_byid raises RuntimeError on device without ID or WWN."""
669+ mypath = "/dev/" + self.random_string()
670+ m_udev.return_value = {'DEVTYPE': 'disk'}
671+ with self.assertRaises(RuntimeError):
672+ block_meta.make_dname_byid(mypath)
673+
674+ @mock.patch('curtin.commands.block_meta.udevadm_info')
675+ def test_udevinfo_not_called_if_info_provided(self, m_udev):
676+ """dname_byid does not invoke udevadm_info if using info dict"""
677+ myserial = self.random_string()
678+ self.assertEqual(
679+ [['ENV{ID_SERIAL}=="%s"' % myserial]],
680+ block_meta.make_dname_byid(
681+ self.random_string(),
682+ info={'DEVTYPE': 'disk', 'ID_SERIAL': myserial}))
683+ self.assertEqual(0, m_udev.call_count)
684+
685+ @mock.patch('curtin.commands.block_meta.udevadm_info')
686+ def test_udevinfo_called_if_info_not_provided(self, m_udev):
687+ """dname_byid should call udevadm_info if no data given."""
688+ myserial = self.random_string()
689+ mypath = "/dev/" + self.random_string()
690+ m_udev.return_value = {
691+ 'DEVTYPE': 'disk', 'ID_SERIAL': myserial, 'DEVNAME': mypath}
692+ self.assertEqual(
693+ [['ENV{ID_SERIAL}=="%s"' % myserial]],
694+ block_meta.make_dname_byid(mypath))
695+ self.assertEqual(
696+ [mock.call(path=mypath)], m_udev.call_args_list)
697+
698+ def test_disk_with_only_serial(self):
699+ """test dname_byid returns rules for ID_SERIAL"""
700+ mypath = "/dev/" + self.random_string()
701+ myserial = self.random_string()
702+ info = {'DEVTYPE': 'disk', 'DEVNAME': mypath, 'ID_SERIAL': myserial}
703+ self.assertEqual(
704+ [['ENV{ID_SERIAL}=="%s"' % myserial]],
705+ block_meta.make_dname_byid(mypath, info=info))
706+
707+ def test_disk_with_only_wwn(self):
708+ """test dname_byid returns rules for ID_WWN_WITH_EXTENSION"""
709+ mypath = "/dev/" + self.random_string()
710+ mywwn = self.random_string()
711+ info = {'DEVTYPE': 'disk', 'DEVNAME': mypath,
712+ 'ID_WWN_WITH_EXTENSION': mywwn}
713+ self.assertEqual(
714+ [['ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn]],
715+ block_meta.make_dname_byid(mypath, info=info))
716+
717+ def test_disk_with_both_id_wwn(self):
718+ """test dname_byid returns rules with both ID_SERIAL and ID_WWN"""
719+ mypath = "/dev/" + self.random_string()
720+ myserial = self.random_string()
721+ mywwn = self.random_string()
722+ info = {'DEVTYPE': 'disk', 'ID_SERIAL': myserial,
723+ 'ID_WWN_WITH_EXTENSION': mywwn,
724+ 'DEVNAME': mypath}
725+ self.assertEqual(
726+ [['ENV{ID_SERIAL}=="%s"' % myserial,
727+ 'ENV{ID_WWN_WITH_EXTENSION}=="%s"' % mywwn]],
728+ block_meta.make_dname_byid(mypath, info=info))
729+
730+
731 # vi: ts=4 expandtab syntax=python
732diff --git a/tests/unittests/test_udev.py b/tests/unittests/test_udev.py
733new file mode 100644
734index 0000000..0a070d5
735--- /dev/null
736+++ b/tests/unittests/test_udev.py
737@@ -0,0 +1,68 @@
738+# This file is part of curtin. See LICENSE file for copyright and license info.
739+
740+import mock
741+
742+from curtin import udev
743+from curtin import util
744+from .helpers import CiTestCase
745+
746+
747+UDEVADM_INFO_QUERY = """\
748+DEVLINKS=/dev/disk/by-id/nvme-eui.0025388b710116a1
749+DEVNAME=/dev/nvme0n1
750+DEVPATH=/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/nvme/nvme0/nvme0n1
751+DEVTYPE=disk
752+ID_PART_TABLE_TYPE=gpt
753+ID_PART_TABLE_UUID=ea0b9ddc-a114-4e01-b257-750d86e3a944
754+ID_SERIAL=SAMSUNG MZVLB1T0HALR-000L7_S3TPNY0JB00151
755+ID_SERIAL_SHORT=S3TPNY0JB00151
756+MAJOR=259
757+MINOR=0
758+SUBSYSTEM=block
759+TAGS=:systemd:
760+USEC_INITIALIZED=2026691
761+"""
762+
763+INFO_DICT = {
764+ 'DEVLINKS': ['/dev/disk/by-id/nvme-eui.0025388b710116a1'],
765+ 'DEVNAME': '/dev/nvme0n1',
766+ 'DEVPATH':
767+ '/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/nvme/nvme0/nvme0n1',
768+ 'DEVTYPE': 'disk',
769+ 'ID_PART_TABLE_TYPE': 'gpt',
770+ 'ID_PART_TABLE_UUID': 'ea0b9ddc-a114-4e01-b257-750d86e3a944',
771+ 'ID_SERIAL': 'SAMSUNG MZVLB1T0HALR-000L7_S3TPNY0JB00151',
772+ 'ID_SERIAL_SHORT': 'S3TPNY0JB00151',
773+ 'MAJOR': '259',
774+ 'MINOR': '0',
775+ 'SUBSYSTEM': 'block',
776+ 'TAGS': ':systemd:',
777+ 'USEC_INITIALIZED': '2026691'
778+}
779+
780+
781+class TestUdevInfo(CiTestCase):
782+
783+ @mock.patch('curtin.util.subp')
784+ def test_udevadm_info(self, m_subp):
785+ """ udevadm_info returns dictionary for specified device """
786+ mypath = '/dev/nvme0n1'
787+ m_subp.return_value = (UDEVADM_INFO_QUERY, "")
788+ info = udev.udevadm_info(mypath)
789+ m_subp.assert_called_with(
790+ ['udevadm', 'info', '--query=property', mypath], capture=True)
791+ self.assertEqual(sorted(INFO_DICT), sorted(info))
792+
793+ def test_udevadm_info_no_path(self):
794+ """ udevadm_info raises ValueError for invalid path value"""
795+ mypath = None
796+ with self.assertRaises(ValueError):
797+ udev.udevadm_info(mypath)
798+
799+ @mock.patch('curtin.util.subp')
800+ def test_udevadm_info_path_not_exists(self, m_subp):
801+ """ udevadm_info raises ProcessExecutionError for invalid path value"""
802+ mypath = self.random_string()
803+ m_subp.side_effect = util.ProcessExecutionError()
804+ with self.assertRaises(util.ProcessExecutionError):
805+ udev.udevadm_info(mypath)
806diff --git a/tests/vmtests/__init__.py b/tests/vmtests/__init__.py
807index 51eda5c..5ffa632 100644
808--- a/tests/vmtests/__init__.py
809+++ b/tests/vmtests/__init__.py
810@@ -21,6 +21,7 @@ from curtin.block import iscsi
811
812 from .report_webhook_logger import CaptureReporting
813 from curtin.commands.install import INSTALL_PASS_MSG
814+from curtin.commands.block_meta import sanitize_dname
815
816 from .image_sync import query as imagesync_query
817 from .image_sync import mirror as imagesync_mirror
818@@ -955,9 +956,18 @@ class VMBaseClass(TestCase):
819 storage_config = cls.get_class_storage_config()
820 cls.disk_wwns = ["wwn=%s" % x.get('wwn') for x in storage_config
821 if 'wwn' in x]
822- cls.disk_serials = ["serial=%s" % x.get('serial')
823- for x in storage_config if 'serial' in x]
824+ cls.disk_serials = []
825+ cls.nvme_serials = []
826+ for x in storage_config:
827+ if 'serial' in x:
828+ serial = x.get('serial')
829+ if serial.startswith('nvme'):
830+ cls.nvme_serials.append("serial=%s" % serial)
831+ else:
832+ cls.disk_serials.append("serial=%s" % serial)
833
834+ logger.info("disk_serials: %s", cls.disk_serials)
835+ logger.info("nvme_serials: %s", cls.nvme_serials)
836 target_disk = "{}:{}:{}:{}:".format(cls.td.target_disk,
837 "",
838 cls.disk_driver,
839@@ -989,11 +999,13 @@ class VMBaseClass(TestCase):
840 disks.extend(['--disk', extra_disk])
841
842 # build nvme disk args if needed
843+ logger.info('nvme disks: %s', cls.nvme_disks)
844 for (disk_no, disk_sz) in enumerate(cls.nvme_disks):
845 dpath = os.path.join(cls.td.disks, 'nvme_disk_%d.img' % disk_no)
846+ nvme_serial = cls.nvme_serials[disk_no]
847 nvme_disk = '{}:{}:nvme:{}:{}'.format(dpath, disk_sz,
848 cls.disk_block_size,
849- "serial=nvme-%d" % disk_no)
850+ "%s" % nvme_serial)
851 disks.extend(['--disk', nvme_disk])
852
853 # build iscsi disk args if needed
854@@ -1222,6 +1234,8 @@ class VMBaseClass(TestCase):
855 dpath = os.path.join(cls.td.disks, 'nvme_disk_%d.img' % disk_no)
856 disk = '--disk={},driver={},format={},{}'.format(
857 dpath, disk_driver, TARGET_IMAGE_FORMAT, bsize_args)
858+ if len(cls.nvme_serials):
859+ disk += ",%s" % cls.nvme_serials[disk_no]
860 nvme_disks.extend([disk])
861
862 # unlike NVMe disks, we do not want to configure the iSCSI disks
863@@ -1541,8 +1555,41 @@ class VMBaseClass(TestCase):
864 for diskname, part in self.disk_to_check:
865 if part is not 0:
866 link = diskname + "-part" + str(part)
867- self.assertIn(link, contents)
868- self.assertIn(diskname, contents)
869+ self.assertIn(link, contents.splitlines())
870+ self.assertIn(diskname, contents.splitlines())
871+
872+ @skip_if_flag('expected_failure')
873+ def test_dname_rules(self, disk_to_check=None):
874+ if self.target_distro != "ubuntu":
875+ raise SkipTest("dname not present in non-ubuntu releases")
876+
877+ if not disk_to_check:
878+ disk_to_check = self.disk_to_check
879+ if disk_to_check is None:
880+ logger.debug('test_dname_rules: no disks to check')
881+ return
882+ logger.debug('test_dname_rules: checking disks: %s', disk_to_check)
883+ self.output_files_exist(["udev_rules.d"])
884+
885+ cfg = yaml.load(self.load_collect_file("root/curtin-install-cfg.yaml"))
886+ stgcfg = cfg.get("storage", {}).get("config", [])
887+ disks = [ent for ent in stgcfg if (ent.get('type') == 'disk' and
888+ 'name' in ent)]
889+ key_to_udev = {
890+ 'serial': 'ID_SERIAL',
891+ 'wwn': 'ID_WWN_WITH_EXTENSION',
892+ }
893+ for disk in disks:
894+ dname_file = "%s.rules" % sanitize_dname(disk.get('name'))
895+ contents = self.load_collect_file("udev_rules.d/%s" % dname_file)
896+ for key, key_name in key_to_udev.items():
897+ value = disk.get(key)
898+ if value:
899+ # serials may include spaces, udev replaces them with # _
900+ if ' ' in value:
901+ value = value.replace(' ', '_')
902+ self.assertIn(key_name, contents)
903+ self.assertIn(value, contents)
904
905 @skip_if_flag('expected_failure')
906 def test_reporting_data(self):
907@@ -1765,6 +1812,9 @@ class PsuedoVMBaseClass(VMBaseClass):
908 def test_dname(self, disk_to_check=None):
909 pass
910
911+ def test_dname_rules(self, disk_to_check=None):
912+ pass
913+
914 def test_interfacesd_eth0_removed(self):
915 pass
916
917diff --git a/tests/vmtests/test_basic.py b/tests/vmtests/test_basic.py
918index 5cb8c30..48f07d6 100644
919--- a/tests/vmtests/test_basic.py
920+++ b/tests/vmtests/test_basic.py
921@@ -17,9 +17,14 @@ class TestBasicAbs(VMBaseClass):
922 dirty_disks = True
923 conf_file = "examples/tests/basic.yaml"
924 extra_disks = ['128G', '128G', '4G']
925- nvme_disks = ['4G']
926- disk_to_check = [('main_disk_with_in---valid--dname', 1),
927- ('main_disk_with_in---valid--dname', 2)]
928+ disk_to_check = [('btrfs_volume', 0),
929+ ('main_disk_with_in---valid--dname', 0),
930+ ('main_disk_with_in---valid--dname', 1),
931+ ('main_disk_with_in---valid--dname', 2),
932+ ('pnum_disk', 0),
933+ ('pnum_disk', 1),
934+ ('pnum_disk', 10),
935+ ('sparedisk', 0)]
936 extra_collect_scripts = [textwrap.dedent("""
937 cd OUTPUT_COLLECT_D
938 blkid -o export /dev/vda | cat >blkid_output_vda
939@@ -249,7 +254,6 @@ class TestBasicScsiAbs(TestBasicAbs):
940 conf_file = "examples/tests/basic_scsi.yaml"
941 disk_driver = 'scsi-hd'
942 extra_disks = ['128G', '128G', '4G']
943- nvme_disks = ['4G']
944 extra_collect_scripts = [textwrap.dedent("""
945 cd OUTPUT_COLLECT_D
946 blkid -o export /dev/sda | cat >blkid_output_sda
947diff --git a/tests/vmtests/test_nvme.py b/tests/vmtests/test_nvme.py
948index 7755b53..60d9d86 100644
949--- a/tests/vmtests/test_nvme.py
950+++ b/tests/vmtests/test_nvme.py
951@@ -19,9 +19,10 @@ class TestNvmeAbs(VMBaseClass):
952 conf_file = "examples/tests/nvme.yaml"
953 extra_disks = []
954 nvme_disks = ['4G', '4G']
955- disk_to_check = [('main_disk', 1), ('main_disk', 2), ('main_disk', 15),
956- ('nvme_disk', 1), ('nvme_disk', 2), ('nvme_disk', 3),
957- ('second_nvme', 1)]
958+ disk_to_check = [
959+ ('main_disk', 1), ('main_disk', 2), ('main_disk', 15),
960+ ('nvme_disk', 0), ('nvme_disk', 1), ('nvme_disk', 2), ('nvme_disk', 3),
961+ ('second_nvme', 0), ('second_nvme', 1)]
962 extra_collect_scripts = [textwrap.dedent("""
963 cd OUTPUT_COLLECT_D
964 ls /sys/class/ > sys_class
965@@ -97,7 +98,8 @@ class TestNvmeBcacheAbs(TestNvmeAbs):
966 extra_disks = ['10G']
967 nvme_disks = ['6G']
968 uefi = True
969- disk_to_check = [('sda', 1), ('sda', 2), ('sda', 3)]
970+ disk_to_check = [('sda', 1), ('sda', 2), ('sda', 3),
971+ ('sdb', 0), ('nvme0n1', 0)]
972
973 extra_collect_scripts = [textwrap.dedent("""
974 cd OUTPUT_COLLECT_D

Subscribers

People subscribed via source and target branches