Merge pull request #3647 from Yelp/COMPINFRA-2833

make sidecar resource limit configurable
Yelp · Aug 3, 2023 · adb2dd8 · adb2dd8
2 parents 7970984 + 3546a24
commit adb2dd8
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 26 deletions.
diff --git a/.gitignore b/.gitignore
@@ -52,3 +52,5 @@ unique-run
 
 # Coverage artifacts
 .coverage
+
+unique-run
diff --git a/paasta_tools/kubernetes_tools.py b/paasta_tools/kubernetes_tools.py
@@ -148,6 +148,7 @@
 from paasta_tools.utils import DockerVolume
 from paasta_tools.utils import get_config_hash
 from paasta_tools.utils import get_git_sha_from_dockerurl
+from paasta_tools.utils import KubeContainerResourceRequest
 from paasta_tools.utils import load_service_instance_config
 from paasta_tools.utils import load_system_paasta_config
 from paasta_tools.utils import load_v2_deployments_json
@@ -201,6 +202,11 @@
 DEFAULT_USE_PROMETHEUS_CPU = False
 DEFAULT_USE_PROMETHEUS_UWSGI = True
 DEFAULT_USE_RESOURCE_METRICS_CPU = True
+DEFAULT_SIDECAR_REQUEST: KubeContainerResourceRequest = {
+    "cpu": 0.1,
+    "memory": "1024Mi",
+    "ephemeral-storage": "256Mi",
+}
 
 
 # conditions is None when creating a new HPA, but the client raises an error in that case.
@@ -290,17 +296,6 @@ def _set_disrupted_pods(self: Any, disrupted_pods: Mapping[str, datetime]) -> No
     self._disrupted_pods = disrupted_pods
 
 
-KubeContainerResourceRequest = TypedDict(
-    "KubeContainerResourceRequest",
-    {
-        "cpu": float,
-        "memory": str,
-        "ephemeral-storage": str,
-    },
-    total=False,
-)
-
-
 SidecarResourceRequirements = TypedDict(
     "SidecarResourceRequirements",
     {
@@ -1055,7 +1050,10 @@ def get_hacheck_sidecar_container(
                         )
                     )
                 ),
-                resources=self.get_sidecar_resource_requirements("hacheck"),
+                resources=self.get_sidecar_resource_requirements(
+                    "hacheck",
+                    system_paasta_config,
+                ),
                 name=HACHECK_POD_NAME,
                 env=self.get_kubernetes_environment() + [hacheck_registrations_env],
                 ports=[V1ContainerPort(container_port=6666)],
@@ -1082,7 +1080,10 @@ def get_uwsgi_exporter_sidecar_container(
 
             return V1Container(
                 image=system_paasta_config.get_uwsgi_exporter_sidecar_image_url(),
-                resources=self.get_sidecar_resource_requirements("uwsgi_exporter"),
+                resources=self.get_sidecar_resource_requirements(
+                    "uwsgi_exporter",
+                    system_paasta_config,
+                ),
                 name=UWSGI_EXPORTER_POD_NAME,
                 env=self.get_kubernetes_environment() + [stats_port_env],
                 ports=[V1ContainerPort(container_port=9117)],
@@ -1126,7 +1127,9 @@ def get_gunicorn_exporter_sidecar_container(
         if self.should_run_gunicorn_exporter_sidecar():
             return V1Container(
                 image=system_paasta_config.get_gunicorn_exporter_sidecar_image_url(),
-                resources=self.get_sidecar_resource_requirements("gunicorn_exporter"),
+                resources=self.get_sidecar_resource_requirements(
+                    "gunicorn_exporter", system_paasta_config
+                ),
                 name=GUNICORN_EXPORTER_POD_NAME,
                 env=self.get_kubernetes_environment(),
                 ports=[V1ContainerPort(container_port=9117)],
@@ -1306,15 +1309,36 @@ def get_resource_requirements(self) -> V1ResourceRequirements:
         return V1ResourceRequirements(limits=limits, requests=requests)
 
     def get_sidecar_resource_requirements(
-        self, sidecar_name: str
+        self,
+        sidecar_name: str,
+        system_paasta_config: SystemPaastaConfig,
     ) -> V1ResourceRequirements:
+        """
+        Sidecar request/limits are set with varying levels of priority, with
+        elements further down the list taking precedence:
+        * hard-coded paasta default
+        * SystemPaastaConfig
+        * per-service soaconfig overrides
+
+        Additionally, for the time being we do not expose a way to set
+        limits separately from requests - these values will always mirror
+        each other
+
+        NOTE: changing any of these will cause a bounce of all services that
+        run the sidecars affected by the resource change
+        """
         config = self.config_dict.get("sidecar_resource_requirements", {}).get(
             sidecar_name, {}
         )
+        sidecar_requirements_config = (
+            system_paasta_config.get_sidecar_requirements_config().get(
+                sidecar_name, DEFAULT_SIDECAR_REQUEST
+            )
+        )
         requests: KubeContainerResourceRequest = {
-            "cpu": 0.1,
-            "memory": "1024Mi",
-            "ephemeral-storage": "256Mi",
+            "cpu": sidecar_requirements_config.get("cpu"),
+            "memory": sidecar_requirements_config.get("memory"),
+            "ephemeral-storage": sidecar_requirements_config.get("ephemeral-storage"),
         }
         requests.update(config.get("requests", {}))
 

diff --git a/paasta_tools/utils.py b/paasta_tools/utils.py
@@ -349,6 +349,17 @@ class DockerParameter(TypedDict):
     value: str
 
 
+KubeContainerResourceRequest = TypedDict(
+    "KubeContainerResourceRequest",
+    {
+        "cpu": float,
+        "memory": str,
+        "ephemeral-storage": str,
+    },
+    total=False,
+)
+
+
 def safe_deploy_blacklist(input: UnsafeDeployBlacklist) -> DeployBlacklist:
     return [(t, l) for t, l in input]
 
@@ -2017,6 +2028,7 @@ class SystemPaastaConfigDict(TypedDict, total=False):
     spark_kubeconfig: str
     kube_clusters: Dict
     spark_use_eks_default: bool
+    sidecar_requirements_config: Dict[str, KubeContainerResourceRequest]
     eks_cluster_aliases: Dict[str, str]
 
 
@@ -2097,6 +2109,11 @@ def __repr__(self) -> str:
     def get_spark_use_eks_default(self) -> bool:
         return self.config_dict.get("spark_use_eks_default", False)
 
+    def get_sidecar_requirements_config(
+        self,
+    ) -> Dict[str, KubeContainerResourceRequest]:
+        return self.config_dict.get("sidecar_requirements_config", {})
+
     def get_tron_default_pool_override(self) -> str:
         """Get the default pool override variable defined in this host's cluster config file.
 

diff --git a/tests/test_kubernetes_tools.py b/tests/test_kubernetes_tools.py
@@ -516,6 +516,21 @@ def test_get_sidecar_containers(self):
             "paasta_tools.kubernetes_tools.KubernetesDeploymentConfig.get_readiness_check_script",
             autospec=True,
             return_value=["/nail/blah.sh"],
+        ), mock.patch(
+            "paasta_tools.kubernetes_tools.KubernetesDeploymentConfig.get_sidecar_resource_requirements",
+            autospec=True,
+            return_value={
+                "limits": {
+                    "cpu": 0.1,
+                    "memory": "1024Mi",
+                    "ephemeral-storage": "256Mi",
+                },
+                "requests": {
+                    "cpu": 0.1,
+                    "memory": "1024Mi",
+                    "ephemeral-storage": "256Mi",
+                },
+            },
         ):
             mock_system_config = mock.Mock(
                 get_hacheck_sidecar_image_url=mock.Mock(
@@ -656,9 +671,10 @@ def test_get_sidecar_resource_requirements(self):
                 },
             }
         }
+        system_paasta_config = mock.Mock()
 
         assert self.deployment.get_sidecar_resource_requirements(
-            "hacheck"
+            "hacheck", system_paasta_config
         ) == V1ResourceRequirements(
             limits={"cpu": 0.3, "memory": "1025Mi", "ephemeral-storage": "257Mi"},
             requests={"cpu": 0.2, "memory": "1024Mi", "ephemeral-storage": "256Mi"},
@@ -675,9 +691,9 @@ def test_get_sidecar_resource_requirements_default_limits(self):
                 },
             }
         }
-
+        system_paasta_config = mock.Mock()
         assert self.deployment.get_sidecar_resource_requirements(
-            "hacheck"
+            "hacheck", system_paasta_config
         ) == V1ResourceRequirements(
             limits={"cpu": 0.2, "memory": "1025Mi", "ephemeral-storage": "257Mi"},
             requests={"cpu": 0.2, "memory": "1025Mi", "ephemeral-storage": "257Mi"},
@@ -690,11 +706,27 @@ def test_get_sidecar_resource_requirements_default_requirements(self):
         except KeyError:
             pass
 
+        system_paasta_config = mock.Mock(
+            get_sidecar_requirements_config=mock.Mock(
+                return_value={
+                    "hacheck": {
+                        "cpu": 0.1,
+                        "memory": "512Mi",
+                        "ephemeral-storage": "256Mi",
+                    },
+                    "uwsgi_exporter": {
+                        "cpu": 0.1,
+                        "memory": "512Mi",
+                        "ephemeral-storage": "256Mi",
+                    },
+                }
+            )
+        )
         assert self.deployment.get_sidecar_resource_requirements(
-            "hacheck"
+            "hacheck", system_paasta_config
         ) == V1ResourceRequirements(
-            limits={"cpu": 0.1, "memory": "1024Mi", "ephemeral-storage": "256Mi"},
-            requests={"cpu": 0.1, "memory": "1024Mi", "ephemeral-storage": "256Mi"},
+            limits={"cpu": 0.1, "memory": "512Mi", "ephemeral-storage": "256Mi"},
+            requests={"cpu": 0.1, "memory": "512Mi", "ephemeral-storage": "256Mi"},
         )
 
     def test_get_sidecar_resource_requirements_limits_override_default_requirements(
@@ -706,9 +738,24 @@ def test_get_sidecar_resource_requirements_limits_override_default_requirements(
                 "limits": {"cpu": 1.0},
             }
         }
-
+        system_paasta_config = mock.Mock(
+            get_sidecar_requirements_config=mock.Mock(
+                return_value={
+                    "hacheck": {
+                        "cpu": 0.1,
+                        "memory": "1024Mi",
+                        "ephemeral-storage": "256Mi",
+                    },
+                    "uwsgi_exporter": {
+                        "cpu": 0.1,
+                        "memory": "1024Mi",
+                        "ephemeral-storage": "256Mi",
+                    },
+                }
+            )
+        )
         assert self.deployment.get_sidecar_resource_requirements(
-            "hacheck"
+            "hacheck", system_paasta_config
         ) == V1ResourceRequirements(
             limits={"cpu": 1.0, "memory": "1024Mi", "ephemeral-storage": "256Mi"},
             requests={"cpu": 0.1, "memory": "1024Mi", "ephemeral-storage": "256Mi"},