Merge into master : frbsd-azure-branch : lp:~redriver/cloud-init : Git : Code : cloud-init

Reviewer	Review Type	Date Requested	Status
Scott Moser		2017-01-17	Approve on 2017-05-10
Server Team CI bot	continuous-integration		Approve on 2017-05-08
Review via email: mp+314895@code.launchpad.net

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-03:

#

Hongjjang,
Thanks for submitting this, I will try to take a look next week.
Sorry for the slow response.

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-05:

#

Download full text (4.5 KiB)

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

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

- util.py:
freebsd really calls its x86_64 arch 'amd64' ? wow. maybe at this point we
should just s...

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

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

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

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

- 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

Revision history for this message

Jake Smith (c0by) wrote on 2017-02-06:

#

Hi,

I have been doing some testing on FreeBSD.

The current growpart seems to work fine with FreeBSD 11.0 with GPT/UFS - the partition gets resized. The issue is with filesystem resize in resizefs. Its a simple fix, before you can modify the MBR you must first set kern.geom.debugflags=16, then when calling growfs you must pass the '-y' flag. You should then set debugflags back to it's original value.

Basically this, I did a quick hack to test and seems ok:

current_debugflags = sysctl -n kern.geom.debugflags
sysctl kern.geom.debugflags=16
growfs -y /dev/vtbd0p2
sysctl kern.geom.debugflags=current_debugflags

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-06:

#

Download full text (6.8 KiB)

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

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.

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-07:

#

Download full text (4.4 KiB)

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

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

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-07:

#

Download full text (4.8 KiB)

> > 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.
>
Ok. I see. Anyway, I observed it does not work as expected with default user.
I'll investigate 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.
> > > ...

> > 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.
> 
Ok. I see. Anyway, I observed it does not work as expected with default user.
I'll investigate 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.
> 
> 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")
>
Ok, that looks better. I'll apply it.

> 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'.
Unfortunately, there is no counterpart for 'ip' on FreeBSD. That is why I have to parse ifconfig.

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-07:

#

> Hi,
>
> I have been doing some testing on FreeBSD.
>
> The current growpart seems to work fine with FreeBSD 11.0 with GPT/UFS - the
> partition gets resized. The issue is with filesystem resize in resizefs. Its a
> simple fix, before you can modify the MBR you must first set
> kern.geom.debugflags=16, then when calling growfs you must pass the '-y' flag.
> You should then set debugflags back to it's original value.
>
> Basically this, I did a quick hack to test and seems ok:
>
> current_debugflags = sysctl -n kern.geom.debugflags
> sysctl kern.geom.debugflags=16
> growfs -y /dev/vtbd0p2
> sysctl kern.geom.debugflags=current_debugflags
My problem is growfs always reports error since the disk is already full expanded.

# growfs -y /dev/da0p2
growfs: requested size 28GB is not larger than the current filesystem size 28GB
# echo $?
1

My disk is formatted to be UFS:
# gpart show
=> 40 62914480 da0 GPT (30G)
        40 1024 1 freebsd-boot (512K)
      1064 58719232 2 freebsd-ufs (28G)
  58720296 3145728 3 freebsd-swap (1.5G)
  61866024 1048496 - free - (512M)

=> 63 146800577 da1 MBR (70G)
         63 1985 - free - (993K)
       2048 146796544 1 ntfs (70G)
  146798592 2048 - free - (1.0M)

Revision history for this message

Jake Smith (c0by) wrote on 2017-02-07:

#

> > Hi,
> >
> > I have been doing some testing on FreeBSD.
> >
> > The current growpart seems to work fine with FreeBSD 11.0 with GPT/UFS - the
> > partition gets resized. The issue is with filesystem resize in resizefs. Its
> a
> > simple fix, before you can modify the MBR you must first set
> > kern.geom.debugflags=16, then when calling growfs you must pass the '-y'
> flag.
> > You should then set debugflags back to it's original value.
> >
> > Basically this, I did a quick hack to test and seems ok:
> >
> > current_debugflags = sysctl -n kern.geom.debugflags
> > sysctl kern.geom.debugflags=16
> > growfs -y /dev/vtbd0p2
> > sysctl kern.geom.debugflags=current_debugflags
> My problem is growfs always reports error since the disk is already full
> expanded.
>
> # growfs -y /dev/da0p2
> growfs: requested size 28GB is not larger than the current filesystem size
> 28GB
> # echo $?
> 1
>
> My disk is formatted to be UFS:
> # gpart show
> => 40 62914480 da0 GPT (30G)
> 40 1024 1 freebsd-boot (512K)
> 1064 58719232 2 freebsd-ufs (28G)
> 58720296 3145728 3 freebsd-swap (1.5G)
> 61866024 1048496 - free - (512M)
>
> => 63 146800577 da1 MBR (70G)
> 63 1985 - free - (993K)
> 2048 146796544 1 ntfs (70G)
> 146798592 2048 - free - (1.0M)

Not sure what the most elegant solution would be to avoid exit(1) when there is nothing to do...

Just thinking out loud here:

1.
Can resizefs be called to ignore exit status 1? Could be dangerous?

2.
Is there a way to pass back from growpart to see if gpart size did anything, if so then run growfs?

3.
You could do a dry run first and check the exit status before continuing:
# growfs -N -y /dev/da0p2

4.
Could we look at the current size of the file system and compare to gpart, (there is probably a better way to get the fs size):

# dumpfs -m /
# newfs command for / (/dev/da0p2)
newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -j -k 5240 -m 8 -o time -s 83885872 /dev/da0p2

Grab -s <size> round it and compare against

# gpart show
=> 40 83886000 da0 MBR (466G)
40 128 1 freebsd-boot (64K)
168 83886000 2 freebsd-ufs (40.0G)

> > Hi,
> >
> > I have been doing some testing on FreeBSD.
> >
> > The current growpart seems to work fine with FreeBSD 11.0 with GPT/UFS - the
> > partition gets resized. The issue is with filesystem resize in resizefs. Its
> a
> > simple fix, before you can modify the MBR you must first set
> > kern.geom.debugflags=16, then when calling growfs you must pass the '-y'
> flag.
> > You should then set debugflags back to it's original value.
> >
> > Basically this, I did a quick hack to test and seems ok:
> >
> > current_debugflags = sysctl -n kern.geom.debugflags
> > sysctl kern.geom.debugflags=16
> > growfs -y /dev/vtbd0p2
> > sysctl kern.geom.debugflags=current_debugflags
> My problem is growfs always reports error since the disk is already full
> expanded.
> 
> # growfs -y /dev/da0p2
> growfs: requested size 28GB is not larger than the current filesystem size
> 28GB
> # echo $?
> 1
> 
> My disk is formatted to be UFS:
> # gpart show
> =>      40  62914480  da0  GPT  (30G)
>         40      1024    1  freebsd-boot  (512K)
>       1064  58719232    2  freebsd-ufs  (28G)
>   58720296   3145728    3  freebsd-swap  (1.5G)
>   61866024   1048496       - free -  (512M)
> 
> =>       63  146800577  da1  MBR  (70G)
>          63       1985       - free -  (993K)
>        2048  146796544    1  ntfs  (70G)
>   146798592       2048       - free -  (1.0M)

Not sure what the most elegant solution would be to avoid exit(1) when there is nothing to do...

Just thinking out loud here:

1.
Can resizefs be called to ignore exit status 1? Could be dangerous?

2.
Is there a way to pass back from growpart to see if gpart size did anything, if so then run growfs?

3.
You could do a dry run first and check the exit status before continuing:
# growfs -N -y /dev/da0p2

4.
Could we look at the current size of the file system and compare to gpart, (there is probably a better way to get the fs size):

# dumpfs -m /
# newfs command for / (/dev/da0p2)
newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -j -k 5240 -m 8 -o time -s 83885872 /dev/da0p2

Grab -s <size> round it and compare against

# gpart show   
=>       40  83886000  da0  MBR  (466G)
         40  128         1  freebsd-boot (64K)
        168  83886000    2  freebsd-ufs  (40.0G)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-08:

#

> > > Hi,
> > >
> > > I have been doing some testing on FreeBSD.
> > >
> > > The current growpart seems to work fine with FreeBSD 11.0 with GPT/UFS -
> the
> > > partition gets resized. The issue is with filesystem resize in resizefs.
> Its
> > a
> > > simple fix, before you can modify the MBR you must first set
> > > kern.geom.debugflags=16, then when calling growfs you must pass the '-y'
> > flag.
> > > You should then set debugflags back to it's original value.
> > >
> > > Basically this, I did a quick hack to test and seems ok:
> > >
> > > current_debugflags = sysctl -n kern.geom.debugflags
> > > sysctl kern.geom.debugflags=16
> > > growfs -y /dev/vtbd0p2
> > > sysctl kern.geom.debugflags=current_debugflags
> > My problem is growfs always reports error since the disk is already full
> > expanded.
> >
> > # growfs -y /dev/da0p2
> > growfs: requested size 28GB is not larger than the current filesystem size
> > 28GB
> > # echo $?
> > 1
> >
> > My disk is formatted to be UFS:
> > # gpart show
> > => 40 62914480 da0 GPT (30G)
> > 40 1024 1 freebsd-boot (512K)
> > 1064 58719232 2 freebsd-ufs (28G)
> > 58720296 3145728 3 freebsd-swap (1.5G)
> > 61866024 1048496 - free - (512M)
> >
> > => 63 146800577 da1 MBR (70G)
> > 63 1985 - free - (993K)
> > 2048 146796544 1 ntfs (70G)
> > 146798592 2048 - free - (1.0M)
>
> Not sure what the most elegant solution would be to avoid exit(1) when there
> is nothing to do...
>
> Just thinking out loud here:
>
> 1.
> Can resizefs be called to ignore exit status 1? Could be dangerous?
>
> 2.
> Is there a way to pass back from growpart to see if gpart size did anything,
> if so then run growfs?
>
> 3.
> You could do a dry run first and check the exit status before continuing:
> # growfs -N -y /dev/da0p2
>
> 4.
> Could we look at the current size of the file system and compare to gpart,
> (there is probably a better way to get the fs size):
>
> # dumpfs -m /
> # newfs command for / (/dev/da0p2)
> newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -j
> -k 5240 -m 8 -o time -s 83885872 /dev/da0p2
>
> Grab -s <size> round it and compare against
>
> # gpart show
> => 40 83886000 da0 MBR (466G)
> 40 128 1 freebsd-boot (64K)
> 168 83886000 2 freebsd-ufs (40.0G)
Firstly, I appreciate your help to resolve this issue. I prefer the 4th solution. Thanks.

> > > Hi,
> > >
> > > I have been doing some testing on FreeBSD.
> > >
> > > The current growpart seems to work fine with FreeBSD 11.0 with GPT/UFS -
> the
> > > partition gets resized. The issue is with filesystem resize in resizefs.
> Its
> > a
> > > simple fix, before you can modify the MBR you must first set
> > > kern.geom.debugflags=16, then when calling growfs you must pass the '-y'
> > flag.
> > > You should then set debugflags back to it's original value.
> > >
> > > Basically this, I did a quick hack to test and seems ok:
> > >
> > > current_debugflags = sysctl -n kern.geom.debugflags
> > > sysctl kern.geom.debugflags=16
> > > growfs -y /dev/vtbd0p2
> > > sysctl kern.geom.debugflags=current_debugflags
> > My problem is growfs always reports error since the disk is already full
> > expanded.
> >
> > # growfs -y /dev/da0p2
> > growfs: requested size 28GB is not larger than the current filesystem size
> > 28GB
> > # echo $?
> > 1
> >
> > My disk is formatted to be UFS:
> > # gpart show
> > =>      40  62914480  da0  GPT  (30G)
> >         40      1024    1  freebsd-boot  (512K)
> >       1064  58719232    2  freebsd-ufs  (28G)
> >   58720296   3145728    3  freebsd-swap  (1.5G)
> >   61866024   1048496       - free -  (512M)
> >
> > =>       63  146800577  da1  MBR  (70G)
> >          63       1985       - free -  (993K)
> >        2048  146796544    1  ntfs  (70G)
> >   146798592       2048       - free -  (1.0M)
> 
> Not sure what the most elegant solution would be to avoid exit(1) when there
> is nothing to do...
> 
> Just thinking out loud here:
> 
> 1.
> Can resizefs be called to ignore exit status 1? Could be dangerous?
> 
> 2.
> Is there a way to pass back from growpart to see if gpart size did anything,
> if so then run growfs?
> 
> 3.
> You could do a dry run first and check the exit status before continuing:
> # growfs -N -y /dev/da0p2
> 
> 4.
> Could we look at the current size of the file system and compare to gpart,
> (there is probably a better way to get the fs size):
> 
> # dumpfs -m /
> # newfs command for / (/dev/da0p2)
> newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -j
> -k 5240 -m 8 -o time -s 83885872 /dev/da0p2
> 
> Grab -s <size> round it and compare against
> 
> # gpart show
> =>       40  83886000  da0  MBR  (466G)
>          40  128         1  freebsd-boot (64K)
>         168  83886000    2  freebsd-ufs  (40.0G)
Firstly, I appreciate your help to resolve this issue. I prefer the 4th solution. Thanks.

Revision history for this message

Jake Smith (c0by) wrote on 2017-02-08:

#

Just checked the code, you will need to round the output from gpart to the nearest whole block, see this example:

# gpart show md0
=> 34 297086 md0 GPT (145M)
34 297086 1 freebsd-ufs (145M)

# dumpfs -m /mnt
# newfs command for /mnt (/dev/md0p1)
newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -k 368 -m 8 -o time -s 297080 /dev/md0p1

So gpart reports 297086 but dumpfs reports 297080, making your code think it needs to do a resize.

Should be able to round the number using the fragment size (-f <frag-size>) like this, assuming a sector size of 512:

>>> gpart = 297086
>>> frag = 4096
>>> (gpart) - (gpart) % (frag / 512)
297080.0

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-10:

#

> Just checked the code, you will need to round the output from gpart to the
> nearest whole block, see this example:
>
> # gpart show md0
> => 34 297086 md0 GPT (145M)
> 34 297086 1 freebsd-ufs (145M)
>
> # dumpfs -m /mnt
> # newfs command for /mnt (/dev/md0p1)
> newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -k
> 368 -m 8 -o time -s 297080 /dev/md0p1
>
> So gpart reports 297086 but dumpfs reports 297080, making your code think it
> needs to do a resize.
>
> Should be able to round the number using the fragment size (-f <frag-size>)
> like this, assuming a sector size of 512:
>
> >>> gpart = 297086
> >>> frag = 4096
> >>> (gpart) - (gpart) % (frag / 512)
> 297080.0
Thanks. I'll apply it to my patch.

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-12:

#

> Just checked the code, you will need to round the output from gpart to the
> nearest whole block, see this example:
>
> # gpart show md0
> => 34 297086 md0 GPT (145M)
> 34 297086 1 freebsd-ufs (145M)
>
> # dumpfs -m /mnt
> # newfs command for /mnt (/dev/md0p1)
> newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384 -h 64 -i 8192 -k
> 368 -m 8 -o time -s 297080 /dev/md0p1
>
> So gpart reports 297086 but dumpfs reports 297080, making your code think it
> needs to do a resize.
>
> Should be able to round the number using the fragment size (-f <frag-size>)
> like this, assuming a sector size of 512:
>
> >>> gpart = 297086
> >>> frag = 4096
> >>> (gpart) - (gpart) % (frag / 512)
> 297080.0
Another question, when I install cloud-init on FreeBSD, the cloud.cfg is used and I have to manually replace it with cloud.cfg-freebsd. Is it better select cloud configuration according to --init-system option. I want to implement like the following:

if (the value of --init-system option equal to sysvinit_freebsd)
then
replace cloud.cfg with cloud.cfg-FreeBSD
fi

But I also find tools/build-on-FreeBSD contains some logic similar. My question is where build-on-FreeBSD is invoked? My cloud-init installation is "python setup.py --init-system=sysvinit_freebsd", does I miss anything?

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-13:

#

one more issue I found is the user information provided by Azure cannot override the default one. The reason is: Stages.Modules.cfg depends on helpers.ConfigMerger to merge the cfg. But the internal function helpers._read_cfg says env_configs overrides datasource_configs. See the comments below. It also mentions the input config overrides env_config, but on Azure, the user information comes from datasource, how can I get input config ahead of datasource?

The quick fix is move "cfgs.extend(self._get_datasource_configs())" ahead of "cfgs.extend(self._get_env_configs())".

I also wonder why Ubuntu on Azure does not encounter this issue. It looks like Ubuntu on Azure invokes cloudinitlocal, but I'm still confused how Ubuntu can override env_configs.

    def _read_cfg(self):
        # Input config files override
        # env config files which
        # override instance configs
        # which override datasource
        # configs which override
        # base configuration
        cfgs = []
        if self._fns:
            for c_fn in self._fns:
                try:
                    cfgs.append(util.read_conf(c_fn))
                except Exception:
                    util.logexc(LOG, "Failed loading of configuration from %s",
                                c_fn)

        cfgs.extend(self._get_env_configs())
        cfgs.extend(self._get_instance_configs())
        cfgs.extend(self._get_datasource_configs())
        if self._base_cfg:
            cfgs.append(self._base_cfg)
        return util.mergemanydict(cfgs)

Revision history for this message

Scott Moser (smoser) wrote on 2017-02-17:

#

I'm confused because Ubuntu's cloud.cfg is exactly what is in
config/cloud.cfg in trunk. which has:

  users:
     - default
  ...

  system_info:
     # This will affect which distro class gets used
     distro: ubuntu
     # Default user name + that default users groups (if added/used)
     default_user:
       name: ubuntu
       lock_passwd: True
       gecos: Ubuntu
       groups: [adm, audio, cdrom, dialout, dip, floppy, lxd, netdev, plugdev, sudo, video]
       sudo: ["ALL=(ALL) NOPASSWD:ALL"]
       shell: /bin/bash

Thats almost identical to what config/cloud.cfg-freebsd has in your
branch.

I think that comment is actually wrong. If you look at how that method
works, it basically builds an array of configs (dictionaries), with
environment config (from environment CFG_ENV_NAME) first, then isntance
configs, then datasource, then base. Then calls mergemanydict with that.

So basically:
  util.mergemanydict([
     {'foo': 'from enviroment'},
     {'foo': 'from instance, no user override'},
     {'system_info': {'default_user': {'name': 'smoser-from-ds'}}},
     {'system_info': {'default_user': {'name': 'ubuntu'}}},
  ])

Calling that returns:
{'foo': 'from enviroment',
'system_info': {'default_user': {'name': 'smoser-from-ds'}}}

So I'm pretty sure the comment is just wrong. Its probably because
'mergemanydict' has very odd usage in that precedence goes to the *first*
value, not the last.

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-17:

#

Download full text (4.0 KiB)

> I'm confused because Ubuntu's cloud.cfg is exactly what is in
> config/cloud.cfg in trunk. which has:
>
> users:
> - default
> ...
>
> system_info:
> # This will affect which distro class gets used
> distro: ubuntu
> # Default user name + that default users groups (if added/used)
> default_user:
> name: ubuntu
> lock_passwd: True
> gecos: Ubuntu
> groups: [adm, audio, cdrom, dialout, dip, floppy, lxd, netdev, plugdev,
> sudo, video]
> sudo: ["ALL=(ALL) NOPASSWD:ALL"]
> shell: /bin/bash
>
> Thats almost identical to what config/cloud.cfg-freebsd has in your
> branch.
>
> I think that comment is actually wrong. If you look at how that method
> works, it basically builds an array of configs (dictionaries), with
> environment config (from environment CFG_ENV_NAME) first, then isntance
> configs, then datasource, then base. Then calls mergemanydict with that.
>
> So basically:
> util.mergemanydict([
> {'foo': 'from enviroment'},
> {'foo': 'from instance, no user override'},
> {'system_info': {'default_user': {'name': 'smoser-from-ds'}}},
> {'system_info': {'default_user': {'name': 'ubuntu'}}},
> ])
>
> Calling that returns:
> {'foo': 'from enviroment',
> 'system_info': {'default_user': {'name': 'smoser-from-ds'}}}
>
> So I'm pretty sure the comment is just wrong. Its probably because
> 'mergemanydict' has very odd usage in that precedence goes to the *first*
> value, not the last.
Ok. Let's ignore that comment. I debug this code snippet and still found it there.
I added a test function in helper.py, and specify two input files '/usr/local/etc/cloud/cloud.cfg' and 'ds.dat'
The content of ds.dat is as following.

{'disk_setup': {'ephemeral0': {'table_type': 'gpt', 'layout': [100], 'overwrite': True}}, 'fs_setup': [{'device': 'ephemeral0.1', 'replace_fs': 'ntfs', 'filesystem': 'freebsd-ufs'}], 'system_info': {'default_user': {'passwd': '$6$WSLBpwonNkC0Yi2b$w0xih6LR1X.TSyjn2mYSNiHITd.2oqD/j6CdoskUoXhH04/N8bvi6GB3smdZ4JT5hMrbT1svqtkkQeYY/B2fZ/', 'name': u'azureuser', 'lock_passwd': False}}, 'ssh_pwauth': True}

The output indicates that the username is still "freebsd" even though the "passwd" field was updated. So, in my environment, there is something different from the environment on Ubuntu.

{"ssh_pwauth": true, "users": ["default"], "disable_root": false, "disk_setup": {"ephemeral0": {"table_type": "gpt", "layout": [100], "overwrite": true}}, "system_info": {"default_user": {"shell": "/bin/tcsh", "gecos": "FreeBSD", "name": "freebsd", "groups": ["wheel"], "passwd": "$6$WSLBpwonNkC0Yi2b$w0xih6LR1X.TSyjn2mYSNiHITd.2oqD/j6CdoskUoXhH04/N8bvi6GB3smdZ4JT5hMrbT1svqtkkQeYY/B2fZ/", "sudo": ["ALL=(ALL) NOPASSWD:ALL"], "lock_passwd": true}, "distro": "freebsd"}, "syslog_fix_perms": "root:wheel", "fs_setup": [{"device": "ephemeral0.1", "replace_fs": "ntfs", "filesystem": "freebsd-ufs"}], "cloud_init_modules": ["seed_random", "bootcmd", "growpart", "resizefs", "set_hostname", "update_hostname", "users-groups", "ssh"], "preserve_hostname": false, "cloud_final_modules": ["rightscale_userdata", "scripts-vendor", "scripts-per-once", "scripts-per-boot", "sc...

> I'm confused because Ubuntu's cloud.cfg is exactly what is in
> config/cloud.cfg in trunk.  which has:
> 
>   users:
>      - default
>   ...
> 
>   system_info:
>      # This will affect which distro class gets used
>      distro: ubuntu
>      # Default user name + that default users groups (if added/used)
>      default_user:
>        name: ubuntu
>        lock_passwd: True
>        gecos: Ubuntu
>        groups: [adm, audio, cdrom, dialout, dip, floppy, lxd, netdev, plugdev,
> sudo, video]
>        sudo: ["ALL=(ALL) NOPASSWD:ALL"]
>        shell: /bin/bash
> 
> Thats almost identical to what config/cloud.cfg-freebsd has in your
> branch.
> 
> I think that comment is actually wrong.  If you look at how that method
> works, it basically builds an array of configs (dictionaries), with
> environment config (from environment CFG_ENV_NAME) first, then isntance
> configs, then datasource, then base.  Then calls mergemanydict with that.
> 
> So basically:
>   util.mergemanydict([
>      {'foo': 'from enviroment'},
>      {'foo': 'from instance, no user override'},
>      {'system_info': {'default_user': {'name': 'smoser-from-ds'}}},
>      {'system_info': {'default_user': {'name': 'ubuntu'}}},
>   ])
> 
> Calling that returns:
> {'foo': 'from enviroment',
>  'system_info': {'default_user': {'name': 'smoser-from-ds'}}}
> 
> So I'm pretty sure the comment is just wrong.  Its probably because
> 'mergemanydict' has very odd usage in that precedence goes to the *first*
> value, not the last.
Ok. Let's ignore that comment. I debug this code snippet and still found it there.
I added a test function in helper.py, and specify two input files '/usr/local/etc/cloud/cloud.cfg' and 'ds.dat'
The content of ds.dat is as following.

{'disk_setup': {'ephemeral0': {'table_type': 'gpt', 'layout': [100], 'overwrite': True}}, 'fs_setup': [{'device': 'ephemeral0.1', 'replace_fs': 'ntfs', 'filesystem': 'freebsd-ufs'}], 'system_info': {'default_user': {'passwd': '$6$WSLBpwonNkC0Yi2b$w0xih6LR1X.TSyjn2mYSNiHITd.2oqD/j6CdoskUoXhH04/N8bvi6GB3smdZ4JT5hMrbT1svqtkkQeYY/B2fZ/', 'name': u'azureuser', 'lock_passwd': False}}, 'ssh_pwauth': True}

The output indicates that the username is still "freebsd" even though the "passwd" field was updated. So, in my environment, there is something different from the environment on Ubuntu.

{"ssh_pwauth": true, "users": ["default"], "disable_root": false, "disk_setup": {"ephemeral0": {"table_type": "gpt", "layout": [100], "overwrite": true}}, "system_info": {"default_user": {"shell": "/bin/tcsh", "gecos": "FreeBSD", "name": "freebsd", "groups": ["wheel"], "passwd": "$6$WSLBpwonNkC0Yi2b$w0xih6LR1X.TSyjn2mYSNiHITd.2oqD/j6CdoskUoXhH04/N8bvi6GB3smdZ4JT5hMrbT1svqtkkQeYY/B2fZ/", "sudo": ["ALL=(ALL) NOPASSWD:ALL"], "lock_passwd": true}, "distro": "freebsd"}, "syslog_fix_perms": "root:wheel", "fs_setup": [{"device": "ephemeral0.1", "replace_fs": "ntfs", "filesystem": "freebsd-ufs"}], "cloud_init_modules": ["seed_random", "bootcmd", "growpart", "resizefs", "set_hostname", "update_hostname", "users-groups", "ssh"], "preserve_hostname": false, "cloud_final_modules": ["rightscale_userdata", "scripts-vendor", "scripts-per-once", "scripts-per-boot", "scripts-per-instance", "scripts-user", "ssh-authkey-fingerprints", "keys-to-console", "phone-home", "final-message", "power-state-change"], "datasource_list": ["ConfigDrive", "OpenStack", "Ec2"], "cloud_config_modules": ["ssh-import-id", "locale", "set-passwords", "package-update-upgrade-install", "timezone", "disable-ec2-metadata", "runcmd"]}

def test_read_cfg():
        # Input config files override
        # env config files which
        # override instance configs
        # which override datasource
        # configs which override
        # base configuration
        cfgs = []
        env_config = []
        env_config.append(util.read_conf('/usr/local/etc/cloud/cloud.cfg'))
        datasource_config = []
        datasource_config.append(util.read_conf('ds.dat'))
        
        cfgs.extend(env_config)
        cfgs.extend({})
        cfgs.extend(datasource_config)
        print(json.dumps(util.mergemanydict(cfgs)))

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-17:

#

> I'm confused because Ubuntu's cloud.cfg is exactly what is in
> config/cloud.cfg in trunk. which has:
>
> users:
> - default
> ...
>
> system_info:
> # This will affect which distro class gets used
> distro: ubuntu
> # Default user name + that default users groups (if added/used)
> default_user:
> name: ubuntu
> lock_passwd: True
> gecos: Ubuntu
> groups: [adm, audio, cdrom, dialout, dip, floppy, lxd, netdev, plugdev,
> sudo, video]
> sudo: ["ALL=(ALL) NOPASSWD:ALL"]
> shell: /bin/bash
>
> Thats almost identical to what config/cloud.cfg-freebsd has in your
> branch.
>
> I think that comment is actually wrong. If you look at how that method
> works, it basically builds an array of configs (dictionaries), with
> environment config (from environment CFG_ENV_NAME) first, then isntance
> configs, then datasource, then base. Then calls mergemanydict with that.
>
> So basically:
> util.mergemanydict([
> {'foo': 'from enviroment'},
> {'foo': 'from instance, no user override'},
> {'system_info': {'default_user': {'name': 'smoser-from-ds'}}},
> {'system_info': {'default_user': {'name': 'ubuntu'}}},
> ])
>
> Calling that returns:
> {'foo': 'from enviroment',
> 'system_info': {'default_user': {'name': 'smoser-from-ds'}}}
>
> So I'm pretty sure the comment is just wrong. Its probably because
> 'mergemanydict' has very odd usage in that precedence goes to the *first*
> value, not the last.
Sorry, you are right, Ubuntu's cloud.cfg also specifies default user "ubuntu". That is not the root cause why cloud-init does not work on FreeBSD.
New finding is the helper.py will not load cloud.cfg as env_configs because only FreeBSD defines the CLOUD_CFG, which tells helpers.py to load env_configs.
I grep "CLOUD_CFG" from systemd and sysvinit and find:
grep "CLOUD_CFG" sysvinit/* -rn
sysvinit/freebsd/cloudconfig:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
sysvinit/freebsd/cloudfinal:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
sysvinit/freebsd/cloudinit:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
sysvinit/freebsd/cloudinitlocal:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg

in the helpers.py, I observed:

CFG_ENV_NAME = "CLOUD_CFG"
...
    def _get_env_configs(self):
        e_cfgs = []
        if CFG_ENV_NAME in os.environ:
            e_fn = os.environ[CFG_ENV_NAME]
            try:
                e_cfgs.append(util.read_conf(e_fn))
            except Exception:
                util.logexc(LOG, 'Failed loading of env. config from %s',
                            e_fn)
        return e_cfgs

I don't know why FreeBSD needs to export this environment, but none of the other distros need it.

> I'm confused because Ubuntu's cloud.cfg is exactly what is in
> config/cloud.cfg in trunk.  which has:
> 
>   users:
>      - default
>   ...
> 
>   system_info:
>      # This will affect which distro class gets used
>      distro: ubuntu
>      # Default user name + that default users groups (if added/used)
>      default_user:
>        name: ubuntu
>        lock_passwd: True
>        gecos: Ubuntu
>        groups: [adm, audio, cdrom, dialout, dip, floppy, lxd, netdev, plugdev,
> sudo, video]
>        sudo: ["ALL=(ALL) NOPASSWD:ALL"]
>        shell: /bin/bash
> 
> Thats almost identical to what config/cloud.cfg-freebsd has in your
> branch.
> 
> I think that comment is actually wrong.  If you look at how that method
> works, it basically builds an array of configs (dictionaries), with
> environment config (from environment CFG_ENV_NAME) first, then isntance
> configs, then datasource, then base.  Then calls mergemanydict with that.
> 
> So basically:
>   util.mergemanydict([
>      {'foo': 'from enviroment'},
>      {'foo': 'from instance, no user override'},
>      {'system_info': {'default_user': {'name': 'smoser-from-ds'}}},
>      {'system_info': {'default_user': {'name': 'ubuntu'}}},
>   ])
> 
> Calling that returns:
> {'foo': 'from enviroment',
>  'system_info': {'default_user': {'name': 'smoser-from-ds'}}}
> 
> So I'm pretty sure the comment is just wrong.  Its probably because
> 'mergemanydict' has very odd usage in that precedence goes to the *first*
> value, not the last.
Sorry, you are right, Ubuntu's cloud.cfg also specifies default user "ubuntu". That is not the root cause why cloud-init does not work on FreeBSD.
New finding is the helper.py will not load cloud.cfg as env_configs because only FreeBSD defines the CLOUD_CFG, which tells helpers.py to load env_configs.
I grep "CLOUD_CFG" from systemd and sysvinit and find:
 grep "CLOUD_CFG" sysvinit/* -rn
sysvinit/freebsd/cloudconfig:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
sysvinit/freebsd/cloudfinal:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
sysvinit/freebsd/cloudinit:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
sysvinit/freebsd/cloudinitlocal:10:export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg

in the helpers.py, I observed:

CFG_ENV_NAME = "CLOUD_CFG"
...
    def _get_env_configs(self):
        e_cfgs = []
        if CFG_ENV_NAME in os.environ:
            e_fn = os.environ[CFG_ENV_NAME]
            try:
                e_cfgs.append(util.read_conf(e_fn))
            except Exception:
                util.logexc(LOG, 'Failed loading of env. config from %s',
                            e_fn)
        return e_cfgs

I don't know why FreeBSD needs to export this environment, but none of the other distros need it.

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-02-24:

#

I have finished all the needed fixes.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-06:

#

PASSED: Continuous integration, rev:400a89bc938bf351f37530f12118f2281c938908
https://jenkins.ubuntu.com/server/job/cloud-init-ci/83/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/83
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/83
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/83
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-amd64/83
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/83

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/83/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2017-03-09:

#

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.

b.) I think we can simplify the '_can_skip_resize_*' things a bit.:w

c.) is your '_can_skip_resize_ufs' for performance? or is it actually needed?

review: Needs Fixing

Revision history for this message

Hongjiang Zhang (redriver) on 2017-03-10:

#

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-10:

#

FAILED: Continuous integration, rev:85137a5ffe65aeeabf02d030ffe22897ae48fba5
https://jenkins.ubuntu.com/server/job/cloud-init-ci/96/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/96/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/96/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/96/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/96/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/96/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/96/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-10:

#

PASSED: Continuous integration, rev:316b4b389cd60b58d0213ba47766476f6e739b5e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/97/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/97
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/97
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/97
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/97
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/97

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/97/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-03-10:

#

> 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/__init__.py. 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.

Revision history for this message

Scott Moser (smoser) wrote on 2017-03-13:

#

> > 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/__init__.py. 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)

devent2dev basically is supposed to turn either a filesystem location ("/home")
or a device into a device that contains that filesystem. Input of '/dev/sda'
should return '/dev/sda'. Input of "/home" should return the device that
/home's filesystem is on (what should be rezied).

Your made a change to resize_devices in cloudinit/config/cc_growpart.py to
take a 'cloud'.

- resize_devices then calls devent2dev(devent, cloud)
- devent2dev calls cloud.datasource.get_mount_info(devent)
This change:
- result = util.get_mount_info(devent)
+ result = cloud.datasource.get_mount_info(devent)

I'd have preferred for 'util.get_mount_info' to have code that says:
   if is_FreeBSD:
      # get the device one way
   else:
      # get it for linux.

Instead, since you're going through 'datasource', your fix will only work
on Azure.

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

OK, thats fine.

Revision history for this message

Scott Moser (smoser) wrote on 2017-03-13:

#

I think you might be able to not change anything in the get_mount_info path, but rather just fix a bug in 'parse_mount(path)'.

--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2056,6 +2056,8 @@ def parse_mount(path):
     mount_locs = mountoutput.splitlines()
     for line in mount_locs:
         m = re.search(r'^(/dev/[\S]+) on (/.*) $(.+), .+, (.+)$$', line)
+ if not m:
+ continue
         devpth = m.group(1)
         mount_point = m.group(2)
         fs_type = m.group(3)

then, in my testing on a freebsd instance in digital ocean :
$ python -c 'from cloudinit import util; print(util.get_mount_info("/"))'
('/dev/gpt/rootfs', 'ufs', '/')

Actually, even without the change it works for me with 'mount' output like:
$ mount
/dev/gpt/rootfs on / (ufs, local, soft-updates)
devfs on /dev (devfs, local, multilabel)
fdescfs on /dev/fd (fdescfs)
/dev/vtbd1 on /var/lib/cloud/seed/config_drive (cd9660, local, read-only)

Note, that it does not work unless you give it a mount point as input. Ie, util.get_mount_info("/etc") will not work while it does work on linux. We could fix that though, by just looking through all the mount points and finding the one that 'path' is on.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-14:

#

FAILED: Continuous integration, rev:cddf09f0a2695d4d91380c9dbc128d0a2c1e42f7
https://jenkins.ubuntu.com/server/job/cloud-init-ci/112/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/112/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/112/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/112/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/112/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/112/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/112/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-14:

#

FAILED: Continuous integration, rev:27ab16c21a16d65a64b72dcd670bcc9c014b4fd9
https://jenkins.ubuntu.com/server/job/cloud-init-ci/113/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/113/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/113/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/113/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/113/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/113/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/113/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-14:

#

PASSED: Continuous integration, rev:1bafe24b28593c59636eaa7470d2594addfa7a05
https://jenkins.ubuntu.com/server/job/cloud-init-ci/114/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/114
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/114
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/114
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/114
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/114

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/114/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-03-15:

#

I have done the fix. I also fixed the issue you mentioned for get_mount_info by providing a path.

> I think you might be able to not change anything in the get_mount_info path,
> but rather just fix a bug in 'parse_mount(path)'.
>
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -2056,6 +2056,8 @@ def parse_mount(path):
> mount_locs = mountoutput.splitlines()
> for line in mount_locs:
> m = re.search(r'^(/dev/[\S]+) on (/.*) $(.+), .+, (.+)$$', line)
> + if not m:
> + continue
> devpth = m.group(1)
> mount_point = m.group(2)
> fs_type = m.group(3)
>
>
> then, in my testing on a freebsd instance in digital ocean :
> $ python -c 'from cloudinit import util; print(util.get_mount_info("/"))'
> ('/dev/gpt/rootfs', 'ufs', '/')
>
> Actually, even without the change it works for me with 'mount' output like:
> $ mount
> /dev/gpt/rootfs on / (ufs, local, soft-updates)
> devfs on /dev (devfs, local, multilabel)
> fdescfs on /dev/fd (fdescfs)
> /dev/vtbd1 on /var/lib/cloud/seed/config_drive (cd9660, local, read-only)
>
>
> Note, that it does not work unless you give it a mount point as input. Ie,
> util.get_mount_info("/etc") will not work while it does work on linux. We
> could fix that though, by just looking through all the mount points and
> finding the one that 'path' is on.

Revision history for this message

Scott Moser (smoser) wrote on 2017-03-24:

#

Hi,
Sorry for the slow reply.
We are getting closer.
I have some comments inline.

review: Needs Fixing

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-28:

#

PASSED: Continuous integration, rev:1505ec9ccc59e55e10b99244fb7fa8fdaa37c3c2
https://jenkins.ubuntu.com/server/job/cloud-init-ci/168/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/168
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/168
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/168
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/168
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/168

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/168/rebuild

review: Approve (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-28:

#

PASSED: Continuous integration, rev:35141b85edc4d1e7d09ca974c9fc3456957ea384
https://jenkins.ubuntu.com/server/job/cloud-init-ci/169/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/169
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/169
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/169
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/169
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/169

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/169/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-03-28:

#

> Hi,
> Sorry for the slow reply.
> We are getting closer.
> I have some comments inline.
I have removed 'cloud' parameter from devent2dev, and I added more check in util.get_mount_info to specially handle the case for FreeBSD on Azure.

Is there any other mechanism for me to easily check whether it is running on Hyper-V?

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-03-28:

#

> Hi,
> Sorry for the slow reply.
> We are getting closer.
> I have some comments inline.
I have removed 'cloud' parameter from devent2dev, and I added more check in util.get_mount_info to specially handle the case for FreeBSD on Azure.

Is there any other mechanism for me to easily check whether it is running on Hyper-V?

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-29:

#

PASSED: Continuous integration, rev:a6a04d1d8404b27658faf3c254840cf246fa4b75
https://jenkins.ubuntu.com/server/job/cloud-init-ci/176/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/176
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/176
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/176
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/176
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/176

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/176/rebuild

review: Approve (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-29:

#

FAILED: Continuous integration, rev:d91244316aa2d9c2195018bb004ca6eccfd9692e
https://jenkins.ubuntu.com/server/job/cloud-init-ci/177/
Executed test runs:
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/177/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/177/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/177/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/177/console
    FAILURE: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/177/console

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/177/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-03-29:

#

PASSED: Continuous integration, rev:9f509bee5f64fc2332a3896f17336f7c6d55ee15
https://jenkins.ubuntu.com/server/job/cloud-init-ci/179/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/179
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/179
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/179
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/179
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/179

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/179/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2017-04-12:

#

there are som ecomments in line.
I have a mp i will submit to you to cleanup some others.

Revision history for this message

Scott Moser (smoser) wrote on 2017-04-12:

#

please see my changes at https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/322475

Revision history for this message

Scott Moser (smoser) wrote on 2017-04-12:

#

Other than that and I think it looks really good.
thank you for adding the unit tests.

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-04-13:

#

Download full text (5.2 KiB)

> diff --git a/cloudinit/util.py b/cloudinit/util.py index
> 3301957..ef84e32 100644
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -2089,6 +2144,8 @@ def get_mount_info(path, log=LOG):
> #
> # So use /proc/$$/mountinfo to find the device underlying the
> # input path.
> + if is_FreeBSD_on_hyperv():

>but why "freebsd on hyperv"
>why are you not just checking "is this freebsd".
>what is specific to Azure here?

FreeBSD on Azure used a special label on disk. If we want to
get the /dev/daXXX from mount information, we need to handle
it specially.

> + return get_mount_info_freebsd_on_Azure(path, log)
> mountinfo_path = '/proc/%s/mountinfo' % os.getpid()
> if os.path.exists(mountinfo_path):
> lines = load_file(mountinfo_path).splitlines()
> diff --git a/tests/unittests/test_handler/test_handler_resizefs.py
> b/tests/unittests/test_handler/test_handler_resizefs.py
> new file mode 100644
> index 0000000..b5384e4
> --- /dev/null
> +++ b/tests/unittests/test_handler/test_handler_resizefs.py
> @@ -0,0 +1,73 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +from cloudinit.config import cc_resizefs
> +
> +import unittest
> +
> +try:
> + from unittest import mock
> +except ImportError:
> + import mock
> +
> +
> +class TestResizefs(unittest.TestCase):
> + def setUp(self):
> + super(TestResizefs, self).setUp()
> + self.name = "resizefs"
> +
> + @mock.patch('cloudinit.config.cc_resizefs._get_dumpfs_output')
> + @mock.patch('cloudinit.config.cc_resizefs._get_gpart_output')
> + def test_skip_ufs_resize(self, gpart_out, dumpfs_out):
> + fs_type = "ufs"
> + resize_what = "/"
> + devpth = "/dev/da0p2"
> + dumpfs_out.return_value = "# newfs command for / "\
> + "(/dev/label/rootfs)\n" \
> + "newfs -O 2 -U -a 4 -b "\
> + "32768 -d 32768 -e 4096 "\
> + "-f 4096 -g 16384 -h 64 "\
> + "-i 8192 -j -k 6408 -m 8 "\
> + "-o time -s 58719232 "\
> + "/dev/label/rootfs\n"
> + gpart_out.return_value = """
> +=> 40 62914480 da0 GPT (30G)
> + 40 1024 1 freebsd-boot (512K)
> + 1064 58719232 2 freebsd-ufs (28G)
> + 58720296 3145728 3 freebsd-swap (1.5G)
> + 61866024 1048496 - free - (512M)
> +"""
> + res = cc_resizefs.can_skip_resize(fs_type,
> + resize_what,
> + devpth)
> + self.assertTrue(res)
> +
> + @mock.patch('cloudinit.config.cc_resizefs._get_dumpfs_output')
> + @mock.patch('cloudinit.config.cc_resizefs._get_gpart_output')
> + def test_skip_ufs_resize_roundup(self, gpart_out, dumpfs_out):
> + fs_type = "ufs"
> + resize_what = "/"
> + devpth = "/dev/da0p2"
> + dumpfs_out.return_value = "# newfs command for / "\
> + ...

> diff --git a/cloudinit/util.py b/cloudinit/util.py index 
> 3301957..ef84e32 100644
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -2089,6 +2144,8 @@ def get_mount_info(path, log=LOG):
>      #
>      # So use /proc/$$/mountinfo to find the device underlying the
>      # input path.
> +    if is_FreeBSD_on_hyperv():

>but why "freebsd on hyperv"
>why are you not just checking "is this freebsd".
>what is specific to Azure here?

FreeBSD on Azure used a special label on disk. If we want to
get the /dev/daXXX from mount information, we need to handle
it specially.

> +        return get_mount_info_freebsd_on_Azure(path, log)
>      mountinfo_path = '/proc/%s/mountinfo' % os.getpid()
>      if os.path.exists(mountinfo_path):
>          lines = load_file(mountinfo_path).splitlines()
> diff --git a/tests/unittests/test_handler/test_handler_resizefs.py 
> b/tests/unittests/test_handler/test_handler_resizefs.py
> new file mode 100644
> index 0000000..b5384e4
> --- /dev/null
> +++ b/tests/unittests/test_handler/test_handler_resizefs.py
> @@ -0,0 +1,73 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +from cloudinit.config import cc_resizefs
> +
> +import unittest
> +
> +try:
> +    from unittest import mock
> +except ImportError:
> +    import mock
> +
> +
> +class TestResizefs(unittest.TestCase):
> +    def setUp(self):
> +        super(TestResizefs, self).setUp()
> +        self.name = "resizefs"
> +
> +    @mock.patch('cloudinit.config.cc_resizefs._get_dumpfs_output')
> +    @mock.patch('cloudinit.config.cc_resizefs._get_gpart_output')
> +    def test_skip_ufs_resize(self, gpart_out, dumpfs_out):
> +        fs_type = "ufs"
> +        resize_what = "/"
> +        devpth = "/dev/da0p2"
> +        dumpfs_out.return_value = "# newfs command for / "\
> +                                  "(/dev/label/rootfs)\n" \
> +                                  "newfs -O 2 -U -a 4 -b "\
> +                                  "32768 -d 32768 -e 4096 "\
> +                                  "-f 4096 -g 16384 -h 64 "\
> +                                  "-i 8192 -j -k 6408 -m 8 "\
> +                                  "-o time -s 58719232 "\
> +                                  "/dev/label/rootfs\n"
> +        gpart_out.return_value = """
> +=>      40  62914480  da0  GPT  (30G)
> +        40      1024    1  freebsd-boot  (512K)
> +      1064  58719232    2  freebsd-ufs  (28G)
> +  58720296   3145728    3  freebsd-swap  (1.5G)
> +  61866024   1048496       - free -  (512M)
> +"""
> +        res = cc_resizefs.can_skip_resize(fs_type,
> +                                          resize_what,
> +                                          devpth)
> +        self.assertTrue(res)
> +
> +    @mock.patch('cloudinit.config.cc_resizefs._get_dumpfs_output')
> +    @mock.patch('cloudinit.config.cc_resizefs._get_gpart_output')
> +    def test_skip_ufs_resize_roundup(self, gpart_out, dumpfs_out):
> +        fs_type = "ufs"
> +        resize_what = "/"
> +        devpth = "/dev/da0p2"
> +        dumpfs_out.return_value = "# newfs command for / "\
> +                                  "(/dev/label/rootfs)\n" \
> +                                  "newfs -O 2 -U -a 4 -b "\
> +                                  "32768 -d 32768 -e 4096 "\
> +                                  "-f 4096 -g 16384 -h 64 "\
> +                                  "-i 8192 -j -k 368 -m 8 "\
> +                                  "-o time -s 297080 "\
> +                                  "/dev/label/rootfs\n"
> +        gpart_out.return_value = """
> +=>      34  297086  da0  GPT  (145M)
> +        34  297086    1  freebsd-ufs  (145M)
> +"""
> +        res = cc_resizefs.can_skip_resize(fs_type,
> +                                          resize_what,
> +                                          devpth)
> +        self.assertTrue(res)
> +
> +
> +class Bunch(object):

>this seems not necessary.

Ok.

> +    def __init__(self, **kwds):
> +        self.__dict__.update(kwds)
> +
> +
> +# vi: ts=4 expandtab
> diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py 
> index 4f07d80..bd22759 100644
> --- a/tests/unittests/test_net.py
> +++ b/tests/unittests/test_net.py
> @@ -1044,6 +1044,9 @@ class TestNetplanPostcommands(CiTestCase):
>      @mock.patch.object(netplan, "get_devicelist")
>      @mock.patch('cloudinit.util.subp')
>      def test_netplan_postcmds(self, mock_subp, mock_devlist):
> +        # FreeBSD does not have 'netplan' cmd

>this should not be necessary. netplan is never called (it is mocked).

Ok. Let me double check it.

> +        if util.is_FreeBSD():
> +            return
>          mock_devlist.side_effect = [['lo']]
>          tmp_dir = self.tmp_dir()
>          ns = network_state.parse_net_config_data(self.mycfg,

--
https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.launchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Cf2afe4f90815495139aa08d481d2ac90%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276189228313859&sdata=XdfR50OqWiiw%2BYeJLmyMfagkHlBWlNSBfQFPWwIHmp4%3D&reserved=0
You are the owner of ~redriver/cloud-init:frbsd-azure-branch.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-04-13:

#

PASSED: Continuous integration, rev:6a0c7e539c5131e756e1485e292d2f77a22fbac2
https://jenkins.ubuntu.com/server/job/cloud-init-ci/252/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/252
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/252
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/252
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/252
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/252

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/252/rebuild

review: Approve (continuous-integration)

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-04-13:

#

PASSED: Continuous integration, rev:bba43d77e4c44e0a38f3c12b46f741b0a380c509
https://jenkins.ubuntu.com/server/job/cloud-init-ci/253/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/253
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/253
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/253
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/253
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/253

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/253/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-04-14:

#

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?

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.launchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e130608d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserved=0
You are the owner of ~redriver/cloud-init:frbsd-azure-branch.

Revision history for this message

Scott Moser (smoser) wrote on 2017-04-14:

#

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.launchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e130608d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserved=0
> You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-04-14:

#

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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: mp+314895@code.launchpad.net
> 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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-04-25:

#

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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

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 <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: mp+314895@code.launchpad.net
> 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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

~redriver/cloud-init:frbsd-azure-branch updated on 2017-04-25

c1cf3ad... by Hongjiang Zhang on 2017-04-25

Remove Azure specific check

Take the get_mount_info for Azure as a common case on FreeBSD.
This was only triggered if the device info '/dev/xxx' is illegal.

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-04-25:

#

PASSED: Continuous integration, rev:c1cf3ad1fc125a8087cf7e6501a781ddf9556345
https://jenkins.ubuntu.com/server/job/cloud-init-ci/279/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/279
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/279
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/279
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/279
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/279

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/279/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-04-25:

#

Download full text (5.4 KiB)

By the way, I 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>
Subj...

By the way, I 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' <smoser@ubuntu.com>
Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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 <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: mp+314895@code.launchpad.net
> 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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

Revision history for this message

Kylie Liang (kyliel) wrote on 2017-04-27:

#

Download full text (3.2 KiB)

Hi Scott,

Thank you for your effort. Could you please help take minutes to see any else we should do to get the patch merged? Feel free let us know your thoughts. It is our highest priority to get this work for BSD users. Your support is appreciated. Thank you.

Thanks,
Kylie Liang

-----Original Message-----
From: Hongjiang Zhang
Sent: Tuesday, April 25, 2017 11:38 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

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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=q...

Hi Scott,

Thank you for your effort. Could you please help take minutes to see any else we should do to get the patch merged? Feel free let us know your thoughts. It is our highest priority to get this work for BSD users. Your support is appreciated. Thank you.

Thanks,
Kylie Liang

-----Original Message-----
From: Hongjiang Zhang 
Sent: Tuesday, April 25, 2017 11:38 AM
To: Scott Moser <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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 <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: mp+314895@code.launchpad.net
> 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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

~redriver/cloud-init:frbsd-azure-branch updated on 2017-04-27

1b1e0a8... by Hongjiang Zhang on 2017-04-27: fix the issue of 'ssh does not exist in /etc/rc.d'

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-04-27:

#

PASSED: Continuous integration, rev:1b1e0a8fe6547bd29f5d0313567b8e8785dec736
https://jenkins.ubuntu.com/server/job/cloud-init-ci/288/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/288
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/288
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/288
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-s390x/288
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/288

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/288/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-05-02:

#

Download full text (5.4 KiB)

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

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' <smoser@ubuntu.com>
Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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 <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: mp+314895@code.launchpad.net
> 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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-05-04:

#

Download full text (6.1 KiB)

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

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

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

-----Original Message-----
From: Hongjiang Zhang 
Sent: Tuesday, May 2, 2017 10:26 AM
To: 'Scott Moser' <smoser@ubuntu.com>; 'scott.moser@canonical.com' <scott.moser@canonical.com>
Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
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' <smoser@ubuntu.com>
Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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 <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Friday, April 14, 2017 10:46 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> Of Scott Moser
> Sent: Thursday, April 13, 2017 2:51 AM
> To: mp+314895@code.launchpad.net
> 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%2Bmerge%
> 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
>

Revision history for this message

Scott Moser (smoser) wrote on 2017-05-04:

#

Download full text (7.0 KiB)

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://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
[2] https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
>
> -----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
> + ...

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://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
[2] https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+ref/cl_on_Azure_0.7.9-fixups
>
> -----Original Message-----
> From: Hongjiang Zhang
> Sent: Tuesday, May 2, 2017 10:26 AM
> To: 'Scott Moser' <smoser@ubuntu.com>; 'scott.moser@canonical.com' <scott.moser@canonical.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
> 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' <smoser@ubuntu.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
> 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 <smoser@ubuntu.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
> 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:ssmoser2@gmail.com] On Behalf Of Scott Moser
> Sent: Friday, April 14, 2017 10:46 AM
> To: Hongjiang Zhang <honzhan@microsoft.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; Josh Poulson <jopoulso@microsoft.com>
> 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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf
> > Of Scott Moser
> > Sent: Thursday, April 13, 2017 2:51 AM
> > To: mp+314895@code.launchpad.net
> > 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%2Bmerge%
> > 2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7Ca62135a0b9514c7e1306
> > 08d481d4cf8c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636276198391
> > 371945&sdata=qGDZNXUPvGHGs3%2ByT685EVs8jwnZQ12YIWBT%2BBrMPb8%3D&reserv
> > ed=0 You are the owner of ~redriver/cloud-init:frbsd-azure-branch.
> >
>
>

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-05-04:

#

Download full text (8.8 KiB)

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

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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Thursday, May 4, 2017 11:56 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Josh Poulson <jopoulso@microsoft.com>; Kylie Liang <kyliel@microsoft.com>
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' <smoser@ubuntu.com>; 'scott.moser@canonical.com' 
> <scott.moser@canonical.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
> 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' <smoser@ubuntu.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; 
> Kylie Liang <kyliel@microsoft.com>; Josh Poulson 
> <jopoulso@microsoft.com>
> 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 <smoser@ubuntu.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; 
> Josh Poulson <jopoulso@microsoft.com>
> 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:ssmoser2@gmail.com] On Behalf Of Scott Moser
> Sent: Friday, April 14, 2017 10:46 AM
> To: Hongjiang Zhang <honzhan@microsoft.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; 
> Josh Poulson <jopoulso@microsoft.com>
> 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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> > Of Scott Moser
> > Sent: Thursday, April 13, 2017 2:51 AM
> > To: mp+314895@code.launchpad.net
> > 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.
> >
>
>

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-05-04:

#

Download full text (10.0 KiB)

Hi Scott,

Sorry for making you confused. Thanks for your reviewing on the branch cl_on_Azure_0.7.9, which is totally for FreeBSD ports, and that patch mainly comes from merge request of 314895.

I hope the 1st step is to make cloud-init head work for FreeBSD on Azure, and the 2nd step is to make FreeBSD ports work.

Could you please focus on my merge request of 314895?

>diff --git a/requirements.txt b/requirements.txt
>index 2559b31..0c4951f 100644
>--- a/requirements.txt
>+++ b/requirements.txt
>@@ -28,7 +28,7 @@ configobj>=5.0.2
> pyyaml
>
> # The new main entrypoint uses argparse instead of optparse
>-# argparse
>+argparse
>
> # Requests handles ssl correctly!
> requests

I removed "argparse" because I found FreeBSD ports will fail to run. The reason is python 2.7 has made argparse builtin library. FreeBSD currently python version is 2.7, so, that is a FreeBSD specific change.
That is why I create FreeBSD ports to hold that change.

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang
Sent: Thursday, May 4, 2017 12:49 PM
To: 'Scott Moser' <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

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

Hi Scott,

Sorry for making you confused. Thanks for your reviewing on the branch cl_on_Azure_0.7.9, which is totally for FreeBSD ports, and that patch mainly comes from merge request of 314895.

I hope the 1st step is to make cloud-init head work for FreeBSD on Azure, and the 2nd step is to make FreeBSD ports work.

Could you please focus on my merge request of 314895?

>diff --git a/requirements.txt b/requirements.txt
>index 2559b31..0c4951f 100644
>--- a/requirements.txt
>+++ b/requirements.txt
>@@ -28,7 +28,7 @@ configobj>=5.0.2
> pyyaml
>
> # The new main entrypoint uses argparse instead of optparse
>-# argparse
>+argparse
>
> # Requests handles ssl correctly!
> requests

I removed "argparse" because I found FreeBSD ports will fail to run. The reason is python 2.7 has made argparse builtin library. FreeBSD currently python version is 2.7, so, that is a FreeBSD specific change.
That is why I create FreeBSD ports to hold that change.

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang 
Sent: Thursday, May 4, 2017 12:49 PM
To: 'Scott Moser' <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Josh Poulson <jopoulso@microsoft.com>; Kylie Liang <kyliel@microsoft.com>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Thursday, May 4, 2017 11:56 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Josh Poulson <jopoulso@microsoft.com>; Kylie Liang <kyliel@microsoft.com>
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' <smoser@ubuntu.com>; 'scott.moser@canonical.com' 
> <scott.moser@canonical.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
> 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' <smoser@ubuntu.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; 
> Kylie Liang <kyliel@microsoft.com>; Josh Poulson 
> <jopoulso@microsoft.com>
> 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 <smoser@ubuntu.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; 
> Josh Poulson <jopoulso@microsoft.com>
> 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:ssmoser2@gmail.com] On Behalf Of Scott Moser
> Sent: Friday, April 14, 2017 10:46 AM
> To: Hongjiang Zhang <honzhan@microsoft.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; 
> Josh Poulson <jopoulso@microsoft.com>
> 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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> > Of Scott Moser
> > Sent: Thursday, April 13, 2017 2:51 AM
> > To: mp+314895@code.launchpad.net
> > 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.
> >
>
>

~redriver/cloud-init:frbsd-azure-branch updated on 2017-05-04

77669d5... by Hongjiang Zhang on 2017-05-04: modify according to Scott comments

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-05-04:

#

PASSED: Continuous integration, rev:77669d5afd5686d07268ce9f51e9e5ea1c46fc83
https://jenkins.ubuntu.com/server/job/cloud-init-ci/303/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/303
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/303
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/303
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/303

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/303/rebuild

review: Approve (continuous-integration)

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-05-04:

#

Download full text (10.5 KiB)

Hi Scott,

I did not propose the branch of cl_on_Azure_0.7.9. Instead, I manually merged your diff to my existing merge request (https://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+merge/314895)

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang
Sent: Thursday, May 4, 2017 1:09 PM
To: 'Scott Moser' <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,

Sorry for making you confused. Thanks for your reviewing on the branch cl_on_Azure_0.7.9, which is totally for FreeBSD ports, and that patch mainly comes from merge request of 314895.

I hope the 1st step is to make cloud-init head work for FreeBSD on Azure, and the 2nd step is to make FreeBSD ports work.

Could you please focus on my merge request of 314895?

>diff --git a/requirements.txt b/requirements.txt index 2559b31..0c4951f
>100644
>--- a/requirements.txt
>+++ b/requirements.txt
>@@ -28,7 +28,7 @@ configobj>=5.0.2
> pyyaml
>
> # The new main entrypoint uses argparse instead of optparse -#
>argparse
>+argparse
>
> # Requests handles ssl correctly!
> requests

I removed "argparse" because I found FreeBSD ports will fail to run. The reason is python 2.7 has made argparse builtin library. FreeBSD currently python version is 2.7, so, that is a FreeBSD specific change.
That is why I create FreeBSD ports to hold that change.

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang
Sent: Thursday, May 4, 2017 12:49 PM
To: 'Scott Moser' <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

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

Hi Scott,

I did not propose the branch of cl_on_Azure_0.7.9. Instead, I manually merged your diff to my existing merge request (https://code.launchpad.net/~redriver/cloud-init/+git/cloud-init/+merge/314895)

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang 
Sent: Thursday, May 4, 2017 1:09 PM
To: 'Scott Moser' <smoser@ubuntu.com>
Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

Hi Scott,

Sorry for making you confused. Thanks for your reviewing on the branch cl_on_Azure_0.7.9, which is totally for FreeBSD ports, and that patch mainly comes from merge request of 314895.

I hope the 1st step is to make cloud-init head work for FreeBSD on Azure, and the 2nd step is to make FreeBSD ports work.

Could you please focus on my merge request of 314895?

>diff --git a/requirements.txt b/requirements.txt index 2559b31..0c4951f 
>100644
>--- a/requirements.txt
>+++ b/requirements.txt
>@@ -28,7 +28,7 @@ configobj>=5.0.2
> pyyaml
>
> # The new main entrypoint uses argparse instead of optparse -# 
>argparse
>+argparse
>
> # Requests handles ssl correctly!
> requests

I removed "argparse" because I found FreeBSD ports will fail to run. The reason is python 2.7 has made argparse builtin library. FreeBSD currently python version is 2.7, so, that is a FreeBSD specific change.
That is why I create FreeBSD ports to hold that change.

Thanks
Hongjiang Zhang

-----Original Message-----
From: Hongjiang Zhang
Sent: Thursday, May 4, 2017 12:49 PM
To: 'Scott Moser' <smoser@ubuntu.com>
Cc: mp+314895@code.launchpad.net; Josh Poulson <jopoulso@microsoft.com>; Kylie Liang <kyliel@microsoft.com>
Subject: RE: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

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:ssmoser2@gmail.com] On Behalf Of Scott Moser
Sent: Thursday, May 4, 2017 11:56 AM
To: Hongjiang Zhang <honzhan@microsoft.com>
Cc: mp+314895@code.launchpad.net; Josh Poulson <jopoulso@microsoft.com>; Kylie Liang <kyliel@microsoft.com>
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' <smoser@ubuntu.com>; 'scott.moser@canonical.com' 
> <scott.moser@canonical.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>
> 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' <smoser@ubuntu.com>
> Cc: 'mp+314895@code.launchpad.net' <mp+314895@code.launchpad.net>; 
> Kylie Liang <kyliel@microsoft.com>; Josh Poulson 
> <jopoulso@microsoft.com>
> 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 <smoser@ubuntu.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; 
> Josh Poulson <jopoulso@microsoft.com>
> 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:ssmoser2@gmail.com] On Behalf Of Scott Moser
> Sent: Friday, April 14, 2017 10:46 AM
> To: Hongjiang Zhang <honzhan@microsoft.com>
> Cc: mp+314895@code.launchpad.net; Kylie Liang <kyliel@microsoft.com>; 
> Josh Poulson <jopoulso@microsoft.com>
> 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: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf 
> > Of Scott Moser
> > Sent: Thursday, April 13, 2017 2:51 AM
> > To: mp+314895@code.launchpad.net
> > 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.
> >
>
>

~redriver/cloud-init:frbsd-azure-branch updated on 2017-05-08

0ff61d4... by Hongjiang Zhang on 2017-05-08: remove Linux specific installation on FreeBSD

Revision history for this message

Server Team CI bot (server-team-bot) wrote on 2017-05-08:

#

PASSED: Continuous integration, rev:0ff61d4ed2e69da5cb3c98938491188c77da83d2
https://jenkins.ubuntu.com/server/job/cloud-init-ci/304/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-amd64/304
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-arm64/304
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=metal-ppc64el/304
    SUCCESS: https://jenkins.ubuntu.com/server/job/cloud-init-ci/nodes=vm-i386/304

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/304/rebuild

review: Approve (continuous-integration)

Revision history for this message

Scott Moser (smoser) wrote on 2017-05-10:

#

I've merged this.
The diff from the merged commit (0a71d5a870b416f2c86c8bc196004bb3fc0768a0)
to your tip (0ff61d4) when rebased is just:

diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
index d9f31bfd..bad112fe 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -272,7 +272,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', '-l']
         (nics, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
- LOG.warn("Error running %s: %s", cmd, err)
+ LOG.warning("Error running %s: %s", cmd, err)
             return None
         return nics

@@ -281,7 +281,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', ifname]
         (if_result, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
- LOG.warn("Error running %s: %s", cmd, err)
+ LOG.warning("Error running %s: %s", cmd, err)
             return None
         return if_result

@@ -290,7 +290,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', '-l', 'ether']
         (nics, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
- LOG.warn("Error running %s: %s", cmd, err)
+ LOG.warning("Error running %s: %s", cmd, err)
             return None
         return nics

diff --git a/setup.py b/setup.py
index 46d05219..4616599b 100755
--- a/setup.py
+++ b/setup.py
@@ -173,8 +173,9 @@ else:
             [f for f in glob('doc/examples/seed/*') if is_f(f)]),
     ]
     if os.uname()[0] != 'FreeBSD':
- data_files.append([
- (ETC + '/NetworkManager/dispatcher.d/', ['tools/hook-network-manager']),
+ data_files.extend([
+ (ETC + '/NetworkManager/dispatcher.d/',
+ ['tools/hook-network-manager']),
             (ETC + '/dhcp/dhclient-exit-hooks.d/', ['tools/hook-dhclient']),
             (LIB + '/udev/rules.d', [f for f in glob('udev/*.rules')])
         ])

review: Approve

Revision history for this message

Hongjiang Zhang (redriver) wrote on 2017-05-11:

#

Hi Scott,

Great to see it is merged finally!

I appreciated all your review and help in the past 2 months.

Thanks
Hongjiang Zhang

-----Original Message-----
From: <email address hidden> [mailto:<email address hidden>] On Behalf Of Scott Moser
Sent: Thursday, May 11, 2017 1:05 AM
To: <email address hidden>
Subject: Re: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

Review: Approve

I've merged this.
The diff from the merged commit (0a71d5a870b416f2c86c8bc196004bb3fc0768a0)
to your tip (0ff61d4) when rebased is just:

diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index d9f31bfd..bad112fe 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -272,7 +272,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', '-l']
         (nics, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
- LOG.warn("Error running %s: %s", cmd, err)
+ LOG.warning("Error running %s: %s", cmd, err)
             return None
         return nics

@@ -281,7 +281,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', ifname]
         (if_result, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
- LOG.warn("Error running %s: %s", cmd, err)
+ LOG.warning("Error running %s: %s", cmd, err)
             return None
         return if_result

@@ -290,7 +290,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', '-l', 'ether']
         (nics, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
- LOG.warn("Error running %s: %s", cmd, err)
+ LOG.warning("Error running %s: %s", cmd, err)
             return None
         return nics

diff --git a/setup.py b/setup.py
index 46d05219..4616599b 100755
--- a/setup.py
+++ b/setup.py
@@ -173,8 +173,9 @@ else:
             [f for f in glob('doc/examples/seed/*') if is_f(f)]),
     ]
     if os.uname()[0] != 'FreeBSD':
- data_files.append([
- (ETC + '/NetworkManager/dispatcher.d/', ['tools/hook-network-manager']),
+ data_files.extend([
+ (ETC + '/NetworkManager/dispatcher.d/',
+ ['tools/hook-network-manager']),
             (ETC + '/dhcp/dhclient-exit-hooks.d/', ['tools/hook-dhclient']),
             (LIB + '/udev/rules.d', [f for f in glob('udev/*.rules')])
         ])

--
https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.launchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7C6ecd4fba7ef945442b5008d497c6a74e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636300326842539159&sdata=LLwO0W1%2F0bm4EoNr%2BFlbUZmPEphENyRUv1cF1IafsN8%3D&reserved=0
You are the owner of ~redriver/cloud-init:frbsd-azure-branch.

Hi Scott,

Great to see it is merged finally!

I appreciated all your review and help in the past 2 months.

Thanks
Hongjiang Zhang

-----Original Message-----
From: bounces@canonical.com [mailto:bounces@canonical.com] On Behalf Of Scott Moser
Sent: Thursday, May 11, 2017 1:05 AM
To: mp+314895@code.launchpad.net
Subject: Re: [Merge] ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

Review: Approve

I've merged this.
The diff from the merged commit (0a71d5a870b416f2c86c8bc196004bb3fc0768a0)
to your tip (0ff61d4) when rebased is just:

diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py index d9f31bfd..bad112fe 100644
--- a/cloudinit/distros/freebsd.py
+++ b/cloudinit/distros/freebsd.py
@@ -272,7 +272,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', '-l']
         (nics, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
-            LOG.warn("Error running %s: %s", cmd, err)
+            LOG.warning("Error running %s: %s", cmd, err)
             return None
         return nics
 
@@ -281,7 +281,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', ifname]
         (if_result, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
-            LOG.warn("Error running %s: %s", cmd, err)
+            LOG.warning("Error running %s: %s", cmd, err)
             return None
         return if_result
 
@@ -290,7 +290,7 @@ class Distro(distros.Distro):
         cmd = ['ifconfig', '-l', 'ether']
         (nics, err) = util.subp(cmd, rcs=[0, 1])
         if len(err):
-            LOG.warn("Error running %s: %s", cmd, err)
+            LOG.warning("Error running %s: %s", cmd, err)
             return None
         return nics
 
diff --git a/setup.py b/setup.py
index 46d05219..4616599b 100755
--- a/setup.py
+++ b/setup.py
@@ -173,8 +173,9 @@ else:
             [f for f in glob('doc/examples/seed/*') if is_f(f)]),
     ]
     if os.uname()[0] != 'FreeBSD':
-        data_files.append([
-            (ETC + '/NetworkManager/dispatcher.d/', ['tools/hook-network-manager']),
+        data_files.extend([
+            (ETC + '/NetworkManager/dispatcher.d/',
+             ['tools/hook-network-manager']),
             (ETC + '/dhcp/dhclient-exit-hooks.d/', ['tools/hook-dhclient']),
             (LIB + '/udev/rules.d', [f for f in glob('udev/*.rules')])
         ])

--
https://na01.safelinks.protection.outlook.com/?url=https:%2F%2Fcode.launchpad.net%2F~redriver%2Fcloud-init%2F%2Bgit%2Fcloud-init%2F%2Bmerge%2F314895&data=02%7C01%7Chonzhan%40microsoft.com%7C6ecd4fba7ef945442b5008d497c6a74e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636300326842539159&sdata=LLwO0W1%2F0bm4EoNr%2BFlbUZmPEphENyRUv1cF1IafsN8%3D&reserved=0
You are the owner of ~redriver/cloud-init:frbsd-azure-branch.

cloud-init

Merge ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master

Commit message

Description of the change

Preview Diff

Subscribers

 diff --git a/cloudinit/config/cc_resizefs.py b/cloudinit/config/cc_resizefs.py
 index 60e3ab5..ceee952 100644
 --- a/cloudinit/config/cc_resizefs.py
 +++ b/cloudinit/config/cc_resizefs.py
@@ -33,7 +33,10 @@ disabled altogether by setting ``resize_rootfs`` to ``false``.
  """
  import errno
++import getopt
  import os
++import re
++import shlex
  import stat
  from cloudinit.settings import PER_ALWAYS
@@ -58,6 +61,62 @@ def _resize_ufs(mount_point, devpth):
      return ('growfs', devpth)
++def _get_dumpfs_output(mount_point):
++    dumpfs_res, err = util.subp(['dumpfs', '-m', mount_point])
++    return dumpfs_res
++
++
++def _get_gpart_output(part):
++    gpart_res, err = util.subp(['gpart', 'show', part])
++    return gpart_res
++
++
++def _can_skip_resize_ufs(mount_point, devpth):
++    # extract the current fs sector size
++    """
++    # dumpfs -m /
++    # newfs command for / (/dev/label/rootfs)
++      newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 -f 4096 -g 16384
++            -h 64 -i 8192 -j -k 6408 -m 8 -o time -s 58719232 /dev/label/rootf
++    """
++    cur_fs_sz = None
++    frag_sz = None
++    dumpfs_res = _get_dumpfs_output(mount_point)
++    for line in dumpfs_res.splitlines():
++        if not line.startswith('#'):
++            newfs_cmd = shlex.split(line)
++            opt_value = 'O:Ua:s:b:d:e:f:g:h:i:jk:m:o:'
++            optlist, args = getopt.getopt(newfs_cmd[1:], opt_value)
++            for o, a in optlist:
++                if o == "-s":
++                    cur_fs_sz = int(a)
++                if o == "-f":
++                    frag_sz = int(a)
++    # check the current partition size
++    """
++    # gpart show /dev/da0
++=>      40  62914480  da0  GPT  (30G)
++        40      1024    1  freebsd-boot  (512K)
++      1064  58719232    2  freebsd-ufs  (28G)
++  58720296   3145728    3  freebsd-swap  (1.5G)
++  61866024   1048496       - free -  (512M)
++    """
++    expect_sz = None
++    m = re.search('^(/dev/.+)p([0-9])$', devpth)
++    gpart_res = _get_gpart_output(m.group(1))
++    for line in gpart_res.splitlines():
++        if re.search(r"freebsd-ufs", line):
++            fields = line.split()
++            expect_sz = int(fields[1])
++    # Normalize the gpart sector size,
++    # because the size is not exactly the same as fs size.
++    normal_expect_sz = (expect_sz - expect_sz % (frag_sz / 512))
++    if normal_expect_sz == cur_fs_sz:
++        return True
++    else:
++        return False
++
++
  # Do not use a dictionary as these commands should be able to be used
  # for multiple filesystem types if possible, e.g. one command for
  # ext2, ext3 and ext4.
@@ -68,9 +127,40 @@ RESIZE_FS_PREFIXES_CMDS = [
      ('ufs', _resize_ufs),
+ ]
++RESIZE_FS_PRECHECK_CMDS = {
++    'ufs': _can_skip_resize_ufs
++}
++
  NOBLOCK = "noblock"
++def rootdev_from_cmdline(cmdline):
++    found = None
++    for tok in cmdline.split():
++        if tok.startswith("root="):
++            found = tok[5:]
++            break
++    if found is None:
++        return None
++
++    if found.startswith("/dev/"):
++        return found
++    if found.startswith("LABEL="):
++        return "/dev/disk/by-label/" + found[len("LABEL="):]
++    if found.startswith("UUID="):
++        return "/dev/disk/by-uuid/" + found[len("UUID="):]
++
++    return "/dev/" + found
++
++
++def can_skip_resize(fs_type, resize_what, devpth):
++    fstype_lc = fs_type.lower()
++    for i, func in RESIZE_FS_PRECHECK_CMDS.items():
++        if fstype_lc.startswith(i):
++            return func(resize_what, devpth)
++    return False
++
++
  def handle(name, cfg, _cloud, log, args):
      if len(args) != 0:
          resize_root = args[0]
@@ -139,6 +229,11 @@ def handle(name, cfg, _cloud, log, args):
          return
      resizer = None
++    if can_skip_resize(fs_type, resize_what, devpth):
++        log.debug("Skip resize filesystem type %s for %s",
++                  fs_type, resize_what)
++        return
++
      fstype_lc = fs_type.lower()
      for (pfix, root_cmd) in RESIZE_FS_PREFIXES_CMDS:
          if fstype_lc.startswith(pfix):
 diff --git a/cloudinit/distros/__init__.py b/cloudinit/distros/__init__.py
 index 28650b8..f56c0cf 100755
 --- a/cloudinit/distros/__init__.py
 +++ b/cloudinit/distros/__init__.py
@@ -155,6 +155,9 @@ class Distro(object):
              ns, header=header, render_hwaddress=True)
          return self.apply_network(contents, bring_up=bring_up)
++    def generate_fallback_config(self):
++        return net.generate_fallback_config()
++
      def apply_network_config(self, netconfig, bring_up=False):
          # apply network config netconfig
          # This method is preferred to apply_network which only takes
 diff --git a/cloudinit/distros/freebsd.py b/cloudinit/distros/freebsd.py
 index 183e445..d9f31bf 100644
 --- a/cloudinit/distros/freebsd.py
 +++ b/cloudinit/distros/freebsd.py
@@ -30,6 +30,7 @@ class Distro(distros.Distro):
      login_conf_fn_bak = '/etc/login.conf.orig'
      resolv_conf_fn = '/etc/resolv.conf'
      ci_sudoers_fn = '/usr/local/etc/sudoers.d/90-cloud-init-users'
++    default_primary_nic = 'hn0'
      def __init__(self, name, cfg, paths):
          distros.Distro.__init__(self, name, cfg, paths)
@@ -38,6 +39,8 @@ class Distro(distros.Distro):
          # should only happen say once per instance...)
          self._runner = helpers.Runners(paths)
          self.osfamily = 'freebsd'
++        self.ipv4_pat = re.compile(r"\s+inet\s+\d+[.]\d+[.]\d+[.]\d+")
++        cfg['ssh_svcname'] = 'sshd'
      # Updates a key in /etc/rc.conf.
      def updatercconf(self, key, value):
@@ -183,7 +186,6 @@ class Distro(distros.Distro):
              "gecos": '-c',
              "primary_group": '-g',
              "groups": '-G',
--            "passwd": '-h',
              "shell": '-s',
              "inactive": '-E',
+         }
@@ -193,19 +195,11 @@ class Distro(distros.Distro):
              "no_log_init": '--no-log-init',
+         }
--        redact_opts = ['passwd']
--
          for key, val in kwargs.items():
              if (key in adduser_opts and val and
                 isinstance(val, six.string_types)):
                  adduser_cmd.extend([adduser_opts[key], val])
--                # Redact certain fields from the logs
--                if key in redact_opts:
--                    log_adduser_cmd.extend([adduser_opts[key], 'REDACTED'])
--                else:
--                    log_adduser_cmd.extend([adduser_opts[key], val])
--
              elif key in adduser_flags and val:
                  adduser_cmd.append(adduser_flags[key])
                  log_adduser_cmd.append(adduser_flags[key])
@@ -226,19 +220,21 @@ class Distro(distros.Distro):
          except Exception as e:
              util.logexc(LOG, "Failed to create user %s", name)
              raise e
++        # Set the password if it is provided
++        # For security consideration, only hashed passwd is assumed
++        passwd_val = kwargs.get('passwd', None)
++        if passwd_val is not None:
++            self.set_passwd(name, passwd_val, hashed=True)
      def set_passwd(self, user, passwd, hashed=False):
--        cmd = ['pw', 'usermod', user]
--
          if hashed:
--            cmd.append('-H')
++            hash_opt = "-H"
          else:
--            cmd.append('-h')
--
--        cmd.append('0')
++            hash_opt = "-h"
          try:
--            util.subp(cmd, passwd, logstring="chpasswd for %s" % user)
++            util.subp(['pw', 'usermod', user, hash_opt, '0'],
++                      data=passwd, logstring="chpasswd for %s" % user)
          except Exception as e:
              util.logexc(LOG, "Failed to set password for %s", user)
              raise e
@@ -271,6 +267,255 @@ class Distro(distros.Distro):
              keys = set(kwargs['ssh_authorized_keys']) or []
              ssh_util.setup_user_keys(keys, name, options=None)
++    @staticmethod
++    def get_ifconfig_list():
++        cmd = ['ifconfig', '-l']
++        (nics, err) = util.subp(cmd, rcs=[0, 1])
++        if len(err):
++            LOG.warn("Error running %s: %s", cmd, err)
++            return None
++        return nics
++
++    @staticmethod
++    def get_ifconfig_ifname_out(ifname):
++        cmd = ['ifconfig', ifname]
++        (if_result, err) = util.subp(cmd, rcs=[0, 1])
++        if len(err):
++            LOG.warn("Error running %s: %s", cmd, err)
++            return None
++        return if_result
++
++    @staticmethod
++    def get_ifconfig_ether():
++        cmd = ['ifconfig', '-l', 'ether']
++        (nics, err) = util.subp(cmd, rcs=[0, 1])
++        if len(err):
++            LOG.warn("Error running %s: %s", cmd, err)
++            return None
++        return nics
++
++    @staticmethod
++    def get_interface_mac(ifname):
++        if_result = Distro.get_ifconfig_ifname_out(ifname)
++        for item in if_result.splitlines():
++            if item.find('ether ') != -1:
++                mac = str(item.split()[1])
++                if mac:
++                    return mac
++
++    @staticmethod
++    def get_devicelist():
++        nics = Distro.get_ifconfig_list()
++        return nics.split()
++
++    @staticmethod
++    def get_ipv6():
++        ipv6 = []
++        nics = Distro.get_devicelist()
++        for nic in nics:
++            if_result = Distro.get_ifconfig_ifname_out(nic)
++            for item in if_result.splitlines():
++                if item.find("inet6 ") != -1 and item.find("scopeid") == -1:
++                    ipv6.append(nic)
++        return ipv6
++
++    def get_ipv4(self):
++        ipv4 = []
++        nics = Distro.get_devicelist()
++        for nic in nics:
++            if_result = Distro.get_ifconfig_ifname_out(nic)
++            for item in if_result.splitlines():
++                print(item)
++                if self.ipv4_pat.match(item):
++                    ipv4.append(nic)
++        return ipv4
++
++    def is_up(self, ifname):
++        if_result = Distro.get_ifconfig_ifname_out(ifname)
++        pat = "^" + ifname
++        for item in if_result.splitlines():
++            if re.match(pat, item):
++                flags = item.split('<')[1].split('>')[0]
++                if flags.find("UP") != -1:
++                    return True
++
++    def _get_current_rename_info(self, check_downable=True):
++        """Collect information necessary for rename_interfaces."""
++        names = Distro.get_devicelist()
++        bymac = {}
++        for n in names:
++            bymac[Distro.get_interface_mac(n)] = {
++                'name': n, 'up': self.is_up(n), 'downable': None}
++
++        if check_downable:
++            nics_with_addresses = set()
++            ipv6 = self.get_ipv6()
++            ipv4 = self.get_ipv4()
++            for bytes_out in (ipv6, ipv4):
++                for i in ipv6:
++                    nics_with_addresses.update(i)
++                for i in ipv4:
++                    nics_with_addresses.update(i)
++
++        for d in bymac.values():
++            d['downable'] = (d['up'] is False or
++                             d['name'] not in nics_with_addresses)
++
++        return bymac
++
++    def _rename_interfaces(self, renames):
++        if not len(renames):
++            LOG.debug("no interfaces to rename")
++            return
++
++        current_info = self._get_current_rename_info()
++
++        cur_bymac = {}
++        for mac, data in current_info.items():
++            cur = data.copy()
++            cur['mac'] = mac
++            cur_bymac[mac] = cur
++
++        def update_byname(bymac):
++            return dict((data['name'], data)
++                        for data in bymac.values())
++
++        def rename(cur, new):
++            util.subp(["ifconfig", cur, "name", new], capture=True)
++
++        def down(name):
++            util.subp(["ifconfig", name, "down"], capture=True)
++
++        def up(name):
++            util.subp(["ifconfig", name, "up"], capture=True)
++
++        ops = []
++        errors = []
++        ups = []
++        cur_byname = update_byname(cur_bymac)
++        tmpname_fmt = "cirename%d"
++        tmpi = -1
++
++        for mac, new_name in renames:
++            cur = cur_bymac.get(mac, {})
++            cur_name = cur.get('name')
++            cur_ops = []
++            if cur_name == new_name:
++                # nothing to do
++                continue
++
++            if not cur_name:
++                errors.append("[nic not present] Cannot rename mac=%s to %s"
++                              ", not available." % (mac, new_name))
++                continue
++
++            if cur['up']:
++                msg = "[busy] Error renaming mac=%s from %s to %s"
++                if not cur['downable']:
++                    errors.append(msg % (mac, cur_name, new_name))
++                    continue
++                cur['up'] = False
++                cur_ops.append(("down", mac, new_name, (cur_name,)))
++                ups.append(("up", mac, new_name, (new_name,)))
++
++            if new_name in cur_byname:
++                target = cur_byname[new_name]
++                if target['up']:
++                    msg = "[busy-target] Error renaming mac=%s from %s to %s."
++                    if not target['downable']:
++                        errors.append(msg % (mac, cur_name, new_name))
++                        continue
++                    else:
++                        cur_ops.append(("down", mac, new_name, (new_name,)))
++
++                tmp_name = None
++                while tmp_name is None or tmp_name in cur_byname:
++                    tmpi += 1
++                    tmp_name = tmpname_fmt % tmpi
++
++                cur_ops.append(("rename", mac, new_name, (new_name, tmp_name)))
++                target['name'] = tmp_name
++                cur_byname = update_byname(cur_bymac)
++                if target['up']:
++                    ups.append(("up", mac, new_name, (tmp_name,)))
++
++            cur_ops.append(("rename", mac, new_name, (cur['name'], new_name)))
++            cur['name'] = new_name
++            cur_byname = update_byname(cur_bymac)
++            ops += cur_ops
++
++        opmap = {'rename': rename, 'down': down, 'up': up}
++        if len(ops) + len(ups) == 0:
++            if len(errors):
++                LOG.debug("unable to do any work for renaming of %s", renames)
++            else:
++                LOG.debug("no work necessary for renaming of %s", renames)
++        else:
++            LOG.debug("achieving renaming of %s with ops %s",
++                      renames, ops + ups)
++
++            for op, mac, new_name, params in ops + ups:
++                try:
++                    opmap.get(op)(*params)
++                except Exception as e:
++                    errors.append(
++                        "[unknown] Error performing %s%s for %s, %s: %s" %
++                        (op, params, mac, new_name, e))
++        if len(errors):
++            raise Exception('\n'.join(errors))
++
++    def apply_network_config_names(self, netcfg):
++        renames = []
++        for ent in netcfg.get('config', {}):
++            if ent.get('type') != 'physical':
++                continue
++            mac = ent.get('mac_address')
++            name = ent.get('name')
++            if not mac:
++                continue
++            renames.append([mac, name])
++        return self._rename_interfaces(renames)
++
++    @classmethod
++    def generate_fallback_config(self):
++        nics = Distro.get_ifconfig_ether()
++        if nics is None:
++            LOG.debug("Fail to get network interfaces")
++            return None
++        potential_interfaces = nics.split()
++        connected = []
++        for nic in potential_interfaces:
++            pat = "^" + nic
++            if_result = Distro.get_ifconfig_ifname_out(nic)
++            for item in if_result.split("\n"):
++                if re.match(pat, item):
++                    flags = item.split('<')[1].split('>')[0]
++                    if flags.find("RUNNING") != -1:
++                        connected.append(nic)
++        if connected:
++            potential_interfaces = connected
++        names = list(sorted(potential_interfaces))
++        default_pri_nic = Distro.default_primary_nic
++        if default_pri_nic in names:
++            names.remove(default_pri_nic)
++            names.insert(0, default_pri_nic)
++        target_name = None
++        target_mac = None
++        for name in names:
++            mac = Distro.get_interface_mac(name)
++            if mac:
++                target_name = name
++                target_mac = mac
++                break
++        if target_mac and target_name:
++            nconf = {'config': [], 'version': 1}
++            nconf['config'].append(
++                {'type': 'physical', 'name': target_name,
++                 'mac_address': target_mac, 'subnets': [{'type': 'dhcp'}]})
++            return nconf
++        else:
++            return None
++
      def _write_network(self, settings):
          entries = net_util.translate_network(settings)
          nameservers = []
 diff --git a/cloudinit/settings.py b/cloudinit/settings.py
 index dbafead..411960d 100644
 --- a/cloudinit/settings.py
 +++ b/cloudinit/settings.py
@@ -39,7 +39,7 @@ CFG_BUILTIN = {
      ],
      'def_log_file': '/var/log/cloud-init.log',
      'log_cfgs': [],
--    'syslog_fix_perms': ['syslog:adm', 'root:adm'],
++    'syslog_fix_perms': ['syslog:adm', 'root:adm', 'root:wheel'],
      'system_info': {
          'paths': {
              'cloud_dir': '/var/lib/cloud',
 diff --git a/cloudinit/sources/DataSourceAzure.py b/cloudinit/sources/DataSourceAzure.py
 index 04358b7..5254e18 100644
 --- a/cloudinit/sources/DataSourceAzure.py
 +++ b/cloudinit/sources/DataSourceAzure.py
@@ -10,6 +10,7 @@ import crypt
  from functools import partial
  import os
  import os.path
++import re
  import time
  from xml.dom import minidom
  import xml.etree.ElementTree as ET
@@ -32,19 +33,160 @@ BOUNCE_COMMAND = [
  # azure systems will always have a resource disk, and 66-azure-ephemeral.rules
  # ensures that it gets linked to this path.
  RESOURCE_DISK_PATH = '/dev/disk/cloud/azure_resource'
++DEFAULT_PRIMARY_NIC = 'eth0'
++LEASE_FILE = '/var/lib/dhcp/dhclient.eth0.leases'
++DEFAULT_FS = 'ext4'
++
++
++def find_storvscid_from_sysctl_pnpinfo(sysctl_out, deviceid):
++    # extract the 'X' from dev.storvsc.X. if deviceid matches
++    """
++    dev.storvsc.1.%pnpinfo:
++        classid=32412632-86cb-44a2-9b5c-50d1417354f5
++        deviceid=00000000-0001-8899-0000-000000000000
++    """
++    for line in sysctl_out.splitlines():
++        if re.search(r"pnpinfo", line):
++            fields = line.split()
++            if len(fields) >= 3:
++                columns = fields[2].split('=')
++                if (len(columns) >= 2 and
++                        columns[0] == "deviceid" and
++                        columns[1].startswith(deviceid)):
++                    comps = fields[0].split('.')
++                    return comps[2]
++    return None
++
++
++def find_busdev_from_disk(camcontrol_out, disk_drv):
++    # find the scbusX from 'camcontrol devlist -b' output
++    # if disk_drv matches the specified disk driver, i.e. blkvsc1
++    """
++    scbus0 on ata0 bus 0
++    scbus1 on ata1 bus 0
++    scbus2 on blkvsc0 bus 0
++    scbus3 on blkvsc1 bus 0
++    scbus4 on storvsc2 bus 0
++    scbus5 on storvsc3 bus 0
++    scbus-1 on xpt0 bus 0
++    """
++    for line in camcontrol_out.splitlines():
++        if re.search(disk_drv, line):
++            items = line.split()
++            return items[0]
++    return None
++
++
++def find_dev_from_busdev(camcontrol_out, busdev):
++    # find the daX from 'camcontrol devlist' output
++    # if busdev matches the specified value, i.e. 'scbus2'
++    """
++    <Msft Virtual CD/ROM 1.0>          at scbus1 target 0 lun 0 (cd0,pass0)
++    <Msft Virtual Disk 1.0>            at scbus2 target 0 lun 0 (da0,pass1)
++    <Msft Virtual Disk 1.0>            at scbus3 target 1 lun 0 (da1,pass2)
++    """
++    for line in camcontrol_out.splitlines():
++        if re.search(busdev, line):
++            items = line.split('(')
++            if len(items) == 2:
++                dev_pass = items[1].split(',')
++                return dev_pass[0]
++    return None
++
++
++def get_dev_storvsc_sysctl():
++    try:
++        sysctl_out, err = util.subp(['sysctl', 'dev.storvsc'])
++    except util.ProcessExecutionError:
++        LOG.debug("Fail to execute sysctl dev.storvsc")
++        return None
++    return sysctl_out
++
++
++def get_camcontrol_dev_bus():
++    try:
++        camcontrol_b_out, err = util.subp(['camcontrol', 'devlist', '-b'])
++    except util.ProcessExecutionError:
++        LOG.debug("Fail to execute camcontrol devlist -b")
++        return None
++    return camcontrol_b_out
++
++
++def get_camcontrol_dev():
++    try:
++        camcontrol_out, err = util.subp(['camcontrol', 'devlist'])
++    except util.ProcessExecutionError:
++        LOG.debug("Fail to execute camcontrol devlist")
++        return None
++    return camcontrol_out
++
++
++def get_resource_disk_on_freebsd(port_id):
++    g0 = "00000000"
++    if port_id > 1:
++        g0 = "00000001"
++        port_id = port_id - 2
++    g1 = "000" + str(port_id)
++    g0g1 = "{0}-{1}".format(g0, g1)
++    """
++    search 'X' from
++       'dev.storvsc.X.%pnpinfo:
++           classid=32412632-86cb-44a2-9b5c-50d1417354f5
++           deviceid=00000000-0001-8899-0000-000000000000'
++    """
++    sysctl_out = get_dev_storvsc_sysctl()
++
++    storvscid = find_storvscid_from_sysctl_pnpinfo(sysctl_out, g0g1)
++    if not storvscid:
++        LOG.debug("Fail to find storvsc id from sysctl")
++        return None
++
++    camcontrol_b_out = get_camcontrol_dev_bus()
++    camcontrol_out = get_camcontrol_dev()
++    # try to find /dev/XX from 'blkvsc' device
++    blkvsc = "blkvsc{0}".format(storvscid)
++    scbusx = find_busdev_from_disk(camcontrol_b_out, blkvsc)
++    if scbusx:
++        devname = find_dev_from_busdev(camcontrol_out, scbusx)
++        if devname is None:
++            LOG.debug("Fail to find /dev/daX")
++            return None
++        return devname
++    # try to find /dev/XX from 'storvsc' device
++    storvsc = "storvsc{0}".format(storvscid)
++    scbusx = find_busdev_from_disk(camcontrol_b_out, storvsc)
++    if scbusx:
++        devname = find_dev_from_busdev(camcontrol_out, scbusx)
++        if devname is None:
++            LOG.debug("Fail to find /dev/daX")
++            return None
++        return devname
++    return None
++
++# update the FreeBSD specific information
++if util.is_FreeBSD():
++    DEFAULT_PRIMARY_NIC = 'hn0'
++    LEASE_FILE = '/var/db/dhclient.leases.hn0'
++    DEFAULT_FS = 'freebsd-ufs'
++    res_disk = get_resource_disk_on_freebsd(1)
++    if res_disk is not None:
++        LOG.debug("resource disk is not None")
++        RESOURCE_DISK_PATH = "/dev/" + res_disk
++    else:
++        LOG.debug("resource disk is None")
  BUILTIN_DS_CONFIG = {
      'agent_command': AGENT_START_BUILTIN,
      'data_dir': "/var/lib/waagent",
      'set_hostname': True,
      'hostname_bounce': {
--        'interface': 'eth0',
++        'interface': DEFAULT_PRIMARY_NIC,
          'policy': True,
          'command': BOUNCE_COMMAND,
          'hostname_command': 'hostname',
      },
      'disk_aliases': {'ephemeral0': RESOURCE_DISK_PATH},
--    'dhclient_lease_file': '/var/lib/dhcp/dhclient.eth0.leases',
++    'dhclient_lease_file': LEASE_FILE,
+ }
  BUILTIN_CLOUD_CONFIG = {
@@ -53,7 +195,7 @@ BUILTIN_CLOUD_CONFIG = {
                         'layout': [100],
                         'overwrite': True},
      },
--    'fs_setup': [{'filesystem': 'ext4',
++    'fs_setup': [{'filesystem': DEFAULT_FS,
                    'device': 'ephemeral0.1',
                    'replace_fs': 'ntfs'}],
+ }
@@ -190,7 +332,11 @@ class DataSourceAzureNet(sources.DataSource):
          for cdev in candidates:
              try:
                  if cdev.startswith("/dev/"):
--                    ret = util.mount_cb(cdev, load_azure_ds_dir)
++                    if util.is_FreeBSD():
++                        ret = util.mount_cb(cdev, load_azure_ds_dir,
++                                            mtype="udf", sync=False)
++                    else:
++                        ret = util.mount_cb(cdev, load_azure_ds_dir)
                  else:
                      ret = load_azure_ds_dir(cdev)
@@ -218,11 +364,12 @@ class DataSourceAzureNet(sources.DataSource):
              LOG.debug("using files cached in %s", ddir)
          # azure / hyper-v provides random data here
--        seed = util.load_file("/sys/firmware/acpi/tables/OEM0",
--                              quiet=True, decode=False)
--        if seed:
--            self.metadata['random_seed'] = seed
--
++        if not util.is_FreeBSD():
++            seed = util.load_file("/sys/firmware/acpi/tables/OEM0",
++                                  quiet=True, decode=False)
++            if seed:
++                self.metadata['random_seed'] = seed
++        # TODO. find the seed on FreeBSD platform
          # now update ds_cfg to reflect contents pass in config
          user_ds_cfg = util.get_cfg_by_path(self.cfg, DS_CFG_PATH, {})
          self.ds_cfg = util.mergemanydict([user_ds_cfg, self.ds_cfg])
@@ -633,8 +780,19 @@ def encrypt_pass(password, salt_id="$6$"):
  def list_possible_azure_ds_devs():
      # return a sorted list of devices that might have a azure datasource
      devlist = []
--    for fstype in ("iso9660", "udf"):
--        devlist.extend(util.find_devs_with("TYPE=%s" % fstype))
++    if util.is_FreeBSD():
++        cdrom_dev = "/dev/cd0"
++        try:
++            util.subp(["mount", "-o", "ro", "-t", "udf", cdrom_dev,
++                       "/mnt/cdrom/secure"])
++        except util.ProcessExecutionError:
++            LOG.debug("Fail to mount cd")
++            return devlist
++        util.subp(["umount", "/mnt/cdrom/secure"])
++        devlist.append(cdrom_dev)
++    else:
++        for fstype in ("iso9660", "udf"):
++            devlist.extend(util.find_devs_with("TYPE=%s" % fstype))
      devlist.sort(reverse=True)
      return devlist
 diff --git a/cloudinit/sources/helpers/azure.py b/cloudinit/sources/helpers/azure.py
 index 6e01aa4..e22409d 100644
 --- a/cloudinit/sources/helpers/azure.py
 +++ b/cloudinit/sources/helpers/azure.py
@@ -29,6 +29,14 @@ def cd(newdir):
          os.chdir(prevdir)
++def _get_dhcp_endpoint_option_name():
++    if util.is_FreeBSD():
++        azure_endpoint = "option-245"
++    else:
++        azure_endpoint = "unknown-245"
++    return azure_endpoint
++
++
  class AzureEndpointHttpClient(object):
      headers = {
@@ -235,8 +243,9 @@ class WALinuxAgentShim(object):
          leases = []
          content = util.load_file(fallback_lease_file)
          LOG.debug("content is %s", content)
++        option_name = _get_dhcp_endpoint_option_name()
          for line in content.splitlines():
--            if 'unknown-245' in line:
++            if option_name in line:
                  # Example line from Ubuntu
                  # option unknown-245 a8:3f:81:10;
                  leases.append(line.strip(' ').split(' ', 2)[-1].strip(';\n"'))
 diff --git a/cloudinit/stages.py b/cloudinit/stages.py
 index f7191b0..ad55782 100644
 --- a/cloudinit/stages.py
 +++ b/cloudinit/stages.py
@@ -624,7 +624,7 @@ class Init(object):
                  return (None, loc)
              if ncfg:
                  return (ncfg, loc)
--        return (net.generate_fallback_config(), "fallback")
++        return (self.distro.generate_fallback_config(), "fallback")
      def apply_network_config(self, bring_up):
          netcfg, src = self._find_networking_config()
 diff --git a/cloudinit/util.py b/cloudinit/util.py
 index 22af99d..27a9833 100644
 --- 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)
          mount_point = m.group(2)
          fs_type = m.group(3)
@@ -2336,7 +2385,8 @@ def read_dmi_data(key):
      uname_arch = os.uname()[4]
      if not (uname_arch == "x86_64" or
              (uname_arch.startswith("i") and uname_arch[2:] == "86") or
--            uname_arch == 'aarch64'):
++            uname_arch == 'aarch64' or
++            uname_arch == 'amd64'):
          LOG.debug("dmidata is not supported on %s", uname_arch)
          return None
 diff --git a/config/cloud.cfg-freebsd b/config/cloud.cfg-freebsd
 index be664f5..d666c39 100644
 --- a/config/cloud.cfg-freebsd
 +++ b/config/cloud.cfg-freebsd
@@ -5,7 +5,7 @@ syslog_fix_perms: root:wheel
  # This should not be required, but leave it in place until the real cause of
  # not beeing able to find -any- datasources is resolved.
--datasource_list: ['ConfigDrive', 'OpenStack', 'Ec2']
++datasource_list: ['ConfigDrive', 'Azure', 'OpenStack', 'Ec2']
  # A set of users which may be applied and/or used by various modules
  # when a 'default' entry is found it will reference the 'default_user'
 diff --git a/setup.py b/setup.py
 index 32a44d9..46d0521 100755
 --- a/setup.py
 +++ b/setup.py
@@ -89,7 +89,6 @@ LIB = "/lib"
  if os.uname()[0] == 'FreeBSD':
      USR = "/usr/local"
      USR_LIB_EXEC = "/usr/local/lib"
--    ETC = "/usr/local/etc"
  elif os.path.isfile('/etc/redhat-release'):
      USR_LIB_EXEC = "/usr/libexec"
@@ -164,8 +163,6 @@ else:
          (ETC + '/cloud', glob('config/*.cfg')),
          (ETC + '/cloud/cloud.cfg.d', glob('config/cloud.cfg.d/*')),
          (ETC + '/cloud/templates', glob('templates/*')),
--        (ETC + '/NetworkManager/dispatcher.d/', ['tools/hook-network-manager']),
--        (ETC + '/dhcp/dhclient-exit-hooks.d/', ['tools/hook-dhclient']),
          (USR_LIB_EXEC + '/cloud-init', ['tools/ds-identify',
                                          'tools/uncloud-init',
                                          'tools/write-ssh-key-fingerprints']),
@@ -174,8 +171,13 @@ else:
              [f for f in glob('doc/examples/*') if is_f(f)]),
          (USR + '/share/doc/cloud-init/examples/seed',
              [f for f in glob('doc/examples/seed/*') if is_f(f)]),
--        (LIB + '/udev/rules.d', [f for f in glob('udev/*.rules')]),
+     ]
++    if os.uname()[0] != 'FreeBSD':
++        data_files.append([
++            (ETC + '/NetworkManager/dispatcher.d/', ['tools/hook-network-manager']),
++            (ETC + '/dhcp/dhclient-exit-hooks.d/', ['tools/hook-dhclient']),
++            (LIB + '/udev/rules.d', [f for f in glob('udev/*.rules')])
++        ])
      # Use a subclass for install that handles
      # adding on the right init system configuration files
      cmdclass = {
 diff --git a/sysvinit/freebsd/cloudconfig b/sysvinit/freebsd/cloudconfig
 index 01bc061..e4064fa 100755
 --- a/sysvinit/freebsd/cloudconfig
 +++ b/sysvinit/freebsd/cloudconfig
@@ -7,24 +7,14 @@
  . /etc/rc.subr
  PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
--export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
  name="cloudconfig"
  command="/usr/local/bin/cloud-init"
  start_cmd="cloudconfig_start"
  stop_cmd=":"
  rcvar="cloudinit_enable"
--start_precmd="cloudinit_override"
  start_cmd="cloudconfig_start"
--cloudinit_override()
--{
--	# If there exist sysconfig/defaults variable override files use it...
--	if [ -f /etc/defaults/cloud-init ]; then
--		. /etc/defaults/cloud-init
--	fi
--}
--
  cloudconfig_start()
+ {
  	echo "${command} starting"
 diff --git a/sysvinit/freebsd/cloudfinal b/sysvinit/freebsd/cloudfinal
 index 1b487aa..b6894c3 100755
 --- a/sysvinit/freebsd/cloudfinal
 +++ b/sysvinit/freebsd/cloudfinal
@@ -7,24 +7,14 @@
  . /etc/rc.subr
  PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
--export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
  name="cloudfinal"
  command="/usr/local/bin/cloud-init"
  start_cmd="cloudfinal_start"
  stop_cmd=":"
  rcvar="cloudinit_enable"
--start_precmd="cloudinit_override"
  start_cmd="cloudfinal_start"
--cloudinit_override()
--{
--	# If there exist sysconfig/defaults variable override files use it...
--	if [ -f /etc/defaults/cloud-init ]; then
--		 . /etc/defaults/cloud-init
--	fi
--}
--
  cloudfinal_start()
+ {
  	echo -n "${command} starting"
 diff --git a/sysvinit/freebsd/cloudinit b/sysvinit/freebsd/cloudinit
 index 862eeab..3326300 100755
 --- a/sysvinit/freebsd/cloudinit
 +++ b/sysvinit/freebsd/cloudinit
@@ -7,24 +7,14 @@
  . /etc/rc.subr
  PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
--export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
  name="cloudinit"
  command="/usr/local/bin/cloud-init"
  start_cmd="cloudinit_start"
  stop_cmd=":"
  rcvar="cloudinit_enable"
--start_precmd="cloudinit_override"
  start_cmd="cloudinit_start"
--cloudinit_override()
--{
--	# If there exist sysconfig/defaults variable override files use it...
--	if [ -f /etc/defaults/cloud-init ]; then
--		. /etc/defaults/cloud-init
--	fi
--}
--
  cloudinit_start()
+ {
  	echo -n "${command} starting"
 diff --git a/sysvinit/freebsd/cloudinitlocal b/sysvinit/freebsd/cloudinitlocal
 index fb342a0..11a5eb1 100755
 --- a/sysvinit/freebsd/cloudinitlocal
 +++ b/sysvinit/freebsd/cloudinitlocal
@@ -1,30 +1,20 @@
  #!/bin/sh
  # PROVIDE: cloudinitlocal
--# REQUIRE: mountcritlocal
++# REQUIRE: ldconfig mountcritlocal
  # BEFORE:  NETWORKING FILESYSTEMS cloudinit cloudconfig cloudfinal
  . /etc/rc.subr
  PATH="/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"
--export CLOUD_CFG=/usr/local/etc/cloud/cloud.cfg
  name="cloudinitlocal"
  command="/usr/local/bin/cloud-init"
  start_cmd="cloudlocal_start"
  stop_cmd=":"
  rcvar="cloudinit_enable"
--start_precmd="cloudinit_override"
  start_cmd="cloudlocal_start"
--cloudinit_override()
--{
--	# If there exist sysconfig/defaults variable override files use it...
--	if [ -f /etc/defaults/cloud-init ]; then
--		. /etc/defaults/cloud-init
--	fi
--}
--
  cloudlocal_start()
+ {
  	echo -n "${command} starting"
 diff --git a/tests/unittests/test_datasource/test_azure.py b/tests/unittests/test_datasource/test_azure.py
 index 8d22bb5..e6b0dcb 100644
 --- a/tests/unittests/test_datasource/test_azure.py
 +++ b/tests/unittests/test_datasource/test_azure.py
@@ -3,6 +3,8 @@
  from cloudinit import helpers
  from cloudinit.util import b64e, decode_binary, load_file
  from cloudinit.sources import DataSourceAzure
++from cloudinit.util import find_freebsd_part
++from cloudinit.util import get_path_dev_freebsd
  from ..helpers import TestCase, populate_dir, mock, ExitStack, PY26, SkipTest
@@ -95,6 +97,41 @@ class TestAzureDataSource(TestCase):
          for module, name, new in patches:
              self.patches.enter_context(mock.patch.object(module, name, new))
++    def _get_mockds(self):
++        mod = DataSourceAzure
++        sysctl_out = "dev.storvsc.3.%pnpinfo: "\
++                     "classid=ba6163d9-04a1-4d29-b605-72e2ffb1dc7f "\
++                     "deviceid=f8b3781b-1e82-4818-a1c3-63d806ec15bb\n"
++        sysctl_out += "dev.storvsc.2.%pnpinfo: "\
++                      "classid=ba6163d9-04a1-4d29-b605-72e2ffb1dc7f "\
++                      "deviceid=f8b3781a-1e82-4818-a1c3-63d806ec15bb\n"
++        sysctl_out += "dev.storvsc.1.%pnpinfo: "\
++                      "classid=32412632-86cb-44a2-9b5c-50d1417354f5 "\
++                      "deviceid=00000000-0001-8899-0000-000000000000\n"
++        camctl_devbus = """
++scbus0 on ata0 bus 0
++scbus1 on ata1 bus 0
++scbus2 on blkvsc0 bus 0
++scbus3 on blkvsc1 bus 0
++scbus4 on storvsc2 bus 0
++scbus5 on storvsc3 bus 0
++scbus-1 on xpt0 bus 0
++        """
++        camctl_dev = """
++<Msft Virtual CD/ROM 1.0>          at scbus1 target 0 lun 0 (cd0,pass0)
++<Msft Virtual Disk 1.0>            at scbus2 target 0 lun 0 (da0,pass1)
++<Msft Virtual Disk 1.0>            at scbus3 target 1 lun 0 (da1,pass2)
++        """
++        self.apply_patches([
++            (mod, 'get_dev_storvsc_sysctl', mock.MagicMock(
++                return_value=sysctl_out)),
++            (mod, 'get_camcontrol_dev_bus', mock.MagicMock(
++                return_value=camctl_devbus)),
++            (mod, 'get_camcontrol_dev', mock.MagicMock(
++                return_value=camctl_dev))
++        ])
++        return mod
++
      def _get_ds(self, data, agent_command=None):
          def dsdevs():
@@ -177,6 +214,34 @@ class TestAzureDataSource(TestCase):
              return
          raise AssertionError("XML is the same")
++    def test_get_resource_disk(self):
++        ds = self._get_mockds()
++        dev = ds.get_resource_disk_on_freebsd(1)
++        self.assertEqual("da1", dev)
++
++    @mock.patch('cloudinit.util.subp')
++    def test_find_freebsd_part_on_Azure(self, mock_subp):
++        glabel_out = '''
++gptid/fa52d426-c337-11e6-8911-00155d4c5e47  N/A  da0p1
++                              label/rootfs  N/A  da0p2
++                                label/swap  N/A  da0p3
++'''
++        mock_subp.return_value = (glabel_out, "")
++        res = find_freebsd_part("/dev/label/rootfs")
++        self.assertEqual("da0p2", res)
++
++    def test_get_path_dev_freebsd_on_Azure(self):
++        mnt_list = '''
++/dev/label/rootfs  /                ufs     rw              1 1
++devfs              /dev             devfs   rw,multilabel   0 0
++fdescfs            /dev/fd          fdescfs rw              0 0
++/dev/da1s1         /mnt/resource    ufs     rw              2 2
++'''
++        with mock.patch.object(os.path, 'exists',
++                               return_value=True):
++            res = get_path_dev_freebsd('/etc', mnt_list)
++            self.assertNotEqual(res, None)
++
      def test_basic_seed_dir(self):
          odata = {'HostName': "myhost", 'UserName': "myuser"}
          data = {'ovfcontent': construct_valid_ovf_env(data=odata),
 diff --git a/tests/unittests/test_datasource/test_azure_helper.py b/tests/unittests/test_datasource/test_azure_helper.py
 index aafdebd..b2d2971 100644
 --- a/tests/unittests/test_datasource/test_azure_helper.py
 +++ b/tests/unittests/test_datasource/test_azure_helper.py
@@ -3,7 +3,6 @@
  import os
  from cloudinit.sources.helpers import azure as azure_helper
--
  from ..helpers import ExitStack, mock, TestCase
@@ -72,10 +71,11 @@ class TestFindEndpoint(TestCase):
      @staticmethod
      def _build_lease_content(encoded_address):
++        endpoint = azure_helper._get_dhcp_endpoint_option_name()
          return '\n'.join([
              'lease {',
              ' interface "eth0";',
--            ' option unknown-245 {0};'.format(encoded_address),
++            ' option {0} {1};'.format(endpoint, encoded_address),
              '}'])
      def test_from_dhcp_client(self):
 diff --git a/tests/unittests/test_datasource/test_cloudstack.py b/tests/unittests/test_datasource/test_cloudstack.py
 index e93d28d..1d3d2f1 100644
 --- a/tests/unittests/test_datasource/test_cloudstack.py
 +++ b/tests/unittests/test_datasource/test_cloudstack.py
@@ -15,6 +15,11 @@ class TestCloudStackPasswordFetching(TestCase):
          mod_name = 'cloudinit.sources.DataSourceCloudStack'
          self.patches.enter_context(mock.patch('{0}.ec2'.format(mod_name)))
          self.patches.enter_context(mock.patch('{0}.uhelp'.format(mod_name)))
++        default_gw = "192.201.20.0"
++        mod_name = 'cloudinit.sources.DataSourceCloudStack.get_default_gateway'
++        get_default_gw = mock.MagicMock(return_value=default_gw)
++        self.patches.enter_context(
++            mock.patch(mod_name, get_default_gw))
      def _set_password_server_response(self, response_string):
          subp = mock.MagicMock(return_value=(response_string, ''))
 diff --git a/tests/unittests/test_distros/test_netconfig.py b/tests/unittests/test_distros/test_netconfig.py
 index 8837066..1e10a33 100644
 --- a/tests/unittests/test_distros/test_netconfig.py
 +++ b/tests/unittests/test_distros/test_netconfig.py
@@ -178,6 +178,20 @@ class WriteBuffer(object):
  class TestNetCfgDistro(TestCase):
++    frbsd_ifout = """\
++hn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500
++        options=51b<RXCSUM,TXCSUM,VLAN_MTU,VLAN_HWTAGGING,TSO4,LRO>
++        ether 00:15:5d:4c:73:00
++        inet6 fe80::215:5dff:fe4c:7300%hn0 prefixlen 64 scopeid 0x2
++        inet 10.156.76.127 netmask 0xfffffc00 broadcast 10.156.79.255
++        nd6 options=23<PERFORMNUD,ACCEPT_RTADV,AUTO_LINKLOCAL>
++        media: Ethernet autoselect (10Gbase-T <full-duplex>)
++        status: active
++"""
++
++    def setUp(self):
++        super(TestNetCfgDistro, self).setUp()
++
      def _get_distro(self, dname, renderers=None):
          cls = distros.fetch(dname)
          cfg = settings.CFG_BUILTIN
@@ -251,6 +265,7 @@ class TestNetCfgDistro(TestCase):
      def test_apply_network_config_v1_to_netplan_ub(self):
          renderers = ['netplan']
++        devlist = ['eth0', 'lo']
          ub_distro = self._get_distro('ubuntu', renderers=renderers)
          with ExitStack() as mocks:
              write_bufs = {}
@@ -272,6 +287,9 @@ class TestNetCfgDistro(TestCase):
                  mock.patch.object(util, 'subp', return_value=(0, 0)))
              mocks.enter_context(
                  mock.patch.object(os.path, 'isfile', return_value=False))
++            mocks.enter_context(
++                mock.patch("cloudinit.net.netplan.get_devicelist",
++                           return_value=devlist))
              ub_distro.apply_network_config(V1_NET_CFG, False)
@@ -285,6 +303,7 @@ class TestNetCfgDistro(TestCase):
      def test_apply_network_config_v2_passthrough_ub(self):
          renderers = ['netplan']
++        devlist = ['eth0', 'lo']
          ub_distro = self._get_distro('ubuntu', renderers=renderers)
          with ExitStack() as mocks:
              write_bufs = {}
@@ -306,7 +325,10 @@ class TestNetCfgDistro(TestCase):
                  mock.patch.object(util, 'subp', return_value=(0, 0)))
              mocks.enter_context(
                  mock.patch.object(os.path, 'isfile', return_value=False))
--
++            # FreeBSD does not have '/sys/class/net' file,
++            # so we need mock here.
++            mocks.enter_context(
++                mock.patch.object(os, 'listdir', return_value=devlist))
              ub_distro.apply_network_config(V2_NET_CFG, False)
              self.assertEqual(len(write_bufs), 1)
@@ -328,6 +350,29 @@ class TestNetCfgDistro(TestCase):
          for (k, v) in b1.items():
              self.assertEqual(v, b2[k])
++    @mock.patch('cloudinit.distros.freebsd.Distro.get_ifconfig_list')
++    @mock.patch('cloudinit.distros.freebsd.Distro.get_ifconfig_ifname_out')
++    def test_get_ip_nic_freebsd(self, ifname_out, iflist):
++        frbsd_distro = self._get_distro('freebsd')
++        iflist.return_value = "lo0 hn0"
++        ifname_out.return_value = self.frbsd_ifout
++        res = frbsd_distro.get_ipv4()
++        self.assertEqual(res, ['lo0', 'hn0'])
++        res = frbsd_distro.get_ipv6()
++        self.assertEqual(res, [])
++
++    @mock.patch('cloudinit.distros.freebsd.Distro.get_ifconfig_ether')
++    @mock.patch('cloudinit.distros.freebsd.Distro.get_ifconfig_ifname_out')
++    @mock.patch('cloudinit.distros.freebsd.Distro.get_interface_mac')
++    def test_generate_fallback_config_freebsd(self, mac, ifname_out, if_ether):
++        frbsd_distro = self._get_distro('freebsd')
++
++        if_ether.return_value = 'hn0'
++        ifname_out.return_value = self.frbsd_ifout
++        mac.return_value = '00:15:5d:4c:73:00'
++        res = frbsd_distro.generate_fallback_config()
++        self.assertIsNotNone(res)
++
      def test_simple_write_rh(self):
          rh_distro = self._get_distro('rhel')
 diff --git a/tests/unittests/test_handler/test_handler_resizefs.py b/tests/unittests/test_handler/test_handler_resizefs.py
 new file mode 100644
 index 0000000..52591b8
 --- /dev/null
 +++ b/tests/unittests/test_handler/test_handler_resizefs.py
@@ -0,0 +1,59 @@
++# This file is part of cloud-init. See LICENSE file for license information.
++
++from cloudinit.config import cc_resizefs
++
++import textwrap
++import unittest
++
++try:
++    from unittest import mock
++except ImportError:
++    import mock
++
++
++class TestResizefs(unittest.TestCase):
++    def setUp(self):
++        super(TestResizefs, self).setUp()
++        self.name = "resizefs"
++
++    @mock.patch('cloudinit.config.cc_resizefs._get_dumpfs_output')
++    @mock.patch('cloudinit.config.cc_resizefs._get_gpart_output')
++    def test_skip_ufs_resize(self, gpart_out, dumpfs_out):
++        fs_type = "ufs"
++        resize_what = "/"
++        devpth = "/dev/da0p2"
++        dumpfs_out.return_value = (
++            "# newfs command for / (/dev/label/rootfs)\n"
++            "newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 "
++            "-f 4096 -g 16384 -h 64 -i 8192 -j -k 6408 -m 8 "
++            "-o time -s 58719232 /dev/label/rootfs\n")
++        gpart_out.return_value = textwrap.dedent("""\
++            =>      40  62914480  da0  GPT  (30G)
++                    40      1024    1  freebsd-boot  (512K)
++                  1064  58719232    2  freebsd-ufs  (28G)
++              58720296   3145728    3  freebsd-swap  (1.5G)
++              61866024   1048496       - free -  (512M)
++            """)
++        res = cc_resizefs.can_skip_resize(fs_type, resize_what, devpth)
++        self.assertTrue(res)
++
++    @mock.patch('cloudinit.config.cc_resizefs._get_dumpfs_output')
++    @mock.patch('cloudinit.config.cc_resizefs._get_gpart_output')
++    def test_skip_ufs_resize_roundup(self, gpart_out, dumpfs_out):
++        fs_type = "ufs"
++        resize_what = "/"
++        devpth = "/dev/da0p2"
++        dumpfs_out.return_value = (
++            "# newfs command for / (/dev/label/rootfs)\n"
++            "newfs -O 2 -U -a 4 -b 32768 -d 32768 -e 4096 "
++            "-f 4096 -g 16384 -h 64 -i 8192 -j -k 368 -m 8 "
++            "-o time -s 297080 /dev/label/rootfs\n")
++        gpart_out.return_value = textwrap.dedent("""\
++            =>      34  297086  da0  GPT  (145M)
++                    34  297086    1  freebsd-ufs  (145M)
++            """)
++        res = cc_resizefs.can_skip_resize(fs_type, resize_what, devpth)
++        self.assertTrue(res)
++
++
++# vi: ts=4 expandtab
 diff --git a/tests/unittests/test_net.py b/tests/unittests/test_net.py
 index 89e7536..5d2dd03 100644
 --- a/tests/unittests/test_net.py
 +++ b/tests/unittests/test_net.py
@@ -1120,14 +1120,14 @@ class TestNetplanPostcommands(CiTestCase):
          render_target = 'netplan.yaml'
          renderer = netplan.Renderer(
              {'netplan_path': render_target, 'postcmds': True})
--        renderer.render_network_state(render_dir, ns)
--
          expected = [
              mock.call(['netplan', 'generate'], capture=True),
              mock.call(['udevadm', 'test-builtin', 'net_setup_link',
                         '/sys/class/net/lo'], capture=True),
+         ]
--        mock_subp.assert_has_calls(expected)
++        with mock.patch.object(os.path, 'islink', return_value=True):
++            renderer.render_network_state(render_dir, ns)
++            mock_subp.assert_has_calls(expected)
  class TestEniNetworkStateToEni(CiTestCase):
 diff --git a/tests/unittests/test_util.py b/tests/unittests/test_util.py
 index 5d21b4b..189caca 100644
 --- a/tests/unittests/test_util.py
 +++ b/tests/unittests/test_util.py
@@ -596,7 +596,8 @@ class TestSubp(helpers.TestCase):
      def test_subp_capture_stderr(self):
          data = b'hello world'
          (out, err) = util.subp(self.stdin2err, capture=True,
--                               decode=False, data=data)
++                               decode=False, data=data,
++                               update_env={'LC_ALL': 'C'})
          self.assertEqual(err, data)
          self.assertEqual(out, b'')
 diff --git a/tools/build-on-freebsd b/tools/build-on-freebsd
 index 8436498..ccc10b4 100755
 --- a/tools/build-on-freebsd
 +++ b/tools/build-on-freebsd
@@ -10,9 +10,7 @@ depschecked=/tmp/c-i.dependencieschecked
  pkgs="
     dmidecode
     e2fsprogs
--   gpart
     py27-Jinja2
--   py27-argparse
     py27-boto
     py27-cheetah
     py27-configobj
@@ -38,7 +36,7 @@ python setup.py build
  python setup.py install -O1 --skip-build --prefix /usr/local/ --init-system sysvinit_freebsd
  # Install the correct config file:
--cp config/cloud.cfg-freebsd /usr/local/etc/cloud/cloud.cfg
++cp config/cloud.cfg-freebsd /etc/cloud/cloud.cfg
  # Enable cloud-init in /etc/rc.conf:
  sed -i.bak -e "/cloudinit_enable=.*/d" /etc/rc.conf