Commit 29bfaa4a authored by Konstantinos Togias's avatar Konstantinos Togias
Browse files

fix: align container resource requests with canonical units

parent b3a42a90
Loading
Loading
Loading
Loading
+4 −0
Changes for ci/README.md: 4 added lines, 0 removed lines.
Original line number Diff line number Diff line
@@ -54,3 +54,7 @@ The downstream trigger disables inheritance of SDK default variables and passes
only the explicit SDK artifact coordinates. Job-only paths such as TMPDIR must
be established by the owning project. Candidate rc1 exposed this boundary issue
and was not published; rc2 retains a new immutable tag instead of moving rc1.

Packaged container chart checks render canonical CPU millicores and decimal MB
requests, including omission and absence of invented limits. Provider regressions
exercise the same mapping through activation and update.
+17 −5
Changes for ci/verify.py: 17 added lines, 5 removed lines.
Original line number Diff line number Diff line
@@ -182,13 +182,23 @@ def charts():
    chart = Path(openop_tfsdk.__file__).parent / "contract/adapters/charts/container"
    helm = helm_binary()
    image = "example.invalid/fixture@sha256:" + "a" * 64
    for name, service_type, ports in [
        ("none", "ClusterIP", []),
        ("internal", "ClusterIP", [{"name": "p80-tcp", "port": 80, "protocol": "TCP"}]),
        ("external", "NodePort", [{"name": "p80-tcp", "port": 80, "protocol": "TCP"}]),
    for name, service_type, ports, requests in [
        ("none", "ClusterIP", [], {}),
        ("internal", "ClusterIP", [{"name": "p80-tcp", "port": 80, "protocol": "TCP"}], {}),
        ("external", "NodePort", [{"name": "p80-tcp", "port": 80, "protocol": "TCP"}], {}),
        ("requests", "ClusterIP", [], {"cpu": "250m", "memory": "257M"}),
    ]:
        values = OUT / f"chart-{name}.json"
        write_json(values, {"image": image, "serviceType": service_type, "ports": ports})
        expected_resources = {"requests": requests} if requests else {}
        write_json(
            values,
            {
                "image": image,
                "serviceType": service_type,
                "ports": ports,
                "resources": expected_resources,
            },
        )
        run(helm, "lint", chart, "--strict", "-f", values)
        raw = subprocess.check_output(
            [
@@ -208,6 +218,8 @@ def charts():
        deployment = objects["Deployment"]
        spec = deployment["spec"]["template"]["spec"]["containers"][0]
        assert spec["image"] == image
        assert spec.get("resources", {}) == expected_resources
        assert "limits" not in spec.get("resources", {})
        assert spec.get("ports", []) == [
            {"name": p["name"], "containerPort": p["port"], "protocol": p["protocol"]}
            for p in ports
+7 −3
Changes for src/openop_tfsdk/contract/adapters/kubernetes.py: 7 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -106,10 +106,14 @@ def _chart_args(unit) -> list:
    if compute is not None:
        cpu_millicores = getattr(compute, "cpu_millicores", None)
        memory_mb = getattr(compute, "memory_mb", None)
        if cpu_millicores is not None:
        if cpu_millicores is not None and cpu_millicores < 0:
            raise ValidationError("container compute requests must be nonnegative")
        if memory_mb is not None and memory_mb < 0:
            raise ValidationError("container compute requests must be nonnegative")
        if cpu_millicores is not None and cpu_millicores > 0:
            requests["cpu"] = f"{cpu_millicores}m"
        if memory_mb is not None:
            requests["memory"] = f"{memory_mb}Mi"
        if memory_mb is not None and memory_mb > 0:
            requests["memory"] = f"{memory_mb}M"
    resources = {"requests": requests} if requests else {}
    values = {
        "image": unit.artifact_ref,
+64 −3
Changes for tests/contract/test_kubernetes_container.py: 64 added lines, 3 removed lines.
Original line number Diff line number Diff line
@@ -13,7 +13,11 @@ from openop_tfsdk.contract.adapters.kubernetes import (
    KubernetesCapabilityAdapter,
)
from openop_tfsdk.contract.errors import ValidationError
from openop_tfsdk.contract.execution_models import LifecycleState, ServiceEndpoint
from openop_tfsdk.contract.execution_models import (
    ComputeRequirements,
    LifecycleState,
    ServiceEndpoint,
)

HOST = {"endpoint_host": "192.0.2.10"}

@@ -112,11 +116,11 @@ def test_container_without_ports_gets_no_external_service(kubernetes_settings, h
        ),
        (
            SimpleNamespace(cpu_millicores=None, memory_mb=1024),
            {"requests": {"memory": "1024Mi"}},
            {"requests": {"memory": "1024M"}},
        ),
        (
            SimpleNamespace(cpu_millicores=500, memory_mb=1024),
            {"requests": {"cpu": "500m", "memory": "1024Mi"}},
            {"requests": {"cpu": "500m", "memory": "1024M"}},
        ),
    ],
)
@@ -221,3 +225,60 @@ def test_unreadable_service_still_activates(kubernetes_settings, monkeypatch, ob
    result = KubernetesCapabilityAdapter({**kubernetes_settings, **HOST}).activate(request)
    assert result.state == LifecycleState.ACTIVE
    assert result.endpoints is None


def _with_compute(request, cpu_millicores, memory_mb):
    request.deployment_unit.resource_requirements = ComputeRequirements(
        schema_version="srm.compute/v1",
        compute={
            "cpu_millicores": cpu_millicores,
            "memory_mb": memory_mb,
            "storage": [],
        },
        topology={},
        interfaces=[],
        standalone=False,
    )
    return request


@pytest.mark.parametrize("action", ["activate", "update"])
@pytest.mark.parametrize(
    "cpu,memory,expected",
    [
        (1500, 257, {"cpu": "1500m", "memory": "257M"}),
        (1000, 256, {"cpu": "1000m", "memory": "256M"}),
        (0, 256, {"memory": "256M"}),
        (500, 0, {"cpu": "500m"}),
        (0, 0, {}),
        (500, None, {"cpu": "500m"}),
        (None, 257, {"memory": "257M"}),
        (None, None, {}),
    ],
)
def test_container_applies_canonical_compute_as_requests_without_inventing_limits(
    kubernetes_settings, helm_calls, action, cpu, memory, expected
):
    request = _with_compute(_request("container"), cpu, memory)
    getattr(KubernetesCapabilityAdapter(kubernetes_settings), action)(request)
    values = _values(helm_calls[0])
    assert values.get("resources", {}) == ({"requests": expected} if expected else {})
    assert "limits" not in values.get("resources", {})


@pytest.mark.parametrize("action", ["activate", "update"])
@pytest.mark.parametrize("cpu,memory", [(-1, 256), (1000, -1)])
def test_negative_compute_is_rejected_before_helm(
    kubernetes_settings, helm_calls, action, cpu, memory
):
    request = _with_compute(_request("container"), cpu, memory)
    with pytest.raises(ValidationError, match="nonnegative"):
        getattr(KubernetesCapabilityAdapter(kubernetes_settings), action)(request)
    assert helm_calls == []


def test_custom_helm_chart_keeps_its_existing_values_contract(kubernetes_settings, helm_calls):
    request = _with_compute(_request("helm", artifact_ref="oci://registry/app:1.0"), 1000, 256)
    KubernetesCapabilityAdapter(kubernetes_settings).activate(request)
    assert helm_calls[0][3] == "oci://registry/app:1.0"
    assert "--set-json" not in helm_calls[0]