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

Hongjiang Zhang (redriver) wrote :

Thanks for you careful review and valuable comments.

> Note, I really dont have a lot of experience on FreeBSD. I relied heavily
> on others who added freebsd support to tell me what made sense, and I'll do
> the same for you.
That is ok for me.

> Thank you for your efforts here... There are definitely some things we need
> to re-work to better support different "distros" throughout the code base.
> We'll see these same sorts of things as aixtools is working on getting better
> aix support in.
>
> One such example is your moving generate_fallback_config to the distro object.
I agree. Azure supports both Linux and FreeBSD, so for the same data source, there are two distros and different distros sometimes have different native commands. So, we need to consider how to support them in an elegant way.

> Overall, comments, much of the code needs some unit tests. We simply can't
> avoid breaking things inadvertently without them get_resource_disk_on_freebsd
> and the networking code specifically.
All right. I'll add some unit tests.

> Please bear with me... you have a lot of good things here, we just need
> to work out some details.
>
> - adding the specific config/cloud.cfg-freebsd-azure
> I'd much prefer we adjust the already freebsd specific cloud.cfg-freebsd.
>
> The changes you have there are:
> - add Azure to the datasource_list. I think its right to just put
> Azure before Openstack and Ec2 so we have the list of 4 sources.
Ok.
> - you disabled growpart and resizefs
> I wonder why these were not disabled by the original freebsd work.
> Can you explain that? If they just aren't going to work, then lets
> remove them from the list and update the docstring to mention lack
> of support on freebsd. If they are only broken on Azure, lets figure
> out why.
On Azure, FreeBSD image used a fixed size VHD, so gpart is not necessary. Anyway, there is some error reported in the log if I enabled it. For the same reason, I disabled resizefs on Azure. Well, if we decide to use the same cloud-init.cfg for FreeBSD. I have to fix those error message and enable growpart and resizefs.

> - default user changes
> - removing of the default 'name'. This should not be necessary as
> linux uses the datasource provided username on azure when its there.
> - changing of shell to csh: my understanding (per git log) is that tcsh
> is the default shell on freebsd, so using that makes more sense.
> if Azure wishes to carry a different default shell, they can do that
> in local config. Generally though, commonality is better.
> - removal of lock_passwd: azure can carry that locally if they want.
For unknown reason, if I did not remove the default 'name' on Azure, it will stop the data source provided username. Ok, that may be a bug on Azure. In addition, the default username 'freebsd' is created but locked passwd, then why do we need it?

>
> - resizefs: it seems like you made some changes for this
> in an attempt to make it work, but then disabled it in your
> config. If it isn't going to work, lets not make ancillary changes there.
> these include
> tests/unittests/test_handler/test_handler_growpart.py
> cloudinit/config/cc_resizefs.py
> get_mount_info (i think)
>
> I'm not opposed to doing this stuff at some point, but just want to make
> sure all changes are intended.
>
> - you moved 'get_mount_info' from the distro to the datasource?
> that doesn't seem right. Possibly related only to resizefs.
FreeBSD on Azure has defined its dev label in order to avoid /dev/daX loading issue, which means if we execute 'mount', we can only find the dev label (like 'rootfs') instead of something like '/dev/daX'. This get_mount_info will return the expected '/dev/daX' on Azure.

I modified this code in order to avoid potential issue, and disable resizefs because Azure does not require it. As I have mentioned, I'll enable it.

>
> - why the changes set_passwd?
> You made 2 changes here:
> a.) use -H 0 and -h 0 rather than just -H or -h
> I'm fine to be explicit, but it seems like it shouldnt be necessary
> or it would never have worked.
> b.) you use a shell to 'echo ... |' when we were previously just
> providing the password as standard in to the command (via subp
> 'data').
> the shell is more processes to do the same work, did it not work for
> you before?
It does not work because 'pw' command only accepts password from stdin. I do not why the origin one works, I guess maybe it works on early FreeBSD version. Anyway, we should fix it.

> - is there a reason you are not using set_passwd from add_user ?
Oh, ok. I should use set_passwd in add_user.

> - util.py:
> freebsd really calls its x86_64 arch 'amd64' ? wow. maybe at this point we
> should just simplify that check to list:
> if arch in ('i386', 'i486', 'i586', 'i686', 'amd64', 'x86_64',
> 'aarch64'):
Ok, agree.

> - generate_fallback_config to the distro object
> this is indeed the least invasive change, and I'm ok with it for now
> but longer term, i want the net module to be able to do that. Ie, I'd like
> the following to do the right thing:
> from cloudinit import net
> net.generate_fallback_config()
>
> comments on generate_fallback_config:
> - you have lots of LOG.info() that is clearly developer debug like stuff
> drop what you can to debug.
> - more generically, do you think that parsing 'ifconfig' is the right
> way to do this for freebsd?
I'll remove those LOG.info(). If we have to use builtin command, for example, 'ifconfig', we have to parse 'ifconfig' output. Maybe there are some other utility commands can help, but it takes more time to investigate. I think we can polish this if I find better alternatives.

> - list_possible_azure_ds_devs
> you are relying on whether or not there is something written to stderr to
> determine if the mount passed or failed. Wouldn't it be better to check
> the return code of the command ?
Yes, I'll change that.

> - get_resource_disk_on_freebsd
> this uses a bunch of shell/awk/grep, better to load it into python and
> parse it there. also, unit tests on this would make it easier for you
> to develop and much less likely for someone to break.
Good suggestion. I'll use python to do that instead of shell/awk/grep.

> - self.metadata['random_seed']
> I dont think this should be necessary, as that path will not exist
> on freebsd (/sys/firmware/acpi/tables/OEM0) and quiet=True to load_file
> means to not raise error.... so we should basically already do nothing
> on freebsd. Right?
Yes, there is no such path on FreeBSD, but FreeBSD has 'acpidump -d' to dump all the content which contains a field 'OEM TABLE ID'. It looks like I have to get the OEM table by myself. Anyway, it is not a block issue and can be assigned a lower priority.

« Back to merge proposal