watcher and api-server horizontal scaling - #1434
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
khrm
left a comment
There was a problem hiding this comment.
Can you please remove reference to internal jira in the title?
|
Just to confirm, this is still WIP, right? If so, please indicate it on the PR e.g. prefix the PR subject with |
Should I create an upstream github issue for this feature? |
IMO that's not needed, just for the sake of having one. The GitHub issue is a way for the users and contributors to communicate issues and features requests. This one is almost done, so no really need for it. |
There was a problem hiding this comment.
Pull request overview
Adds horizontal scaling for the Results API and watcher using headless services, gRPC round-robin balancing, and bucket-based watcher sharding.
Changes:
- Adds a three-replica API and watcher scaling component.
- Configures DNS-based gRPC balancing and TLS certificate generation.
- Adds deployment documentation and service-config validation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
docs/install.md |
Documents TLS setup for scaled deployments. |
config/overlays/ha-local-db/kustomization.yaml |
Adds an HA local-database overlay. |
config/components/horizontal-scaling/watcher-statefulset.yaml |
Defines the sharded watcher StatefulSet. |
config/components/horizontal-scaling/watcher-headless-service.yaml |
Adds watcher stable network identities. |
config/components/horizontal-scaling/README.md |
Documents scaling behavior and operation. |
config/components/horizontal-scaling/kustomization.yaml |
Composes and configures scaling resources. |
config/components/horizontal-scaling/generate-tls-cert.sh |
Generates SAN-enabled TLS certificates. |
config/components/horizontal-scaling/api-service-headless.yaml |
Exposes API pod endpoints through DNS. |
cmd/watcher/main.go |
Enables round-robin gRPC balancing. |
cmd/watcher/main_test.go |
Validates the default balancing configuration. |
Suppressed comments (2)
config/components/horizontal-scaling/watcher-statefulset.yaml:84
- These stateful-election variables make Knative assign each ordinal an unopposed, fixed bucket rather than a lease-backed bucket. If any watcher pod is unavailable, no surviving replica takes its shard, so reconciliations for that shard stop; this does not provide the high availability promised by the component. Use standard bucket leader election for failover, or explicitly make the no-failover behavior a documented constraint.
- name: STATEFUL_CONTROLLER_ORDINAL
valueFrom:
fieldRef:
fieldPath: metadata.name
- name: STATEFUL_SERVICE_NAME
config/components/horizontal-scaling/README.md:192
- This compatibility claim is incorrect.
logs-filepatches only the base watcher Deployment, so the replacement StatefulSet never receives-logs_api; it also mounts one ReadWriteOnce PVC into the API Deployment that this component scales to three replicas. A successful Kustomize patch does not make the combined components functional.
- **Other components**: Components like `logs-file` that patch the watcher Deployment continue to work. The base Deployment is scaled to 0 replicas (not deleted), so patches apply without error.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Please also add ha-base-only overlay (the same as the one you added without the local-db component) |
Done |
There was a problem hiding this comment.
Thank you for the PR. Few comments, nothing major.
Please squash the commits. There is no really need to keep piling them for every change, unless you really want to split the PR into multiple commits encapsulating different functionality. Even when you squash, the diff is visible in GitHub if someone is interested in what changed between two pushes.
Update the actual commit message as well as the PR description. See my comment below, the PR description should also include release note as well as kind label (it should be /kind feature in this case).
Update the doc with information how the metrics should be scraped when using that ha mode. Something like When using horizontal scaling, configure Prometheus to scrape individual pods (via PodMonitor or targeting the headless service) rather than the ClusterIP service. The ClusterIP only hits one random pod per scrape, giving an incomplete view. The headless services already expose the metrics port (9090), so they can be used as scrape targets directly..
| @@ -0,0 +1,50 @@ | |||
| /* | |||
| Copyright 2020 The Tekton Authors | |||
There was a problem hiding this comment.
| Copyright 2020 The Tekton Authors | |
| Copyright 2026 The Tekton Authors |
| When updating the watcher StatefulSet (new image, config change): | ||
|
|
||
| 1. Kubernetes performs a rolling update, replacing one pod at a time | ||
| 2. Knative demotes the terminating pod's buckets and promotes a different replica |
There was a problem hiding this comment.
I believe this is wrong. The bucket is left without an owner until the statefulset creates the replica with the respective ordinal.
|
|
||
| 1. Kubernetes performs a rolling update, replacing one pod at a time | ||
| 2. Knative demotes the terminating pod's buckets and promotes a different replica | ||
| 3. The new pod starts, cache syncs, and takes ownership of assigned buckets |
There was a problem hiding this comment.
| 3. The new pod starts, cache syncs, and takes ownership of assigned buckets | |
| 3. The new pod starts, cache syncs, and takes ownership of assigned bucket |
|
|
||
| ### DNS Re-Resolution Latency | ||
|
|
||
| gRPC re-resolves DNS every 30 minutes by default. When scaling the API Deployment up or down, existing watcher connections may take up to 30 minutes to discover new endpoints or remove old ones. |
There was a problem hiding this comment.
Can you please share the reference to that source code/doc? Checking vendor/google.golang.org/grpc/internal/resolver/dns/dns_resolver.go I see MinResolutionInterval = 30 * time.Second and ResolvingTimeout = 30 * time.Second but nothing about retry timeout or 30 minutes.
There was a problem hiding this comment.
Seems like this statement is also not correct, I'll update the documentation.
There was a problem hiding this comment.
updated in the last commit
|
|
||
| **Safe scale-down:** | ||
|
|
||
| Kubernetes deletes StatefulSet pods in reverse ordinal order (2, 1, 0). Knative promotes remaining replicas to take ownership of orphaned buckets via informer resync. No reconciliations are lost during scale-down. |
There was a problem hiding this comment.
I believe this is wrong, see my comment below.
| ctx, cancel := context.WithTimeout(ctx, 1*time.Minute) | ||
| defer cancel() | ||
| return grpc.DialContext(ctx, apiAddr, opts...) | ||
| return grpc.NewClient(apiAddr, opts...) |
There was a problem hiding this comment.
This is a significant change of behavior, because of the removal of grpc.WithBlock() which is not supported with grpc.NewClient. The client is no longer blocking, instead it returns immediately and relies on lazy connection establishment. I'm not against the change, but it should be very well documented both as part of the commit message (both the actual commit message and PR description) and the release notes (part of the PR description).
There was a problem hiding this comment.
I tested this manually and it created a bigger issue. The logs functionality is completely skipped because grpc.NewClient returns immediately and the DNS resolution is not complete yet. The message in the log Unable to inject logs client, logs will not be stored: rpc error: code = Unavailable desc = no children to pick from indicates that. I tested a fix which seems to be working:
diff --git a/cmd/watcher/main.go b/cmd/watcher/main.go
index 6b99887..624086e 100644
--- a/cmd/watcher/main.go
+++ b/cmd/watcher/main.go
@@ -191,6 +191,7 @@ func connectToAPIServer(apiAddr string, authMode string) (*grpc.ClientConn, erro
opts := []grpc.DialOption{
grpc.WithDefaultServiceConfig(defaultServiceConfig),
grpc.WithConnectParams(connectParams),
+ grpc.WithDefaultCallOptions(grpc.WaitForReady(true)),
}
// Add in additional credentials to requests if desired.
switch authMode {
this is also what https://github.com/grpc/grpc-go/blob/master/Documentation/anti-patterns.md recommends.
| ROOT="$(git rev-parse --show-toplevel)" | ||
|
|
||
| echo "Installing Tekton Pipelines..." | ||
| TEKTON_PIPELINE_CONFIG=${TEKTON_PIPELINE_CONFIG:-"https://storage.googleapis.com/tekton-releases/pipeline/latest/release.yaml"} |
There was a problem hiding this comment.
storage.googleapis.com is the old storage and will result the installation of very old version of Tekton Pipeline. test/e2e/01-install.sh has the actual updated host.
| @@ -0,0 +1,85 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
There is a lot of duplication (and some introduced errors) between this newly added script and the existing one test/e2e/01-install.sh. here is an Ai generated optimization idea. Please verify before using.
Script duplication: 01-install.sh, 01-install-ha.sh, generate-tls-cert.sh
The three scripts share most of their logic. 01-install-ha.sh is a near-complete copy of 01-install.sh with only a few differences:
- Extra SAN in the TLS cert (
api-service-headless) - Different kustomize overlay (
kustomize-ha/vskustomize/) - Different pod wait strategy (label-based vs Deployment-based)
generate-tls-cert.sh duplicates the OpenSSL + LibreSSL cert generation a third time.
Suggestion
Collapse into a single 01-install.sh that accepts a mode parameter:
#!/bin/bash
set -e
MODE="${1:-standard}" # "standard" or "ha"
export KO_DOCKER_REPO=${KO_DOCKER_REPO:-"kind.local"}
export KIND_CLUSTER_NAME=${KIND_CLUSTER_NAME:-"tekton-results"}
export SA_TOKEN_PATH=${SA_TOKEN_PATH:-"/tmp/tekton-results/tokens"}
export SSL_CERT_PATH=${SSL_CERT_PATH:="/tmp/tekton-results/ssl"}
ROOT="$(git rev-parse --show-toplevel)"
# --- Install Tekton Pipelines (shared) ---
TEKTON_PIPELINE_CONFIG=${TEKTON_PIPELINE_CONFIG:-"https://infra.tekton.dev/tekton-releases/pipeline/latest/release.yaml"}
kubectl apply --filename "${TEKTON_PIPELINE_CONFIG}"
kubectl wait --for=condition=ready pod -l app=tekton-pipelines-controller -n tekton-pipelines --timeout=120s
kubectl wait --for=condition=ready pod -l app=tekton-pipelines-webhook -n tekton-pipelines --timeout=120s
# --- DB secret (shared) ---
kubectl create secret generic tekton-results-postgres \
--namespace="tekton-pipelines" \
--from-literal=POSTGRES_USER=postgres \
--from-literal=POSTGRES_PASSWORD="$(openssl rand -base64 20)" || true
# --- TLS cert generation — delegate to the shared helper ---
export NAMESPACE="tekton-pipelines"
export OUTPUT_DIR="${SSL_CERT_PATH}"
export SECRET_NAME="tekton-results-tls"
if [ "$MODE" = "ha" ]; then
# The generate-tls-cert.sh script already includes both the regular
# and headless service names in the SAN.
"${ROOT}/config/components/horizontal-scaling/generate-tls-cert.sh"
else
# Standard mode: single SAN only.
# Could extend generate-tls-cert.sh to accept EXTRA_SANS, or
# keep the inline openssl call for the simple case.
mkdir -p "${SSL_CERT_PATH}"
altNames="DNS:tekton-results-api-service.tekton-pipelines.svc.cluster.local"
if [ "${SSL_INCLUDE_LOCALHOST:-false}" = "true" ]; then
altNames+=",DNS:localhost"
fi
openssl req -x509 \
-newkey rsa:4096 \
-keyout "${SSL_CERT_PATH}/key.pem" \
-out "${SSL_CERT_PATH}/cert.pem" \
-days 365 -nodes \
-subj "/CN=tekton-results-api-service.tekton-pipelines.svc.cluster.local" \
-addext "subjectAltName = ${altNames}"
kubectl create secret tls -n tekton-pipelines tekton-results-tls \
--cert="${SSL_CERT_PATH}/cert.pem" \
--key="${SSL_CERT_PATH}/key.pem" || true
fi
# --- Deploy (mode-specific overlay) ---
if [ "$MODE" = "ha" ]; then
KUSTOMIZE_DIR="${ROOT}/test/e2e/kustomize-ha"
else
KUSTOMIZE_DIR="${ROOT}/test/e2e/kustomize"
fi
extra_ko_params="linux/$(go env GOARCH)"
kubectl kustomize "$KUSTOMIZE_DIR" | ko apply --platform="$extra_ko_params" -f -
# --- SA tokens (shared) ---
mkdir -p "${SA_TOKEN_PATH}"
for sa in all-namespaces-read-access single-namespace-read-access all-namespaces-admin-access all-namespaces-impersonate-access; do
kubectl create token "$sa" > "${SA_TOKEN_PATH}/${sa}"
done
# --- Wait for pods (shared, label-based works for both modes) ---
kubectl wait --for=condition=ready pod -l app.kubernetes.io/name=tekton-results-api -n tekton-pipelines --timeout=300s
kubectl wait --for=condition=ready pod -l app.kubernetes.io/name=tekton-results-watcher -n tekton-pipelines --timeout=300s
kubectl wait --for=condition=ready pod -l app.kubernetes.io/name=tekton-results-postgres -n tekton-pipelines --timeout=120sThen update e2e.sh to call:
if [ "$1" == "--ha" ]; then
${REPO}/test/e2e/01-install.sh ha
else
${REPO}/test/e2e/01-install.sh
fiTo fully eliminate the third copy, generate-tls-cert.sh could be extended to support an EXTRA_SANS env var so it handles both the single-SAN and multi-SAN cases, making it usable as the sole cert generation entry point for all paths.
There was a problem hiding this comment.
Refactored the scripts
| echo "" | ||
| kubectl get pods -n tekton-pipelines | grep tekton-results | ||
| echo "" | ||
| echo "Run E2E tests: go test -v --tags=e2e,e2e_ha -run TestHorizontalScaling ." |
There was a problem hiding this comment.
Run E2E tests: go test -v --tags=e2e,e2e_ha -run TestHorizontalScaling .
[ results] -> go test -v --tags=e2e,e2e_ha -run TestHorizontalScaling .
.
no Go files in /tmp/results
FAIL . [setup failed]
FAIL
that should be "Run E2E tests: go test -v --tags=e2e,e2e_ha -run TestHorizontalScaling ./..." or something stricter if you want to limit it to specific directory.
4080763 to
18af2aa
Compare
|
/hold |
18af2aa to
bd049df
Compare
|
/kind feature |
320fa6d to
e0cc739
Compare
| topologyKey: kubernetes.io/hostname | ||
| weight: 100 | ||
| serviceAccountName: watcher | ||
| containers: |
There was a problem hiding this comment.
Missing liveness/readiness probes on the watcher StatefulSet:
We should define a livenessProbe or readinessProbe. Knative's 'shareadmin' already serves /heath and /readiness on port 8080 inside the watcher, but the StatefulSet spec never tells Kubernetes to check them. In the HA setup with bucket sharding, this means if one watcher replica gets stuck, K8s will keep reporting it as "Ready 1/1". That replica's bucket of TR/PR will silently stop being reconciled ( no alert or detection ).
IMO we can fix this by adding livenessProbe or readinessProbe to the container spec ( port 8080 also needs to be added )
There was a problem hiding this comment.
While I agree with that, it's a pre-existing state and it applies to the Deployment as well (when we deploy as single replica). So IMO it makes sense to fix that in a separate PR which fixes the issue for both Deployment and StatefulSet. We can open a separate GitHub issue for it, so it's not forgotten.
There was a problem hiding this comment.
Yes, that makes sense to have a different PR for this.
| - name: METRICS_DOMAIN | ||
| value: tekton.dev/results | ||
| - name: TEKTON_RESULTS_API_SERVICE | ||
| value: dns:///tekton-results-api-service-headless.tekton-pipelines.svc.cluster.local:8080 |
There was a problem hiding this comment.
The TEKTON_RESULTS_API_SERVICE env var hardcodes tekton-pipelines in the DNS name. The base Deployment has the same hardcoding, but it gets patched by the operator at deploy time. Since this StatefulSet is a new resource, the operator would also need to be updated to patch it for non-default namespaces.
There was a problem hiding this comment.
Good call. I'll add to that, that the operator should not just patch, but ultimately manage that new Service object and the Route associated with it. Here we do not have that capability to change dynamically the type of deployment (single replicas vs scaleable deployments). The work on the operator side can be managed as a separate issue on the operator repo. We need that PR merged first and the next version of Results including that change to be referenced by the operator because of the change in the Watcher main file.
|
PodDisruptionBudget (PDB) for HA components can be added: |
cf93440 to
48639ae
Compare
Added PodDisruptionBudget to the watcher StatefulSet. |
|
I have reviewed this PR and the changes looks good to me. /lgtm |
df0a906 to
9596fef
Compare
| ### grpc.NewClient Migration | ||
|
|
||
| The watcher uses `grpc.NewClient` (introduced in gRPC-Go 1.58) instead of the deprecated `grpc.DialContext`. | ||
|
|
||
| **Key differences:** | ||
|
|
||
| | `grpc.DialContext` (deprecated) | `grpc.NewClient` (current) | | ||
| |--------------------------------|---------------------------| | ||
| | Blocking by default | Non-blocking - returns immediately | | ||
| | Accepts `context.Context` | No context parameter | | ||
| | Supports `grpc.WithBlock()` | Does not support `grpc.WithBlock()` | | ||
| | Connection established during dial | Connections established lazily on first RPC | |
There was a problem hiding this comment.
This is more of a implementation detail, while I see this doc as a customer focused documentation. IMO we can just drop that.
| ### grpc.NewClient Migration | |
| The watcher uses `grpc.NewClient` (introduced in gRPC-Go 1.58) instead of the deprecated `grpc.DialContext`. | |
| **Key differences:** | |
| | `grpc.DialContext` (deprecated) | `grpc.NewClient` (current) | | |
| |--------------------------------|---------------------------| | |
| | Blocking by default | Non-blocking - returns immediately | | |
| | Accepts `context.Context` | No context parameter | | |
| | Supports `grpc.WithBlock()` | Does not support `grpc.WithBlock()` | | |
| | Connection established during dial | Connections established lazily on first RPC | |
| } | ||
|
|
||
| main | ||
| main $@ |
There was a problem hiding this comment.
| main $@ | |
| main "$@" |
There was a problem hiding this comment.
@enarha Thanks for pointing this out, I missed it. Yes it should be main "$@"
There was a problem hiding this comment.
I tried this locally and this works both ways but thanks anyways, I'll update it.
There was a problem hiding this comment.
I tried this locally and this works both ways but thanks anyways, I'll update it.
It works with the existing ha parameter, but that could break when the shell performs word splitting and glob expansion with other parameters we add to it. This is a shell correctness issue.
| 4. The watcher only reconciles the resource if the computed bucket matches one of its assigned buckets | ||
| 5. Resources are deterministically assigned to exactly one watcher, eliminating duplicate reconciliation | ||
|
|
||
| Note: Deprecated blocking grpc.DialContext client connection function has been replaced with newer grpc.NewClient function. grpc.WithBlock is uncompatible with the new scaling functionality and has been removed and replaced with grpc.WithConnectParams(connectParams) to use new scaling parameters; |
There was a problem hiding this comment.
typo
| Note: Deprecated blocking grpc.DialContext client connection function has been replaced with newer grpc.NewClient function. grpc.WithBlock is uncompatible with the new scaling functionality and has been removed and replaced with grpc.WithConnectParams(connectParams) to use new scaling parameters; | |
| Note: Deprecated blocking grpc.DialContext client connection function has been replaced with newer grpc.NewClient function. grpc.WithBlock is incompatible with the new scaling functionality and has been removed and replaced with grpc.WithConnectParams(connectParams) to use new scaling parameters; |
Assisted-by: Claude Signed-off-by: obetsun <obetsun@redhat.com>
900e93e to
0467a23
Compare
Changes
tekton-results-watcherwith 3 replicas to be used to scale Watcher pods instead of Deployment ;tekton-results-apiscaled to 3 replicas;tekton-results-api-service-headlesshas been added withclusterIP: Nonefor load balancing to be able to use DNS resolution;Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
Assisted-by: Claude