Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 7 additions & 1 deletion deploy/helm/sandbox-env/Chart.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,12 @@ description: |
releases coexist in the shared `agent-sandbox-system` namespace.
Requires the sandbox-operator chart to already be installed.
type: application
# 0.10.0: preview-gateway resilience knobs, all opt-in and default-off —
# previewGateway.autoscaling (HPA over the Istio-generated Deployment),
# previewGateway.pdb (PDB over the generated pods), previewGateway.retries
# (EnvoyFilter retrying GET-only 502s from the sandbox daemon, hard-scoped
# to this gateway's workloadSelector). No behavior change for existing
# consumers.
# 0.9.0: org-fs is mandatory — removed the `orgFs.enabled` + `hostUsers`
# toggles (sidecar always deployed, hostUsers fixed true). Minor bump:
# behavior change for consumers that ran with org-fs disabled / userns
Expand All @@ -19,7 +25,7 @@ type: application
# 0.8.0: org-fs sidecar (orgFs.*) — privileged rclone mounter + main-container
# mounts. NOTE: the publish workflow skips existing versions, so this MUST
# stay ahead of the latest published chart (0.7.5 at time of writing).
version: 0.9.0
version: 0.10.0
# appVersion tracks the studio-sandbox image version (image.tag default).
appVersion: "0.5.7"
kubeVersion: ">=1.30.0-0"
23 changes: 23 additions & 0 deletions deploy/helm/sandbox-env/templates/_helpers.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,29 @@ atomic and gives a pointer to the right install command.
{{- fail "sandbox-env: previewGateway.enabled=true with tlsTermination=gateway requires cert-manager (cert-manager.io/v1). Install: helm install cert-manager jetstack/cert-manager -n cert-manager --create-namespace --set crds.enabled=true — or set previewGateway.tlsTermination=loadBalancer to terminate TLS at the cloud LB instead." -}}
{{- end }}
{{- end }}
{{/* All three resilience knobs assume Istio's auto-deploy conventions:
the HPA targets the generated Deployment by its Istio-specific name
(`<previewName>-istio`), the PDB selects pods by the label Istio's
controller stamps on them, and the retry EnvoyFilter is an Istio CRD
outright. Under another gatewayClassName the HPA/PDB would render but
silently bind to nothing — fail loudly instead. */}}
{{- range tuple "autoscaling" "pdb" "retries" }}
{{- if and (get $.Values.previewGateway .).enabled (ne $.Values.previewGateway.gatewayClassName "istio") }}
{{- fail (printf "sandbox-env: previewGateway.%s.enabled=true requires gatewayClassName=istio — it attaches to the Deployment/pods/EnvoyFilter conventions of Istio's gateway auto-deploy (got %q)" . $.Values.previewGateway.gatewayClassName) -}}
{{- end }}
{{- end }}
{{- if .Values.previewGateway.retries.enabled }}
{{- if not (.Capabilities.APIVersions.Has "networking.istio.io/v1alpha3") }}
{{- fail "sandbox-env: previewGateway.retries.enabled=true requires Istio (networking.istio.io/v1alpha3 EnvoyFilter CRD not found)." -}}
{{- end }}
{{/* In Istio's root namespace (istio-system by default) an EnvoyFilter's
workloadSelector matches labels across ALL namespaces instead of just
its own — the namespace half of the scoping barrier disappears. Run
the preview gateway in a dedicated namespace to enable retries. */}}
{{- if eq .Values.previewGateway.namespace "istio-system" }}
{{- fail "sandbox-env: previewGateway.retries.enabled=true is not allowed with previewGateway.namespace=istio-system — a root-namespace EnvoyFilter selects workloads cluster-wide. Move the preview gateway to a dedicated namespace (e.g. agent-sandbox-system)." -}}
{{- end }}
{{- end }}
{{- end }}
{{- end }}

Expand Down
30 changes: 30 additions & 0 deletions deploy/helm/sandbox-env/templates/sandbox-preview-hpa.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
{{- if and .Values.previewGateway.enabled .Values.previewGateway.autoscaling.enabled }}
{{- $previewName := include "sandbox-env.previewName" . }}
# HPA over the Deployment that Istio's gateway controller generates for the
# preview Gateway (`<previewName>-istio`). This is the Istio-documented way
# to scale an auto-deployed Gateway API gateway: the controller never writes
# `spec.replicas` after creation, so the HPA owns replica count without
# conflict. Scoped by namespace + target name — it cannot reach any other
# gateway (the sites gateway lives in istio-system under a different name).
apiVersion: autoscaling/v2
kind: HorizontalPodAutoscaler
metadata:
name: {{ $previewName }}-istio

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2: Autoscaling can silently be ineffective when previewGateway.gatewayClassName is not istio, because the HPA target Deployment name is fixed to -istio. Consider gating previewGateway.autoscaling.enabled to Istio (validation) or deriving the target from the selected gateway controller instead of hard-coding the suffix.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At deploy/helm/sandbox-env/templates/sandbox-preview-hpa.yaml, line 12:

<comment>Autoscaling can silently be ineffective when `previewGateway.gatewayClassName` is not `istio`, because the HPA target Deployment name is fixed to `-istio`. Consider gating `previewGateway.autoscaling.enabled` to Istio (validation) or deriving the target from the selected gateway controller instead of hard-coding the suffix.</comment>

<file context>
@@ -0,0 +1,30 @@
+apiVersion: autoscaling/v2
+kind: HorizontalPodAutoscaler
+metadata:
+  name: {{ $previewName }}-istio
+  namespace: {{ .Values.previewGateway.namespace }}
+  labels:
</file context>

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed — extended the existing retries-only validation to all three knobs: autoscaling/pdb/retries.enabled=true each fail at template time when gatewayClassName != istio. PDB had the same issue (the gateway.networking.k8s.io/gateway-name pod label is also Istio's controller convention), so it's gated too. Deriving the generated Deployment name per controller isn't worth it here — every other controller has different generated-workload conventions, and the chart's data path (per-claim HTTPRoutes minted by mesh) is only exercised against Istio anyway.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks — the Istio-specific boundary is clear, and the PDB/gateway-name detail is useful. The original autoscaling point still stands, but the broader contract is that these preview-gateway resilience knobs should be gated together to Istio-only conventions.

Thanks for the feedback! I've saved this as a new learning to improve future reviews.

namespace: {{ .Values.previewGateway.namespace }}
labels:
{{- include "sandbox-env.sandboxPreviewLabels" . | nindent 4 }}
spec:
scaleTargetRef:
apiVersion: apps/v1
kind: Deployment
name: {{ $previewName }}-istio
minReplicas: {{ .Values.previewGateway.autoscaling.minReplicas }}
maxReplicas: {{ .Values.previewGateway.autoscaling.maxReplicas }}
metrics:
- type: Resource
resource:
name: cpu
target:
type: Utilization
averageUtilization: {{ .Values.previewGateway.autoscaling.targetCPUUtilizationPercentage }}
{{- end }}
22 changes: 22 additions & 0 deletions deploy/helm/sandbox-env/templates/sandbox-preview-pdb.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{{- if and .Values.previewGateway.enabled .Values.previewGateway.pdb.enabled }}
{{- $previewName := include "sandbox-env.previewName" . }}
# PDB over the pods of the Istio-generated preview gateway Deployment,
# selected by the `gateway-name` label the controller stamps on them. With
# minAvailable 1 and a single replica, voluntary evictions (Karpenter
# consolidation/drift, node drains) are blocked outright — the gateway is
# the only ingress for every preview URL, so an eviction is a full preview
# outage. Spot interruptions and node failures are NOT covered by a PDB;
# raise previewGateway.autoscaling.minReplicas to 2 for that.
apiVersion: policy/v1
kind: PodDisruptionBudget
metadata:
name: {{ $previewName }}-istio
namespace: {{ .Values.previewGateway.namespace }}
labels:
{{- include "sandbox-env.sandboxPreviewLabels" . | nindent 4 }}
spec:
minAvailable: {{ .Values.previewGateway.pdb.minAvailable }}
selector:
matchLabels:
gateway.networking.k8s.io/gateway-name: {{ $previewName }}
{{- end }}
60 changes: 60 additions & 0 deletions deploy/helm/sandbox-env/templates/sandbox-preview-retry.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
{{- if and .Values.previewGateway.enabled .Values.previewGateway.retries.enabled }}
{{- $previewName := include "sandbox-env.previewName" . }}
{{- $retries := .Values.previewGateway.retries }}
# Retry policy for the preview data path. Istio's built-in default retry
# policy covers connection-level failures only — an upstream that ANSWERS
# 502 is returned to the client as-is. On this path that 502 is the sandbox
# daemon reporting "dev server (port 3000) not up yet" during boot/restart,
# so a short, spaced retry budget absorbs the blip instead of surfacing it.
#
# Scope: the workloadSelector below is HARD-CODED to this gateway's
# `gateway-name` label and the resource lands in the gateway's namespace —
# double barrier (namespace + label), so this filter can never apply to any
# other Envoy in the mesh. validations.yaml rejects rendering it into
# istio-system, where root-namespace EnvoyFilter semantics would make the
# label selector match cluster-wide.
#
# Safety properties of the policy:
# - GET-only (`retriable_request_headers` on :method): no request-body
# buffering, no replayed mutations.
# - 503 is intentionally not retried: the daemon's 503 is the
# auto-refreshing "Connecting…" page — a response the user should see
# immediately, not after a retry budget.
#
# EnvoyFilter is v1alpha3 and coupled to Envoy internals — smoke-test on
# stg after every Istio upgrade (see PR notes / values.yaml).
apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
name: {{ $previewName }}-retry
namespace: {{ .Values.previewGateway.namespace }}
labels:
{{- include "sandbox-env.sandboxPreviewLabels" . | nindent 4 }}
spec:
workloadSelector:
labels:
gateway.networking.k8s.io/gateway-name: {{ $previewName }}
configPatches:
- applyTo: HTTP_ROUTE
match:
context: GATEWAY
patch:
operation: MERGE
value:
route:
retry_policy:
retry_on: {{ $retries.retryOn | quote }}
retriable_status_codes:
{{- range $retries.retriableStatusCodes }}
- {{ . }}
{{- end }}
num_retries: {{ $retries.attempts }}
per_try_timeout: {{ $retries.perTryTimeout }}
retry_back_off:
base_interval: {{ $retries.backoffBaseInterval }}
max_interval: {{ $retries.backoffMaxInterval }}
retriable_request_headers:
- name: ":method"
string_match:
exact: "GET"
{{- end }}
55 changes: 55 additions & 0 deletions deploy/helm/sandbox-env/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,61 @@ previewGateway:
annotations: {}
labels: {}

# ── gateway resilience (all opt-in) ──────────────────────────────────
# The Gateway controller (Istio) generates a Deployment named
# `agent-sandbox-preview-<envName>-istio` with a single replica and no
# autoscaling. The three blocks below harden that generated workload
# without owning it: HPA/PDB attach to the generated Deployment/pods by
# name/label (the documented pattern for Istio auto-deployed gateways —
# the controller does not manage `replicas`, so it never fights the HPA).

# HorizontalPodAutoscaler targeting the generated gateway Deployment.
# minReplicas 1 keeps today's footprint; raise to 2 for HA across node
# rotations (the single replica is a full preview-path outage while it
# reschedules).
autoscaling:
enabled: false
minReplicas: 1
maxReplicas: 3
targetCPUUtilizationPercentage: 80

# PodDisruptionBudget over the generated gateway pods. With minAvailable 1
# and a single replica this blocks voluntary evictions entirely (Karpenter
# consolidation/drift must wait) — which is the point; it does NOT protect
# against spot interruptions or node failure. Pair with autoscaling
# minReplicas 2 to make those a non-event too.
pdb:
enabled: false
minAvailable: 1

# Retry policy for the preview data path, applied via an Istio EnvoyFilter
# scoped to THIS gateway's pods only (hard-coded workloadSelector on the
# `gateway.networking.k8s.io/gateway-name` label — it can never widen to
# other gateways). Istio's default retry policy does not retry an upstream
# that answered 502, which is exactly what the sandbox daemon returns
# while the dev server inside the sandbox is down/restarting. Retries are
# gated to GET (no body buffering, no replayed mutations) and spaced with
# backoff so a 2-attempt budget bridges a short dev-server restart
# (~3-5s); a sandbox that is down for longer still 502s after the budget.
# Requires gatewayClassName=istio (EnvoyFilter is an Istio CRD).
retries:
enabled: false
# Total retry attempts after the initial request.
attempts: 2
# Per-attempt upstream timeout.
perTryTimeout: "5s"
# Envoy retry_on conditions. `retriable-status-codes` activates
# `retriableStatusCodes` below; the rest cover connection-level
# failures (reset, refused, connect timeout).
retryOn: "connect-failure,refused-stream,reset,retriable-status-codes"
# Upstream response codes to retry. 503 is intentionally NOT here: the
# daemon's 503 is the auto-refreshing "Connecting…" page — retrying it
# would only delay a response the user can already see.
retriableStatusCodes: [502]
# Exponential backoff between attempts.
backoffBaseInterval: "0.5s"
backoffMaxInterval: "2s"

# ── mesh cross-references ──────────────────────────────────────────────
# Tells this chart where the chart-deco-studio release for THIS
# environment runs so the RBAC RoleBinding, NetworkPolicy ingress
Expand Down
Loading