Merge "Fix ConfigValue comparisons"

nodepool/config.py

@@ -38,6 +38,27 @@ class Config(ConfigValue):
         self.providers = {}
         self.provider_managers = {}
         self.zookeeper_servers = {}
+        self.elementsdir = None
+        self.imagesdir = None
+        self.build_log_dir = None
+        self.build_log_retention = None
+        self.max_hold_age = None
+        self.webapp = None
+
+    def __eq__(self, other):
+        if isinstance(other, Config):
+            return (self.diskimages == other.diskimages and
+                    self.labels == other.labels and
+                    self.providers == other.providers and
+                    self.provider_managers == other.provider_managers and
+                    self.zookeeper_servers == other.zookeeper_servers and
+                    self.elementsdir == other.elementsdir and
+                    self.imagesdir == other.imagesdir and
+                    self.build_log_dir == other.build_log_dir and
+                    self.build_log_retention == other.build_log_retention and
+                    self.max_hold_age == other.max_hold_age and
+                    self.webapp == other.webapp)
+        return False
 
     def setElementsDir(self, value):
         self.elementsdir = value
@@ -125,25 +146,46 @@ class Config(ConfigValue):
 
 
 class Label(ConfigValue):
+    def __init__(self):
+        self.name = None
+        self.max_ready_age = None
+        self.min_ready = None
+        self.pools = None
+
+    def __eq__(self, other):
+        if isinstance(other, Label):
+            return (self.name == other.name and
+                    self.max_ready_age == other.max_ready_age and
+                    self.min_ready == other.min_ready and
+                    self.pools == other.pools)
+        return False
+
     def __repr__(self):
         return "<Label %s>" % self.name
 
 
 class DiskImage(ConfigValue):
-    def __eq__(self, other):
-        if (other.name != self.name or
-            other.elements != self.elements or
-            other.release != self.release or
-            other.rebuild_age != self.rebuild_age or
-            other.env_vars != self.env_vars or
-            other.image_types != self.image_types or
-            other.pause != self.pause or
-            other.username != self.username):
-            return False
-        return True
+    def __init__(self):
+        self.name = None
+        self.elements = None
+        self.release = None
+        self.rebuild_age = None
+        self.env_vars = None
+        self.image_types = None
+        self.pause = False
+        self.username = None
 
-    def __ne__(self, other):
-        return not self.__eq__(other)
+    def __eq__(self, other):
+        if isinstance(other, DiskImage):
+            return (other.name == self.name and
+                    other.elements == self.elements and
+                    other.release == self.release and
+                    other.rebuild_age == self.rebuild_age and
+                    other.env_vars == self.env_vars and
+                    other.image_types == self.image_types and
+                    other.pause == self.pause and
+                    other.username == self.username)
+        return False
 
     def __repr__(self):
         return "<DiskImage %s>" % self.name

nodepool/driver/__init__.py

@@ -682,12 +682,10 @@ class NodeLauncher(threading.Thread, stats.StatsReporter):
             self.log.exception("Exception while reporting stats:")
 
 
-class ConfigValue(object):
+class ConfigValue(object, metaclass=abc.ABCMeta):
+    @abc.abstractmethod
     def __eq__(self, other):
-        if isinstance(other, ConfigValue):
-            if other.__dict__ == self.__dict__:
-                return True
-        return False
+        pass
 
     def __ne__(self, other):
         return not self.__eq__(other)
@@ -695,12 +693,24 @@ class ConfigValue(object):
 
 class ConfigPool(ConfigValue):
     def __init__(self):
-        self.labels = []
+        self.labels = {}
         self.max_servers = math.inf
 
+    def __eq__(self, other):
+        if isinstance(other, ConfigPool):
+            return (self.labels == other.labels and
+                    self.max_servers == other.max_servers)
+        return False
+
 
 class Driver(ConfigValue):
-    pass
+    def __init__(self):
+        self.name = None
+
+    def __eq__(self, other):
+        if isinstance(other, Driver):
+            return self.name == other.name
+        return False
 
 
 class ProviderConfig(ConfigValue, metaclass=abc.ABCMeta):
@@ -716,6 +726,14 @@ 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
 
@@ -735,10 +753,6 @@ class ProviderConfig(ConfigValue, metaclass=abc.ABCMeta):
         '''
         pass
 
-    @abc.abstractmethod
-    def __eq__(self, other):
-        pass
-
     @abc.abstractmethod
     def reset():
         pass

nodepool/driver/openstack/config.py

@@ -24,11 +24,49 @@ from nodepool.driver import ConfigPool
 
 
 class ProviderDiskImage(ConfigValue):
+    def __init__(self):
+        self.name = None
+        self.pause = False
+        self.config_drive = None
+        self.connection_type = None
+        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
 
 
 class ProviderCloudImage(ConfigValue):
+    def __init__(self):
+        self.name = None
+        self.config_drive = None
+        self.image_id = None
+        self.image_name = None
+        self.username = None
+        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.connection_type == other.connection_type and
+                    self.connection_port == other.connection_port)
+        return False
+
     def __repr__(self):
         return "<ProviderCloudImage %s>" % self.name
 
@@ -47,30 +85,68 @@ class ProviderCloudImage(ConfigValue):
 
 
 class ProviderLabel(ConfigValue):
+    def __init__(self):
+        self.name = None
+        self.diskimage = None
+        self.cloud_image = None
+        self.min_ram = None
+        self.flavor_name = None
+        self.key_name = None
+        self.console_log = False
+        self.boot_from_volume = False
+        self.volume_size = None
+        # The ProviderPool object that owns this label.
+        self.pool = None
+
     def __eq__(self, other):
-        if (other.diskimage != self.diskimage or
-            other.cloud_image != self.cloud_image or
-            other.min_ram != self.min_ram or
-            other.flavor_name != self.flavor_name or
-            other.key_name != self.key_name):
-            return False
-        return True
+        if isinstance(other, ProviderLabel):
+            # NOTE(Shrews): We intentionally do not compare 'pool' here
+            # since this causes recursive checks with ProviderPool.
+            return (other.diskimage == self.diskimage and
+                    other.cloud_image == self.cloud_image and
+                    other.min_ram == self.min_ram and
+                    other.flavor_name == self.flavor_name and
+                    other.key_name == self.key_name and
+                    other.name == self.name and
+                    other.console_log == self.console_log and
+                    other.boot_from_volume == self.boot_from_volume and
+                    other.volume_size == self.volume_size)
+        return False
 
     def __repr__(self):
         return "<ProviderLabel %s>" % self.name
 
 
 class ProviderPool(ConfigPool):
+    def __init__(self):
+        self.name = None
+        self.max_cores = None
+        self.max_ram = None
+        self.azs = None
+        self.networks = None
+        self.auto_floating_ip = True
+        self.host_key_checking = True
+        self.labels = None
+        # The OpenStackProviderConfig object that owns this pool.
+        self.provider = None
+
+        # Initialize base class attributes
+        super().__init__()
+
     def __eq__(self, other):
-        if (other.labels != self.labels or
-            other.max_cores != self.max_cores or
-            other.max_servers != self.max_servers or
-            other.max_ram != self.max_ram or
-            other.azs != self.azs or
-            other.host_key_checking != self.host_key_checking or
-            other.networks != self.networks):
-            return False
-        return True
+        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.azs == self.azs and
+                    other.networks == self.networks 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
@@ -81,20 +157,31 @@ class OpenStackProviderConfig(ProviderConfig):
 
     def __init__(self, *args, **kwargs):
         self.__pools = {}
+        self.cloud_config = None
+        self.image_type = None
+        self.rate = None
+        self.boot_timeout = None
+        self.launch_timeout = None
+        self.clean_floating_ips = None
+        self.diskimages = {}
+        self.cloud_images = {}
+        self.hostname_format = None
+        self.image_name_format = None
         super().__init__(*args, **kwargs)
 
     def __eq__(self, other):
-        if (other.cloud_config != self.cloud_config or
-            other.pools != self.pools or
-            other.image_type != self.image_type or
-            other.rate != self.rate or
-            other.boot_timeout != self.boot_timeout or
-            other.launch_timeout != self.launch_timeout or
-            other.clean_floating_ips != self.clean_floating_ips or
-            other.max_concurrency != self.max_concurrency or
-            other.diskimages != self.diskimages):
-            return False
-        return True
+        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.diskimages == self.diskimages and
+                    other.cloud_images == self.cloud_images)
+        return False
 
     def _cloudKwargs(self):
         cloud_kwargs = {}
@@ -143,7 +230,6 @@ class OpenStackProviderConfig(ProviderConfig):
             'winrm': 5986,
         }
 
-        self.diskimages = {}
         for image in self.provider.get('diskimages', []):
             i = ProviderDiskImage()
             i.name = image['name']
@@ -171,7 +257,6 @@ class OpenStackProviderConfig(ProviderConfig):
                     #               % i.name)
                     i.meta = {}
 
-        self.cloud_images = {}
         for image in self.provider.get('cloud-images', []):
             i = ProviderCloudImage()
             i.name = image['name']
@@ -197,7 +282,7 @@ class OpenStackProviderConfig(ProviderConfig):
             pp.networks = pool.get('networks', [])
             pp.auto_floating_ip = bool(pool.get('auto-floating-ip', True))
             pp.host_key_checking = bool(pool.get('host-key-checking', True))
-            pp.labels = {}
+
             for label in pool.get('labels', []):
                 pl = ProviderLabel()
                 pl.name = label['name']

nodepool/driver/static/config.py

@@ -22,11 +22,21 @@ from nodepool.config import as_list
 
 
 class StaticPool(ConfigPool):
+    def __init__(self):
+        self.name = None
+        self.nodes = []
+        # The StaticProviderConfig that owns this pool.
+        self.provider = None
+
+        # Initialize base class attributes
+        super().__init__()
+
     def __eq__(self, other):
-        if (other.labels != self.labels or
-            other.nodes != self.nodes):
-            return False
-        return True
+        if isinstance(other, StaticPool):
+            return (super().__eq__(other) and
+                    other.name == self.name and
+                    other.nodes == self.nodes)
+        return False
 
     def __repr__(self):
         return "<StaticPool %s>" % self.name
@@ -38,9 +48,11 @@ class StaticProviderConfig(ProviderConfig):
         super().__init__(*args, **kwargs)
 
     def __eq__(self, other):
-        if other.pools != self.pools:
-            return False
-        return True
+        if isinstance(other, StaticProviderConfig):
+            return (super().__eq__(other) and
+                    other.manage_images == self.manage_images and
+                    other.pools == self.pools)
+        return False
 
     @staticmethod
     def reset():
@@ -60,8 +72,8 @@ class StaticProviderConfig(ProviderConfig):
             pp.name = pool['name']
             pp.provider = self
             self.pools[pp.name] = pp
+            # WARNING: This intentionally changes the type!
             pp.labels = set()
-            pp.nodes = []
             for node in pool.get('nodes', []):
                 pp.nodes.append({
                     'name': node['name'],

nodepool/tests/test_config_comparisons.py

@@ -0,0 +1,127 @@
+#
+# 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 import tests
+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.openstack.config import OpenStackProviderConfig
+from nodepool.driver.openstack.config import ProviderDiskImage
+from nodepool.driver.openstack.config import ProviderCloudImage
+from nodepool.driver.openstack.config import ProviderLabel
+from nodepool.driver.openstack.config import ProviderPool
+from nodepool.driver.static.config import StaticPool
+from nodepool.driver.static.config import StaticProviderConfig
+
+
+class TestConfigComparisons(tests.BaseTestCase):
+
+    def test_ConfigPool(self):
+        a = ConfigPool()
+        b = ConfigPool()
+        self.assertEqual(a, b)
+        a.max_servers = 5
+        self.assertNotEqual(a, b)
+
+    def test_Driver(self):
+        a = Driver()
+        b = Driver()
+        self.assertEqual(a, b)
+        a.name = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_Config(self):
+        a = Config()
+        b = Config()
+        self.assertEqual(a, b)
+        a.imagesdir = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_Label(self):
+        a = Label()
+        b = Label()
+        self.assertEqual(a, b)
+        a.name = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_DiskImage(self):
+        a = DiskImage()
+        b = DiskImage()
+        self.assertEqual(a, b)
+        a.name = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_ProviderDiskImage(self):
+        a = ProviderDiskImage()
+        b = ProviderDiskImage()
+        self.assertEqual(a, b)
+        a.name = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_ProviderCloudImage(self):
+        a = ProviderCloudImage()
+        b = ProviderCloudImage()
+        self.assertEqual(a, b)
+        a.name = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_ProviderLabel(self):
+        a = ProviderLabel()
+        b = ProviderLabel()
+        self.assertEqual(a, b)
+        a.name = "foo"
+        self.assertNotEqual(a, b)
+
+    def test_ProviderPool(self):
+        a = ProviderPool()
+        b = ProviderPool()
+        self.assertEqual(a, b)
+        # intentionally change an attribute of the base class
+        a.max_servers = 5
+        self.assertNotEqual(a, b)
+
+        c = ConfigPool()
+        d = ProviderPool()
+        self.assertNotEqual(c, d)
+
+    def test_OpenStackProviderConfig(self):
+        provider = {'name': 'foo'}
+        a = OpenStackProviderConfig(provider)
+        b = OpenStackProviderConfig(provider)
+        self.assertEqual(a, b)
+        # intentionally change an attribute of the base class
+        a.name = 'bar'
+        self.assertNotEqual(a, b)
+
+    def test_StaticPool(self):
+        a = StaticPool()
+        b = StaticPool()
+        self.assertEqual(a, b)
+        # intentionally change an attribute of the base class
+        a.max_servers = 5
+        self.assertNotEqual(a, b)
+        c = ConfigPool()
+        self.assertNotEqual(b, c)
+
+    def test_StaticProviderConfig(self):
+        provider = {'name': 'foo'}
+        a = StaticProviderConfig(provider)
+        b = StaticProviderConfig(provider)
+        self.assertEqual(a, b)
+        # intentionally change an attribute of the base class
+        a.name = 'bar'
+        self.assertNotEqual(a, b)