Force node hold expiration to integer type

Zuul gives us the node hold requests and it may not provide the node
hold expiration value as an integer (this issue is also being fixed at
https://review.openstack.org/597592 in zuul). To guard against hold
clients (zuul and possibly others) giving us the wrong type for the
expiration values we coerce it to int() here if not None.

If the coercion fails we assume a value of 0 which means no expiration.
This allows the node to still be held as the user expected, it just
won't expire.

Change-Id: Iaa9500490b8536470978d94fd01cbaad23d1d647

nodepool/tests/test_launcher.py

@@ -867,6 +867,55 @@ class TestLauncher(tests.DBTestCase):
         manager = pool.getProviderManager('fake-provider')
         self.waitForInstanceDeletion(manager, node.external_id)
 
+    def test_hold_expiration_str_type(self):
+        """Test a held node is deleted when past its operator-specified TTL,
+        even when the type is bad"""
+        configfile = self.setup_config('node_max_hold_age_no_default.yaml')
+        pool = self.useNodepool(configfile, watermark_sleep=1)
+        self.useBuilder(configfile)
+        pool.start()
+        self.waitForImage('fake-provider', 'fake-image')
+        self.log.debug("Waiting for initial pool...")
+        nodes = self.waitForNodes('fake-label')
+        self.log.debug("...done waiting for initial pool.")
+        node = nodes[0]
+        self.log.debug("Holding node %s..." % node.id)
+        # hold the node
+        node.state = zk.HOLD
+        node.comment = 'testing'
+        node.hold_expiration = '1'
+        self.zk.lockNode(node, blocking=False)
+        self.zk.storeNode(node)
+        self.zk.unlockNode(node)
+        znode = self.zk.getNode(node.id)
+        self.log.debug("Node %s in state '%s'" % (znode.id, znode.state))
+        # Wait for the instance to be cleaned up
+        manager = pool.getProviderManager('fake-provider')
+        self.waitForInstanceDeletion(manager, node.external_id)
+
+    def test_hold_expiration_bad_type_coercion(self):
+        """Test a held node uses default expiration value when type is bad"""
+        configfile = self.setup_config('node_max_hold_age_no_default.yaml')
+        pool = self.useNodepool(configfile, watermark_sleep=1)
+        self.useBuilder(configfile)
+        pool.start()
+        self.waitForImage('fake-provider', 'fake-image')
+        self.log.debug("Waiting for initial pool...")
+        nodes = self.waitForNodes('fake-label')
+        self.log.debug("...done waiting for initial pool.")
+        node = nodes[0]
+        self.log.debug("Holding node %s..." % node.id)
+        # hold the node
+        node.state = zk.HOLD
+        node.comment = 'testing'
+        node.hold_expiration = 'notanumber'
+        self.zk.lockNode(node, blocking=False)
+        self.zk.storeNode(node)
+        self.zk.unlockNode(node)
+        znode = self.zk.getNode(node.id)
+        self.log.debug("Node %s in state '%s'" % (znode.id, znode.state))
+        self.assertEqual(znode.hold_expiration, 0)
+
     def test_hold_expiration_lower_than_default(self):
         """Test a held node is deleted when past its operator-specified TTL,
         with max-hold-age set in the configuration"""

nodepool/zk.py

@@ -631,7 +631,19 @@ class Node(BaseModel):
         o.username = d.get('username', 'zuul')
         o.connection_type = d.get('connection_type')
         o.host_keys = d.get('host_keys', [])
-        o.hold_expiration = d.get('hold_expiration')
+        hold_expiration = d.get('hold_expiration')
+        if hold_expiration is not None:
+            try:
+                # We try to force this to an integer value because we do
+                # relative second based age comparisons using this value
+                # and those need to be a number type.
+                o.hold_expiration = int(hold_expiration)
+            except ValueError:
+                # Coercion to int failed, just use default of 0,
+                # which means no expiration
+                o.hold_expiration = 0
+        else:
+            o.hold_expiration = hold_expiration
         return o