curtin

Code review comment for lp:~raharper/curtin/trunk.debug-mdadm

trunk.debug-mdadm
Merge into trunk

Revision history for this message

Ryan Harper (raharper) wrote on 2017-01-30:

Thanks for the review. Comments inline, but mostly ACKs for me to fix.

On Mon, Jan 30, 2017 at 12:32 PM, Scott Moser <email address hidden> wrote:

> I think this looks good.
> there are some small requests for enhancement inline.
>
> Diff comments:
>
> > === modified file 'curtin/block/__init__.py'
> > --- curtin/block/__init__.py 2016-11-18 15:12:19 +0000
> > +++ curtin/block/__init__.py 2017-01-27 17:22:00 +0000
> > @@ -742,7 +754,17 @@
> > LOG.debug("%s is %s bytes. wiping with buflen=%s",
> > path, size, buflen)
> >
> > - with open(path, "rb+") as fp:
> > + try:
> > + fd = os.open(path, os.O_RDWR | os.O_EXCL)
> > + except OSError:
> > + LOG.exception("Failed to exclusively open device")
>
> which device ? I dont think we even print the path to the device at all
> here.
>

It ends up in the exception stacktrace, however, we should be explicit in
the message.
ACK

>
> > + holders = get_holders(path)
> > + LOG.error('Device holders with exclusive access: %s', holders)
> > + mount_points = util.device_is_mounted(path)
> > + LOG.error('Device mounts: %s', mount_points)
> > + raise
> > +
> > + with os.fdopen(fd, "rb+") as fp:
> > while True:
> > pbuf = readfunc(buflen)
> > pos = fp.tell()
>
> i think this is sane, and gets 'fd' above closed except in the case where
> this open fails.
> i think if os.fdopen(fd) fails, then fd will not ever get a close() on it.
> Not that it should fail, but
> " If fdopen() raises an exception, it leaves fd untouched (unclosed)"
>

Lemme look at this.

>
> > @@ -772,11 +794,18 @@
> > if not (is_block or os.path.isfile(path)):
> > raise ValueError("%s: not an existing file or block device",
> path)
> >
> > + pt_names = []
> > if partitions and is_block:
> > ptdata = sysfs_partition_data(path)
> > for kname, ptnum, start, size in ptdata:
> > - offsets.append(start)
> > - offsets.append(start + size - zero_size)
> > + pt_names.append(dev_path(kname))
> > + # offsets.append(start)
> > + # offsets.append(start + size - zero_size)
> > + pt_names.reverse()
> > +
> > + for pt in pt_names:
> > + LOG.debug('Wiping partition: %s', pt)
>
> give the device path to be cleaar:
> Wiping partitition %s on %s, pt, path
>

I think that's implicit in the pt name since it's comprised of
dev_path(kname)
Ie, it's going to be items like '/dev/sda3', '/dev/nvme0n1p1'

>
> > + quick_zero(pt, partitions=False)
> >
> > LOG.debug("wiping 1M on %s at offsets %s", path, offsets)
> > return zero_file_at_offsets(path, offsets, buflen=buflen,
> count=count)
> > @@ -796,7 +825,19 @@
> > buf = b'\0' * buflen
> > tot = buflen * count
> > msg_vals = {'path': path, 'tot': buflen * count}
> > - with open(path, "rb+") as fp:
> > +
> > + # ensure we're the only open fd on the device
> > + try:
> > + fd = os.open(path, os.O_RDWR | os.O_EXCL)
> > + except OSError:
> > + LOG.exception("Failed to exclusively open device")
>
> which device ? perhaps this fail path could be a function? and reused here
> and above.
>

Yeah, a util.device_exclusive_open() or whatever.

ACK, will examine

> > fp.seek(0, 2)
> > size = fp.tell()
> >
> > === modified file 'curtin/block/clear_holders.py'
> > --- curtin/block/clear_holders.py 2016-08-30 21:56:13 +0000
> > +++ curtin/block/clear_holders.py 2017-01-27 17:22:00 +0000
> > @@ -62,6 +63,25 @@
> > return os.path.realpath(os.path.join(sysfs_path, 'bcache',
> 'cache'))
> >
> >
> > +def umount_device(device):
> > + """
> > + Attempt to unmount a device and any other devices which may be
> > + mounted on top of the devices mountpoint
> > + """
> > + proc_mounts = block.get_proc_mounts()
> > + mount_entries = [entry for entry in proc_mounts if device in entry]
>
> I think its more strictly correct to do:
> mount_entries = [entry for entry in proc_mounts if device == entry[0]]
>

ACK

>
> > + if len(mount_entries) > 0:
> > + for me in mount_entries:
>
> i think you probably need to walk this reversed.
> for me in reversed(mount_entries)
>

I've not found we have multiple mounts per device; but that is an edge case;
I don;t think we have any knowledge as to the order of unmounting them;

I think you're thinking of submounts, which are dirs which are mounted on
top of this
mount entry; below, you can see those mounts are sorted in reverse.

>
> > + LOG.debug('Device is mounted: %s', me)
> > + mount_point = me[1]
> > + submounts = [entry for entry in proc_mounts
> > + if mount_point in entry[1]]
> > + umount_list = sorted(submounts, key=lambda x: x[1],
> reverse=True)
>
> i dont know that this sorted is right... i am pretty sure that
> /proc/mounts is in the order of things done. so walking it backwards is
> right. i think you're sorting on the path which i dont think is right.
>

The top mount is finding if a specific device is mounted or not; that may
be the only mount we have to remove.
The submounts are any mounts that contain 'mount_point' as substring
element of the mount point, so '/tmp/tmpxxxx/target/dev' for example

It's only the submounts that we need to umount first.

>
> i've used 'umount_r' in mount-image-callback before, maybe it would be
> nice to just have that function here.
>

OK, I can look at that.

>
> umount_r(mountpoint):
> # unmount mountpoint and anything under it.
> for i in reversed(block.get_proc_mounts()):
> mp = i[1]
> if mp == mountpoint or mp.startswith(mountpoint + "/"):
> do_umount(mp)
>
> possibly we can even just add a recursive flag to do_umount()
>

do_umount takes mountpoints, in my case, I was dealing with device names
that happened to be mounted
somewhere. I still need a device_mountpoints() method which takes a device
and returns where it's mounted (and any submounts)

>
> > + for mp in [m[1] for m in umount_list]:
> > + LOG.debug('Umounting submounts at: %s', mp)
> > + util.do_umount(mp)
> > +
> > +
> > def shutdown_bcache(device):
> > """
> > Shut down bcache for specified bcache device
> >
> > === modified file 'curtin/block/mdadm.py'
> > --- curtin/block/mdadm.py 2016-08-30 21:56:13 +0000
> > +++ curtin/block/mdadm.py 2017-01-27 17:22:00 +0000
> > @@ -241,14 +257,17 @@
> > assert_valid_devpath(devpath)
> >
> > LOG.info("mdadm stopping: %s" % devpath)
> > - util.subp(["mdadm", "--stop", devpath], rcs=[0, 1], capture=True)
> > + out, _err = util.subp(["mdadm", "--stop", devpath], capture=True)
> > + LOG.debug("mdadm stop:\n%s\n%s", out, _err)
>
> see below
>
> >
> >
> > def mdadm_remove(devpath):
> > assert_valid_devpath(devpath)
> >
> > LOG.info("mdadm removing: %s" % devpath)
> > - util.subp(["mdadm", "--remove", devpath], rcs=[0, 1], capture=True)
> > + out, _err = util.subp(["mdadm", "--remove", devpath],
>
> dont use _err if you then use it. using an '_' in the assignment from a
> return usually indicates you're not interested ind it, so here just use
> 'err'
>

ACK.

>
> > + rcs=[0], capture=True)
> > + LOG.debug("mdadm remove:\n%s\n%s", out, _err)
> >
> >
> > def mdadm_query_detail(md_devname, export=MDADM_USE_EXPORT):
> >
> > === modified file 'curtin/util.py'
> > --- curtin/util.py 2016-11-30 16:00:39 +0000
> > +++ curtin/util.py 2017-01-27 17:22:00 +0000
> > @@ -274,6 +274,18 @@
> > return False
> >
> >
> > +def device_is_mounted(device):
>
> name indicates it wil return a boolean, but it returns an array.
>
> Additionally, a device *could* be mounted more than once (bind's show up
> that way i think). Maybe change the name to load_device_mounts?
>

ACK

>
> > + # return mount entry if device is in /proc/mounts
> > + mounts = ""
> > + with open("/proc/mounts", "r") as fp:
> > + mounts = fp.read()
> > +
> > + for line in mounts.splitlines():
> > + if line.split()[0] == device:
> > + return line
> > + return []
> > +
> > +
> > def do_mount(src, target, opts=None):
> > # mount src at target with opts and return True
> > # if already mounted, return False
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.debug-
> mdadm/+merge/315805
> You are the owner of lp:~raharper/curtin/trunk.debug-mdadm.
>

Thanks for the review.  Comments inline, but mostly ACKs for me to fix.

On Mon, Jan 30, 2017 at 12:32 PM, Scott Moser <smoser@ubuntu.com> wrote:

> I think this looks good.
> there are some small requests for enhancement inline.
>
> Diff comments:
>
> > === modified file 'curtin/block/__init__.py'
> > --- curtin/block/__init__.py  2016-11-18 15:12:19 +0000
> > +++ curtin/block/__init__.py  2017-01-27 17:22:00 +0000
> > @@ -742,7 +754,17 @@
> >      LOG.debug("%s is %s bytes. wiping with buflen=%s",
> >                path, size, buflen)
> >
> > -    with open(path, "rb+") as fp:
> > +    try:
> > +        fd = os.open(path, os.O_RDWR | os.O_EXCL)
> > +    except OSError:
> > +        LOG.exception("Failed to exclusively open device")
>
> which device ? I dont think we even print the path to the device at all
> here.
>

It ends up in the exception stacktrace, however, we should be explicit in
the message.
ACK

>
> > +        holders = get_holders(path)
> > +        LOG.error('Device holders with exclusive access: %s', holders)
> > +        mount_points = util.device_is_mounted(path)
> > +        LOG.error('Device mounts: %s', mount_points)
> > +        raise
> > +
> > +    with os.fdopen(fd, "rb+") as fp:
> >          while True:
> >              pbuf = readfunc(buflen)
> >              pos = fp.tell()
>
> i think this is sane, and gets 'fd' above closed except in the case where
> this open fails.
> i think if os.fdopen(fd) fails, then fd will not ever get a close() on it.
> Not that it should fail, but
>   " If fdopen() raises an exception, it leaves fd untouched (unclosed)"
>

Lemme look at this.

>
> > @@ -772,11 +794,18 @@
> >      if not (is_block or os.path.isfile(path)):
> >          raise ValueError("%s: not an existing file or block device",
> path)
> >
> > +    pt_names = []
> >      if partitions and is_block:
> >          ptdata = sysfs_partition_data(path)
> >          for kname, ptnum, start, size in ptdata:
> > -            offsets.append(start)
> > -            offsets.append(start + size - zero_size)
> > +            pt_names.append(dev_path(kname))
> > +            # offsets.append(start)
> > +            # offsets.append(start + size - zero_size)
> > +        pt_names.reverse()
> > +
> > +    for pt in pt_names:
> > +        LOG.debug('Wiping partition: %s', pt)
>
> give the device path to be cleaar:
> Wiping partitition %s on %s, pt, path
>

I think that's implicit in the pt name since it's comprised of
dev_path(kname)
Ie, it's going to be items like '/dev/sda3', '/dev/nvme0n1p1'

>
> > +        quick_zero(pt, partitions=False)
> >
> >      LOG.debug("wiping 1M on %s at offsets %s", path, offsets)
> >      return zero_file_at_offsets(path, offsets, buflen=buflen,
> count=count)
> > @@ -796,7 +825,19 @@
> >      buf = b'\0' * buflen
> >      tot = buflen * count
> >      msg_vals = {'path': path, 'tot': buflen * count}
> > -    with open(path, "rb+") as fp:
> > +
> > +    # ensure we're the only open fd on the device
> > +    try:
> > +        fd = os.open(path, os.O_RDWR | os.O_EXCL)
> > +    except OSError:
> > +        LOG.exception("Failed to exclusively open device")
>
> which device ? perhaps this fail path could be a function? and reused here
> and above.
>

Yeah, a util.device_exclusive_open() or whatever.

ACK, will examine

> >          fp.seek(0, 2)
> >          size = fp.tell()
> >
> > === modified file 'curtin/block/clear_holders.py'
> > --- curtin/block/clear_holders.py     2016-08-30 21:56:13 +0000
> > +++ curtin/block/clear_holders.py     2017-01-27 17:22:00 +0000
> > @@ -62,6 +63,25 @@
> >      return os.path.realpath(os.path.join(sysfs_path, 'bcache',
> 'cache'))
> >
> >
> > +def umount_device(device):
> > +    """
> > +    Attempt to unmount a device and any other devices which may be
> > +    mounted on top of the devices mountpoint
> > +    """
> > +    proc_mounts = block.get_proc_mounts()
> > +    mount_entries = [entry for entry in proc_mounts if device in entry]
>
> I think its more strictly correct to do:
>  mount_entries = [entry for entry in proc_mounts if device == entry[0]]
>

ACK

>
> > +    if len(mount_entries) > 0:
> > +        for me in mount_entries:
>
> i think you probably need to walk this reversed.
> for me in reversed(mount_entries)
>

I've not found we have multiple mounts per device; but that is an edge case;
I don;t think we have any knowledge as to the order of unmounting them;

I think you're thinking of submounts, which are dirs which are mounted on
top of this
mount entry; below, you can see those mounts are sorted in reverse.

>
> > +            LOG.debug('Device is mounted: %s', me)
> > +            mount_point = me[1]
> > +            submounts = [entry for entry in proc_mounts
> > +                         if mount_point in entry[1]]
> > +            umount_list = sorted(submounts, key=lambda x: x[1],
> reverse=True)
>
> i dont know that this sorted is right... i am pretty sure that
> /proc/mounts is in the order of things done.  so walking it backwards is
> right.  i think you're sorting on the path which i dont think is right.
>

It's only the submounts that we need to umount first.

>
> i've used 'umount_r' in mount-image-callback before, maybe it would be
> nice to just have that function here.
>

OK, I can look at that.

>
> umount_r(mountpoint):
>    # unmount mountpoint and anything under it.
>    for i in reversed(block.get_proc_mounts()):
>       mp = i[1]
>       if mp == mountpoint or mp.startswith(mountpoint + "/"):
>          do_umount(mp)
>
> possibly we can even just add a recursive flag to do_umount()
>

do_umount takes mountpoints, in my case, I was dealing with device names
that happened to be mounted
somewhere.  I still need a device_mountpoints() method which takes a device
and returns where it's mounted (and any submounts)

>
> > +            for mp in [m[1] for m in umount_list]:
> > +                LOG.debug('Umounting submounts at: %s', mp)
> > +                util.do_umount(mp)
> > +
> > +
> >  def shutdown_bcache(device):
> >      """
> >      Shut down bcache for specified bcache device
> >
> > === modified file 'curtin/block/mdadm.py'
> > --- curtin/block/mdadm.py     2016-08-30 21:56:13 +0000
> > +++ curtin/block/mdadm.py     2017-01-27 17:22:00 +0000
> > @@ -241,14 +257,17 @@
> >      assert_valid_devpath(devpath)
> >
> >      LOG.info("mdadm stopping: %s" % devpath)
> > -    util.subp(["mdadm", "--stop", devpath], rcs=[0, 1], capture=True)
> > +    out, _err = util.subp(["mdadm", "--stop", devpath], capture=True)
> > +    LOG.debug("mdadm stop:\n%s\n%s", out, _err)
>
> see below
>
> >
> >
> >  def mdadm_remove(devpath):
> >      assert_valid_devpath(devpath)
> >
> >      LOG.info("mdadm removing: %s" % devpath)
> > -    util.subp(["mdadm", "--remove", devpath], rcs=[0, 1], capture=True)
> > +    out, _err = util.subp(["mdadm", "--remove", devpath],
>
> dont use _err if you then use it. using an '_' in the assignment from a
> return usually indicates you're not interested ind it, so here just use
> 'err'
>

ACK.

>
> > +                          rcs=[0], capture=True)
> > +    LOG.debug("mdadm remove:\n%s\n%s", out, _err)
> >
> >
> >  def mdadm_query_detail(md_devname, export=MDADM_USE_EXPORT):
> >
> > === modified file 'curtin/util.py'
> > --- curtin/util.py    2016-11-30 16:00:39 +0000
> > +++ curtin/util.py    2017-01-27 17:22:00 +0000
> > @@ -274,6 +274,18 @@
> >      return False
> >
> >
> > +def device_is_mounted(device):
>
> name indicates it wil return a boolean, but it returns an array.
>
> Additionally, a device *could* be mounted more than once (bind's show up
> that way i think). Maybe change the name to load_device_mounts?
>

ACK

>
> > +    # return mount entry if device is in /proc/mounts
> > +    mounts = ""
> > +    with open("/proc/mounts", "r") as fp:
> > +        mounts = fp.read()
> > +
> > +    for line in mounts.splitlines():
> > +        if line.split()[0] == device:
> > +            return line
> > +    return []
> > +
> > +
> >  def do_mount(src, target, opts=None):
> >      # mount src at target with opts and return True
> >      # if already mounted, return False
>
>
> --
> https://code.launchpad.net/~raharper/curtin/trunk.debug-
> mdadm/+merge/315805
> You are the owner of lp:~raharper/curtin/trunk.debug-mdadm.
>

« Back to merge proposal