perf(controller): cache repository lookups - #2939
Conversation
| - apiGroups: ["pipelinesascode.tekton.dev"] | ||
| resources: ["repositories"] | ||
| verbs: ["get", "create", "list"] | ||
| verbs: ["get", "create", "list", "watch"] |
There was a problem hiding this comment.
Is this needed or handled by informer?
There was a problem hiding this comment.
yeah, watch would be needed I think when using informer
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2939 +/- ##
==========================================
+ Coverage 80.61% 80.66% +0.05%
==========================================
Files 164 164
Lines 13910 13946 +36
==========================================
+ Hits 11213 11250 +37
+ Misses 1974 1971 -3
- Partials 723 725 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Paco Review 🔍This PR adds a shared informer/lister cache for Repository custom resources to avoid live Kubernetes API calls on the webhook hot path. It threads a new RepositoryLister through params.Run with GetRepository/ListRepositories helper methods that fall back to direct API calls when no lister is configured, updates matcher.GetRepoByName/MatchEventURLRepo and github.MatchEventURLRepo to use these helpers, wires up the informer factory and cache-sync wait in main.go, and adds the 'watch' RBAC verb needed for the new informer. Review difficulty: 4/5 (Hard) — The change introduces a new caching layer that alters read semantics on the webhook authorization/matching path across several files, which is broad in blast radius and requires careful reasoning about consistency and fallback correctness. 1 new inline comment(s) found. Reviewed commit: 4205d34 |
pipelines-as-code
Bot
added
paco/review-hard
![pipelines-as-code[bot]](https://avatars.githubusercontent.com/in/112592?s=60&v=4){kind=small}
| func (r *Run) GetRepository(ctx context.Context, ns, name string) (*apipac.Repository, error) { | ||
| if r.RepositoryLister != nil { | ||
| repo, err := r.RepositoryLister.Repositories(ns).Get(name) | ||
| if err != nil { |
There was a problem hiding this comment.
[MEDIUM] Switching repository lookups on the webhook path (MatchEventURLRepo, GetRepoByName, SetupAuthenticatedClient's global-repo fetch) from always-fresh API Get/List calls to a shared informer lister introduces eventual consistency: a Repository that was just created, updated, or deleted may not yet be reflected in the local cache when a webhook event arrives, because the cache is only updated once the corresponding watch event has been delivered and processed. Previously these lookups always hit the live API, so this is a behavior change for a security/config-relevant path (which Repository governs an incoming webhook event). Consider documenting this trade-off explicitly and confirming callers on this path can tolerate a short window of staleness (e.g., immediately after creating/deleting a Repository).
make testRunning unit tests... github.com/openshift-pipelines/pipelines-as-code/pkg/acl: ✓ Expand aliases (0.00s) ✓ Expand aliases expand alias (0.00s) ✓ Expand aliases expand alias dedups (0.00s) ✓ Expand aliases no owner have aliases (0.00s) ✓ Expand aliases no owner or aliases (0.00s) ✓ Expand aliases owners dedups (0.00s) ✓ Match regexp (0.00s) ✓ Match regexp match (0.00s) ✓ Match regexp nomatch (0.00s) ✓ Regexp (0.00s) ✓ Regexp bad match regexp (0.00s) ✓ Regexp bad match regexp newline space (0.00s) ✓ Regexp bad match regexp with invalid sha (0.00s) ✓ Regexp good in the middle (0.00s) ✓ Regexp good match regexp (0.00s) ✓ Regexp good match regexp newline (0.00s) ✓ Regexp good match regexp trailing spaces (0.00s) ✓ Regexp good match regexp with full sha (0.00s) ✓ Regexp good match regexp with short sha (0.00s) ✓ Regexp good match regexp with uppercase sha (0.00s) ✓ User in owner file (0.01s) ✓ User in owner file bad owners aliases yaml file (0.00s) ✓ User in owner file bad owners yaml file (0.00s) ✓ User in owner file no owners file (0.00s) ✓ User in owner file user alias in .* filters (0.00s) ✓ User in owner file user in .* filters (0.00s) ✓ User in owner file user in approvers (0.00s) ✓ User in owner file user in other filters (0.00s) ✓ User in owner file user in owners aliases file (0.00s) ✓ User in owner file user in reviewers (0.00s) ✓ User in owner file user not in .* filters (0.00s) ✓ User in owner file user not in owner file (0.00s) ✓ User in owner file user not in owners aliases file (0.00s)github.com/openshift-pipelines/pipelines-as-code/pkg/action: github.com/openshift-pipelines/pipelines-as-code/pkg/apis/incoming: github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode: github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys: github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1: github.com/openshift-pipelines/pipelines-as-code/pkg/cel: github.com/openshift-pipelines/pipelines-as-code/pkg/changedfiles: github.com/openshift-pipelines/pipelines-as-code/pkg/cli/browser: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac: github.com/openshift-pipelines/pipelines-as-code/pkg/cli/prompt: github.com/openshift-pipelines/pipelines-as-code/pkg/cli/info: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/completion: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/deleterepo: github.com/openshift-pipelines/pipelines-as-code/pkg/cli: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/create: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/logs: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/versioncmd: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/cel: github.com/openshift-pipelines/pipelines-as-code/pkg/consoleui: github.com/openshift-pipelines/pipelines-as-code/pkg/errors: github.com/openshift-pipelines/pipelines-as-code/pkg/configutil: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/bootstrap: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/clientset/versioned: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/generate: github.com/openshift-pipelines/pipelines-as-code/pkg/events: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/clientset/versioned/scheme: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/clientset/versioned/typed/pipelinesascode/v1alpha1: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/clientset/versioned/typed/pipelinesascode/v1alpha1/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/informers/externalversions: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/clientset/versioned/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/informers/externalversions/internalinterfaces: github.com/openshift-pipelines/pipelines-as-code/pkg/cli/webhook: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/informers/externalversions/pipelinesascode: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/informers/externalversions/pipelinesascode/v1alpha1: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/client: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/client/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/factory/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/factory/filtered: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/factory/filtered/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/pipelinesascode/v1alpha1/repository: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/pipelinesascode/v1alpha1/repository/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/pipelinesascode/v1alpha1/repository/filtered: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/pipelinesascode/v1alpha1/repository/filtered/fake: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/injection/informers/factory: github.com/openshift-pipelines/pipelines-as-code/pkg/git: github.com/openshift-pipelines/pipelines-as-code/pkg/generated/listers/pipelinesascode/v1alpha1: github.com/openshift-pipelines/pipelines-as-code/pkg/formatting: github.com/openshift-pipelines/pipelines-as-code/pkg/informer/transform: github.com/openshift-pipelines/pipelines-as-code/pkg/hostpolicy: github.com/openshift-pipelines/pipelines-as-code/pkg/llm/providers/gemini: github.com/openshift-pipelines/pipelines-as-code/pkg/llm/providers/openai: github.com/openshift-pipelines/pipelines-as-code/pkg/opscomments: github.com/openshift-pipelines/pipelines-as-code/pkg/params/clients: github.com/openshift-pipelines/pipelines-as-code/pkg/params/info: github.com/openshift-pipelines/pipelines-as-code/pkg/params/settings: github.com/openshift-pipelines/pipelines-as-code/pkg/params/triggertype: github.com/openshift-pipelines/pipelines-as-code/pkg/params/versiondata: github.com/openshift-pipelines/pipelines-as-code/pkg/cli/status: github.com/openshift-pipelines/pipelines-as-code/pkg/pipelinerunmetrics: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/listcmd: github.com/openshift-pipelines/pipelines-as-code/pkg/provider: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/info: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketcloud/test: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/describe: github.com/openshift-pipelines/pipelines-as-code/pkg/gitclient: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketcloud/types: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/test: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter/types: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketdatacenter: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitea/forgejostructs: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitea/test: github.com/openshift-pipelines/pipelines-as-code/pkg/hub: github.com/openshift-pipelines/pipelines-as-code/pkg/llm/context: github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction/status: github.com/openshift-pipelines/pipelines-as-code/pkg/kubeinteraction: github.com/openshift-pipelines/pipelines-as-code/pkg/customparams: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/resolve: github.com/openshift-pipelines/pipelines-as-code/pkg/adapter: github.com/openshift-pipelines/pipelines-as-code/pkg/matcher: github.com/openshift-pipelines/pipelines-as-code/pkg/llm: github.com/openshift-pipelines/pipelines-as-code/pkg/cmd/tknpac/webhook: github.com/openshift-pipelines/pipelines-as-code/pkg/policy: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/bitbucketcloud: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitea: github.com/openshift-pipelines/pipelines-as-code/pkg/params: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitlab/test: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/providermetrics: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/retryhttp: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/status: github.com/openshift-pipelines/pipelines-as-code/pkg/random: github.com/openshift-pipelines/pipelines-as-code/pkg/queue: github.com/openshift-pipelines/pipelines-as-code/pkg/secrets/types: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github/app: github.com/openshift-pipelines/pipelines-as-code/pkg/test/cli: github.com/openshift-pipelines/pipelines-as-code/pkg/test/concurrency: github.com/openshift-pipelines/pipelines-as-code/pkg/test/clients: github.com/openshift-pipelines/pipelines-as-code/pkg/test/github: github.com/openshift-pipelines/pipelines-as-code/pkg/test/http: github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint: github.com/openshift-pipelines/pipelines-as-code/pkg/test/logger: github.com/openshift-pipelines/pipelines-as-code/pkg/test/provider: github.com/openshift-pipelines/pipelines-as-code/pkg/test/tekton: github.com/openshift-pipelines/pipelines-as-code/pkg/test/repository: github.com/openshift-pipelines/pipelines-as-code/pkg/test/nonoai: github.com/openshift-pipelines/pipelines-as-code/pkg/test/tracing: github.com/openshift-pipelines/pipelines-as-code/pkg/templates: github.com/openshift-pipelines/pipelines-as-code/pkg/secrets: github.com/openshift-pipelines/pipelines-as-code/pkg/webhook: github.com/openshift-pipelines/pipelines-as-code/pkg/tracing: github.com/openshift-pipelines/pipelines-as-code/pkg/tlsconfig: github.com/openshift-pipelines/pipelines-as-code/pkg/vcshost: github.com/openshift-pipelines/pipelines-as-code/pkg/sort: github.com/openshift-pipelines/pipelines-as-code/pkg/reconciler: github.com/openshift-pipelines/pipelines-as-code/pkg/resolve: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github: github.com/openshift-pipelines/pipelines-as-code/pkg/provider/gitlab: github.com/openshift-pipelines/pipelines-as-code/pkg/pipelineascode: DONE 3649 tests in 27.455s $ make lint
Linting go files...
0 issues.
Linting go files with nilaway...
Checking Go formatting with gofumpt...
Linting yaml files...
Linting markdown files...
Grammar check with vale of documentation...
CodeSpell on docs content
Linting python files...
All checks passed!
7 files already formatted
Linting shell script files...
Checking E2E test naming conventions...
All E2E tests follow the naming convention. |
The controller listed all Repository CRs cluster-wide from the API server on every incoming event, including events for repositories that are not onboarded. On busy multi-tenant clusters this put sustained load on the API server, scaling with both event rate and the number of repositories. Add an informer-backed RepositoryLister on params.Run with GetRepository/ListRepositories accessors that serve lookups from an in-memory cache and fall back to the API when no lister is set. Results are deep-copied so callers cannot mutate the shared cache, the cache is trimmed via RepositoryForCache, and the GitHub match path is routed through the cache as well. Secrets are intentionally not cached to avoid serving stale tokens after rotation and to keep the controller memory footprint bounded. Signed-off-by: Akshay Pant <akpant@redhat.com> Co-authored-by: Zaki Shaikh <zashaikh@redhat.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
4205d34 to
c6e91ff
Compare
|
Added unit tests in c6e91ff and attached the local |
| pacInformerFactory := pacinformers.NewSharedInformerFactory(run.Clients.PipelineAsCode, 10*time.Minute) | ||
| repoInformer := pacInformerFactory.Pipelinesascode().V1alpha1().Repositories() | ||
| if err := repoInformer.Informer().SetTransform(transform.RepositoryForCache); err != nil { | ||
| log.Fatal("failed to set transform on repository informer: ", err) | ||
| } | ||
| run.RepositoryLister = repoInformer.Lister() | ||
| pacInformerFactory.Start(ctx.Done()) | ||
|
|
There was a problem hiding this comment.
can we do the same way it's done for watcher?
There was a problem hiding this comment.
and also you need to setTransform for cache optimization as done in watcher already (see the code reference in above link)
| }, | ||
| ) | ||
| // A namespace pins the lookup to a single repository we can fetch by name. | ||
| if ns != "" { |
There was a problem hiding this comment.
| if ns != "" { | |
| if ns != "" && cs.RepositoryLister != nil { |
| @@ -1091,14 +1091,14 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C | |||
| } | |||
There was a problem hiding this comment.
pkg/provider/gitlab/parse_payload.go also matches the repository
2 participants
📝 Description of the Change
Problem
The controller resolves the
Repositoryfor an incoming event by listingrepositories directly from the API server. This happens on every event, with
an empty namespace (cluster-wide), via
matcher.MatchEventURLRepoand theGitHub provider's package-local
MatchEventURLRepo.On busy, multi-tenant clusters this is a significant, sustained source of
API-server load: the cost of each lookup is
O(number of Repository CRs in the cluster)and is paid once per event — including for events whose repository isnot onboarded and can never match.
Change
Introduce an informer-backed cache (lister) for
Repositoryresources soper-event lookups are served from an in-memory cache kept in sync by a single
watch, instead of a fresh cluster-wide
Listper event.RepositoryListertoparams.Run, populated at controller startup from ashared informer factory.
*params.Runthat centralize the cache-or-API decisionso call sites no longer hand-roll it:
GetRepository(ctx, ns, name)ListRepositories(ctx, ns)— all namespaces whenns == "".Both fall back to a live API call when no lister is configured (CLI, tests),
and both return deep copies so callers can mutate results without
corrupting the shared cache.
MatchEventURLRepothroughListRepositoriessothe highest-volume path also benefits from the cache. (The
githubpackagecannot import
matcherwithout an import cycle, so the local function is keptbut made cache-aware.)
repoByUniqueNamehelper shared by the cached and API paths.transform.RepositoryForCache(stripsManagedFields/Annotations,keeps
Spec) to the informer, matching the watcher's repository cache so thefootprint stays small.
detection, so events rejected early (bad payload, unknown provider) skip the
lookup entirely.
Correctness details
factory.Start().Start()only runs informers already registered, and requesting the lister iswhat registers the informer — doing this in the wrong order leaves the cache
permanently empty.
ClusterRolegainswatchonrepositories(inaddition to the existing
get, create, list), required by the informer.WaitForCacheSync's return value is checked and startupfails loudly on an incomplete sync rather than silently serving an empty cache.
downstream code calls
repo.Spec.Merge(...)in place; returningDeepCopy()prevents cache corruption and data races across concurrent request goroutines.
🔗 Linked GitHub Issue
Fixes #
🧪 Testing Strategy
🤖 AI Assistance
AI assistance can be used for various tasks, such as code generation,
documentation, or testing.
Please indicate whether you have used AI assistance
for this PR and provide details if applicable.
Important
Slop will be simply rejected, if you are using AI assistance you need to make sure you
understand the code generated and that it meets the project's standards. you
need at least know how to run the code and deploy it (if needed). See
startpaac to make it easy
to deploy and test your code changes.
If the majority of the code in this PR was generated by an AI, please add a
Co-authored-bytrailer to your commit message.For example:
Co-authored-by: Claude noreply@anthropic.com
✅ Submitter Checklist
fix:,feat:) matches the "Type of Change" I selected above.make testandmake lintlocally to check for and fix anyissues. For an efficient workflow, I have considered installing
pre-commit and running
pre-commit installtoautomate these checks.