perf: create (pipeline|task)run timeout checks in background #3302
Conversation
tekton-robot
added
the
release-note
|
|
tekton-robot
added
the
size/S
tekton-robot
added
the
needs-ok-to-test
|
Hi @eddie4941. Thanks for your PR. I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions 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/test-infra repository. |
| @@ -111,7 +111,7 @@ func TestTaskRunCheckTimeouts(t *testing.T) { | |||
| } | |||
|
|
|||
| th.SetCallbackFunc(f) | |||
| th.CheckTimeouts(context.Background(), testNs, c.Kube, c.Pipeline) | |||
| go th.CheckTimeouts(context.Background(), testNs, c.Kube, c.Pipeline) | |||
There was a problem hiding this comment.
The test already use a polling approach below via wait.PollImmediate so Its okay not to block here. However, if leaving this here non blocking is a concern, im happy to block until the call is done to preserve old behavior.
|
/ok-to-test |
tekton-robot
added
kind/feature
Both the pipelinerun and taskrun controllers start timeout checks at startup. They do this by iterating though each namespace and spawning a go routine for each pipelinerun/taskrun to run the check in the background. Although each timeout check is done in a go routine, the iteration through namespaces and creation of the timeout checks is done in a blocking manner. This adds significant latency at startup when the number of namespaces is large. Ultimately, this causes a delay in how fast each controllers can actually start reconciling resources. To speed up the startup time, this changes the logic so that the iteration through namespaces is done in the background. The timeout checks were already carried out in separate go routines and were therefore safe to use in a concurrent context, so no extra logic was needed to make this change work in a concurrent context.
eddie4941
force-pushed
the
controller-startup-speed
branch
from
b66932a
to
9df57bf
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbwsg 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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
Both the pipelinerun and taskrun controllers start timeout checks at
startup. They do this by iterating though each namespace and spawning a
go routine for each pipelinerun/taskrun to run the check in the
background. Although each timeout check is done in a go routine, the
iteration through namespaces and creation of the timeout checks is done
in a blocking manner. This adds significant latency at startup when the
number of namespaces is large. Ultimately, this causes a delay in how
fast each controllers can actually start reconciling resources. To
speed up the startup time, this changes the logic so that the iteration
through namespaces is done in the background. The timeout checks were
already carried out in separate go routines and were therefore safe to
use in a concurrent context, so no extra logic was needed to make this
change work in a concurrent context.
Changes
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe release Task to build and release this image.
Reviewer Notes
If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.
Release Notes