fix(healthchecks): when migrating from old healthchecks also add in readiness probe

Also get the max() between liveness and readiness initial seconds delay to use in deploy timeout. It is smart enough to deal with if one or both are missing as well.

Fixes #874

Author: helgi

rootfs/api/models/config.py

@@ -56,6 +56,20 @@ def _migrate_legacy_healthcheck(self):
             }
         }
 
+        self.healthcheck['readinessProbe'] = {
+            'initialDelaySeconds': delay,
+            'timeoutSeconds': timeout,
+            'periodSeconds': period_seconds,
+            'successThreshold': success_threshold,
+            'failureThreshold': failure_threshold,
+            'httpGet': {
+                'path': path,
+            }
+        }
+
+        # Unset all the old values
+        self.values = {k: v for k, v in self.values.items() if not k.startswith('HEALTHCHECK_')}
+
     def set_registry(self):
         # lower case all registry options for consistency
         self.registry = {key.lower(): value for key, value in self.registry.copy().items()}

rootfs/api/tests/deployments/test_config.py

@@ -773,39 +773,6 @@ def test_unauthorized_user_cannot_modify_config(self, mock_requests):
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 403)
 
-    def test_healthchecks(self, mock_requests):
-        """
-        Test that healthchecks can be applied
-        """
-        response = self.client.post('/v2/apps')
-        self.assertEqual(response.status_code, 201, response.data)
-        app_id = response.data['id']
-
-        # Set a healthcheck option before URL is around (URL is required for full setting)
-        resp = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_INITIAL_DELAY': '25'})}
-        )
-        self.assertEqual(resp.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_INITIAL_DELAY', resp.data['values'])
-        self.assertEqual(resp.data['values']['HEALTHCHECK_INITIAL_DELAY'], '25')
-
-        # Set healthcheck URL to get defaults set
-        resp = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
-        )
-        self.assertEqual(resp.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_URL', resp.data['values'])
-        self.assertEqual(resp.data['values']['HEALTHCHECK_URL'], '/health')
-
-        # post a new build
-        response = self.client.post(
-            "/v2/apps/{app_id}/builds".format(**locals()),
-            {'image': 'quay.io/autotest/example'}
-        )
-        self.assertEqual(response.status_code, 201, response.data)
-
     def test_healthchecks_validations(self, mock_requests):
         """
         Test that healthchecks validations work
@@ -929,8 +896,8 @@ def test_config_healthchecks_legacy(self, mock_requests):
             {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
         )
         self.assertEqual(response.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_URL', response.data['values'])
-        self.assertEqual(response.data['values']['HEALTHCHECK_URL'], '/health')
+        # this gets migrated to the new healtcheck format
+        self.assertNotIn('HEALTHCHECK_URL', response.data['values'])
         # legacy defaults
         expected = {
             'livenessProbe': {
@@ -942,6 +909,16 @@ def test_config_healthchecks_legacy(self, mock_requests):
                 'httpGet': {
                     'path': '/health'
                 }
+            },
+            'readinessProbe': {
+                'initialDelaySeconds': 50,
+                'timeoutSeconds': 50,
+                'periodSeconds': 10,
+                'successThreshold': 1,
+                'failureThreshold': 3,
+                'httpGet': {
+                    'path': '/health'
+                }
             }
         }
         actual = app.config_set.latest().healthcheck
@@ -951,6 +928,7 @@ def test_config_healthchecks_legacy(self, mock_requests):
             '/v2/apps/{app.id}/config'.format(**locals()),
             {
                 'values': json.dumps({
+                    'HEALTHCHECK_URL': '/health',
                     'HEALTHCHECK_INITIAL_DELAY': '25',
                     'HEALTHCHECK_TIMEOUT': '10',
                     'HEALTHCHECK_PERIOD_SECONDS': '5',
@@ -959,8 +937,8 @@ def test_config_healthchecks_legacy(self, mock_requests):
             }
         )
         self.assertEqual(response.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_INITIAL_DELAY', response.data['values'])
-        self.assertEqual(response.data['values']['HEALTHCHECK_INITIAL_DELAY'], '25')
+        # this gets migrated to the new healtcheck format
+        self.assertNotIn('HEALTHCHECK_INITIAL_DELAY', response.data['values'])
         expected['livenessProbe'] = {
             'initialDelaySeconds': 25,
             'timeoutSeconds': 10,
@@ -971,5 +949,6 @@ def test_config_healthchecks_legacy(self, mock_requests):
                 'path': '/health'
             }
         }
+        expected['readinessProbe'] = expected['livenessProbe']
         actual = app.config_set.latest().healthcheck
         self.assertEqual(expected, actual)

rootfs/api/tests/test_config.py

@@ -771,39 +771,6 @@ def test_unauthorized_user_cannot_modify_config(self, mock_requests):
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 403)
 
-    def test_healthchecks(self, mock_requests):
-        """
-        Test that healthchecks can be applied
-        """
-        response = self.client.post('/v2/apps')
-        self.assertEqual(response.status_code, 201, response.data)
-        app_id = response.data['id']
-
-        # Set a healthcheck option before URL is around (URL is required for full setting)
-        resp = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_INITIAL_DELAY': '25'})}
-        )
-        self.assertEqual(resp.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_INITIAL_DELAY', resp.data['values'])
-        self.assertEqual(resp.data['values']['HEALTHCHECK_INITIAL_DELAY'], '25')
-
-        # Set healthcheck URL to get defaults set
-        resp = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
-        )
-        self.assertEqual(resp.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_URL', resp.data['values'])
-        self.assertEqual(resp.data['values']['HEALTHCHECK_URL'], '/health')
-
-        # post a new build
-        response = self.client.post(
-            "/v2/apps/{app_id}/builds".format(**locals()),
-            {'image': 'quay.io/autotest/example'}
-        )
-        self.assertEqual(response.status_code, 201, response.data)
-
     def test_healthchecks_validations(self, mock_requests):
         """
         Test that healthchecks validations work
@@ -927,8 +894,8 @@ def test_config_healthchecks_legacy(self, mock_requests):
             {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
         )
         self.assertEqual(response.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_URL', response.data['values'])
-        self.assertEqual(response.data['values']['HEALTHCHECK_URL'], '/health')
+        # this gets migrated to the new healtcheck format
+        self.assertNotIn('HEALTHCHECK_URL', response.data['values'])
         # legacy defaults
         expected = {
             'livenessProbe': {
@@ -940,6 +907,16 @@ def test_config_healthchecks_legacy(self, mock_requests):
                 'httpGet': {
                     'path': '/health'
                 }
+            },
+            'readinessProbe': {
+                'initialDelaySeconds': 50,
+                'timeoutSeconds': 50,
+                'periodSeconds': 10,
+                'successThreshold': 1,
+                'failureThreshold': 3,
+                'httpGet': {
+                    'path': '/health'
+                }
             }
         }
         actual = app.config_set.latest().healthcheck
@@ -949,6 +926,7 @@ def test_config_healthchecks_legacy(self, mock_requests):
             '/v2/apps/{app.id}/config'.format(**locals()),
             {
                 'values': json.dumps({
+                    'HEALTHCHECK_URL': '/health',
                     'HEALTHCHECK_INITIAL_DELAY': '25',
                     'HEALTHCHECK_TIMEOUT': '10',
                     'HEALTHCHECK_PERIOD_SECONDS': '5',
@@ -957,8 +935,8 @@ def test_config_healthchecks_legacy(self, mock_requests):
             }
         )
         self.assertEqual(response.status_code, 201, response.data)
-        self.assertIn('HEALTHCHECK_INITIAL_DELAY', response.data['values'])
-        self.assertEqual(response.data['values']['HEALTHCHECK_INITIAL_DELAY'], '25')
+        # this gets migrated to the new healtcheck format
+        self.assertNotIn('HEALTHCHECK_INITIAL_DELAY', response.data['values'])
         expected['livenessProbe'] = {
             'initialDelaySeconds': 25,
             'timeoutSeconds': 10,
@@ -969,5 +947,6 @@ def test_config_healthchecks_legacy(self, mock_requests):
                 'path': '/health'
             }
         }
+        expected['readinessProbe'] = expected['livenessProbe']
         actual = app.config_set.latest().healthcheck
         self.assertEqual(expected, actual)

rootfs/scheduler/__init__.py

@@ -875,26 +875,45 @@ def _wait_until_pods_terminate(self, namespace, labels, current, desired):
 
         self.log(namespace, "{} pods are terminated".format(delta))
 
-    def _wait_until_pods_are_ready(self, namespace, containers, labels, desired):  # noqa
-        # If desired is 0 then there is no ready state to check on
-        if desired == 0:
-            return
+    def _deploy_probe_timeout(self, timeout, namespace, labels, containers):
+        """
+        Added in additional timeouts based on readiness and liveness probe
 
-        timeout = self.deploy_timeout
+        Uses the max of the two instead of combining them as the checks are stacked.
+        """
 
         container_name = '{}-{}'.format(labels['app'], labels['type'])
         container = self._find_container(container_name, containers)
 
         # get health info from container
+        added_timeout = []
         if 'readinessProbe' in container:
             # If there is initial delay on the readiness check then timeout needs to be higher
             # this is to account for kubernetes having readiness check report as failure until
             # the initial delay period is up
-            delay = int(container['readinessProbe'].get('initialDelaySeconds', 50))
-            self.log(namespace, "adding {}s on to the original {}s timeout to account for the initial delay specified in the readiness probe".format(delay, timeout))  # noqa
+            added_timeout.append(int(container['readinessProbe'].get('initialDelaySeconds', 50)))
+
+        if 'livenessProbe' in container:
+            # If there is initial delay on the readiness check then timeout needs to be higher
+            # this is to account for kubernetes having liveness check report as failure until
+            # the initial delay period is up
+            added_timeout.append(int(container['livenessProbe'].get('initialDelaySeconds', 50)))
+
+        if added_timeout:
+            delay = max(added_timeout)
+            self.log(namespace, "adding {}s on to the original {}s timeout to account for the initial delay specified in the liveness / readiness probe".format(delay, timeout))  # noqa
             timeout += delay
 
-        self.log(namespace, "waiting for {} pods to be in services ({}s timeout)".format(desired, timeout))  # noqa
+        return timeout
+
+    def _wait_until_pods_are_ready(self, namespace, containers, labels, desired):  # noqa
+        # If desired is 0 then there is no ready state to check on
+        if desired == 0:
+            return
+
+        timeout = self.deploy_timeout
+        timeout = self._deploy_probe_timeout(timeout, namespace, labels, containers)
+        self.log(namespace, "waiting for {} pods in {} namespace to be in services ({}s timeout)".format(desired, namespace, timeout))  # noqa
 
         # Ensure the minimum desired number of pods are available
         waited = 0
@@ -1655,15 +1674,7 @@ def _wait_until_deployment_is_ready(self, namespace, name, **kwargs):
             return
 
         # get health info from container
-        container_name = '{}-{}'.format(labels['app'], labels['type'])
-        container = self._find_container(container_name, containers)
-        if 'readinessProbe' in container:
-            # If there is initial delay on the readiness check then timeout needs to be higher
-            # this is to account for kubernetes having readiness check report as failure until
-            # the initial delay period is up
-            delay = int(container['readinessProbe'].get('initialDelaySeconds', 50))
-            self.log(namespace, "adding {}s on to the original {}s timeout to account for the initial delay specified in the readiness probe".format(delay, deploy_timeout))  # noqa
-            deploy_timeout += delay
+        deploy_timeout = self._deploy_probe_timeout(deploy_timeout, namespace, labels, containers)
 
         # a rough calculation that figures out an overall timeout
         timeout = len(batches) * deploy_timeout