feat(limits-cmd): accept new limits:set value type

Accept new cli format limits:set web=500m/2000m
Value is map to requests/limits parameter in Kubernetes resource management.
See workflow#562

Author: zinuzoid

rootfs/api/serializers.py

@@ -19,8 +19,10 @@
 # proc type name is alphanumeric
 # https://docs-v2.readthedocs.io/en/latest/using-workflow/process-types-and-the-procfile/#declaring-process-types
 PROCTYPE_MATCH = re.compile(r'^(?P<type>[a-z0-9]+)$')
-MEMLIMIT_MATCH = re.compile(r'^(?P<mem>[0-9]+(MB|KB|GB|[BKMG]))$', re.IGNORECASE)
-CPUSHARE_MATCH = re.compile(r'^(?P<cpu>[-+]?[0-9]*\.?[0-9]+[m]{0,1})$')
+MEMLIMIT_MATCH = re.compile(
+    r'^(?P<mem>(([0-9]+(MB|KB|GB|[BKMG])|0)(/([0-9]+(MB|KB|GB|[BKMG])))?))$', re.IGNORECASE)
+CPUSHARE_MATCH = re.compile(
+    r'^(?P<cpu>(([-+]?[0-9]*\.?[0-9]+[m]?)(/([-+]?[0-9]*\.?[0-9]+[m]?))?))$')
 TAGVAL_MATCH = re.compile(r'^(?:[a-zA-Z\d][-\.\w]{0,61})?[a-zA-Z\d]$')
 CONFIGKEY_MATCH = re.compile(r'^[a-z_]+[a-z0-9_]*$', re.IGNORECASE)
 PROBE_SCHEMA = {
@@ -273,7 +275,8 @@ def validate_memory(self, data):
 
             if not re.match(MEMLIMIT_MATCH, str(value)):
                 raise serializers.ValidationError(
-                    "Limit format: <number><unit>, where unit = B, K, M or G")
+                    "Memory limit format: <number><unit> or <number><unit>/<number><unit>, "
+                    "where unit = B, K, M or G")
 
         return data
 
@@ -287,7 +290,8 @@ def validate_cpu(self, data):
 
             shares = re.match(CPUSHARE_MATCH, str(value))
             if not shares:
-                raise serializers.ValidationError("CPU shares must be a numeric value")
+                raise serializers.ValidationError(
+                    "CPU limit format: <value> or <value>/<value>, where value must be a numeric")
 
         return data
 

rootfs/api/tests/test_config.py

@@ -169,7 +169,9 @@ def test_response_data_types_converted(self, mock_requests):
         body = {'cpu': json.dumps({'web': 'this will fail'})}
         response = self.client.post(url, body)
         self.assertEqual(response.status_code, 400, response.data)
-        self.assertIn('CPU shares must be a numeric value', response.data['cpu'])
+        self.assertIn(
+            'CPU limit format: <value> or <value>/<value>, where value must be a numeric',
+            response.data['cpu'])
 
     def test_config_set_same_key(self, mock_requests):
         """

rootfs/api/tests/test_limits.py

@@ -6,6 +6,7 @@
 from rest_framework.authtoken.models import Token
 
 from api.serializers import MEMLIMIT_MATCH
+from api.serializers import CPUSHARE_MATCH
 from api.tests import adapter, DeisTransactionTestCase
 
 
@@ -26,12 +27,31 @@ def tearDown(self):
 
     def test_memlimit_regex(self, mock_requests):
         """Tests the regex for unit format used by "deis limits:set --memory=<limit>"."""
+        self.assertTrue(MEMLIMIT_MATCH.match("0/100MB"))
+        self.assertTrue(MEMLIMIT_MATCH.match("200GB/100MB"))
         self.assertTrue(MEMLIMIT_MATCH.match("20MB"))
-        self.assertFalse(MEMLIMIT_MATCH.match("20MK"))
         self.assertTrue(MEMLIMIT_MATCH.match("20gb"))
+        self.assertTrue(MEMLIMIT_MATCH.match("0m"))
+        self.assertFalse(MEMLIMIT_MATCH.match("20MK"))
+        self.assertFalse(MEMLIMIT_MATCH.match("10"))
         self.assertFalse(MEMLIMIT_MATCH.match("20gK"))
+        self.assertFalse(MEMLIMIT_MATCH.match("mb"))
 
-    def test_limit_memory(self, mock_requests):
+    def test_cpushare_regex(self, mock_requests):
+        """Tests the regex for unit format used by "deis limits:set --cpu=<limit>"."""
+        self.assertTrue(CPUSHARE_MATCH.match("0/2"))
+        self.assertTrue(CPUSHARE_MATCH.match("500m/600m"))
+        self.assertTrue(CPUSHARE_MATCH.match("0.5"))
+        self.assertTrue(CPUSHARE_MATCH.match(".123"))
+        self.assertTrue(CPUSHARE_MATCH.match("1.123"))
+        self.assertTrue(CPUSHARE_MATCH.match("200m"))
+        self.assertTrue(CPUSHARE_MATCH.match("0"))
+        self.assertFalse(CPUSHARE_MATCH.match("20MK"))
+        self.assertFalse(CPUSHARE_MATCH.match("20gK"))
+        self.assertFalse(CPUSHARE_MATCH.match("m"))
+        self.assertFalse(CPUSHARE_MATCH.match("."))
+
+    def test_request_limit_memory(self, mock_requests):
         """
         Test that limit is auto-created for a new app and that
         limits can be updated using a PATCH
@@ -101,6 +121,38 @@ def test_limit_memory(self, mock_requests):
         self.assertIn('web', memory)
         self.assertEqual(memory['web'], '1G')
 
+        # add with requests/limits
+        body = {'memory': json.dumps({'db': '1G/2G'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+
+        # read the limit again
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200, response.data)
+        memory = response.data['memory']
+        self.assertIn('worker', memory)
+        self.assertEqual(memory['worker'], '512M')
+        self.assertIn('web', memory)
+        self.assertEqual(memory['web'], '1G')
+        self.assertIn('db', memory)
+        self.assertEqual(memory['db'], '1G/2G')
+
+        # replace one with requests/limits
+        body = {'memory': json.dumps({'web': '3G/4G'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+
+        # read the limit again
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200, response.data)
+        memory = response.data['memory']
+        self.assertIn('worker', memory)
+        self.assertEqual(memory['worker'], '512M')
+        self.assertIn('web', memory)
+        self.assertEqual(memory['web'], '3G/4G')
+        self.assertIn('db', memory)
+        self.assertEqual(memory['db'], '1G/2G')
+
         # unset a value
         body = {'memory': json.dumps({'worker': None})}
         response = self.client.post(url, body)
@@ -129,9 +181,9 @@ def test_limit_memory(self, mock_requests):
         self.assertEqual(response.status_code, 405, response.data)
         return limit4
 
-    def test_limit_cpu(self, mock_requests):
+    def test_request_limit_cpu(self, mock_requests):
         """
-        Test that CPU limits can be set
+        Test that CPU requests/limits can be set
         """
         app_id = self.create_app()
         url = '/v2/apps/{app_id}/config'.format(**locals())
@@ -150,7 +202,7 @@ def test_limit_cpu(self, mock_requests):
         self.assertEqual(response.status_code, 201, response.data)
         limit1 = response.data
 
-        # check memory limits
+        # check cpu limits
         response = self.client.get(url)
         self.assertEqual(response.status_code, 200, response.data)
         self.assertIn('cpu', response.data)
@@ -181,6 +233,38 @@ def test_limit_cpu(self, mock_requests):
         self.assertIn('web', cpu)
         self.assertEqual(cpu['web'], '1024')
 
+        # add with requests/limits
+        body = {'cpu': json.dumps({'db': '1/2'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+
+        # read the limit again
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200, response.data)
+        cpu = response.data['cpu']
+        self.assertIn('worker', cpu)
+        self.assertEqual(cpu['worker'], '512m')
+        self.assertIn('web', cpu)
+        self.assertEqual(cpu['web'], '1024')
+        self.assertIn('db', cpu)
+        self.assertEqual(cpu['db'], '1/2')
+
+        # replace one with requests/limits
+        body = {'cpu': json.dumps({'web': '300m/4000m'})}
+        response = self.client.post(url, body)
+        self.assertEqual(response.status_code, 201, response.data)
+
+        # read the limit again
+        response = self.client.get(url)
+        self.assertEqual(response.status_code, 200, response.data)
+        cpu = response.data['cpu']
+        self.assertIn('worker', cpu)
+        self.assertEqual(cpu['worker'], '512m')
+        self.assertIn('web', cpu)
+        self.assertEqual(cpu['web'], '300m/4000m')
+        self.assertIn('db', cpu)
+        self.assertEqual(cpu['db'], '1/2')
+
         # unset a value
         body = {'cpu': json.dumps({'worker': None})}
         response = self.client.post(url, body)

rootfs/scheduler/resources/pod.py

@@ -1,3 +1,4 @@
+from collections import defaultdict
 from datetime import datetime, timedelta
 import operator
 import os
@@ -172,9 +173,6 @@ def manifest(self, namespace, name, image, **kwargs):
 
     def _set_container(self, namespace, container_name, data, **kwargs):
         """Set app container information (env, healthcheck, etc) on a Pod"""
-        app_type = kwargs.get('app_type')
-        mem = kwargs.get('memory', {}).get(app_type)
-        cpu = kwargs.get('cpu', {}).get(app_type)
         env = kwargs.get('envs', {})
 
         # container name
@@ -222,23 +220,48 @@ def _set_container(self, namespace, container_name, data, **kwargs):
         # list sorted by dict key name
         data['env'].sort(key=operator.itemgetter('name'))
 
+        self._set_resources(data, kwargs)
+
+        self._set_health_checks(data, env, **kwargs)
+
+    def _set_resources(self, container, kwargs):
+        """ Set CPU/memory resource management manifest """
+        app_type = kwargs.get("app_type")
+        mem = kwargs.get("memory", {}).get(app_type)
+        cpu = kwargs.get("cpu", {}).get(app_type)
+
         if mem or cpu:
-            data["resources"] = {"limits": {}}
+            resources = defaultdict(dict)
+
+            if mem:
+                if "/" in mem:
+                    parts = mem.split("/")
+                    resources["requests"]["memory"] = self._format_memory(parts[0])
+                    resources["limits"]["memory"] = self._format_memory(parts[1])
+                else:
+                    resources["limits"]["memory"] = self._format_memory(mem)
+
+            if cpu:
+                # CPU needs to be defined as lower case
+                if "/" in cpu:
+                    parts = cpu.split("/")
+                    resources["requests"]["cpu"] = parts[0].lower()
+                    resources["limits"]["cpu"] = parts[1].lower()
+                else:
+                    resources["limits"]["cpu"] = cpu.lower()
+
+            if resources:
+                container["resources"] = dict(resources)
 
-        if mem:
-            if mem[-2:-1].isalpha() and mem[-1].isalpha():
-                mem = mem[:-1]
+    def _format_memory(self, mem):
+        """ Format memory limit value """
+        if mem[-2:-1].isalpha() and mem[-1].isalpha():
+            mem = mem[:-1]
 
+        if mem[-1].isalpha():
             # memory needs to be upper cased (only first char)
             mem = mem.upper() + "i"
-            data["resources"]["limits"]["memory"] = mem
-
-        if cpu:
-            # CPU needs to be defined as lower case
-            data["resources"]["limits"]["cpu"] = cpu.lower()
-
-        # add in healthchecks
-        self._set_health_checks(data, env, **kwargs)
+        return mem
 
     def _set_health_checks(self, container, env, **kwargs):
         healthchecks = kwargs.get('healthcheck', None)

rootfs/scheduler/tests/test_pod_resources.py

@@ -0,0 +1,55 @@
+import unittest
+from scheduler.resources.pod import Pod
+
+
+class TestSchedulerPodResources(unittest.TestCase):
+    def test_manifest_limits(self):
+        cpu_cases = [
+            {"app_type": "web", "cpu": {"cmd": "2"},
+             "expected": None},
+            {"app_type": "web", "cpu": {"web": "2"},
+             "expected": {"limits": {"cpu": "2"}}},
+            {"app_type": "web", "cpu": {"web": "0/3"},
+             "expected": {"requests": {"cpu": "0"}, "limits": {"cpu": "3"}}},
+            {"app_type": "web", "cpu": {"web": "4/5"},
+             "expected": {"requests": {"cpu": "4"}, "limits": {"cpu": "5"}}},
+            {"app_type": "web", "cpu": {"web": "400m/500m"},
+             "expected": {"requests": {"cpu": "400m"}, "limits": {"cpu": "500m"}}},
+            {"app_type": "web", "cpu": {"web": "0.6/0.7"},
+             "expected": {"requests": {"cpu": "0.6"}, "limits": {"cpu": "0.7"}}},
+        ]
+
+        mem_cases = [
+            {"app_type": "web", "memory": {"cmd": "2G"},
+             "expected": None},
+            {"app_type": "web", "memory": {"web": "200M"},
+             "expected": {"limits": {"memory": "200Mi"}}},
+            {"app_type": "web", "memory": {"web": "0/3G"},
+             "expected": {"requests": {"memory": "0"}, "limits": {"memory": "3Gi"}}},
+            {"app_type": "web", "memory": {"web": "400M/500MB"},
+             "expected": {"requests": {"memory": "400Mi"}, "limits": {"memory": "500Mi"}}},
+        ]
+
+        for caze in cpu_cases:
+            manifest = Pod("").manifest("",
+                                        "",
+                                        "",
+                                        app_type=caze["app_type"],
+                                        cpu=caze["cpu"])
+            self._assert_resources(caze, manifest)
+
+        for caze in mem_cases:
+            manifest = Pod("").manifest("",
+                                        "",
+                                        "",
+                                        app_type=caze["app_type"],
+                                        memory=caze["memory"])
+            self._assert_resources(caze, manifest)
+
+    def _assert_resources(self, caze, manifest):
+        resources_parent = manifest["spec"]["containers"][0]
+        expected = caze["expected"]
+        if expected:
+            self.assertEqual(resources_parent["resources"], expected, caze)
+        else:
+            self.assertTrue("resources" not in resources_parent, caze)