Skip to content

Add repo cache handler to porch-server and repository GC to repo-controller - #1130

Open
kushnaidu wants to merge 10 commits into
kptdev:mainfrom
Nordix:warmup-cache-server
Open

Add repo cache handler to porch-server and repository GC to repo-controller #1130
kushnaidu wants to merge 10 commits into
kptdev:mainfrom
Nordix:warmup-cache-server

Conversation

@kushnaidu

@kushnaidu kushnaidu commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

V1ALPHA2 implementation is not required

Add repo cache handler to porch-server and repository GC to repo-controller


Description

  • What changed:
    Added repo_cache_handler.go in porch-server: a controller-runtime reconciler that watches Repository CRs, opens them when Ready, and evicts on delete using a finalizer to guarantee cleanup.
    Added EvictCachedRepository to the Cache interface (dbcache and crcache): iterates the sync.Map by namespace/name, removes the entry, and closes the git clone without touching the database.
    Added gc.go in repo-controller: periodically removes orphaned repository entries from the DB that no longer have a corresponding Repository CR.
    Added unit tests for all new code.

  • Why it’s needed:
    Without eviction, deleted repositories leak in porch-server — the sync.Map entry and git directory persist indefinitely.
    Without GC, repos deleted while porch-controller is down/cache miss/db error leave orphaned rows in the database.
    DB writes are owned exclusively by porch-controller, so porch-server uses memory-only operations to avoid race conditions.

  • How it works:
    The repo cache controller uses controller-runtime (informer + work queue, not a raw watch loop). It reconciles on Create (startup resync), Update (Ready status, spec change, DeletionTimestamp), and ignores Delete events (handled via finalizer).
    When a repo becomes Ready: calls OpenRepository to populate the cache and adds a porch.kpt.dev/repo-cache finalizer to guarantee eviction.
    When DeletionTimestamp is set: calls EvictCachedRepository regardless of finalizer presence, then removes the finalizer if it exists so the object can be garbage collected.
    Repos with the porch.kpt.dev/v1alpha2-migration annotation are skipped entirely.
    Concurrency is bounded by --max-parallel-repo-lists (default 10) via MaxConcurrentReconciles.
    EvictCachedRepository iterates the sync.Map matching by namespace/name. The shared git directory pool's refCounting ensures in-flight operations complete safely.
    GC runs every 10 minutes (configurable via --gc-interval), lists repo keys from the DB, compares against Repository CRs in Kubernetes, and deletes any that are orphaned.


Related Issue(s)

  • Closes/Fixes #

Type of Change

  • Bug fix
  • New feature
  • Enhancement
  • Refactor
  • Documentation
  • Tests
  • Other: ________

Checklist

  • Code follows project style guidelines
  • Self-reviewed changes
  • Tests added/updated
  • Documentation added/updated
  • All tests and gating checks pass

Testing Instructions (Optional)


Additional Notes (Optional)

  • Known issues:
  • Further improvements:
  • Review notes:

AI Disclosure

Amazon Q was used.

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
@kushnaidu
kushnaidu requested review from a team July 22, 2026 15:34
@netlify

netlify Bot commented Jul 22, 2026

Copy link
Copy Markdown

Deploy Preview for kpt-porch ready!

Name Link
🔨 Latest commit a5ef0e6
🔍 Latest deploy log https://app.netlify.com/projects/kpt-porch/deploys/6a6780b460a2ed000897b1fc
😎 Deploy Preview https://deploy-preview-1130--kpt-porch.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@kushnaidu
kushnaidu requested a review from nagygergo July 22, 2026 15:34
@dosubot dosubot Bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jul 22, 2026
@kushnaidu kushnaidu added the bug Something isn't working label Jul 22, 2026
@kushnaidu kushnaidu self-assigned this Jul 22, 2026
@github-actions
github-actions Bot requested a review from Copilot July 22, 2026 16:04

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Adds a startup cache warmup to PorchServer and defensive repo resolution for DB-loaded package revisions to avoid “no associated repository” failures after a server restart, plus improves observability by logging list/get failures.

Changes:

  • Warm up the in-memory repository cache on porch-server startup by listing Repository CRs and opening them concurrently.
  • Lazily resolve dbPackageRevision.repo from the cache in UpdateLifecycle / UpdateResources when missing.
  • Add error/warning logs for package/package-revision list/get paths to improve diagnostics.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
pkg/apiserver/apiserver.go Adds warmupRepositories and runs it on server start; adds MaxConcurrentLists wiring.
pkg/apiserver/apiserver_test.go Adds unit tests for warmup behavior (timeouts, concurrency, error handling).
pkg/cache/dbcache/dbpackagerevision.go Adds cache-based repo re-hydration for DB-loaded package revisions.
pkg/registry/porch/packagecommon.go Adds error/warn logs for list failures and not-found conditions.
pkg/registry/porch/packagerevisionresources.go Adds error/warn logs for list failures and not-found conditions.

Comment thread pkg/apiserver/apiserver.go Outdated
Comment thread pkg/apiserver/apiserver.go Outdated
Comment thread pkg/registry/porch/packagecommon.go Outdated
Comment thread pkg/registry/porch/packagerevisionresources.go Outdated
Comment thread pkg/cache/dbcache/dbpackagerevision.go Outdated
Comment thread pkg/apiserver/apiserver_test.go Outdated
Comment thread pkg/apiserver/apiserver_test.go Outdated
@nagygergo

nagygergo commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

I think that in case of large sets of repositories and number of repositories >> number of changes to pkgrevs in a short time, forcing re-caching at pod startup before serving the APIs can mean a lot of unnecessary unavailability.

Wouldn't it be better if all the calls that require that a repo is cached block/wait until that repo is cached?

@liamfallon

Copy link
Copy Markdown
Contributor

Please include the 1alpha2 implementation as well.

@kushnaidu

Copy link
Copy Markdown
Contributor Author

Hey @nagygergo, true maybe we should either bring back the old background.go way which runs in a go routine or just fix the propose-delete issue for now and think of a better solution.

@kushnaidu

Copy link
Copy Markdown
Contributor Author

Hey @liamfallon, I don't think there's a problem with v1alpha2, I'll verify this again.

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
@dosubot dosubot Bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:M This PR changes 30-99 lines, ignoring generated files. labels Jul 23, 2026
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
@kushnaidu
kushnaidu force-pushed the warmup-cache-server branch from 1bc534b to 28ed9a2 Compare July 23, 2026 18:45
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
@kushnaidu
kushnaidu force-pushed the warmup-cache-server branch from 8e2f2be to a7cb46f Compare July 23, 2026 18:51
@nagygergo

nagygergo commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

@kushnaidu I think that Repository readiness should be tied to having a cached replica in database, i.e. the read cache is populated.
In case git repo is not cloned, i.e it is a write-cache miss, then the operation will take a bit more time, just as with any other cache miss situations.
So requiring that the cache is loaded at first write operation time against the repo is the right solution.

@kushnaidu

Copy link
Copy Markdown
Contributor Author

Hey @nagygergo , yes I agree. I was also hesitent to bring back the full repo cache handling.
I'll simplify this completely to just handle cache eviction when a repo is deleted. the code will not watch for ADDED/MODIFIED events. We can the webhook to also block secret changes to the spec.
In that way, we will not have the heap leak and the write cache will get populated on the first write operation on the repo.

Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jul 24, 2026
@kushnaidu kushnaidu changed the title Add repo cache warmup to porch server Add repo cache eviction handler to porch-server and repository GC to repo-controller Jul 24, 2026
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>

@nagygergo nagygergo Jul 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can it happen that repositories are removed from etcd, but their corresponding entry is not cleaned up from the database? It should be handled with finalisers ideally.
There is no guarantee that this gc is triggered when a repository is removed then re-added, and I assume that can lead to consistency issues.

@kushnaidu kushnaidu Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is handled by Finalizers. I faced an issue during the delete from the DB(DB error). The function gave a false positive.
I tested quick repo deletes and creates keep the gc interval very low, I didn't face any problem. We could make the GC run only at the start or the pod.

Comment thread pkg/apiserver/repo_cache_handler.go Outdated
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
Signed-off-by: Kushal Harish Naidu <kushal.harish.naidu@ericsson.com>
@kushnaidu kushnaidu changed the title Add repo cache eviction handler to porch-server and repository GC to repo-controller Add repo cache handler to porch-server and repository GC to repo-controller Jul 27, 2026
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
62.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working do-not-merge/work-in-progress #ededed size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants