cloud-init

Code review comment for ~redriver/cloud-init:frbsd-azure-branch

Revision history for this message

Scott Moser (smoser) wrote on 2017-05-04:

On Thu, 4 May 2017, Hongjiang Zhang wrote:

> Hi Scott,
>
> I don't want to bother you again and again, but I really hope you can take 3~5 minutes to go through my patch, because it has taken at least 3 months on review since merge request was created.
>
> Your main concern is why I need Azure specific code in get_mount_info, and I have removed this dependence.
>
> Do you have any other concern?
>
> Thanks
> Hongjiang Zhang

Hongjiang,
I'm really sorry for being so unresponsive.
I really do not mind you bothering me and appreciate your persistence.

I grabbed [1] and made some changes. I think that was your latest.
I pushed that to [2].

Please take a look, and integrate my changes. Then, please propose that
branch and write a good commit message that describes all the fixes you've
made.

I'll give it another look tomorrow. We are really close.

Thank you.

[1] https://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
[2] https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Tuesday, May 2, 2017 10:26 AM
> To: 'Scott Moser' <email address hidden>; '<email address hidden>' <email address hidden>
> Cc: '<email address hidden>' <email address hidden>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> Hi Scott,
>
> I have removed the Azure specific check for "get_mount_info".
> get_mount_info_freebsd() was invoked only if the normal parse_mount failed on FreeBSD platform.
> What do you think?
>
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -565,6 +565,10 @@ def is_ipv4(instr):
> return len(toks) == 4
>
>
> +def is_FreeBSD():
> + return system_info()['platform'].startswith('FreeBSD')
> +
> +
> def get_cfg_option_bool(yobj, key, default=False):
> if key not in yobj:
> return default
> @@ -2055,11 +2059,56 @@ def parse_mtab(path):
> return None
>
>
> +def find_freebsd_part(label_part):
> + if label_part.startswith("/dev/label/"):
> + target_label = label_part[5:]
> + (label_part, err) = subp(['glabel', 'status', '-s'])
> + for labels in label_part.split("\n"):
> + items = labels.split()
> + if len(items) > 0 and items[0].startswith(target_label):
> + label_part = items[2]
> + break
> + label_part = str(label_part)
> + return label_part
> +
> +
> +def get_path_dev_freebsd(path, mnt_list):
> + path_found = None
> + for line in mnt_list.split("\n"):
> + items = line.split()
> + if (len(items) > 2 and os.path.exists(items[1] + path)):
> + path_found = line
> + break
> + return path_found
> +
> +
> +def get_mount_info_freebsd(path, log=LOG):
> + (result, err) = subp(['mount', '-p', path], rcs=[0, 1])
> + if len(err):
> + # find a path if the input is not a mounting point
> + (mnt_list, err) = subp(['mount', '-p'])
> + path_found = get_path_dev_freebsd(path, mnt_list)
> + if (path_found is None):
> + return None
> + result = path_found
> + ret = result.split()
> + label_part = find_freebsd_part(ret[0])
> + return "/dev/" + label_part, ret[2], ret[1]
> +
> +
> def parse_mount(path):
> (mountoutput, _err) = subp("mount")
> mount_locs = mountoutput.splitlines()
> for line in mount_locs:
> m = re.search(r'^(/dev/[\S]+) on (/.*) $(.+), .+, (.+)$$', line)
> + if not m:
> + continue
> + # check whether the dev refers to a label on FreeBSD
> + # for example, if dev is '/dev/label/rootfs', we should
> + # continue finding the real device like '/dev/da0'.
> + devm = re.search('^(/dev/.+)p([0-9])$', m.group(1))
> + if (not devm and is_FreeBSD()):
> + return get_mount_info_freebsd(path)
> devpth = m.group(1)
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Tuesday, April 25, 2017 11:38 AM
> To: 'Scott Moser' <email address hidden>
> Cc: '<email address hidden>' <email address hidden>; Kylie Liang <email address hidden>; Josh Poulson <email address hidden>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> Hello Scott,
>
> I'm not sure whether you are still confused about why we need Azure specific code, or is there anything I can do to help make any progress on this merge?
>
> Thanks
> Hongjiang Zhang
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Friday, April 14, 2017 10:58 AM
> To: Scott Moser <email address hidden>
> Cc: <email address hidden>; Kylie Liang <email address hidden>; Josh Poulson <email address hidden>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> The reason is FreeBSD on Azure have more than 1 disks attached, and the rootfs located in one disk, for example, /dev/da0, but I cannot directly write something below:
> /dev/da0 / ufs rw 1 1
> Because the rootfs may be switched to /dev/da1. The solution is label the disk. For FreeBSD on Azure, I labeled /dev/da0 with "rootfs", so for get_mount_info, I have to find "rootfs" first, and then find the corresponding /dev/da0.
>
> But on Linux, there is a /proc/$$/mountinfo to help find the device. For Azure, there is also /dev/disk/cloud/azure_root on Linux.
> Both make the get_mount_info easy to implement.
>
>
> -----Original Message-----
> From: Scott Moser [mailto:<email address hidden>] On Behalf Of Scott Moser
> Sent: Friday, April 14, 2017 10:46 AM
> To: Hongjiang Zhang <email address hidden>
> Cc: <email address hidden>; Kylie Liang <email address hidden>; Josh Poulson <email address hidden>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> On Fri, 14 Apr 2017, Hongjiang Zhang wrote:
>
> > Hi Scott,
> >
> > I have manually merged your modifications to my branch and fixed a unit test issue.
> > Do you think it is ok for merging?
>
> The only thing I had left was that I'm still confused as what why we need azure specific code in that code path.
>
> I have to think about it some more and probably go play with it a little on freebsd.
>
>
> >
> > Thanks
> > Hongjiang Zhang
> >
> >
> > -----Original Message-----
> > From: <email address hidden> [mailto:<email address hidden>] On Behalf
> > Of Scott Moser
> > Sent: Thursday, April 13, 2017 2:51 AM
> > To: <email address hidden>
> > Subject: Re: [Merge] ~redriver/cloud-init:frbsd-azure-branch into
> > cloud-init:master
> >
> > Other than that and I think it looks really good.
> > thank you for adding the unit tests.
> >
> > --
> > https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.la
> > unchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%
> > 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> > 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> > 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> > ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
> >
>
>

On Thu, 4 May 2017, Hongjiang Zhang wrote:

Hongjiang,
I'm really sorry for being so unresponsive.
I really do not mind you bothering me and appreciate your persistence.

I grabbed [1] and made some changes.  I think that was your latest.
I pushed that to [2].

Please take a look, and integrate my changes.  Then, please propose that
branch and write a good commit message that describes all the fixes you've
made.

I'll give it another look tomorrow.  We are really close.

Thank you.

[1] https://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
[2] https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Tuesday, May 2, 2017 10:26 AM
> To: 'Scott Moser' <smoser@ubuntu.com>; 'scott.moser@canonical.com' <scott.moser@canonical.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> Hi Scott,
>
> I have removed the Azure specific check for "get_mount_info".
> get_mount_info_freebsd() was invoked only if the normal parse_mount failed on FreeBSD platform.
> What do you think?
>
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -565,6 +565,10 @@ def is_ipv4(instr):
>      return len(toks) == 4
>
>
> +def is_FreeBSD():
> +    return system_info()['platform'].startswith('FreeBSD')
> +
> +
>  def get_cfg_option_bool(yobj, key, default=False):
>      if key not in yobj:
>          return default
> @@ -2055,11 +2059,56 @@ def parse_mtab(path):
>      return None
>
>
> +def find_freebsd_part(label_part):
> +    if label_part.startswith("/dev/label/"):
> +        target_label = label_part[5:]
> +        (label_part, err) = subp(['glabel', 'status', '-s'])
> +        for labels in label_part.split("\n"):
> +            items = labels.split()
> +            if len(items) > 0 and items[0].startswith(target_label):
> +                label_part = items[2]
> +                break
> +        label_part = str(label_part)
> +    return label_part
> +
> +
> +def get_path_dev_freebsd(path, mnt_list):
> +    path_found = None
> +    for line in mnt_list.split("\n"):
> +        items = line.split()
> +        if (len(items) > 2 and os.path.exists(items[1] + path)):
> +            path_found = line
> +            break
> +    return path_found
> +
> +
> +def get_mount_info_freebsd(path, log=LOG):
> +    (result, err) = subp(['mount', '-p', path], rcs=[0, 1])
> +    if len(err):
> +        # find a path if the input is not a mounting point
> +        (mnt_list, err) = subp(['mount', '-p'])
> +        path_found = get_path_dev_freebsd(path, mnt_list)
> +        if (path_found is None):
> +            return None
> +        result = path_found
> +    ret = result.split()
> +    label_part = find_freebsd_part(ret[0])
> +    return "/dev/" + label_part, ret[2], ret[1]
> +
> +
>  def parse_mount(path):
>      (mountoutput, _err) = subp("mount")
>      mount_locs = mountoutput.splitlines()
>      for line in mount_locs:
>          m = re.search(r'^(/dev/[\S]+) on (/.*) $(.+), .+, (.+)$$', line)
> +        if not m:
> +            continue
> +        # check whether the dev refers to a label on FreeBSD
> +        # for example, if dev is '/dev/label/rootfs', we should
> +        # continue finding the real device like '/dev/da0'.
> +        devm = re.search('^(/dev/.+)p([0-9])$', m.group(1))
> +        if (not devm and is_FreeBSD()):
> +            return get_mount_info_freebsd(path)
>          devpth = m.group(1)
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Tuesday, April 25, 2017 11:38 AM
> To: 'Scott Moser' <smoser@ubuntu.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> Hello Scott,
>
> I'm not sure whether you are still confused about why we need Azure specific code, or is there anything I can do to help make any progress on this merge?
>
> Thanks
> Hongjiang Zhang
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Friday, April 14, 2017 10:58 AM
> To: Scott Moser <smoser@ubuntu.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> The reason is FreeBSD on Azure have more than 1 disks attached, and the rootfs located in one disk, for example, /dev/da0, but I cannot directly write something below:
> /dev/da0	/	ufs	rw 1 1
> Because the rootfs may be switched to /dev/da1. The solution is label the disk. For FreeBSD on Azure, I labeled /dev/da0 with "rootfs", so for get_mount_info, I have to find "rootfs" first, and then find the corresponding /dev/da0.
>
> But on Linux, there is a /proc/$$/mountinfo to help find the device. For Azure, there is also /dev/disk/cloud/azure_root on Linux.
> Both make the get_mount_info easy to implement.
>
>
> -----Original Message-----
> From: Scott Moser [mailto:ssmoser2@gmail.com] On Behalf Of Scott Moser
> Sent: Friday, April 14, 2017 10:46 AM
> To: Hongjiang Zhang <honzhan@microsoft.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
> Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
>
> On Fri, 14 Apr 2017, Hongjiang Zhang wrote:
>
> > Hi Scott,
> >
> > I have manually merged your modifications to my branch and fixed a unit test issue.
> > Do you think it is ok for merging?
>
> The only thing I had left was that I'm still confused as what why we need azure specific code in that code path.
>
> I have to think about it some more and probably go play with it a little on freebsd.
>
>
> >
> > Thanks
> > Hongjiang Zhang
> >
> >
> > -----Original Message-----
> > From: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf
> > Of Scott Moser
> > Sent: Thursday, April 13, 2017 2:51 AM
> > To: mp+314895@code.launchpad.net
> > Subject: Re: [Merge] ~redriver/cloud-init:frbsd-azure-branch into
> > cloud-init:master
> >
> > Other than that and I think it looks really good.
> > thank you for adding the unit tests.
> >
> > --
> > https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.la
> > unchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%
> > 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> > 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> > 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> > ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
> >
>
>

« Back to merge proposal