feat(healthcheck): Add support to create healthcheck per proctype Requires workflow-cli#160

kmala · kmala · commit 2a41942f2118 · 2016-08-11T11:59:54.000-04:00
diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py
@@ -409,6 +409,10 @@ def _scale_pods(self, scale_types):
             if port:
                 envs['PORT'] = port
 
+            healthcheck = release.config.get_healthcheck().get('scale_type', {})
+            if not healthcheck and scale_type in ['web', 'cmd']:
+                healthcheck = release.config.get_healthcheck().get('web/cmd', {})
+
             kwargs = {
                 'memory': release.config.memory,
                 'cpu': release.config.cpu,
@@ -419,7 +423,7 @@ def _scale_pods(self, scale_types):
                 'replicas': replicas,
                 'app_type': scale_type,
                 'build_type': release.build.type,
-                'healthcheck': release.config.healthcheck,
+                'healthcheck': healthcheck,
                 'routable': routable,
                 'deploy_batches': batches,
                 'deploy_timeout': deploy_timeout,
@@ -487,6 +491,10 @@ def deploy(self, release, force_deploy=False):
             if port:
                 envs['PORT'] = port
 
+            healthcheck = release.config.get_healthcheck().get('scale_type', {})
+            if not healthcheck and scale_type in ['web', 'cmd']:
+                healthcheck = release.config.get_healthcheck().get('web/cmd', {})
+
             deploys[scale_type] = {
                 'memory': release.config.memory,
                 'cpu': release.config.cpu,
@@ -498,7 +506,7 @@ def deploy(self, release, force_deploy=False):
                 'version': version,
                 'app_type': scale_type,
                 'build_type': release.build.type,
-                'healthcheck': release.config.healthcheck,
+                'healthcheck': healthcheck,
                 'routable': routable,
                 'deploy_batches': batches,
                 'deploy_timeout': deploy_timeout,
diff --git a/rootfs/api/models/config.py b/rootfs/api/models/config.py
@@ -46,7 +46,8 @@ def _migrate_legacy_healthcheck(self):
         success_threshold = int(self.values.get('HEALTHCHECK_SUCCESS_THRESHOLD', 1))
         failure_threshold = int(self.values.get('HEALTHCHECK_FAILURE_THRESHOLD', 3))
 
-        self.healthcheck['livenessProbe'] = {
+        self.healthcheck['web/cmd'] = {}
+        self.healthcheck['web/cmd']['livenessProbe'] = {
             'initialDelaySeconds': delay,
             'timeoutSeconds': timeout,
             'periodSeconds': period_seconds,
@@ -57,7 +58,7 @@ def _migrate_legacy_healthcheck(self):
             }
         }
 
-        self.healthcheck['readinessProbe'] = {
+        self.healthcheck['web/cmd']['readinessProbe'] = {
             'initialDelaySeconds': delay,
             'timeoutSeconds': timeout,
             'periodSeconds': period_seconds,
@@ -71,6 +72,12 @@ def _migrate_legacy_healthcheck(self):
         # Unset all the old values
         self.values = {k: v for k, v in self.values.items() if not k.startswith('HEALTHCHECK_')}
 
+    def get_healthcheck(self):
+        if('livenessProbe' in self.healthcheck.keys() or
+           'readinessProbe' in self.healthcheck.keys()):
+            return {'web/cmd': self.healthcheck}
+        return self.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()}
@@ -105,6 +112,38 @@ def set_tags(self, previous_config):
 
         raise DeisException(message)
 
+    def set_healthcheck(self, previous_config):
+        data = getattr(previous_config, 'healthcheck', {}).copy()
+        new_data = getattr(self, 'healthcheck', {}).copy()
+        # update the config data for healthcheck if they are not
+        # present for per proctype
+        # TODO: This is required for backward compatibility and can be
+        # removed in next major version change.
+        if 'livenessProbe' in data.keys() or 'readinessProbe' in data.keys():
+            data = {'web/cmd': data.copy()}
+        if 'livenessProbe' in new_data.keys() or 'readinessProbe' in new_data.keys():  # noqa
+            new_data = {'web/cmd': new_data.copy()}
+
+        # remove config keys if a null value is provided
+        for key, value in new_data.items():
+            if value is None:
+                # error if unsetting non-existing key
+                if key not in data:
+                    raise UnprocessableEntity('{} does not exist under {}'.format(key, 'healthcheck'))  # noqa
+                data.pop(key)
+            else:
+                for probeType, probe in value.items():
+                    if probe is None:
+                        # error if unsetting non-existing key
+                        if key not in data or probeType not in data[key].keys():
+                            raise UnprocessableEntity('{} does not exist under {}'.format(key, 'healthcheck'))  # noqa
+                        data[key].pop(probeType)
+                    else:
+                        if key not in data:
+                            data[key] = {}
+                        data[key][probeType] = probe
+        setattr(self, 'healthcheck', data)
+
     def save(self, **kwargs):
         """merge the old config with the new"""
         try:
@@ -116,7 +155,7 @@ def save(self, **kwargs):
                 # usually means a totally new app
                 previous_config = self.app.config_set.latest()
 
-            for attr in ['cpu', 'memory', 'tags', 'registry', 'values', 'healthcheck']:
+            for attr in ['cpu', 'memory', 'tags', 'registry', 'values']:
                 data = getattr(previous_config, attr, {}).copy()
                 new_data = getattr(self, attr, {}).copy()
 
@@ -131,6 +170,7 @@ def save(self, **kwargs):
                         data[key] = value
                 setattr(self, attr, data)
 
+            self.set_healthcheck(previous_config)
             self._migrate_legacy_healthcheck()
             self.set_registry()
             self.set_tags(previous_config)
diff --git a/rootfs/api/models/release.py b/rootfs/api/models/release.py
@@ -464,17 +464,17 @@ def save(self, *args, **kwargs):  # noqa
                         self.summary += ' and '
                     self.summary += "{} {}".format(self.config.owner, changes)
 
-                # if the registry information changed, log the dict diff
+                # if the healthcheck information changed, log the dict diff
                 changes = []
                 old_healthcheck = old_config.healthcheck if old_config else {}
                 diff = dict_diff(self.config.healthcheck, old_healthcheck)
                 # try to be as succinct as possible
-                added = ', '.join(k for k in diff.get('added', {}))
-                added = 'added healthcheck info ' + added if added else ''
-                changed = ', '.join(k for k in diff.get('changed', {}))
-                changed = 'changed healthcheck info ' + changed if changed else ''
-                deleted = ', '.join(k for k in diff.get('deleted', {}))
-                deleted = 'deleted healthcheck info ' + deleted if deleted else ''
+                added = ', '.join(list(map(lambda x: 'default' if x == '' else x, [k for k in diff.get('added', {})])))  # noqa
+                added = 'added healthcheck info for proc type ' + added if added else ''
+                changed = ', '.join(list(map(lambda x: 'default' if x == '' else x, [k for k in diff.get('changed', {})])))  # noqa
+                changed = 'changed healthcheck info for proc type ' + changed if changed else ''
+                deleted = ', '.join(list(map(lambda x: 'default' if x == '' else x, [k for k in diff.get('deleted', {})])))  # noqa
+                deleted = 'deleted healthcheck info for proc type ' + deleted if deleted else ''
                 changes = ', '.join(i for i in (added, changed, deleted) if i)
                 if changes:
                     if self.summary:
diff --git a/rootfs/api/serializers.py b/rootfs/api/serializers.py
@@ -326,27 +326,29 @@ def validate_registry(self, data):
         return data
 
     def validate_healthcheck(self, data):
-        for key, value in data.items():
-            if value is None:
+        for procType, healthcheck in data.items():
+            if healthcheck is None:
                 continue
+            for key, value in healthcheck.items():
+                if value is None:
+                    continue
+                if key not in ['livenessProbe', 'readinessProbe']:
+                    raise serializers.ValidationError(
+                        "Healthcheck keys must be either livenessProbe or readinessProbe")
+                try:
+                    jsonschema.validate(value, PROBE_SCHEMA)
+                except jsonschema.ValidationError as e:
+                    raise serializers.ValidationError(
+                        "could not validate {}: {}".format(value, e.message))
 
-            if key not in ['livenessProbe', 'readinessProbe']:
+            # http://kubernetes.io/docs/api-reference/v1/definitions/#_v1_probe
+            # liveness only supports successThreshold=1, no other value
+            # This is not in the schema since readiness supports other values
+            threshold = jmespath.search('livenessProbe.successThreshold', healthcheck)
+            if threshold is not None and threshold != 1:
                 raise serializers.ValidationError(
-                    "Healthcheck keys must be either livenessProbe or readinessProbe")
-            try:
-                jsonschema.validate(value, PROBE_SCHEMA)
-            except jsonschema.ValidationError as e:
-                raise serializers.ValidationError(
-                    "could not validate {}: {}".format(value, e.message))
-
-        # http://kubernetes.io/docs/api-reference/v1/definitions/#_v1_probe
-        # liveness only supports successThreshold=1, no other value
-        # This is not in the schema since readiness supports other values
-        threshold = jmespath.search('livenessProbe.successThreshold', data)
-        if threshold is not None and threshold != 1:
-            raise serializers.ValidationError(
-                'livenessProbe successThreshold can only be 1'
-            )
+                    'livenessProbe successThreshold can only be 1'
+                )
 
         return data
 
diff --git a/rootfs/api/tests/test_healthchecks.py b/rootfs/api/tests/test_healthchecks.py
@@ -76,31 +76,65 @@ def test_config_healthchecks(self, mock_requests):
         Test that healthchecks can be applied
         """
         app_id = self.create_app()
-        readiness_probe = {'healthcheck': {'readinessProbe': {'httpGet': {'port': 5000}}}}
+        readiness_probe = {'healthcheck': {'web/cmd': {'readinessProbe':
+                                                       {'httpGet': {'port': 5000}}}}}
 
         response = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
             readiness_probe)
         self.assertEqual(response.status_code, 201, response.data)
-        self.assertIn('readinessProbe', response.data['healthcheck'])
+        self.assertIn('readinessProbe', response.data['healthcheck']['web/cmd'])
         self.assertEqual(response.data['healthcheck'], readiness_probe['healthcheck'])
 
-        liveness_probe = {'healthcheck': {'livenessProbe':
+        liveness_probe = {'healthcheck': {'web/cmd': {'livenessProbe':
                                           {'httpGet': {'port': 5000},
-                                           'successThreshold': 1}}}
+                                           'successThreshold': 1}}}}
         response = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
             liveness_probe)
         self.assertEqual(response.status_code, 201, response.data)
-        self.assertIn('livenessProbe', response.data['healthcheck'])
+        self.assertIn('livenessProbe', response.data['healthcheck']['web/cmd'])
         self.assertEqual(
-            response.data['healthcheck']['livenessProbe'],
-            liveness_probe['healthcheck']['livenessProbe'])
+            response.data['healthcheck']['web/cmd']['livenessProbe'],
+            liveness_probe['healthcheck']['web/cmd']['livenessProbe'])
         # check that the readiness probe is still there too!
-        self.assertIn('readinessProbe', response.data['healthcheck'])
+        self.assertIn('readinessProbe', response.data['healthcheck']['web/cmd'])
         self.assertEqual(
-            response.data['healthcheck']['readinessProbe'],
-            readiness_probe['healthcheck']['readinessProbe'])
+            response.data['healthcheck']['web/cmd']['readinessProbe'],
+            readiness_probe['healthcheck']['web/cmd']['readinessProbe'])
+
+        # check that config fails if trying to unset non-existing healthcheck
+        response = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'healthcheck': {'invalid_proctype': None}})
+        self.assertEqual(response.status_code, 422, response.data)
+
+        # remove a probeType
+        response = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'healthcheck': {'web/cmd': {'livenessProbe': None}}})
+        self.assertEqual(response.status_code, 201, response.data)
+        self.assertNotIn('livenessProbe', response.data['healthcheck']['web/cmd'])
+        self.assertIn('readinessProbe', response.data['healthcheck']['web/cmd'])
+
+        # check that config fails if trying to unset non-existing probeType
+        response = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'healthcheck': {'web/cmd': {'livenessProbe': None}}})
+        self.assertEqual(response.status_code, 422, response.data)
+
+        # check that config fails if trying to unset non-existing probeType
+        response = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'healthcheck': {'invalid_proctype': {'livenessProbe': None}}})
+        self.assertEqual(response.status_code, 422, response.data)
+
+        # check that config fails if trying to unset non-existing probeType
+        response = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'healthcheck': {'web/cmd': None}})
+        self.assertEqual(response.status_code, 201, response.data)
+        self.assertNotIn('web/cmd', response.data['healthcheck'])
 
         # post a new build
         response = self.client.post(
@@ -115,29 +149,37 @@ def test_config_healthchecks_validations(self, mock_requests):
         """
         app_id = self.create_app()
 
+        # Set a probe different from liveness/readiness
+        response = self.client.post(
+            '/v2/apps/{app_id}/config'.format(**locals()),
+            {'healthcheck': json.dumps({'web/cmd': {'testProbe':
+                                        {'httpGet': {'port': '50'}, 'initialDelaySeconds': "1"}}})}
+        )
+        self.assertEqual(response.status_code, 400, response.data)
+
         # Set one of the values that require a numeric value to a string
         response = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
-            {'healthcheck': json.dumps({'livenessProbe':
-                                        {'httpGet': {'port': '50'}, 'initialDelaySeconds': "t"}})}
+            {'healthcheck': json.dumps({'web/cmd': {'livenessProbe':
+                                        {'httpGet': {'port': '50'}, 'initialDelaySeconds': "t"}}})}
         )
         self.assertEqual(response.status_code, 400, response.data)
 
         # Don't set one of the mandatory value
         response = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
-            {'healthcheck': json.dumps({'livenessProbe':
-                                        {'httpGet': {'path': '/'}, 'initialDelaySeconds': 1}})}
+            {'healthcheck': json.dumps({'web/cmd': {'livenessProbe':
+                                        {'httpGet': {'path': '/'}, 'initialDelaySeconds': 1}}})}
         )
         self.assertEqual(response.status_code, 400, response.data)
 
         # set liveness success threshold to a non-1 value
         # Don't set one of the mandatory value
         response = self.client.post(
             '/v2/apps/{app_id}/config'.format(**locals()),
-            {'healthcheck': {'livenessProbe':
+            {'healthcheck': {'web/cmd': {'livenessProbe':
                              {'httpGet': {'path': '/', 'port': 5000},
-                              'successThreshold': 5}}}
+                              'successThreshold': 5}}}}
         )
         self.assertEqual(response.status_code, 400, response.data)
 
@@ -158,7 +200,7 @@ def test_config_healthchecks_legacy(self, mock_requests):
         # this gets migrated to the new healtcheck format
         self.assertNotIn('HEALTHCHECK_URL', response.data['values'])
         # legacy defaults
-        expected = {
+        expected = {'web/cmd': {
             'livenessProbe': {
                 'initialDelaySeconds': 50,
                 'timeoutSeconds': 50,
@@ -179,6 +221,7 @@ def test_config_healthchecks_legacy(self, mock_requests):
                     'path': '/health'
                 }
             }
+            }
         }
         actual = app.config_set.latest().healthcheck
         self.assertEqual(actual, expected)
@@ -198,7 +241,7 @@ def test_config_healthchecks_legacy(self, mock_requests):
         self.assertEqual(response.status_code, 201, response.data)
         # this gets migrated to the new healtcheck format
         self.assertNotIn('HEALTHCHECK_INITIAL_DELAY', response.data['values'])
-        expected['livenessProbe'] = {
+        expected['web/cmd']['livenessProbe'] = {
             'initialDelaySeconds': 25,
             'timeoutSeconds': 10,
             'periodSeconds': 5,
@@ -208,6 +251,6 @@ def test_config_healthchecks_legacy(self, mock_requests):
                 'path': '/health'
             }
         }
-        expected['readinessProbe'] = expected['livenessProbe']
+        expected['web/cmd']['readinessProbe'] = expected['web/cmd']['livenessProbe']
         actual = app.config_set.latest().healthcheck
         self.assertEqual(expected, actual)
diff --git a/rootfs/scheduler/__init__.py b/rootfs/scheduler/__init__.py
@@ -581,23 +581,22 @@ def _set_container(self, namespace, container_name, data, **kwargs):
             data["resources"]["limits"]["cpu"] = cpu.lower()
 
         # add in healthchecks
-        self._set_health_checks(data, env, kwargs)
-
-    def _set_health_checks(self, container, env, kwargs):
-        if kwargs.get('routable', False):
-            healthchecks = kwargs.get('healthcheck', None)
-            if healthchecks:
-                # check if a port is present. if not, auto-populate it
-                # TODO: rip this out when we stop supporting deis config:set HEALTHCHECK_URL
-                if (
-                    healthchecks.get('livenessProbe') is not None and
-                    healthchecks['livenessProbe'].get('httpGet') is not None and
-                    healthchecks['livenessProbe']['httpGet'].get('port') is None
-                ):
-                    healthchecks['livenessProbe']['httpGet']['port'] = env['PORT']
-                container.update(healthchecks)
-            else:
-                self._default_readiness_probe(container, kwargs.get('build_type'), env.get('PORT', None))  # noqa
+        self._set_health_checks(data, env, **kwargs)
+
+    def _set_health_checks(self, container, env, **kwargs):
+        healthchecks = kwargs.get('healthcheck', None)
+        if healthchecks:
+            # check if a port is present. if not, auto-populate it
+            # TODO: rip this out when we stop supporting deis config:set HEALTHCHECK_URL
+            if (
+                healthchecks.get('livenessProbe') is not None and
+                healthchecks['livenessProbe'].get('httpGet') is not None and
+                healthchecks['livenessProbe']['httpGet'].get('port') is None
+            ):
+                healthchecks['livenessProbe']['httpGet']['port'] = env['PORT']
+            container.update(healthchecks)
+        elif kwargs.get('routable', False):
+            self._default_readiness_probe(container, kwargs.get('build_type'), env.get('PORT', None))  # noqa
 
     def _get_private_registry_config(self, registry, image):
         secret_name = settings.REGISTRY_SECRET_PREFIX
diff --git a/rootfs/scheduler/tests/test_scheduler.py b/rootfs/scheduler/tests/test_scheduler.py
