Support configurable replica count in valkey-cluster-init job#17
Open
the-mentor wants to merge 11 commits into
Open
Support configurable replica count in valkey-cluster-init job#17the-mentor wants to merge 11 commits into
the-mentor wants to merge 11 commits into
Conversation
* wip - publish release * release yaml
* router: add pluggable networking providers * make websockets work in agw, make agw default in install script Signed-off-by: Peter Jausovec <peter.jausovec@solo.io> --------- Signed-off-by: Peter Jausovec <peter.jausovec@solo.io> Co-authored-by: John Howard <john.howard@solo.io> Co-authored-by: Peter Jausovec <peter.jausovec@solo.io>
* enable websockets (kagent-dev#4) Signed-off-by: Peter Jausovec <peter.jausovec@solo.io> Co-authored-by: Peter Jausovec <peter.jausovec@solo.io> * feat: allow running with vanilla k8s - add a helm chart - allow JWT auth instead of mTLS * update helm chart images * fix rbac. note that JWT verification is not cached and might not work on some k8s distributions that not expose the JWKS * fix: add chart boilerplate headers * fix: support jwt helm install on plain kind * feat: add substrate crds helm chart * feat: make jwt helm installs standalone * fix: make helm defaults cloud-neutral * fix: sync crd chart templates * fix: use agentgateway in helm chart * fix: update agentgateway install overlays * fix: project agentgateway tls key separately --------- Signed-off-by: Peter Jausovec <peter.jausovec@solo.io> Co-authored-by: Eitan Yarmush <eitan.yarmush@solo.io> Co-authored-by: Peter Jausovec <peter.jausovec@solo.io>
…ev#13) * atenet: make system namespace and component Service names configurable The dns-controller (`atenet dns`) and router (`atenet router`) hardcoded the substrate namespace ("ate-system") and the component Service names ("atenet-router", "dns") from the canonical install manifests under `manifests/ate-install/`. Deployments that deviate from that layout — running in a different namespace, renaming the Services, or composing substrate into a larger install that rewrites resource names — silently break: the dns-controller can't find atenet-router, the router can't find itself for /statusz, and the cluster's actor DNS never gets patched. Expose the relevant names as flags on the cobra commands and as fields on `dns.Controller` / `router.RouterConfig`. Defaults match the values in `manifests/ate-install/` so existing deployments are unaffected: atenet dns: --system-namespace (default "ate-system") --router-service-name (default "atenet-router") --dns-service-name (default "dns") atenet router: --router-service-name (default "atenet-router") * ateapi: make atelet namespace configurable via --atelet-namespace The atelet pod informer hardcoded `ateletNamespace = "ate-system"`, so ate-api-server could only locate atelet pods in that namespace. Deployments that run atelet elsewhere — an alternative install layout or a larger composition that relocates substrate components — leave the informer's cache empty and ResumeActor fails with `found 0 atelet pods on node "<node>", expected 1`. Promote the constant to an exported default and accept the namespace as a parameter to `AteletInformer`. Add an `--atelet-namespace` flag on the ateapi binary (default DefaultAteletNamespace) that callers override when needed. * chart: pass system namespace and Service names to dns-controller and router Wire the new flags added in the previous commit through the Helm templates so the canonical-render defaults are overridden when the chart is used as a subchart (e.g. the kagent-enterprise composition where substrate.fullname prefixes all component Service names). For atenet-dns the dns-controller now receives: --system-namespace={{ .Release.Namespace }} --router-service-name={{ include "substrate.fullname" (list "atenet-router" .) }} --dns-service-name={{ include "substrate.fullname" (list "dns" .) }} For atenet-router the /statusz lookup gets: --router-service-name={{ include "substrate.fullname" (list "atenet-router" .) }} When the release name equals the chart name ("substrate") these expand to the canonical bare names, preserving existing behavior for top-level installs. * chart: pass --atelet-namespace to ate-api-server Wire the new ateapi flag from the previous commit through the chart so the atelet pod informer watches the chart's release namespace by default. Canonical render (release name "substrate" in namespace "ate-system") still produces "--atelet-namespace=ate-system", so behavior is unchanged for top-level installs. * chart: regenerate manifests/ate-install/ from current Helm chart Re-runs `make helm-template` so the checked-in render matches the chart. Brings in rustfs.yaml, the s3-backed atelet storage envvars, the trimmed valkey manifest, and drops the no-longer-templated sandboxconfig-gvisor and sandboxconfig-validation manifests. `make verify-helm-template` now passes. * review: centralize install defaults, derive atelet namespace from POD_NAMESPACE Addresses review comments on agent-substrate#350: - New internal/installdefaults package owns SystemNamespace, RouterServiceName, DNSServiceName. dns, router, and controlapi/informer drop their duplicate Default* constants and reference installdefaults via the matching flag declarations and tests. - Drop the --atelet-namespace flag on ateapi. The namespace is now resolved at startup from the POD_NAMESPACE env var (Kubernetes' downward API), falling back to installdefaults.SystemNamespace for non-k8s invocations (tests, local dev). atelet and ateapi share a namespace in every supported deployment topology, so a separate knob was dead weight. * review: derive atenet's system namespace from POD_NAMESPACE Same rationale as the prior atelet-namespace change: atenet, atenet-router, and substrate's CoreDNS live in a single namespace in every supported deployment topology, so a separate --system-namespace flag was dead weight. Resolve from the POD_NAMESPACE env var (Kubernetes' downward API) with installdefaults.SystemNamespace as the fallback for non-k8s runs. --router-service-name and --dns-service-name stay as flags because a subchart deployment renames those Services with a release prefix, and the binary can't derive that from pod metadata. * review: NamespaceFromPodEnv helper, APIServiceName const, ateclient hardcodes Three follow-ups from the self-review: - Extract the POD_NAMESPACE-with-SystemNamespace-fallback pattern into installdefaults.NamespaceFromPodEnv() so ateapi and atenet share a single implementation (also makes a third call site one line instead of four if anyone needs one). - Add installdefaults.PodNamespaceEnv ("POD_NAMESPACE") and APIServiceName ("api") so the constant set covers every name in the canonical install layout that's referenced by Go code. - Route internal/ateclient/builder.go's previously-hardcoded "ate-system" and "api" lookups through installdefaults, so kubectl-ate's port-forward no longer bypasses the new single source of truth. ate-controller (ServiceAccount), ate-api-server-deployment (Deployment), and "api.ate-system.svc" (JWT audience) are still hardcoded but their configurability needs a real flag/discovery story and is out of scope for this PR. * chart: render ate-client ServiceAccount in every mode The JWT install overlay (manifests/ate-install/jwt) references ate-client.yaml as a top-level resource, but the chart previously guarded the SA behind {{ if eq .Values.auth.mode "jwt" }} so render-manifests.sh (mtls) never emitted it. That divergence broke verify-helm-template after merging the upstream JWT fix that added a hand-maintained manifests/ate-install/ate-client.yaml. The SA is harmless in mtls installs (unused), so render it unconditionally so the chart is the single source of truth.
…dev#14) ActorWorkflow.ResumeActor and SuspendActor used to derive their workflow ctx from the Redis lock TTL via acquireActorLock(ctx, id, 30s, 2s) — the workflow deadline and the lock TTL were a single 28s knob. That meant image pulls / restores that legitimately need more than 28s death-looped forever, while raising the knob also raised how long peers wait to retry an actor after a crashed ateapi replica. Split the two concerns: - Lock TTL stays short (30s constant, internal). Bounds peer failover. - Workflow deadline is a separate operator-configurable knob via the new --actor-workflow-deadline pflag (default 5m). Bounds a single Resume/Suspend. - A heartbeat goroutine refreshes the lock every lockTTL/3 (~10s) for the full workflow duration. On RefreshLock=false or any Redis error (peer stole the lock, Redis blip), the workflow ctx is cancelled with errLostActorLock as the cause so in-flight steps unwind cleanly and the mutual-exclusion invariant is preserved. - The release function stops the heartbeat (waits for goroutine exit) before best-effort ReleaseLock. Adds store.Interface.RefreshLock with a Redis CAS Lua script mirroring the existing ReleaseLock script.
The init job hardcoded `for i in 0 1 2 3 4 5` loops, assuming exactly 6 Valkey pods. This broke DNS resolution and cluster creation for any other `valkey.replicas` value. Inject the replica count from Helm and iterate over it with a POSIX-safe while loop so the job works for any count. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
EItanya
force-pushed
the
main
branch
2 times, most recently
from
eec90e9 to
0e3b4ed
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The
valkey-cluster-initJob hardcodedfor i in 0 1 2 3 4 5loops, assuming exactly 6 Valkey pods. Any othervalkey.replicasvalue would break:--cluster createnode list, so cluster creation would fail or omit nodes.Meanwhile the StatefulSet already scales via
.Values.valkey.replicas(default6), so the init job was the only thing pinned to 6.Changes
REPLICAS={{ .Values.valkey.replicas }}).forloops with POSIX-safewhileloops that iterate0 .. REPLICAS-1.Testing
helm template charts/substrate→ rendersREPLICAS=6(default, unchanged behavior).helm template charts/substrate --set valkey.replicas=3→ rendersREPLICAS=3with the loops iterating over it.🤖 Generated with Claude Code