-
Couldn't load subscription status.
- Fork 134
fix: Cannot deploy MCPRegistry from git in OpenShift #2263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2263 +/- ##
==========================================
+ Coverage 54.30% 54.40% +0.10%
==========================================
Files 240 241 +1
Lines 23594 23695 +101
==========================================
+ Hits 12812 12891 +79
- Misses 9567 9587 +20
- Partials 1215 1217 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR optimizes Git repository operations by implementing sparse checkout to reduce memory usage and improve performance. The root cause was that /tmp was not writable in Kubernetes deployments and clones required excessive memory, causing controller restarts. Additionally, the repository was being cloned twice unnecessarily.
Key changes:
- Replaced multi-step clone operations with a single
FetchFileSparsemethod using sparse checkout - Added configurable workspace directory via
WORKSPACE_DIRenvironment variable - Mounted
emptyDirvolume at/workspacein Kubernetes deployments
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
deploy/charts/operator/templates/deployment.yaml |
Added WORKSPACE_DIR environment variable and mounted workspace volume |
cmd/thv-operator/pkg/sources/git.go |
Refactored to use sparse checkout and eliminated duplicate cloning |
cmd/thv-operator/pkg/sources/git_test.go |
Updated tests to use new FetchFileSparse API |
cmd/thv-operator/pkg/git/client.go |
Implemented sparse checkout with path traversal protection |
cmd/thv-operator/pkg/git/integration_test.go |
Updated integration tests for new API with improved test coverage |
cmd/thv-operator/pkg/git/client_test.go |
Added comprehensive unit tests including security tests |
cmd/thv-operator/pkg/git/e2e_test.go |
Added E2E tests for real repository operations |
cmd/thv-operator/pkg/git/doc.go |
Updated documentation to reflect sparse checkout implementation |
cmd/thv-operator/pkg/git/commit_test.go |
Removed obsolete commit-specific tests |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@claude can you review this with a focus on security? |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
a38cbc7 to
3a9f74e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better! A couple of findings inline
cmd/thv-operator/pkg/git/client.go
Outdated
| storerCache := cache.NewObjectLRUDefault() | ||
| storer := filesystem.NewStorage(storerFs, storerCache) | ||
|
|
||
| repo, err := git.Clone(storer, memFS, cloneOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We went from git.PlainCloneContext to git.Clone (no context), could we use git.CloneContext ?
| repoInfo.Repository = nil | ||
|
|
||
| // 5. Force GC to reclaim memory | ||
| runtime.GC() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't think nudging the GC is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this call, the Pod still crashes with OOMKilled error.
Looks like it's really needed.
I would postpone this finalization once the logic moves to the thv-registry-api, since the same issue will be there, WDYT?
…client Using configurable workspacedir to checkout the repo Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
- more logging - increased memory size Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
…y FS - added env variables to expedite the GC and avoid out of memory errors Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
3a9f74e to
987ef13
Compare
chart version bump run task operator-manifests Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Signed-off-by: Daniele Martinoli <dmartino@redhat.com>
Fixes #2250
Root cause
Proposed solution
- Using sparse checkout to reduce the local repo size:~ - Note: this also copies the other files and the subfloders from the registry data folder~
Alternatives