Merge ~smoser/cloud-init:feature/enable-aliyun into cloud-init:master

[Scott Moser]

AliYun: Enable platform identification and enable by default.

AliYun cloud platform is now identifying themselves by setting the dmi
product id to the well known value "Alibaba Cloud ECS". The changes here
identify that properly in tools/ds-identify and in the DataSourceAliYun.

Since the 'get_data' for AliYun now identifies itself correctly, we can
enable AliYun by default.

LP: #1638931

[Scott Moser (smoser)]

This supersedes https://code.launchpad.net/~jingni.wjj/cloud-init/+git/cloud-init/+merge/323306.
It has some re-wording of Junjie's commit, and then a commit to show my additional changes.

[Scott Moser (smoser)]

This has no merge proposal shown because of bug 1693543

[Server Team CI bot (server-team-bot)]

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

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

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

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

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

review: Approve (continuous-integration)

[Junjie.Wang (jingni.wjj)]

Hi,smoser
    I review this code change at
    https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/324625
    that good ,and test it in our AliYun platform ,it's also work fine .thanks

review: Approve

[Chad Smith (chad.smith)]

Thanks for this good work approved pending your decision on the following comments.

My comments will be made per the consolidated visual diff @ http://paste.ubuntu.com/24716726/

line 49 in the above diff:
 Since order of datasources matters to determine which datasource is detectred first in both cloudinit/settings.py CFG_BUILTIN and tools/ds-identify DI_DSLIST_DEFAULT. Why are these two lists not ordered the same? AliYun is placed just before Ec2 in ds-identify but way at the beginning settings.py. Should we make an attempt to ensure both lists are consistently ordered?

line 95:
- Please add a comment in the code about the intent of NO_EC2_METADATA in in cloudinit/sources/DataSourceEc2.py as you did in the commit message. The other devs looking at the code for a subclass of DataSourceEc2 could know how and why to make use of this value. commit messages are lost, comments live forever :).

line 103 cloudinit/sources/DataSourceAliYun.py:
- s/NO_EC2_MD/NO_EC2_METADATA/ let's reduce two letter acronyms throughout the code as it frequently involves a second glance to figure out what the intent of the variable is to determine context.

line 104 cloudinit/sources/DataSourceAliYun.py:
I see no unit test coverage of "elif self.cloud_platform == Platforms.NO_EC2_MD:"

line 124 tests/unittests/test_datasource/test_aliyun.py:
   Since we have mocked _is_aliyun we aren't really testing anywhere that we are getting this info from dmi. It might be better to mock util.read_dmi_data as that is the interface we are calling into. Then we can validate the calls into that interface as follows:

    @mock.patch("cloudinit.util.read_dmi_data")
    @httpretty.activate
    def test_with_mock_server(self, m_read_dmi):
        m_read_dmi.return_value = ay.ALIYUN_PRODUCT
        self.regist_default_server()
        self.ds.get_data()
        self._test_get_data()
        self._test_get_sshkey()
        self._test_get_iid()
        self._test_host_name()
        m_read_dmi.assert_called_with('system-product-name')

line 140 tests/unittests/test_datasource/test_common.py
   These network tests feel like they aren't all testing the right things. Why is the test
   Why is test_expected_nondefault_network_sources_found still passing or needed? I was expecting to see that AliYun is dropped from nondefault sources.

line 175: Since DataSourceAliYum.ALIYUN_PRODUCT is already defined in the module, we should use that variable in unit tests instead VALID_CFGs instead of re-typing the string so there is only one source of truth for this data. It also aids in grepping source to find out where ALIYUN_PRODUCT is used throughout code and unit tests.

review: Approve

[Scott Moser (smoser)]

> Thanks for this good work approved pending your decision on the following
> comments.
>
> My comments will be made per the consolidated visual diff @
> http://paste.ubuntu.com/24716726/
>
> line 49 in the above diff:
> Since order of datasources matters to determine which datasource is detectred
> first in both cloudinit/settings.py CFG_BUILTIN and tools/ds-identify
> DI_DSLIST_DEFAULT. Why are these two lists not ordered the same? AliYun is
> placed just before Ec2 in ds-identify but way at the beginning settings.py.
> Should we make an attempt to ensure both lists are consistently ordered?

I think it probably makes sense to put AliYun at the end on both lists
at this point. I will make that change.

the list in ds_identify is only used in the case that there is no explicit
list defined in /etc/cloud/cloud.cfg (or cfg.d/*). So its not that common
of a case that this list is referenced (at least not in Ubuntu where the
package contains a list of datasources that can be configured with
dpkg-reconfigure).

The list that is in ds-identify right now is kind of done over iterative
development when i first worked on it. It started out as a list
where I had definitive sources at the front and network datasources
at the back ('Ec2', 'GCE'...).

I believe I put 'Aliyun' before 'Ec2' because I was thinking that Ec2 might
end up claiming 'found' on Aliyun cloud. That is now not the case unless
Aliyun starts crafting dmi data that looks like Ec2... which would break
themselves.

I think we can/should put some more thought into how this list (and that in
settings.py) is maintained. Once 'strict_id' is turned on in EC2 by default
we will really be able to re-order almost at whim.

> line 95:
> - Please add a comment in the code about the intent of NO_EC2_METADATA in in
> cloudinit/sources/DataSourceEc2.py as you did in the commit message. The other
> devs looking at the code for a subclass of DataSourceEc2 could know how and
> why to make use of this value. commit messages are lost, comments live forever
> :).

Well, I sort of agree. (I read 'git log' and 'git blame' a lot, and expect
that other developers do also. I don't take the time to write useful
commit messages for no reason).

> line 103 cloudinit/sources/DataSourceAliYun.py:
> - s/NO_EC2_MD/NO_EC2_METADATA/ let's reduce two letter acronyms throughout the
> code as it frequently involves a second glance to figure out what the intent
> of the variable is to determine context.

OK.

>
> line 104 cloudinit/sources/DataSourceAliYun.py:
> I see no unit test coverage of "elif self.cloud_platform ==
> Platforms.NO_EC2_MD:"

i'll try to put something together.

> line 124 tests/unittests/test_datasource/test_aliyun.py:
> Since we have mocked _is_aliyun we aren't really testing anywhere that we
> are getting this info from dmi. It might be better to mock util.read_dmi_data
> as that is the interface we are calling into. Then we can validate the calls
> into that interface as follows:
>
> @mock.patch("cloudinit.util.read_dmi_data")
> @httpretty.activate
> def test_with_mock_server(self, m_read_dmi):
> m_read_dmi.return_value = ay.ALIYUN_PRODUCT
> self.regist_defa...

[Scott Moser (smoser)]

OK, So i thought some more, and with regard to the order of the two 'datasource_lists'.

Some points to think about
a.) If EC2's 'strict_id' is set to "warn" or "false", then the Ec2 datasource
will poll and time out very annoyingly. "true" is the default setting in
trunk, but not yet in xenial. historically, Ec2 was the first "polling"
datasource. Any sources that did not positively identify themselves had to be
after Ec2 in the default list. That was because "the early bird gets the
worm". Basically in the event of cloud-init not having good information on
where it was running Ec2 got first dibs.

b.) the order in tools/ds-identify doesnt matter so much.
Remember that this list is only consulted if there is no datasource_list declared in /etc/cloud/cloud.cfg or /etc/cloud/cloud.cfg.d/*. So in that situation
the following matter:
 i.) if there were multiple datasources "DS_FOUND" (or DS_MAYBE), then they are placed into the list in the order of the DI_DSLIST_DEFAULT. All DS_FOUND preceeed DS_MAYBE in such a list.
 ii.) if there are no DS_FOUND by ds-identify, and strict_id is false, then Ec2 would be return DS_MAYBE. Having it first in that list means others after it would have to wait.

[Chad Smith (chad.smith)]

 Thanks for the clarification and background on the datasource list ordering. And I see your point on git log (as that's how I determined what the intent of the var was in the first place).

+1 on the separate mocked unit test for _is_aliyun() to exercise the interface we are actually calling. It'd be nice at some point if we had a fake dmidecode or SMBIOS test resource which could be used to seed datasource testing. Then we wouldn't really have to 'mock', just inject required dmi values and let things run.

Per the importing the ay.ALIYUN_PRODUCT variable into unit tests, I agree with your consistency comment. Having a module variable imported into that module's unit tests only asserts that the module consistently references whatever the variable's value is (correct or incorrect). I'd suggest though that copying that string directly into the unit test does the same type of conisitency validation. The module writer could have defined a variable which won't work on aliyun systems in the first place, and then tested that ALIYUN_PRODUCT == "blah" as well. It doesn't actually assert that this value will work on aliyun environments. We'd probably have to rely on integration tests for that.

Also this suggestion was to try to also bridge some consistency between our cloudinit/source/DataSourceAliyun.py and ds-identify. It feels like things are somewhat shallow in the consistency checks between tools/ds-identify logic and datasource detection logic in cloudinit/source/DataSource*py. It's probably due in part to the transition to strict ds validation etc. In a perfect world all this common logic to parse DMI data or metadata could be in one place, but I realize ds-identify has some constraints on where it is run (and speed) so loading up python modules probably doesn't make sense.