curtin

Merge lp://staging/~raharper/curtin/trunk.vmtest-multipath into lp://staging/~curtin-dev/curtin/trunk

trunk.vmtest-multipath
Merge into trunk

Proposed by Ryan Harper on 2016-06-09

Status:

Merged

Merged at revision:

396

Proposed branch:

lp://staging/~raharper/curtin/trunk.vmtest-multipath

Merge into:

lp://staging/~curtin-dev/curtin/trunk

Diff against target:

846 lines (+513/-68)

10 files modified

curtin/block/__init__.py (+9/-3)
curtin/commands/block_meta.py (+7/-2)
curtin/commands/curthooks.py (+17/-1)
curtin/util.py (+56/-0)
examples/tests/basic_scsi.yaml (+72/-0)
examples/tests/multipath.yaml (+38/-0)
tests/vmtests/__init__.py (+97/-45)
tests/vmtests/test_basic.py (+117/-0)
tests/vmtests/test_multipath.py (+63/-0)
tools/launch (+37/-17)

To merge this branch:

bzr merge lp://staging/~raharper/curtin/trunk.vmtest-multipath

Medium

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Server Team CI bot	continuous-integration		Approve on 2016-06-24
Scott Moser (community)		2016-06-09	Approve on 2016-06-24
Christian Ehrhardt 			Needs Fixing on 2016-06-10
Review via email: mp+297007@code.staging.launchpad.net

Commit message

Add multipath testing

Add multipath testing via a test-case option, cls.multipath which will
if set to true, duplicate all disks in a test class. This has the effect
of looking exactly like multipath setups where we see the same device
with two paths.

To handle devices with wwn or serial numbers with spaces in them
curtin.block was updated to ensure it supports finding devices via there
serial or wwn attribute. Specifically for serial numbers with spaces, we
currently will escape the spaces to underscores; this is supported by
observing the names that show up in /dev/disk/by-id/* for those devices.

For wwn, /dev/disk/by-id/wwn-$wwn ; curtin resolves the symlink target to
obtain a kernel devname for use with sysfs.

For serial with spaces, curtin attempts to remove whitespace like udev to
find the disk under /dev/disk/by-id/ and determine a devname.

Xenial multipath fails without ensuring the multipath/bindings file is
written with whitespace removed; this is triggered on in-target mulitpath
versions >= 1.5.0 fixing LP:1551937.

Description of the change

Add multipath testing

For wwn, /dev/disk/by-id/wwn-$wwn ; curtin resolves the symlink target to
obtain a kernel devname for use with sysfs.

For serial with spaces, curtin attempts to remove whitespace like udev to
find the disk under /dev/disk/by-id/ and determine a devname.

Xenial multipath fails without ensuring the multipath/bindings file is
written with whitespace removed; this is triggered on in-target mulitpath
versions >= 1.5.0 fixing LP:1551937.

Revision history for this message

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

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

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

review: Approve (continuous-integration)

Revision history for this message

Christian Ehrhardt  (paelzer) wrote on 2016-06-10:

Hi Ryan,
nice work on multipath testing!

I have found nothing severe but a collection of things that could be more clear, more polished or just were unclear at least to me.

review: Needs Fixing

Revision history for this message

Christian Ehrhardt  (paelzer) wrote on 2016-06-10:

Probably also worth to mention that actually running the tests failed me.
I did an sync-images to ensure I'm not just on some older sh$§.

But still running the full testsuite on your branch won't fully work:
Note at the same environment my last branch works (just to ensure the system setup isn't borked)

rm -rf output/; CURTIN_VMTEST_KEEP_DATA_FAIL=all ./tools/jenkins-runner -vv --nologcapture --processes=5 --process-timeout=3600 tests/vmtests 2>&1 | tee ~/curtin-test-multipath.log; rm -rf output/
Yours: => http://paste.ubuntu.com/17167038/
Mine as reference: => http://paste.ubuntu.com/17167042/

You can give it a look yourself it is on ubuntu@horsea:/mnt/nvme/trunk.vmtest-multipath and the logfiles of my runs are in home.
I imported your LP id to ubuntu@

Revision history for this message

Scott Moser (smoser) wrote on 2016-06-10:

On Fri, 10 Jun 2016, ChristianEhrhardt wrote:

> Review: Needs Fixing
>
> Hi Ryan,
> nice work on multipath testing!
>
> I have found nothing severe but a collection of things that could be more clear, more polished or just were unclear at least to me.
>
> Do you think it would make sense to make this a parameter maybe from cls.multipath.num_parallel or so?
> Effectively what you did is already so great, I don't see why it has to be always 2.
> That way you could do some sort of limit testing by setting it to (48*2)*(96*2) which would be the system z max.
> Not really that number :-), but driving the test with 2,16,64 could be a great check!
>
> General question: Is there a limit on qemu (commandline) that we have to be scared of?

Probably not too much to worry about. I'm pretty sure that its total of
command line + environment < 128k.

I just tried:
  $ ./foo.sh
  128000
  129000
  130000
  131000
  /tmp/foo.sh: 8: /tmp/foo.sh: /bin/true: Argument list too long
  i=131072 2
  $ cat foo.sh
  #!/bin/sh
  b=""; i=0
  while i=$(($i+1)) && b="$b."; do
      [ $i -lt 128000 ] && continue
      [ $(($i%1000)) -eq 0 ] && echo $i
      /bin/true "$b" || { ret=$?; break; }
  done
  echo i=$i $ret

Revision history for this message

Ryan Harper (raharper) wrote on 2016-06-10:

Download full text (16.9 KiB)

> Hi Ryan,
> nice work on multipath testing!
>
> I have found nothing severe but a collection of things that could be more clear, more polished or just were unclear at least to me.
>

Thanks for the review Christian. Picking up most of your suggestions.
A few things will need to put off for now.

> > === modified file 'curtin/block/__init__.py'
> > --- curtin/block/__init__.py 2016-04-22 16:24:00 +0000
> > +++ curtin/block/__init__.py 2016-06-09 21:47:02 +0000
> > @@ -356,6 +356,7 @@
> > cmd.append('--replace-whitespace')
> > try:
> > (out, err) = util.subp(cmd, capture=True)
> > + LOG.debug("scsi_id output raw:\n%s", out)
>
> Since it is debug level anyway I'd think it would be nice to report the "err"
> as well. I mean there could be err output even without
> util.ProcessExecutionError in some cases - and especially those are the ones we
> will read the debug level info.
>

> > scsi_wwid = out.rstrip('\n')
> > return scsi_wwid
> > except util.ProcessExecutionError as e:
> >
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py 2016-04-22 16:24:00 +0000
> > +++ curtin/commands/block_meta.py 2016-06-09 21:47:02 +0000
> > @@ -379,9 +379,14 @@
> > if vol.get('serial'):
> > volume_path = block.lookup_disk(vol.get('serial'))
> > elif vol.get('path'):
> > - volume_path = vol.get('path')
> > + # resolve any symlinks to the dev_kname so sys/class/block access
> > + # is valid. ie, there are no udev generated values in sysfs
> > + volume_path = os.path.realpath(vol.get('path'))
>
> I'm not sure so this is honestly just a question - could there be cases
> where os.path.realpath would throw some kind of OSError? Like on broken
> links or anything - and if so should we guard us against it?
> (same 4 lines below)

I'm sure os.path.realpath() throw and exception, however, I don't think there
is anything that we can do about that and still recover. That is, if
we fail to find the realpath to the device, we cannot fix that, and we'd exit.

Without catching the exception, we get the same semantics by allowing the
error to propigate upward.

> > === modified file 'curtin/commands/curthooks.py'
> > --- curtin/commands/curthooks.py 2016-05-17 21:14:36 +0000
> > +++ curtin/commands/curthooks.py 2016-06-09 21:47:02 +0000
> > @@ -638,6 +638,20 @@
> > LOG.info("Detected multipath devices. Installing support via %s", mppkgs)
> >
> > util.install_packages(mppkgs, target=target)
> > + replace_spaces = True
> > + try:
> > + # check in-target version
> > + pkg_ver = util.get_package_version('multipath-tools', target)
> > + upstream = pkg_ver.split('-')[0]
> > + major, minor, micro = upstream.split(".", 2)
> > + val = 1000 * int(major) + 100 * int(minor)
>
> Maybe moving the splitting into util as well (or an extra .py).
> This is very format depending, but I'd think there are only three or four kinds in Ubunutu.
> It would be nice for re-usability if you would define that as util for this format.
> Whoever needs another ...

> Hi Ryan,
> nice work on multipath testing!
> 
> I have found nothing severe but a collection of things that could be more clear, more polished or just were unclear at least to me.
>

Thanks for the review Christian.  Picking up most of your suggestions.
A few things will need to put off for now.

> > === modified file 'curtin/block/__init__.py'
> > --- curtin/block/__init__.py  2016-04-22 16:24:00 +0000
> > +++ curtin/block/__init__.py  2016-06-09 21:47:02 +0000
> > @@ -356,6 +356,7 @@
> >          cmd.append('--replace-whitespace')
> >      try:
> >          (out, err) = util.subp(cmd, capture=True)
> > +        LOG.debug("scsi_id output raw:\n%s", out)
> 
> Since it is debug level anyway I'd think it would be nice to report the "err"
> as well.  I mean there could be err output even without
> util.ProcessExecutionError in some cases - and especially those are the ones we
> will read the debug level info.
>

> >          scsi_wwid = out.rstrip('\n')
> >          return scsi_wwid
> >      except util.ProcessExecutionError as e:
> >
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py     2016-04-22 16:24:00 +0000
> > +++ curtin/commands/block_meta.py     2016-06-09 21:47:02 +0000
> > @@ -379,9 +379,14 @@
> >          if vol.get('serial'):
> >              volume_path = block.lookup_disk(vol.get('serial'))
> >          elif vol.get('path'):
> > -            volume_path = vol.get('path')
> > +            # resolve any symlinks to the dev_kname so sys/class/block access
> > +            # is valid.  ie, there are no udev generated values in sysfs
> > +            volume_path = os.path.realpath(vol.get('path'))
> 
> I'm not sure so this is honestly just a question - could there be cases
> where os.path.realpath would throw some kind of OSError?  Like on broken
> links or anything - and if so should we guard us against it?
> (same 4 lines below)

I'm sure os.path.realpath() throw and exception, however, I don't think there
is anything that we can do about that and still recover.  That is, if 
we fail to find the realpath to the device, we cannot fix that, and we'd exit.

Without catching the exception, we get the same semantics by allowing the
error to propigate upward.

> > === modified file 'curtin/commands/curthooks.py'
> > --- curtin/commands/curthooks.py      2016-05-17 21:14:36 +0000
> > +++ curtin/commands/curthooks.py      2016-06-09 21:47:02 +0000
> > @@ -638,6 +638,20 @@
> >      LOG.info("Detected multipath devices. Installing support via %s", mppkgs)
> >
> >      util.install_packages(mppkgs, target=target)
> > +    replace_spaces = True
> > +    try:
> > +        # check in-target version
> > +        pkg_ver = util.get_package_version('multipath-tools', target)
> > +        upstream = pkg_ver.split('-')[0]
> > +        major, minor, micro = upstream.split(".", 2)
> > +        val = 1000 * int(major) + 100 * int(minor)
> 
> Maybe moving the splitting into util as well (or an extra .py).
> This is very format depending, but I'd think there are only three or four kinds in Ubunutu.
> It would be nice for re-usability if you would define that as util for this format.
> Whoever needs another can add it, whoever needs the same can use your function.
>

That's reasonable.  I was actually thinking about collapsing the parsing of
the version inside the get_package_version() method and returning a dict,
similar to lsb_release.  Callers can then pick out the various fields they
want to compare.

> > +        LOG.debug("multipath version is val=%s major=%s minor=%s micro=%s",
> 
> You don't care about micro (also I see it contains all kind of non ordered
> stuff like git id) in the calculation.  If ignored anyway maybe dropping
> it from the log report and making the assignment a "_" to go by pep8 and such?

Micro was used for logging detail, and while it didn't matter in the decision
it's still useful for logging what package version was found.

> > +                  val, major, minor, micro)
> > +        if val < 500:
> 
> It seems to be before multipath 0.5 right - a comment for the human readable
> version this is guarding would be nice

ACK

> > === modified file 'tests/vmtests/__init__.py'
> > --- tests/vmtests/__init__.py 2016-05-18 01:08:46 +0000
> > +++ tests/vmtests/__init__.py 2016-06-09 21:47:02 +0000
> > @@ -438,21 +441,51 @@
> >              netdevs.extend(["--netdev=" + DEFAULT_BRIDGE])
> >
> >          # build disk arguments
> > -        # --disk source:size:driver:block_size
> > -        extra_disks = []
> > +        disks = []
> > +        sc = util.load_file(cls.conf_file)
> > +        storage_config = yaml.load(sc)['storage']['config']
> > +        cls.wwns = ["wwn=%s" % x.get('wwn') for x in storage_config
> 
> IM-very-HO wwns/serials or disk_serials/disk_wwns
> Same prefix for both might make it slightly more readable

ACK

> > +                    if 'wwn' in x]
> > +        cls.disk_serials = ["serial=%s" % x.get('serial')
> > +                            for x in storage_config if 'serial' in x]
> > +
> > +        target_disk = "{}:{}:{}:{}:".format(cls.td.target_disk,
> > +                                            "",
> > +                                            cls.disk_driver,
> > +                                            cls.disk_block_size)
> > +        if len(cls.wwns):
> > +            target_disk += cls.wwns[0]
> > +
> > +        if len(cls.disk_serials):
> > +            target_disk += cls.disk_serials[0]
> > +
> > +        disks.extend(['--disk', target_disk])
> > +
> > +        # --disk source:size:driver:block_size:devopts
> >          for (disk_no, disk_sz) in enumerate(cls.extra_disks):
> >              dpath = os.path.join(cls.td.disks, 'extra_disk_%d.img' % disk_no)
> > -            extra_disks.extend(
> > -                ['--disk', '{}:{}:{}:{}'.format(dpath, disk_sz, "",
> > -                                                cls.disk_block_size)])
> > +            extra_disk = '{}:{}:{}:{}:'.format(dpath, disk_sz,
> > +                                               cls.disk_driver,
> > +                                               cls.disk_block_size)
> > +            if len(cls.wwns):
> > +                w_index = disk_no + 1
> > +                if w_index < len(cls.wwns):
> > +                    extra_disk += cls.wwns[w_index]
> > +
> 
> These two sections look so similar, I failed to come up with a good suggestion
> but it feels they could be united more.  If you have spare 5 minutes you might
> give it a thought.

I don't want to boil the ocean here yet;  In general, I'd really like to have
vmtest construct it's parameters purely from examining the conf_file yaml
directly.  This would greatly simplify creation of new tests as well as making
it much simpler to import a config from a bug and reproducing errors.  However
it may require in some cases, like nvme, adding additional information into
the yaml structure to provide additional details: disk driver, total disk size
are two examples.

I'm not ready for that refactor at the moment.

> > -        # --disk source:size:driver:block_size
> > -        target_disk = "{}:{}:{}:{}".format(cls.td.target_disk, "", "",
> > -                                           cls.disk_block_size)
> > -        cmd.extend(netdevs + ["--disk", target_disk] +
> > -                   extra_disks + nvme_disks +
> > +        if cls.multipath:
> > +            disks = disks * 2
> 
> Do you think it would make sense to make this a parameter maybe from
> cls.multipath.num_parallel or so?  Effectively what you did is already so
> great, I don't see why it has to be always 2.

Yeah, I can do that.

> That way you could do some sort of limit testing by setting it to
> (48*2)*(96*2) which would be the system z max.  Not really that number :-),
> but driving the test with 2,16,64 could be a great check!

Yep.

> 
> General question: Is there a limit on qemu (commandline) that we have to be
> scared of?

I don't think the command line processing will have a limit; though that's
certainly possible; in the past there was a one 4K page limit to hold all of
the commandline parameters; I don't know if that's still the case.

It's more likely we'd hit qemu device limits (like pci slots for virtio) or
others.  That's out-of-scope for vmtest however interesting it may be.

> >          target_disks = []
> > -        for (disk_no, disk) in enumerate([cls.td.target_disk,
> > -                                          cls.td.output_disk]):
> > -            d = '--disk={},driver={},format={},{}'.format(disk, disk_driver,
> > +        for (disk_no, disk) in enumerate([cls.td.target_disk]):
> > +            d = '--disk={},driver={},format={},{}'.format(disk,
> > +                                                          cls.disk_driver,
> >                                                            TARGET_IMAGE_FORMAT,
> >                                                            bsize_args)
> > +            if len(cls.wwns):
> > +                d += ",%s" % cls.wwns[0]
> > +            if len(cls.disk_serials):
> > +                d += ",%s" % cls.disk_serials[0]
> > +
> >              target_disks.extend([d])
> >
> >          extra_disks = []
> >          for (disk_no, disk_sz) in enumerate(cls.extra_disks):
> >              dpath = os.path.join(cls.td.disks, 'extra_disk_%d.img' % disk_no)
> > -            d = '--disk={},driver={},format={},{}'.format(dpath, disk_driver,
> > +            d = '--disk={},driver={},format={},{}'.format(dpath,
> > +                                                          cls.disk_driver,
> >                                                            TARGET_IMAGE_FORMAT,
> >                                                            bsize_args)
> > +            if len(cls.wwns):
> > +                w_index = disk_no + 1
> > +                if w_index < len(cls.wwns):
> > +                    d += ",%s" % cls.wwns[w_index]
> 
> a bit more than just "d" maybe?
> Even some of my checkers highlight that, so it creates noise distracting from
> real issues.

ACK.

> > +
> > +            if len(cls.disk_serials):
> > +                w_index = disk_no + 1
> > +                if w_index < len(cls.disk_serials):
> > +                    d += ",%s" % cls.disk_serials[w_index]
> > +
> 
> Aren't those two if sections the same as above - maybe even more reasonable to
> think about simplifying or reusing them in a method.

launch and xkvm command building is similar but slightly different due to the
varying syntax between the two commands.  In this case, it matches but it 
may not always.

The general abstraction is that for a given vmtest class, we *may* include 
disk serial or disk wwn values.  If they're present, we'd like to inject that
into the disk configuration.  I don't think there is much else to do here
without the larger refactoring of conf_file conversion to launch/xkvm syntax.

> > 
> >              extra_disks.extend([d])
> >
> >          nvme_disks = []
> > @@ -561,6 +605,18 @@
> >                                                            bsize_args)
> >              nvme_disks.extend([d])
> >
> > +        if cls.multipath:
> > +            target_disks = target_disks * 2
> > +            extra_disks = extra_disks * 2
> > +            nvme_disks = nvme_disks * 2
> 
> As before I'd love to see a dynamic multiplier.
> OTOH we might want to get it working with *2 before doing that step - up to you.

ACK 
It works with two; I've not tested with more than that;  that may end up being
more of a multipath test than a
"curtin-detect-and-configure-multipath-and-still-boot" test that we want.

> +        # output disk is always virtio-blk, with serial of output_disk.img
> +        output_disk = '--disk={},driver={},format={},{},{}'.format(
> +            cls.td.output_disk, 'virtio-blk',
> +            TARGET_IMAGE_FORMAT, bsize_args,
> +            'serial=%s' % cls.td.output_disk)

Here's the critical error that broke most of the tests.  We end up
passing the serial as a full path (/home/$user/foo/bar/wark.img).
That's unfortunately too long and the collectd script couldn't find
the disk image.

It was meant to be os.path.basename(cls.td.output_disk) which
results in: serial=output_disk.img

Which gives us:

/dev/disk/by-id/virtio-output_disk.img

Fixing this passes all by 23 tests; those are networking
tests which don't use storage config, hence the code
above isn't check if the conf_file includes a storage config.

Fixing that now.

> > === modified file 'tests/vmtests/test_basic.py'
> > --- tests/vmtests/test_basic.py       2016-05-17 20:55:46 +0000
> > +++ tests/vmtests/test_basic.py       2016-06-09 21:47:02 +0000
> > @@ -233,3 +233,120 @@
> >
> >  class XenialTestBasic(relbase.xenial, TestBasicAbs):
> >      __test__ = True
> > +
> > +
> > +class TestBasicScsiAbs(TestBasicAbs):
> > +    conf_file = "examples/tests/basic_scsi.yaml"
> > +    disk_driver = 'scsi-hd'
> > +    extra_disks = ['128G', '128G', '4G']
> > +    nvme_disks = ['4G']
> > +    collect_scripts = [textwrap.dedent("""
> > +        cd OUTPUT_COLLECT_D
> > +        blkid -o export /dev/sda > blkid_output_sda
> > +        blkid -o export /dev/sda1 > blkid_output_sda1
> > +        blkid -o export /dev/sda2 > blkid_output_sda2
> > +        btrfs-show-super /dev/sdc > btrfs_show_super_sdc
> > +        cat /proc/partitions > proc_partitions
> > +        ls -al /dev/disk/by-uuid/ > ls_uuid
> > +        ls -al /dev/disk/by-id/ > ls_disk_id
> > +        cat /etc/fstab > fstab
> > +        mkdir -p /dev/disk/by-dname
> > +        ls /dev/disk/by-dname/ > ls_dname
> > +        find /etc/network/interfaces.d > find_interfacesd
> > +
> > +        v=""
> 
> A bit more than just "v" would be nice

Heh, that's cut-n-paste from the existing Basic class test.
I'll defend the use of single char variables in a shell script
since we all know programmers are lazy typers.

> > +    def test_whole_disk_format(self):
> > +        # confirm the whole disk format is the expected device
> > +        with open(os.path.join(self.td.collect,
> > +                  "btrfs_show_super_sdc"), "r") as fp:
> > +            btrfs_show_super = fp.read()
> > +
> > +        with open(os.path.join(self.td.collect, "ls_uuid"), "r") as fp:
> > +            ls_uuid = fp.read()
> > +
> > +        # extract uuid from btrfs superblock
> > +        btrfs_fsid = [line for line in btrfs_show_super.split('\n')
> > +                      if line.startswith('fsid\t\t')]
> > +        self.assertEqual(len(btrfs_fsid), 1)
> > +        btrfs_uuid = btrfs_fsid[0].split()[1]
> > +        self.assertTrue(btrfs_uuid is not None)
> > +
> > +        # extract uuid from /dev/disk/by-uuid on /dev/sdc
> > +        # parsing ls -al output on /dev/disk/by-uuid:
> > +        # lrwxrwxrwx 1 root root   9 Dec  4 20:02
> > +        #  d591e9e9-825a-4f0a-b280-3bfaf470b83c -> ../../vdg
> > +        uuid = [line.split()[8] for line in ls_uuid.split('\n')
> > +                if 'sdc' in line]
> > +        self.assertEqual(len(uuid), 1)
> > +        uuid = uuid.pop()
> > +        self.assertTrue(uuid is not None)
> 
> I don't know if we expose that in the storage yaml, but if so it would make a
> nice test (probably on an extra disk) force a uuid via mkfs.btrfs --uuid and
> check for that known uuid as well?

We do, the type: format structure allows users to specify the uuid value.

http://people.canonical.com/~rharper/curtin/topics/storage.html#format-command

I don't recall if we're current exercising that, so that's a good TODO.

> > +
> > +        # compare them
> > +        self.assertEqual(uuid, btrfs_uuid)
> > +
> > +
> > +class XenialTestScsiBasic(relbase.xenial, TestBasicScsiAbs):
> > +    __test__ = True
> >
> > === modified file 'tools/launch'
> > --- tools/launch      2016-04-08 03:14:21 +0000
> > +++ tools/launch      2016-06-09 21:47:02 +0000
> > @@ -280,7 +285,7 @@
> >          case "$cur" in
> >                 --add) addfiles[${#addfiles[@]}]="$next"; shift;;
> >              -a|--append) uappend="$next"; shift;;
> > -            -A|--arch) arch_hint="$next"; shift;;
> > +            -A|--arch) arch_hint="$next"; shift;;
> 
> You changed that from a tab and 7 trailing spaces to just 7 trailing spaces.
> Any reason to keep the spaces?

Yes, tabs are evil.

> 
> BTW - there is much more whitespace damage around that we could fix if we want.

I know; it's unrelated; I hate that I missed the tabs when I merged the s390x
branch.

> >              -d|--disk) disks[${#disks[@]}]="$next"; shift;;
> >              -h|--help) Usage ; exit 0;;
> >              -i|--initrd) initrd="$next"; shift;;
> > @@ -625,6 +640,11 @@
> >      return $ret
> >  }
> >
> > +random_wwn() {
> 
> I wanted to ask for collision detection and started to look around.
> It seems this function isn't used anymore right?

There wasn't any detection; it would be needed for that 0.000000001 % chance
that 4 random 15-bit integers would repeat themselves in the same way.

I've dropped the function for now; In general, I'd like to always provide
a serial# for all disks, if one is not provided.  And for scsi_hd, I wanted
to provide wwn if not provided.  However, I ended up passing those values in
for the test-cases that needed them (namely multipath and basic_scsi) for now.

lp://staging/~raharper/curtin/trunk.vmtest-multipath updated on 2016-06-10

393. By Ryan Harper on 2016-06-10: Only use the filename of the output disk, not the whole path.
394. By Ryan Harper on 2016-06-10: Add error output along side scsi_id debug output.
395. By Ryan Harper on 2016-06-10: Not all vmtests use storage config, fail gracefully
396. By Ryan Harper on 2016-06-10: Move version parsing into utility method, return version dictionary for callers
397. By Ryan Harper on 2016-06-10: Use disk_wwns to match disk_serials
398. By Ryan Harper on 2016-06-10: Replace hardcoded number of paths for multipath to class variable, cls.multipath_num_paths; default to 2
399. By Ryan Harper on 2016-06-10: Replace single character temp variable in disk parameter construction; update formatting.

Revision history for this message

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

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

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

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2016-06-24:

I approve of this branch.
We actually deployed a power8 system with multipath for xenial with it at revno 399.
I had conversation with Ryan in irc, and asked that we improve the get_package_version to do the multiplication of major/minor/micro centrally. something like this: http://paste.ubuntu.com/17806224/

i'd like that fixed, and then please merge. whoowho!

review: Approve

lp://staging/~raharper/curtin/trunk.vmtest-multipath updated on 2016-06-24

400. By Ryan Harper on 2016-06-23: merge from trunk
401. By Ryan Harper on 2016-06-24: Update get_package_version to move sematic_version calculation internally.
402. By Ryan Harper on 2016-06-24: fixes from diglett run