Skip to content

docs(demo): pin the kube context in the demo READMEs and the two scripts that still omit it - #576

Open
LuisMend12 wants to merge 2 commits into
NVIDIA:mainfrom
LuisMend12:docs/pin-kube-context-in-demos
Open

docs(demo): pin the kube context in the demo READMEs and the two scripts that still omit it#576
LuisMend12 wants to merge 2 commits into
NVIDIA:mainfrom
LuisMend12:docs/pin-kube-context-in-demos

Conversation

@LuisMend12

Copy link
Copy Markdown

Summary

compute-domain/run.sh and failure-injection/run.sh set CLUSTER_NAME and stop there — no KUBE_CONTEXT, no wrapper, so every kubectl/helm call in both scripts runs against the ambient kubeconfig context instead of the demo's own Kind cluster. A documented/scripted command that happens to match a real DaemonSet, namespace, or release name on the reader's actual cluster returns plausible output and looks like it worked — while it silently operated on the wrong cluster.

This brings both scripts in line with the KUBE_CONTEXT / kubectl_ctx() idiom already established in standalone/demo.sh and node-wide-injection/run.sh:

  • Added KUBE_CONTEXT="kind-${CLUSTER_NAME}" and a kubectl_ctx() { command kubectl --context "${KUBE_CONTEXT}" "$@"; } wrapper to each script.
  • Replaced every bare kubectl call with kubectl_ctx (mentions of kubectl inside prose comments were left untouched).
  • Added --kube-context "${KUBE_CONTEXT}" to every helm call.

Also pinned the manual/documented commands that were still unpinned:

  • compute-domain/README.md: the helm install command and the four kubectl commands in the manual-reproduction and troubleshooting sections. This also makes the existing "Custom cluster name" note truthful — it already claimed every command below it uses --context, which wasn't the case until now.
  • failure-injection/README.md: the interactive kubectl exec example.
  • standalone/README.md: helm uninstall — a destructive command, and previously the one command in this particular demo that was less safe than its own script.
  • with-fgo/README.md: both helm calls and every kubectl call in the verification section.

node-wide-injection/README.md was already fully pinned (its run.sh already carries the idiom), so no changes were needed there.

Fixes #574

Test plan

  • bash -n on both modified scripts — no syntax errors
  • Confirmed no bare (unpinned) kubectl call sites remain in either script or any of the four modified READMEs (only prose mentions of kubectl were left alone)
  • Confirmed every helm invocation across the changed files now carries --kube-context/--kube-context consistent with the rest of the codebase's idiom
  • Not run against a live Kind cluster in this environment — the change only adds context-pinning flags to existing, already-tested commands; no command's arguments or logic otherwise changed

…ripts

compute-domain/run.sh and failure-injection/run.sh set CLUSTER_NAME and
never pinned a kube context anywhere -- every kubectl/helm call ran
against the ambient context. A documented command that happens to
match a real DaemonSet/namespace on the reader's actual cluster looks
like it worked, when it actually operated on the wrong cluster.

Brought both scripts in line with the existing KUBE_CONTEXT /
kubectl_ctx idiom already used by standalone/demo.sh and
node-wide-injection/run.sh:
- Added KUBE_CONTEXT="kind-${CLUSTER_NAME}" and a kubectl_ctx()
  wrapper to each script.
- Replaced every bare kubectl call with kubectl_ctx (kubectl mentions
  inside prose comments left untouched).
- Added --kube-context "${KUBE_CONTEXT}" to every helm call.

Also pinned the manual-reproduction / troubleshooting commands in the
READMEs that were still unpinned:
- compute-domain/README.md: helm install, and the four kubectl
  commands in the manual-reproduction and troubleshooting sections.
  This also makes the "Custom cluster name" note truthful again --
  it already claimed every command below it uses --context.
- failure-injection/README.md: the interactive kubectl exec example.
- standalone/README.md: helm uninstall -- destructive, and previously
  the one command in this demo that was less safe than its own script.
- with-fgo/README.md: both helm calls and every kubectl call in the
  verification section.

node-wide-injection/README.md was already fully pinned (its run.sh
already had the idiom), so no changes were needed there.

Fixes NVIDIA#574
@copy-pr-bot

copy-pr-bot Bot commented Aug 2, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@ArangoGutierrez ArangoGutierrez 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.

Nothing here blocks merge.

That's true of the change itself — the sweep is complete. No bare kubectl is left in either script, all four helm calls carry --kube-context, every runnable command in the four READMEs is pinned, and bash -n is clean on both. node-wide-injection and nv-sentinel were already covered, as the description says.

Separately, and before it can merge: the commit has no Signed-off-by line, which is what the red DCO check is about, and main also requires GPG-signed commits. git commit --amend -s -S and a force-push clear both.

One leftover from the issue:

  1. [should-fix] Renaming the cluster now means updating this flag too, but the "Custom cluster name" checklist a little further down still lists only kind / kubectl --context / kind load. That was issue #574's third item, so adding helm --kube-context to it would close the last piece. (docs/demo/compute-domain/README.md:147)

# chart pulls the default upstream image which does not have the
# real IMEX layer baked in).
helm install nvml-mock deployments/nvml-mock/helm/nvml-mock \
--kube-context kind-nvml-mock-compute-domain \

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.

Renaming the cluster now means updating this flag too, but the "Custom cluster name" checklist a little further down still lists only kind / kubectl --context / kind load. That was issue #574's third item, so adding helm --kube-context to it would close the last piece.

@ArangoGutierrez ArangoGutierrez added the priority/p1 P1: works but not as documented; small impact; workaround exists. Max fix SLA: 6 months. label Aug 3, 2026
@ArangoGutierrez ArangoGutierrez added this to the v0.4.0 milestone Aug 3, 2026
Signed-off-by: LuisM23 <128239393+LuisMend12@users.noreply.github.com>
@ArangoGutierrez

Copy link
Copy Markdown
Collaborator
image

hey @LuisMend12 please address the DCO warning so we can move on with this PR

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

Labels

priority/p1 P1: works but not as documented; small impact; workaround exists. Max fix SLA: 6 months.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

docs(demo): pin the kube context in the demo READMEs and the two scripts that still omit it

2 participants