Skip to content

Commit

Permalink
Merge pull request #3746 from Yelp/jfong/PAASTA-18122
Browse files Browse the repository at this point in the history 
Stop deleting brutal deployments
  • Loading branch information
@jfongatyelp
jfongatyelp authored Dec 14, 2023
2 parents 8abffa4 + 2699d41 commit f33a07b
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 143 deletions.
60 changes: 0 additions & 60 deletions paasta_tools/kubernetes/application/controller_wrappers.py
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import logging
import threading
from abc import ABC
from abc import abstractmethod
from time import sleep
from typing import Optional
from typing import Union

Expand All @@ -17,11 +15,9 @@
from paasta_tools.kubernetes_tools import create_deployment
from paasta_tools.kubernetes_tools import create_pod_disruption_budget
from paasta_tools.kubernetes_tools import create_stateful_set
from paasta_tools.kubernetes_tools import force_delete_pods
from paasta_tools.kubernetes_tools import KubeClient
from paasta_tools.kubernetes_tools import KubeDeployment
from paasta_tools.kubernetes_tools import KubernetesDeploymentConfig
from paasta_tools.kubernetes_tools import list_all_deployments
from paasta_tools.kubernetes_tools import load_kubernetes_service_config_no_cache
from paasta_tools.kubernetes_tools import paasta_prefixed
from paasta_tools.kubernetes_tools import pod_disruption_budget_for_service_instance
Expand Down Expand Up @@ -248,65 +244,9 @@ def create(self, kube_client: KubeClient) -> None:
self.ensure_pod_disruption_budget(kube_client, self.soa_config.get_namespace())
self.sync_horizontal_pod_autoscaler(kube_client)

def deep_delete_and_create(self, kube_client: KubeClient) -> None:
self.deep_delete(kube_client)
timer = 0
while (
self.kube_deployment
in set(list_all_deployments(kube_client, self.soa_config.get_namespace()))
and timer < 60
):
sleep(1)
timer += 1

if timer >= 60 and self.kube_deployment in set(
list_all_deployments(kube_client, self.soa_config.get_namespace())
):
# When deleting then immediately creating, we need to use Background
# deletion to ensure we can create the deployment immediately
self.deep_delete(kube_client, propagation_policy="Background")

try:
force_delete_pods(
self.item.metadata.name,
self.kube_deployment.service,
self.kube_deployment.instance,
self.item.metadata.namespace,
kube_client,
)
except ApiException as e:
if e.status == 404:
# Pod(s) may have been deleted by GC before we got to it
# We can consider this a success
self.logging.debug(
"pods already deleted for {} from namespace/{}. Continuing.".format(
self.kube_deployment.service, self.item.metadata.namespace
)
)
else:
raise

if self.kube_deployment in set(
list_all_deployments(kube_client, self.soa_config.get_namespace())
):
# deployment deletion failed, we cannot continue
raise Exception(f"Could not delete deployment {self.item.metadata.name}")
else:
self.logging.info(
"deleted deploy/{} from namespace/{}".format(
self.kube_deployment.service, self.item.metadata.namespace
)
)
self.create(kube_client=kube_client)

def update(self, kube_client: KubeClient) -> None:
# If HPA is enabled, do not update replicas.
# In all other cases, replica is set to max(instances, min_instances)
if self.soa_config.config_dict.get("bounce_method", "") == "brutal":
threading.Thread(
target=self.deep_delete_and_create, args=[KubeClient()]
).start()
return
update_deployment(
kube_client=kube_client,
formatted_deployment=self.item,
Expand Down
71 changes: 0 additions & 71 deletions tests/kubernetes/application/test_controller_wrapper.py
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import kubernetes.client
import mock
import pytest
from kubernetes.client import V1DeleteOptions
from kubernetes.client.rest import ApiException

from paasta_tools.kubernetes.application.controller_wrappers import Application
Expand All @@ -27,75 +25,6 @@ def mock_load_system_paasta_config():
yield mock_load_system_paasta_config


def test_brutal_bounce(mock_load_system_paasta_config):
# mock the new client used to brutal bounce in the background using threading.
mock_cloned_client = mock.MagicMock()

with mock.patch(
"paasta_tools.kubernetes.application.controller_wrappers.KubeClient",
return_value=mock_cloned_client,
autospec=True,
):
with mock.patch(
"paasta_tools.kubernetes.application.controller_wrappers.threading.Thread",
autospec=True,
) as mock_deep_delete_and_create:
mock_client = mock.MagicMock()

app = mock.MagicMock()
app.item.metadata.name = "fake_name"
app.item.metadata.namespace = "faasta"

# we do NOT call deep_delete_and_create
app = setup_app({}, True)
DeploymentWrapper.update(self=app, kube_client=mock_client)

assert mock_deep_delete_and_create.call_count == 0

# we call deep_delete_and_create: when bounce_method is brutal
config_dict = {"instances": 1, "bounce_method": "brutal"}

app = setup_app(config_dict, True)
app.update(kube_client=mock_client)

mock_deep_delete_and_create.assert_called_once_with(
target=app.deep_delete_and_create, args=[mock_cloned_client]
)


def test_deep_delete_and_create(mock_load_system_paasta_config):
with mock.patch(
"paasta_tools.kubernetes.application.controller_wrappers.sleep", autospec=True
), mock.patch(
"paasta_tools.kubernetes.application.controller_wrappers.list_all_deployments",
autospec=True,
) as mock_list_deployments, mock.patch(
"paasta_tools.kubernetes.application.controller_wrappers.force_delete_pods",
autospec=True,
) as mock_force_delete_pods:
mock_kube_client = mock.MagicMock()
mock_kube_client.deployments = mock.Mock(spec=kubernetes.client.AppsV1Api)
config_dict = {"instances": 1, "bounce_method": "brutal"}
app = setup_app(config_dict, True)
# This mocks being unable to delete the deployment
mock_list_deployments.return_value = [app.kube_deployment]
delete_options = V1DeleteOptions(propagation_policy="Background")

with pytest.raises(Exception):
# test deep_delete_and_create makes kubeclient calls correctly
app.deep_delete_and_create(mock_kube_client)
mock_force_delete_pods.assert_called_with(
app.item.metadata.name,
app.kube_deployment.service,
app.kube_deployment.instance,
app.item.metadata.namespace,
mock_kube_client,
)
mock_kube_client.deployments.delete_namespaced_deployment.assert_called_with(
app.item.metadata.name, app.item.metadata.namespace, body=delete_options
)


@pytest.mark.parametrize("bounce_margin_factor_set", [True, False])
def test_ensure_pod_disruption_budget_create(
bounce_margin_factor_set,
Expand Down
52 changes: 40 additions & 12 deletions tests/test_kubernetes_tools.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -354,26 +354,54 @@ def test_get_bounce_method(self):
with pytest.raises(Exception):
self.deployment.get_bounce_method()

def test_get_deployment_strategy(self):
@pytest.mark.parametrize(
"bounce_method,bounce_margin_factor,expected_strategy,expected_rolling_update_deploy",
[
(
"crossover",
1,
"RollingUpdate",
V1RollingUpdateDeployment(max_surge="100%", max_unavailable="0%"),
),
(
"crossover",
0.3,
"RollingUpdate",
V1RollingUpdateDeployment(max_surge="100%", max_unavailable="70%"),
),
# b_m_f does not actually contribute to settings for brutal
(
"brutal",
0.5,
"RollingUpdate",
V1RollingUpdateDeployment(max_surge="100%", max_unavailable="100%"),
),
("downthenup", 1, "Recreate", None),
],
)
def test_get_deployment_strategy(
self,
bounce_method,
bounce_margin_factor,
expected_strategy,
expected_rolling_update_deploy,
):
with mock.patch(
"paasta_tools.kubernetes_tools.KubernetesDeploymentConfig.get_bounce_method",
autospec=True,
return_value="crossover",
) as mock_get_bounce_method:
return_value=bounce_method,
), mock.patch(
"paasta_tools.kubernetes_tools.KubernetesDeploymentConfig.get_bounce_margin_factor",
autospec=True,
return_value=bounce_margin_factor,
):
assert (
self.deployment.get_deployment_strategy_config()
== V1DeploymentStrategy(
type="RollingUpdate",
rolling_update=V1RollingUpdateDeployment(
max_surge="100%", max_unavailable="0%"
),
type=expected_strategy,
rolling_update=expected_rolling_update_deploy,
)
)
mock_get_bounce_method.return_value = "downthenup"
assert (
self.deployment.get_deployment_strategy_config()
== V1DeploymentStrategy(type="Recreate")
)

def test_get_sanitised_volume_name(self):
assert (
Expand Down

0 comments on commit f33a07b

Please sign in to comment.