search

tektoncd / results

Long term storage of execution results.
Apache License 2.0
78 stars 74 forks source link

alllow for database connection, grpc worker thread pool, and k8s client level tuning of the watcher and api servers #744

Closed gabemontero closed 7 months ago

gabemontero commented 7 months ago

Changes

pulls in https://github.com/tektoncd/results/pull/741 and now does

/kind feature

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you review them:

Release Notes

The number of idle and max database connections, grpc worker pool goroutines, k8s client qps and burst,  in the api server can be now configured using environment variables.
The k8s client qps and burst settings can be set via command line arguments on the watcher
No defaults from prior settings have been changed.
gabemontero commented 7 months ago

@khrm @sayan-biswas @avinal @enarha PTAL

tekton-robot commented 7 months ago

The following is the coverage report on the affected files. Say /test pull-tekton-results-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/api/main.go 11.4% 9.6% -1.8
pkg/api/server/config/config.go Do not exist 0.0%
gabemontero commented 7 months ago

bump @khrm @sayan-biswas can I get a look at this please ??

also, in relation to the discussions in https://github.com/tektoncd/results/pull/741 about the db pool size, if it will expedite things, I'm fine at this point setting the defaults to 2 or whatever default size will expedite this getting merged

thanks

khrm commented 7 months ago

@gabemontero Sure. I will take a look tomorrow.

BTW, I wanted to test the effect of db maxPool to 10.

tekton-robot commented 7 months ago

The following is the coverage report on the affected files. Say /test pull-tekton-results-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/api/main.go 11.4% 9.6% -1.8
pkg/api/server/config/config.go Do not exist 0.0%
khrm commented 7 months ago

To verify this PR behavior vs default behavior, I install local DB in the cluster as we do in ./test/e2e/01-install.sh.

Then I ran tests once parallelly with 10: Main branch results: https://gist.github.com/khrm/62a6e24eb3b7926fd8678c7d257775e3 This PR results: https://gist.github.com/khrm/58ba044b5dc9436a91ab5554a44ed020

I merged the main branch to this PR to avoid any discrepancy from earlier PRs. We can see that the former is more performant than the latter.

gabemontero commented 7 months ago

To verify this PR behavior vs default behavior, I install local DB in the cluster as we do in ./test/e2e/01-install.sh.

Then I ran tests once parallelly with 10: Main branch results: https://gist.github.com/khrm/62a6e24eb3b7926fd8678c7d257775e3 This PR results: https://gist.github.com/khrm/58ba044b5dc9436a91ab5554a44ed020

I merged the main branch to this PR to avoid any discrepancy from earlier PRs. We can see that the former is more performant than the latter.

OK I will do the change I note in https://github.com/tektoncd/results/pull/744#discussion_r1555759184 so that the main branch behavior is not changed at all unless those env are set.

Not sure what precisely is different between your tests, my tests, and @stuartwdouglas tests but I don't think it matters at this point.

As long as we can change these settings and run tests in larger test clusters to see improvements, that is sufficient.

tekton-robot commented 7 months ago

The following is the coverage report on the affected files. Say /test pull-tekton-results-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/api/main.go 11.4% 9.3% -2.1
pkg/api/server/config/config.go Do not exist 0.0%
sayan-biswas commented 7 months ago

/approve

The README needs a minor change and commits squashed.

gabemontero commented 7 months ago

README updated, commits squashed, PR title / description / release notes refreshed @sayan-biswas @khrm

I suggest viewing diffs with ignoring whitespace, i.e. https://github.com/tektoncd/results/pull/744/files?w=1 to filter out the widening of the READM md columns as I added more explanation about golang vs. DB driver defaults

tekton-robot commented 7 months ago

The following is the coverage report on the affected files. Say /test pull-tekton-results-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
cmd/api/main.go 11.4% 9.3% -2.1
pkg/api/server/config/config.go Do not exist 0.0%
tekton-robot commented 7 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: khrm, sayan-biswas

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/tektoncd/results/blob/main/OWNERS)~~ [khrm,sayan-biswas] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
khrm commented 7 months ago

@sayan-biswas @avinal Can you take a cursory look? I think it looks good. We can give lgtm here.

gabemontero commented 7 months ago

@sayan-biswas @avinal Can you take a cursory look? I think it looks good. We can give lgtm here.

@sayan-biswas did previously apply /approve with https://github.com/tektoncd/results/pull/744#issuecomment-2048356840

But of course if either @sayan-biswas or @avinal could take another look soon that would be great.

sayan-biswas commented 7 months ago

/lgtm