feat(self-managed): wire autoscaler observability#384
Open
borao wants to merge 6 commits into
Open
Conversation
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
📝 WalkthroughWalkthroughThe change adds function autoscaler metrics exposure and monitoring, introduces self-managed observability and autoscaler configuration, wires derived environment values, validates deployment prerequisites, conditionally deploys observability components and the autoscaler through Helmfile, and supports optional static NVCF API tokens from Vault. ChangesObservability and function autoscaler
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HelmfileTemplate
participant ObservabilityReleases
participant MetricsBackends
participant FunctionAutoscaler
HelmfileTemplate->>ObservabilityReleases: Validate settings and define releases
ObservabilityReleases->>MetricsBackends: Install enabled backends
ObservabilityReleases->>FunctionAutoscaler: Deploy when enabled
FunctionAutoscaler->>MetricsBackends: Depend on enabled metrics backends
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml`:
- Around line 57-70: Update the api monitor definition in values.yaml to target
the existing Service port named http instead of management-port. Leave the sis
monitor and other api settings unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e44c9508-d7db-4027-be45-82aec9f69ee4
📒 Files selected for processing (5)
deploy/helm/function-autoscaler/values.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
Comment on lines
57
to
70
| - name: api | ||
| enabled: true | ||
| path: /actuator/prometheus | ||
| port: management-port | ||
| selector: | ||
| app.kubernetes.io/name: helm-nvcf-api | ||
| - name: sis | ||
| enabled: true | ||
| namespaces: | ||
| - sis | ||
| path: /actuator/prometheus | ||
| port: management-port | ||
| selector: | ||
| app.kubernetes.io/name: helm-nvcf-sis |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm api/sis services expose a named 'management-port' and inspect metrics gating.
fd -t f -e yaml -e gotmpl -e tpl . deploy/helm 2>/dev/null | xargs rg -nP -C2 'management-port|actuator/prometheus' 2>/dev/null
rg -nP -C2 'MANAGEMENT_PROMETHEUS_METRICS_EXPORT_ENABLED|observability\.metrics\.enabled' deploy/stacks/self-managedRepository: NVIDIA/nvcf
Length of output: 4962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the observability monitor chart and the api/sis service templates.
fd -t f 'values.yaml' deploy/stacks/observability/charts/nvcf-default-monitors deploy/helm 2>/dev/null
printf '\n---\n'
fd -t f -g 'deployment*.yaml' -g 'service*.yaml' deploy/helm 2>/dev/null | sort
printf '\n--- nvcf-default-monitors values excerpt ---\n'
sed -n '1,140p' deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml
printf '\n--- api/sis service/deployment excerpts ---\n'
for f in \
deploy/helm/api-keys-colocated/api-keys/templates/deployment.yaml \
deploy/helm/api-keys-colocated/api-keys/templates/service.yaml \
deploy/helm/api-keys-colocated/sis/templates/deployment.yaml \
deploy/helm/api-keys-colocated/sis/templates/service.yaml
do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,180p' "$f"
echo
fi
done
printf '\n--- metrics gating references ---\n'
rg -n -C2 'MANAGEMENT_PROMETHEUS_METRICS_EXPORT_ENABLED|MANAGEMENT_METRICS_ENABLE_ALL|observability\.metrics\.enabled|management-port' deploy/helm deploy/stacks/self-managed deploy/stacks/observability 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 16110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the exact files and line ranges implied by the review comment.
echo '--- deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml ---'
nl -ba deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml | sed -n '45,90p'
echo
echo '--- deploy/stacks/self-managed/global.yaml.gotmpl ---'
nl -ba deploy/stacks/self-managed/global.yaml.gotmpl | sed -n '332,345p'
echo
echo '--- api-keys service deployment/service ---'
for f in \
deploy/helm/api-keys-colocated/api-keys/templates/deployment.yaml \
deploy/helm/api-keys-colocated/api-keys/templates/service.yaml \
deploy/helm/api-keys-colocated/sis/templates/deployment.yaml \
deploy/helm/api-keys-colocated/sis/templates/service.yaml
do
if [ -f "$f" ]; then
echo "### $f"
nl -ba "$f" | sed -n '1,140p'
echo
fi
done
echo '--- search for management-port service names in relevant charts ---'
rg -n -C2 'name:\s*management-port|port:\s*management-port|management-port:' deploy/helm/api-keys-colocated deploy/stacks/observability deploy/stacks/self-managed 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the exact service definitions for api and sis, plus the chart mode wiring.
for f in \
deploy/helm/api-keys-colocated/api-keys/templates/service.yaml \
deploy/helm/api-keys-colocated/sis/templates/service.yaml \
deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml \
deploy/stacks/observability/charts/nvcf-default-monitors/templates/*.yaml
do
[ -f "$f" ] || continue
echo "### $f"
nl -ba "$f" | sed -n '1,220p'
echo
done
echo '--- grep for observability mode and metrics defaults ---'
rg -n -C3 'observability\.mode|metrics\.enabled|enabled:\s*true|enabled:\s*false' deploy/stacks/self-managed deploy/stacks/observability 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 257
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for the actual Service definitions and any management-port service name on api/sis charts.
rg -n -C3 'kind:\s*Service|name:\s*management-port|targetPort:\s*management-port|port:\s*management-port' \
deploy/helm/api-keys-colocated deploy/helm 2>/dev/null
printf '\n--- files mentioning helm-nvcf-sis ---\n'
rg -n -C2 'helm-nvcf-sis|app.kubernetes.io/name:\s*helm-nvcf-sis|sis' deploy/helm 2>/dev/null
printf '\n--- service templates under api-keys-colocated ---\n'
fd -t f 'service.yaml' deploy/helm/api-keys-colocated 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the sis chart/service definitions and limit output to the relevant lines.
fd -t f -g '*sis*' deploy/helm 2>/dev/null | sort
printf '\n--- sis references with service/port names ---\n'
rg -n -C2 'kind:\s*Service|name:\s*management-port|targetPort:|port: management-port|app.kubernetes.io/name:\s*helm-nvcf-sis|helm-nvcf-sis' deploy/helm 2>/dev/null | head -n 200Repository: NVIDIA/nvcf
Length of output: 15848
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect how the nvcf-default-monitors chart enables ServiceMonitors and what it keys off.
fd -t f -g '*.yaml' -g '*.yml' -g '*.gotmpl' deploy/stacks/observability/charts/nvcf-default-monitors 2>/dev/null | sort
printf '\n--- chart templates mentioning controlPlane/mode/enabled ---\n'
rg -n -C3 'controlPlane|install|existing|disabled|enabled:|namespaceSelector|ServiceMonitor' deploy/stacks/observability/charts/nvcf-default-monitors 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 11698
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the observability stack wiring for mode -> chart enablement.
rg -n -C3 'observability\.mode|nvcf-default-monitors|controlPlane\.enabled|default-monitors|install|existing|disabled' \
deploy/stacks/observability deploy/stacks/self-managed 2>/dev/nullRepository: NVIDIA/nvcf
Length of output: 42627
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find the sis service/deployment templates and any management port exposure.
rg -n -C3 'helm-nvcf-sis|management-port|managementPort|kind:\s*Service|ports:' deploy/helm/api-keys-colocated deploy/helm 2>/dev/null | head -n 250Repository: NVIDIA/nvcf
Length of output: 19568
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the exact service port names used by api-keys and sis-related templates.
for f in \
deploy/helm/api-keys-colocated/api-keys/templates/service.yaml \
deploy/helm/api-keys-colocated/api-keys/templates/deployment.yaml \
deploy/helm/api-keys-colocated/api-keys/values.yaml \
deploy/helm/api-keys-colocated/sis/templates/service.yaml \
deploy/helm/api-keys-colocated/sis/templates/deployment.yaml \
deploy/helm/api-keys-colocated/sis/values.yaml
do
[ -f "$f" ] || continue
echo "### $f"
sed -n '1,140p' "$f"
echo
doneRepository: NVIDIA/nvcf
Length of output: 7266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether api-keys or sis consume the global metrics-export env vars.
rg -n -C3 'MANAGEMENT_PROMETHEUS_METRICS_EXPORT_ENABLED|MANAGEMENT_METRICS_ENABLE_ALL|global\.observability\.metrics|actuator/prometheus|management-port' \
deploy/helm/api-keys-colocated deploy/helm 2>/dev/null | head -n 250Repository: NVIDIA/nvcf
Length of output: 5941
Retarget api to an existing Service port. The api Service only exposes http, so port: management-port won’t match unless that Service adds a named management-port port.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml` around
lines 57 - 70, Update the api monitor definition in values.yaml to target the
existing Service port named http instead of management-port. Leave the sis
monitor and other api settings unchanged.
borao
enabled auto-merge
July 24, 2026 17:23
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml`:
- Around line 75-79: Update the invocation-service monitor configuration in
nvcf-default-monitors values so its enabled state matches an available metrics
Service endpoint: add and wire a metrics port for helm-nvcf-invocation-service
if metrics are exposed there, or set enabled to false while no metrics Service
port exists. Preserve the monitor’s metrics port/path defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ee8c0f77-cf66-467a-974a-e640323718fa
📒 Files selected for processing (4)
deploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
🚧 Files skipped from review as they are similar to previous changes (3)
- deploy/stacks/self-managed/global.yaml.gotmpl
- deploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
- deploy/stacks/self-managed/environments/base.yaml
Comment on lines
75
to
+79
| - name: invocation-service | ||
| enabled: false | ||
| enabled: true | ||
| selector: | ||
| app.kubernetes.io/instance: invocation-service | ||
| app.kubernetes.io/name: helm-nvcf-invocation-service |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'invocation-service|name:\s*metrics|targetPort:\s*metrics|/metrics' deploy/helm deploy/stacks/self-managedRepository: NVIDIA/nvcf
Length of output: 43866
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the invocation-service chart for its metrics service/endpoint contract
printf '\n== values ==\n'
sed -n '130,165p' deploy/helm/http-invocation/values.yaml
printf '\n== deployment ==\n'
sed -n '1,140p' deploy/helm/http-invocation/templates/deployment.yaml
printf '\n== service ==\n'
sed -n '1,120p' deploy/helm/http-invocation/templates/service.yaml
printf '\n== default monitor values ==\n'
sed -n '60,95p' deploy/stacks/observability/charts/nvcf-default-monitors/values.yamlRepository: NVIDIA/nvcf
Length of output: 7163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== default monitor templates ==\n'
fd -a . deploy/stacks/observability/charts/nvcf-default-monitors/templates
printf '\n== monitor template references ==\n'
rg -n -C3 'path:|port:|ServiceMonitor|PodMonitor|invocation-service' deploy/stacks/observability/charts/nvcf-default-monitors/templates deploy/stacks/observability/charts/nvcf-default-monitors/values.yamlRepository: NVIDIA/nvcf
Length of output: 9066
Add a metrics Service port before enabling the invocation-service monitor.
deploy/helm/http-invocation only publishes grpc and http service ports, so this monitor’s default port: metrics/path: /metrics has no Service endpoint to scrape. If metrics are exposed elsewhere, wire that port here; otherwise keep the monitor disabled.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml` around
lines 75 - 79, Update the invocation-service monitor configuration in
nvcf-default-monitors values so its enabled state matches an available metrics
Service endpoint: add and wire a metrics port for helm-nvcf-invocation-service
if metrics are exposed there, or set enabled to false while no metrics Service
port exists. Preserve the monitor’s metrics port/path defaults.
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/function-autoscaler/vault-agent-templates/secrets.json.tmpl`:
- Around line 10-12: Make the static-token block around secret path
services/nvcf-api/jwt/sign/nvcf-autoscaler-service conditional on the dedicated
static-token configuration, so deployments without that setting skip the secret
lookup and allow OAuth2 fallback. Preserve nvcfApiToken rendering when
static-token mode is enabled, and add coverage for both enabled and disabled
modes.
In
`@src/control-plane-services/function-autoscaler/crates/server/src/nvcf_api/oauth2_client.rs`:
- Around line 171-173: Update the refresh-loop logic around get_nvcf_api_token
and token_cache so a present static NVCF API token prevents repeated background
refresh attempts without caching it as a refreshable token. Continue checking
get_nvcf_api_token on each interval so token rotation or removal is detected,
and only perform dynamic refresh when no static token is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: da5f9ee6-7b32-4909-a01f-1a458b54aa7b
📒 Files selected for processing (6)
deploy/helm/function-autoscaler/vault-agent-templates/secrets.json.tmpldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmplsrc/control-plane-services/function-autoscaler/crates/server/src/nvcf_api/oauth2_client.rssrc/control-plane-services/function-autoscaler/crates/server/src/secrets/secrets_config.rssrc/control-plane-services/function-autoscaler/crates/server/src/secrets/secrets_file_watcher.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- deploy/stacks/self-managed/global.yaml.gotmpl
- deploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
Comment on lines
+171
to
+173
| if let Some(token) = secrets_watcher.get_nvcf_api_token() { | ||
| return Ok(token); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Avoid leaving the refresh loop in a permanent “needs refresh” state.
Lines [171]-[173] return the static token without populating token_cache. The background task therefore continues seeing None as “needs refresh” and invokes this path on every refresh interval, logging a successful refresh each time. Skip background refresh while get_nvcf_api_token() is present, while continuing to re-check it for rotation or removal.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/control-plane-services/function-autoscaler/crates/server/src/nvcf_api/oauth2_client.rs`
around lines 171 - 173, Update the refresh-loop logic around get_nvcf_api_token
and token_cache so a present static NVCF API token prevents repeated background
refresh attempts without caching it as a refreshable token. Continue checking
get_nvcf_api_token on each interval so token rotation or removal is detected,
and only perform dynamic refresh when no static token is present.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
deploy/helm/function-autoscaler/scripts/test-render-static-token.sh (1)
68-80: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest the missing-path failure case.
The new
requiredvalidation is release-gating, but this script only tests successful renders. Add a case withenabled=trueand an empty path that must fail.As per coding guidelines, code changes must include tests, or the Pull Request must explain why tests are not applicable.
Proposed test
+missing_path_render="$tmpdir/missing-path.yaml" +if render \ + --set functionautoscaler.vault.staticNvcfApiToken.enabled=true \ + --set functionautoscaler.vault.staticNvcfApiToken.path= \ + > "$missing_path_render" 2>&1; then + echo "expected static-token rendering without a path to fail" >&2 + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/function-autoscaler/scripts/test-render-static-token.sh` around lines 68 - 80, Add a failure-case test to the static token rendering checks using `staticTokenRender`/`render` with `staticNvcfApiToken.enabled=true` and an explicitly empty path. Assert that rendering fails and validates the required-path error, while preserving the existing successful default and custom-path cases.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@deploy/helm/function-autoscaler/scripts/test-render-static-token.sh`:
- Around line 68-80: Add a failure-case test to the static token rendering
checks using `staticTokenRender`/`render` with `staticNvcfApiToken.enabled=true`
and an explicitly empty path. Assert that rendering fails and validates the
required-path error, while preserving the existing successful default and
custom-path cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c02d5df8-4202-4539-b45c-941a9868d8ac
📒 Files selected for processing (4)
deploy/helm/function-autoscaler/scripts/test-render-static-token.shdeploy/helm/function-autoscaler/templates/configmap-vault-agent-template.yamldeploy/helm/function-autoscaler/values.yamldeploy/helm/function-autoscaler/vault-agent-templates/secrets.json.tmpl
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/helm/function-autoscaler/values.yaml
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
1 participant
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.
TL;DR
Wire the function autoscaler into the self-managed stack with the managed observability path it needs to read VictoriaMetrics metrics.
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
observability.mode: install, or an explicit external TSDB URL inobservability.mode: existing.For the Reviewer
Please review the deployment-mode validation and chart ownership boundaries in:
deploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpldeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmplFor QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
git diff --check origin/main...HEADhelmfile listwith managed observability, state metrics, and function autoscaler enabled renders the full metrics path.observability.mode: existingwith autoscaler enabled requires an explicit TSDB URL.helmfile buildagainst real OCI mirrors, because this local environment uses placeholder registry settings.Issues
Relates to #15
Checklist
Summary by CodeRabbit