Add pool scheduling integration tests#78
Conversation
Exercise the resource pool scheduler end-to-end via the example-tests binary: 8 worker tests (demand 1), 2 exclusive tests (demand 2), and 3 non-pool tests (no resource demands). Assertions verify pool capacity is never exceeded, exclusive tests run in isolation, and non-pool tests run unconstrained. Includes an ASCII timeline chart in test output. Also adds ResourcePools to ExtensionTestResult so downstream consumers can inspect the pool demand that was active during a test run. Ref: openshift-eng#73 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: not-stbenjam The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @not-stbenjam. Thanks for your PR. I'm waiting for a openshift-eng member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
WalkthroughAdds a ChangesPool-aware scheduling example and tests
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Ginkgo Test
participant Binary as example-tests binary
participant Ext as ExtensionTestSpecs
participant Runner as Spec.Run
Test->>Binary: run-suite example/pools
Binary->>Ext: AddSpecs(PoolTestSpecs())
Ext->>Runner: Run(spec) for each spec
Runner-->>Ext: ExtensionTestResult{ResourcePools}
Ext-->>Binary: JSON results + scheduler logs (stderr)
Binary-->>Test: stdout/stderr output
Test->>Test: compute peakConcurrency, overlaps, renderTimeline
Possibly related PRs
🚥 Pre-merge checks | ✅ 10 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (10 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 |
openshift-ci
Bot
added
ok-to-test
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/framework/run.go (2)
139-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPool capacity
2is duplicated and hardcoded across files.The literal
2(matchingResourcePools: map[string]int{"workers": 2}incmd/example-tests/main.go) is repeated in the assertion here and in the timeline header string. If the suite capacity changes inmain.go, these become silently stale instead of failing/updating together.♻️ Suggested fix: export a shared constant
+// in test/example/pools.go +const WorkersPoolCapacity = 2Then reference
example.WorkersPoolCapacityfrom bothcmd/example-tests/main.go'sResourcePoolsmap andtest/framework/run.go's assertions/timeline label instead of the literal2.Also applies to: 301-301
🤖 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 `@test/framework/run.go` around lines 139 - 140, The pool capacity value is hardcoded in multiple places, so it can drift from the actual configured capacity. Introduce a shared exported constant like example.WorkersPoolCapacity and use it both in cmd/example-tests/main.go’s ResourcePools map and in test/framework/run.go where peakConcurrency is asserted and the timeline label is built. Update the relevant references in run.go and any matching label text so they derive from the shared constant instead of literal 2 values.
306-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
fmt.Fprintfinstead ofWriteString(fmt.Sprintf(...)).Flagged by staticcheck (QF1012) at lines 306, 307, and 339.
🔧 Proposed fix
- b.WriteString(fmt.Sprintf("%*s0ms%*s%s\n", pad, "", chartWidth-3-len(endLabel), "", endLabel)) - b.WriteString(fmt.Sprintf("%*s|%*s|\n", pad, "", chartWidth-1, "")) + fmt.Fprintf(&b, "%*s0ms%*s%s\n", pad, "", chartWidth-3-len(endLabel), "", endLabel) + fmt.Fprintf(&b, "%*s|%*s|\n", pad, "", chartWidth-1, "")- b.WriteString(fmt.Sprintf(" %-*s %s %s\n", maxName, name, demandStr, string(line))) + fmt.Fprintf(&b, " %-*s %s %s\n", maxName, name, demandStr, string(line))Also applies to: 339-339
🤖 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 `@test/framework/run.go` around lines 306 - 307, The string building in the chart rendering logic uses WriteString(fmt.Sprintf(...)), which staticcheck flags as unnecessary formatting overhead. Update the affected writes in the chart construction code (the block that emits the end label and the vertical bar, plus the matching call around the later chart output) to write formatted output directly with fmt.Fprintf on the existing buffer, keeping the same format strings and arguments while removing the intermediate Sprintf calls.Source: Linters/SAST tools
🤖 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/framework/run.go`:
- Around line 168-191: The per-test overlap assertion in the non-pool test block
is too strict and can fail when a no-pool test starts after others have already
finished; update the `It("should run non-pool tests without being blocked by
pool constraints")` check in `test/framework/run.go` to use a weaker aggregate
concurrency validation instead of requiring every `np` in `noPool` to overlap
with some `other`. Keep the `poolDemand`, `overlaps`, and `results`-based setup,
but change the logic so it verifies overall non-pool concurrency without
depending on each individual test overlapping.
---
Nitpick comments:
In `@test/framework/run.go`:
- Around line 139-140: The pool capacity value is hardcoded in multiple places,
so it can drift from the actual configured capacity. Introduce a shared exported
constant like example.WorkersPoolCapacity and use it both in
cmd/example-tests/main.go’s ResourcePools map and in test/framework/run.go where
peakConcurrency is asserted and the timeline label is built. Update the relevant
references in run.go and any matching label text so they derive from the shared
constant instead of literal 2 values.
- Around line 306-307: The string building in the chart rendering logic uses
WriteString(fmt.Sprintf(...)), which staticcheck flags as unnecessary formatting
overhead. Update the affected writes in the chart construction code (the block
that emits the end label and the vertical bar, plus the matching call around the
later chart output) to write formatted output directly with fmt.Fprintf on the
existing buffer, keeping the same format strings and arguments while removing
the intermediate Sprintf calls.
🪄 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: Repository: openshift-eng/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cbc5a3a3-13a5-409d-9768-40b95886ca63
📒 Files selected for processing (6)
cmd/example-tests/main.gopkg/extension/extensiontests/spec.gopkg/extension/extensiontests/types.gotest/example/pools.gotest/framework/info.gotest/framework/run.go
| It("should run non-pool tests without being blocked by pool constraints", func() { | ||
| var noPool e.ExtensionTestResults | ||
| for _, r := range results { | ||
| if poolDemand(r) == 0 { | ||
| noPool = append(noPool, r) | ||
| } | ||
| } | ||
| Expect(noPool).To(HaveLen(3), "Expected 3 non-pool tests") | ||
|
|
||
| for _, np := range noPool { | ||
| overlappedSomething := false | ||
| for _, other := range results { | ||
| if other.Name == np.Name { | ||
| continue | ||
| } | ||
| if overlaps(np, other) { | ||
| overlappedSomething = true | ||
| break | ||
| } | ||
| } | ||
| Expect(overlappedSomething).To(BeTrue(), | ||
| "Non-pool test %q should have overlapped with at least one other test (not serialized)", np.Name) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file around the cited lines
sed -n '1,260p' test/framework/run.go
# Find helper definitions used by the assertion
rg -n "poolDemand|overlaps|results|ExtensionTestResults|should run non-pool tests" test/framework/run.go test -SRepository: openshift-eng/openshift-tests-extension
Length of output: 13222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the rest of the test file for scheduling assumptions and timeline rendering
sed -n '252,420p' test/framework/run.go
# Locate the example suite definitions that produce the 13 results
rg -n "example/pools|ResourcePools|no-pool|exclusive|worker" -S .
# Find the ExtensionTestResult type to understand timestamp precision and fields
fd -a "extensiontests" pkg test
rg -n "type ExtensionTestResult|type ExtensionTestResults|StartTime|EndTime|ResourcePools" pkg test -SRepository: openshift-eng/openshift-tests-extension
Length of output: 35314
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the example pool suite definition
sed -n '1,220p' test/example/pools.go
# Inspect scheduler behavior around resource-pool and zero-demand tests
sed -n '1,420p' pkg/extension/extensiontests/scheduler.go
# Inspect the result type and timestamp precision
sed -n '1,220p' pkg/extension/extensiontests/types.goRepository: openshift-eng/openshift-tests-extension
Length of output: 19215
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the run-suite command path for worker count / execution model
sed -n '1,260p' pkg/cmd/cmdrun/runsuite.go
# Find where tests are actually executed and whether worker concurrency is fixed
rg -n "GetNextTestToRun|MarkTestComplete|worker|parallel|GOMAXPROCS|runtime.NumCPU|runOpts" pkg/cmd pkg/extension -S
# Look for any tests that assert overlap or non-overlap semantics in the scheduler package
rg -n "overlap|peakConcurrency|non-pool|exclusive" pkg/extension/extensiontests/scheduler_test.go test/framework/run.go -SRepository: openshift-eng/openshift-tests-extension
Length of output: 18799
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the spec runner worker loop
sed -n '180,320p' pkg/extension/extensiontests/spec.go
# Inspect dbtime wrapper/serialization to see timestamp precision
sed -n '1,220p' pkg/dbtime/dbtime.go
# Inspect scheduler tests around zero-demand / overlap behavior
sed -n '1000,1065p' pkg/extension/extensiontests/scheduler_test.goRepository: openshift-eng/openshift-tests-extension
Length of output: 4778
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect example test suite config for concurrency / parallelism
sed -n '1,140p' cmd/example-tests/main.go
# Inspect the scheduler test that exercises zero-demand specs
sed -n '1018,1065p' pkg/extension/extensiontests/scheduler_test.goRepository: openshift-eng/openshift-tests-extension
Length of output: 7094
Relax the no-pool overlap assertion test/framework/run.go:168-191
Requiring every no-pool test to overlap with another test is too strong: one can legitimately start after the rest have finished, which makes this fail intermittently even when scheduling is correct. Use a weaker aggregate concurrency check 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 `@test/framework/run.go` around lines 168 - 191, The per-test overlap assertion
in the non-pool test block is too strict and can fail when a no-pool test starts
after others have already finished; update the `It("should run non-pool tests
without being blocked by pool constraints")` check in `test/framework/run.go` to
use a weaker aggregate concurrency validation instead of requiring every `np` in
`noPool` to overlap with some `other`. Keep the `poolDemand`, `overlaps`, and
`results`-based setup, but change the logic so it verifies overall non-pool
concurrency without depending on each individual test overlapping.
2 participants
Summary
example/poolssuite with 13 tests exercising the resource pool scheduler end-to-end: 8 worker tests (demand 1 unit), 2 exclusive tests (demand 2 units = full capacity), and 3 non-pool tests (no resource demands)ResourcePoolsfield toExtensionTestResultso test results carry pool demand metadata in JSON outputBuilds on the pool-aware scheduler from #73.
Test plan
make buildcompiles cleanly./example-tests run-suite example/pools— all 13 tests pass,resourcePoolspresent in JSONmake integration— all 25 tests pass (including 5 new pool scheduling tests)(1),(2),(-)demand annotations🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes