[TEP-0137] Rework the events controller cache #6914
Conversation
tekton-robot
added
kind/misc
|
[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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
needs-rebase
afrittoli
changed the title
The events controllers for different resources (CustomRun, TaskRun and PipelineRun) will be almost identical, with only the resource type being different. This commit refactors the CustomRun events controller to factor up as much as possible of the reconciler, controller and test logic so that we can reuse it in the upcoming commits for the other resources. I've done a slight change of strategy in the unit test structure compared to what we do for the core controller tests. A set of tests verifies as much as possible of the shared functions, by mocking the event functionality away. These tests are independent of the specific target format of the events. Most of the functionality of the ReconcileKind functions is handled in reconciler tests, which do not need a controller object or a config map watcher to run, which reduces the complexity of these tests without sacrifying coverage. Finally, a smaller set of tests covers the controller -> reconciler logic, so verify that our controller works well when invoking the ReconcileKind indirectly through the generated package. Signed-off-by: Andrea Frittoli <[email protected]>
tekton-robot
removed
the
needs-rebase
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
Switch the cache implementation from a simple LRU to the bigcache library which provides better performance and reduces the need for garbage collection. Further improve the cache by reducing the size of keys, by adopting hashing of keys and thus storing smaller data in the cache. Hashing also allows for detecting changes in a larger surface of the data. While todays keys are based on the event type and resource metadata, we can now include conditions in the equation as well, to provide and enhanced logic for event sending. Storing a resource metadata and its condition in the hashed keys means we can detect immediately if no relevant change was made in a resource, and quickly finish the reconcile cycle. Additionally storing the event type along with the resource metadata, means that we can detect if a specific message was already sent for a specific resource. To be able to rely on the cache to decide which event to send, we need to ensure that reconcile cycles for the same resource are never executed concurrently. To achieve this, the thread number (or concurrency) is set to 1 in the controller. Scalability needs can be addressed by scaling the events controller horizontally. The sharding of resources across instances of the reconciler is based on the resource key, so the same resource will always be directed to the same controller thus avoiding any cache miss. Signed-off-by: Andrea Frittoli <[email protected]>
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
@afrittoli: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
tekton-robot
added
the
needs-rebase
|
@afrittoli: PR needs rebase. 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. |
|
@afrittoli do you think we can get this in for this milestone? Or should I move it over to 0.57? |
|
This PR has not made any progress since Pipelines v0.51 when it was initiated introduced. Clearing it from the milestone. Please add back as appropriate. Thanks! |
Dependencies:
Once dependencies are merged, the PR will contain 1 commit only
Changes
Rework the events controller cache
Switch the cache implementation from a simple LRU to the bigcache
library which provides better performance and reduces the need
for garbage collection.
Further improve the cache by reducing the size of keys, by
adopting hashing of keys and thus storing smaller data in the cache.
Hashing also allows for detecting changes in a larger surface of
the data. While todays keys are based on the event type and
resource metadata, we can now include conditions in the equation
as well, to provide and enhanced logic for event sending.
Storing a resource metadata and its condition in the hashed keys
means we can detect immediately if no relevant change was made
in a resource, and quickly finish the reconcile cycle.
Additionally storing the event type along with the resource
metadata, means that we can detect if a specific message was
already sent for a specific resource.
To be able to rely on the cache to decide which event to send,
we need to ensure that reconcile cycles for the same resource
are never executed concurrently. To achieve this, the thread
number (or concurrency) is set to 1 in the controller.
Scalability needs can be addressed by scaling the events
controller horizontally. The sharding of resources across
instances of the reconciler is based on the resource key, so
the same resource will always be directed to the same controller
thus avoiding any cache miss.
Signed-off-by: Andrea Frittoli [email protected]
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes
/kind misc