-
Notifications
You must be signed in to change notification settings - Fork 198
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Ensure k8s pod names/labels are RFC 1123 compliant (#3639)
- Modified Kubernetes pod names and labels to conform to RFC 1123 for DNS subdomain names and labels, ensuring compliance with Kubernetes naming conventions. - Modified `KubernetesProvider.submit()` to return an eight-character hex value as the job ID instead of the pod name. - Replaced the trailing timestamp in the pod name with the job ID to improve collision avoidance. - Replaced `app` pod label with `parsl-job-id`. - Updated container name to use job ID.
- Loading branch information
Showing
5 changed files
with
292 additions
and
29 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import re | ||
| from unittest import mock | ||
|
|
||
| import pytest | ||
|
|
||
| from parsl.providers.kubernetes.kube import KubernetesProvider | ||
| from parsl.tests.test_utils.test_sanitize_dns import DNS_SUBDOMAIN_REGEX | ||
|
|
||
| _MOCK_BASE = "parsl.providers.kubernetes.kube" | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mock_kube_config(): | ||
| with mock.patch(f"{_MOCK_BASE}.config") as mock_config: | ||
| mock_config.load_kube_config.return_value = None | ||
| yield mock_config | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_kube_client(): | ||
| mock_client = mock.MagicMock() | ||
| with mock.patch(f"{_MOCK_BASE}.client.CoreV1Api") as mock_api: | ||
| mock_api.return_value = mock_client | ||
| yield mock_client | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| def test_submit_happy_path(mock_kube_client: mock.MagicMock): | ||
| image = "test-image" | ||
| namespace = "test-namespace" | ||
| cmd_string = "test-command" | ||
| volumes = [("test-volume", "test-mount-path")] | ||
| service_account_name = "test-service-account" | ||
| annotations = {"test-annotation": "test-value"} | ||
| max_cpu = 2 | ||
| max_mem = "2Gi" | ||
| init_cpu = 1 | ||
| init_mem = "1Gi" | ||
| provider = KubernetesProvider( | ||
| image=image, | ||
| persistent_volumes=volumes, | ||
| namespace=namespace, | ||
| service_account_name=service_account_name, | ||
| annotations=annotations, | ||
| max_cpu=max_cpu, | ||
| max_mem=max_mem, | ||
| init_cpu=init_cpu, | ||
| init_mem=init_mem, | ||
| ) | ||
|
|
||
| job_name = "test.job.name" | ||
| job_id = provider.submit(cmd_string=cmd_string, tasks_per_node=1, job_name=job_name) | ||
|
|
||
| assert job_id in provider.resources | ||
| assert mock_kube_client.create_namespaced_pod.call_count == 1 | ||
|
|
||
| call_args = mock_kube_client.create_namespaced_pod.call_args[1] | ||
| pod = call_args["body"] | ||
| container = pod.spec.containers[0] | ||
| volume = container.volume_mounts[0] | ||
|
|
||
| assert image == container.image | ||
| assert namespace == call_args["namespace"] | ||
| assert any(cmd_string in arg for arg in container.args) | ||
| assert volumes[0] == (volume.name, volume.mount_path) | ||
| assert service_account_name == pod.spec.service_account_name | ||
| assert annotations == pod.metadata.annotations | ||
| assert str(max_cpu) == container.resources.limits["cpu"] | ||
| assert max_mem == container.resources.limits["memory"] | ||
| assert str(init_cpu) == container.resources.requests["cpu"] | ||
| assert init_mem == container.resources.requests["memory"] | ||
| assert job_id == pod.metadata.labels["parsl-job-id"] | ||
| assert job_id == container.name | ||
| assert f"{job_name}.{job_id}" == pod.metadata.name | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @mock.patch(f"{_MOCK_BASE}.KubernetesProvider._create_pod") | ||
| @pytest.mark.parametrize("char", (".", "-")) | ||
| def test_submit_pod_name_includes_job_id(mock_create_pod: mock.MagicMock, char: str): | ||
| provider = KubernetesProvider(image="test-image") | ||
|
|
||
| job_name = "a." * 121 + f"a{char}" + "a" * 9 | ||
| assert len(job_name) == 253 # Max length for pod name | ||
| job_id = provider.submit(cmd_string="test-command", tasks_per_node=1, job_name=job_name) | ||
|
|
||
| expected_pod_name = job_name[:253 - len(job_id) - 2] + f".{job_id}" | ||
| actual_pod_name = mock_create_pod.call_args[1]["pod_name"] | ||
| assert re.match(DNS_SUBDOMAIN_REGEX, actual_pod_name) | ||
| assert expected_pod_name == actual_pod_name | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @mock.patch(f"{_MOCK_BASE}.KubernetesProvider._create_pod") | ||
| @mock.patch(f"{_MOCK_BASE}.logger") | ||
| @pytest.mark.parametrize("job_name", ("", ".", "-", "a.-.a", "$$$")) | ||
| def test_submit_invalid_job_name(mock_logger: mock.MagicMock, mock_create_pod: mock.MagicMock, job_name: str): | ||
| provider = KubernetesProvider(image="test-image") | ||
| job_id = provider.submit(cmd_string="test-command", tasks_per_node=1, job_name=job_name) | ||
| assert mock_logger.warning.call_count == 1 | ||
| assert f"Invalid pod name '{job_name}' for job '{job_id}'" in mock_logger.warning.call_args[0][0] | ||
| assert f"parsl.kube.{job_id}" == mock_create_pod.call_args[1]["pod_name"] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| import random | ||
| import re | ||
|
|
||
| import pytest | ||
|
|
||
| from parsl.utils import sanitize_dns_label_rfc1123, sanitize_dns_subdomain_rfc1123 | ||
|
|
||
| # Ref: https://datatracker.ietf.org/doc/html/rfc1123 | ||
| DNS_LABEL_REGEX = r'^[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?$' | ||
| DNS_SUBDOMAIN_REGEX = r'^[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?(\.[a-z0-9]([-a-z0-9]{0,61}[a-z0-9])?)*$' | ||
|
|
||
| test_labels = [ | ||
| "example-label-123", # Valid label | ||
| "EXAMPLE", # Case sensitivity | ||
| "!@#example*", # Remove invalid characters | ||
| "--leading-and-trailing--", # Leading and trailing hyphens | ||
| "..leading.and.trailing..", # Leading and tailing dots | ||
| "multiple..dots", # Consecutive dots | ||
| "valid--label", # Consecutive hyphens | ||
| "a" * random.randint(64, 70), # Longer than 63 characters | ||
| f"{'a' * 62}-a", # Trailing hyphen at max length | ||
| ] | ||
|
|
||
|
|
||
| def _generate_test_subdomains(num_subdomains: int): | ||
| subdomains = [] | ||
| for _ in range(num_subdomains): | ||
| num_labels = random.randint(1, 5) | ||
| labels = [test_labels[random.randint(0, num_labels - 1)] for _ in range(num_labels)] | ||
| subdomain = ".".join(labels) | ||
| subdomains.append(subdomain) | ||
| return subdomains | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @pytest.mark.parametrize("raw_string", test_labels) | ||
| def test_sanitize_dns_label_rfc1123(raw_string: str): | ||
| print(sanitize_dns_label_rfc1123(raw_string)) | ||
| assert re.match(DNS_LABEL_REGEX, sanitize_dns_label_rfc1123(raw_string)) | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @pytest.mark.parametrize("raw_string", ("", "-", "@", "$$$")) | ||
| def test_sanitize_dns_label_rfc1123_empty(raw_string: str): | ||
| with pytest.raises(ValueError) as e_info: | ||
| sanitize_dns_label_rfc1123(raw_string) | ||
| assert str(e_info.value) == f"Sanitized DNS label is empty for input '{raw_string}'" | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @pytest.mark.parametrize("raw_string", _generate_test_subdomains(10)) | ||
| def test_sanitize_dns_subdomain_rfc1123(raw_string: str): | ||
| assert re.match(DNS_SUBDOMAIN_REGEX, sanitize_dns_subdomain_rfc1123(raw_string)) | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @pytest.mark.parametrize("char", ("-", ".")) | ||
| def test_sanitize_dns_subdomain_rfc1123_trailing_non_alphanumeric_at_max_length(char: str): | ||
| raw_string = (f"{'a' * 61}." * 4) + f".aaaa{char}a" | ||
| assert re.match(DNS_SUBDOMAIN_REGEX, sanitize_dns_subdomain_rfc1123(raw_string)) | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @pytest.mark.parametrize("raw_string", ("", ".", "...")) | ||
| def test_sanitize_dns_subdomain_rfc1123_empty(raw_string: str): | ||
| with pytest.raises(ValueError) as e_info: | ||
| sanitize_dns_subdomain_rfc1123(raw_string) | ||
| assert str(e_info.value) == f"Sanitized DNS subdomain is empty for input '{raw_string}'" | ||
|
|
||
|
|
||
| @pytest.mark.local | ||
| @pytest.mark.parametrize( | ||
| "raw_string", ("a" * 253, "a" * random.randint(254, 300)), ids=("254 chars", ">253 chars") | ||
| ) | ||
| def test_sanitize_dns_subdomain_rfc1123_max_length(raw_string: str): | ||
| assert len(sanitize_dns_subdomain_rfc1123(raw_string)) <= 253 |
Oops, something went wrong.