feat(self-managed): add profile-based observability - #509
Merged
Conversation
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe observability stack moves from mode-based configuration to profile-based ownership, adds profile-derived monitor and metrics backend resolution, integrates the function autoscaler for selected self-managed profiles, and adds Helmfile, chart, and integration validation. ChangesObservability profile migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Values as observability.profile
participant Helmfiles as observability Helmfiles
participant Monitors as default-monitors
participant Collector as otel-collector
participant Autoscaler as function-autoscaler
participant NVCA as nvca-operator
Values->>Helmfiles: select profile and component ownership
Helmfiles->>Monitors: render profile-derived monitor groups
Helmfiles->>Collector: pass resolved collector and metrics settings
Helmfiles->>Autoscaler: render release for control or all profiles
Values->>NVCA: derive collector and BYOObservability settings
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
deploy/stacks/observability/Makefile (1)
25-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSplit
testtarget to satisfy checkmake's max body length.Static analysis flags the
testtarget body (8 lines) exceeding checkmake's 5-line limit. Consider splitting into alintprerequisite target plus a slimmertesttarget that just runs the two scripts.♻️ Example split
-test: - `@helm` lint charts/nvcf-default-monitors - `@helm` lint charts/nvcf-observability-contract - `@helm` lint charts/nvcf-otel-collector - `@helm` lint ../../helm/function-autoscaler \ - --set functionautoscaler.image.registry=nvcr.io \ - --set functionautoscaler.image.repository=test/nvcf-function-autoscaler - `@tests/profile-defaults.sh` - @../self-managed/tests/observability-autoscaler.sh +lint: + `@helm` lint charts/nvcf-default-monitors + `@helm` lint charts/nvcf-observability-contract + `@helm` lint charts/nvcf-otel-collector + `@helm` lint ../../helm/function-autoscaler \ + --set functionautoscaler.image.registry=nvcr.io \ + --set functionautoscaler.image.repository=test/nvcf-function-autoscaler + +test: lint + `@tests/profile-defaults.sh` + @../self-managed/tests/observability-autoscaler.sh🤖 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/Makefile` around lines 25 - 34, Split the Makefile’s test target into a lint prerequisite containing the four helm lint commands and a shorter test target that depends on lint and runs only tests/profile-defaults.sh and ../self-managed/tests/observability-autoscaler.sh. Preserve the existing command order and behavior while keeping each target body within checkmake’s five-line limit.Source: Linters/SAST tools
deploy/stacks/self-managed/environments/base.yaml (1)
136-155: 🧹 Nitpick | 🔵 TrivialNew default installs the full shared observability stack for every self-managed control-plane deployment.
Switching the self-managed default to
observability.profile: controlmeans Prometheus Operator CRDs, the OTel Operator, otel-collector, VictoriaMetrics, and default-monitors are now installed unconditionally out of the box, even for minimal/local test clusters. Worth confirming this is the intended default footprint for smaller self-managed environments (e.g.local.yaml) and documenting the added resource requirements in the stack README/AGENTS.md if not already covered elsewhere.🤖 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/self-managed/environments/base.yaml` around lines 136 - 155, The self-managed base configuration now enables the full observability stack by default via observability.profile: control. Confirm whether this footprint is intended for minimal environments such as local.yaml; if not, change the base default to preserve lightweight deployments and explicitly opt control-plane environments into the stack. If the default is intentional, retain it and document the added resource requirements in the relevant stack documentation.
🤖 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/AGENTS.md`:
- Around line 25-32: Add development-instruction sections to AGENTS.md covering
build commands, test commands, code style, and subtree-specific conventions.
Document the required commands make template HELMFILE_ENV=local and make test,
while preserving the existing scope and Key Files content.
In `@deploy/stacks/self-managed/global.yaml.gotmpl`:
- Around line 592-610: The functionautoscaler checksum currently hashes
unresolved top-level values instead of the rendered observability contract.
Update the `$observabilityContract` construction used by
`checksum/observability-contract` to consume the resolved metricsBackend and
autoscalerIntegration values forwarded by the observability infrastructure
render, or otherwise thread those resolved values into this template, so the
annotation changes whenever the ConfigMap-backed contract changes.
---
Nitpick comments:
In `@deploy/stacks/observability/Makefile`:
- Around line 25-34: Split the Makefile’s test target into a lint prerequisite
containing the four helm lint commands and a shorter test target that depends on
lint and runs only tests/profile-defaults.sh and
../self-managed/tests/observability-autoscaler.sh. Preserve the existing command
order and behavior while keeping each target body within checkmake’s five-line
limit.
In `@deploy/stacks/self-managed/environments/base.yaml`:
- Around line 136-155: The self-managed base configuration now enables the full
observability stack by default via observability.profile: control. Confirm
whether this footprint is intended for minimal environments such as local.yaml;
if not, change the base default to preserve lightweight deployments and
explicitly opt control-plane environments into the stack. If the default is
intentional, retain it and document the added resource requirements in the
relevant stack documentation.
🪄 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: 28159a2b-aa8b-4742-a251-241dee71eebc
📒 Files selected for processing (27)
ai-tooling/dev/skills/nvcf-explore-stack/SKILL.mddeploy/helm/function-autoscaler/Chart.yamldeploy/helm/function-autoscaler/templates/deployment.yamldeploy/helm/function-autoscaler/values.yamldeploy/stacks/observability/AGENTS.mddeploy/stacks/observability/Makefiledeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/Chart.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/nvca-servicemonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/worker-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/charts/nvcf-observability-contract/Chart.yamldeploy/stacks/observability/charts/nvcf-observability-contract/templates/autoscaler-configmap.yamldeploy/stacks/observability/charts/nvcf-observability-contract/templates/profile-configmap.yamldeploy/stacks/observability/charts/nvcf-observability-contract/values.yamldeploy/stacks/observability/charts/nvcf-otel-collector/Chart.yamldeploy/stacks/observability/charts/nvcf-otel-collector/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/environments/local.yamldeploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/00-observability-infrastructure.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpldeploy/stacks/self-managed/tests/observability-autoscaler.sh
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 `@ai-tooling/dev/skills/nvcf-explore-stack/SKILL.md`:
- Around line 73-77: Revise the “Which subtree do I edit to change X” guidance
to remove the claim that all stack sources are monorepo-native. Limit editable
paths to checked-in sources and direct users to the external repository or
Helmfile chart reference for charts not present in the monorepo.
🪄 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: 4e63fc26-c0ff-489d-af26-4976a5fe4916
📒 Files selected for processing (6)
ai-tooling/dev/skills/nvcf-explore-stack/SKILL.mddeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/tests/observability-autoscaler.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/stacks/self-managed/environments/base.yaml
- deploy/stacks/self-managed/tests/observability-autoscaler.sh
- deploy/stacks/observability/tests/profile-defaults.sh
- deploy/stacks/observability/README.md
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
Signed-off-by: Kristina Pathak <kpathak@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
deploy/stacks/observability/Makefile (1)
25-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit the
testrecipe to satisfy Makefile lint.
checkmakereportsmaxbodylengthhere: the target has eight recipe lines versus the configured limit of five. Extract chart linting and shell validations into helper targets or a dedicated script, then maketestdepend on them.🤖 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/Makefile` around lines 25 - 34, Reduce the recipe length of the test target to satisfy checkmake’s maxbodylength limit. Extract the Helm lint commands and shell validation commands into helper targets or a dedicated script, then make test depend on those helpers while preserving all existing validations.Source: Linters/SAST tools
🤖 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/templates/nvca-servicemonitor.yaml`:
- Around line 18-39: Remove the compute-plane monitor wiring from both
deploy/stacks/observability/charts/nvcf-default-monitors/templates/nvca-servicemonitor.yaml
lines 18-39 and
deploy/stacks/observability/charts/nvcf-default-monitors/templates/worker-podmonitor.yaml
lines 18-38, or relocate those resources to the owning compute-plane layer.
Ensure the shared observability release renders neither the NVCA ServiceMonitor
nor the worker PodMonitor, and does not add control-plane or compute-plane
targets.
In `@deploy/stacks/observability/tests/profile-defaults.sh`:
- Around line 186-204: Add mTLS contract coverage alongside the existing token
scenario in profile-defaults.sh. Render the observability-contract with mTLS
authentication and assert the generated manifest contains
TIMESERIES_DB__AUTH_MODE plus the expected certificate-path and private-key-path
fields, using the established test helpers and failure pattern.
---
Nitpick comments:
In `@deploy/stacks/observability/Makefile`:
- Around line 25-34: Reduce the recipe length of the test target to satisfy
checkmake’s maxbodylength limit. Extract the Helm lint commands and shell
validation commands into helper targets or a dedicated script, then make test
depend on those helpers while preserving all existing validations.
🪄 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: 226f04c1-e784-412f-8936-e290f17981f4
📒 Files selected for processing (26)
ai-tooling/dev/skills/nvcf-explore-stack/SKILL.mddeploy/helm/function-autoscaler/templates/deployment.yamldeploy/helm/function-autoscaler/values.yamldeploy/stacks/observability/AGENTS.mddeploy/stacks/observability/Makefiledeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/Chart.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/nvca-servicemonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/worker-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/charts/nvcf-observability-contract/Chart.yamldeploy/stacks/observability/charts/nvcf-observability-contract/templates/autoscaler-configmap.yamldeploy/stacks/observability/charts/nvcf-observability-contract/templates/profile-configmap.yamldeploy/stacks/observability/charts/nvcf-observability-contract/values.yamldeploy/stacks/observability/charts/nvcf-otel-collector/Chart.yamldeploy/stacks/observability/charts/nvcf-otel-collector/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/environments/local.yamldeploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/00-observability-infrastructure.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpldeploy/stacks/self-managed/tests/observability-autoscaler.sh
🚧 Files skipped from review as they are similar to previous changes (18)
- deploy/stacks/observability/environments/local.yaml
- deploy/stacks/observability/charts/nvcf-otel-collector/Chart.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/Chart.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yaml
- deploy/stacks/observability/charts/nvcf-observability-contract/values.yaml
- deploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
- deploy/stacks/observability/charts/nvcf-observability-contract/Chart.yaml
- deploy/stacks/self-managed/environments/base.yaml
- deploy/stacks/self-managed/tests/observability-autoscaler.sh
- deploy/stacks/observability/charts/nvcf-otel-collector/values.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml
- deploy/stacks/observability/README.md
- deploy/stacks/observability/AGENTS.md
- deploy/stacks/observability/environments/base.yaml
- deploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpl
- deploy/helm/function-autoscaler/values.yaml
- deploy/helm/function-autoscaler/templates/deployment.yaml
- ai-tooling/dev/skills/nvcf-explore-stack/SKILL.md
Signed-off-by: Kristina Pathak <kpathak@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 76-79: Replace the unsupported namespaceSelector.matchExpressions
in the default PodMonitor configuration with namespaceSelector.matchNames
containing the explicit worker namespace names. Add a profile-defaults.sh
assertion that verifies the rendered PodMonitor uses the expected matchNames
structure and values.
🪄 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: 12581773-3e5f-4b4d-b254-bc2d5cf1b3d4
📒 Files selected for processing (8)
deploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/servicemonitors.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/worker-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpldeploy/stacks/observability/tests/profile-defaults.sh
🚧 Files skipped from review as they are similar to previous changes (4)
- deploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yaml
- deploy/stacks/observability/environments/base.yaml
- deploy/stacks/observability/README.md
- deploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpl
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/self-managed/global.yaml.gotmpl`:
- Around line 596-622: Update the functionautoscaler namespace field to derive
from autoscalerIntegration.namespace instead of hard-coding nvcf, keeping the
pod and contract ConfigMap co-located. Add a render test covering a non-default
autoscalerIntegration namespace and verifying both resources use it.
🪄 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: 42de8336-4c13-4e34-9c5b-aa0db4696737
📒 Files selected for processing (26)
ai-tooling/dev/skills/nvcf-explore-stack/SKILL.mddeploy/helm/function-autoscaler/templates/deployment.yamldeploy/helm/function-autoscaler/values.yamldeploy/stacks/observability/AGENTS.mddeploy/stacks/observability/Makefiledeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/Chart.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/servicemonitors.yamldeploy/stacks/observability/charts/nvcf-default-monitors/templates/worker-podmonitor.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/charts/nvcf-observability-contract/Chart.yamldeploy/stacks/observability/charts/nvcf-observability-contract/templates/autoscaler-configmap.yamldeploy/stacks/observability/charts/nvcf-observability-contract/templates/profile-configmap.yamldeploy/stacks/observability/charts/nvcf-observability-contract/values.yamldeploy/stacks/observability/charts/nvcf-otel-collector/Chart.yamldeploy/stacks/observability/charts/nvcf-otel-collector/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/environments/local.yamldeploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/00-observability-infrastructure.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpldeploy/stacks/self-managed/tests/observability-autoscaler.sh
🚧 Files skipped from review as they are similar to previous changes (18)
- deploy/stacks/observability/charts/nvcf-otel-collector/values.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/Chart.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/templates/dcgm-podmonitor.yaml
- deploy/stacks/observability/charts/nvcf-observability-contract/values.yaml
- deploy/stacks/observability/charts/nvcf-otel-collector/Chart.yaml
- deploy/stacks/observability/environments/local.yaml
- deploy/stacks/observability/charts/nvcf-observability-contract/Chart.yaml
- deploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
- deploy/helm/function-autoscaler/values.yaml
- deploy/stacks/self-managed/helmfile.d/00-observability-infrastructure.yaml.gotmpl
- deploy/stacks/observability/AGENTS.md
- deploy/stacks/self-managed/environments/base.yaml
- deploy/stacks/self-managed/tests/observability-autoscaler.sh
- deploy/helm/function-autoscaler/templates/deployment.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml
- deploy/stacks/observability/environments/base.yaml
- deploy/stacks/observability/tests/profile-defaults.sh
- ai-tooling/dev/skills/nvcf-explore-stack/SKILL.md
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/templates/podmonitors.yaml`:
- Around line 33-37: Update the generic PodMonitor labels template to merge
defaultMonitors.commonLabels with each monitor’s labels, ensuring common labels
are included while per-monitor labels take precedence on conflicts. Preserve the
existing chart labels and apply this behavior to the PodMonitors rendered by the
surrounding template.
🪄 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: 731e9d11-997a-4576-9182-28563b500c5c
📒 Files selected for processing (13)
deploy/stacks/observability/AGENTS.mddeploy/stacks/observability/Makefiledeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/templates/podmonitors.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/environments/base.yamldeploy/stacks/self-managed/global.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/00-observability-infrastructure.yaml.gotmpldeploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpldeploy/stacks/self-managed/tests/observability-autoscaler.sh
💤 Files with no reviewable changes (3)
- deploy/stacks/observability/Makefile
- deploy/stacks/observability/helmfile.d/01-observability.yaml.gotmpl
- deploy/stacks/observability/AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (5)
- deploy/stacks/self-managed/helmfile.d/00-observability-infrastructure.yaml.gotmpl
- deploy/stacks/self-managed/helmfile.d/03-observability.yaml.gotmpl
- deploy/stacks/self-managed/environments/base.yaml
- deploy/stacks/observability/README.md
- deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml
4 tasks
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/README.md`:
- Around line 20-23: Update the enabled-profiles description near the reusable
and self-managed stack defaults to qualify that these components are installed
by default, unless an existing ownership override prevents this stack from
installing them. Keep the listed components and profile defaults unchanged, and
align the wording with the documented override behavior below.
- Around line 97-102: Update the bundled VictoriaMetrics endpoint documentation
near the remote write and PromQL URLs to state that the monitoring namespace is
configuration-dependent. Explain that users overriding the Helmfile-derived
namespace must replace “monitoring” in the service hostname with their
configured namespace.
- Around line 129-133: Update the Validate section in the observability README
to include git diff --check alongside the existing template and test commands,
ensuring the documented validation steps satisfy the repository requirement
before committing.
🪄 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: 0947be5c-08e2-424b-adda-c2b028504b8e
📒 Files selected for processing (8)
ai-tooling/dev/skills/nvcf-explore-stack/SKILL.mddeploy/stacks/observability/AGENTS.mddeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/templates/podmonitors.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/tests/observability-autoscaler.sh
🚧 Files skipped from review as they are similar to previous changes (7)
- deploy/stacks/self-managed/tests/observability-autoscaler.sh
- deploy/stacks/observability/AGENTS.md
- deploy/stacks/observability/environments/base.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/templates/podmonitors.yaml
- deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml
- deploy/stacks/observability/tests/profile-defaults.sh
- ai-tooling/dev/skills/nvcf-explore-stack/SKILL.md
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/nvcf-compute-plane/tests/observability-profile.sh`:
- Around line 75-90: Extend the observability profile test around render_values
to provide explicit otelCollector imageRepository and imageTag values, then
assert both are preserved in the rendered compute-overrides.yaml. Use the
existing YAML inspection/assertion patterns and collector-related symbols in
observability-profile.sh, while keeping the enabled and feature-gate checks
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: a89be95f-b9b7-46c8-8401-1c3cb87df2a8
⛔ Files ignored due to path filters (1)
deploy/stacks/nvcf-compute-plane/testdata/golden/local/02-nvca.yaml-31ea6bf9-nvca-operator/helm-nvca-operator/templates/self-managed-nvcfbackend-cm.yamlis excluded by!**/testdata/**
📒 Files selected for processing (15)
ai-tooling/dev/skills/nvcf-explore-stack/SKILL.mddeploy/stacks/nvcf-compute-plane/Makefiledeploy/stacks/nvcf-compute-plane/README.mddeploy/stacks/nvcf-compute-plane/environments/base.yamldeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpldeploy/stacks/nvcf-compute-plane/tests/compare-golden.shdeploy/stacks/nvcf-compute-plane/tests/observability-profile.shdeploy/stacks/observability/AGENTS.mddeploy/stacks/observability/Makefiledeploy/stacks/observability/README.mddeploy/stacks/observability/charts/nvcf-default-monitors/templates/podmonitors.yamldeploy/stacks/observability/charts/nvcf-default-monitors/values.yamldeploy/stacks/observability/environments/base.yamldeploy/stacks/observability/tests/profile-defaults.shdeploy/stacks/self-managed/tests/observability-autoscaler.sh
🚧 Files skipped from review as they are similar to previous changes (9)
- deploy/stacks/observability/AGENTS.md
- deploy/stacks/observability/Makefile
- deploy/stacks/observability/charts/nvcf-default-monitors/templates/podmonitors.yaml
- ai-tooling/dev/skills/nvcf-explore-stack/SKILL.md
- deploy/stacks/observability/README.md
- deploy/stacks/observability/tests/profile-defaults.sh
- deploy/stacks/self-managed/tests/observability-autoscaler.sh
- deploy/stacks/observability/charts/nvcf-default-monitors/values.yaml
- deploy/stacks/observability/environments/base.yaml
Collaborator
Author
|
Local k3d smoke test on
The autoscaler result validates profile selection, rendered configuration, packaging, probes, Service/DNS connectivity, and chart lifecycle. It does not validate the production Rust autoscaler scaling loop, Cassandra integration, or NVCF API calls. |
sbaum1994
approved these changes
Jul 29, 2026
This was referenced Jul 29, 2026
borao
approved these changes
Jul 29, 2026
This was referenced Jul 30, 2026
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
3 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.
TL;DR
disabled,control,compute, orallprofile.Additional Details
controlandall; defaults NVCA, DCGM, and worker monitors on forcomputeandall.BYOObservabilityforcomputeandall, while preserving explicit NVCA overrides.ServiceMonitortemplate and one genericPodMonitortemplate.install,existing, anddisabled.controlandall, pins image1.18.3, and uses chart0.1.0.For the Reviewer
01-observability.yaml.gotmpl.global.yaml.gotmpl.deploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmpl.For QA
make testindeploy/stacks/observabilitymake test-localindeploy/stacks/nvcf-compute-planepython3 .cursor/hooks/validate-skill-fanout.pygit diff --checkLive-cluster QA remains for autoscaler readiness and scaling.
Issues
Relates to #15
Checklist
Summary by CodeRabbit
disabled,control,compute,all) with component ownership modes and profile-driven defaults.make testtarget.