curtin

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

1562249
Merge into trunk

Proposed by Wesley Wiedenmeier on 2016-06-28

Status:	Merged
Merged at revision:	406
Proposed branch:	lp://staging/~wesley-wiedenmeier/curtin/1562249
Merge into:	lp://staging/~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://staging/~wesley-wiedenmeier/curtin/1562249
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Server Team CI bot	continuous-integration		Approve on 2016-07-25
Ryan Harper (community)		2016-06-28	Needs Fixing on 2016-07-12
Review via email: mp+298482@code.staging.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

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2016-06-28:

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 on 2016-06-29:

- 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 on 2016-06-29:

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 on 2016-07-01:

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 on 2016-07-01:

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 on 2016-07-01:

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 on 2016-07-01:

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 on 2016-07-07:

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 on 2016-07-07:

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://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-08

415. By Wesley Wiedenmeier on 2016-07-08: 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 on 2016-07-08:

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://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-11

416. By Wesley Wiedenmeier on 2016-07-11: 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 on 2016-07-11: Also allow underscore in dname
418. By Wesley Wiedenmeier on 2016-07-11: 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 on 2016-07-11:

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://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-11

419. By Wesley Wiedenmeier on 2016-07-11: 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 on 2016-07-11: Added make_dname unittests

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2016-07-11:

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 on 2016-07-12:

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://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-13

421. By Wesley Wiedenmeier on 2016-07-13: cleaned up fixes to block and block_meta

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2016-07-13:

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 on 2016-07-13:

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 on 2016-07-13:

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

lp://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-21

422. By Wesley Wiedenmeier on 2016-07-21: Merge from trunk

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2016-07-21:

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 on 2016-07-21:

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 on 2016-07-25:

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

lp://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-25

423. By Wesley Wiedenmeier on 2016-07-25: Removed unneeded and unused call to os.path.normpath in
get_blockdev_for_partition()
424. By Wesley Wiedenmeier on 2016-07-25: 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 on 2016-07-25: Added test cases for mpath and dm devices in block.partition_kname

Revision history for this message

Wesley Wiedenmeier (wesley-wiedenmeier) wrote on 2016-07-25:

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 on 2016-07-25:

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 on 2016-07-25:

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 on 2016-07-30:

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://staging/~wesley-wiedenmeier/curtin/1562249 updated on 2016-07-30

426. By Wesley Wiedenmeier on 2016-07-30: 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