fix(healthcheck): delete outdated code

duanhongyi · duanhongyi · commit 3cea16605c95 · 2024-03-06T18:15:16.000+08:00
diff --git a/rootfs/api/models/app.py b/rootfs/api/models/app.py
@@ -1118,9 +1118,7 @@ def _gather_app_settings(self, release, app_settings, process_type, replicas, vo
         # https://www.drycc.cc/applications/managing-app-processes/#default-process-types
         routable = True if process_type == 'web' and app_settings.routable else False
 
-        healthcheck = config.get_healthcheck().get(process_type, {})
-        if not healthcheck and process_type == 'web':
-            healthcheck = config.get_healthcheck().get('web', {})
+        healthcheck = config.healthcheck.get(process_type, {})
         volumes, volume_mounts = self._get_volumes_and_mounts(process_type, volumes)
         return {
             'memory': memory,
diff --git a/rootfs/api/models/config.py b/rootfs/api/models/config.py
@@ -40,55 +40,6 @@ class Meta:
     def __str__(self):
         return "{}-{}".format(self.app.id, str(self.uuid)[:7])
 
-    def _migrate_legacy_healthcheck(self):
-        """
-        Get all healthchecks options together for use in scheduler
-        """
-        # return if no legacy healthcheck is found
-        if 'HEALTHCHECK_URL' not in self.values.keys():
-            return
-
-        path = self.values.get('HEALTHCHECK_URL', '/')
-        timeout = int(self.values.get('HEALTHCHECK_TIMEOUT', 50))
-        delay = int(self.values.get('HEALTHCHECK_INITIAL_DELAY', 50))
-        period_seconds = int(self.values.get('HEALTHCHECK_PERIOD_SECONDS', 10))
-        success_threshold = int(self.values.get('HEALTHCHECK_SUCCESS_THRESHOLD', 1))
-        failure_threshold = int(self.values.get('HEALTHCHECK_FAILURE_THRESHOLD', 3))
-
-        self.healthcheck['web'] = {}
-        self.healthcheck['web']['livenessProbe'] = {
-            'initialDelaySeconds': delay,
-            'timeoutSeconds': timeout,
-            'periodSeconds': period_seconds,
-            'successThreshold': success_threshold,
-            'failureThreshold': failure_threshold,
-            'httpGet': {
-                'path': path,
-            }
-        }
-
-        self.healthcheck['web']['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 get_healthcheck(self):
-        if (
-            'livenessProbe' in self.healthcheck.keys() or
-            'readinessProbe' in self.healthcheck.keys()
-        ):
-            return {'web': self.healthcheck}
-        return self.healthcheck
-
     def _set_cpu_memory(self):
         """
         According to settings.KUBERNETES_CPU_MEMORY_RATIO corrects cpu and memory
@@ -200,15 +151,6 @@ def set_tags(self, previous_config):
     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': data.copy()}
-        if 'livenessProbe' in new_data.keys() or 'readinessProbe' in new_data.keys():  # noqa
-            new_data = {'web': new_data.copy()}
-
         # remove config keys if a null value is provided
         for key, value in new_data.items():
             if value is None:
@@ -258,7 +200,6 @@ def save(self, **kwargs):
                 setattr(self, attr, data)
             self._set_cpu_memory()
             self.set_healthcheck(previous_config)
-            self._migrate_legacy_healthcheck()
             self.set_registry()
             self.set_tags(previous_config)
         except Config.DoesNotExist:
diff --git a/rootfs/api/serializers.py b/rootfs/api/serializers.py
@@ -4,11 +4,9 @@
 import time
 import json
 import logging
-import jmespath
 import re
 import jsonschema
 import idna
-from urllib.parse import urlparse
 
 from django.conf import settings
 from django.contrib.auth import get_user_model
@@ -62,6 +60,8 @@
 ) % (settings.KUBERNETES_LIMITS_MAX_VOLUME, settings.KUBERNETES_LIMITS_MIN_VOLUME)
 VOLUME_PATH_MATCH = re.compile(r'^\/(\w+\/?)+$', re.IGNORECASE)
 METRIC_EVERY_MATCH = re.compile(r'^[1-9][0-9]*m$')
+HEALTHCHECK_MATCH = re.compile(r'^(livenessProbe|readinessProbe|startupProbe)$')
+HEALTHCHECK_MISMATCH_MSG = "Healthcheck pattern: %s" % HEALTHCHECK_MATCH.pattern
 
 
 class JSONFieldSerializer(serializers.JSONField):
@@ -211,31 +211,6 @@ def validate_values(data):
                     # check if hte port is between 1 and 65535. One extra added for range()
                     # http://kubernetes.io/docs/api-reference/v1/definitions/#_v1_serviceport
                     raise serializers.ValidationError('PORT needs to be between 1 and 65535')
-
-            # Validate HEALTHCHECK_*
-            if key == 'HEALTHCHECK_URL':
-                # Only Path information is supported, not query / anchor or anything else
-                # Path is the only thing Kubernetes supports right now
-                # See https://github.com/drycc/controller/issues/774
-                uri = urlparse(value)
-
-                if not uri.path:
-                    raise serializers.ValidationError(
-                        '{} is missing a URI path (such as /healthz). '
-                        'Without it no health check can be done'.format(key)
-                    )
-
-                # Disallow everything but path
-                # https://docs.python.org/3/library/urllib.parse.html
-                if uri.query or uri.fragment or uri.netloc:
-                    raise serializers.ValidationError(
-                        '{} can only be a URI path (such as /healthz) that does not contain '
-                        'other things such as query params'.format(key)
-                    )
-            elif key.startswith('HEALTHCHECK_') and not str(value).isnumeric():
-                # all other healthchecks are integers
-                raise serializers.ValidationError('{} can only be a numeric value'.format(key))
-
         return data
 
     @staticmethod
@@ -345,30 +320,19 @@ def validate_registry(data):
 
     @staticmethod
     def validate_healthcheck(data):
-        for procType, healthcheck in data.items():
+        for _, 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")
+                if not re.match(HEALTHCHECK_MATCH, key):
+                    raise serializers.ValidationError(HEALTHCHECK_MISMATCH_MSG)
                 try:
                     jsonschema.validate(value, HEALTHCHECK_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', healthcheck)
-            if threshold is not None and threshold != 1:
-                raise serializers.ValidationError(
-                    'livenessProbe successThreshold can only be 1'
-                )
-
         return data
 
 
diff --git a/rootfs/api/tests/test_healthchecks.py b/rootfs/api/tests/test_healthchecks.py
@@ -5,7 +5,6 @@
 from django.contrib.auth import get_user_model
 from rest_framework.authtoken.models import Token
 
-from api.models.app import App
 from api.tests import adapter, DryccTransactionTestCase
 
 User = get_user_model()
@@ -26,50 +25,6 @@ def tearDown(self):
         # make sure every test has a clean slate for k8s mocking
         cache.clear()
 
-    def test_healthchecks_validations(self, mock_requests):
-        """
-        Test that healthchecks validations work
-        """
-        app_id = self.create_app()
-
-        # Set one of the values that require a numeric value to a string
-        response = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_INITIAL_DELAY': 'horse'})}
-        )
-        self.assertEqual(response.status_code, 400, response.data)
-
-        # test URL - Path is the only allowed thing
-        # Try setting various things such as query param
-
-        # query param
-        response = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': '/health?testing=0'})}
-        )
-        self.assertEqual(response.status_code, 400, response.data)
-
-        # fragment
-        response = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': '/health#db'})}
-        )
-        self.assertEqual(response.status_code, 400, response.data)
-
-        # netloc
-        response = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': 'http://someurl.com/health/'})}
-        )
-        self.assertEqual(response.status_code, 400, response.data)
-
-        # no path
-        response = self.client.post(
-            '/v2/apps/{app_id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': 'http://someurl.com'})}
-        )
-        self.assertEqual(response.status_code, 400, response.data)
-
     def test_config_healthchecks(self, mock_requests):
         """
         Test that healthchecks can be applied
@@ -171,85 +126,3 @@ def test_config_healthchecks_validations(self, mock_requests):
                                         {'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': {'web': {'livenessProbe':
-                             {'httpGet': {'path': '/', 'port': 5000},
-                              'successThreshold': 5}}}}
-        )
-        self.assertEqual(response.status_code, 400, response.data)
-
-    def test_config_healthchecks_legacy(self, mock_requests):
-        """
-        Test that when a user uses `drycc config:set HEALTHCHECK_URL=/`, the config
-        object is rolled over to the `healthcheck` field.
-        """
-        app_id = self.create_app()
-        app = App.objects.get(id=app_id)
-
-        # Set healthcheck URL to get defaults set
-        response = self.client.post(
-            '/v2/apps/{app.id}/config'.format(**locals()),
-            {'values': json.dumps({'HEALTHCHECK_URL': '/health'})}
-        )
-        self.assertEqual(response.status_code, 201, response.data)
-        # this gets migrated to the new healtcheck format
-        self.assertNotIn('HEALTHCHECK_URL', response.data['values'])
-        # legacy defaults
-        expected = {'web': {
-            'livenessProbe': {
-                'initialDelaySeconds': 50,
-                'timeoutSeconds': 50,
-                'periodSeconds': 10,
-                'successThreshold': 1,
-                'failureThreshold': 3,
-                'httpGet': {
-                    'path': '/health'
-                }
-            },
-            'readinessProbe': {
-                'initialDelaySeconds': 50,
-                'timeoutSeconds': 50,
-                'periodSeconds': 10,
-                'successThreshold': 1,
-                'failureThreshold': 3,
-                'httpGet': {
-                    'path': '/health'
-                }
-            }
-            }
-        }
-        actual = app.config_set.latest().healthcheck
-        self.assertEqual(actual, expected)
-        # Now set all the envvars and check to make sure they are written properly
-        response = self.client.post(
-            '/v2/apps/{app.id}/config'.format(**locals()),
-            {
-                'values': json.dumps({
-                    'HEALTHCHECK_URL': '/health',
-                    'HEALTHCHECK_INITIAL_DELAY': '25',
-                    'HEALTHCHECK_TIMEOUT': '10',
-                    'HEALTHCHECK_PERIOD_SECONDS': '5',
-                    'HEALTHCHECK_SUCCESS_THRESHOLD': '2',
-                    'HEALTHCHECK_FAILURE_THRESHOLD': '2'})
-            }
-        )
-        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['web']['livenessProbe'] = {
-            'initialDelaySeconds': 25,
-            'timeoutSeconds': 10,
-            'periodSeconds': 5,
-            'successThreshold': 2,
-            'failureThreshold': 2,
-            'httpGet': {
-                'path': '/health'
-            }
-        }
-        expected['web']['readinessProbe'] = expected['web']['livenessProbe']
-        actual = app.config_set.latest().healthcheck
-        self.assertEqual(expected, actual)
diff --git a/rootfs/requirements.txt b/rootfs/requirements.txt
@@ -11,7 +11,6 @@ gunicorn==20.1.0
 uvicorn[standard]==0.22.0
 asgiref==3.7.2
 idna==3.4
-jmespath==1.0.1
 jsonschema==4.18.0
 morph==0.1.5
 packaging==23.1
diff --git a/rootfs/scheduler/resources/pod.py b/rootfs/scheduler/resources/pod.py
@@ -295,8 +295,6 @@ def _get_memory(size):
     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 drycc config:set HEALTHCHECK_URL
             if (
                 healthchecks.get('livenessProbe') is not None and
                 healthchecks['livenessProbe'].get('httpGet') is not None and
@@ -348,7 +346,6 @@ def _default_container_readiness_probe(port, delay=5, timeout=5, period_seconds=
         """
         readinessprobe = {
             'readinessProbe': {
-                # an exec probe
                 'tcpSocket': {
                     "port": int(port)
                 },
