Telemetry store agent tested and dusted - #1
Conversation
- Introduce GPUTelemetry struct with utilization, memory, ECC fields - Introduce NodeTelemetry struct with GPU slice, topology, staleness - Add Validate(), DeepCopy(), IsStale(), IsStaleAt() methods - Define sentinel constants (UnknownUtilization, UnknownMemory) - SchemaVersion field for future-proof struct evolution
…d deep copy - 23 test functions covering GPUTelemetry and NodeTelemetry - Validate happy/sad paths: empty GPUID, out-of-range utilization, negative memory, free-exceeds-total, duplicate GPU IDs - DeepCopy independence: GPU slice, topology maps, nil cases - IsStale/IsStaleAt boundary and deterministic tests - TopologyMatrix deep copy edge case
- Define GPUNodeStatus, GPUNodeStatusSpec, GPUNodeStatusStatus, GPUStatusEntry, and GPUNodeStatusList structs - JSON tags match CRD YAML field names for serialization - Hand-written DeepCopyInto/DeepCopyObject methods (no code-gen) - GPU slice deep copy creates independent backing arrays - Implements runtime.Object interface for client-go compatibility
- Implement Set/Get/Delete/List/Snapshot/Len/MarkStale/MarkStaleAt - sync.RWMutex: exclusive write lock, shared read lock - Deep-copy on all read and write boundaries (no aliasing) - 20 tests: CRUD, staleness sweep, snapshot independence, 4 concurrent stress tests (reads+writes, mark-stale, snapshot, set+delete)
- Agent.Run(ctx) polls GPUStatsProvider on configurable interval - Per-GPU retry with MaxRetries and RetryDelay - Sentinel values (Known=false) on exhausted retries - MetricsHook callbacks: OnPollComplete, OnStoreWrite - Context cancellation marks all store records stale (1ns threshold) - 17 tests: constructor validation, PollOnce happy/error paths, topology error non-fatal, multi-GPU independence, RunOneTick, context cancellation, shutdown staleness, MetricsHook, race test
- apiextensions.k8s.io/v1 CustomResourceDefinition - Group: gpu.amshithnair.dev, version: v1alpha1 - Status subresource enabled (agent-only writes to .status) - OpenAPI v3 schema with full validation constraints - Printer columns: Node, Stale, LastUpdated, Age - Short name: gpuns, scope: Namespaced
…cklist - docs/phase3.md: architecture, data model, store/agent design, concurrency model, staleness model, failure handling, testing guide, future extensions, known limitations, rollback notes - scripts/phase3-test.ps1 + .sh: automated quality gate (fmt, vet, lint, build, test, race) - scripts/phase3-manual.ps1 + .sh: cluster CRD lifecycle verification - checklists/phase3-manual-checklist.md: 60-test inventory across 3 packages with step-by-step sign-off procedure
📝 WalkthroughWalkthroughPhase 3 adds versioned telemetry models, a thread-safe store, a retrying polling agent, GPUNodeStatus API and CRD definitions, cross-platform verification scripts, architecture documentation, and a manual verification checklist. ChangesPhase 3 telemetry implementation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Agent
participant GPUStatsProvider
participant TelemetryStore
participant MetricsHook
Agent->>GPUStatsProvider: poll GPU metrics with retries
GPUStatsProvider-->>Agent: return metrics or errors
Agent->>TelemetryStore: write snapshot and mark stale records
Agent->>MetricsHook: report poll and store events
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (9)
pkg/agent/agent.go (3)
215-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFold
buildSnapshotintopollto remove the duplicated snapshot literal.Both paths build the identical struct and both call
GetTopology; having two copies invites drift (e.g. the cancellation path would silently miss any new field).♻️ Proposed refactor
for _, gpuID := range a.cfg.GPUIDs { select { case <-ctx.Done(): - remaining := len(a.cfg.GPUIDs) - len(gpus) - errCount += remaining + errCount += len(a.cfg.GPUIDs) - len(gpus) for i := len(gpus); i < len(a.cfg.GPUIDs); i++ { gpus = append(gpus, conservativeGPU(a.cfg.GPUIDs[i])) } return a.buildSnapshot(gpus), errCount default: } @@ - // Collect topology (best-effort; errors produce a zero TopologyMatrix). - topo, _ := a.provider.GetTopology(a.cfg.NodeID) - - snap := telemetry.NodeTelemetry{ - NodeID: a.cfg.NodeID, - SchemaVersion: telemetry.SchemaVersion, - GPUs: gpus, - Topology: topo, - CollectedAt: a.cfg.clock(), - Stale: false, - } - return snap, errCount + return a.buildSnapshot(gpus), errCount }🤖 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 `@pkg/agent/agent.go` around lines 215 - 268, Fold the cancellation-path snapshot construction from buildSnapshot into poll so NodeTelemetry is assembled in one place. Preserve the existing topology lookup, collected timestamp, fields, and cancellation behavior, then remove the now-unused Agent.buildSnapshot method.
187-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
durationbypasses the injected clock.
startcomes froma.cfg.clock()but the elapsed time usestime.Since, so any non-real clock yields a meaningless duration inOnPollComplete. Use the same source for both.♻️ Proposed fix
start := a.cfg.clock() snapshot, errCount := a.poll(ctx) - duration := time.Since(start) + duration := a.cfg.clock().Sub(start)🤖 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 `@pkg/agent/agent.go` around lines 187 - 201, Update runOneTick to calculate duration using a second reading from the injected a.cfg.clock instead of time.Since(start), ensuring OnPollComplete receives elapsed time from the same clock source used for start.
344-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the three retry helpers into one generic helper. The bodies are identical, and this module already targets Go 1.22, so a single free function taking
*Agentcan replace all three withoutinterface{}or reflection.🤖 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 `@pkg/agent/agent.go` around lines 344 - 393, Replace retryFloat, retryInt64, and retryUint64 with one Go 1.22 generic free function that accepts *Agent and a typed callback, preserving the existing retry count, delay, success return, and final error behavior; update all callers to use the generic helper and remove the duplicated methods.pkg/store/store.go (1)
125-148: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate the two sweep implementations.
♻️ Proposed refactor
func (s *TelemetryStore) MarkStale(threshold time.Duration) { - now := time.Now() - s.mu.Lock() - for id, rec := range s.records { - if !rec.Stale && rec.IsStaleAt(now, threshold) { - rec.Stale = true - s.records[id] = rec - } - } - s.mu.Unlock() + s.MarkStaleAt(time.Now(), threshold) }🤖 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 `@pkg/store/store.go` around lines 125 - 148, Deduplicate the stale-record sweep by having MarkStale obtain the current time and delegate to MarkStaleAt, leaving the iteration and mutation logic centralized in MarkStaleAt. Preserve the existing stale-marking behavior and locking semantics.pkg/telemetry/model.go (1)
157-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse
IsStaleintoIsStaleAt.Duplicated staleness logic; keeps the boundary semantics in one place.
♻️ Proposed refactor
func (n NodeTelemetry) IsStale(threshold time.Duration) bool { - if n.CollectedAt.IsZero() { - // A zero timestamp is treated as infinitely stale. - return true - } - return time.Since(n.CollectedAt) > threshold + return n.IsStaleAt(time.Now(), threshold) }🤖 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 `@pkg/telemetry/model.go` around lines 157 - 173, Remove the duplicated staleness logic from IsStale and make it delegate to IsStaleAt using the current time. Preserve the existing zero-timestamp and threshold boundary semantics centralized in IsStaleAt.pkg/store/store_test.go (1)
36-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
Set's input deep copy.Reads are well covered, but the write boundary isn't: mutating the caller's record after
Setshould not affect the store.💚 Suggested test
func TestStore_Set_DeepCopiesInput(t *testing.T) { s := store.NewTelemetryStore() rec := telemetry.NodeTelemetry{ NodeID: "node-1", SchemaVersion: telemetry.SchemaVersion, CollectedAt: time.Now(), GPUs: []telemetry.GPUTelemetry{{GPUID: "gpu-0"}}, Topology: telemetry.TopologyMatrix{Matrix: map[string]map[string]int{"gpu-0": {"gpu-1": 1}}}, } s.Set(rec) rec.GPUs[0].GPUID = "mutated" rec.Topology.Matrix["gpu-0"]["gpu-1"] = 999 got, _ := s.Get("node-1") if got.GPUs[0].GPUID != "gpu-0" || got.Topology.Matrix["gpu-0"]["gpu-1"] != 1 { t.Error("store aliases caller-owned data passed to Set") } }🤖 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 `@pkg/store/store_test.go` around lines 36 - 48, Add a test alongside TestStore_SetAndGet that constructs a record with nested GPU and topology map data, calls Set, mutates those caller-owned nested fields afterward, and verifies Get still returns the original values, confirming Set deep-copies its input.scripts/phase3-manual.sh (2)
55-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
kubectl version --shortis deprecated/removed on newer kubectl clients.This is already defensively wrapped (
2>/dev/null | head -1 || true), so it won't break the script, but on kubectl releases where--shortis fully removed it'll just print a blank client-version line instead of useful info.🩹 Proposed fix
-echo " kubectl : $(kubectl version --client --short 2>/dev/null | head -1 || true)" +echo " kubectl : $(kubectl version --client -o json 2>/dev/null | jq -r '.clientVersion.gitVersion' || 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 `@scripts/phase3-manual.sh` at line 55, Update the kubectl version command in the version-reporting echo to use the current supported client-version invocation without the deprecated --short flag, while preserving the existing stderr suppression, first-line extraction, and non-failing fallback behavior.
17-17: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cleanup on failure —
set -eaborts before Section D runs.Same concern as the PowerShell counterpart: any failing step in Sections A-C exits the script immediately (via
set -e), leaving the CRD and sample object applied to the cluster with no cleanup attempted. Atrap 'cleanup' EXIT(with cleanup guarded to runkubectl delete ... --ignore-not-found) would make this idempotent-safe on failure.Also applies to: 67-169
🤖 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 `@scripts/phase3-manual.sh` at line 17, Update scripts/phase3-manual.sh to register an EXIT trap that invokes the existing cleanup logic when Sections A–C fail under set -e. Ensure cleanup uses guarded kubectl delete operations with --ignore-not-found so CRDs and sample objects are removed safely without masking the original failure.scripts/phase3-manual.ps1 (1)
41-48: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cleanup on failure — a mid-script
Assert-ExitCodefailure leaves the CRD/object applied.Any failed step from A1 through C2 calls
exit 1immediately, skipping Section D. On a shared test cluster this leavesgpunodestatuses.gpu.amshithnair.devand the sample object behind for manual cleanup. Consider wrapping the lifecycle steps in try/finally so cleanup always runs.♻️ Sketch
+try { # SECTION A, B, C steps... +} +finally { + Write-Section "SECTION D: Cleanup" + kubectl delete gpuns node-gpu-01 -n gpu-scheduler --ignore-not-found + kubectl delete crd gpunodestatuses.gpu.amshithnair.dev --ignore-not-found +}Also applies to: 88-229
🤖 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 `@scripts/phase3-manual.ps1` around lines 41 - 48, Ensure the lifecycle steps from A1 through C2 execute within try/finally control flow so failures from Assert-ExitCode do not bypass cleanup. Move or invoke the Section D cleanup for the CRD and sample object from the finally path, while preserving the existing success/failure reporting and cleanup behavior.
🤖 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 `@checklists/phase3-manual-checklist.md`:
- Around line 322-326: The checklist’s Section C description is inaccurate:
rename it from “Cleanup” to “Stale Simulation” and remove the claim that the PS1
script skips it, while retaining the stale flag patch and verification steps for
both scripts.
- Around line 269-283: Update the Phase 3 checklist assertions to match the
implemented API: change GPUNodeStatusStatus.LastUpdated from string to
time.Time, and remove the DeepCopyObject/runtime.Object requirement from the
deepcopy verification or explicitly defer it to Phase 4. Keep the remaining
deep-copy and field/tag checks unchanged.
In `@pkg/agent/agent_test.go`:
- Around line 419-452: Update TestAgent_StalenessThreshold_Applied so
StalenessThreshold is meaningfully greater than PollInterval, while keeping the
existing poll and staleness behavior under test. Adjust the timing values
together as needed so a freshly written record is not marked stale before the
assertion, including under slower or race-enabled execution.
- Around line 335-344: Update the shutdown assertion in the agent test to
document the actual MarkStale(1) behavior used by Agent.Run, rather than
MarkStale(0). Remove the dead Get failure early-return and assert directly that
the existing record is present and stale after shutdown.
In `@pkg/agent/agent.go`:
- Around line 291-307: In the memory update block of the agent’s GPU telemetry
collection flow, stop assigning freeMem to TotalMemoryMB. Until the provider
exposes total memory, set TotalMemoryMB to telemetry.UnknownMemory and ensure
the known-state flags remain consistent so Validate accepts the record—prefer
adding and propagating a separate TotalMemoryKnown flag if the model supports
it; otherwise set MemoryKnown to false.
In `@pkg/api/v1alpha1/types.go`:
- Around line 101-102: Change the LastUpdated field from time.Time to *time.Time
so json omitempty omits it when no successful poll has occurred. Update any
assignments or consumers of LastUpdated to use pointer values while preserving
the existing RFC3339 timestamp behavior for populated values.
In `@pkg/store/store.go`:
- Around line 6-9: Correct the package documentation for the store’s read
operations, especially List and Snapshot, so it no longer claims allocations
never occur while holding the read lock. Keep the implementation unchanged and
accurately describe the current locking and copying behavior.
In `@scripts/phase3-test.ps1`:
- Around line 145-157: Update the STEP 2 formatting check in phase3-test.ps1 to
use gofmt -l ./... instead of go fmt ./..., so it only lists unformatted files
without modifying the working tree. Preserve the existing failure handling and
pass behavior, including reporting listed offenders and failing when any are
found.
- Line 27: Update the native version and environment probes in phase3-test.ps1,
including go version, kind version, docker --version, golangci-lint version, and
go env, so they do not merge stderr with 2>&1 under PowerShell 5.1. Execute
these probes within a Continue error-action scope or otherwise capture stderr
without promoting benign native output to terminating errors, while preserving
the script’s global Stop behavior elsewhere.
In `@scripts/phase3-test.sh`:
- Around line 105-111: Update the STEP 2 formatting check in
scripts/phase3-test.sh to use gofmt -l instead of go fmt ./..., so the
validation only lists unformatted files without mutating the working tree.
Preserve the existing FMT_OUT failure handling, messaging, and pass behavior.
- Around line 66-70: Update the kubectl version reporting command in the kubectl
availability check to use kubectl version --client with the JSONPath expression
for clientVersion.gitVersion, replacing the removed --short flag while
preserving the existing fallback behavior.
---
Nitpick comments:
In `@pkg/agent/agent.go`:
- Around line 215-268: Fold the cancellation-path snapshot construction from
buildSnapshot into poll so NodeTelemetry is assembled in one place. Preserve the
existing topology lookup, collected timestamp, fields, and cancellation
behavior, then remove the now-unused Agent.buildSnapshot method.
- Around line 187-201: Update runOneTick to calculate duration using a second
reading from the injected a.cfg.clock instead of time.Since(start), ensuring
OnPollComplete receives elapsed time from the same clock source used for start.
- Around line 344-393: Replace retryFloat, retryInt64, and retryUint64 with one
Go 1.22 generic free function that accepts *Agent and a typed callback,
preserving the existing retry count, delay, success return, and final error
behavior; update all callers to use the generic helper and remove the duplicated
methods.
In `@pkg/store/store_test.go`:
- Around line 36-48: Add a test alongside TestStore_SetAndGet that constructs a
record with nested GPU and topology map data, calls Set, mutates those
caller-owned nested fields afterward, and verifies Get still returns the
original values, confirming Set deep-copies its input.
In `@pkg/store/store.go`:
- Around line 125-148: Deduplicate the stale-record sweep by having MarkStale
obtain the current time and delegate to MarkStaleAt, leaving the iteration and
mutation logic centralized in MarkStaleAt. Preserve the existing stale-marking
behavior and locking semantics.
In `@pkg/telemetry/model.go`:
- Around line 157-173: Remove the duplicated staleness logic from IsStale and
make it delegate to IsStaleAt using the current time. Preserve the existing
zero-timestamp and threshold boundary semantics centralized in IsStaleAt.
In `@scripts/phase3-manual.ps1`:
- Around line 41-48: Ensure the lifecycle steps from A1 through C2 execute
within try/finally control flow so failures from Assert-ExitCode do not bypass
cleanup. Move or invoke the Section D cleanup for the CRD and sample object from
the finally path, while preserving the existing success/failure reporting and
cleanup behavior.
In `@scripts/phase3-manual.sh`:
- Line 55: Update the kubectl version command in the version-reporting echo to
use the current supported client-version invocation without the deprecated
--short flag, while preserving the existing stderr suppression, first-line
extraction, and non-failing fallback behavior.
- Line 17: Update scripts/phase3-manual.sh to register an EXIT trap that invokes
the existing cleanup logic when Sections A–C fail under set -e. Ensure cleanup
uses guarded kubectl delete operations with --ignore-not-found so CRDs and
sample objects are removed safely without masking the original failure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e52565c-cd83-404e-a914-7cbfe1e66651
📒 Files selected for processing (15)
checklists/phase3-manual-checklist.mddocs/phase3.mdmanifests/gpunodestatus-crd.yamlpkg/agent/agent.gopkg/agent/agent_test.gopkg/api/v1alpha1/deepcopy.gopkg/api/v1alpha1/types.gopkg/store/store.gopkg/store/store_test.gopkg/telemetry/model.gopkg/telemetry/model_test.goscripts/phase3-manual.ps1scripts/phase3-manual.shscripts/phase3-test.ps1scripts/phase3-test.sh
| Open `pkg/api/v1alpha1/types.go` and verify: | ||
|
|
||
| - [ ] `GPUNodeStatus` struct has `TypeMeta`, `ObjectMeta`, `Spec`, and `Status` fields. | ||
| - [ ] `GPUNodeStatusSpec` has `NodeName string` and `GPUIDs []string`. | ||
| - [ ] `GPUNodeStatusStatus` has `GPUs []GPUStatusEntry`, `LastUpdated string`, `Stale bool`, | ||
| `SchemaVersion int`. | ||
| - [ ] `GPUStatusEntry` mirrors the CRD schema fields (gpuID, utilizationPct, etc.). | ||
| - [ ] JSON tags on all fields match the CRD YAML field names exactly. | ||
|
|
||
| Open `pkg/api/v1alpha1/deepcopy.go` and verify: | ||
|
|
||
| - [ ] `DeepCopyInto` methods exist for all CRD types. | ||
| - [ ] `DeepCopyObject` method exists on `GPUNodeStatus`, returning `runtime.Object`. | ||
| - [ ] GPU slice deep copy creates a new slice (not just a header copy). | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Checklist assertions don't match the actual Go types.
Two items here can never pass as written:
- Line 273 says
LastUpdated string, butpkg/api/v1alpha1/types.godeclaresLastUpdated time.Time. - Line 281 requires a
DeepCopyObjectmethod returningruntime.Object, butpkg/api/v1alpha1/deepcopy.gohas no such method — and can't, since the package doc in types.go explicitly states Phase 3 avoids importingk8s.io/apimachinery(soruntime.Objectisn't available). This item is currently unsatisfiable, which blocks the checklist's own sign-off gate ("Do not proceed to Phase 4 until all boxes are checked").
Update the checklist to match the current type (time.Time) and either drop the DeepCopyObject requirement or explicitly defer it to Phase 4.
🤖 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 `@checklists/phase3-manual-checklist.md` around lines 269 - 283, Update the
Phase 3 checklist assertions to match the implemented API: change
GPUNodeStatusStatus.LastUpdated from string to time.Time, and remove the
DeepCopyObject/runtime.Object requirement from the deepcopy verification or
explicitly defer it to Phase 4. Keep the remaining deep-copy and field/tag
checks unchanged.
| - [ ] **Section C — Cleanup** (bash script only; PS1 skips directly to cleanup): | ||
| - [ ] Stale flag set to `true` via status patch. | ||
| - [ ] `kubectl get gpuns` shows `Stale = true` in printer column. | ||
|
|
||
| - [ ] **Section D — Cleanup**: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Incorrect claim about PS1 script behavior.
This states Section C ("Cleanup"... actually Stale Simulation) is "bash script only; PS1 skips directly to cleanup." But scripts/phase3-manual.ps1 implements SECTION C: Stale Simulation (patches stale=true and verifies it) identically to the bash script. The section label itself also appears mislabeled as "Cleanup" when both scripts title it "Stale Simulation."
🤖 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 `@checklists/phase3-manual-checklist.md` around lines 322 - 326, The
checklist’s Section C description is inaccurate: rename it from “Cleanup” to
“Stale Simulation” and remove the claim that the PS1 script skips it, while
retaining the stale flag patch and verification steps for both scripts.
| // After shutdown the agent calls MarkStale(0), which marks all records stale | ||
| // regardless of their CollectedAt timestamp. | ||
| rec, ok := st.Get(nodeID) | ||
| if !ok { | ||
| // Record might have been deleted — that's also acceptable; skip assertion. | ||
| return | ||
| } | ||
| if !rec.Stale { | ||
| t.Error("record should be stale after agent shutdown (MarkStale(0) called)") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment doesn't match the implementation.
Agent.Run calls a.store.MarkStale(1) (1ns threshold) on shutdown, not MarkStale(0). Also, nothing in this test deletes the record, so the early-return branch is dead and weakens the assertion.
📝 Proposed fix
- // After shutdown the agent calls MarkStale(0), which marks all records stale
- // regardless of their CollectedAt timestamp.
+ // After shutdown the agent calls MarkStale(1ns), which marks every record
+ // with a real CollectedAt timestamp stale.
rec, ok := st.Get(nodeID)
if !ok {
- // Record might have been deleted — that's also acceptable; skip assertion.
- return
+ t.Fatal("expected record to still be present after shutdown")
}
if !rec.Stale {
- t.Error("record should be stale after agent shutdown (MarkStale(0) called)")
+ t.Error("record should be stale after agent shutdown")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // After shutdown the agent calls MarkStale(0), which marks all records stale | |
| // regardless of their CollectedAt timestamp. | |
| rec, ok := st.Get(nodeID) | |
| if !ok { | |
| // Record might have been deleted — that's also acceptable; skip assertion. | |
| return | |
| } | |
| if !rec.Stale { | |
| t.Error("record should be stale after agent shutdown (MarkStale(0) called)") | |
| } | |
| // After shutdown the agent calls MarkStale(1ns), which marks every record | |
| // with a real CollectedAt timestamp stale. | |
| rec, ok := st.Get(nodeID) | |
| if !ok { | |
| t.Fatal("expected record to still be present after shutdown") | |
| } | |
| if !rec.Stale { | |
| t.Error("record should be stale after agent shutdown") | |
| } |
🤖 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 `@pkg/agent/agent_test.go` around lines 335 - 344, Update the shutdown
assertion in the agent test to document the actual MarkStale(1) behavior used by
Agent.Run, rather than MarkStale(0). Remove the dead Get failure early-return
and assert directly that the existing record is present and stale after
shutdown.
| func TestAgent_StalenessThreshold_Applied(t *testing.T) { | ||
| nodeID := "node-1" | ||
| fake := buildFake(nodeID, 1) | ||
| cfg := fastConfig(nodeID, 1) | ||
| cfg.StalenessThreshold = 5 * time.Millisecond | ||
| cfg.PollInterval = 5 * time.Millisecond | ||
| st := store.NewTelemetryStore() | ||
|
|
||
| // Manually insert a very old record. | ||
| old := telemetry.NodeTelemetry{ | ||
| NodeID: nodeID, | ||
| SchemaVersion: telemetry.SchemaVersion, | ||
| CollectedAt: time.Now().Add(-1 * time.Hour), | ||
| } | ||
| st.Set(old) | ||
|
|
||
| a, _ := agent.NewAgent(cfg, fake, st) | ||
|
|
||
| // Run one full tick (poll + MarkStale). | ||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) | ||
| defer cancel() | ||
| go func() { _ = a.Run(ctx) }() | ||
| time.Sleep(20 * time.Millisecond) | ||
|
|
||
| rec, ok := st.Get(nodeID) | ||
| if !ok { | ||
| t.Fatal("expected record in store") | ||
| } | ||
| // After at least one poll cycle the agent writes a fresh record and then | ||
| // MarkStale runs. The fresh record should NOT be stale. | ||
| if rec.Stale { | ||
| t.Error("fresh record written by agent should not be stale immediately after poll") | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Flaky: StalenessThreshold equals PollInterval.
With both at 5ms, a record written by tick N is already ≥5ms old when tick N+1's MarkStale runs, so under load (especially with -race) the assertion at Line 449 fails intermittently. Give staleness meaningful headroom over the poll interval.
💚 Proposed fix
- cfg.StalenessThreshold = 5 * time.Millisecond
+ cfg.StalenessThreshold = 200 * time.Millisecond
cfg.PollInterval = 5 * time.Millisecond📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func TestAgent_StalenessThreshold_Applied(t *testing.T) { | |
| nodeID := "node-1" | |
| fake := buildFake(nodeID, 1) | |
| cfg := fastConfig(nodeID, 1) | |
| cfg.StalenessThreshold = 5 * time.Millisecond | |
| cfg.PollInterval = 5 * time.Millisecond | |
| st := store.NewTelemetryStore() | |
| // Manually insert a very old record. | |
| old := telemetry.NodeTelemetry{ | |
| NodeID: nodeID, | |
| SchemaVersion: telemetry.SchemaVersion, | |
| CollectedAt: time.Now().Add(-1 * time.Hour), | |
| } | |
| st.Set(old) | |
| a, _ := agent.NewAgent(cfg, fake, st) | |
| // Run one full tick (poll + MarkStale). | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) | |
| defer cancel() | |
| go func() { _ = a.Run(ctx) }() | |
| time.Sleep(20 * time.Millisecond) | |
| rec, ok := st.Get(nodeID) | |
| if !ok { | |
| t.Fatal("expected record in store") | |
| } | |
| // After at least one poll cycle the agent writes a fresh record and then | |
| // MarkStale runs. The fresh record should NOT be stale. | |
| if rec.Stale { | |
| t.Error("fresh record written by agent should not be stale immediately after poll") | |
| } | |
| } | |
| func TestAgent_StalenessThreshold_Applied(t *testing.T) { | |
| nodeID := "node-1" | |
| fake := buildFake(nodeID, 1) | |
| cfg := fastConfig(nodeID, 1) | |
| cfg.StalenessThreshold = 200 * time.Millisecond | |
| cfg.PollInterval = 5 * time.Millisecond | |
| st := store.NewTelemetryStore() | |
| // Manually insert a very old record. | |
| old := telemetry.NodeTelemetry{ | |
| NodeID: nodeID, | |
| SchemaVersion: telemetry.SchemaVersion, | |
| CollectedAt: time.Now().Add(-1 * time.Hour), | |
| } | |
| st.Set(old) | |
| a, _ := agent.NewAgent(cfg, fake, st) | |
| // Run one full tick (poll + MarkStale). | |
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Millisecond) | |
| defer cancel() | |
| go func() { _ = a.Run(ctx) }() | |
| time.Sleep(20 * time.Millisecond) | |
| rec, ok := st.Get(nodeID) | |
| if !ok { | |
| t.Fatal("expected record in store") | |
| } | |
| // After at least one poll cycle the agent writes a fresh record and then | |
| // MarkStale runs. The fresh record should NOT be stale. | |
| if rec.Stale { | |
| t.Error("fresh record written by agent should not be stale immediately after poll") | |
| } | |
| } |
🤖 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 `@pkg/agent/agent_test.go` around lines 419 - 452, Update
TestAgent_StalenessThreshold_Applied so StalenessThreshold is meaningfully
greater than PollInterval, while keeping the existing poll and staleness
behavior under test. Adjust the timing values together as needed so a freshly
written record is not marked stale before the assertion, including under slower
or race-enabled execution.
| freeMem, err := a.retryInt64(ctx, func() (int64, error) { | ||
| return a.provider.GetFreeMemoryMB(gpuID) | ||
| }) | ||
| // We need TotalMemoryMB too. The interface does not expose it directly; | ||
| // derive it from FreeMemoryMB only (FakeProvider stores exact values). | ||
| // The real NVML provider will be updated in Phase 4 to return total memory. | ||
| // For now, store free memory and mark known/unknown appropriately. | ||
| if err == nil { | ||
| gpu.FreeMemoryMB = freeMem | ||
| gpu.TotalMemoryMB = freeMem // Phase 4 TODO: query total separately | ||
| gpu.MemoryKnown = true | ||
| } else { | ||
| gpu.FreeMemoryMB = telemetry.UnknownMemory | ||
| gpu.TotalMemoryMB = telemetry.UnknownMemory | ||
| gpu.MemoryKnown = false | ||
| anyError = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
TotalMemoryMB = freeMem publishes wrong telemetry with MemoryKnown = true.
Every GPU is reported as having 100% of its memory free. Since FreeMemoryMB == TotalMemoryMB is a valid combination, Validate won't catch it either, so downstream scorers will read a fabricated total as authoritative. Prefer marking total unknown rather than inventing it: set TotalMemoryMB = telemetry.UnknownMemory until the provider exposes total memory — but note that then Validate will reject the record because TotalMemoryMB <= 0 while MemoryKnown is true, so the model needs a separate TotalMemoryKnown flag (or MemoryKnown must be false).
Want me to open an issue tracking the Phase 4 total-memory provider extension?
🤖 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 `@pkg/agent/agent.go` around lines 291 - 307, In the memory update block of the
agent’s GPU telemetry collection flow, stop assigning freeMem to TotalMemoryMB.
Until the provider exposes total memory, set TotalMemoryMB to
telemetry.UnknownMemory and ensure the known-state flags remain consistent so
Validate accepts the record—prefer adding and propagating a separate
TotalMemoryKnown flag if the model supports it; otherwise set MemoryKnown to
false.
| // All mutable state is guarded by a single sync.RWMutex. Read operations | ||
| // (Get, List, Snapshot, Len) acquire a shared read-lock and release it before | ||
| // returning — they never hold the lock while performing heap allocations. | ||
| // Write operations (Set, Delete, MarkStale) acquire an exclusive write-lock. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Package doc contradicts the implementation.
List and Snapshot do allocate and deep-copy while holding the read lock, so the "never hold the lock while performing heap allocations" claim is inaccurate. Either fix the doc or copy under lock into a local slice/map and deep-copy after release.
📝 Doc correction
-// All mutable state is guarded by a single sync.RWMutex. Read operations
-// (Get, List, Snapshot, Len) acquire a shared read-lock and release it before
-// returning — they never hold the lock while performing heap allocations.
-// Write operations (Set, Delete, MarkStale) acquire an exclusive write-lock.
+// All mutable state is guarded by a single sync.RWMutex. Read operations
+// (Get, List, Snapshot, Len) acquire a shared read-lock; List and Snapshot
+// deep-copy each record while the read-lock is held, so their cost scales with
+// store size. Write operations (Set, Delete, MarkStale) take an exclusive lock.Also applies to: 85-106
🤖 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 `@pkg/store/store.go` around lines 6 - 9, Correct the package documentation for
the store’s read operations, especially List and Snapshot, so it no longer
claims allocations never occur while holding the read lock. Keep the
implementation unchanged and accurately describe the current locking and copying
behavior.
| $env:SKIP_RACE = "1" bypass race detector (auto-set on Windows) | ||
| #> | ||
|
|
||
| $ErrorActionPreference = "Stop" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PowerShell ErrorActionPreference Stop native command stderr 2>&1 NativeCommandError
💡 Result:
In modern PowerShell (7.4 and later), the behavior of native commands with regard to error handling has been modernized to prevent the confusion caused by stderr output interacting with $ErrorActionPreference [1][2]. Key points regarding your query: 1. Stderr vs. ErrorActionPreference: Since PowerShell 7.1, the output written by native commands to stderr no longer automatically triggers $ErrorActionPreference='Stop' [3][2]. This change was implemented because many native commands use stderr for non-error information (like progress or status messages), and forcing them to stop execution caused widespread issues [3]. 2. Redirection (2>&1): When you use 2>&1 to redirect stderr to the success stream, that output is treated as standard stream content [4][5]. It is not treated as a PowerShell error record, and therefore, it does not interact with $ErrorActionPreference or produce NativeCommandError objects [1][2]. 3. Controlling Exit-Code Based Errors: If you want a native command to trigger a PowerShell error when it fails (i.e., returns a non-zero exit code), you should use the $PSNativeCommandUseErrorActionPreference variable [6][2]. - When set to $true, PowerShell will emit a NativeCommandExitException if the command exits with a non-zero code [6][7]. - Because this emits a proper PowerShell error, it will respect $ErrorActionPreference [2][8]. If $ErrorActionPreference is set to 'Stop', this error will become a script-terminating or statement-terminating error (depending on context) that can be caught with try/catch blocks [6][2]. In summary, if you want your script to stop when a native command fails, do not rely on stderr content [3]. Instead, set $PSNativeCommandUseErrorActionPreference = $true and ensure your $ErrorActionPreference is set to 'Stop' [6][2]. Top results: [1], [6], [2], [7]
Citations:
- 1: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_preference_variables?view=powershell-7.5
- 2: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_preference_variables?view=powershell-7.4
- 3:
$ErrorActionPreferencedoesn't affect stderr output for native commands MicrosoftDocs/PowerShell-Docs#6466 - 4: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_redirection?view=powershell-7.6
- 5: https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_redirection?view=powershell-7.5
- 6: https://github.com/MicrosoftDocs/PowerShell-Docs/blob/main/reference/7.5/Microsoft.PowerShell.Core/About/about_Error_Handling.md
- 7: Native Command Error Handling PowerShell/PowerShell-RFC#277
- 8: $PSNativeCommandUseErrorActionPreference does not work when error stream is discarded PowerShell/PowerShell#27543
🏁 Script executed:
set -euo pipefail
# Locate and inspect the script with line numbers
git ls-files 'scripts/phase3-test.ps1'
wc -l scripts/phase3-test.ps1
cat -n scripts/phase3-test.ps1 | sed -n '1,220p'Repository: amshithnair/gpu-aware-scheduler
Length of output: 9163
🌐 Web query:
Windows PowerShell 5.1 native command stderr 2>&1 $ErrorActionPreference Stop NativeCommandError
💡 Result:
In Windows PowerShell 5.1, when you use the redirection 2>&1 with a native command, the command's stderr output is captured and injected into PowerShell’s success output stream as [System.Management.Automation.ErrorRecord] objects [1][2][3]. The issues you are experiencing with $ErrorActionPreference = 'Stop' and NativeCommandError are caused by the following behavior in version 5.1: 1. Redirection Triggers Error Stream: Because the stderr lines are wrapped in ErrorRecord objects to become part of the PowerShell pipeline, PowerShell interprets these records as errors [4][1]. 2. Interaction with Stop: When $ErrorActionPreference is set to 'Stop', PowerShell promotes these non-terminating error records to terminating errors, causing your script to abort immediately upon receiving the first line of stderr output from the native command [4][5]. Workarounds for Windows PowerShell 5.1: If you need to use 2>&1 while $ErrorActionPreference = 'Stop' is in effect, you must avoid the automatic error handling associated with these records: * Temporarily change the preference: Wrap your native command execution in a scope where $ErrorActionPreference is set to 'Continue' [4]. powershell $oldPreference = $ErrorActionPreference $ErrorActionPreference = 'Continue' try { $output = & your-native-command 2>&1 } finally { $ErrorActionPreference = $oldPreference } * Convert objects to strings: You can pipe the output to % ToString or similar methods to force the ErrorRecord objects to render as simple string output, which prevents them from being treated as errors by the preference variable [5][1]. powershell # Merges stderr into success stream and converts error records to strings $output = & your-native-command 2>&1 | ForEach-Object { $_.ToString() } * Separate processing: If you need to keep stdout and stderr separate, avoid 2>&1 in PowerShell. Instead, redirect stderr to a temporary file, then read it after the command completes [2]. powershell $tempFile = [System.IO.Path]::GetTempFileName() $output = & your-native-command 2> $tempFile $errors = Get-Content $tempFile Remove-Item $tempFile Note: These issues are specific to Windows PowerShell 5.1. Newer versions of PowerShell (Core/7+) have introduced improvements and experimental features (such as PSNotApplyErrorActionToStderr) to better handle native command output and exit codes without this undesired side effect [6][7][8].
Citations:
- 1: https://stackoverflow.com/questions/52970939/right-usage-of-21-tee-or-tee-with-powershell
- 2: https://stackoverflow.com/questions/8184827/powershell-capture-the-output-from-external-process-that-writes-to-stderr-in-a
- 3: https://stackoverflow.com/questions/44488202/powershell-streaming-output
- 4: https://stackoverflow.com/questions/10666101/lastexitcode-0-but-false-in-powershell-redirecting-stderr-to-stdout-gives
- 5: https://stackoverflow.com/questions/59366288/dont-throw-powershell-exception-on-writes-to-stderr-from-external-command
- 6: $ErrorActionPreference='Stop' and native output to stderr PowerShell/PowerShell#14273
- 7:
$ErrorActionPreferencedoesn't affect stderr output for native commands MicrosoftDocs/PowerShell-Docs#6466 - 8: Switch
$PSNativeCommandUseErrorActionPreferenceto$truewhen feature is enabled PowerShell/PowerShell#18695
Avoid 2>&1 on native version probes under PowerShell 5.1.
Because this script requires PS 5.1, merging stderr from native commands can surface ErrorRecords; with $ErrorActionPreference = "Stop", benign output from go version, kind version, docker --version, golangci-lint version, or go env can terminate the run. Wrap these probes in a Continue scope or capture stderr without promoting it to an error.
🤖 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 `@scripts/phase3-test.ps1` at line 27, Update the native version and
environment probes in phase3-test.ps1, including go version, kind version,
docker --version, golangci-lint version, and go env, so they do not merge stderr
with 2>&1 under PowerShell 5.1. Execute these probes within a Continue
error-action scope or otherwise capture stderr without promoting benign native
output to terminating errors, while preserving the script’s global Stop behavior
elsewhere.
| Write-Header "STEP 2: go fmt ./..." | ||
| $fmtOut = go fmt ./... 2>&1 | ||
| if ($LASTEXITCODE -ne 0) { | ||
| Write-Host $fmtOut | ||
| Write-Fail "STEP 2: go fmt ./..." "exit code $LASTEXITCODE" | ||
| exit 1 | ||
| } | ||
| if ($fmtOut) { | ||
| Write-Host $fmtOut | ||
| Write-Fail "STEP 2: go fmt ./..." "go fmt reformatted files -- run 'go fmt ./...' locally" | ||
| exit 1 | ||
| } | ||
| Write-Pass "STEP 2: go fmt ./..." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
go fmt rewrites files instead of checking them.
The gate reformats the working tree before failing, so a "failed" run leaves modified sources behind. Use gofmt -l to report offenders without mutating them.
🛠️ Proposed fix
-Write-Header "STEP 2: go fmt ./..."
-$fmtOut = go fmt ./... 2>&1
+Write-Header "STEP 2: gofmt -l ."
+$fmtOut = gofmt -l . 2>&1
if ($LASTEXITCODE -ne 0) {
Write-Host $fmtOut
- Write-Fail "STEP 2: go fmt ./..." "exit code $LASTEXITCODE"
+ Write-Fail "STEP 2: gofmt -l ." "exit code $LASTEXITCODE"
exit 1
}
if ($fmtOut) {
Write-Host $fmtOut
- Write-Fail "STEP 2: go fmt ./..." "go fmt reformatted files -- run 'go fmt ./...' locally"
+ Write-Fail "STEP 2: gofmt -l ." "files are not gofmt-clean -- run 'go fmt ./...' locally"
exit 1
}
-Write-Pass "STEP 2: go fmt ./..."
+Write-Pass "STEP 2: gofmt -l ."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Write-Header "STEP 2: go fmt ./..." | |
| $fmtOut = go fmt ./... 2>&1 | |
| if ($LASTEXITCODE -ne 0) { | |
| Write-Host $fmtOut | |
| Write-Fail "STEP 2: go fmt ./..." "exit code $LASTEXITCODE" | |
| exit 1 | |
| } | |
| if ($fmtOut) { | |
| Write-Host $fmtOut | |
| Write-Fail "STEP 2: go fmt ./..." "go fmt reformatted files -- run 'go fmt ./...' locally" | |
| exit 1 | |
| } | |
| Write-Pass "STEP 2: go fmt ./..." | |
| Write-Header "STEP 2: gofmt -l ." | |
| $fmtOut = gofmt -l . 2>&1 | |
| if ($LASTEXITCODE -ne 0) { | |
| Write-Host $fmtOut | |
| Write-Fail "STEP 2: gofmt -l ." "exit code $LASTEXITCODE" | |
| exit 1 | |
| } | |
| if ($fmtOut) { | |
| Write-Host $fmtOut | |
| Write-Fail "STEP 2: gofmt -l ." "files are not gofmt-clean -- run 'go fmt ./...' locally" | |
| exit 1 | |
| } | |
| Write-Pass "STEP 2: gofmt -l ." |
🤖 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 `@scripts/phase3-test.ps1` around lines 145 - 157, Update the STEP 2 formatting
check in phase3-test.ps1 to use gofmt -l ./... instead of go fmt ./..., so it
only lists unformatted files without modifying the working tree. Preserve the
existing failure handling and pass behavior, including reporting listed
offenders and failing when any are found.
| if command -v kubectl &>/dev/null; then | ||
| echo " kubectl : $(kubectl version --client --short 2>/dev/null | head -1 || true)" | ||
| else | ||
| skip "kubectl -- not found. CRD dry-run unavailable (run phase3-manual.sh separately)" | ||
| fi |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
kubectl version --client --short flag removed release
💡 Result:
The --short flag for the kubectl version command was removed in Kubernetes v1.28 [1][2]. Prior to its removal, the --short output format became the new default behavior for kubectl version, and the --short flag itself was deprecated [1][2]. As a result, running kubectl version with the --short flag in newer versions of kubectl will return an error, such as error: unknown flag: --short [3]. If you need structured version output, the recommended approach is to use the --output flag, such as kubectl version --output=json or kubectl version --output=yaml [1][2][4].
Citations:
- 1: https://github.com/kubernetes/kubernetes/pull/116720/files
- 2: Remove long/golang version information making short the default kubernetes/kubernetes#116720
- 3: Docs: fix kubectl command kubernetes-sigs/kustomize#5760
- 4: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_version/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l scripts/phase3-test.sh
sed -n '55,80p' scripts/phase3-test.shRepository: amshithnair/gpu-aware-scheduler
Length of output: 954
kubectl version --client --short is removed in kubectl 1.28+
With 2>/dev/null || true, this can print a blank version on newer clients. Use kubectl version --client -o jsonpath='{.clientVersion.gitVersion}' instead.
🤖 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 `@scripts/phase3-test.sh` around lines 66 - 70, Update the kubectl version
reporting command in the kubectl availability check to use kubectl version
--client with the JSONPath expression for clientVersion.gitVersion, replacing
the removed --short flag while preserving the existing fallback behavior.
| header "STEP 2: go fmt ./..." | ||
| FMT_OUT="$(go fmt ./... 2>&1)" | ||
| if [ -n "${FMT_OUT}" ]; then | ||
| echo "${FMT_OUT}" | ||
| fail "go fmt reformatted files. Run 'go fmt ./...' locally before re-running." | ||
| fi | ||
| pass "STEP 2: go fmt ./..." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
go fmt mutates the tree; prefer gofmt -l.
Same concern as scripts/phase3-test.ps1 Line 146 — a failing gate should not leave reformatted files behind.
🛠️ Proposed fix
-header "STEP 2: go fmt ./..."
-FMT_OUT="$(go fmt ./... 2>&1)"
+header "STEP 2: gofmt -l ."
+FMT_OUT="$(gofmt -l . 2>&1)"
if [ -n "${FMT_OUT}" ]; then
echo "${FMT_OUT}"
- fail "go fmt reformatted files. Run 'go fmt ./...' locally before re-running."
+ fail "files are not gofmt-clean. Run 'go fmt ./...' locally before re-running."
fi
-pass "STEP 2: go fmt ./..."
+pass "STEP 2: gofmt -l ."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| header "STEP 2: go fmt ./..." | |
| FMT_OUT="$(go fmt ./... 2>&1)" | |
| if [ -n "${FMT_OUT}" ]; then | |
| echo "${FMT_OUT}" | |
| fail "go fmt reformatted files. Run 'go fmt ./...' locally before re-running." | |
| fi | |
| pass "STEP 2: go fmt ./..." | |
| header "STEP 2: gofmt -l ." | |
| FMT_OUT="$(gofmt -l . 2>&1)" | |
| if [ -n "${FMT_OUT}" ]; then | |
| echo "${FMT_OUT}" | |
| fail "files are not gofmt-clean. Run 'go fmt ./...' locally before re-running." | |
| fi | |
| pass "STEP 2: gofmt -l ." |
🤖 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 `@scripts/phase3-test.sh` around lines 105 - 111, Update the STEP 2 formatting
check in scripts/phase3-test.sh to use gofmt -l instead of go fmt ./..., so the
validation only lists unformatted files without mutating the working tree.
Preserve the existing FMT_OUT failure handling, messaging, and pass behavior.
1 participant
Summary by CodeRabbit
New Features
GPUNodeStatusKubernetes resource and schema for exposing node and GPU telemetry.Documentation
Tests and Verification