From be4b8f1416fdc4195e9f3e6688939c0bd923de5a Mon Sep 17 00:00:00 2001 From: Clark Boylan Date: Sun, 7 Jan 2018 12:44:15 -0800 Subject: [PATCH] Clarify provider manager vs provider config There are places where we handle both the provider config and provider manager objects. We had a bug where we called a manager method on the config object which failed. Since python doesn't type check ahead of time let us try and make this easier on developers and very explicitly name the provider manager provider_manager and the provider config provider_config. Change-Id: I36919672cdbf9a5c66297ff49f304b9040eee1cb --- nodepool/driver/__init__.py | 2 +- nodepool/driver/openstack/handler.py | 40 +++++++++++++----------- nodepool/launcher.py | 12 +++---- nodepool/tests/test_nodelaunchmanager.py | 3 +- 4 files changed, 31 insertions(+), 26 deletions(-) diff --git a/nodepool/driver/__init__.py b/nodepool/driver/__init__.py index 83d940a83..256645e06 100644 --- a/nodepool/driver/__init__.py +++ b/nodepool/driver/__init__.py @@ -258,7 +258,7 @@ class NodeLaunchManager(object): self._threads = [] self._zk = zk self._pool = pool - self._manager = provider_manager + self._provider_manager = provider_manager self._requestor = requestor @property diff --git a/nodepool/driver/openstack/handler.py b/nodepool/driver/openstack/handler.py index 143941244..4bfc7839f 100644 --- a/nodepool/driver/openstack/handler.py +++ b/nodepool/driver/openstack/handler.py @@ -52,23 +52,24 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): self.log = logging.getLogger("nodepool.NodeLauncher-%s" % node.id) self._zk = zk self._label = provider_label - self._manager = provider_manager + self._provider_manager = provider_manager self._node = node self._retries = retries self._image_name = None self._requestor = requestor self._pool = self._label.pool - self._provider = self._pool.provider + self._provider_config = self._pool.provider if self._label.diskimage: - self._diskimage = self._provider.diskimages[self._label.diskimage.name] + self._diskimage = \ + self._provider_config.diskimages[self._label.diskimage.name] else: self._diskimage = None def logConsole(self, server_id, hostname): if not self._label.console_log: return - console = self._manager.getServerConsole(server_id) + console = self._provider_manager.getServerConsole(server_id) if console: self.log.debug('Console log from hostname %s:' % hostname) for line in console.splitlines(): @@ -78,12 +79,12 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): if self._label.diskimage: # launch using diskimage cloud_image = self._zk.getMostRecentImageUpload( - self._diskimage.name, self._provider.name) + self._diskimage.name, self._provider_config.name) if not cloud_image: raise exceptions.LaunchNodepoolException( "Unable to find current cloud image %s in %s" % - (self._diskimage.name, self._provider.name) + (self._diskimage.name, self._provider_config.name) ) config_drive = self._diskimage.config_drive @@ -107,12 +108,13 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): username = self._label.cloud_image.username connection_type = self._label.cloud_image.connection_type - hostname = self._provider.hostname_format.format( - label=self._label, provider=self._provider, node=self._node + hostname = self._provider_config.hostname_format.format( + label=self._label, provider=self._provider_config, node=self._node ) self.log.info("Creating server with hostname %s in %s from image %s " - "for node id: %s" % (hostname, self._provider.name, + "for node id: %s" % (hostname, + self._provider_config.name, image_name, self._node.id)) @@ -121,7 +123,7 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): # because that isn't available in ZooKeeper until after the server is # active, which could cause a race in leak detection. - server = self._manager.createServer( + server = self._provider_manager.createServer( hostname, image=image_external, min_ram=self._label.min_ram, @@ -148,8 +150,8 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): self.log.debug("Waiting for server %s for node id: %s" % (server.id, self._node.id)) - server = self._manager.waitForServer( - server, self._provider.launch_timeout, + server = self._provider_manager.waitForServer( + server, self._provider_config.launch_timeout, auto_ip=self._pool.auto_floating_ip) if server.status != 'ACTIVE': @@ -196,7 +198,7 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): try: self.log.debug("Gathering host keys for node %s", self._node.id) host_keys = utils.keyscan( - interface_ip, timeout=self._provider.boot_timeout) + interface_ip, timeout=self._provider_config.boot_timeout) if not host_keys: raise exceptions.LaunchKeyscanException( "Unable to gather host keys") @@ -220,8 +222,10 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): attempts, self._retries, self._node.id) # If we created an instance, delete it. if self._node.external_id: - self._manager.cleanupNode(self._node.external_id) - self._manager.waitForNodeCleanup(self._node.external_id) + self._provider_manager.cleanupNode(self._node.external_id) + self._provider_manager.waitForNodeCleanup( + self._node.external_id + ) self._node.external_id = None self._node.public_ipv4 = None self._node.public_ipv6 = None @@ -232,7 +236,7 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): # Invalidate the quota cache if we encountered a quota error. if 'quota exceeded' in str(e).lower(): self.log.info("Quota exceeded, invalidating quota cache") - self._manager.invalidateQuotaCache() + self._provider_manager.invalidateQuotaCache() attempts += 1 self._node.state = zk.READY @@ -261,7 +265,7 @@ class NodeLauncher(threading.Thread, stats.StatsReporter): self.recordLaunchStats(statsd_key, dt, self._image_name, self._node.provider, self._node.az, self._requestor) - self.updateNodeStats(self._zk, self._provider) + self.updateNodeStats(self._zk, self._provider_config) except Exception: self.log.exception("Exception while reporting stats:") @@ -279,7 +283,7 @@ class OpenStackNodeLaunchManager(NodeLaunchManager): ''' self._nodes.append(node) provider_label = self._pool.labels[node.type] - t = NodeLauncher(self._zk, provider_label, self._manager, + t = NodeLauncher(self._zk, provider_label, self._provider_manager, self._requestor, node, self._retries) t.start() self._threads.append(t) diff --git a/nodepool/launcher.py b/nodepool/launcher.py index aa30507aa..58a1a8257 100755 --- a/nodepool/launcher.py +++ b/nodepool/launcher.py @@ -44,12 +44,12 @@ SUSPEND_WAIT_TIME = 30 # How long to wait between checks for ZooKeeper class NodeDeleter(threading.Thread, stats.StatsReporter): log = logging.getLogger("nodepool.NodeDeleter") - def __init__(self, zk, manager, node): + def __init__(self, zk, provider_manager, node): threading.Thread.__init__(self, name='NodeDeleter for %s %s' % (node.provider, node.external_id)) stats.StatsReporter.__init__(self) self._zk = zk - self._manager = manager + self._provider_manager = provider_manager self._node = node @staticmethod @@ -60,8 +60,8 @@ class NodeDeleter(threading.Thread, stats.StatsReporter): This is a class method so we can support instantaneous deletes. :param ZooKeeper zk_conn: A ZooKeeper object to use. - :param ProviderManager manager: ProviderManager object to use for - deleting the server. + :param ProviderManager provider_manager: ProviderManager object to + use fo deleting the server. :param Node node: A locked Node object that describes the server to delete. :param bool node_exists: True if the node actually exists in ZooKeeper. @@ -101,10 +101,10 @@ class NodeDeleter(threading.Thread, stats.StatsReporter): else: node_exists = True - self.delete(self._zk, self._manager, self._node, node_exists) + self.delete(self._zk, self._provider_manager, self._node, node_exists) try: - self.updateNodeStats(self._zk, self._manager.provider) + self.updateNodeStats(self._zk, self._provider_manager.provider) except Exception: self.log.exception("Exception while reporting stats:") diff --git a/nodepool/tests/test_nodelaunchmanager.py b/nodepool/tests/test_nodelaunchmanager.py index 7ba659319..fd4b3b8f4 100644 --- a/nodepool/tests/test_nodelaunchmanager.py +++ b/nodepool/tests/test_nodelaunchmanager.py @@ -61,7 +61,8 @@ class TestNodeLaunchManager(tests.DBTestCase): time.sleep(0) self.assertEqual(len(mgr.ready_nodes), 1) self.assertEqual(len(mgr.failed_nodes), 0) - self.assertEqual(mgr._manager.listNodes()[0]['metadata']['groups'], + nodes = mgr._provider_manager.listNodes() + self.assertEqual(nodes[0]['metadata']['groups'], 'fake-provider,fake-image,fake-label') @mock.patch('nodepool.driver.openstack.handler.NodeLauncher._launchNode')