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

Hongjiang Zhang (redriver) wrote :

> There is a lot of good stuff here, thank you for your work and your patience.
> Some good things:
> - the generate_fallback_config() is good, thanks.
> - the unit tests added for that and for the resize.
> There are some things that still need fixing though, and I have some in-line
> comments to help elaborate.
> a.) You've fixed some general things inside of the DataSourceAzureNet only.
> Ie, you made devent2deev run through the datasource, which means
> that that code wont work running freebsd on Ec2. Better to have
> logic in it that does the right thing both places.
When I change devent2dev, I have considered other platforms. I have put a generic implementation of get_mount_info in sources/ So, for other platform, for example, EC2, it will invoke the util.get_mount_info. See:
    def get_mount_info(self, path, log=LOG):
        return util.get_mount_info(path, log)

Why this function cannot be put to util?
FreeBSD on Azure uses glabel to label /dev/da0p2 with 'rootfs', so if we want to find '/' partition, we should first find /dev/label/rootfs through '/', and then map it to /dev/da0p2.

> b.) I think we can simplify the '_can_skip_resize_*' things a bit.:w
Yes, I have changed it per your suggestion.

> c.) is your '_can_skip_resize_ufs' for performance? or is it actually needed?
It is used to check whether resize is necessary. On Azure, the partition has already fully covered the whole disk. It will report error in log if we resize it by force. This pre-check wants to avoid such error.

« Back to merge proposal