Conversation
Greptile SummaryThis PR reduces Kubernetes API traffic and centralizes client rate-limit configuration.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
H[Helm kubeClient settings] --> C[Provider and engine parameters]
C --> K[Kubernetes engine client]
C --> D[DRA provider client]
C --> S[Slinky engine client]
C --> N[NFD shared token bucket]
N --> T[NFD typed client]
N --> Y[NFD dynamic client]
K --> L[List selected Nodes once]
L --> M{Topology labels changed?}
M -- No --> O[Skip Node]
M -- Yes --> P[Patch changed labels]
T --> A[Kubernetes API]
Y --> A
D --> A
S --> A
O --> A
P --> A
Reviews (6): Last reviewed commit: "addressed comments" | Re-trigger Greptile |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #438 +/- ##
==========================================
+ Coverage 72.15% 74.95% +2.79%
==========================================
Files 89 95 +6
Lines 5689 6547 +858
==========================================
+ Hits 4105 4907 +802
+ Misses 1382 1378 -4
- Partials 202 262 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
🌿 Preview your docs: https://nvidia-preview-pull-request-438.docs.buildwithfern.com/topograph |
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Found 6 issues. The rate-limit centralization and the PATCH-only reconciliation both look right; the notes below are mostly about guards that stopped discriminating, plus one default that shifts.
kubeBurst: 10is an unanchored substring that also matcheskubeBurst: 100, so this case cannot pin the client-go burst default it exists to prove. Changing the template todefault 100renderskubeBurst: 100here and all 120 chart tests still pass; anchoring the value would catch it. (charts/topograph/tests/configmap_test.yaml:80)- This PR adds
kubeQPSandkubeBurstas request parameters on the k8s and NFD engines, but the API reference loses its four existing entries rather than gaining two more. They stay live request parameters - the server applies configproviderParams/engineParamsonly when the payload omits the key - so direct API callers have nothing documenting them. (docs/api.md:105) - This only calls the function with
(0, 0), so nothing checks that a configured qps/burst actually reaches the shared token bucket. Rewriting the body tonormalizedClientRateLimits(0, 0), which discards both arguments and would makekubeClient.qpsa no-op for NFD, leaves this package andpkg/engines/nfdgreen. (internal/k8s/client_test.go:60) - Flipping this case to a positive assertion for k8s leaves slurm, nfd, and graph uncovered, since the
by defaultcase above already renders with the chart's default k8s engine. Widening the template guard tone .Values.engine.name "slinky"grants those engines node patch and the suite stays green; anotContainscase withengine.name: slurmwould restore it. (charts/topograph/tests/rbac_test.yaml:152) - The comment documents that this always installs a bucket, but the NFD loader calls it unconditionally, so an unconfigured deployment now shares one 5 QPS bucket where the typed and dynamic clients each built their own before - roughly 10 QPS aggregate down to 5, in a PR aimed at reducing throttling. An early return on zero/zero would keep the old default, or a CHANGELOG note if the lower aggregate is intended. (internal/k8s/client.go:26)
- A node in the graph but outside the engine
nodeSelectorwas labeled through the old GET path; it now errors, andApplyNodeLabelsstops on the first error, so the request fails with a 502 after some nodes are already patched. Logging and skipping would let the pass finish for the nodes in scope. (pkg/engines/k8s/kubernetes.go:43)
…ent rate limits Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
Signed-off-by: Dmitry Shmulevich <dshmulevich@nvidia.com>
ArangoGutierrez
left a comment
ArangoGutierrez
left a comment
There was a problem hiding this comment.
Second pass over the changes since e5f17d0. The five points from the first round all landed, and I mutation-checked four of them: anchoring kubeBurst: 10$ now catches a default 100 template change, the shared-limiter test goes red when the function discards its arguments, and widening the RBAC guard to ne "slinky" now fails two cases. Five things left, one of them a test that does not discriminate.
- This 50 ms window is shorter than client-go's 100 ms WaitForCacheSync poll period, so
Starthas not returned yet either way and the case passes unchanged against the oldgo s.run(). Mutating the implementation back togo s.run()keeps this green; widening the window to 500 ms fails that mutant and still passes the real one. (pkg/node_observer/status_informer_test.go:267) - With the per-node GET gone, this branch cannot tell a node outside the nodeSelector from one that does not exist, so a wrong node name in a request
nodespayload is swallowed andGenerateOutputstill answers OK. Counting the skipped nodes and surfacing the total would keep a genuine typo visible. (pkg/engines/k8s/kubernetes.go:44) kubeQPSandkubeBurstare still live request parameters: the mapstructure tags remain on the DRA provider and the k8s, NFD and Slinky engines,getParametersvalidates them with a 400, and config params apply only when the payload omits the key, so a payload value wins overkubeClient. This PR adds two more of them and drops the last entries documenting any, which the doc-impact table asks to keep in sync. (docs/api.md:105)Startnow blocks onrun, butrunonly re-checksstopChbetweenprocesscalls and the news.cancel()reaches only the informer factories. A SIGTERM landing mid-retry waits outDoRequestWithRetries, whosetime.Sleepand timeout-lesshttp.Clientobserve the root context thatStopnever cancels, so shutdown stalls up to the 5 minutemaxRetryAfter. (pkg/node_observer/status_informer.go:136)- The only test for
requestOnAPIServerDeletepasses a live*corev1.Pod, so the newDeletedFinalStateUnknownbranch never runs: deleting that whole case leaves the package green. Relists after a watch gap deliver tombstones, which is the rolling-deployment path this handler was added for. (pkg/node_observer/status_informer_test.go:169)
| select { | ||
| case err := <-done: | ||
| require.Failf(t, "Start returned before Stop", "error: %v", err) | ||
| case <-time.After(50 * time.Millisecond): |
There was a problem hiding this comment.
This 50 ms window is shorter than client-go's 100 ms WaitForCacheSync poll period, so Start has not returned yet either way and the case passes unchanged against the old go s.run(). Mutating the implementation back to go s.run() keeps this green; widening the window to 500 ms fails that mutant and still passes the real one.
| node, ok := eng.cachedNodeMap[nodeName] | ||
| if !ok { | ||
| if len(eng.params.NodeSelector) != 0 { | ||
| klog.Warningf("Skipping topology labels for node %q because it does not match the engine nodeSelector", nodeName) |
There was a problem hiding this comment.
With the per-node GET gone, this branch cannot tell a node outside the nodeSelector from one that does not exist, so a wrong node name in a request nodes payload is swallowed and GenerateOutput still answers OK. Counting the skipped nodes and surfacing the total would keep a genuine typo visible.
| - **configUpdateMode**: (optional) Used in: [`slinky`]. By default, the full topology YAML is written in the Slurm ConfigMap. `skeleton-only` overrides to include switches or blocks only (no node lines); `none` skips updating the topology key in the ConfigMap. | ||
| - **nodes**: (optional) Supplies the cluster nodes used for topology generation as an array of regions mapping instance IDs to node names. | ||
|
|
||
| For Helm deployments, configure Kubernetes client rate limits only through |
There was a problem hiding this comment.
kubeQPS and kubeBurst are still live request parameters: the mapstructure tags remain on the DRA provider and the k8s, NFD and Slinky engines, getParameters validates them with a 400, and config params apply only when the payload omits the key, so a payload value wins over kubeClient. This PR adds two more of them and drops the last entries documenting any, which the doc-impact table asks to keep in sync.
|
|
||
| go s.run() | ||
|
|
||
| s.run() |
There was a problem hiding this comment.
Start now blocks on run, but run only re-checks stopCh between process calls and the new s.cancel() reaches only the informer factories. A SIGTERM landing mid-retry waits out DoRequestWithRetries, whose time.Sleep and timeout-less http.Client observe the root context that Stop never cancels, so shutdown stalls up to the 5 minute maxRetryAfter.
| )) | ||
| } | ||
|
|
||
| func TestAPIServerPodDeleteQueuesRequest(t *testing.T) { |
There was a problem hiding this comment.
The only test for requestOnAPIServerDelete passes a live *corev1.Pod, so the new DeletedFinalStateUnknown branch never runs: deleting that whole case leaves the package green. Relists after a watch gap deliver tombstones, which is the rolling-deployment path this handler was added for.
2 participants
Description
Large-cluster topology reconciliation could spend several minutes waiting on client-go’s default rate limiter. The Kubernetes engine performed a per-node GET followed by a full Node UPDATE, generating roughly two API requests per node even when topology labels were already current.
This PR reduces Kubernetes API traffic and adds consistent rate-limit configuration across Kubernetes-backed components.
Changes
nvidia.com/gpu.cliquebehavior.updatetopatch.kubeQPSandkubeBurstsupport to the Kubernetes and NFD engines.closes #437