Repository navigation
Conversation
tekton-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
tekton-robot
added
the
size/XXL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
d653a7c to
6a5db79
Compare
olegbet
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
/kind feature |
tekton-robot
added
the
kind/feature
olegbet
changed the title
tekton-robot
added
release-note-none
olegbet
changed the title
6a5db79 to
ffb328e
Compare
|
/retest |
ffb328e to
9ea8734
Compare
There was a problem hiding this comment.
🟡 Changes recommended
External database setup, dataset provenance, percentile calculation, and JSON output correctness have unresolved defects.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds a deterministic performance-benchmarking framework for measuring Tekton Results API storage and query throughput.
Changes:
- Adds dataset generators, realistic PipelineRun/TaskRun templates, metrics, and versioned reports.
- Adds load, store, query, and mixed benchmark workloads.
- Adds kind environment automation, tests, and documentation.
File summaries
| File | Description |
|---|---|
.gitignore |
Ignores benchmark outputs. |
docs/README.md |
Links performance documentation. |
docs/performance.md |
Documents benchmark usage. |
test/e2e/01-install.sh |
Extends token lifetime. |
test/performance/README.md |
Documents the framework. |
test/performance/baselines/.gitkeep |
Reserves baseline directory. |
test/performance/datasets/seed-small.md |
Defines the seed dataset. |
test/performance/environments/kind/00-kind-up.sh |
Creates the kind cluster. |
test/performance/environments/kind/01-install-localdb.sh |
Installs local Postgres. |
test/performance/environments/kind/02-install-externaldb.sh |
Installs with external Postgres. |
test/performance/environments/kind/03-smoke.sh |
Runs benchmark smoke checks. |
test/performance/environments/kind/postgres-patch.yaml |
Pins Postgres resources/config. |
test/performance/environments/kind/postgresql.conf |
Defines benchmark DB tuning. |
test/performance/generator/definition.go |
Builds golden dataset definitions. |
test/performance/generator/distribution.go |
Implements deterministic distributions. |
test/performance/generator/generator.go |
Materializes benchmark instances. |
test/performance/generator/generator_test.go |
Tests generator behavior. |
test/performance/generator/pipelinerun.go |
Generates PipelineRuns and TaskRuns. |
test/performance/generator/template.go |
Loads weighted templates. |
test/performance/generator/template_mix_test.go |
Tests template selection. |
test/performance/generator/templates/README.md |
Documents template contracts. |
test/performance/generator/templates/default/simple/pipelinerun.yaml |
Provides default PipelineRun data. |
test/performance/generator/templates/default/simple/taskruns/build-container.yaml |
Provides a build TaskRun. |
test/performance/generator/templates/default/simple/taskruns/clone-repository.yaml |
Provides a clone TaskRun. |
test/performance/generator/templates/default/simple/template.yaml |
Configures the default template. |
test/performance/generator/templates/fbc-builder/pipelinerun.yaml |
Provides a large reference PipelineRun. |
test/performance/generator/templates/fbc-builder/taskruns/apply-tags.yaml |
Adds apply-tags fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/build-image-index.yaml |
Adds image-index fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/build.yaml |
Adds large build fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/clone-repository.yaml |
Adds realistic clone fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/deprecated-base-image-check.yaml |
Adds image-check fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/fbc-fips-check-oci-ta.yaml |
Adds FIPS-check fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/fbc-inject-lifecycle.yaml |
Adds lifecycle fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/fbc-target-index-pruning-check.yaml |
Adds index-pruning fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/init.yaml |
Adds initialization fixture data. |
test/performance/generator/templates/fbc-builder/taskruns/validate-fbc.yaml |
Adds validation fixture data. |
test/performance/generator/templates/fbc-builder/template.yaml |
Configures the FBC template. |
test/performance/generator/templates/sample/pipelinerun.yaml |
Provides sample PipelineRun data. |
test/performance/generator/templates/sample/taskrun.yaml |
Provides sample TaskRun data. |
test/performance/generator/templates/sample/template.yaml |
Configures the sample template. |
test/performance/harness/apiclient.go |
Constructs API clients. |
test/performance/harness/config.go |
Defines benchmark flags. |
test/performance/harness/dataset.go |
Configures deterministic datasets. |
test/performance/harness/harness_test.go |
Tests harness helpers. |
test/performance/harness/loader.go |
Loads and verifies seed data. |
test/performance/harness/main.go |
Implements benchmark commands. |
test/performance/harness/mixed.go |
Runs mixed workloads. |
test/performance/harness/output_test.go |
Tests output handling. |
test/performance/harness/query.go |
Runs paginated query workloads. |
test/performance/harness/store.go |
Replays write lifecycles. |
test/performance/harness/worker.go |
Coordinates benchmark workers. |
test/performance/metrics/metrics.go |
Collects latency/error metrics. |
test/performance/metrics/metrics_test.go |
Tests metric calculations. |
test/performance/report/meta.go |
Captures report provenance. |
test/performance/report/report.go |
Defines the report schema. |
test/performance/report/report_test.go |
Tests report generation. |
Review details
Suppressed comments (1)
test/performance/harness/config.go:69
- The committed seed specification identifies the current dataset as
seed-small-v4, but the CLI defaults toseed-small-v1. Every default load and benchmark will therefore label v4-generated content as v1, defeating the version-based comparability contract.
- Files reviewed: 55/56 changed files
- Comments generated: 10
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
a177dc3 to
4bebd02
Compare
4bebd02 to
e1bf421
Compare
| return nil, nil, fmt.Errorf("computing dataset definition: %w", err) | ||
| } | ||
| m := runIndexed(ctx, cfg.Concurrency, 0, cfg.Count, func(ctx context.Context, index int, m *metrics.MetricSet) { | ||
| inst, err := g.At(index) |
There was a problem hiding this comment.
loadSeed never checks if any writes failed, it always returns nil error even if half the records failed to persist. May be we can add a check.
There was a problem hiding this comment.
loadSeed records errors to the metrics object with m.Observe method and is actually not meant to fail on any of the errors so it would probably make sense to let it pass on them.
There was a problem hiding this comment.
The metrics capture is good for reporting, but the caller (loadCmd) has no way to know the seed dataset is incomplete. If 50% of records failed to persist, the subsequent query or mixed workload will silently run against a partial dataset, producing misleading benchmark numbers. A middle ground: don't fail hard, but return/log a summary like "X of Y records failed to store" so the user knows the seed is degraded. The metrics file captures per-op errors, but someone running ./harness load from CLI won't see that until they parse the JSON report.
| // compares them to the golden definition. It returns an error describing any | ||
| // mismatch so the loader can fail loudly rather than benchmark against a partial | ||
| // dataset. | ||
| func verifySeed(ctx context.Context, l lister, def *generator.DatasetDefinition) error { |
There was a problem hiding this comment.
Verification uses wildcard parent "-/results/-" and only checks total PipelineRun/TaskRun counts. All records landing in one namespace would pass. I think we should iterate def.PerNamespace and verify counts per namespace.
| Version: g.cfg.Version, | ||
| Seed: g.cfg.Seed, | ||
| Count: g.cfg.Count, | ||
| GeneratedAt: time.Now().UTC().Format(time.RFC3339), |
There was a problem hiding this comment.
DatasetDefinition.GeneratedAt changes every call, so bench dataset output differs between runs. Either we can drop the field from the struct, or set it to a fixed sentinel.
| return nil, nil, err | ||
| } | ||
| def, err := g.Definition() | ||
| if err != nil { |
There was a problem hiding this comment.
bench load discards storeInstance's returned sendLogEntry, so there's no record of what was sent, the compliance suite needs this for ingest verification. Collect entries the same way newStoreCmd does.
| // splitWorkers divides a worker budget between readers and writers by ratio, | ||
| // guaranteeing at least one of each when the budget allows. | ||
| func splitWorkers(total, readRatio, writeRatio int) (writers, readers int) { | ||
| if total < 2 { |
There was a problem hiding this comment.
Function returns (1, 1) = 2 workers when asked for 1, violating the budget. IMO we can return (1, 0) or (0, 1) for total < 2 instead of (1, 1)
| Short: "Benchmark parallel store (live range) and query (seed range)", | ||
| RunE: func(cmd *cobra.Command, _ []string) error { | ||
| if cfg.Concurrency <= 1 { | ||
| return fmt.Errorf("concurrency for mixed mode must be grater than 1") |
There was a problem hiding this comment.
Typo "grater" in the error message.
| // empty, TLS verification is skipped (convenient for a local kind cluster with a | ||
| // self-signed cert). | ||
|
|
||
| func NewAPIClients(cfg ClientConfig) (*APIClients, error) { |
There was a problem hiding this comment.
NewAPIClients creates gRPC and REST connections but never exposes a way to close them.
| ``` | ||
|
|
||
| Run the store, query, and mixed workloads. Each command writes a JSON report | ||
| under `test/performance/output/`, which the harness creates on demand. Omit |
There was a problem hiding this comment.
We are not really planning to commit in git those results of the performance testing, at least not in the results repo. I would suggest that by default the reports are written somewhere in /tmp, e.g. /tmp/<date>-<commit>. We can later decide for a platform to upload it (it may be some other Git repo or whatever), but that's not part of the current issue.
I see you added the output files to gitignore, but the output directory should not be here either IMO.
| --- | ||
| --> | ||
|
|
||
| # Performance Benchmarking |
There was a problem hiding this comment.
Putting the comment here, but it's regarding the performance.md file. IMO the docs folder is more suited for user facing documentation. I'll suggest this file to move to the performance folder. There is a README file there already, so you can combine the content if needed. My point is that if user looks into the docs folder and sees file named performance.md, he/she might think it's about performance related optimizations and guides. That BTW is something else we are missing in our documentation because the default settings are not optimized for performance, but that's just a side note.
| export API_SERVER_ADDR=https://localhost:8080 | ||
| CERT=/tmp/tekton-results/ssl/tekton-results-cert.pem | ||
| TOKEN=/tmp/tekton-results/tokens/all-namespaces-admin-access |
There was a problem hiding this comment.
Given it's our script creating the environment, we should know the values already. We can still keep the ENV vars for the users to override the default values, but maybe that should not be mandatory. Also the syntax is a bit off, we either need an export on each line or escape the new lines with \.
| --output test/performance/output/mixed.json --duration 60 | ||
| ``` | ||
|
|
||
| The `load --verify` command recomputes the dataset content hash and reports |
There was a problem hiding this comment.
so it only verifies without loading the datasets? Or loads and verifies? It sounds like it's the latter, but can you please confirm?
There was a problem hiding this comment.
Yes, it load the seed dataset through the API and verify counts, I'll update the documentation comment.
| export BENCH_DB_URL="postgres://<user>:<pass>@<host>:5432/results?sslmode=require" | ||
| ./test/performance/environments/kind/02-install-externaldb.sh | ||
| # The installer cannot export into this shell; set the report label here: | ||
| export BENCH_DB_BACKEND=external |
There was a problem hiding this comment.
nit: Both DB and BACKEND to me mean the same, so maybe BENCH_DB_TYPE? If we are only planning to accept one value here external, it could also be BENCH_DB_EXTERNAL=true.
|
|
||
| - [Roadmap](roadmap.md) | ||
| - [Results API Query Cookbook](api/README.md#cookbook) | ||
| - [Performance Benchmarking](performance.md) |
There was a problem hiding this comment.
that should be also removed if you remove the performance.md file from the docs folder.
| @@ -0,0 +1,3 @@ | |||
| # Committed baseline reports land here (one JSON per mode/tier/dataset version). | |||
There was a problem hiding this comment.
I'm not sure I understand the purpose here, but as I mentioned earlier, IMO we should not commit any reports nor baselines. I agree it'll be nice to keep track or baselines and specific measurements for comparison, but it should not be in the tekton results repo.
There was a problem hiding this comment.
It was meant to persist the latest reports to compare the later runs with it, but it does make sense to have some separate temporary repo for that.
| ``` | ||
|
|
||
| `generator`, `metrics`, and `report` are importable packages — the compare tool | ||
| (Story 02) and compliance suite (Story 03) reuse the generator, datasets, and |
There was a problem hiding this comment.
We should avoid mentioning stories, they do not have any meaning in code. You can say currently we have X and adding Y and Z is planned, where X, Y and Z are the actual features and not references.
There was a problem hiding this comment.
go run ./test/performance/harness load --verify \
--cert "${SSL_CERT_PATH}/tekton-results-cert.pem" \
--token "${SA_TOKEN_PATH}/all-namespaces-admin-access"
seems to be very slow. I waited for ~20 mins and it was far from the finished. So I looked into it and found (expectedly) that with the default qps (5) and burst (10), the k8s api queries were slowing the process down. So I bumped these to 50 and 100 respectively through the tekton-results-api-config configMap and rolled out a new Results API instance. This improved the speed, but eventually the seed verification failed.
go run ./test/performance/harness load --verify --cert "${SSL_CERT_PATH}/tekton-results-cert.pem" --token "${SA_TOKEN_PATH}/all-namespaces-admin-access"
Loading template "simple" from templates/default/simple...
Creating generator...
error: seed verification failed: row-count mismatch: PipelineRuns got 998 want 1000, TaskRuns got 3021 want 3028
exit status 1
I'm assuming this could be result of the roll out of the api pod.
I tried on a new deployment and bumped the api qps and burst before running the seed and this time it was significantly quicker.
time go run ./test/performance/harness load --verify --cert "${SSL_CERT_PATH}/tekton-results-cert.pem" --token "${SA_TOKEN_PATH}/all-namespaces-admin-access"
Loading template "simple" from templates/default/simple...
Creating generator...
seed verified: 1000 PipelineRuns, dataset hash e4e707dbbae0f81b929277f3eee2b8037a692b4f1f0414fce526d30ab852b8c8
{
"schema_version": "1",
"meta": {
"git_commit": "e1bf421d6059798888920de8524ae0a2e8896b69",
"git_dirty": false,
"dataset_version": "seed-small-v1",
"dataset_hash": "e4e707dbbae0f81b929277f3eee2b8037a692b4f1f0414fce526d30ab852b8c8",
"tier": "tier1",
"mode": "load",
"started_at": "2026-10-18T14:16:10Z",
"duration_ms": 279169,
"hostname": "xxx",
"api_server_addr": "https://localhost:8080",
"db_backend": "local",
"server_image": ""
},
"config": {
"count": 1000,
"concurrency": 8,
"duration_sec": 0,
"transport": "grpc",
"read_ratio": 0,
"write_ratio": 0,
"dataset": "seed-small",
"seed": 42
},
"metrics": {
"throughput_per_sec": 25.174705297724294,
"total_ops": 7028,
"total_errors": 0,
"by_op": {
"create_child_record": {
"count": 3028,
"errors": 0,
"error_codes": {
"OK": 3028
},
"p50_ms": 319.983599,
"p90_ms": 359.245217,
"p99_ms": 422.225187,
"min_ms": 4.241181,
"max_ms": 491.527029,
"mean_ms": 317.450755
},
"create_record": {
"count": 1000,
"errors": 0,
"error_codes": {
"OK": 1000
},
"p50_ms": 320.21912,
"p90_ms": 351.638204,
"p99_ms": 413.343513,
"min_ms": 4.482895,
"max_ms": 465.844868,
"mean_ms": 316.387736
},
"create_result": {
"count": 1000,
"errors": 0,
"error_codes": {
"OK": 1000
},
"p50_ms": 319.58464,
"p90_ms": 353.740945,
"p99_ms": 424.228629,
"min_ms": 4.422872,
"max_ms": 472.725226,
"mean_ms": 314.84384
},
"update_record": {
"count": 1000,
"errors": 0,
"error_codes": {
"OK": 1000
},
"p50_ms": 320.018055,
"p90_ms": 355.730706,
"p99_ms": 425.553204,
"min_ms": 4.450035,
"max_ms": 470.503902,
"mean_ms": 317.046036
},
"update_result": {
"count": 1000,
"errors": 0,
"error_codes": {
"OK": 1000
},
"p50_ms": 319.781984,
"p90_ms": 354.168935,
"p99_ms": 414.655482,
"min_ms": 4.548607,
"max_ms": 459.429215,
"mean_ms": 315.363235
}
}
}
}
real 4m39.653s
user 0m3.457s
sys 0m1.541s
I'm considering making qps=50 and burst=100 the default values, but I need to do more testing. So in the meantime, consider setting these values through the template/kustomize for the testing.
As a separate note, see that I replaced the value of hostname with xxx. We should be very careful with that as it leaks information about hostname and username. Unless you have a good reason to include it and a proper way to sanitize it, I'll suggest you drop it.
| Mode string `json:"mode"` | ||
| StartedAt string `json:"started_at"` | ||
| DurationMS int64 `json:"duration_ms"` | ||
| Hostname string `json:"hostname"` |
There was a problem hiding this comment.
This should be either dropped or replaced with a hash, so we can still compare the reports based on where the test was executed without leaking hostname and username.
I did a couple of more tests and one good option is disabling the authentication during the test ( |
e1bf421 to
56b01c7
Compare
tekton-robot
added
the
needs-rebase
|
/retest |
tekton-robot
removed
the
needs-rebase
c9cd6b7 to
d7eae5c
Compare
|
/ok-to-test |
Assisted-by: Claude, Codex Performance benchmark framework and seed datasets. - Performance benchmark framework added to monitor throughput performance improvements/regressions of Tekton results API server and database. - Generic PipelineRun and TaskRun templates added to benchmark against Signed-off-by: obetsun <obetsun@redhat.com>
cdc5fc0 to
847bba4
Compare
Assisted-by: Claude
Changes
Performance benchmark framework and seed datasets.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you review them:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes