Skip to content

fix(helm): point node-observer at rendered service name#376

Open
dmitsh wants to merge 1 commit into
mainfrom
ds-svc
Open

fix(helm): point node-observer at rendered service name#376
dmitsh wants to merge 1 commit into
mainfrom
ds-svc

Conversation

@dmitsh

@dmitsh dmitsh commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

Description

Build generateTopologyUrl from the Topograph Service fullname so the
node-observer targets the Service actually rendered by the chart.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.
  • All commits are signed off per DCO (git commit -s).

Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
@dmitsh dmitsh requested a review from ravisoundar as a code owner June 30, 2026 22:31
@dmitsh dmitsh requested a review from giuliocalzo June 30, 2026 22:31
@codecov

codecov Bot commented Jun 30, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.52%. Comparing base (7edb997) to head (a08180c).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #376      +/-   ##
==========================================
+ Coverage   71.31%   71.52%   +0.20%     
==========================================
  Files          86       88       +2     
  Lines        5574     5643      +69     
==========================================
+ Hits         3975     4036      +61     
- Misses       1403     1407       +4     
- Partials      196      200       +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@greptile-apps

greptile-apps Bot commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes the generateTopologyUrl value in the node-observer ConfigMap, which previously used .Release.Name verbatim (e.g., chart-ci) instead of the rendered Service name (e.g., chart-ci-topograph), causing the node-observer to target a non-existent host.

  • A new topograph.serviceName helper is introduced in _helpers.tpl; when rendering as the main chart it delegates to topograph.fullname (honoring fullnameOverride/nameOverride), and when rendering from a subchart context it replicates the release+chart-name concatenation logic with \"topograph\" hardcoded.
  • All eight test snapshots and the subchart integration test are updated to assert the corrected hostname chart-ci-topograph; documentation is updated to match.

Confidence Score: 4/5

Safe to merge; the fix corrects a clear misconfiguration in the node-observer ConfigMap and is well-covered by updated snapshot and subchart integration tests.

The change is targeted and correct for the default deployment case. The one gap — the subchart branch hardcoding 'topograph' without consulting nameOverride or fullnameOverride — would cause the node-observer to point at the wrong service name if users set those overrides on the topograph subchart, but this scenario is uncommon and the previous code was already broken in all cases.

charts/topograph/templates/_helpers.tpl — specifically the subchart branch of topograph.serviceName (lines 84–92) which skips override checks.

Important Files Changed

Filename Overview
charts/topograph/templates/_helpers.tpl Introduces topograph.serviceName helper to derive the correct Service fullname; fixes the main-chart path fully via topograph.fullname, but the subchart branch hardcodes 'topograph' and skips nameOverride/fullnameOverride.
charts/topograph/tests/snapshot/render_snapshot_test.yaml.snap Snapshot consistently updated across all test scenarios from chart-ci to chart-ci-topograph; checksums regenerated correctly.
charts/topograph/tests/subcharts_test.yaml New matchRegex assertion added to verify the node-observer configmap points to chart-ci-topograph; good regression coverage for the fix.
docs/engines/k8s.md Documentation updated to reflect the new default DNS name format -topograph..svc.cluster.local.
CHANGELOG.md Changelog entry added under [Unreleased] / Fixed with a clear description of the fix.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["include 'topograph.url' $"] --> B["include 'topograph.serviceName' ."]
    B --> C{".Chart.Name == 'topograph'?"}
    C -- Yes --> D["include 'topograph.fullname' .\n(honors fullnameOverride / nameOverride)"]
    C -- No\nSubchart context --> E["hardcode name := 'topograph'"]
    E --> F{".Release.Name contains 'topograph'?"}
    F -- Yes --> G[".Release.Name | trunc 63 | trimSuffix '-'"]
    F -- No --> H["printf '%s-%s' .Release.Name 'topograph' | trunc 63"]
    D --> I["Assemble URL\nhttp://serviceName.namespace.svc.cluster.local:port"]
    G --> I
    H --> I
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A["include 'topograph.url' $"] --> B["include 'topograph.serviceName' ."]
    B --> C{".Chart.Name == 'topograph'?"}
    C -- Yes --> D["include 'topograph.fullname' .\n(honors fullnameOverride / nameOverride)"]
    C -- No\nSubchart context --> E["hardcode name := 'topograph'"]
    E --> F{".Release.Name contains 'topograph'?"}
    F -- Yes --> G[".Release.Name | trunc 63 | trimSuffix '-'"]
    F -- No --> H["printf '%s-%s' .Release.Name 'topograph' | trunc 63"]
    D --> I["Assemble URL\nhttp://serviceName.namespace.svc.cluster.local:port"]
    G --> I
    H --> I
Loading

Reviews (1): Last reviewed commit: "fix(helm): point node-observer at render..." | Re-trigger Greptile

Comment on lines +84 to +92
{{- else }}
{{- /* Subcharts render with their own .Chart.Name, so use the parent chart name explicitly. */}}
{{- $name := "topograph" }}
{{- if contains $name .Release.Name }}
{{- .Release.Name | trunc 63 | trimSuffix "-" }}
{{- else }}
{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" }}
{{- end }}
{{- end }}

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 Subchart path ignores nameOverride and fullnameOverride

When topograph.serviceName is evaluated from a subchart context (e.g., node-observer), it hardcodes "topograph" as the chart name and skips the fullnameOverride/nameOverride checks that topograph.fullname performs on lines 14–23. If a user sets fullnameOverride: my-svc (or nameOverride: my-svc) in the topograph chart's values.yaml, the actual Service will be named my-svc but the node-observer configmap will still resolve to <release>-topograph, breaking connectivity. One way to close the gap would be exposing global.topographServiceName so subcharts can consume it without needing to replicate parent-chart value scoping.

@ravisoundar ravisoundar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants