Code review comment for lp:~raharper/curtin/trunk.implement_raid_preserve

On Mon, Dec 7, 2015 at 10:27 AM, Scott Moser <email address hidden> wrote:

> our block_meta has been what really *should* be curtin/block/__init__.py
> there are a lot of functions there that are general purpose, not
> specifically tied to 'block_meta'.
>
> just after having been looking through code on many different MP's
> recently, i've sen a lot of stuff in block_meta that i'd like in
> block/__init__.py.
>

That's good to know. I don't mind moving them out there.

>
>
> Diff comments:
>
> > === modified file 'curtin/commands/block_meta.py'
> > --- curtin/commands/block_meta.py 2015-11-18 18:12:54 +0000
> > +++ curtin/commands/block_meta.py 2015-12-07 15:22:18 +0000
> > @@ -438,6 +438,75 @@
> > return volume_path
> >
> >
> > +def check_raid_array(mdname, raidlevel, devices=[], spares=[]):
> > + LOG.debug('Checking mdadm array: '
> > + 'name={} raidlevel={} devices={} spares={}'.format(
> > + mdname, raidlevel, devices, spares))
> > +
> > + # query the target raid device
> > + md_devname = os.path.join('/dev', mdname)
>
> beware os.path.join("/dev/", "/tmp/foo") returns /tmp/foo.
>

I think we're more concerned about:

 os.path.join("/dev", "/tmp/foo")
'/tmp/foo'

> I'd like a general get_full_device_path_for_device_name(devname="sda")
> that woudl return /dev/sda or what not.
>

I think most of it is here (but expects the volume id)

def get_path_to_storage_volume()

Instead you want it to take what? kernel devnames? /dev/XXXX, or XXX?
Is this just a smarter os.path.join for devices?

>
> > + (out, _err) = util.subp(["mdadm", "--query", "--detail", "--export",
>
> this next block of code should be in a function. mdadm_query_devname()
>

OK

>
> > + md_devname], capture=True)
> > + if _err:
> > + # this is not fatal as we may need to assemble the array
> > + LOG.warn("raid device '%s' does not exist" % md_devname)
> > + return False
> > +
> > + # Convert mdadm --query --detail --export key=value into dictionary
> > + md_query_data = {k: v for k, v in (x.split('=')
>
> I'm guessing you want to use shlex here.
> see curtin/block/__init__.py's blkid() which returns a dictionary and
> parses output using shlex.
>

Hrm, do you want me to make a general:

def subp_to_dict(cmd)

?

And have blkid use that? the K=V logic is currently embedded in there.

>
> > + for x in out.strip().split('\n'))}
> > +
> > + # confirm we have /dev/{mdname} by following the udev symlink
> > + mduuid_path = ('/dev/disk/by-id/md-uuid-' +
> > + '{}'.format(md_query_data['MD_UUID']))
> > + mdquery_devname = os.path.realpath(mduuid_path)
> > + if md_devname != mdquery_devname:
>
> this is a good check thank.s
>

NP, we should probably add that to the vmtests raid check as well.

>
> > + raise ValueError("Couldn't find correct raid device."
>
> need a ' ' after the '.' there or your value eror shows
> "device.MD_UUID={}".
>
>
OK

> > + "MD_UUID={} points to {} but expected {}" % (
> > + md_query_data['MD_UUID'],
> > + mdquery_devname,
> > + md_devname))
> > +
> > + def _compare_devlist(expected, found):
> > + LOG.debug('comparing device lists: '
> > + 'expected: {} found: {}'.format(expected, found))
> > + expected = set(expected)
> > + found = set(found)
> > + if expected != found:
> > + missing = expected.difference(found)
> > + extra = found.difference(expected)
> > + raise ValueError("RAID array device list does not match."
> > + " Missing: {} Extra: {}".format(missing,
> extra))
> > +
> > + # confirm spares, if any
> > + mdquery_spares = ['/dev/' + re.findall('MD_DEVICE_(.*)_ROLE',
> k).pop()
> > + for k, v in md_query_data.items()
> > + if v == 'spare']
> > + LOG.debug('mdadm spares found: {}'.format(mdquery_spares))
> > + if spares:
> > + _compare_devlist(spares, mdquery_spares)
> > +
> > + # confirm devices
> > + mdquery_devices = [v for (k, v) in md_query_data.items()
> > + if k.endswith('_DEV') and v not in
> mdquery_spares]
> > + LOG.debug('mdadm devices found: {}'.format(mdquery_devices))
> > + _compare_devlist(devices, mdquery_devices)
> > +
> > + # confirm raidlevel
> > + def _rl(raidlevel):
> > + return str(raidlevel).lower().replace('raid', '')
> > +
> > + raid_level_message = (
> > + " Expected: {} Found: {}".format(raidlevel,
> > + md_query_data['MD_LEVEL']))
> > + LOG.debug(raid_level_message)
> > + if _rl(raidlevel) != _rl(md_query_data['MD_LEVEL']):
> > + raise ValueError("Invalid RAID level:" + raid_level_message)
> > +
> > + LOG.debug('mdadm array OK: {}'.format(mdname))
> > + return True
> > +
> > +
> > def disk_handler(info, storage_config):
> > ptable = info.get('ptable')
> >
> > @@ -913,6 +983,28 @@
> > if mdname:
> > mdnameparm = "--name=%s" % info.get('mdname')
> >
> > + # Handle preserve flag
> > + if info.get('preserve'):
> > + # check if the array is already up, if not try to assemble
> > + if not check_raid_array(info.get('name'), raidlevel,
> > + device_paths, spare_device_paths):
> > + LOG.info("assembling preserved raid for "
>
> this maybe is a function 'assemble_raid(devnames=[]) ?
>

Yeah, we have create and assemble and query now. All could go into
block/__init__.py

>
> > + "{}".format(info.get('name')))
> > + cmd = ["mdadm", "--assemble", "/dev/%s" % info.get('name'),
> > + "--run"] + device_paths
> > + if spare_devices:
> > + cmd += spare_device_paths
> > + util.subp(" ".join(cmd), shell=True)
> > + util.subp(["udevadm", "settle"])
> > +
> > + # try again after attempting to assemble
> > + if not check_raid_array(info.get('name'), raidlevel,
> > + devices, spare_devices):
> > + raise ValueError("Unable to confirm preserved raid
> array: "
> > + " {}".format(info.get('name')))
> > + # raid is all OK
> > + return
> > +
> > cmd = ["mdadm", "--create", "/dev/%s" % info.get('name'), "--run",
> > "--level=%s" % raidlevel, "--raid-devices=%s" %
> len(device_paths),
> > mdnameparm]
>
>
> --
>
> https://code.launchpad.net/~raharper/curtin/trunk.implement_raid_preserve/+merge/279603
> You are the owner of lp:~raharper/curtin/trunk.implement_raid_preserve.
>