Merge ~redriver/cloud-init:frbsd-azure-branch into cloud-init:master
| Status: | Merged |
|---|---|
| Merged at revision: | 0a71d5a870b416f2c86c8bc196004bb3fc0768a0 |
| Proposed branch: | ~redriver/cloud-init:frbsd-azure-branch |
| Merge into: | cloud-init:master |
| Diff against target: |
1374 lines (+782/-87) 22 files modified
cloudinit/config/cc_resizefs.py (+95/-0) cloudinit/distros/__init__.py (+3/-0) cloudinit/distros/freebsd.py (+261/-16) cloudinit/settings.py (+1/-1) cloudinit/sources/DataSourceAzure.py (+169/-11) cloudinit/sources/helpers/azure.py (+10/-1) cloudinit/stages.py (+1/-1) cloudinit/util.py (+51/-1) config/cloud.cfg-freebsd (+1/-1) setup.py (+6/-4) sysvinit/freebsd/cloudconfig (+0/-10) sysvinit/freebsd/cloudfinal (+0/-10) sysvinit/freebsd/cloudinit (+0/-10) sysvinit/freebsd/cloudinitlocal (+1/-11) tests/unittests/test_datasource/test_azure.py (+65/-0) tests/unittests/test_datasource/test_azure_helper.py (+2/-2) tests/unittests/test_datasource/test_cloudstack.py (+5/-0) tests/unittests/test_distros/test_netconfig.py (+46/-1) tests/unittests/test_handler/test_handler_resizefs.py (+59/-0) tests/unittests/test_net.py (+3/-3) tests/unittests/test_util.py (+2/-1) tools/build-on-freebsd (+1/-3) |
| Related bugs: |
| 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:
|
|||
Commit message
This patch targets to make FreeBSD 10.3 or 11 work on Azure. The modifications abide by the rule of
(1) making as less modification as possible
(2) delegating the platform or cloud specific operations to the specific classes instead of adding platform checking.
The main modifications constitute the following items:
1. Commands for network configuration are different from the ones on Ubuntu, for example, get ipv4/ipv6/mac, so, some of the functions called by apply_network_
2. Password setting through "pw" can only work through pipe, like "echo PASSWD|pw ... -H 0".
3. Default group for root user is "wheel" on FreeBSD. So, 'root:wheel' should be added syslog_fix_perms field.
4. There are some changes related to FreeBSD on Azure (DataSourceAzure):
(a) The leases file for dhclient is different from the one on Linux, so, the lease file path should be generated according to platform. The other similar platform related values: default primary network interface (hn0)and default file system (ufs)
(b) Mounting issues, including mounting CD, and get mount information
(c) Azure protocol depends on a field of dhclient lease file, but its name is different on FreeBSD. (azure.py, "option-245" vs. "unknown-245")
(d) Method to find resource (ephemeral) disk is a little complex because FreeBSD does not provide /dev/disk/
(e) Add a cloud.cfg for FreeBSD on Azure. Azure sets user information through ovf file, so the default configuration should not contain any user information.
Some of the features have not been fully implemented, for example, get seed from "/sys/firmware/
| Scott Moser (smoser) wrote : | # |
| Scott Moser (smoser) wrote : | # |
Note, I really dont have a lot of experience on FreeBSD. I relied heavily
on others who added freebsd support to tell me what made sense, and I'll do
the same for you.
Thank you for your efforts here... There are definitely some things we need
to re-work to better support different "distros" throughout the code base.
We'll see these same sorts of things as aixtools is working on getting better
aix support in.
One such example is your moving generate_
Overall, comments, much of the code needs some unit tests. We simply can't
avoid breaking things inadvertently without them get_resource_
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/
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/
cloudinit/
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...
| Jake Smith (c0by) wrote : | # |
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.
Basically this, I did a quick hack to test and seems ok:
current_debugflags = sysctl -n kern.geom.
sysctl kern.geom.
growfs -y /dev/vtbd0p2
sysctl kern.geom.
| Hongjiang Zhang (redriver) wrote : | # |
Thanks for you careful review and valuable comments.
> Note, I really dont have a lot of experience on FreeBSD. I relied heavily
> on others who added freebsd support to tell me what made sense, and I'll do
> the same for you.
That is ok for me.
> Thank you for your efforts here... There are definitely some things we need
> to re-work to better support different "distros" throughout the code base.
> We'll see these same sorts of things as aixtools is working on getting better
> aix support in.
>
> One such example is your moving generate_
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_
> 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/
> 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...
| Scott Moser (smoser) wrote : | # |
> Thanks for you careful review and valuable comments.
>
> > Overall, comments, much of the code needs some unit tests. We simply can't
> > avoid breaking things inadvertently without them
> get_resource_
> > 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/
> > 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
...
| Hongjiang Zhang (redriver) wrote : | # |
> > Thanks for you careful review and valuable comments.
> >
> > > Overall, comments, much of the code needs some unit tests. We simply
> can't
> > > avoid breaking things inadvertently without them
> > get_resource_
> > > 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/
> > > 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.
> > > ...
| Hongjiang Zhang (redriver) wrote : | # |
> 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.
> 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.
> sysctl kern.geom.
> growfs -y /dev/vtbd0p2
> sysctl kern.geom.
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)
| Jake Smith (c0by) wrote : | # |
> > 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.
> 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.
> > sysctl kern.geom.
> > growfs -y /dev/vtbd0p2
> > sysctl kern.geom.
> 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)
| Hongjiang Zhang (redriver) wrote : | # |
> > > 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.
> > 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.
> > > sysctl kern.geom.
> > > growfs -y /dev/vtbd0p2
> > > sysctl kern.geom.
> > 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.
| Jake Smith (c0by) wrote : | # |
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
| Hongjiang Zhang (redriver) wrote : | # |
> 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.
| Hongjiang Zhang (redriver) wrote : | # |
> 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-
| Hongjiang Zhang (redriver) wrote : | # |
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.
The quick fix is move "cfgs.extend(
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:
if self._base_cfg:
return util.mergemanyd
| Scott Moser (smoser) wrote : | # |
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/
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.
{'foo': 'from enviroment'},
{'foo': 'from instance, no user override'},
{'
{'
])
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.
| Hongjiang Zhang (redriver) wrote : | # |
> 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/
> 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.mergemanyd
> {'foo': 'from enviroment'},
> {'foo': 'from instance, no user override'},
> {'system_info': {'default_user': {'name': 'smoser-
> {'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/
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$WSLBpwonNkC
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$WSLBpwonNkC
| Hongjiang Zhang (redriver) wrote : | # |
> 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/
> 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.mergemanyd
> {'foo': 'from enviroment'},
> {'foo': 'from instance, no user override'},
> {'system_info': {'default_user': {'name': 'smoser-
> {'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/
sysvinit/
sysvinit/
sysvinit/
in the helpers.py, I observed:
CFG_ENV_NAME = "CLOUD_CFG"
...
def _get_env_
e_cfgs = []
if CFG_ENV_NAME in os.environ:
e_fn = os.environ[
try:
except Exception:
return e_cfgs
I don't know why FreeBSD needs to export this environment, but none of the other distros need it.
| Hongjiang Zhang (redriver) wrote : | # |
I have finished all the needed fixes.
PASSED: Continuous integration, rev:400a89bc938
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Scott Moser (smoser) wrote : | # |
There is a lot of good stuff here, thank you for your work and your patience.
Some good things:
- the generate_
- 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_
c.) is your '_can_skip_
FAILED: Continuous integration, rev:85137a5ffe6
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:316b4b389cd
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) wrote : | # |
> There is a lot of good stuff here, thank you for your work and your patience.
> Some good things:
> - the generate_
> - 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/
def get_mount_
return util.get_
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_
Yes, I have changed it per your suggestion.
> c.) is your '_can_skip_
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.
| Scott Moser (smoser) wrote : | # |
> > 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/
> platform, for example, EC2, it will invoke the util.get_
> def get_mount_
> return util.get_
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/
take a 'cloud'.
- resize_devices then calls devent2dev(devent, cloud)
- devent2dev calls cloud.datasourc
This change:
- result = util.get_
+ result = cloud.datasourc
I'd have preferred for 'util.get_
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_
> 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.
| Scott Moser (smoser) wrote : | # |
I think you might be able to not change anything in the get_mount_info path, but rather just fix a bug in 'parse_
--- a/cloudinit/util.py
+++ b/cloudinit/util.py
@@ -2056,6 +2056,8 @@ def parse_mount(path):
mount_locs = mountoutput.
for line in mount_locs:
m = re.search(
+ if not m:
+ continue
devpth = m.group(1)
fs_type = m.group(3)
then, in my testing on a freebsd instance in digital ocean :
$ python -c 'from cloudinit import util; print(util.
('/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/
Note, that it does not work unless you give it a mount point as input. Ie, util.get_
FAILED: Continuous integration, rev:cddf09f0a26
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:27ab16c21a1
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:1bafe24b285
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) wrote : | # |
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_
>
> --- a/cloudinit/util.py
> +++ b/cloudinit/util.py
> @@ -2056,6 +2056,8 @@ def parse_mount(path):
> mount_locs = mountoutput.
> for line in mount_locs:
> m = re.search(
> + 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.
> ('/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/
>
>
> Note, that it does not work unless you give it a mount point as input. Ie,
> util.get_
> could fix that though, by just looking through all the mount points and
> finding the one that 'path' is on.
| Scott Moser (smoser) wrote : | # |
Hi,
Sorry for the slow reply.
We are getting closer.
I have some comments inline.
PASSED: Continuous integration, rev:1505ec9ccc5
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:35141b85edc
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) wrote : | # |
> 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?
| Hongjiang Zhang (redriver) wrote : | # |
> 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?
PASSED: Continuous integration, rev:a6a04d1d840
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
FAILED: Continuous integration, rev:d91244316aa
https:/
Executed test runs:
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
FAILURE: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:9f509bee5f6
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Scott Moser (smoser) wrote : | # |
there are som ecomments in line.
I have a mp i will submit to you to cleanup some others.
| Scott Moser (smoser) wrote : | # |
please see my changes at https:/
| Scott Moser (smoser) wrote : | # |
Other than that and I think it looks really good.
thank you for adding the unit tests.
| Hongjiang Zhang (redriver) wrote : | # |
> 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_
> #
> # So use /proc/$$/mountinfo to find the device underlying the
> # input path.
> + if is_FreeBSD_
>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_
> mountinfo_path = '/proc/
> if os.path.
> lines = load_file(
> diff --git a/tests/
> b/tests/
> new file mode 100644
> index 0000000..b5384e4
> --- /dev/null
> +++ b/tests/
> @@ -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(
> + def setUp(self):
> + super(TestResizefs, self).setUp()
> + self.name = "resizefs"
> +
> + @mock.patch(
> + @mock.patch(
> + def test_skip_
> + fs_type = "ufs"
> + resize_what = "/"
> + devpth = "/dev/da0p2"
> + dumpfs_
> + "(/dev/
> + "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/
> + gpart_out.
> +=> 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.
> + resize_what,
> + devpth)
> + self.assertTrue
> +
> + @mock.patch(
> + @mock.patch(
> + def test_skip_
> + fs_type = "ufs"
> + resize_what = "/"
> + devpth = "/dev/da0p2"
> + dumpfs_
> + ...
PASSED: Continuous integration, rev:6a0c7e539c5
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
PASSED: Continuous integration, rev:bba43d77e4c
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) 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?
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/
Other than that and I think it looks really good.
thank you for adding the unit tests.
--
https:/
You are the owner of ~redriver/
| Scott Moser (smoser) wrote : | # |
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/
>
> Other than that and I think it looks really good.
> thank you for adding the unit tests.
>
> --
> https:/
> You are the owner of ~redriver/
>
| Hongjiang Zhang (redriver) wrote : | # |
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/
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/
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:master
>
> Other than that and I think it looks really good.
> thank you for adding the unit tests.
>
> --
> https:/
> unchpad.
> 2F314895&
> 08d481d4cf8c%
> 371945&
> ed=0 You are the owner of ~redriver/
>
| Hongjiang Zhang (redriver) wrote : | # |
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/
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/
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/
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:master
>
> Other than that and I think it looks really good.
> thank you for adding the unit tests.
>
> --
> https:/
> unchpad.
> 2F314895&
> 08d481d4cf8c%
> 371945&
> ed=0 You are the owner of ~redriver/
>
- c1cf3ad... by Hongjiang Zhang on 2017-04-25
PASSED: Continuous integration, rev:c1cf3ad1fc1
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) wrote : | # |
By the way, I removed the Azure specific check for "get_mount_info".
get_mount_
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_
+
+
def get_cfg_
if key not in yobj:
return default
@@ -2055,11 +2059,56 @@ def parse_mtab(path):
return None
+def find_freebsd_
+ if label_part.
+ target_label = label_part[5:]
+ (label_part, err) = subp(['glabel', 'status', '-s'])
+ for labels in label_part.
+ items = labels.split()
+ if len(items) > 0 and items[0]
+ label_part = items[2]
+ break
+ label_part = str(label_part)
+ return label_part
+
+
+def get_path_
+ path_found = None
+ for line in mnt_list.
+ items = line.split()
+ if (len(items) > 2 and os.path.
+ path_found = line
+ break
+ return path_found
+
+
+def get_mount_
+ (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_
+ if (path_found is None):
+ return None
+ result = path_found
+ ret = result.split()
+ label_part = find_freebsd_
+ return "/dev/" + label_part, ret[2], ret[1]
+
+
def parse_mount(path):
(mountoutput, _err) = subp("mount")
mount_locs = mountoutput.
for line in mount_locs:
m = re.search(
+ if not m:
+ continue
+ # check whether the dev refers to a label on FreeBSD
+ # for example, if dev is '/dev/label/
+ # continue finding the real device like '/dev/da0'.
+ devm = re.search(
+ if (not devm and is_FreeBSD()):
+ return get_mount_
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/
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...
| Kylie Liang (kyliel) wrote : | # |
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/
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/
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/
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/
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:master
>
> Other than that and I think it looks really good.
> thank you for adding the unit tests.
>
> --
> https:/
> unchpad.
> 2F314895&
> 08d481d4cf8c%
> 371945&sdata=q...
- 1b1e0a8... by Hongjiang Zhang on 2017-04-27
PASSED: Continuous integration, rev:1b1e0a8fe65
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) wrote : | # |
Hi Scott,
I have removed the Azure specific check for "get_mount_info".
get_mount_
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_
+
+
def get_cfg_
if key not in yobj:
return default
@@ -2055,11 +2059,56 @@ def parse_mtab(path):
return None
+def find_freebsd_
+ if label_part.
+ target_label = label_part[5:]
+ (label_part, err) = subp(['glabel', 'status', '-s'])
+ for labels in label_part.
+ items = labels.split()
+ if len(items) > 0 and items[0]
+ label_part = items[2]
+ break
+ label_part = str(label_part)
+ return label_part
+
+
+def get_path_
+ path_found = None
+ for line in mnt_list.
+ items = line.split()
+ if (len(items) > 2 and os.path.
+ path_found = line
+ break
+ return path_found
+
+
+def get_mount_
+ (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_
+ if (path_found is None):
+ return None
+ result = path_found
+ ret = result.split()
+ label_part = find_freebsd_
+ return "/dev/" + label_part, ret[2], ret[1]
+
+
def parse_mount(path):
(mountoutput, _err) = subp("mount")
mount_locs = mountoutput.
for line in mount_locs:
m = re.search(
+ if not m:
+ continue
+ # check whether the dev refers to a label on FreeBSD
+ # for example, if dev is '/dev/label/
+ # continue finding the real device like '/dev/da0'.
+ devm = re.search(
+ if (not devm and is_FreeBSD()):
+ return get_mount_
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/
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...
| Hongjiang Zhang (redriver) 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
-----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/
Hi Scott,
I have removed the Azure specific check for "get_mount_info".
get_mount_
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_
+
+
def get_cfg_
if key not in yobj:
return default
@@ -2055,11 +2059,56 @@ def parse_mtab(path):
return None
+def find_freebsd_
+ if label_part.
+ target_label = label_part[5:]
+ (label_part, err) = subp(['glabel', 'status', '-s'])
+ for labels in label_part.
+ items = labels.split()
+ if len(items) > 0 and items[0]
+ label_part = items[2]
+ break
+ label_part = str(label_part)
+ return label_part
+
+
+def get_path_
+ path_found = None
+ for line in mnt_list.
+ items = line.split()
+ if (len(items) > 2 and os.path.
+ path_found = line
+ break
+ return path_found
+
+
+def get_mount_
+ (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_
+ if (path_found is None):
+ return None
+ result = path_found
+ ret = result.split()
+ label_part = find_freebsd_
+ return "/dev/" + label_part, ret[2], ret[1]
+
+
def parse_mount(path):
(mountoutput, _err) = subp("mount")
mount_locs = mountoutput.
for line in mount_locs:
m = re.search(
+ if not m:
+ continue
+ # check whether the dev refers to a label on FreeBSD
+ # for example, if dev is '/dev/label/
+ # continue finding the real device like '/dev/da0'.
+ devm = re.search(
+ if (not devm and is_FreeBSD()):
+ return get_mount_
devpth = m.group(1)
-----Original Message-----
From: Hongjiang Zhang
Sent: Tuesday, April 25, 2...
| Scott Moser (smoser) wrote : | # |
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:/
[2] https:/
>
> -----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/
>
> Hi Scott,
>
> I have removed the Azure specific check for "get_mount_info".
> get_mount_
> 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_
> +
> +
> def get_cfg_
> if key not in yobj:
> return default
> @@ -2055,11 +2059,56 @@ def parse_mtab(path):
> return None
>
>
> +def find_freebsd_
> + if label_part.
> + target_label = label_part[5:]
> + (label_part, err) = subp(['glabel', 'status', '-s'])
> + for labels in label_part.
> + items = labels.split()
> + if len(items) > 0 and items[0]
> + label_part = items[2]
> + break
> + label_part = str(label_part)
> + return label_part
> +
> +
> +def get_path_
> + path_found = None
> + for line in mnt_list.
> + items = line.split()
> + if (len(items) > 2 and os.path.
> + path_found = line
> + break
> + return path_found
> +
> +
> +def get_mount_
> + (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_
> + if (path_found is None):
> + return None
> + result = path_found
> + ...
| Hongjiang Zhang (redriver) wrote : | # |
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:/
I hope you can give some comments about another merge request: https:/
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:/
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/
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:/
[2] https:/
>
> -----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:master
>
> Hi Scott,
>
> I have removed the Azure ...
| Hongjiang Zhang (redriver) wrote : | # |
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/
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:/
I hope you can give some comments about another merge request: https:/
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:/
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/
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 ...
- 77669d5... by Hongjiang Zhang on 2017-05-04
PASSED: Continuous integration, rev:77669d5afd5
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Hongjiang Zhang (redriver) wrote : | # |
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:/
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/
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/
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:/
I hope you can give some comments about another merge request: https:/
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:/
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/
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...
- 0ff61d4... by Hongjiang Zhang on 2017-05-08
PASSED: Continuous integration, rev:0ff61d4ed2e
https:/
Executed test runs:
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
SUCCESS: https:/
Click here to trigger a rebuild:
https:/
| Scott Moser (smoser) wrote : | # |
I've merged this.
The diff from the merged commit (0a71d5a870b416
to your tip (0ff61d4) when rebased is just:
diff --git a/cloudinit/
index d9f31bfd..bad112fe 100644
--- a/cloudinit/
+++ b/cloudinit/
@@ -272,7 +272,7 @@ class 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(
cmd = ['ifconfig', ifname]
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(
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/
]
if os.uname()[0] != 'FreeBSD':
- data_files.append([
- (ETC + '/NetworkManage
+ data_files.extend([
+ (ETC + '/NetworkManage
+ ['tools/
(ETC + '/dhcp/
(LIB + '/udev/rules.d', [f for f in glob('udev/
])
| Hongjiang Zhang (redriver) wrote : | # |
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/
Review: Approve
I've merged this.
The diff from the merged commit (0a71d5a870b416
to your tip (0ff61d4) when rebased is just:
diff --git a/cloudinit/
--- a/cloudinit/
+++ b/cloudinit/
@@ -272,7 +272,7 @@ class 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(
cmd = ['ifconfig', ifname]
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(
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/
]
if os.uname()[0] != 'FreeBSD':
- data_files.append([
- (ETC + '/NetworkManage
+ data_files.extend([
+ (ETC + '/NetworkManage
+ ['tools/
(ETC + '/dhcp/
(LIB + '/udev/rules.d', [f for f in glob('udev/
])
--
https:/
You are the owner of ~redriver/
Hongjjang,
Thanks for submitting this, I will try to take a look next week.
Sorry for the slow response.