Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nagygergo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Skipping CI for Draft Pull Request. |
nagygergo
force-pushed
the
create-remove-fetches
branch
from
60c8e5b to
9580f39
Compare
|
Thank you for picking this up :) this is an important problem to solve. |
nagygergo
force-pushed
the
create-remove-fetches
branch
from
9580f39 to
69bec1f
Compare
replacing way to compute deletion proposed packages
nagygergo
force-pushed
the
create-remove-fetches
branch
from
69bec1f to
f77f5da
Compare
|
|
@nagygergo Could we capture the content here as an issue and close the PR? |
There was a problem hiding this comment.
Pull request overview
This PR is working toward “removing fetches” in the Git external repo implementation by adjusting how package revisions are retrieved and how repository state is tracked, and by expanding test coverage around lifecycle edge cases.
Changes:
- Replace
GetPackageRevisionwithGetPackageRevisionWithoutFetchat call sites. - Remove deletion-proposed ref caching and compute deletion-proposed lifecycle by direct ref lookup; cache lifecycle on
gitPackageRevision. - Introduce an internal repository
versioncounter and add several new Git lifecycle/approval tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/task/clone.go | Uses GetPackageRevisionWithoutFetch when cloning from Git. |
| pkg/externalrepo/git/ref.go | Removes unused deletion-proposed branch parsing helper. |
| pkg/externalrepo/git/push.go | Adds documentation to RequireRef. |
| pkg/externalrepo/git/package_tree.go | Populates gitPackageRevision.lifecycle during discovery. |
| pkg/externalrepo/git/package.go | Stores lifecycle on gitPackageRevision and returns cached value. |
| pkg/externalrepo/git/git_test.go | Updates roundtrip test and adds multiple lifecycle edge-case tests. |
| pkg/externalrepo/git/git.go | Renames API, removes deletion-proposed cache, adds version counter, changes lifecycle detection logic, and removes some fetch behavior. |
| pkg/cache/crcache/repository.go | Removes explicit repo.Refresh() on force refresh path. |
Comments suppressed due to low confidence (1)
pkg/cache/crcache/repository.go:185
- forceRefresh no longer forces a refresh of the underlying repository state; it only nils the in-memory maps. Because refreshAllCachedPackages can return early when Version() matches lastVersion, a forceRefresh call can end up returning stale cached data without ever calling ListPackageRevisions/refreshing the repo. Consider bypassing the version short-circuit when forceRefresh==true, or reintroducing an explicit underlying refresh/fetch before rebuilding the cache.
if forceRefresh {
packages = nil
packageRevisions = nil
}
if packages == nil {
packages, packageRevisions, err = r.refreshAllCachedPackages(ctx)
}
return packages, packageRevisions, err
You can also share your feedback on Copilot code review. Take the survey.
| func (r *gitRepository) fetchRemoteRepository(ctx context.Context) error { | ||
| ctx, span := tracer.Start(ctx, "gitRepository::fetchRemoteRepository", trace.WithAttributes()) | ||
| defer span.End() | ||
|
|
||
| // Fetch | ||
| switch err := r.doGitWithAuth(ctx, func(auth transport.AuthMethod) error { | ||
| return r.repo.Fetch(&git.FetchOptions{ | ||
| RemoteName: OriginName, | ||
| Auth: auth, | ||
| Prune: true, | ||
| CABundle: r.caBundle, | ||
| }) | ||
| }); err { | ||
| case nil: // OK | ||
| case git.NoErrAlreadyUpToDate: | ||
| case transport.ErrEmptyRemoteRepository: | ||
|
|
||
| default: | ||
| return fmt.Errorf("cannot fetch repository %s/%s: %w", r.Key().Namespace, r.Key().Name, err) | ||
| } | ||
| r.updateRepositoryVersion() | ||
|
|
||
| return nil |
There was a problem hiding this comment.
fetchRemoteRepository() calls updateRepositoryVersion() even when the fetch is a no-op (git.NoErrAlreadyUpToDate). With the new version-counter-based Version(), this means simply polling/fetching will advance the version and can force the cache to refresh continuously even when nothing changed. Consider only incrementing the repository version when the fetch actually updates refs, and similarly avoid bumping the version on no-op pushes.
| if _, found := r.deletionProposedCache[branchName]; found { | ||
| return v1alpha1.PackageRevisionLifecycleDeletionProposed | ||
| branchNameInLocal := createDeletionProposedName(pkgRev.Key()).RefInLocal() | ||
| klog.Infof("looking for branch %q", branchNameInLocal) |
There was a problem hiding this comment.
checkPublishedLifecycle() logs at Info level for every lifecycle check. This function runs during package discovery/listing, so this can generate very high log volume in production. Consider lowering to a verbose level (e.g., klog.V(4).Infof) or removing the log entirely.
| klog.Infof("looking for branch %q", branchNameInLocal) | |
| klog.V(4).Infof("looking for branch %q", branchNameInLocal) |
| // Requries a reference to be existent with the correct during the push. | ||
| // The ref is evaluated on the client side only during a send-pack session in go-git, and doesn't provide a guarantee | ||
| // that the reference will be locked on the server side during the push. | ||
| // Reference cannot be pointing to plumbing.ZeroHash. |
There was a problem hiding this comment.
Spelling/grammar in this new doc comment: "Requries" → "Requires", and the sentence "to be existent with the correct during the push" is unclear. Please fix the typo and rephrase so the contract is unambiguous (e.g., that the ref must exist and match the expected hash).
| // Requries a reference to be existent with the correct during the push. | |
| // The ref is evaluated on the client side only during a send-pack session in go-git, and doesn't provide a guarantee | |
| // that the reference will be locked on the server side during the push. | |
| // Reference cannot be pointing to plumbing.ZeroHash. | |
| // Requires the given reference to exist and to point to the specified hash when the push is performed. | |
| // The ref is evaluated on the client side only during a send-pack session in go-git, and doesn't provide a guarantee | |
| // that the reference will be locked on the server side during the push. | |
| // The reference cannot point to plumbing.ZeroHash. |
| /// 2^32 > 1 billion possible initial repository versions can happen, | ||
| // so collisions are very unlikely. | ||
| // Also even if incrementing from 2^32 to 2^64 before the uint64 every second, | ||
| // it would take about 500 billion years to get uint64 to overlflow. |
There was a problem hiding this comment.
The new repository version comments contain typos/formatting issues (e.g., "///" and "overlflow") and an incorrect magnitude note (2^32 is ~4.3 billion, not just >1 billion). Please clean up the comment text so it’s accurate and consistently formatted (single //).
| /// 2^32 > 1 billion possible initial repository versions can happen, | |
| // so collisions are very unlikely. | |
| // Also even if incrementing from 2^32 to 2^64 before the uint64 every second, | |
| // it would take about 500 billion years to get uint64 to overlflow. | |
| // of collision because there are 2^32 (approximately 4.3 billion) possible | |
| // initial repository versions. | |
| // Even if the uint64 repository version were incremented every second, | |
| // it would take on the order of 500 billion years to overflow. |
| t.Fatalf("Expected PackageRevision to be in deletion proposed state, got %s", prs[0].Lifecycle(ctx)) | ||
| } | ||
| } else { | ||
| if prs2[prToBeVerified].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecyclePublished { | ||
| t.Fatalf("Expected PackageRevision to be in published state, got %s", prs[prToBeVerified].Lifecycle(ctx)) |
There was a problem hiding this comment.
These failure messages reference the wrong slice (prs / prs[prToBeVerified]) when validating prs2. If this assertion fails, the reported lifecycle may be unrelated/misleading. Use prs2[prToBeVerified] in the error message formatting.
| t.Fatalf("Expected PackageRevision to be in deletion proposed state, got %s", prs[0].Lifecycle(ctx)) | |
| } | |
| } else { | |
| if prs2[prToBeVerified].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecyclePublished { | |
| t.Fatalf("Expected PackageRevision to be in published state, got %s", prs[prToBeVerified].Lifecycle(ctx)) | |
| t.Fatalf("Expected PackageRevision to be in deletion proposed state, got %s", prs2[prToBeVerified].Lifecycle(ctx)) | |
| } | |
| } else { | |
| if prs2[prToBeVerified].Lifecycle(ctx) != v1alpha1.PackageRevisionLifecyclePublished { | |
| t.Fatalf("Expected PackageRevision to be in published state, got %s", prs2[prToBeVerified].Lifecycle(ctx)) |
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| func (p *gitPackageRevision) Lifecycle(ctx context.Context) v1alpha1.PackageRevisionLifecycle { | ||
| return p.repo.GetLifecycle(ctx, p) | ||
| return p.lifecycle | ||
| } | ||
|
|
||
| func (p *gitPackageRevision) UpdateLifecycle(ctx context.Context, new v1alpha1.PackageRevisionLifecycle) error { | ||
| ctx, span := tracer.Start(ctx, "gitPackageRevision::UpdateLifecycle", trace.WithAttributes()) | ||
| defer span.End() | ||
| err := p.repo.UpdateLifecycle(ctx, p, new) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| p.lifecycle = new | ||
|
|
||
| return p.repo.UpdateLifecycle(ctx, p, new) | ||
| return nil |
There was a problem hiding this comment.
gitPackageRevision.lifecycle is now read/written without synchronization (Lifecycle() reads directly; UpdateLifecycle() writes directly). Since gitPackageRevision already has a mutex used for other fields, this introduces a potential data race under concurrent access (e.g., cache/watchers). Consider guarding lifecycle reads/writes with p.mutex (or using an atomic/immutable design) to keep race-detector clean and behavior deterministic.
| ErrorIfMissing MainBranchStrategy = iota // ErrorIsMissing | ||
| CreateIfMissing // CreateIfMissing | ||
| SkipVerification // SkipVerification | ||
| SkipVerification // SkipVerification\ |
There was a problem hiding this comment.
The enum comment for SkipVerification has a trailing "" which looks accidental and can be confusing when reading generated docs. Please remove the stray backslash from the comment.
| SkipVerification // SkipVerification\ | |
| SkipVerification // SkipVerification |
| t.Fatalf("Failed to create commit helper: %v", err) | ||
| } | ||
|
|
||
| err = ch.storeFile(filepath.Join(packageName, newFile), newFileContent) |
There was a problem hiding this comment.
In this test, storeFile paths are split on "/" inside commitHelper (see split() in commit.go). Using filepath.Join here will produce OS-specific separators (e.g., "\" on Windows) and can break the test/behavior. Prefer path.Join for git tree paths.
| err = ch.storeFile(filepath.Join(packageName, newFile), newFileContent) | |
| err = ch.storeFile(path.Join(packageName, newFile), newFileContent) |
| func(retryNumber int) error { | ||
| klog.Infof("Deleting PackageRevision try number %d", retryNumber) | ||
|
|
||
| if version == 0 { | ||
| return nil, pkgerrors.New("Version cannot be empty for the next package revision") | ||
| } | ||
| d.prKey.Revision = version | ||
| if retryNumber > 0 { | ||
| err := r.fetchRemoteRepository(ctx) | ||
| if err != nil { |
There was a problem hiding this comment.
The retry log message says "Deleting PackageRevision" but this code is in ClosePackageRevisionDraft and is retrying a close/push due to ref conflicts. Consider updating the message (and possibly lowering verbosity) so operational logs accurately reflect what is being retried.
| func (r *gitRepository) Version(ctx context.Context) (string, error) { | ||
| _, span := tracer.Start(ctx, "gitRepository::Version", trace.WithAttributes()) | ||
| defer span.End() | ||
| r.mutex.Lock() | ||
| defer r.mutex.Unlock() | ||
|
|
||
| if err := r.fetchRemoteRepositoryWithRetry(ctx); err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| refs, err := r.repo.References() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
|
|
||
| b := bytes.Buffer{} | ||
| for { | ||
| ref, err := refs.Next() | ||
| if err == io.EOF { | ||
| break | ||
| } | ||
|
|
||
| b.WriteString(ref.String()) | ||
| } | ||
| return strconv.FormatUint(r.version, 10), nil | ||
|
|
||
| hash := sha256.Sum256(b.Bytes()) | ||
| return hex.EncodeToString(hash[:]), nil | ||
| } |
There was a problem hiding this comment.
gitRepository.Version() no longer fetches/inspects remote refs and now just returns an internal counter. The crcache layer uses Repository.Version() to decide whether to refresh (refreshAllCachedPackages returns early when versions match), so this change breaks the cache’s ability to reliably detect upstream changes and can also cause perpetual refresh/version skew if ListPackageRevisions/fetch increments the counter after Version() is sampled. Consider either restoring a remote-ref–derived, stable version (e.g., hash of refs after fetch), or updating the cache contract: Version() should reflect remote changes, and/or lastVersion should be captured after any operation that can mutate it (ListPackageRevisions/fetch/push).
No description provided.