From 5fc46193d3e65d0eaaaa45bcf41c5b35b4e80df7 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Tue, 26 Aug 2014 13:41:46 -0700 Subject: Optimize away most of the path_exists checks --- cloudinit/sources/helpers/openstack.py | 101 +++++++++++++++++---------------- 1 file changed, 52 insertions(+), 49 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 0fac0335..361c9994 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -149,10 +149,6 @@ class BaseReader(object): def _path_join(self, base, *add_ons): pass - @abc.abstractmethod - def _path_exists(self, path): - pass - @abc.abstractmethod def _path_read(self, path): pass @@ -170,22 +166,9 @@ class BaseReader(object): path = self._path_join(self.base_path, "openstack", *path_pieces) return self._path_read(path) + @abc.abstractmethod def _find_working_version(self, version): - search_versions = [version] + list(OS_VERSIONS) - for potential_version in search_versions: - if not potential_version: - continue - path = self._path_join(self.base_path, "openstack", - potential_version) - if self._path_exists(path): - if potential_version != version: - LOG.debug("Version '%s' not available, attempting to use" - " version '%s' instead", version, - potential_version) - return potential_version - LOG.debug("Version '%s' not available, attempting to use '%s'" - " instead", version, OS_LATEST) - return OS_LATEST + pass def read_v2(self, version=None): """Reads a version 2 formatted location. @@ -228,15 +211,14 @@ class BaseReader(object): path = self._path_join(self.base_path, path) data = None found = False - if self._path_exists(path): - try: - data = self._path_read(path) - except IOError: - raise NonReadable("Failed to read: %s" % path) - found = True + try: + data = self._path_read(path) + except IOError: + pass else: - if required: - raise NonReadable("Missing mandatory path: %s" % path) + found = True + if required and not found: + raise NonReadable("Missing mandatory path: %s" % path) if found and translator: try: data = translator(data) @@ -315,6 +297,23 @@ class ConfigDriveReader(BaseReader): def _path_read(self, path): return util.load_file(path) + def _find_working_version(self, version): + search_versions = [version] + list(OS_VERSIONS) + for potential_version in search_versions: + if not potential_version: + continue + path = self._path_join(self.base_path, "openstack", + potential_version) + if self._path_exists(path): + if potential_version != version: + LOG.debug("Version '%s' not available, attempting to use" + " version '%s' instead", version, + potential_version) + return potential_version + LOG.debug("Version '%s' not available, attempting to use '%s'" + " instead", version, OS_LATEST) + return OS_LATEST + def _read_ec2_metadata(self): path = self._path_join(self.base_path, 'ec2', 'latest', 'meta-data.json') @@ -401,6 +400,32 @@ class MetadataReader(BaseReader): self.timeout = float(timeout) self.retries = int(retries) + def _find_working_version(self, version): + search_versions = [version] + list(OS_VERSIONS) + version_path = self._path_join(self.base_path, "openstack") + versions_available = [] + try: + versions = self._path_read(version_path) + except IOError: + pass + else: + for line in versions.splitlines(): + line = line.strip() + if not line: + continue + versions_available.append(line) + for potential_version in search_versions: + if potential_version not in versions_available: + continue + if potential_version != version: + LOG.debug("Version '%s' not available, attempting to use" + " version '%s' instead", version, + potential_version) + return potential_version + LOG.debug("Version '%s' not available, searched %s, attempting to" + " use '%s' instead", version, search_versions, OS_LATEST) + return OS_LATEST + def _path_read(self, path): response = url_helper.readurl(path, retries=self.retries, @@ -408,28 +433,6 @@ class MetadataReader(BaseReader): timeout=self.timeout) return response.contents - def _path_exists(self, path): - - def should_retry_cb(request, cause): - try: - code = int(cause.code) - if code >= 400: - return False - except (TypeError, ValueError): - # Older versions of requests didn't have a code. - pass - return True - - try: - response = url_helper.readurl(path, - retries=self.retries, - ssl_details=self.ssl_details, - timeout=self.timeout, - exception_cb=should_retry_cb) - return response.ok() - except IOError: - return False - def _path_join(self, base, *add_ons): return url_helper.combine_url(base, *add_ons) -- cgit v1.2.3 From cc9e3af6c95b3263a49d4590d9dd176bdc570c99 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Tue, 26 Aug 2014 23:02:58 -0700 Subject: Add logging around failure to read optional/mandatory paths --- cloudinit/sources/helpers/openstack.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 361c9994..e5a38de0 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -213,8 +213,12 @@ class BaseReader(object): found = False try: data = self._path_read(path) - except IOError: - pass + except IOError as e: + if not required: + LOG.debug("Failed reading optional path %s due" + " to: %s", path, e) + else: + LOG.exception("Failed reading mandatory path %s", path) else: found = True if required and not found: -- cgit v1.2.3 From 6a6d3a499c2327b03993bbaea2b9d0df5dc7eb64 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Tue, 26 Aug 2014 23:04:56 -0700 Subject: Just use os.path.exists directly --- cloudinit/sources/helpers/openstack.py | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index e5a38de0..418ab4d1 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -295,9 +295,6 @@ class ConfigDriveReader(BaseReader): components = [base] + list(add_ons) return os.path.join(*components) - def _path_exists(self, path): - return os.path.exists(path) - def _path_read(self, path): return util.load_file(path) @@ -308,7 +305,7 @@ class ConfigDriveReader(BaseReader): continue path = self._path_join(self.base_path, "openstack", potential_version) - if self._path_exists(path): + if os.path.exists(path): if potential_version != version: LOG.debug("Version '%s' not available, attempting to use" " version '%s' instead", version, @@ -321,7 +318,7 @@ class ConfigDriveReader(BaseReader): def _read_ec2_metadata(self): path = self._path_join(self.base_path, 'ec2', 'latest', 'meta-data.json') - if not self._path_exists(path): + if not os.path.exists(path): return {} else: try: @@ -341,7 +338,7 @@ class ConfigDriveReader(BaseReader): found = {} for name in FILES_V1.keys(): path = self._path_join(self.base_path, name) - if self._path_exists(path): + if os.path.exists(path): found[name] = path if len(found) == 0: raise NonReadable("%s: no files found" % (self.base_path)) -- cgit v1.2.3 From 9a7587f35f327b9f8cafce8687832e9e77c1afde Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Tue, 26 Aug 2014 23:08:14 -0700 Subject: Log a warning when unable to fetch the openstack versions --- cloudinit/sources/helpers/openstack.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 418ab4d1..56986a94 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -407,8 +407,9 @@ class MetadataReader(BaseReader): versions_available = [] try: versions = self._path_read(version_path) - except IOError: - pass + except IOError as e: + LOG.warn("Unable to read openstack versions from %s due" + " to: %s", version_path, e) else: for line in versions.splitlines(): line = line.strip() -- cgit v1.2.3 From fb482ce4a36d9b4be75a8bf5b428189548a205d9 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Tue, 26 Aug 2014 23:10:50 -0700 Subject: Show the available versions in the debug log message --- cloudinit/sources/helpers/openstack.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 56986a94..3ceec837 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -424,8 +424,9 @@ class MetadataReader(BaseReader): " version '%s' instead", version, potential_version) return potential_version - LOG.debug("Version '%s' not available, searched %s, attempting to" - " use '%s' instead", version, search_versions, OS_LATEST) + LOG.debug("Version '%s' not available, searched for %s (with available" + " versions being %s), attempting to use '%s' instead", + version, search_versions, versions_available, OS_LATEST) return OS_LATEST def _path_read(self, path): -- cgit v1.2.3 From d701035265c765bc42cb3bc358f2bfd0b41f484b Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Wed, 27 Aug 2014 12:30:23 -0700 Subject: Fixed more of the slowness around fetching and retrying --- cloudinit/sources/helpers/openstack.py | 112 +++++++++++++--------- tests/unittests/test_datasource/test_openstack.py | 23 ++++- 2 files changed, 84 insertions(+), 51 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 3ceec837..2d0fc70e 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -21,6 +21,7 @@ import abc import base64 import copy +import httplib import os from cloudinit import ec2_utils @@ -153,10 +154,31 @@ class BaseReader(object): def _path_read(self, path): pass + @abc.abstractmethod + def _fetch_available_versions(self): + pass + @abc.abstractmethod def _read_ec2_metadata(self): pass + def _find_working_version(self, version): + search_versions = [version] + list(OS_VERSIONS) + available_versions = self._fetch_available_versions() + for potential_version in search_versions: + if not potential_version: + continue + if potential_version not in available_versions: + continue + if potential_version != version: + LOG.debug("Version '%s' not available, attempting to use" + " version '%s' instead", version, + potential_version) + return potential_version + LOG.debug("Version '%s' not available, attempting to use '%s'" + " instead", version, OS_LATEST) + return OS_LATEST + def _read_content_path(self, item): path = item.get('content_path', '').lstrip("/") path_pieces = path.split("/") @@ -166,10 +188,6 @@ class BaseReader(object): path = self._path_join(self.base_path, "openstack", *path_pieces) return self._path_read(path) - @abc.abstractmethod - def _find_working_version(self, version): - pass - def read_v2(self, version=None): """Reads a version 2 formatted location. @@ -290,6 +308,7 @@ class BaseReader(object): class ConfigDriveReader(BaseReader): def __init__(self, base_path): super(ConfigDriveReader, self).__init__(base_path) + self._versions = None def _path_join(self, base, *add_ons): components = [base] + list(add_ons) @@ -298,22 +317,21 @@ class ConfigDriveReader(BaseReader): def _path_read(self, path): return util.load_file(path) - def _find_working_version(self, version): - search_versions = [version] + list(OS_VERSIONS) - for potential_version in search_versions: - if not potential_version: - continue - path = self._path_join(self.base_path, "openstack", - potential_version) - if os.path.exists(path): - if potential_version != version: - LOG.debug("Version '%s' not available, attempting to use" - " version '%s' instead", version, - potential_version) - return potential_version - LOG.debug("Version '%s' not available, attempting to use '%s'" - " instead", version, OS_LATEST) - return OS_LATEST + def _fetch_available_versions(self): + if self._versions is not None: + return self._versions + else: + versions_available = [] + path = self._path_join(self.base_path, 'openstack') + try: + for child in os.listdir(path): + child_path = os.path.join(path, child) + if os.path.isdir(child_path): + versions_available.append(child) + except (OSError, IOError): + pass + self._versions = tuple(versions_available) + return self._versions def _read_ec2_metadata(self): path = self._path_join(self.base_path, @@ -400,40 +418,40 @@ class MetadataReader(BaseReader): self.ssl_details = ssl_details self.timeout = float(timeout) self.retries = int(retries) + self._versions = None - def _find_working_version(self, version): - search_versions = [version] + list(OS_VERSIONS) - version_path = self._path_join(self.base_path, "openstack") - versions_available = [] - try: - versions = self._path_read(version_path) - except IOError as e: - LOG.warn("Unable to read openstack versions from %s due" - " to: %s", version_path, e) + def _fetch_available_versions(self): + if self._versions is not None: + return self._versions else: - for line in versions.splitlines(): - line = line.strip() - if not line: - continue - versions_available.append(line) - for potential_version in search_versions: - if potential_version not in versions_available: - continue - if potential_version != version: - LOG.debug("Version '%s' not available, attempting to use" - " version '%s' instead", version, - potential_version) - return potential_version - LOG.debug("Version '%s' not available, searched for %s (with available" - " versions being %s), attempting to use '%s' instead", - version, search_versions, versions_available, OS_LATEST) - return OS_LATEST + path = self._path_join(self.base_path, "openstack") + versions_available = [] + try: + versions = self._path_read(path) + except IOError as e: + LOG.warn("Unable to read openstack versions from %s due" + " to: %s", path, e) + else: + for line in versions.splitlines(): + line = line.strip() + if not line: + continue + versions_available.append(line) + self._versions = tuple(versions_available) + return self._versions def _path_read(self, path): + + def should_retry(_request_args, cause): + if cause.code == httplib.NOT_FOUND: + return False + return True + response = url_helper.readurl(path, retries=self.retries, ssl_details=self.ssl_details, - timeout=self.timeout) + timeout=self.timeout, + exception_cb=should_retry) return response.contents def _path_join(self, base, *add_ons): diff --git a/tests/unittests/test_datasource/test_openstack.py b/tests/unittests/test_datasource/test_openstack.py index f43cbec8..412ae5a4 100644 --- a/tests/unittests/test_datasource/test_openstack.py +++ b/tests/unittests/test_datasource/test_openstack.py @@ -67,8 +67,8 @@ OSTACK_META = { CONTENT_0 = 'This is contents of /etc/foo.cfg\n' CONTENT_1 = '# this is /etc/bar/bar.cfg\n' OS_FILES = { - 'openstack/2012-08-10/meta_data.json': json.dumps(OSTACK_META), - 'openstack/2012-08-10/user_data': USER_DATA, + 'openstack/latest/meta_data.json': json.dumps(OSTACK_META), + 'openstack/latest/user_data': USER_DATA, 'openstack/content/0000': CONTENT_0, 'openstack/content/0001': CONTENT_1, 'openstack/latest/meta_data.json': json.dumps(OSTACK_META), @@ -78,6 +78,9 @@ OS_FILES = { EC2_FILES = { 'latest/user-data': USER_DATA, } +EC2_VERSIONS = [ + 'lastest', +] def _register_uris(version, ec2_files, ec2_meta, os_files): @@ -85,6 +88,9 @@ def _register_uris(version, ec2_files, ec2_meta, os_files): same data returned by the openstack metadata service (and ec2 service).""" def match_ec2_url(uri, headers): + path = uri.path.strip("/") + if len(path) == 0: + return (200, headers, "\n".join(EC2_VERSIONS)) path = uri.path.lstrip("/") if path in ec2_files: return (200, headers, ec2_files.get(path)) @@ -110,11 +116,20 @@ def _register_uris(version, ec2_files, ec2_meta, os_files): return (200, headers, str(value)) return (404, headers, '') - def get_request_callback(method, uri, headers): - uri = urlparse(uri) + def match_os_uri(uri, headers): + path = uri.path.strip("/") + if path == 'openstack': + return (200, headers, "\n".join([openstack.OS_LATEST])) path = uri.path.lstrip("/") if path in os_files: return (200, headers, os_files.get(path)) + return (404, headers, '') + + def get_request_callback(method, uri, headers): + uri = urlparse(uri) + path = uri.path.lstrip("/").split("/") + if path[0] == 'openstack': + return match_os_uri(uri, headers) return match_ec2_url(uri, headers) hp.register_uri(hp.GET, re.compile(r'http://169.254.169.254/.*'), -- cgit v1.2.3 From 984d36fb8c5feb82c31121ffd5d6a72b4f593499 Mon Sep 17 00:00:00 2001 From: Joshua Harlow Date: Wed, 27 Aug 2014 12:56:50 -0700 Subject: Fix retry cb to reflect what used to exist --- cloudinit/sources/helpers/openstack.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 2d0fc70e..025a2404 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -21,7 +21,6 @@ import abc import base64 import copy -import httplib import os from cloudinit import ec2_utils @@ -442,16 +441,21 @@ class MetadataReader(BaseReader): def _path_read(self, path): - def should_retry(_request_args, cause): - if cause.code == httplib.NOT_FOUND: - return False + def should_retry_cb(_request_args, cause): + try: + code = int(cause.code) + if code >= 400: + return False + except (TypeError, ValueError): + # Older versions of requests didn't have a code. + pass return True response = url_helper.readurl(path, retries=self.retries, ssl_details=self.ssl_details, timeout=self.timeout, - exception_cb=should_retry) + exception_cb=should_retry_cb) return response.contents def _path_join(self, base, *add_ons): -- cgit v1.2.3 From 419e0caab7e005827485460372c7f0d54ac0e9c9 Mon Sep 17 00:00:00 2001 From: Scott Moser Date: Wed, 27 Aug 2014 17:03:43 -0400 Subject: no functional changes, but some minor changes. --- cloudinit/sources/helpers/openstack.py | 75 ++++++++++------------- tests/unittests/test_datasource/test_openstack.py | 2 +- 2 files changed, 34 insertions(+), 43 deletions(-) diff --git a/cloudinit/sources/helpers/openstack.py b/cloudinit/sources/helpers/openstack.py index 025a2404..3c6bb6aa 100644 --- a/cloudinit/sources/helpers/openstack.py +++ b/cloudinit/sources/helpers/openstack.py @@ -162,21 +162,25 @@ class BaseReader(object): pass def _find_working_version(self, version): + try: + versions_available = self._fetch_available_versions(self) + except Exception as e: + LOG.warn("Unable to read openstack versions from %s due to: %s", + self.base_path, e) + versions_available = [] + search_versions = [version] + list(OS_VERSIONS) - available_versions = self._fetch_available_versions() + selected_version = OS_LATEST for potential_version in search_versions: - if not potential_version: + if potential_version not in versions_available: continue - if potential_version not in available_versions: - continue - if potential_version != version: - LOG.debug("Version '%s' not available, attempting to use" - " version '%s' instead", version, - potential_version) - return potential_version - LOG.debug("Version '%s' not available, attempting to use '%s'" - " instead", version, OS_LATEST) - return OS_LATEST + selected_version = potential_version + break + + if selected_version != version: + LOG.warn("Version '%s' not available, attempting to use" + " version '%s' instead", version, selected_version) + return selected_version def _read_content_path(self, item): path = item.get('content_path', '').lstrip("/") @@ -317,20 +321,12 @@ class ConfigDriveReader(BaseReader): return util.load_file(path) def _fetch_available_versions(self): - if self._versions is not None: - return self._versions - else: - versions_available = [] + if self._versions is None: path = self._path_join(self.base_path, 'openstack') - try: - for child in os.listdir(path): - child_path = os.path.join(path, child) - if os.path.isdir(child_path): - versions_available.append(child) - except (OSError, IOError): - pass - self._versions = tuple(versions_available) - return self._versions + found = [d for d in os.listdir(path) + if os.path.isdir(os.path.join(path))] + self._versions = tuple(found) + return self._versions def _read_ec2_metadata(self): path = self._path_join(self.base_path, @@ -420,24 +416,19 @@ class MetadataReader(BaseReader): self._versions = None def _fetch_available_versions(self): + # /openstack/ returns a newline separated list of versions if self._versions is not None: - return self._versions - else: - path = self._path_join(self.base_path, "openstack") - versions_available = [] - try: - versions = self._path_read(path) - except IOError as e: - LOG.warn("Unable to read openstack versions from %s due" - " to: %s", path, e) - else: - for line in versions.splitlines(): - line = line.strip() - if not line: - continue - versions_available.append(line) - self._versions = tuple(versions_available) - return self._versions + return self.os_versions + found = [] + content = self._path_read(version_path) + for line in content.splitlines(): + line = line.strip() + if not line: + continue + found.append(line) + self._versions = tuple(found) + return self._versions + def _path_read(self, path): diff --git a/tests/unittests/test_datasource/test_openstack.py b/tests/unittests/test_datasource/test_openstack.py index 412ae5a4..530fba20 100644 --- a/tests/unittests/test_datasource/test_openstack.py +++ b/tests/unittests/test_datasource/test_openstack.py @@ -79,7 +79,7 @@ EC2_FILES = { 'latest/user-data': USER_DATA, } EC2_VERSIONS = [ - 'lastest', + 'latest', ] -- cgit v1.2.3