Added PDB parameter for maxUnavailable (#237)

* Added PDB parameter for maxUnavailable

* Fixing codestyle

* Add check for max replicas

* Adding feedback

docs/operator_guide.md

@@ -207,6 +207,10 @@ When `include-status-in-app` enabled, the `status` subresource of Application re
 
 This feature is disabled by default.
 
+### pdb-max-unavailable
+
+By default, PDB will be set with maxUnavailable = 1 in case the deployment has more than 1 replica. This parameter allows to change that value either with another int value or with a string percentage (i.e. "20%"). Recommendation is some low value like 10% to avoid issues. Also in case of a number, something below min_replicas should be used to allow evictions.
+
 Deploying an application
 ------------------------
 

fiaas_deploy_daemon/config.py

@@ -306,6 +306,12 @@ def _parse_args(self, args):
             "--include-status-in-app", help="Include status subresource in application CRD", default=False,
             action="store_true"
         )
+        parser.add_argument(
+            "--pdb-max-unavailable",
+            help="The maximum number of pods that can be unavailable after an eviction",
+            default="1",
+            type=_int_or_unicode
+        )
         usage_reporting_parser = parser.add_argument_group("Usage Reporting", USAGE_REPORTING_LONG_HELP)
         usage_reporting_parser.add_argument(
             "--usage-reporting-cluster-name", help="Name of the cluster where the fiaas-deploy-daemon instance resides"

fiaas_deploy_daemon/deployer/kubernetes/pod_disruption_budget.py

@@ -30,9 +30,10 @@
 
 
 class PodDisruptionBudgetDeployer(object):
-    def __init__(self, owner_references, extension_hook):
+    def __init__(self, owner_references, extension_hook, config):
         self._owner_references = owner_references
         self._extension_hook = extension_hook
+        self.max_unavailable = config.pdb_max_unavailable
 
     @retry_on_upsert_conflict
     def deploy(self, app_spec: AppSpec, selector: dict[str, any], labels: dict[str, any]):
@@ -50,9 +51,11 @@ def deploy(self, app_spec: AppSpec, selector: dict[str, any], labels: dict[str,
             annotations=app_spec.annotations.pod_disruption_budget
         )
 
-        # Conservative default to ensure that we don't block evictions.
-        # See https://github.com/fiaas/fiaas-deploy-daemon/issues/220 for discussion.
-        max_unavailable = 1
+        max_unavailable = self.max_unavailable
+        if isinstance(max_unavailable, int):
+            if max_unavailable >= app_spec.autoscaler.min_replicas:
+                max_unavailable = 1
+
         spec = PodDisruptionBudgetSpec(
             selector=LabelSelector(matchLabels=selector),
             maxUnavailable=max_unavailable

tests/fiaas_deploy_daemon/deployer/kubernetes/test_pod_disruption_budget_deploy.py

@@ -21,6 +21,7 @@
 from utils import TypeMatcher
 
 from fiaas_deploy_daemon import ExtensionHookCaller
+from fiaas_deploy_daemon.config import Configuration
 from fiaas_deploy_daemon.deployer.kubernetes.pod_disruption_budget import PodDisruptionBudgetDeployer
 from fiaas_deploy_daemon.specs.models import (
     AutoscalerSpec,
@@ -37,8 +38,14 @@ def extension_hook(self):
         return create_autospec(ExtensionHookCaller, spec_set=True, instance=True)
 
     @pytest.fixture
-    def deployer(self, owner_references, extension_hook):
-        return PodDisruptionBudgetDeployer(owner_references, extension_hook)
+    def deployer(self, owner_references, extension_hook, config):
+        return PodDisruptionBudgetDeployer(owner_references, extension_hook, config)
+
+    @pytest.fixture
+    def config(self):
+        config = create_autospec(Configuration([]), spec_set=True)
+        config.pdb_max_unavailable = 1
+        return config
 
     @pytest.mark.usefixtures("get")
     def test_new_pdb(self, deployer, post, app_spec, owner_references, extension_hook):
@@ -74,3 +81,57 @@ def test_no_pdb_gives_no_put(self, deployer, delete, put, app_spec):
         deployer.deploy(app_spec, SELECTOR, LABELS)
         delete.assert_called_with(PDB_API + app_spec.name)
         pytest.helpers.assert_no_calls(put)
+
+    @pytest.mark.usefixtures("get")
+    def test_setting_max_int(self, deployer, post, app_spec, owner_references, extension_hook):
+        app_spec = app_spec._replace(
+            autoscaler=AutoscalerSpec(enabled=True, min_replicas=4, max_replicas=6, cpu_threshold_percentage=50)
+        )
+        deployer.max_unavailable = 2
+        expected_pdb = {
+            "metadata": pytest.helpers.create_metadata("testapp", labels=LABELS),
+            "spec": {
+                "maxUnavailable": 2,
+                "selector": {"matchExpressions": [], "matchLabels": {"app": "testapp"}},
+            },
+        }
+        mock_response = create_autospec(Response)
+        mock_response.json.return_value = expected_pdb
+        post.return_value = mock_response
+
+        deployer.deploy(app_spec, SELECTOR, LABELS)
+
+        pytest.helpers.assert_any_call(post, PDB_API, expected_pdb)
+        owner_references.apply.assert_called_once_with(TypeMatcher(PodDisruptionBudget), app_spec)
+        extension_hook.apply.assert_called_once_with(TypeMatcher(PodDisruptionBudget), app_spec)
+
+    def test_no_pdb_max_over_min(self, deployer: PodDisruptionBudgetDeployer, delete, post, app_spec):
+        app_spec = app_spec._replace(
+            autoscaler=AutoscalerSpec(enabled=True, min_replicas=1, max_replicas=4, cpu_threshold_percentage=50)
+        )
+        deployer.deploy(app_spec, SELECTOR, LABELS)
+        delete.assert_called_with(PDB_API + app_spec.name)
+        pytest.helpers.assert_no_calls(post)
+
+    @pytest.mark.usefixtures("get")
+    def test_setting_max_str(self, deployer, post, app_spec, owner_references, extension_hook):
+        app_spec = app_spec._replace(
+            autoscaler=AutoscalerSpec(enabled=True, min_replicas=4, max_replicas=6, cpu_threshold_percentage=50)
+        )
+        deployer.max_unavailable = "20%"
+        expected_pdb = {
+            "metadata": pytest.helpers.create_metadata("testapp", labels=LABELS),
+            "spec": {
+                "maxUnavailable": "20%",
+                "selector": {"matchExpressions": [], "matchLabels": {"app": "testapp"}},
+            },
+        }
+        mock_response = create_autospec(Response)
+        mock_response.json.return_value = expected_pdb
+        post.return_value = mock_response
+
+        deployer.deploy(app_spec, SELECTOR, LABELS)
+
+        pytest.helpers.assert_any_call(post, PDB_API, expected_pdb)
+        owner_references.apply.assert_called_once_with(TypeMatcher(PodDisruptionBudget), app_spec)
+        extension_hook.apply.assert_called_once_with(TypeMatcher(PodDisruptionBudget), app_spec)