Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding LimitRanges for paasta managed namespaces #3726

Merged
merged 2 commits into from
Oct 25, 2023
Merged

Adding LimitRanges for paasta managed namespaces #3726

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
327e32b
Adding LimitRanges for paasta managed namespaces
cuza Oct 23, 2023
b37b1c3
Update paasta_tools/kubernetes_tools.py
cuza Oct 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion paasta_tools/kubernetes_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@
from kubernetes.client import V1KeyToPath
from kubernetes.client import V1LabelSelector
from kubernetes.client import V1Lifecycle
from kubernetes.client import V1LimitRange
from kubernetes.client import V1LimitRangeItem
from kubernetes.client import V1LimitRangeSpec
from kubernetes.client import V1Namespace
from kubernetes.client import V1Node
from kubernetes.client import V1NodeAffinity
Expand Down Expand Up @@ -2676,14 +2679,15 @@ def ensure_namespace(kube_client: KubeClient, namespace: str) -> None:
kube_client.core.create_namespace(body=paasta_namespace)

ensure_paasta_api_rolebinding(kube_client, namespace)
ensure_paasta_namespace_limits(kube_client, namespace)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we move this into the if namespace not in namespace_names: once this has run on all our clusters? seems a bit wasteful to always be checking limit ranges when we know that they're guaranteed to exist (once this has run at least once)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice suggestion but we currently have a bunch of existing namespaces without limits :(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? We're inevitably gonna forget about this and then we'll have namespaces that are created by Flux which don't get these limits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, exactly - once we've released this as-is all of these existing namespace will now have limits and we can clean this up to only set limit ranges on brand new namespaces

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops, looks like I responded with a stale page and never saw your comment

@EvanKrall yea, I guess it depends on how we want to treat folks manually creating namespaces (and then reusing those namespaces for paasta: do we want them to have all of paasta's defaults as well or allow for them to do their own thing?

(this bit us earlier today in #cid - they're reusing a namespace for paasta and the limitrange we added affected pre-existing workloads)



def ensure_paasta_api_rolebinding(kube_client: KubeClient, namespace: str) -> None:
rolebindings = get_all_role_bindings(kube_client, namespace=namespace)
rolebinding_names = [item.metadata.name for item in rolebindings]
if "paasta-api-server-per-namespace" not in rolebinding_names:
log.warning(
f"Creating rolebinding paasta-api-server-per-namespace as it does not exist"
f"Creating rolebinding paasta-api-server-per-namespace on {namespace} namespace as it does not exist"
)
role_binding = V1RoleBinding(
metadata=V1ObjectMeta(
Expand All @@ -2707,6 +2711,39 @@ def ensure_paasta_api_rolebinding(kube_client: KubeClient, namespace: str) -> No
)


def ensure_paasta_namespace_limits(kube_client: KubeClient, namespace: str) -> None:
limits = get_all_limit_ranges(kube_client, namespace=namespace)
limits_names = [item.metadata.name for item in limits]
cuza marked this conversation as resolved.
Show resolved Hide resolved
if "limit-mem-cpu-disk-per-container" not in limits_names:
log.warning(
f"Creating limit: limit-mem-cpu-disk-per-container on {namespace} namespace as it does not exist"
)
limit = V1LimitRange(
metadata=V1ObjectMeta(
name="limit-mem-cpu-disk-per-container",
namespace=namespace,
),
spec=V1LimitRangeSpec(
limits=[
V1LimitRangeItem(
type="Container",
default={
"cpu": "1",
"memory": "1024Mi",
"ephemeral-storage": "1Gi",
},
default_request={
"cpu": "1",
"memory": "1024Mi",
"ephemeral-storage": "1Gi",
},
)
]
),
)
kube_client.core.create_namespaced_limit_range(namespace=namespace, body=limit)


def list_deployments_in_all_namespaces(
kube_client: KubeClient, label_selector: str
) -> List[KubeDeployment]:
Expand Down Expand Up @@ -3893,6 +3930,13 @@ def get_all_role_bindings(
return kube_client.rbac.list_namespaced_role_binding(namespace=namespace).items


def get_all_limit_ranges(
kube_client: KubeClient,
namespace: str,
) -> Sequence[V1LimitRange]:
return kube_client.core.list_namespaced_limit_range(namespace).items


_RE_NORMALIZE_IAM_ROLE = re.compile(r"[^0-9a-zA-Z]+")


Expand Down
29 changes: 29 additions & 0 deletions tests/test_kubernetes_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
from paasta_tools.kubernetes_tools import create_stateful_set
from paasta_tools.kubernetes_tools import ensure_namespace
from paasta_tools.kubernetes_tools import ensure_paasta_api_rolebinding
from paasta_tools.kubernetes_tools import ensure_paasta_namespace_limits
from paasta_tools.kubernetes_tools import filter_nodes_by_blacklist
from paasta_tools.kubernetes_tools import filter_pods_by_service_instance
from paasta_tools.kubernetes_tools import force_delete_pods
Expand Down Expand Up @@ -3022,6 +3023,8 @@ def test_KubeClient():
def test_ensure_namespace_doesnt_create_if_namespace_exists():
with mock.patch(
"paasta_tools.kubernetes_tools.ensure_paasta_api_rolebinding", autospec=True
), mock.patch(
"paasta_tools.kubernetes_tools.ensure_paasta_namespace_limits", autospec=True
):
mock_metadata = mock.Mock()
type(mock_metadata).name = "paasta"
Expand All @@ -3036,6 +3039,8 @@ def test_ensure_namespace_doesnt_create_if_namespace_exists():
def test_ensure_namespace_kube_system():
with mock.patch(
"paasta_tools.kubernetes_tools.ensure_paasta_api_rolebinding", autospec=True
), mock.patch(
"paasta_tools.kubernetes_tools.ensure_paasta_namespace_limits", autospec=True
):
mock_metadata = mock.Mock()
type(mock_metadata).name = "kube-system"
Expand All @@ -3050,6 +3055,8 @@ def test_ensure_namespace_kube_system():
def test_ensure_namespace_creates_namespace_if_doesnt_exist():
with mock.patch(
"paasta_tools.kubernetes_tools.ensure_paasta_api_rolebinding", autospec=True
), mock.patch(
"paasta_tools.kubernetes_tools.ensure_paasta_namespace_limits", autospec=True
):
mock_namespaces = mock.Mock(items=[])
mock_client = mock.Mock(
Expand Down Expand Up @@ -3085,6 +3092,28 @@ def test_ensure_paasta_api_rolebinding_doesnt_create_if_exists():
assert not mock_client.rbac.create_namespaced_role_binding.called


def test_ensure_paasta_namespace_limits_creates_if_not_exist():
mock_limits = mock.Mock(items=[])
mock_client = mock.Mock(
core=mock.Mock(list_namespaced_limit_range=mock.Mock(return_value=mock_limits)),
)

ensure_paasta_namespace_limits(mock_client, namespace="paastasvc-cool-service-name")
assert mock_client.core.create_namespaced_limit_range.called


def test_ensure_paasta_namespace_limits_doesnt_create_if_exists():
mock_metadata = mock.Mock()
type(mock_metadata).name = "limit-mem-cpu-disk-per-container"
mock_limits = mock.Mock(items=[mock.Mock(metadata=mock_metadata)])
mock_client = mock.Mock(
core=mock.Mock(list_namespaced_limit_range=mock.Mock(return_value=mock_limits)),
)

ensure_paasta_namespace_limits(mock_client, namespace="paastasvc-cool-service-name")
assert not mock_client.core.create_namespaced_role_binding.called


@pytest.mark.parametrize(
"addl_labels,replicas",
(
Expand Down
Loading