benchmarks: avoid unreliable kubectl attach when fetching metrics#5296
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts how benchmark metrics are fetched to avoid output duplication/collisions caused by kubectl’s attach behavior, by switching to a “create pod → wait → read logs → delete pod” flow and by enhancing the e2e kubectl wrapper to expose stdout/stderr separately.
Changes:
- Extend the e2e
kubectl.Commandwrapper to capture stdout and stderr independently, and addRunStdout. - Update benchmark metrics collection to avoid
kubectl run --attachand instead fetch curl output viakubectl logsafter the pod completes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| e2e/testenv/kubectl/kubectl.go | Split stdout/stderr capture; add RunStdout helper used by benchmarks. |
| benchmarks/record/record.go | Replace kubectl run --attach --rm metrics fetch with pod lifecycle + kubectl logs approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return fmt.Errorf("curl pod %s finished with phase %s", pod, phase) | ||
| } | ||
|
|
||
| out, _, err := k.RunStdout("logs", "--namespace", "cattle-fleet-system", pod) |
There was a problem hiding this comment.
nit: in case we run into more troubles getting these logs, we could replace creating a new pod with a port forward for fetching metrics, as done in the dump CLI.
There was a problem hiding this comment.
Hope it won't be necessary, but yeah, we may have to do that.
Fixes the benchmarks in conjunction with #5267
Additional Information
Checklist