Repository navigation
tests: add a phase for testing with lots of pods, and reduce cost - #1287
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: justinsb The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (13)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughWalkthroughThe PR updates benchmark entrypoints, expands stress phase syntax and defaults, and refactors execution around parsed and resolved phase objects. Reporting, tracker identifiers, profiler filenames, and capacity-resolution tests now use the ChangesBenchmark phase execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
justinsb
changed the title
kubernetes-prow
Bot
added
the
do-not-merge/work-in-progress
| # | ||
| # This calls the scenario directly rather than reusing the presubmit: the | ||
| # cilium presubmit is deliberately small (3 nodes — per-PR CNI signal only), | ||
| # while the daily baseline runs the full 20-node, single-control-plane |
There was a problem hiding this comment.
Instead of "full" let's call it "larger"
| # with the throughput curve still rising, so this job is where the next | ||
| # ceiling will show up first. | ||
| # | ||
| # NOT YET SAFE TO SCHEDULE: the full sweep needs the APF inflight raise |
There was a problem hiding this comment.
Let's not add this script yet. We want to make sure we are compliant with the policy on larger runs
| # This presubmit is the small leg: its job is to catch cilium-specific | ||
| # regressions (endpoint-create limits, agent client throttling, datapath | ||
| # latency — all per-node effects that show up at any scale), not to measure | ||
| # cluster-scale throughput. The kindnet presubmit keeps the scenario's |
There was a problem hiding this comment.
Let's not talk about kindnet (or any other presubmits) in the cilium presubmit
| # Phases are a comma-separated list of names (see test/stress/main.go): | ||
| # fill long-running background sandboxes (scale) | ||
| # fill long-running background sandboxes (fill-per-node * nodes) | ||
| # fill-utilN top the cluster up to N% total worker pod utilization |
There was a problem hiding this comment.
I think fill-pct40 would be clearer - util means utility to me, not utilization
| # the etcd working set all scale with resident pods, so the near-empty | ||
| # numbers are the optimistic bound and the -full leg is the realistic one. | ||
| # Defaults below are sized for the default 20-node cluster (mif400 on top of | ||
| # 80% fill lands at ~98% of slots, which the tool warns about but allows). |
There was a problem hiding this comment.
This is confusing. I think we should describe that we fill to 10%, then we run the throughput at various in-flight configurations, then we fill to 80% and run with 400 concurrent, and that fits because 400 is 18% of 20 nodes and so we hit 98% utilization
| // FillPerNode * worker nodes; "fill-utilN" entries top the cluster up to | ||
| // N% of worker pod capacity, counting pods already present (pre-existing | ||
| // system pods and earlier fill phases). See resolveFillCounts. | ||
| FillCounts []int `json:"fillCounts,omitempty"` |
There was a problem hiding this comment.
Rather than do this, what if Phase was an interface, and we had something like this:
type Fill struct {
Percentage float32
}
func (f *Fill) Run() error {
}
var _ Phase = &Fill{}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dev/ci/periodics/benchmarks-kops-gcp-hero (1)
39-48: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider scaling
--create-concurrencyfor the 80-node hero run.Node count and control-plane size are scaled 4x relative to the daily periodics, but
CreateConcurrencystays at main.go's default of 20 (not overridden here). Thefill-util80leg alone creates ~6,000 sandboxes; worth confirming 20 concurrent creators and the 60m timeout are sufficient at this scale, or exposing/overriding create-concurrency alongside the other sizing knobs.🤖 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 `@dev/ci/periodics/benchmarks-kops-gcp-hero` around lines 39 - 48, Update the 80-node hero benchmark configuration alongside STRESS_NODE_COUNT, STRESS_CONTROL_PLANE_SIZE, and STRESS_TIMEOUT to explicitly expose or override the create-concurrency setting used by the fill phases; choose a value appropriate for creating roughly 6,000 sandboxes within the existing 60-minute timeout, rather than relying on main.go’s default of 20.
🤖 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.
Nitpick comments:
In `@dev/ci/periodics/benchmarks-kops-gcp-hero`:
- Around line 39-48: Update the 80-node hero benchmark configuration alongside
STRESS_NODE_COUNT, STRESS_CONTROL_PLANE_SIZE, and STRESS_TIMEOUT to explicitly
expose or override the create-concurrency setting used by the fill phases;
choose a value appropriate for creating roughly 6,000 sandboxes within the
existing 60-minute timeout, rather than relying on main.go’s default of 20.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 69905550-de6a-4ad6-9cbf-d329754e2c20
📒 Files selected for processing (8)
dev/ci/periodics/benchmarks-kops-gcp-ciliumdev/ci/periodics/benchmarks-kops-gcp-herodev/ci/periodics/benchmarks-kops-gcp-kindnetdev/ci/presubmits/benchmarks-kops-gcp-ciliumtest/benchmarks/scenarios/benchmarks-kops-gcp/runtest/stress/main.gotest/stress/phaseconfig_test.gotest/stress/phases.go
| log.Printf("Fill phase %s: %d sandboxes (%d per worker node * %d nodes)", raw, cfg.FillCounts[i], cfg.FillPerNode, clusterInfo.Nodes) | ||
| case isFillUtil(raw): | ||
| pct, _ := fillUtilPercent(raw) | ||
| log.Printf("Fill phase %s: %d sandboxes (top up to %d%% of %d worker pod slots)", raw, cfg.FillCounts[i], pct, clusterInfo.PodCapacity) |
There was a problem hiding this comment.
I think we could move this to logic to a String() or Description() method on type Phase interface
| } | ||
| n, err := strconv.Atoi(suffix) | ||
| digits := suffix | ||
| if i := strings.IndexFunc(suffix, func(r rune) bool { return r < '0' || r > '9' }); i >= 0 { |
There was a problem hiding this comment.
Rather than complex parsing, what if we did something like this:
fill-pct:80-label:foo
In other words, hyphen separated key-value pairs, with the key and value separated by a colon. We could disallow spaces/colons in the label, I don't think that's a big loss.
Should make it more self-documenting.
| claimsWarmResident := -1 | ||
| for i, raw := range cfg.Phases { | ||
| switch { | ||
| case raw == string(PhaseFill), isFillUtil(raw): |
There was a problem hiding this comment.
This could presumably also be a method on type Phase interface
… feedback) Refactors from justinsb's review on kubernetes-sigs#1287: - The parsed form of a --phases entry is now a Phase interface (Name/Validate/Resolve/Description/Requested/Run) with one struct per kind, replacing the string-switch pipeline (buildPhaseRuns, the requested() switch in buildSummary, the fill-count side tables in Config). Each phase carries its own sizing after Resolve, so Config.FillCount/FillCounts disappear; the per-phase 'requested' field in summary.json is unchanged. The old string type lives on as PhaseName, the label recorded per sandbox in records and summary.json. - Phase arguments are hyphen-separated key:value pairs instead of positional digits with magic suffixes: fill-pct:80 (was fill-util80 - 'util' read as utility, pct says percent), throughput-mif:400-label:hot. Labels are [a-z0-9_.] so the grammar stays unambiguous; every kind accepts label. Legacy throughput-mifN and bare throughput still parse, so existing scripts and open PR branches keep working. - Validation moves ahead of all cluster interaction: parse errors and bad flag combinations (--probe-count, sustained NaN guards, ...) now fail before a kubeconfig is even loaded. CI/scenario changes from the same review: - Drop the weekly hero periodic for now - larger-run policy compliance needs to be confirmed first. - The cilium presubmit comment describes only this job's purpose. - The scenario's phase comment now walks the default list concretely: fill to ~10%, probe + descending mif sweep, fill-pct:80, then mif400 again at density - 400 in flight is ~18% of a 20-node cluster, so the level peaks at ~98% utilization, which fits. - Daily-periodic comment wording (larger, not full).
kubernetes-prow
Bot
removed
the
size/L
kubernetes-prow
Bot
added
the
size/XXL
|
Pushed 4656c2c addressing the review — all points taken:
One behavior note: |
|
Both fixed in 2f229ca:
|
justinsb
changed the title
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
|
Trimmed in the latest push: both daily-periodic comment paragraphs are gone — they explained the change relative to the old delegation setup rather than describing what the job is. Each script now carries one line: daily baseline against main, 20 nodes, single control plane. |
0a699e0 to
d3f8dd0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/stress/phase.go`:
- Around line 34-61: Add Kind() PhaseName to the Phase interface in
test/stress/phase.go and implement it on every concrete phase type to return its
fixed, unlabeled base kind. In test/stress/main.go, add Kind to PhaseSummary,
populate it in buildSummary from phases[i].Kind(), and update printReport to
switch on PhaseName(ps.Kind) rather than the label-bearing ps.Name; apply these
changes at both listed sites.
🪄 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: Pro Plus
Run ID: 9f65a2d5-f99b-4ef1-a105-852643bf60a3
📒 Files selected for processing (13)
dev/ci/periodics/benchmarks-kops-gcp-ciliumdev/ci/periodics/benchmarks-kops-gcp-kindnetdev/ci/presubmits/benchmarks-kops-gcp-ciliumtest/benchmarks/scenarios/benchmarks-kops-gcp/runtest/stress/generate-report/download_results.pytest/stress/main.gotest/stress/phase.gotest/stress/phaseconfig_test.gotest/stress/phases.gotest/stress/pprof.gotest/stress/sustained.gotest/stress/sustained_test.gotest/stress/tracker.go
🚧 Files skipped from review as they are similar to previous changes (4)
- dev/ci/presubmits/benchmarks-kops-gcp-cilium
- dev/ci/periodics/benchmarks-kops-gcp-cilium
- dev/ci/periodics/benchmarks-kops-gcp-kindnet
- test/stress/phases.go
Refactor the tests so that the periodic tests run at moderate size, and the presubmit tests are a bit cheaper. Also in our tests add a phase so that we can test lots of pods / behaviour with lots of pods. fill gains the ability to specify pct:80 (for example) which will fill to 80% of capacity. Then we can run additional phases, verifying e.g. the throughput we can achieve when at 80% instead of at 10%.
98bb502 to
93d46e0
Compare
aditya-shantanu
left a comment
aditya-shantanu
left a comment
There was a problem hiding this comment.
Reviewed the whole change. The Phase interface split (parse -> validate -> resolve -> run) is a clear improvement, and the order-aware capacity walk in resolvePhases (mif600 fits before fill-pct:80 but not after) is exactly right and well covered by TestResolvePhasesCapacityOrdering. I checked the artifact-name compatibility: fileSafePhase and download_results.py use the same character class, and generate_report.py discovers profiles by glob (pprof-*.pprof) without parsing names, so the ':' flattening is safe end to end. The presubmit 3-node phase list also checks out arithmetically (~95% of 330 slots at the pct80 mif50 leg, warning-but-runs as the comment says). One non-blocking nit inline.
/lgtm
| // Kind is the phase's base kind (fill, probe, throughput, ...): Name | ||
| // carries the phase's arguments (probe-label:x), so consumers picking | ||
| // kind-specific output must use Kind, not Name. | ||
| Kind PhaseName `json:"kind,omitempty"` |
There was a problem hiding this comment.
Non-blocking: generate_report.py (line ~219) still selects the probe phase with phase['name'] == 'probe', so a labeled probe entry (probe-label:x) would silently lose its kind-specific report section. Since summary.json now carries kind, a follow-up could switch the report generator to phase.get('kind', phase['name']) — fine to leave for a separate PR.
kubernetes-prow
Bot
added
the
lgtm
Refactor the tests so that the periodic tests run at moderate size, and the presubmit tests are a bit cheaper. We seem to get a lot of signal even at a small number of nodes, we're identifying a lot of problems in upstream kube, and we've found a bunch of issues/configuration settings in our own code also. We'll continue to find those in the periodic tests (and can probably justify even bigger size in the periodic tests).
Also in our tests add a phase so that we can test lots of pods / behaviour with lots of pods.
fillgains the ability to specifypct:80(for example) which will fill to 80% of capacity. Then we can run additional phases, verifying e.g. the throughput we can achieve when at 80% instead of at 10%.This lets us test at (essentially) 100% capacity - 1000 sandboxes with 10 nodes, 2000 with 20. We can investigate maxPodsPerNode and ipv6 to see what happens when we go even denser - kOps supports both. (The one I've not tested personally is swap)
Summary by CodeRabbit
fill-pctandthroughput-mif:<N>formats, with richer phase resolution.