From 82d8c512503121bf6c59cead1c7b7ece33a65054 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Thu, 31 May 2018 14:53:39 -0700 Subject: [PATCH] Create a base Driver class This is, like the drivers in zuul, designed to be a single instance per driver that survives for the life of the process. It is used to further instantiate driver-specific interfaces. Here we have it return the config object for the driver (replacing the previous system which loaded it from specific config files). We also move the reset method from the ProviderConfig to the Driver object. It's currently only used to clear a global os client config object, so this better matches its lifecycle. Change-Id: I1f5a7be9c597be842bfe4dbea8f153d7a96d7b9a --- doc/source/devguide.rst | 2 + nodepool/config.py | 8 +-- nodepool/driver/__init__.py | 73 +++++++++++++++++------ nodepool/driver/fake/__init__.py | 30 ++++++++++ nodepool/driver/openstack/__init__.py | 30 ++++++++++ nodepool/driver/openstack/config.py | 18 ++---- nodepool/driver/static/__init__.py | 21 +++++++ nodepool/driver/static/config.py | 4 -- nodepool/driver/test/__init__.py | 21 +++++++ nodepool/driver/test/config.py | 4 -- nodepool/tests/test_config_comparisons.py | 12 ++-- nodepool/tests/test_drivers.py | 6 -- 12 files changed, 175 insertions(+), 54 deletions(-) diff --git a/doc/source/devguide.rst b/doc/source/devguide.rst index bf6b01ac0..7ff70f4f5 100644 --- a/doc/source/devguide.rst +++ b/doc/source/devguide.rst @@ -54,6 +54,8 @@ its most important pieces:: Drivers ------- +.. autoclass:: nodepool.driver.Driver + :members: .. autoclass:: nodepool.driver.Provider :members: .. autoclass:: nodepool.driver.NodeRequestHandler diff --git a/nodepool/config.py b/nodepool/config.py index 3bcbad88b..fd2241816 100755 --- a/nodepool/config.py +++ b/nodepool/config.py @@ -204,8 +204,8 @@ def get_provider_config(provider): # Ensure legacy configuration still works when using fake cloud if provider.get('name', '').startswith('fake'): provider['driver'] = 'fake' - driver = Drivers.get(provider['driver']) - return driver['config'](provider) + driver = Drivers._get(provider['driver']) + return driver.getProviderConfig(provider) def openConfig(path): @@ -234,8 +234,8 @@ def loadConfig(config_path): config = openConfig(config_path) # Call driver config reset now to clean global hooks like os_client_config - for driver in Drivers.drivers.values(): - driver["config"].reset() + for driver in Drivers._drivers.values(): + driver.reset() newconfig = Config() diff --git a/nodepool/driver/__init__.py b/nodepool/driver/__init__.py index 71a1f333b..436104a26 100644 --- a/nodepool/driver/__init__.py +++ b/nodepool/driver/__init__.py @@ -32,6 +32,7 @@ class Drivers: log = logging.getLogger("nodepool.driver.Drivers") drivers = {} + _drivers = {} # TODO: replace drivers drivers_paths = None @staticmethod @@ -50,7 +51,7 @@ class Drivers: if not obj: error = "no %s implementation found" % parent_class if error: - Drivers.log.error("%s: %s" % (path, error)) + Drivers.log.error("%s: %s", path, error) return False return obj[0][1] @@ -66,15 +67,14 @@ class Drivers: for driver in drivers: driver_path = os.path.join(drivers_path, driver) if driver in Drivers.drivers: - Drivers.log.warning("%s: duplicate driver" % driver_path) + Drivers.log.warning("%s: duplicate driver", driver_path) continue if not os.path.isdir(driver_path) or \ "__init__.py" not in os.listdir(driver_path): continue - Drivers.log.debug("%s: loading driver" % driver_path) + Drivers.log.debug("%s: loading driver", driver_path) driver_obj = {} for name, parent_class in ( - ("config", ProviderConfig), ("provider", Provider), ): driver_obj[name] = Drivers._load_class( @@ -83,10 +83,21 @@ class Drivers: if not driver_obj[name]: break if not driver_obj[name]: - Drivers.log.error("%s: skipping incorrect driver" % - driver_path) + Drivers.log.error( + "%s: skipping incorrect driver from %s.py", + driver_path, name) continue Drivers.drivers[driver] = driver_obj + driver_obj = Drivers._load_class( + driver, os.path.join(driver_path, "__init__.py"), + Driver) + if not driver_obj: + Drivers.log.error( + "%s: skipping incorrect driver from __init__.py", + driver_path) + continue + Drivers._drivers[driver] = driver_obj() + Drivers.drivers_paths = drivers_paths @staticmethod @@ -98,6 +109,42 @@ class Drivers: except KeyError: raise RuntimeError("%s: unknown driver" % name) + # TODO: replace get + @staticmethod + def _get(name): + if not Drivers._drivers: + Drivers.load() + try: + return Drivers._drivers[name] + except KeyError: + raise RuntimeError("%s: unknown driver" % name) + + +class Driver(object, metaclass=abc.ABCMeta): + """The Driver interface + + This is the main entrypoint for a Driver. A single instance of + this will be created for each driver in the system and will + persist for the lifetime of the process. + + The class or instance attribute **name** must be provided as a string. + + """ + + def reset(self): + ''' + Called before loading configuration to reset any global state + ''' + pass + + @abc.abstractmethod + def getProviderConfig(self, provider): + """Return a ProviderConfig instance + + :arg dict provider: The parsed provider configuration + """ + pass + class Provider(object, metaclass=abc.ABCMeta): """The Provider interface @@ -723,12 +770,12 @@ class ConfigPool(ConfigValue): return False -class Driver(ConfigValue): +class DriverConfig(ConfigValue): def __init__(self): self.name = None def __eq__(self, other): - if isinstance(other, Driver): + if isinstance(other, DriverConfig): return self.name == other.name return False @@ -742,7 +789,7 @@ class ProviderConfig(ConfigValue, metaclass=abc.ABCMeta): def __init__(self, provider): self.name = provider['name'] self.provider = provider - self.driver = Driver() + self.driver = DriverConfig() self.driver.name = provider.get('driver', 'openstack') self.max_concurrency = provider.get('max-concurrency', -1) @@ -773,14 +820,6 @@ class ProviderConfig(ConfigValue, metaclass=abc.ABCMeta): ''' pass - # TODO: can we remove this? - @abc.abstractmethod - def reset(): - ''' - Called before loading configuration to reset any global state - ''' - pass - @abc.abstractmethod def load(self, newconfig): ''' diff --git a/nodepool/driver/fake/__init__.py b/nodepool/driver/fake/__init__.py index e69de29bb..f7cb7a4d4 100644 --- a/nodepool/driver/fake/__init__.py +++ b/nodepool/driver/fake/__init__.py @@ -0,0 +1,30 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# +# See the License for the specific language governing permissions and +# limitations under the License. + +import os_client_config + +from nodepool.driver import Driver +from nodepool.driver.fake.config import FakeProviderConfig + + +class FakeDriver(Driver): + def __init__(self): + super().__init__() + self.reset() + + def reset(self): + self.os_client_config = os_client_config.OpenStackConfig() + + def getProviderConfig(self, provider): + return FakeProviderConfig(self, provider) diff --git a/nodepool/driver/openstack/__init__.py b/nodepool/driver/openstack/__init__.py index e69de29bb..c7648edf9 100644 --- a/nodepool/driver/openstack/__init__.py +++ b/nodepool/driver/openstack/__init__.py @@ -0,0 +1,30 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# +# See the License for the specific language governing permissions and +# limitations under the License. + +import os_client_config + +from nodepool.driver import Driver +from nodepool.driver.openstack.config import OpenStackProviderConfig + + +class OpenStackDriver(Driver): + def __init__(self): + super().__init__() + self.reset() + + def reset(self): + self.os_client_config = os_client_config.OpenStackConfig() + + def getProviderConfig(self, provider): + return OpenStackProviderConfig(self, provider) diff --git a/nodepool/driver/openstack/config.py b/nodepool/driver/openstack/config.py index 3c7f811bc..bf6c9f6c6 100644 --- a/nodepool/driver/openstack/config.py +++ b/nodepool/driver/openstack/config.py @@ -15,7 +15,6 @@ # limitations under the License. import math -import os_client_config import voluptuous as v from nodepool.driver import ProviderConfig @@ -155,9 +154,8 @@ class ProviderPool(ConfigPool): class OpenStackProviderConfig(ProviderConfig): - os_client_config = None - - def __init__(self, *args, **kwargs): + def __init__(self, driver, provider): + self.driver_object = driver self.__pools = {} self.cloud_config = None self.image_type = None @@ -169,7 +167,7 @@ class OpenStackProviderConfig(ProviderConfig): self.cloud_images = {} self.hostname_format = None self.image_name_format = None - super().__init__(*args, **kwargs) + super().__init__(provider) def __eq__(self, other): if isinstance(other, OpenStackProviderConfig): @@ -200,16 +198,10 @@ class OpenStackProviderConfig(ProviderConfig): def manage_images(self): return True - @staticmethod - def reset(): - OpenStackProviderConfig.os_client_config = None - def load(self, config): - if OpenStackProviderConfig.os_client_config is None: - OpenStackProviderConfig.os_client_config = \ - os_client_config.OpenStackConfig() cloud_kwargs = self._cloudKwargs() - self.cloud_config = self.os_client_config.get_one_cloud(**cloud_kwargs) + occ = self.driver_object.os_client_config + self.cloud_config = occ.get_one_cloud(**cloud_kwargs) self.image_type = self.cloud_config.config['image_format'] self.region_name = self.provider.get('region-name') diff --git a/nodepool/driver/static/__init__.py b/nodepool/driver/static/__init__.py index e69de29bb..c29c0b85d 100644 --- a/nodepool/driver/static/__init__.py +++ b/nodepool/driver/static/__init__.py @@ -0,0 +1,21 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# +# See the License for the specific language governing permissions and +# limitations under the License. + +from nodepool.driver import Driver +from nodepool.driver.static.config import StaticProviderConfig + + +class StaticDriver(Driver): + def getProviderConfig(self, provider): + return StaticProviderConfig(provider) diff --git a/nodepool/driver/static/config.py b/nodepool/driver/static/config.py index 988a02ca4..e660391ab 100644 --- a/nodepool/driver/static/config.py +++ b/nodepool/driver/static/config.py @@ -54,10 +54,6 @@ class StaticProviderConfig(ProviderConfig): other.pools == self.pools) return False - @staticmethod - def reset(): - pass - @property def pools(self): return self.__pools diff --git a/nodepool/driver/test/__init__.py b/nodepool/driver/test/__init__.py index e69de29bb..25a4bf1ec 100644 --- a/nodepool/driver/test/__init__.py +++ b/nodepool/driver/test/__init__.py @@ -0,0 +1,21 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or +# implied. +# +# See the License for the specific language governing permissions and +# limitations under the License. + +from nodepool.driver import Driver +from nodepool.driver.test.config import TestConfig + + +class TestDriver(Driver): + def getProviderConfig(self, provider): + return TestConfig(provider) diff --git a/nodepool/driver/test/config.py b/nodepool/driver/test/config.py index f9651a494..80f8a662f 100644 --- a/nodepool/driver/test/config.py +++ b/nodepool/driver/test/config.py @@ -31,10 +31,6 @@ class TestConfig(ProviderConfig): def __eq__(self, other): return self.name == other.name - @staticmethod - def reset(): - pass - @property def pools(self): return self.__pools diff --git a/nodepool/tests/test_config_comparisons.py b/nodepool/tests/test_config_comparisons.py index db55be5dc..d7724e818 100644 --- a/nodepool/tests/test_config_comparisons.py +++ b/nodepool/tests/test_config_comparisons.py @@ -18,7 +18,7 @@ from nodepool.config import Config from nodepool.config import DiskImage from nodepool.config import Label from nodepool.driver import ConfigPool -from nodepool.driver import Driver +from nodepool.driver import DriverConfig from nodepool.driver.openstack.config import OpenStackProviderConfig from nodepool.driver.openstack.config import ProviderDiskImage from nodepool.driver.openstack.config import ProviderCloudImage @@ -37,9 +37,9 @@ class TestConfigComparisons(tests.BaseTestCase): a.max_servers = 5 self.assertNotEqual(a, b) - def test_Driver(self): - a = Driver() - b = Driver() + def test_DriverConfig(self): + a = DriverConfig() + b = DriverConfig() self.assertEqual(a, b) a.name = "foo" self.assertNotEqual(a, b) @@ -100,8 +100,8 @@ class TestConfigComparisons(tests.BaseTestCase): def test_OpenStackProviderConfig(self): provider = {'name': 'foo'} - a = OpenStackProviderConfig(provider) - b = OpenStackProviderConfig(provider) + a = OpenStackProviderConfig(None, provider) + b = OpenStackProviderConfig(None, provider) self.assertEqual(a, b) # intentionally change an attribute of the base class a.name = 'bar' diff --git a/nodepool/tests/test_drivers.py b/nodepool/tests/test_drivers.py index 65e373a92..5f575c578 100644 --- a/nodepool/tests/test_drivers.py +++ b/nodepool/tests/test_drivers.py @@ -14,7 +14,6 @@ import os -from nodepool import config as nodepool_config from nodepool import tests from nodepool.driver import Drivers @@ -26,11 +25,6 @@ class TestDrivers(tests.DBTestCase): Drivers.load([drivers_dir]) return super().setup_config(filename) - def test_external_driver_config(self): - configfile = self.setup_config('external_driver.yaml') - nodepool_config.loadConfig(configfile) - self.assertIn("config", Drivers.get("test")) - def test_external_driver_handler(self): configfile = self.setup_config('external_driver.yaml') pool = self.useNodepool(configfile, watermark_sleep=1)