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

Scott Moser (smoser) wrote :

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.

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.

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.

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.
   - 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.
   - 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.

- 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
     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.

- 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?

  - is there a reason you are not using set_passwd from add_user ?

  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'):

- 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

  comments on generate_fallback_config:
    - you have lots of 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?

- 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 ?

- 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.

- 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?

review: Needs Fixing

« Back to merge proposal