From 7ff13bad451888def1d207b50e37a8729c5a96ac Mon Sep 17 00:00:00 2001 From: Nan Gao Date: Fri, 9 Oct 2026 18:26:23 +0800 Subject: [PATCH] fix(helm): let the sandbox provisioner run with several replicas (#6543) * fix(helm): let the sandbox provisioner run with several replicas The chart hard-coded replicas: 1 into the provisioner Deployment, gave it no rollout strategy and no PodDisruptionBudget, and values.yaml had no provisioner.replicas key, so a multi-replica gateway deployment still sent every sandbox create/discover/destroy call through one Pod: while that Pod restarted or its node drained no sandbox could be created anywhere. - values.yaml provisioner gains replicas (default 1, unchanged), the gateway's surge-then-drain RollingUpdate strategy and podDisruptionBudget {enabled: true, minAvailable: 1}. - provisioner-deployment.yaml renders replicas and strategy from values; at 1 replica the result equals the Kubernetes default strategy. - New provisioner-pdb.yaml, rendered only while the provisioner is enabled, the budget is enabled and replicas > 1, selecting on the Deployment's labels and named -provisioner so it never collides with the gateway budget. No multiInstance override: nothing else in the chart renders differently per provisioner replica count. - docker/provisioner/app.py is unchanged. It keeps no state between requests: the labelled sandbox Pods and Services are the only registry, create tolerates the 409 a concurrent creator produces, destroy tolerates 404, NodePorts come from the API server and there is no background loop. The chart README and docker/provisioner/README.md record this, and tests/test_helm_provisioner_replicas.py pins it at source level next to the values, template and helm-render assertions. * docs(changelog): link the provisioner replicas entry to #6543 * fix(helm): preserve percentage PodDisruptionBudget values instead of casting them to 0 Review of #6543: both PDB templates rendered minAvailable through `| default 1 | int`, and sprig's int turns a percentage string such as "50%" -- a value Kubernetes accepts -- into 0, a budget that permits every Pod to be evicted. The gateway budget had the same pre-existing cast. - New shared helper deer-flow.pdbMinAvailable in _helpers.tpl: a YAML or --set number renders as an integer, a digit string from --set-string becomes that integer, a percentage is preserved quoted, and 0, "0%", more than 100%, a non-integer or anything else fails the render with a message naming the values key and the enabled switch to use instead. Unset still falls back to 1. - gateway-pdb.yaml and provisioner-pdb.yaml render minAvailable through it. - values.yaml comments, the chart README multi-replica note and the CHANGELOG entry say integer-or-percentage and that zero is rejected. - tests/test_helm_provisioner_replicas.py pins the helper and the two templates at text level and adds render cases for both budgets: "50%" with three replicas renders "50%", 2 via --set and --set-string renders 2, and abc / 0 / 0% / 150% fail naming the key. * fix(helm): normalize json.Number before the minAvailable string branches Review of #6543: Helm 3.18.0 loads YAML numbers as json.Number, whose kind is "string" but which regexMatch and the other string-typed sprig functions reject at argument validation ("expected string; got json.Number"). deer-flow.pdbMinAvailable gated its string branch on kindIs "string", so the unoverridden chart default minAvailable: 1 made every gateway.replicas=2 / provisioner.replicas=2 render fail on that release, inside the README's documented 3.8+ support window. - The helper keeps the explicit float64/int64/int branch and now converts everything else with toString into $text before the digit, percentage and fail branches; regexMatch, atoi, trimSuffix and quote only ever see that plain string. A json.Number 1 renders 1, a json.Number 1.5 still fails (the digit regex rejects it) and the hint still prints the raw value; bools and other kinds still fail. The helper comment records the Helm 3.18.0 reason. - tests/test_helm_provisioner_replicas.py pins that the string branch runs on the toString-normalized variable, that no string-typed function is applied to $raw and that kindIs "string" is gone, and adds an explicit render case for both budgets with only the replica count set (the unoverridden default must render minAvailable: 1), marked as the Helm 3.18.0 json.Number regression guard. --------- Co-authored-by: Willem Jiang --- CHANGELOG.md | 18 + .../tests/test_helm_provisioner_replicas.py | 310 ++++++++++++++++++ deploy/helm/deer-flow/README.md | 33 +- deploy/helm/deer-flow/templates/_helpers.tpl | 34 ++ .../helm/deer-flow/templates/gateway-pdb.yaml | 2 +- .../templates/provisioner-deployment.yaml | 6 +- .../deer-flow/templates/provisioner-pdb.yaml | 26 ++ deploy/helm/deer-flow/values.yaml | 36 +- docker/provisioner/README.md | 31 ++ 9 files changed, 488 insertions(+), 8 deletions(-) create mode 100644 backend/tests/test_helm_provisioner_replicas.py create mode 100644 deploy/helm/deer-flow/templates/provisioner-pdb.yaml diff --git a/CHANGELOG.md b/CHANGELOG.md index d31f843928..698339c9f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1185,6 +1185,23 @@ This release closes that milestone with **439 merged pull requests**. Upgrading a release that predates `AUTH_JWT_SECRET` generates a new key and signs every browser session out once; the chart README shows how to seed the previous key into the Secret first to keep sessions. ([#6347]) +- **deploy:** The Helm chart no longer pins the sandbox provisioner to one Pod. + `provisioner.replicas` (default 1, unchanged) sets the replica count, the + provisioner Deployment gets the gateway's surge-then-drain rollout strategy + (`maxSurge: 1`, `maxUnavailable: 0`), and a `PodDisruptionBudget` + (`provisioner.podDisruptionBudget`, `minAvailable: 1`) is rendered while + `provisioner.replicas > 1`, so a multi-replica gateway deployment no longer + loses sandbox creation whenever its single provisioner Pod restarts or its + node drains. The provisioner itself needed no change: the labelled sandbox + Pods and Services are its only registry, every handler reads them back from + the API server, and create already tolerates the `409 AlreadyExists` a + concurrent creator produces. The chart README and the provisioner README + record what the replica count rests on and that the budget never renders + for a single replica. Both budgets render `minAvailable` through one helper + that preserves a percentage such as `"50%"` (the previous `int` cast, also + in the pre-existing gateway budget, silently turned it into `0`, a budget + that protects nothing) and fails the render for `0`, `"0%"` or any other + unsupported value with a message naming the values key. ([#6543]) - **persistence:** A second Gateway instance no longer fails startup with `TimeoutError` while another instance runs a PostgreSQL schema migration. The bootstrap advisory lock was taken with a blocking `pg_advisory_lock` on the @@ -9318,3 +9335,4 @@ with **180 merged pull requests** since the first 2.0 milestone tag. [#6512]: https://github.com/bytedance/deer-flow/pull/6512 [#6519]: https://github.com/bytedance/deer-flow/pull/6519 [#6537]: https://github.com/bytedance/deer-flow/pull/6537 +[#6543]: https://github.com/bytedance/deer-flow/pull/6543 diff --git a/backend/tests/test_helm_provisioner_replicas.py b/backend/tests/test_helm_provisioner_replicas.py new file mode 100644 index 0000000000..e03e0da3ca --- /dev/null +++ b/backend/tests/test_helm_provisioner_replicas.py @@ -0,0 +1,310 @@ +"""Regression tests for the Helm chart's sandbox-provisioner replica count. + +The chart hard-coded ``replicas: 1`` into the provisioner Deployment, gave it +no rollout strategy and no PodDisruptionBudget, and ``values.yaml`` had no +``provisioner.replicas`` key, so a multi-replica gateway deployment still sent +every sandbox create/discover/destroy call through one Pod: while that Pod +restarted or its node drained no sandbox could be created. The provisioner +itself keeps no state between requests -- the labelled sandbox Pods and +Services are the only registry, every handler reads them back from the API +server, and create tolerates the 409 a concurrent creator produces -- so the +chart only had to stop pinning it. These tests pin the values keys, the +Deployment wiring, the PodDisruptionBudget guard (enabled and replicas > 1), +the label agreement between the two templates, the operator notes, and the +source-level statelessness the replica claim rests on. + +Review of #6543 found that both budgets rendered ``minAvailable`` through +``| int``, which turns a percentage such as ``"50%"`` (a value Kubernetes +accepts) into ``0`` -- a budget that protects nothing -- so the tests also pin +the shared ``deer-flow.pdbMinAvailable`` helper: integers pass through, digit +strings from ``--set-string`` become integers, percentages stay quoted, and +``0`` or anything else fails the render naming the values key. + +The ``helm template`` tests skip when helm is not installed; CI's runner has it. +""" + +from __future__ import annotations + +import ast +import re +import shutil +import subprocess +from pathlib import Path + +import pytest +import yaml + +REPO_ROOT = Path(__file__).resolve().parents[2] +CHART = REPO_ROOT / "deploy" / "helm" / "deer-flow" +VALUES = CHART / "values.yaml" +README = CHART / "README.md" +DEPLOYMENT_TEMPLATE = CHART / "templates" / "provisioner-deployment.yaml" +PDB_TEMPLATE = CHART / "templates" / "provisioner-pdb.yaml" +GATEWAY_PDB_TEMPLATE = CHART / "templates" / "gateway-pdb.yaml" +HELPERS_TEMPLATE = CHART / "templates" / "_helpers.tpl" +PDB_TEMPLATES = {"gateway": GATEWAY_PDB_TEMPLATE, "provisioner": PDB_TEMPLATE} +PROVISIONER_APP = REPO_ROOT / "docker" / "provisioner" / "app.py" +PROVISIONER_README = REPO_ROOT / "docker" / "provisioner" / "README.md" + +# A selector block as both templates spell it: the shared selector helper plus +# the component label, so the budget can only ever count provisioner Pods. +SELECTOR_BLOCK = re.compile(r"matchLabels:\n\s+\{\{- include \"deer-flow.selectorLabels\" \. \| nindent 6 \}\}\n\s+app\.kubernetes\.io/component: provisioner\n") + +# Methods that mutate a container in place. A module-level container touched by +# one of these from a request handler would be per-Pod state the chart's +# replica claim could not survive. +MUTATING_METHODS = frozenset({"add", "append", "clear", "discard", "extend", "insert", "pop", "popitem", "remove", "setdefault", "update"}) +BACKGROUND_WORK = frozenset({"create_task", "ensure_future", "Thread", "Timer", "add_event_handler", "on_event", "repeat_every"}) + + +def _values() -> dict: + return yaml.safe_load(VALUES.read_text(encoding="utf-8")) + + +def _helm(*args: str) -> subprocess.CompletedProcess[str]: + helm = shutil.which("helm") + if helm is None: + pytest.skip("helm is unavailable") + return subprocess.run([helm, "template", "deer-flow", str(CHART), *args], capture_output=True, text=True) + + +def _render(*args: str) -> list[dict]: + result = _helm(*args) + assert result.returncode == 0, result.stderr + return [document for document in yaml.safe_load_all(result.stdout) if isinstance(document, dict)] + + +def _render_chart(*settings: str) -> list[dict]: + return _render(*(arg for setting in settings for arg in ("--set", setting))) + + +def _render_failure(*args: str) -> str: + """Return helm's stderr for a render that must be refused.""" + result = _helm(*args) + assert result.returncode != 0, "the render must fail instead of producing a budget that protects nothing" + return result.stderr + + +def _min_available_include(component: str) -> str: + return f'minAvailable: {{{{ include "deer-flow.pdbMinAvailable" (dict "value" $pdb.minAvailable "key" "{component}.podDisruptionBudget.minAvailable") }}}}' + + +def _find(documents: list[dict], kind: str, name_suffix: str) -> dict | None: + return next((document for document in documents if document.get("kind") == kind and document["metadata"]["name"].endswith(name_suffix)), None) + + +def _by_kind(documents: list[dict], kind: str, name_suffix: str) -> dict: + document = _find(documents, kind, name_suffix) + assert document is not None, f"no {kind} ending in {name_suffix}" + return document + + +def _root_name(node: ast.expr) -> str | None: + while isinstance(node, (ast.Subscript, ast.Attribute)): + node = node.value + return node.id if isinstance(node, ast.Name) else None + + +def _module_level_mutations(tree: ast.Module) -> list[str]: + """Return every statement that writes into a module-level container from anywhere in the module.""" + module_names = {target.id for node in tree.body if isinstance(node, ast.Assign) for target in node.targets if isinstance(target, ast.Name)} + module_names |= {node.target.id for node in tree.body if isinstance(node, ast.AnnAssign) and isinstance(node.target, ast.Name)} + findings: list[str] = [] + for node in ast.walk(tree): + if isinstance(node, (ast.Assign, ast.AugAssign, ast.AnnAssign)): + targets = node.targets if isinstance(node, ast.Assign) else [node.target] + for target in targets: + if isinstance(target, ast.Subscript) and _root_name(target) in module_names: + findings.append(f"line {node.lineno}: item assignment into {_root_name(target)}") + elif isinstance(node, ast.Delete): + for target in node.targets: + if isinstance(target, ast.Subscript) and _root_name(target) in module_names: + findings.append(f"line {node.lineno}: del on {_root_name(target)}") + elif isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) and node.func.attr in MUTATING_METHODS: + if isinstance(node.func.value, ast.Name) and node.func.value.id in module_names: + findings.append(f"line {node.lineno}: {node.func.value.id}.{node.func.attr}()") + return findings + + +def test_default_values_keep_one_provisioner_replica_with_a_rollout_budget() -> None: + provisioner = _values()["provisioner"] + assert provisioner["enabled"] is True + assert provisioner["replicas"] == 1, "the conservative default is unchanged; operators opt into more" + assert provisioner["strategy"]["type"] == "RollingUpdate" + assert provisioner["strategy"]["rollingUpdate"]["maxSurge"] == 1 + assert provisioner["strategy"]["rollingUpdate"]["maxUnavailable"] == 0, "surge-then-drain keeps one Pod answering sandbox calls through a rollout" + assert provisioner["podDisruptionBudget"]["enabled"] is True + assert provisioner["podDisruptionBudget"]["minAvailable"] == 1 + + +def test_deployment_template_takes_replicas_and_strategy_from_values() -> None: + template = DEPLOYMENT_TEMPLATE.read_text(encoding="utf-8") + assert "replicas: {{ .Values.provisioner.replicas }}" in template + assert re.search(r"^\s*replicas:\s*\d+\s*$", template, flags=re.MULTILINE) is None, "a literal replica count ignores provisioner.replicas" + assert re.search(r"\{\{- with \.Values\.provisioner\.strategy \}\}\n\s+strategy:\n\s+\{\{- toYaml \. \| nindent 4 \}\}\n\s+\{\{- end \}\}", template), "the rollout strategy must come from values like the gateway's" + assert SELECTOR_BLOCK.search(template), "the Deployment selector is what the budget has to match" + + +def test_pdb_template_is_guarded_on_enabled_and_more_than_one_replica() -> None: + template = PDB_TEMPLATE.read_text(encoding="utf-8") + guard = re.search(r"\{\{- if and \.Values\.provisioner\.enabled \$pdb\.enabled \(gt \(int \.Values\.provisioner\.replicas\) 1\) \}\}", template) + assert guard is not None, "render only for an enabled provisioner with an enabled budget and more than one replica" + assert "{{- $pdb := .Values.provisioner.podDisruptionBudget | default dict -}}" in template + assert "apiVersion: policy/v1" in template + assert "kind: PodDisruptionBudget" in template + assert 'name: {{ include "deer-flow.fullname" . }}-provisioner' in template, "must not collide with the gateway budget" + assert _min_available_include("provisioner") in template, "minAvailable goes through the shared helper so a percentage survives" + assert template.count("app.kubernetes.io/component: provisioner") == 2, "once in metadata.labels, once in the selector" + assert SELECTOR_BLOCK.search(template), "the selector must match the Deployment's Pod labels exactly" + + +def test_provisioner_pdb_mirrors_the_gateway_pdb() -> None: + """Same shape as the gateway budget so the two stay reviewable side by side.""" + gateway = GATEWAY_PDB_TEMPLATE.read_text(encoding="utf-8").replace("gateway", "provisioner") + provisioner = PDB_TEMPLATE.read_text(encoding="utf-8") + for line in ("apiVersion: policy/v1", "kind: PodDisruptionBudget", 'name: {{ include "deer-flow.fullname" . }}-provisioner', _min_available_include("provisioner")): + assert line in gateway and line in provisioner, line + + +@pytest.mark.parametrize("component", sorted(PDB_TEMPLATES)) +def test_both_budgets_render_min_available_through_the_shared_helper(component: str) -> None: + """``| int`` turns "50%" into 0; Kubernetes accepts an integer or a percentage, so the chart must preserve both.""" + template = PDB_TEMPLATES[component].read_text(encoding="utf-8") + assert _min_available_include(component) in template + assert re.search(r"minAvailable:.*\|\s*int\b", template) is None, "int coerces a percentage string to 0" + assert "default 1" not in template, "the fallback lives in the helper so both budgets share it" + + +def _helper_block() -> str: + helpers = HELPERS_TEMPLATE.read_text(encoding="utf-8") + start = helpers.index('{{- define "deer-flow.pdbMinAvailable" -}}') + end = helpers.find("{{- define ", start + 1) + return helpers[start:] if end == -1 else helpers[start:end] + + +def test_min_available_helper_preserves_percentages_and_fails_on_zero() -> None: + helper = _helper_block() + assert 'regexMatch "^[0-9]+%$"' in helper, "a percentage is preserved, not cast" + assert 'regexMatch "^[0-9]+$"' in helper, "a digit string from --set-string becomes an integer" + assert "quote" in helper, "the percentage must stay a YAML string" + assert "fail" in helper, "zero and unsupported values refuse to render instead of yielding 0" + assert 'kindIs "invalid"' in helper, "an unset value still falls back to 1" + assert re.search(r"\|\s*int\s*-?\}\}", helper) is None, "no branch may pipe the raw value through int" + + +def test_min_available_helper_normalizes_named_numbers_before_string_functions() -> None: + """Helm 3.18.0 loads YAML numbers as ``json.Number``: its kind is "string", so a ``kindIs "string"`` + gate lets it through, but ``regexMatch`` and the other string-typed functions reject it at argument + validation -- and the unoverridden default ``minAvailable: 1`` then fails every multi-replica render.""" + helper = _helper_block() + assert "$text := toString $raw" in helper, "whatever is not a plain Go number is normalized into a plain string first" + assert 'kindIs "string"' not in helper, "json.Number passes a kind gate without being a plain string" + string_calls = [line for line in helper.splitlines() if re.search(r"\b(regexMatch|atoi|trimSuffix|quote)\b", line)] + assert string_calls, "the digit and percentage branches must still exist" + for line in string_calls: + assert "$raw" not in line, f"string-typed function applied to the raw value: {line.strip()}" + assert "$text" in line, f"string-typed function must take the normalized value: {line.strip()}" + assert re.search(r"printf .*\(got %v\).*\$raw", helper), "the hint still shows the raw value the operator supplied" + + +def test_operator_notes_document_the_provisioner_replicas() -> None: + values = VALUES.read_text(encoding="utf-8") + readme = README.read_text(encoding="utf-8") + provisioner_readme = PROVISIONER_README.read_text(encoding="utf-8") + for needle in ("provisioner.replicas", "409", "PodDisruptionBudget"): + assert needle in readme, needle + assert "409" in values, "the comment explains why a concurrent create on another replica is harmless" + assert "replica" in provisioner_readme, "the provisioner's own README must say it can run several replicas" + assert "app=deer-flow-sandbox" in provisioner_readme or "labels" in provisioner_readme, "and that the labelled K8s objects are the only registry" + assert "that one provisioner" not in readme, "the health note must not promise a single provisioner Pod" + + +def test_provisioner_keeps_no_state_between_requests() -> None: + """The replica claim in values.yaml rests on this; a registry or reaper added here must re-open it.""" + source = PROVISIONER_APP.read_text(encoding="utf-8") + tree = ast.parse(source, filename=str(PROVISIONER_APP)) + globals_declared = {name for node in ast.walk(tree) if isinstance(node, ast.Global) for name in node.names} + assert globals_declared == {"core_v1"}, f"only the K8s client is process-global, found {sorted(globals_declared)}" + assert _module_level_mutations(tree) == [], "a module-level container written at request time is per-Pod state" + background = sorted({node.attr for node in ast.walk(tree) if isinstance(node, ast.Attribute) and node.attr in BACKGROUND_WORK}) + assert background == [], f"a background loop would run once per replica: {background}" + assert source.count("exc.status != 409") >= 2, "Pod and Service create must both tolerate AlreadyExists from a concurrent creator" + assert 'label_selector="app=deer-flow-sandbox"' in source, "the label is the registry every replica lists from" + + +@pytest.mark.parametrize("replicas", [1, 2, 3]) +def test_rendered_provisioner_follows_the_replica_count(replicas: int) -> None: + values = _values()["provisioner"] + documents = _render_chart(f"provisioner.replicas={replicas}") + deployment = _by_kind(documents, "Deployment", "-provisioner") + assert deployment["spec"]["replicas"] == replicas + assert deployment["spec"]["strategy"] == values["strategy"] + + pdb = _find(documents, "PodDisruptionBudget", "-provisioner") + assert (pdb is not None) is (replicas > 1), "a budget of 1 on a single replica would block every node drain" + assert _find(documents, "PodDisruptionBudget", "-gateway") is None, "the gateway stays at one replica in this render" + if pdb is not None: + # Also a Helm 3.18.0 json.Number regression guard for the unoverridden YAML default; the + # explicit gateway + provisioner case is test_rendered_budget_keeps_the_unoverridden_default. + assert pdb["spec"]["minAvailable"] == values["podDisruptionBudget"]["minAvailable"] + assert pdb["spec"]["selector"]["matchLabels"] == deployment["spec"]["selector"]["matchLabels"] + assert pdb["spec"]["selector"]["matchLabels"] == deployment["spec"]["template"]["metadata"]["labels"] + assert pdb["metadata"]["labels"]["app.kubernetes.io/component"] == "provisioner" + assert pdb["metadata"]["namespace"] == deployment["metadata"]["namespace"] + + +def test_rendered_default_matches_the_previous_single_replica_deployment() -> None: + documents = _render_chart() + deployment = _by_kind(documents, "Deployment", "-provisioner") + assert deployment["spec"]["replicas"] == 1 + assert deployment["spec"]["strategy"] == {"type": "RollingUpdate", "rollingUpdate": {"maxSurge": 1, "maxUnavailable": 0}} + assert not any(document.get("kind") == "PodDisruptionBudget" for document in documents) + + +def test_gateway_and_provisioner_budgets_render_side_by_side() -> None: + documents = _render_chart("gateway.replicas=2", "provisioner.replicas=2") + names = sorted(document["metadata"]["name"] for document in documents if document.get("kind") == "PodDisruptionBudget") + assert len(names) == 2 and names[0].endswith("-gateway") and names[1].endswith("-provisioner") + + +def test_disabled_provisioner_renders_neither_deployment_nor_budget() -> None: + documents = _render_chart("provisioner.enabled=false", "provisioner.replicas=2") + assert _find(documents, "Deployment", "-provisioner") is None + assert _find(documents, "PodDisruptionBudget", "-provisioner") is None + + +def test_provisioner_budget_can_be_disabled_independently() -> None: + documents = _render_chart("provisioner.replicas=2", "provisioner.podDisruptionBudget.enabled=false") + assert _by_kind(documents, "Deployment", "-provisioner")["spec"]["replicas"] == 2 + assert _find(documents, "PodDisruptionBudget", "-provisioner") is None + + +@pytest.mark.parametrize("component", sorted(PDB_TEMPLATES)) +def test_rendered_budget_keeps_the_unoverridden_default(component: str) -> None: + """Helm 3.18.0 json.Number regression guard: with only the replica count set, the chart's own + ``minAvailable: 1`` must render as the integer 1 -- that release loads YAML numbers as a named + string type that string-typed template functions reject unless the helper normalizes it first.""" + documents = _render("--set", f"{component}.replicas=2") + assert _by_kind(documents, "PodDisruptionBudget", f"-{component}")["spec"]["minAvailable"] == 1 + + +@pytest.mark.parametrize("component", sorted(PDB_TEMPLATES)) +def test_rendered_budget_preserves_a_percentage(component: str) -> None: + """With three replicas "50%" must keep two Pods; the old ``int`` coercion rendered 0 and protected none.""" + documents = _render("--set", f"{component}.replicas=3", "--set-string", f"{component}.podDisruptionBudget.minAvailable=50%") + assert _by_kind(documents, "PodDisruptionBudget", f"-{component}")["spec"]["minAvailable"] == "50%" + + +@pytest.mark.parametrize("component", sorted(PDB_TEMPLATES)) +@pytest.mark.parametrize("flag", ["--set", "--set-string"]) +def test_rendered_budget_keeps_an_integer_whichever_way_it_is_set(component: str, flag: str) -> None: + documents = _render("--set", f"{component}.replicas=3", flag, f"{component}.podDisruptionBudget.minAvailable=2") + assert _by_kind(documents, "PodDisruptionBudget", f"-{component}")["spec"]["minAvailable"] == 2 + + +@pytest.mark.parametrize("component", sorted(PDB_TEMPLATES)) +@pytest.mark.parametrize(("flag", "value"), [("--set-string", "abc"), ("--set", "0"), ("--set-string", "0%"), ("--set-string", "150%")]) +def test_rendered_budget_refuses_zero_and_unsupported_values(component: str, flag: str, value: str) -> None: + key = f"{component}.podDisruptionBudget.minAvailable" + stderr = _render_failure("--set", f"{component}.replicas=3", flag, f"{key}={value}") + assert key in stderr, "the message must name the values key the operator has to fix" diff --git a/deploy/helm/deer-flow/README.md b/deploy/helm/deer-flow/README.md index 8a47b26300..ca529c38fd 100644 --- a/deploy/helm/deer-flow/README.md +++ b/deploy/helm/deer-flow/README.md @@ -242,7 +242,8 @@ curl http://localhost:2026/health/ready # readiness: database, checkpointer, Postgres or the Redis stream bridge is unreachable (`stream_bridge: unreachable`), so those pods leave the Service instead of accepting runs they cannot stream. The `provisioner` field reports the provisioner's own `/health` but never -changes the status code: every gateway pod shares that one provisioner. +changes the status code: every gateway pod reaches the same `provisioner` +Service, whichever replica answers. Hit the Ingress host (map it in `/etc/hosts` for local clusters) to load the UI. @@ -318,9 +319,33 @@ kubectl -n deer-flow exec deploy/deer-flow-provisioner -- curl -s localhost:8002 override `config:` — a multi-instance gateway that leaves the index under the memory root logs a warning at startup. The index is rebuilt from the Markdown facts on every Pod start, so losing the emptyDir loses nothing. - A `PodDisruptionBudget` (`minAvailable: 1`) is rendered automatically for a - multi-instance gateway (same rule), and the rollout strategy is - surge-then-drain (`maxSurge: 1`, `maxUnavailable: 0`). + A `PodDisruptionBudget` (`minAvailable: 1`; an integer or a percentage + string such as `"50%"`, while `0` fails the render -- disable the budget + instead) is rendered automatically for a multi-instance gateway (same rule), + and the rollout strategy is surge-then-drain (`maxSurge: 1`, + `maxUnavailable: 0`). The sandbox + provisioner is not on the single-instance list: it scales independently + (next item). +- **Provisioner replicas.** `provisioner.replicas` (default 1) scales the + sandbox provisioner. It keeps no state of its own between requests: the + sandbox Pods and Services it creates carry the labels + (`app=deer-flow-sandbox`, `sandbox-id`) that are its only registry; create, + discover, list and destroy all read them back from the API server; create + tolerates the `409 AlreadyExists` a concurrent creator on another replica + produces (several gateway Pods already run that race against one + provisioner) and destroy tolerates `404`; and NodePorts are allocated by the + API server, not the provisioner. The `provisioner` Service therefore spreads + the gateway's calls over any number of replicas without affinity. At 1 + replica it is the last single point of failure in a multi-replica gateway + deployment: while its Pod restarts or its node drains no sandbox can be + created or discovered (running sandboxes are unaffected; the gateway talks + to them directly). A `PodDisruptionBudget` (`provisioner.podDisruptionBudget`, + `minAvailable: 1`) is rendered only while `provisioner.replicas > 1` -- on a + single replica it would block every node drain -- and the rollout strategy + is surge-then-drain (`maxSurge: 1`, `maxUnavailable: 0`), both mirroring + the gateway. There is no `multiInstance` switch for the provisioner because + nothing else in the chart renders differently per replica count; scaling it + with `kubectl scale` only leaves it without the budget. - **App secret.** `-app` holds `BETTER_AUTH_SECRET`, `DEER_FLOW_INTERNAL_AUTH_TOKEN`, `AUTH_JWT_SECRET` (the session-cookie signing key) and `PROVISIONER_API_KEY` (the key the gateway presents to the diff --git a/deploy/helm/deer-flow/templates/_helpers.tpl b/deploy/helm/deer-flow/templates/_helpers.tpl index 2ec38d4343..d0b2d38172 100644 --- a/deploy/helm/deer-flow/templates/_helpers.tpl +++ b/deploy/helm/deer-flow/templates/_helpers.tpl @@ -93,6 +93,40 @@ imagePullSecrets: {{- end -}} {{- end -}} +{{/* PodDisruptionBudget minAvailable as Kubernetes accepts it: a positive + integer or a percentage string such as "50%". Takes a dict with `value` + (the raw values entry; unset falls back to 1) and `key` (the values path, + named in the error). `int` on "50%" is 0 -- a budget that protects + nothing -- so a YAML number renders as an integer, a digit string from + --set-string becomes that integer, a percentage is preserved quoted, and + 0, "0%", more than 100% or anything else fails the render. Anything that + is not a plain Go number is normalized with toString before the string + branches: Helm 3.18.0 loads YAML numbers as json.Number, whose kind is + "string" but which regexMatch and friends reject at argument validation, + so the unoverridden default would otherwise fail every multi-replica + render on that release. */}} +{{- define "deer-flow.pdbMinAvailable" -}} +{{- $raw := .value -}} +{{- if kindIs "invalid" $raw -}}{{- $raw = 1 -}}{{- end -}} +{{- $hint := printf "%s must be a positive integer or a percentage such as \"50%%\" (got %v); a budget of 0 protects nothing, so set %s to false instead" .key $raw (replace ".minAvailable" ".enabled" .key) -}} +{{- if or (kindIs "float64" $raw) (kindIs "int64" $raw) (kindIs "int" $raw) -}} +{{- if or (lt (int $raw) 1) (ne (float64 (int $raw)) (float64 $raw)) -}}{{- fail $hint -}}{{- end -}} +{{- int $raw -}} +{{- else -}} +{{- $text := toString $raw -}} +{{- if regexMatch "^[0-9]+$" $text -}} +{{- if lt (atoi $text) 1 -}}{{- fail $hint -}}{{- end -}} +{{- atoi $text -}} +{{- else if regexMatch "^[0-9]+%$" $text -}} +{{- $percent := atoi (trimSuffix "%" $text) -}} +{{- if or (lt $percent 1) (gt $percent 100) -}}{{- fail $hint -}}{{- end -}} +{{- $text | quote -}} +{{- else -}} +{{- fail $hint -}} +{{- end -}} +{{- end -}} +{{- end -}} + {{/* Name of the postgres StatefulSet/Service. */}} {{- define "deer-flow.postgresFullname" -}} {{- printf "%s-postgres" (include "deer-flow.fullname" .) -}} diff --git a/deploy/helm/deer-flow/templates/gateway-pdb.yaml b/deploy/helm/deer-flow/templates/gateway-pdb.yaml index 29c0d0b45b..4a7ec0c39e 100644 --- a/deploy/helm/deer-flow/templates/gateway-pdb.yaml +++ b/deploy/helm/deer-flow/templates/gateway-pdb.yaml @@ -16,7 +16,7 @@ metadata: {{- include "deer-flow.labels" . | nindent 4 }} app.kubernetes.io/component: gateway spec: - minAvailable: {{ $pdb.minAvailable | default 1 | int }} + minAvailable: {{ include "deer-flow.pdbMinAvailable" (dict "value" $pdb.minAvailable "key" "gateway.podDisruptionBudget.minAvailable") }} selector: matchLabels: {{- include "deer-flow.selectorLabels" . | nindent 6 }} diff --git a/deploy/helm/deer-flow/templates/provisioner-deployment.yaml b/deploy/helm/deer-flow/templates/provisioner-deployment.yaml index a787f00362..fdaf1cfa90 100644 --- a/deploy/helm/deer-flow/templates/provisioner-deployment.yaml +++ b/deploy/helm/deer-flow/templates/provisioner-deployment.yaml @@ -8,7 +8,11 @@ metadata: {{- include "deer-flow.labels" . | nindent 4 }} app.kubernetes.io/component: provisioner spec: - replicas: 1 + replicas: {{ .Values.provisioner.replicas }} + {{- with .Values.provisioner.strategy }} + strategy: + {{- toYaml . | nindent 4 }} + {{- end }} selector: matchLabels: {{- include "deer-flow.selectorLabels" . | nindent 6 }} diff --git a/deploy/helm/deer-flow/templates/provisioner-pdb.yaml b/deploy/helm/deer-flow/templates/provisioner-pdb.yaml new file mode 100644 index 0000000000..226107ce82 --- /dev/null +++ b/deploy/helm/deer-flow/templates/provisioner-pdb.yaml @@ -0,0 +1,26 @@ +{{- /* + PodDisruptionBudget for the sandbox provisioner. Rendered only while the + provisioner is enabled and runs more than one replica: with a single + replica a budget of 1 would block every node drain, and with several it + keeps at least one Pod answering the gateway's sandbox create/discover + calls while nodes are drained. Unlike the gateway there is no multiInstance + override, because nothing else in the chart renders differently per + provisioner replica count; scaling with kubectl alone leaves it unbudgeted. +*/ -}} +{{- $pdb := .Values.provisioner.podDisruptionBudget | default dict -}} +{{- if and .Values.provisioner.enabled $pdb.enabled (gt (int .Values.provisioner.replicas) 1) }} +apiVersion: policy/v1 +kind: PodDisruptionBudget +metadata: + name: {{ include "deer-flow.fullname" . }}-provisioner + namespace: {{ include "deer-flow.namespace" . }} + labels: + {{- include "deer-flow.labels" . | nindent 4 }} + app.kubernetes.io/component: provisioner +spec: + minAvailable: {{ include "deer-flow.pdbMinAvailable" (dict "value" $pdb.minAvailable "key" "provisioner.podDisruptionBudget.minAvailable") }} + selector: + matchLabels: + {{- include "deer-flow.selectorLabels" . | nindent 6 }} + app.kubernetes.io/component: provisioner +{{- end }} diff --git a/deploy/helm/deer-flow/values.yaml b/deploy/helm/deer-flow/values.yaml index a02b822ef6..feb779cf7c 100644 --- a/deploy/helm/deer-flow/values.yaml +++ b/deploy/helm/deer-flow/values.yaml @@ -65,8 +65,10 @@ gateway: maxUnavailable: 0 # PodDisruptionBudget, rendered only for a multi-instance gateway (replicas # > 1 or multiInstance: true; a budget of 1 on a single replica would block - # node drains). minAvailable falls back to 1 when unset; disable the budget - # rather than setting it to 0. + # node drains). minAvailable is an integer or a percentage string such as + # "50%" (quoted; a plain `int` cast would turn it into 0) and falls back to + # 1 when unset. 0 and "0%" fail the render -- a zero budget protects + # nothing -- so disable the budget instead. podDisruptionBudget: enabled: true minAvailable: 1 @@ -146,6 +148,36 @@ provisioner: # `existingSecret` when you supply one (external provisioner); otherwise # its start command defaults the variable to "", so the default `config` # below still loads even when a user-managed Secret lacks the key. + # Replica count. 1 is the conservative default, matching the gateway; >1 is + # safe as the provisioner stands: it keeps no state of its own between + # requests -- the sandbox Pods and Services it labels (`app=deer-flow-sandbox`, + # `sandbox-id`) are the only registry, every handler reads them back from + # the API server, and create tolerates the 409 AlreadyExists a concurrent + # creator on another replica produces (the race several gateway Pods already + # run against one provisioner). NodePorts are allocated by the API server, + # never by the provisioner. The `provisioner` Service spreads requests over + # every replica, so with 1 replica this is the last single point of failure + # in a multi-replica gateway deployment: while its Pod restarts or its node + # drains no sandbox can be created or discovered. Scaling it changes nothing + # else in the chart, so there is no multiInstance switch here. + replicas: 1 + # Rollout strategy. Surge-then-drain keeps one Pod answering sandbox calls + # while a peer starts; a replica has nothing to hand over beyond its + # in-flight requests, so the default termination grace period is enough. + strategy: + type: RollingUpdate + rollingUpdate: + maxSurge: 1 + maxUnavailable: 0 + # PodDisruptionBudget, rendered only for replicas > 1 (a budget of 1 on a + # single replica would block node drains). minAvailable is an integer or a + # percentage string such as "50%" and falls back to 1 when unset; 0 and + # "0%" fail the render -- a zero budget protects nothing -- so disable the + # budget instead. Scaling with `kubectl scale` instead of `replicas` leaves + # the budget unrendered. + podDisruptionBudget: + enabled: true + minAvailable: 1 resources: requests: cpu: 50m diff --git a/docker/provisioner/README.md b/docker/provisioner/README.md index 1b964053d3..a7f029e0cb 100644 --- a/docker/provisioner/README.md +++ b/docker/provisioner/README.md @@ -398,6 +398,37 @@ docker exec deer-flow-gateway curl -s $SANDBOX_URL/v1/sandbox - In ClusterIP mode, test from the backend Pod: `curl http://sandbox-XXX-svc.deer-flow.svc.cluster.local:8080/v1/sandbox` - Check `NODE_HOST` for NodePort deployments, or cluster DNS / NetworkPolicy / service mesh rules for ClusterIP deployments +## Running several replicas + +The provisioner keeps no state of its own between requests, so several +replicas can serve the same backend behind one load-balanced Service (the Helm +chart's `provisioner.replicas`; compose runs a single container): + +- The sandbox Pods and Services it creates carry the labels it later queries + (`app=deer-flow-sandbox`, `sandbox-id`). They are the only registry: create, + status, list and delete all read the Kubernetes API, never a process-local + table, so any replica can answer for a sandbox another replica created. +- `POST /api/sandboxes` is idempotent across replicas. Two creates for the same + `sandbox_id` that race on different replicas both succeed: the loser's Pod + and Service creates return `409 AlreadyExists`, which the handler treats like + its own success before reading the winner's objects back (including + `MAX_SHELL_SESSIONS`, so a lower-capacity winner is still reported to the + Gateway as `409`). Several Gateway Pods already run this race against a + single provisioner. +- `DELETE /api/sandboxes/{sandbox_id}` tolerates `404`, so two replicas + deleting the same sandbox both return success. +- NodePorts are allocated by the API server when the Service is created; the + provisioner only reads the assigned port back. +- No replica runs a background loop (there is no reaper or cache), and + `/api/capabilities` is derived from environment variables every replica of + one Deployment shares. + +The one startup write is `_ensure_namespace()`: when `K8S_NAMESPACE` does not +exist yet, two replicas starting together can both see `404` and race to create +it; the loser's `409` is not caught, so that process exits and is restarted into +the now-existing namespace. Under the Helm chart the provisioner runs inside the +namespace it manages, so this path never runs there. + ## Security Considerations 1. **HostPath Volumes**: The provisioner mounts host directories into sandbox Pods by default. Ensure these paths contain only trusted data. For production, prefer PVC-based volumes (set `SKILLS_PVC_NAME` and `USERDATA_PVC_NAME`) to avoid node-specific data loss risks.