cloud-init

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

  1. Git
  2. lp:~smoser/cloud-init
  3. feature/enable-aliyun
  4. Merge into master
Proposed by Scott Moser on 2017-05-30
Status: Merged
Approved by: Scott Moser on 2017-05-30
Approved revision: 8db9cd3f44c3848574a7e690a53bb3d8962fb9f6
Merged at revision: 4a60af54957634920e84a928aa22b4fc9a6dfd11
Proposed branch: ~smoser/cloud-init:feature/enable-aliyun
Merge into: cloud-init:master
Diff against target: 231 lines (+96/-8)
7 files modified
cloudinit/settings.py (+1/-0)
cloudinit/sources/DataSourceAliYun.py (+13/-1)
cloudinit/sources/DataSourceEc2.py (+7/-0)
tests/unittests/test_datasource/test_aliyun.py (+49/-2)
tests/unittests/test_datasource/test_common.py (+1/-0)
tests/unittests/test_ds_identify.py (+18/-0)
tools/ds-identify (+7/-5)
Reviewer Review Type Date Requested Status
Junjie.Wang (community) 2017-05-30 Approve on 2017-05-31
Server Team CI bot continuous-integration 2017-05-30 Approve on 2017-05-30
Chad Smith 2017-05-30 Approve on 2017-05-30
cloud-init commiters 2017-05-30 Pending
Review via email: mp+324807@code.launchpad.net

This proposal supersedes a proposal from 2017-05-25.

Commit Message

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

To post a comment you must log in.
Scott Moser (smoser) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

This has no merge proposal shown because of bug 1693543

Server Team CI bot (server-team-bot) wrote : Posted in a previous version of this proposal
review: Needs Fixing (continuous-integration)
Server Team CI bot (server-team-bot) wrote : Posted in a previous version of this proposal
review: Approve (continuous-integration)
Junjie.Wang (jingni.wjj) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal

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) wrote : Posted in a previous version of this proposal
Download full text (5.0 KiB)

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

Read more...

Scott Moser (smoser) wrote : Posted in a previous version of this proposal

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.

Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Chad Smith (chad.smith) wrote : Posted in a previous version of this proposal

 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.

Server Team CI bot (server-team-bot) wrote :
review: Needs Fixing (continuous-integration)
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Chad Smith (chad.smith) wrote :

+1 thanks for the discussion.

review: Approve
Server Team CI bot (server-team-bot) wrote :
review: Approve (continuous-integration)
Junjie.Wang (jingni.wjj) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/cloudinit/settings.py b/cloudinit/settings.py
2index 411960d..0abd8a4 100644
3--- a/cloudinit/settings.py
4+++ b/cloudinit/settings.py
5@@ -29,6 +29,7 @@ CFG_BUILTIN = {
6 'MAAS',
7 'GCE',
8 'OpenStack',
9+ 'AliYun',
10 'Ec2',
11 'CloudSigma',
12 'CloudStack',
13diff --git a/cloudinit/sources/DataSourceAliYun.py b/cloudinit/sources/DataSourceAliYun.py
14index 9debe94..380e27c 100644
15--- a/cloudinit/sources/DataSourceAliYun.py
16+++ b/cloudinit/sources/DataSourceAliYun.py
17@@ -4,8 +4,10 @@ import os
18
19 from cloudinit import sources
20 from cloudinit.sources import DataSourceEc2 as EC2
21+from cloudinit import util
22
23 DEF_MD_VERSION = "2016-01-01"
24+ALIYUN_PRODUCT = "Alibaba Cloud ECS"
25
26
27 class DataSourceAliYun(EC2.DataSourceEc2):
28@@ -24,7 +26,17 @@ class DataSourceAliYun(EC2.DataSourceEc2):
29
30 @property
31 def cloud_platform(self):
32- return EC2.Platforms.ALIYUN
33+ if self._cloud_platform is None:
34+ if _is_aliyun():
35+ self._cloud_platform = EC2.Platforms.ALIYUN
36+ else:
37+ self._cloud_platform = EC2.Platforms.NO_EC2_METADATA
38+
39+ return self._cloud_platform
40+
41+
42+def _is_aliyun():
43+ return util.read_dmi_data('system-product-name') == ALIYUN_PRODUCT
44
45
46 def parse_public_keys(public_keys):
47diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
48index 2f9c7ed..9e2fdc0 100644
49--- a/cloudinit/sources/DataSourceEc2.py
50+++ b/cloudinit/sources/DataSourceEc2.py
51@@ -32,7 +32,12 @@ class Platforms(object):
52 AWS = "AWS"
53 BRIGHTBOX = "Brightbox"
54 SEEDED = "Seeded"
55+ # UNKNOWN indicates no positive id. If strict_id is 'warn' or 'false',
56+ # then an attempt at the Ec2 Metadata service will be made.
57 UNKNOWN = "Unknown"
58+ # NO_EC2_METADATA indicates this platform does not have a Ec2 metadata
59+ # service available. No attempt at the Ec2 Metadata service will be made.
60+ NO_EC2_METADATA = "No-EC2-Metadata"
61
62
63 class DataSourceEc2(sources.DataSource):
64@@ -65,6 +70,8 @@ class DataSourceEc2(sources.DataSource):
65 strict_mode, self.cloud_platform)
66 if strict_mode == "true" and self.cloud_platform == Platforms.UNKNOWN:
67 return False
68+ elif self.cloud_platform == Platforms.NO_EC2_METADATA:
69+ return False
70
71 try:
72 if not self.wait_for_metadata_service():
73diff --git a/tests/unittests/test_datasource/test_aliyun.py b/tests/unittests/test_datasource/test_aliyun.py
74index c16d1a6..990bff2 100644
75--- a/tests/unittests/test_datasource/test_aliyun.py
76+++ b/tests/unittests/test_datasource/test_aliyun.py
77@@ -2,6 +2,7 @@
78
79 import functools
80 import httpretty
81+import mock
82 import os
83
84 from .. import helpers as test_helpers
85@@ -111,15 +112,29 @@ class TestAliYunDatasource(test_helpers.HttprettyTestCase):
86 self.assertEqual(self.default_metadata['hostname'],
87 self.ds.get_hostname())
88
89+ @mock.patch("cloudinit.sources.DataSourceAliYun._is_aliyun")
90 @httpretty.activate
91- def test_with_mock_server(self):
92+ def test_with_mock_server(self, m_is_aliyun):
93+ m_is_aliyun.return_value = True
94 self.regist_default_server()
95- self.ds.get_data()
96+ ret = self.ds.get_data()
97+ self.assertEqual(True, ret)
98+ self.assertEqual(1, m_is_aliyun.call_count)
99 self._test_get_data()
100 self._test_get_sshkey()
101 self._test_get_iid()
102 self._test_host_name()
103
104+ @mock.patch("cloudinit.sources.DataSourceAliYun._is_aliyun")
105+ @httpretty.activate
106+ def test_returns_false_when_not_on_aliyun(self, m_is_aliyun):
107+ """If is_aliyun returns false, then get_data should return False."""
108+ m_is_aliyun.return_value = False
109+ self.regist_default_server()
110+ ret = self.ds.get_data()
111+ self.assertEqual(1, m_is_aliyun.call_count)
112+ self.assertEqual(False, ret)
113+
114 def test_parse_public_keys(self):
115 public_keys = {}
116 self.assertEqual(ay.parse_public_keys(public_keys), [])
117@@ -149,4 +164,36 @@ class TestAliYunDatasource(test_helpers.HttprettyTestCase):
118 self.assertEqual(ay.parse_public_keys(public_keys),
119 public_keys['key-pair-0']['openssh-key'])
120
121+
122+class TestIsAliYun(test_helpers.CiTestCase):
123+ ALIYUN_PRODUCT = 'Alibaba Cloud ECS'
124+ read_dmi_data_expected = [mock.call('system-product-name')]
125+
126+ @mock.patch("cloudinit.sources.DataSourceAliYun.util.read_dmi_data")
127+ def test_true_on_aliyun_product(self, m_read_dmi_data):
128+ """Should return true if the dmi product data has expected value."""
129+ m_read_dmi_data.return_value = self.ALIYUN_PRODUCT
130+ ret = ay._is_aliyun()
131+ self.assertEqual(self.read_dmi_data_expected,
132+ m_read_dmi_data.call_args_list)
133+ self.assertEqual(True, ret)
134+
135+ @mock.patch("cloudinit.sources.DataSourceAliYun.util.read_dmi_data")
136+ def test_false_on_empty_string(self, m_read_dmi_data):
137+ """Should return false on empty value returned."""
138+ m_read_dmi_data.return_value = ""
139+ ret = ay._is_aliyun()
140+ self.assertEqual(self.read_dmi_data_expected,
141+ m_read_dmi_data.call_args_list)
142+ self.assertEqual(False, ret)
143+
144+ @mock.patch("cloudinit.sources.DataSourceAliYun.util.read_dmi_data")
145+ def test_false_on_unknown_string(self, m_read_dmi_data):
146+ """Should return false on an unrelated string."""
147+ m_read_dmi_data.return_value = "cubs win"
148+ ret = ay._is_aliyun()
149+ self.assertEqual(self.read_dmi_data_expected,
150+ m_read_dmi_data.call_args_list)
151+ self.assertEqual(False, ret)
152+
153 # vi: ts=4 expandtab
154diff --git a/tests/unittests/test_datasource/test_common.py b/tests/unittests/test_datasource/test_common.py
155index c08717f..7649b9a 100644
156--- a/tests/unittests/test_datasource/test_common.py
157+++ b/tests/unittests/test_datasource/test_common.py
158@@ -36,6 +36,7 @@ DEFAULT_LOCAL = [
159 ]
160
161 DEFAULT_NETWORK = [
162+ AliYun.DataSourceAliYun,
163 AltCloud.DataSourceAltCloud,
164 Azure.DataSourceAzureNet,
165 Bigstep.DataSourceBigstep,
166diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
167index f5694b2..5c26e65 100644
168--- a/tests/unittests/test_ds_identify.py
169+++ b/tests/unittests/test_ds_identify.py
170@@ -220,6 +220,20 @@ class TestDsIdentify(CiTestCase):
171 mydata['files'][cfgpath] = 'datasource_list: ["Ec2", "None"]\n'
172 self._check_via_dict(mydata, rc=RC_FOUND, dslist=['Ec2', DS_NONE])
173
174+ def test_aliyun_identified(self):
175+ """Test that Aliyun cloud is identified by product id."""
176+ self._test_ds_found('AliYun')
177+
178+ def test_aliyun_over_ec2(self):
179+ """Even if all other factors identified Ec2, AliYun should be used."""
180+ mydata = copy.deepcopy(VALID_CFG['Ec2-xen'])
181+ self._test_ds_found('AliYun')
182+ prod_name = VALID_CFG['AliYun']['files'][P_PRODUCT_NAME]
183+ mydata['files'][P_PRODUCT_NAME] = prod_name
184+ policy = "search,found=first,maybe=none,notfound=disabled"
185+ self._check_via_dict(mydata, rc=RC_FOUND, dslist=['AliYun', DS_NONE],
186+ policy_dmi=policy)
187+
188
189 def blkid_out(disks=None):
190 """Convert a list of disk dictionaries into blkid content."""
191@@ -254,6 +268,10 @@ def _print_run_output(rc, out, err, cfg, files):
192
193
194 VALID_CFG = {
195+ 'AliYun': {
196+ 'ds': 'AliYun',
197+ 'files': {P_PRODUCT_NAME: 'Alibaba Cloud ECS\n'},
198+ },
199 'Ec2-hvm': {
200 'ds': 'Ec2',
201 'mocks': [{'name': 'detect_virt', 'RET': 'kvm', 'ret': 0}],
202diff --git a/tools/ds-identify b/tools/ds-identify
203index 74d2653..5fc500b 100755
204--- a/tools/ds-identify
205+++ b/tools/ds-identify
206@@ -110,7 +110,8 @@ DI_DSNAME=""
207 # this has to match the builtin list in cloud-init, it is what will
208 # be searched if there is no setting found in config.
209 DI_DSLIST_DEFAULT="MAAS ConfigDrive NoCloud AltCloud Azure Bigstep \
210-CloudSigma CloudStack DigitalOcean Ec2 GCE OpenNebula OpenStack OVF SmartOS"
211+CloudSigma CloudStack DigitalOcean AliYun Ec2 GCE OpenNebula OpenStack \
212+OVF SmartOS"
213 DI_DSLIST=""
214 DI_MODE=""
215 DI_ON_FOUND=""
216@@ -821,10 +822,11 @@ dscheck_OpenStack() {
217 }
218
219 dscheck_AliYun() {
220- # aliyun is not enabled by default (LP: #1638931)
221- # so if we are here, it is because the datasource_list was
222- # set to include it. Thus, 'maybe'.
223- return $DS_MAYBE
224+ check_seed_dir "AliYun" meta-data user-data && return ${DS_FOUND}
225+ if dmi_product_name_is "Alibaba Cloud ECS"; then
226+ return $DS_FOUND
227+ fi
228+ return $DS_NOT_FOUND
229 }
230
231 dscheck_AltCloud() {

Subscribers

People subscribed via source and target branches

to all changes: