Merge pull request #884 from helgi/fix_probes

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

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