fix(k8s): Move scripts and values over into nemo-platform#342
Merged
matthewgrossman merged 1 commit intoJun 15, 2026
Conversation
Signed-off-by: Matthew Grossman <mgrossman@nvidia.com>
Contributor
📝 WalkthroughWalkthroughAdds a complete Kubernetes E2E testing infrastructure under ChangesKubernetes E2E Infrastructure
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
e2e/k8s/scripts/collect_k8s_logs.sh (1)
53-54: ⚡ Quick winAvoid parsing
kubectl get podstable output.
awk '{print $1}'on tabular output is brittle and can produce invalid pod names. Use structured output (-o name) instead.Suggested change
- PODS=$(kubectl get pods -n "${NS}" --no-headers 2>/dev/null | awk '{print $1}') || true + PODS=$(kubectl get pods -n "${NS}" -o name 2>/dev/null | sed 's#^pod/##') || true🤖 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 `@e2e/k8s/scripts/collect_k8s_logs.sh` around lines 53 - 54, The PODS variable assignment uses brittle awk parsing on kubectl's tabular output which can fail with unusual pod names or formatting changes. Replace the kubectl command with the -o name flag instead of --no-headers piped through awk, so the command becomes kubectl get pods -n "${NS}" -o name which provides structured pod name output in the pods/pod-name format that is reliable and doesn't require text parsing.
🤖 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 `@e2e/k8s/scripts/afterscript_collect_k8s_logs.sh`:
- Around line 5-8: The script has `set -e` which causes it to exit immediately
on any error, but the unguarded `kubectl get pods` and `FAILED_PODS` assignment
can fail and prevent log collection from running. Remove the `set -e` statement
or make the kubectl calls best-effort by appending `|| true` to both the
`kubectl get pods` command and the `FAILED_PODS` assignment so that if these
commands fail, the script continues with log collection instead of exiting
early.
In `@e2e/k8s/scripts/create_rustfs_bucket.sh`:
- Around line 20-25: The bucket creation commands are not idempotent and fail
when run multiple times because they attempt to create a bucket without checking
if it already exists. In e2e/k8s/scripts/create_rustfs_bucket.sh (lines 20-25),
modify the kubectl run command with aws-cli to first check if the bucket exists
using aws s3 ls before attempting s3 mb, or treat the "bucket already exists"
error as a success condition. Apply the same idempotent logic in
e2e/k8s/scripts/install_helm_e2e.sh (lines 81-87) where the inline RustFS bucket
setup is performed, ensuring that repeated E2E runs do not fail due to the
bucket already existing.
- Around line 8-11: The namespace initialization on line 8 is a no-op default
that doesn't actually set a fallback value, allowing an empty NAMESPACE to be
passed to the kubectl command on line 11. Replace the no-op default assignment
with either a valid default value for the NAMESPACE variable, or add explicit
validation that checks if NAMESPACE is empty and exits the script with an error
message before the kubectl wait command is executed. This ensures the script
fails fast with a clear error rather than silently proceeding with an invalid
empty namespace.
In `@e2e/k8s/scripts/install_helm_e2e.sh`:
- Around line 41-46: Add minikube to the prerequisite tool validation loop that
checks for kubectl, helm, and curl. Include minikube in the for loop at the
beginning of the script so that if minikube is not installed, the script will
exit with a clear error message stating minikube is not installed, rather than
producing a misleading error about the cluster not running when minikube is
actually executed later at line 93.
- Around line 61-67: The helm upgrade commands for RustFS do not explicitly
specify a namespace, which can cause the RustFS installation to diverge from the
Helm release namespace when the release namespace is overridden via script
arguments. This breaks service resolution for s3-rustfs which expects RustFS to
be in the same namespace as the release. Add an explicit namespace flag (likely
using a variable that captures the target release namespace from the script's
input arguments) to all helm upgrade commands that install/upgrade RustFS.
Ensure the namespace parameter is consistently applied across all locations
where RustFS is deployed so that RustFS and the release always deploy to the
same namespace.
In `@e2e/k8s/scripts/install_nmp_e2e.sh`:
- Around line 9-10: The variable assignments at lines 9-10 use self-referential
defaults (NMP_E2E_REGISTRY defaults to itself) which result in empty values, and
these empty values are then passed as --set overrides to Helm at lines 29-34,
breaking the image references. Fix lines 9-10 to remove the self-referential
defaults or set sensible defaults instead of self-references. Then fix lines
29-34 to conditionally include the --set overrides only when the
NMP_E2E_REGISTRY and NMP_E2E_TAG variables are non-empty, ensuring invalid empty
image references are never passed to Helm.
- Around line 78-80: Exit code 127 indicates "command not found" which is an
error condition, not success, but the current logic at lines 78-80 breaks the
loop when exit code equals 127, allowing the script to continue and return
success at line 112. This masks startup and execution failures. Remove or change
the condition that breaks the loop on exit code 127 so that error exit codes
like 127 are properly treated as failures, and ensure the script returns a
non-zero exit code to indicate failure rather than allowing successful
completion when critical errors occur.
In `@e2e/k8s/scripts/install_rustfs.sh`:
- Around line 18-19: The `helm repo add rustfs` command on line 18 fails when
rerun because the repository already exists, and with `set -e` this causes the
script to exit. Make the command idempotent by appending `|| true` to suppress
the error when the repository is already registered, or alternatively use the
`--force-update` flag if available in the helm version being used. This ensures
the script can be safely rerun without failure.
In `@e2e/k8s/scripts/setup_local_minikube_cpu.sh`:
- Around line 94-108: The KUBECTL_NS array variable is being expanded without
quotes in multiple locations (lines 94, 96, 98, 102, 106, and 108), which can
cause word splitting issues in bash. Wrap all instances of ${KUBECTL_NS[@]} with
double quotes, changing them to "${KUBECTL_NS[@]}" to ensure proper handling of
the array expansion in the kubectl commands for creating secrets (ngc-api,
nvcrimagepullsecret, and huggingface-token).
In `@e2e/k8s/scripts/setup_local_minikube_gpu.sh`:
- Around line 149-151: The HF_TOKEN variable in the --from-literal argument of
the kubectl create secret command is unquoted, which allows word splitting and
globbing to potentially corrupt the token value if it contains spaces or special
characters. Quote the HF_TOKEN variable in the --from-literal=HF_TOKEN parameter
to preserve the token value exactly as provided.
- Around line 122-124: The kubectl label command that sets
feature.node.kubernetes.io/pci-10de.present=true is executed unconditionally,
which incorrectly advertises GPU capability on CPU-only systems. Add a check to
detect if NVIDIA GPUs are actually present on the system before executing the
label command. Only proceed with the kubectl label nodes command if GPU hardware
is detected (e.g., by checking for nvidia-smi availability or NVIDIA device
presence).
- Around line 50-56: The minikube command invocation in lines 67-70
unconditionally passes the `--gpus "${GPUS}"` flag even when GPUS is empty.
Modify the minikube command construction to conditionally include the `--gpus`
flag only when GPUs are detected (when GPUS="all"), and omit the flag entirely
when no GPUs are detected (GPUS=""), since Minikube requires the flag to be
absent for CPU-only operation rather than accepting an empty value.
In `@e2e/k8s/scripts/wait_for_api.sh`:
- Around line 17-23: The curl commands in both scripts lack per-request network
timeout limits, allowing stalled connections to exceed the intended timeout
guarantees. In e2e/k8s/scripts/wait_for_api.sh (lines 17-23), add
--connect-timeout and --max-time flags to the curl call that polls the API
endpoint. In e2e/k8s/scripts/run_auth_e2e.sh (lines 29-68), define shared curl
timeout arguments as variables at the start of the script, then consistently
apply these timeout flags to all curl invocations used for waiting on the
service, discovery probes, and the 401/200 verification requests to ensure
bounded timeout behavior across all HTTP probes.
In `@e2e/k8s/scripts/wait_for_release_ready.sh`:
- Around line 49-63: The kubectl query at lines 49-63 that scans for image pull
failures should exclude the core-storage binder pod from the fast-fail checks.
Modify the jq filter to add a condition that excludes pods matching the binder
pod name pattern (referenced in the labeling logic at lines 162-171). This
ensures that image pull failures in the auxiliary binder pod do not cause the
monitor to exit nonzero and abort the Helm install, since this is helper
infrastructure rather than a core release component.
In `@e2e/k8s/values/minikube.yaml`:
- Around line 19-23: The volumePermissionsImage key in minikube.yaml is
currently positioned at the core level (same indentation as storage), but it
should be nested under core.storage to match the expected chart configuration.
Increase the indentation of volumePermissionsImage so it becomes a sibling of
storageClass under the storage key, making the full path
core.storage.volumePermissionsImage instead of core.volumePermissionsImage.
---
Nitpick comments:
In `@e2e/k8s/scripts/collect_k8s_logs.sh`:
- Around line 53-54: The PODS variable assignment uses brittle awk parsing on
kubectl's tabular output which can fail with unusual pod names or formatting
changes. Replace the kubectl command with the -o name flag instead of
--no-headers piped through awk, so the command becomes kubectl get pods -n
"${NS}" -o name which provides structured pod name output in the pods/pod-name
format that is reliable and doesn't require text parsing.
🪄 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: bc0fd3be-896a-4748-b02f-40b0234258b3
📒 Files selected for processing (22)
e2e/k8s/scripts/afterscript_collect_k8s_logs.she2e/k8s/scripts/collect_k8s_logs.she2e/k8s/scripts/create_rustfs_bucket.she2e/k8s/scripts/install_helm_e2e.she2e/k8s/scripts/install_nmp_auth_e2e.she2e/k8s/scripts/install_nmp_e2e.she2e/k8s/scripts/install_rustfs.she2e/k8s/scripts/local_build_and_upgrade.she2e/k8s/scripts/prepull_kind_images.she2e/k8s/scripts/run_auth_e2e.she2e/k8s/scripts/setup_local_kind_cpu.she2e/k8s/scripts/setup_local_minikube_cpu.she2e/k8s/scripts/setup_local_minikube_gpu.she2e/k8s/scripts/wait_for_api.she2e/k8s/scripts/wait_for_release_ready.she2e/k8s/values/default.yamle2e/k8s/values/kai-scheduler.yamle2e/k8s/values/kind.yamle2e/k8s/values/minikube-auth-portforward.yamle2e/k8s/values/minikube-auth.yamle2e/k8s/values/minikube.yamle2e/k8s/values/s3-rustfs.yaml
Contributor
|
crookedstorm
approved these changes
Jun 15, 2026
crookedstorm
left a comment
crookedstorm
left a comment
Contributor
There was a problem hiding this comment.
I think this is a good thing to start, but we shouldn't cut over the tests until we can build some containers here and pull them into a smoke test in this repo.
matthewgrossman
enabled auto-merge
June 15, 2026 22:59
matthewgrossman
deleted the
mgrossman/aircore-799-move-minikube-values-and-setup-scripts-from-platform-deploy
branch
June 15, 2026 23:07
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
2 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.
Signed-off-by: Matthew Grossman mgrossman@nvidia.com
Summary by CodeRabbit
Release Notes