Merge ~smoser/cloud-init:aliyun-datasource into ~kaihuan-pkh/cloud-init:aliyun-datasource

[Scott Moser]

Hi,

I've made some changes here locally, and they seem to work fine.

For a squashed commit message, i suggest:

   AliYun: Add new datasource for Ali-Cloud ECS

   Support AliYun(Ali-Cloud ECS). This datasource inherits from EC2,
   the main difference is the meta-server address is changed to
   100.100.100.200.

   The datasource behaves similarly to EC2 and relies on network polling.
   As such, it is not enabled by default.

[lawrence peng (kaihuan-pkh)]

review: Approve

[lawrence peng (kaihuan-pkh)]

review: Approve

[lawrence peng (kaihuan-pkh)]

hi smoser,

I have reviewed your changes and run `make test` passed. but I have some questions:

1. how to make this request (https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/309614) merged to my branch automatically ? I found the only one thing I can do is just change the status from 'needs review' to 'approved' or others. If I change the status to 'merged' but nothing happend, so maybe it's not like github ? if I want to do the real merge to my code, what should I do ?

2. my merge request (https://code.launchpad.net/~kaihuan-pkh/cloud-init/+git/cloud-init/+merge/308483) is now status changed to merged, then I found the master commit is some different with my local branch changes, it added your work; but as question 1 said, my branch is not changed, how it happend ?

3. I found my code not enable DataSourceAliYun by default (it's a mistake), and I wish it could be used like other datasources, so I need add it to cloudinit/settings.py, if I add it to my local branch, should I merge your commit first ? and what sequence to put 'AliYun' insert into settings.py:datasource_list ?

[Scott Moser (smoser)]

On Wed, 2 Nov 2016, lawrence peng wrote:

> hi smoser,
>
>
> I have reviewed your changes and run `make test` passed. but I have some questions:
>
> 1. how to make this request (https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/309614) merged to my branch automatically ? I found the only one thing I can do is just change the status from 'needs review' to 'approved' or others. If I change the status to 'merged' but nothing happend, so maybe it's not like github ? if I want to do the real merge to my code, what should I do ?

launchpad doesn't have any push-button "merge" support. I marked both
branches as 'merged', and then rebased and "squashed" your change to go on
master and merged it. And then pushed to master.

Then, because the commit is different you have to also actually tell
launchpad what commit *was* the one that was merged. You don't have to do
this, but I often to just for completeness.

I generally prefer this method as I think it makes the history more
correct, and gives one sane commit message on the change. I could have
just done a 'git merge' from your branch and then launchpad would have
recognized the branch was merged, but all your intermediate commits would
show in 'git log'.

> 2. my merge request (https://code.launchpad.net/~kaihuan-pkh/cloud-init/+git/cloud-init/+merge/308483) is now status changed to merged, then I found the master commit is some different with my local branch changes, it added your work; but as question 1 said, my branch is not changed, how it happend ?

Yeah, you have to merge yourself and push, or do something and manually
mark as 'merged.

>
> 3. I found my code not enable DataSourceAliYun by default (it's a
> mistake), and I wish it could be used like other datasources, so I need
> add it to cloudinit/settings.py, if I add it to my local branch, should
> I merge your commit first ? and what sequence to put 'AliYun' insert
> into settings.py:datasource_list ?

Well, we can't really enable your datasource by default. This is
unfortunate, but polling on network locations is not really ideal.
Currently the only datasource that is enabled by default that does this is
EC2, and that is really only for legacy reasons.

In order to enable the datasource by default, we need to have a way to
identify without network that this instance is on AliYun. Many of the
other cloud providers provide a smbios identification, or some other way
that we definitively determine that we are on that cloud. Then, if we
don't see that we get out fast without the network polling.

Scott

[lawrence peng (kaihuan-pkh)]

> > 3. I found my code not enable DataSourceAliYun by default (it's a
> > mistake), and I wish it could be used like other datasources, so I need
> > add it to cloudinit/settings.py, if I add it to my local branch, should
> > I merge your commit first ? and what sequence to put 'AliYun' insert
> > into settings.py:datasource_list ?
>
> Well, we can't really enable your datasource by default. This is
> unfortunate, but polling on network locations is not really ideal.
> Currently the only datasource that is enabled by default that does this is
> EC2, and that is really only for legacy reasons.
>
> In order to enable the datasource by default, we need to have a way to
> identify without network that this instance is on AliYun. Many of the
> other cloud providers provide a smbios identification, or some other way
> that we definitively determine that we are on that cloud. Then, if we
> don't see that we get out fast without the network polling.
>
> Scott

so I think the primary reason is that cloud-init will block a long time when call datesource's get_data() method if it's use network ?

in our cloud platform, an instance vm could use an asm instructions `cpuid` (with eax setted to 0x40000100 ), it will returned our platform id keyword "AliYun" into register ebx and ecx, and if vm is not running on our environment, the value is others. so I guess this is a method to identify our platform ?

if I change DataSourceAliYun: get_date() method, first I use the method above to detect the evironment, if it is on AliYun, then I could call the super class (ec2) get_date() to fetch meta-date from our meta-server by network. do this ok ?

[Scott Moser (smoser)]

Yeah, that sounds great.
Please make sure it will work (or return without exception) on non Intel arch. And really don't want to pick up a dependency for this.

Thanks!

On November 3, 2016 4:24:25 AM EDT, lawrence peng <email address hidden> wrote:
>> > 3. I found my code not enable DataSourceAliYun by default (it's a
>> > mistake), and I wish it could be used like other datasources, so I
>need
>> > add it to cloudinit/settings.py, if I add it to my local branch,
>should
>> > I merge your commit first ? and what sequence to put 'AliYun'
>insert
>> > into settings.py:datasource_list ?
>>
>> Well, we can't really enable your datasource by default. This is
>> unfortunate, but polling on network locations is not really ideal.
>> Currently the only datasource that is enabled by default that does
>this is
>> EC2, and that is really only for legacy reasons.
>>
>> In order to enable the datasource by default, we need to have a way
>to
>> identify without network that this instance is on AliYun. Many of
>the
>> other cloud providers provide a smbios identification, or some other
>way
>> that we definitively determine that we are on that cloud. Then, if
>we
>> don't see that we get out fast without the network polling.
>>
>> Scott
>
>so I think the primary reason is that cloud-init will block a long time
>when call datesource's get_data() method if it's use network ?
>
>in our cloud platform, an instance vm could use an asm instructions
>`cpuid` (with eax setted to 0x40000100 ), it will returned our
>platform id keyword "AliYun" into register ebx and ecx, and if vm is
>not running on our environment, the value is others. so I guess this
>is a method to identify our platform ?
>
>if I change DataSourceAliYun: get_date() method, first I use the method
>above to detect the evironment, if it is on AliYun, then I could call
>the super class (ec2) get_date() to fetch meta-date from our
>meta-server by network. do this ok ?
>
>
>
>
>
>--
>https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/309614
>You are the owner of ~smoser/cloud-init:aliyun-datasource.

[Scott Moser (smoser)]

I opened bug 1638931 to track the enabling of the datasource.

Lets move any further conversation there.

Thanks Lawrence.

diff --git a/cloudinit/sources/DataSourceAliYun.py b/cloudinit/sources/DataSourceAliYun.py
index 23a65b7..1995721 100644
--- a/cloudinit/sources/DataSourceAliYun.py
+++ b/cloudinit/sources/DataSourceAliYun.py
@@ -1,75 +1,21 @@
 # vi: ts=4 expandtab
 
 import os
-import time
 
-from cloudinit import log as logging
 from cloudinit import sources
-from cloudinit import url_helper as uhelp
-from cloudinit import util
-
 from cloudinit.sources import DataSourceEc2 as EC2
 
-LOG = logging.getLogger(__name__)
-
-DEF_MD_URL = "http://100.100.100.200"
-
 DEF_MD_VERSION = "2016-01-01"
 
-# Default metadata urls that will be used if none are provided
-# They will be checked for 'resolveability' and some of the
-# following may be discarded if they do not resolve
-DEF_MD_URLS = [DEF_MD_URL, ]
-
 
 class DataSourceAliYun(EC2.DataSourceEc2):
+    metadata_urls = ["http://100.100.100.200"]
+
     def __init__(self, sys_cfg, distro, paths):
         super(DataSourceAliYun, self).__init__(sys_cfg, distro, paths)
-        self.metadata_address = DEF_MD_URL
         self.seed_dir = os.path.join(paths.seed_dir, "AliYun")
         self.api_ver = DEF_MD_VERSION
 
-    def wait_for_metadata_service(self):
-        mcfg = self.ds_cfg
-
-        (max_wait, timeout) = self._get_url_settings()
-        if max_wait <= 0:
-            return False
-
-        # Remove addresses from the list that wont resolve.
-        mdurls = mcfg.get("metadata_urls", DEF_MD_URLS)
-        filtered = [x for x in mdurls if util.is_resolvable_url(x)]
-
-        if set(filtered) != set(mdurls):
-            LOG.debug("Removed the following from metadata urls: %s",
-                      list((set(mdurls) - set(filtered))))
-
-        if len(filtered):
-            mdurls = filtered
-        else:
-            LOG.warn("Empty metadata url list! using default list")
-            mdurls = DEF_MD_URLS
-
-        urls = []
-        url2base = {}
-        for url in mdurls:
-            cur = "%s/%s/meta-data/instance-id" % (url, self.api_ver)
-            urls.append(cur)
-            url2base[cur] = url
-
-        start_time = time.time()
-        url = uhelp.wait_for_url(urls=urls, max_wait=max_wait,
-                                 timeout=timeout, status_cb=LOG.warn)
-
-        if url:
-            LOG.debug("Using metadata source: '%s'", url2base[url])
-        else:
-            LOG.critical("Giving up on md from %s after %s seconds",
-                         urls, int(time.time() - start_time))
-
-        self.metadata_address = url2base.get(url)
-        return bool(url)
-
     def get_hostname(self, fqdn=False, _resolve_ip=False):
         return self.metadata.get('hostname', 'localhost.localdomain')
 
@@ -77,17 +23,6 @@ class DataSourceAliYun(EC2.DataSourceEc2):
         return parse_public_keys(self.metadata.get('public-keys', {}))
 
 
-# Used to match classes to dependencies
-datasources = [
-    (DataSourceAliYun, (sources.DEP_FILESYSTEM, sources.DEP_NETWORK)),
-]
-
-
-# Return a list of data sources that match this set of dependencies
-def get_datasource_list(depends):
-    return sources.list_from_depends(depends, datasources)
-
-
 def parse_public_keys(public_keys):
     keys = []
     for key_id, key_body in public_keys.items():
@@ -102,3 +37,13 @@ def parse_public_keys(public_keys):
             elif isinstance(key, list):
                 keys.extend(key)
     return keys
+
+# Used to match classes to dependencies
+datasources = [
+    (DataSourceAliYun, (sources.DEP_FILESYSTEM, sources.DEP_NETWORK)),
+]
+
+
+# Return a list of data sources that match this set of dependencies
+def get_datasource_list(depends):
+    return sources.list_from_depends(depends, datasources)
diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 6fe2a0b..bc84ef5 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -31,21 +31,19 @@ from cloudinit import util
 
 LOG = logging.getLogger(__name__)
 
-DEF_MD_URL = "http://169.254.169.254"
-
 # Which version we are requesting of the ec2 metadata apis
 DEF_MD_VERSION = '2009-04-04'
 
-# Default metadata urls that will be used if none are provided
-# They will be checked for 'resolveability' and some of the
-# following may be discarded if they do not resolve
-DEF_MD_URLS = [DEF_MD_URL, "http://instance-data.:8773"]
-
 
 class DataSourceEc2(sources.DataSource):
+    # Default metadata urls that will be used if none are provided
+    # They will be checked for 'resolveability' and some of the
+    # following may be discarded if they do not resolve
+    metadata_urls = ["http://169.254.169.254", "http://instance-data.:8773"]
+
     def __init__(self, sys_cfg, distro, paths):
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
-        self.metadata_address = DEF_MD_URL
+        self.metadata_address = None
         self.seed_dir = os.path.join(paths.seed_dir, "ec2")
         self.api_ver = DEF_MD_VERSION
 
@@ -106,7 +104,7 @@ class DataSourceEc2(sources.DataSource):
             return False
 
         # Remove addresses from the list that wont resolve.
-        mdurls = mcfg.get("metadata_urls", DEF_MD_URLS)
+        mdurls = mcfg.get("metadata_urls", self.metadata_urls)
         filtered = [x for x in mdurls if util.is_resolvable_url(x)]
 
         if set(filtered) != set(mdurls):
@@ -117,7 +115,7 @@ class DataSourceEc2(sources.DataSource):
             mdurls = filtered
         else:
             LOG.warn("Empty metadata url list! using default list")
-            mdurls = DEF_MD_URLS
+            mdurls = self.metadata_urls
 
         urls = []
         url2base = {}
diff --git a/tests/unittests/test_datasource/test_aliyun.py b/tests/unittests/test_datasource/test_aliyun.py
index d7d4e99..6f1de07 100644
--- a/tests/unittests/test_datasource/test_aliyun.py
+++ b/tests/unittests/test_datasource/test_aliyun.py
@@ -66,7 +66,7 @@ class TestAliYunDatasource(test_helpers.HttprettyTestCase):
         distro = {}
         paths = helpers.Paths({})
         self.ds = ay.DataSourceAliYun(cfg, distro, paths)
-        self.metadata_address = self.ds.metadata_address
+        self.metadata_address = self.ds.metadata_urls[0]
         self.api_ver = self.ds.api_ver
 
     @property