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

Scott Moser (smoser) wrote :

> Thanks for you careful review and valuable comments.
>
> > 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.

Great.

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

OK, Hopefully we could use Jake's suggestion above and get resizefs and growpart working even if they aren't needed on azure. They're in general good things.

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

Maybe i wasn't clear. I was just saying that this all works as expected on Ubuntu (linux). We have a default config with a user 'ubuntu', but on azure, where a user is provided via the datasource, it overrides just that. So this should be able to work correctly.

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

Sorry, again, I wasn't clear.
Essentially your changes do this:
 util.subp("echo password | pw usermod USER -H 0", shell=True)
That should be pretty much the same thing as:
 util.subp(['pw', 'usermod', USER, '-H', '0'], data="password")

And the second format there does not invoke shell, but puts the bytes 'password' into the stdin of the command.

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

I'm not opposed to using 'ifconfig', just didn't know if that was "the right thing" to use on that platform. Ie, if I was doing new linux support, i'd use the 'ip' command rather than 'ifconfig'.

« Back to merge proposal