zuul
/
nodepool
Code Issues Proposed changes

Azure: reconcile config objects 

The config objects in the Azure driver have drifted a bit.  This
updates them to match the actual used configuration.  It also
reorganizes them to be a little easier to maintain by moving the
initializers into the individual objects.

Finally, the verbose __eq__ methods are removed in favor of a
simpler __eq__ method in the superclass.

Since the OpenStack, k8s, and OpenShift drivers calls super() in
__eq__ methods, they need to be updated at the same time.

This also corrects an unrelated error with a misnamed parameter
in the fake k8s used in the k8s tests.

Change-Id: Id6971ca002879d3fb056fedc7e4ca6ec35dd7434
This commit is contained in:
James E. Blair 2021-03-10 20:10:37 -08:00
parent 14c5cb503e
commit 94fcc70a59
7 changed files with 126 additions and 259 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
nodepool/driver/__init__.py
View File

@ -821,13 +821,27 @@ class NodeRequestHandler(NodeRequestHandlerNotifications,
class ConfigValue(object, metaclass=abc.ABCMeta):
@abc.abstractmethod
ignore_equality = []
def __eq__(self, other):
pass
if isinstance(other, self.__class__):
for k in set(list(self.__dict__.keys()) +
list(other.__dict__.keys())):
if k in self.ignore_equality:
continue
if self.__dict__.get(k) != other.__dict__.get(k):
return False
return True
return False
def __ne__(self, other):
return not self.__eq__(other)
def __repr__(self):
if hasattr(self, 'name'):
return "<%s %s>" % (self.__class__.__name__, self.name)
return "<%s>" % (self.__class__.__name__,)
class ConfigPool(ConfigValue, metaclass=abc.ABCMeta):
'''
@ -839,13 +853,6 @@ class ConfigPool(ConfigValue, metaclass=abc.ABCMeta):
self.max_servers = math.inf
self.node_attributes = None
def __eq__(self, other):
if isinstance(other, ConfigPool):
return (self.labels == other.labels and
self.max_servers == other.max_servers and
self.node_attributes == other.node_attributes)
return False
@classmethod
def getCommonSchemaDict(self):
'''
@ -885,11 +892,6 @@ class DriverConfig(ConfigValue):
def __init__(self):
self.name = None
def __eq__(self, other):
if isinstance(other, DriverConfig):
return self.name == other.name
return False
class ProviderConfig(ConfigValue, metaclass=abc.ABCMeta):
"""The Provider config interface
@ -904,17 +906,6 @@ class ProviderConfig(ConfigValue, metaclass=abc.ABCMeta):
self.driver.name = provider.get('driver', 'openstack')
self.max_concurrency = provider.get('max-concurrency', -1)
def __eq__(self, other):
if isinstance(other, ProviderConfig):
return (self.name == other.name and
self.provider == other.provider and
self.driver == other.driver and
self.max_concurrency == other.max_concurrency)
return False
def __repr__(self):
return "<Provider %s>" % self.name
@classmethod
def getCommonSchemaDict(self):
return {

2
nodepool/driver/azurestate/adapter.py
View File

@ -115,7 +115,7 @@ class AzureCreateStateMachine(statemachine.StateMachine):
self.adapter = adapter
self.retries = retries
self.metadata = metadata
self.tags = label.tags or {}
self.tags = label.tags.copy() or {}
self.tags.update(metadata)
self.hostname = hostname
self.label = label

222
nodepool/driver/azurestate/config.py
View File

@ -23,71 +23,119 @@ from nodepool.driver import ConfigValue
from nodepool.driver import ProviderConfig
class ProviderCloudImage(ConfigValue):
def __init__(self):
self.name = None
self.image_id = None
self.username = None
self.key = None
self.python_path = None
self.connection_type = None
self.connection_port = None
def __eq__(self, other):
if isinstance(other, ProviderCloudImage):
return (self.name == other.name
and self.image_id == other.image_id
and self.username == other.username
and self.key == other.key
and self.python_path == other.python_path
and self.connection_type == other.connection_type
and self.connection_port == other.connection_port)
return False
def __repr__(self):
return "<ProviderCloudImage %s>" % self.name
class AzureProviderCloudImage(ConfigValue):
def __init__(self, image, zuul_public_key):
default_port_mapping = {
'ssh': 22,
'winrm': 5986,
}
self.name = image['name']
self.username = image['username']
# TODO(corvus): remove zuul_public_key
self.key = image.get('key', zuul_public_key)
self.image_reference = image['image-reference']
self.python_path = image.get('python-path')
self.connection_type = image.get('connection-type', 'ssh')
self.connection_port = image.get(
'connection-port',
default_port_mapping.get(self.connection_type, 22))
@property
def external_name(self):
'''Human readable version of external.'''
return self.image_id or self.name
@staticmethod
def getSchema():
azure_image_reference = {
v.Required('sku'): str,
v.Required('publisher'): str,
v.Required('version'): str,
v.Required('offer'): str,
}
return {
v.Required('name'): str,
v.Required('username'): str,
# TODO(corvus): make required when zuul_public_key removed
'key': str,
v.Required('image-reference'): azure_image_reference,
'connection-type': str,
'connection-port': int,
'python-path': str,
# TODO(corvus): shell-type
}
class AzureLabel(ConfigValue):
def __eq__(self, other):
if ( # other.username != self.username or
# other.imageReference != self.imageReference or
other.hardware_profile != self.hardware_profile):
return False
return True
ignore_equality = ['pool']
def __init__(self, label, provider_config, provider_pool):
self.hardware_profile = None
self.name = label['name']
self.pool = provider_pool
cloud_image_name = label['cloud-image']
cloud_image = provider_config.cloud_images.get(
cloud_image_name, None)
if not cloud_image:
raise ValueError(
"cloud-image %s does not exist in provider %s"
" but is referenced in label %s" %
(cloud_image_name, provider_config.name, self.name))
self.cloud_image = cloud_image
self.hardware_profile = label['hardware-profile']
self.tags = label.get('tags', {})
@staticmethod
def getSchema():
azure_hardware_profile = {
v.Required('vm-size'): str,
}
return {
v.Required('name'): str,
v.Required('cloud-image'): str,
v.Required('hardware-profile'): azure_hardware_profile,
'tags': dict,
}
class AzurePool(ConfigPool):
def __eq__(self, other):
if other.labels != self.labels:
return False
return True
ignore_equality = ['provider']
def __repr__(self):
return "<AzurePool %s>" % self.name
def __init__(self, provider_config, pool_config):
super().__init__()
self.provider = provider_config
self.load(pool_config)
def load(self, pool_config):
pass
self.name = pool_config['name']
self.max_servers = pool_config['max-servers']
self.use_internal_ip = bool(pool_config.get('use-internal-ip', False))
self.host_key_checking = bool(pool_config.get(
'host-key-checking', True))
@staticmethod
def getSchema():
azure_label = AzureLabel.getSchema()
pool = ConfigPool.getCommonSchemaDict()
pool.update({
v.Required('name'): str,
v.Required('labels'): [azure_label],
})
return pool
class AzureProviderConfig(ProviderConfig):
def __init__(self, driver, provider):
super().__init__(provider)
self._pools = {}
self.driver_object = driver
self.rate_limit = None
self.launch_retries = None
super().__init__(provider)
def __eq__(self, other):
if (other.location != self.location or
other.pools != self.pools):
return False
return True
@property
def pools(self):
@ -106,6 +154,7 @@ class AzureProviderConfig(ProviderConfig):
self.launch_retries = self.provider.get('launch-retries', 3)
self.boot_timeout = self.provider.get('boot-timeout', 60)
# TODO(corvus): remove
self.zuul_public_key = self.provider['zuul-public-key']
self.location = self.provider['location']
self.subnet_id = self.provider['subnet-id']
@ -115,105 +164,34 @@ class AzureProviderConfig(ProviderConfig):
self.auth_path = self.provider.get(
'auth-path', os.getenv('AZURE_AUTH_LOCATION', None))
default_port_mapping = {
'ssh': 22,
'winrm': 5986,
}
self.cloud_images = {}
for image in self.provider['cloud-images']:
i = ProviderCloudImage()
i.name = image['name']
i.username = image['username']
# i.key = image['key']
i.key = self.zuul_public_key
i.image_reference = image['image-reference']
i.connection_type = image.get('connection-type', 'ssh')
i.connection_port = image.get(
'connection-port',
default_port_mapping.get(i.connection_type, 22))
i = AzureProviderCloudImage(image, self.zuul_public_key)
self.cloud_images[i.name] = i
for pool in self.provider.get('pools', []):
pp = AzurePool()
pp.name = pool['name']
pp.provider = self
pp.max_servers = pool['max-servers']
pp.use_internal_ip = bool(pool.get('use-internal-ip', False))
pp.host_key_checking = bool(pool.get(
'host-key-checking', True))
pp = AzurePool(self, pool)
self._pools[pp.name] = pp
pp.labels = {}
for label in pool.get('labels', []):
pl = AzureLabel()
pl.name = label['name']
pl.pool = pp
pl = AzureLabel(label, self, pp)
pp.labels[pl.name] = pl
cloud_image_name = label['cloud-image']
if cloud_image_name:
cloud_image = self.cloud_images.get(
cloud_image_name, None)
if not cloud_image:
raise ValueError(
"cloud-image %s does not exist in provider %s"
" but is referenced in label %s" %
(cloud_image_name, self.name, pl.name))
# pl.imageReference = cloud_image['image-reference']
pl.cloud_image = cloud_image
# pl.username = cloud_image.get('username', 'zuul')
else:
# pl.imageReference = None
pl.cloud_image = None
# pl.username = 'zuul'
pl.hardware_profile = label['hardware-profile']
config.labels[label['name']].pools.append(pp)
pl.tags = label.get('tags', {})
config.labels[pl.name].pools.append(pp)
def getSchema(self):
provider_cloud_images = AzureProviderCloudImage.getSchema()
azure_image_reference = {
v.Required('sku'): str,
v.Required('publisher'): str,
v.Required('version'): str,
v.Required('offer'): str,
}
azure_hardware_profile = {
v.Required('vm-size'): str,
}
provider_cloud_images = {
v.Required('name'): str,
'username': str,
v.Required('image-reference'): azure_image_reference,
}
azure_label = {
v.Required('name'): str,
v.Required('hardware-profile'): azure_hardware_profile,
v.Required('cloud-image'): str,
v.Optional('tags'): dict,
}
pool = ConfigPool.getCommonSchemaDict()
pool.update({
v.Required('name'): str,
v.Required('labels'): [azure_label],
})
pool = AzurePool.getSchema()
provider = ProviderConfig.getCommonSchemaDict()
provider.update({
v.Required('zuul-public-key'): str,
v.Required('pools'): [pool],
v.Required('location'): str,
v.Required('resource-group'): str,
v.Required('resource-group-location'): str,
v.Required('subnet-id'): str,
v.Required('cloud-images'): [provider_cloud_images],
v.Optional('auth-path'): str,
v.Required('auth-path'): str,
})
return v.Schema(provider)

28
nodepool/driver/kubernetes/config.py
View File

@ -22,31 +22,14 @@ from nodepool.driver import ProviderConfig
class KubernetesLabel(ConfigValue):
def __eq__(self, other):
if isinstance(other, KubernetesLabel):
return (other.name == self.name and
other.type == self.type and
other.image_pull == self.image_pull and
other.python_path == self.python_path and
other.shell_type == self.shell_type and
other.image == self.image and
other.cpu == self.cpu and
other.memory == self.memory and
other.env == self.env and
other.node_selector == self.node_selector)
return False
ignore_equality = ['pool']
def __repr__(self):
return "<KubternetesLabel %s>" % self.name
class KubernetesPool(ConfigPool):
def __eq__(self, other):
if isinstance(other, KubernetesPool):
return (super().__eq__(other) and
other.name == self.name and
other.labels == self.labels)
return False
ignore_equality = ['provider']
def __repr__(self):
return "<KubernetesPool %s>" % self.name
@ -78,13 +61,6 @@ class KubernetesProviderConfig(ProviderConfig):
self.__pools = {}
super().__init__(provider)
def __eq__(self, other):
if isinstance(other, KubernetesProviderConfig):
return (super().__eq__(other) and
other.context == self.context and
other.pools == self.pools)
return False
@property
def pools(self):
return self.__pools

28
nodepool/driver/openshift/config.py
View File

@ -23,31 +23,14 @@ from nodepool.driver import ProviderConfig
class OpenshiftLabel(ConfigValue):
def __eq__(self, other):
if isinstance(other, OpenshiftLabel):
return (other.name == self.name and
other.type == self.type and
other.image_pull == self.image_pull and
other.image == self.image and
other.cpu == self.cpu and
other.memory == self.memory and
other.python_path == self.python_path and
other.shell_type == self.shell_type and
other.env == self.env and
other.node_selector == self.node_selector)
return False
ignore_equality = ['pool']
def __repr__(self):
return "<OpenshiftLabel %s>" % self.name
class OpenshiftPool(ConfigPool):
def __eq__(self, other):
if isinstance(other, OpenshiftPool):
return (super().__eq__(other) and
other.name == self.name and
other.labels == self.labels)
return False
ignore_equality = ['provider']
def __repr__(self):
return "<OpenshiftPool %s>" % self.name
@ -79,13 +62,6 @@ class OpenshiftProviderConfig(ProviderConfig):
self.__pools = {}
super().__init__(provider)
def __eq__(self, other):
if isinstance(other, OpenshiftProviderConfig):
return (super().__eq__(other) and
other.context == self.context and
other.pools == self.pools)
return False
@property
def pools(self):
return self.__pools

62
nodepool/driver/openstack/config.py
View File

@ -31,16 +31,6 @@ class ProviderDiskImage(ConfigValue):
self.connection_port = None
self.meta = None
def __eq__(self, other):
if isinstance(other, ProviderDiskImage):
return (self.name == other.name and
self.pause == other.pause and
self.config_drive == other.config_drive and
self.connection_type == other.connection_type and
self.connection_port == other.connection_port and
self.meta == other.meta)
return False
def __repr__(self):
return "<ProviderDiskImage %s>" % self.name
@ -57,19 +47,6 @@ class ProviderCloudImage(ConfigValue):
self.connection_type = None
self.connection_port = None
def __eq__(self, other):
if isinstance(other, ProviderCloudImage):
return (self.name == other.name and
self.config_drive == other.config_drive and
self.image_id == other.image_id and
self.image_name == other.image_name and
self.username == other.username and
self.python_path == other.python_path and
self.shell_type == other.shell_type and
self.connection_type == other.connection_type and
self.connection_port == other.connection_port)
return False
def __repr__(self):
return "<ProviderCloudImage %s>" % self.name
@ -80,6 +57,8 @@ class ProviderCloudImage(ConfigValue):
class ProviderLabel(ConfigValue):
ignore_equality = ['pool']
def __init__(self):
self.name = None
self.diskimage = None
@ -121,6 +100,8 @@ class ProviderLabel(ConfigValue):
class ProviderPool(ConfigPool):
ignore_equality = ['provider']
def __init__(self):
self.name = None
self.max_cores = None
@ -138,24 +119,6 @@ class ProviderPool(ConfigPool):
# Initialize base class attributes
super().__init__()
def __eq__(self, other):
if isinstance(other, ProviderPool):
# NOTE(Shrews): We intentionally do not compare 'provider' here
# since this causes recursive checks with OpenStackProviderConfig.
return (super().__eq__(other) and
other.name == self.name and
other.max_cores == self.max_cores and
other.max_ram == self.max_ram and
other.ignore_provider_quota == (
self.ignore_provider_quota) and
other.azs == self.azs and
other.networks == self.networks and
other.security_groups == self.security_groups and
other.auto_floating_ip == self.auto_floating_ip and
other.host_key_checking == self.host_key_checking and
other.labels == self.labels)
return False
def __repr__(self):
return "<ProviderPool %s>" % self.name
@ -240,23 +203,6 @@ class OpenStackProviderConfig(ProviderConfig):
self.post_upload_hook = None
super().__init__(provider)
def __eq__(self, other):
if isinstance(other, OpenStackProviderConfig):
return (super().__eq__(other) and
other.cloud_config == self.cloud_config and
other.pools == self.pools and
other.image_type == self.image_type and
other.rate == self.rate and
other.boot_timeout == self.boot_timeout and
other.launch_timeout == self.launch_timeout and
other.clean_floating_ips == self.clean_floating_ips and
other.port_cleanup_interval ==
self.port_cleanup_interval and
other.diskimages == self.diskimages and
other.cloud_images == self.cloud_images and
other.post_upload_hook == self.post_upload_hook)
return False
def _cloudKwargs(self):
cloud_kwargs = {}
for arg in ['region-name', 'cloud']:

2
nodepool/tests/unit/test_driver_kubernetes.py
View File

@ -44,7 +44,7 @@ class FakeCoreClient(object):
self.namespaces.append(FakeNamespace)
return FakeNamespace
def delete_namespace(self, name, delete_body):
def delete_namespace(self, name, body):
to_delete = None
for namespace in self.namespaces:
if namespace.metadata.name == name: