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

Hongjiang Zhang (redriver) wrote :

Hi Scott,

The branch cl_on_Azure_0.7.9 was created recently and used to patch on freebsd ports: cloud-init-azure (see the ports code review: https://reviews.freebsd.org/D10566) since the merge request (314895) to head is still under review.

I hope you can give some comments about another merge request: https://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+merge/314895 This is the main branch I have been working on for almost 3 months.

Since I did not see the hope to complete that merge request of 314895, I have to create another branch: cl_on_Azure_0.7.9 for FreeBSD cloud-init ports.

Now, the cloud-init-azure ports is almost ready, but I still hope you can take some time on reviewing https://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+merge/314895

Thanks
Hongjiang Zhang

-----Original Message-----
From: Scott Moser [mailto:<email address hidden>] On Behalf Of Scott Moser
Sent: Thursday, May 4, 2017 11:56 AM
To: Hongjiang Zhang <email address hidden>
Cc: <email address hidden>; Josh Poulson <email address hidden>; Kylie Liang <email address hidden>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

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://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.launchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bref%2Fcl_on_Azure_0.7.9-fixups&data=02%7C01%7Chonzhan%40microsoft.com%7C8d3cb29db2d9482d62c208d492a172d7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636294669492915135&sdata=v%2FIQi1ZJnqAdMCN%2Bz4NTX1fjIIN9i9MItvrwYBJPhNI%3D&reserved=0
[2] https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.launchpad.net%2F~smoser%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bref%2Fcl_on_Azure_0.7.9-fixups&data=02%7C01%7Chonzhan%40microsoft.com%7C8d3cb29db2d9482d62c208d492a172d7%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636294669492915135&sdata=mw7O6ukLZl8%2FLXRlmCFQAqFTIa7L2VnOz8xky%2BtHNoA%3D&reserved=0
>
> -----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%2Bmerg
> > e%
> > 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e13
> > 06
> > 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C6362761983
> > 91
> > 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&rese
> > rv
> > ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
> >
>
>

« Back to merge proposal