Skip to content
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.

Sign up for GitHub

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

fix: fetch syncedRevision in UpdateRevisionForPaths (#21014) #21015

Merged
merged 12 commits into from
Feb 25, 2025
Merged

fix: fetch syncedRevision in UpdateRevisionForPaths (#21014) #21015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
948464c
fix: check syncRevisionCache in UpdateRevisionForPaths
toyamagu-2021 Dec 1, 2024
4d4dc17
fix
toyamagu-2021 Dec 1, 2024
706625e
fix
toyamagu-2021 Dec 1, 2024
6c50dfa
fix: add fetch function
toyamagu-2021 Dec 2, 2024
41d8400
fix: add comment
toyamagu-2021 Dec 2, 2024
35963d3
fix: throw error
toyamagu-2021 Dec 2, 2024
79e6e26
fix: lint
toyamagu-2021 Dec 2, 2024
a7aa687
fix: only one syncedRevision
toyamagu-2021 Dec 2, 2024
05e2df0
fix: comment
toyamagu-2021 Dec 2, 2024
4bfdc13
Merge branch 'master' into fix-21014
toyamagu-2021 Jan 28, 2025
f7b1388
Merge remote-tracking branch 'upstream/master' into fix-21014
toyamagu-2021 Jan 28, 2025
70e550d
Merge branch 'master' into fix-21014
toyamagu-2021 Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions reposerver/repository/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2507,6 +2507,50 @@ func (s *Service) checkoutRevision(gitClient git.Client, revision string, submod
return closer, err
}

// fetch is a convenience function to fetch revisions
// We assumed that the caller has already initialized the git repo, i.e. gitClient.Init() has been called
func (s *Service) fetch(gitClient git.Client, targetRevisions []string) error {
err := fetch(gitClient, targetRevisions)
if err != nil {
for _, revision := range targetRevisions {
s.metricsServer.IncGitFetchFail(gitClient.Root(), revision)
}
}
return err
}

func fetch(gitClient git.Client, targetRevisions []string) error {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
func fetch(gitClient git.Client, targetRevisions []string) error {
func fetchRevision(gitClient git.Client, targetRevisions []string) error {

Is there a specific reason behind using the same func name as fetch? If not, we can probably change the func name to avoid confusion.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not being particular, but it is the same naming convention as checkoutRevision.

https://github.com/toyamagu-2021/argo-cd/blob/514df2c77c58c8bc5f4da1000e09af0e8e4fb552/reposerver/repository/repository.go#L2554

revisionPresent := true
for _, revision := range targetRevisions {
revisionPresent = gitClient.IsRevisionPresent(revision)
if !revisionPresent {
break
}
}
// Fetching can be skipped if the revision is already present locally.
if revisionPresent {
return nil
}
// Fetching with no revision first. Fetching with an explicit version can cause repo bloat. https://github.com/argoproj/argo-cd/issues/8845
err := gitClient.Fetch("")
if err != nil {
return err
}
for _, revision := range targetRevisions {
if !gitClient.IsRevisionPresent(revision) {
// When fetching with no revision, only refs/heads/* and refs/remotes/origin/* are fetched. If fetch fails
// for the given revision, try explicitly fetching it.
log.Infof("Failed to fetch revision %s: %v", revision, err)
log.Infof("Fallback to fetching specific revision %s. ref might not have been in the default refspec fetched.", revision)

if err := gitClient.Fetch(revision); err != nil {
return status.Errorf(codes.Internal, "Failed to fetch revision %s: %v", revision, err)
}
}
}
return nil
}

func checkoutRevision(gitClient git.Client, revision string, submoduleEnabled bool) error {
err := gitClient.Init()
if err != nil {
Expand Down Expand Up @@ -2847,6 +2891,10 @@ func (s *Service) UpdateRevisionForPaths(_ context.Context, request *apiclient.U
}
defer io.Close(closer)

if err := s.fetch(gitClient, []string{syncedRevision}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

We lock on revision above, but use syncedRevision here. Should it be consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

To exec git diff syncRevision..targetRevision we only need to fetch from remote, so I think we needn't take a lock.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Maybe we can release the lock earlier then.

return nil, status.Errorf(codes.Internal, "unable to fetch git repo %s with syncedRevisions %s: %v", repo.Repo, syncedRevision, err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How would locking above work with both revisions being used here? Can we lock for multiple revisions?

Copy link
Member Author

@toyamagu-2021 toyamagu-2021 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should we take a lock to fetch from the remote? If we checkout we need to take a lock, but for fetch I think we don't need to lock.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know


files, err := gitClient.ChangedFiles(syncedRevision, revision)
if err != nil {
return nil, status.Errorf(codes.Internal, "unable to get changed files for repo %s with revision %s: %v", repo.Repo, revision, err)
Expand Down
80 changes: 73 additions & 7 deletions reposerver/repository/repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3071,6 +3071,61 @@ func TestCheckoutRevisionNotPresentCallFetch(t *testing.T) {
require.NoError(t, err)
}

func TestFetch(t *testing.T) {
revision1 := "0123456789012345678901234567890123456789"
revision2 := "abcdefabcdefabcdefabcdefabcdefabcdefabcd"

gitClient := &gitmocks.Client{}
gitClient.On("Init").Return(nil)
gitClient.On("IsRevisionPresent", revision1).Once().Return(true)
gitClient.On("IsRevisionPresent", revision2).Once().Return(false)
gitClient.On("Fetch", "").Return(nil)
gitClient.On("IsRevisionPresent", revision1).Once().Return(true)
gitClient.On("IsRevisionPresent", revision2).Once().Return(true)

err := fetch(gitClient, []string{revision1, revision2})
require.NoError(t, err)
}

// TestFetchRevisionCanGetNonstandardRefs shows that we can fetch a revision that points to a non-standard ref. In
func TestFetchRevisionCanGetNonstandardRefs(t *testing.T) {
rootPath := t.TempDir()

sourceRepoPath, err := os.MkdirTemp(rootPath, "")
require.NoError(t, err)

// Create a repo such that one commit is on a non-standard ref _and nowhere else_. This is meant to simulate, for
// example, a GitHub ref for a pull into one repo from a fork of that repo.
runGit(t, sourceRepoPath, "init")
runGit(t, sourceRepoPath, "checkout", "-b", "main") // make sure there's a main branch to switch back to
runGit(t, sourceRepoPath, "commit", "-m", "empty", "--allow-empty")
runGit(t, sourceRepoPath, "checkout", "-b", "branch")
runGit(t, sourceRepoPath, "commit", "-m", "empty", "--allow-empty")
sha := runGit(t, sourceRepoPath, "rev-parse", "HEAD")
runGit(t, sourceRepoPath, "update-ref", "refs/pull/123/head", strings.TrimSuffix(sha, "\n"))
runGit(t, sourceRepoPath, "checkout", "main")
runGit(t, sourceRepoPath, "branch", "-D", "branch")

destRepoPath, err := os.MkdirTemp(rootPath, "")
require.NoError(t, err)

gitClient, err := git.NewClientExt("file://"+sourceRepoPath, destRepoPath, &git.NopCreds{}, true, false, "", "")
require.NoError(t, err)

// We should initialize repository
err = gitClient.Init()
require.NoError(t, err)

pullSha, err := gitClient.LsRemote("refs/pull/123/head")
require.NoError(t, err)

err = fetch(gitClient, []string{"does-not-exist"})
require.Error(t, err)

err = fetch(gitClient, []string{pullSha})
require.NoError(t, err)
}

// runGit runs a git command in the given working directory. If the command succeeds, it returns the combined standard
// and error output. If it fails, it stops the test with a failure message.
func runGit(t *testing.T, workDir string, args ...string) string {
Expand Down Expand Up @@ -3746,9 +3801,13 @@ func TestUpdateRevisionForPaths(t *testing.T) {
{name: "ChangedFilesDoNothing", fields: func() fields {
s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, paths *iomocks.TempPaths) {
gitClient.On("Init").Return(nil)
gitClient.On("IsRevisionPresent", mock.Anything).Return(false)
gitClient.On("Fetch", mock.Anything).Return(nil)
gitClient.On("Checkout", mock.Anything, mock.Anything).Return("", nil)
gitClient.On("Fetch", mock.Anything).Once().Return(nil)
gitClient.On("IsRevisionPresent", "632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false)
gitClient.On("Checkout", "632039659e542ed7de0c170a4fcc1c571b288fc0", mock.Anything).Once().Return("", nil)
// fetch
gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(false)
gitClient.On("Fetch", mock.Anything).Once().Return(nil)
gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true)
gitClient.On("LsRemote", "HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil)
gitClient.On("LsRemote", "SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil)
paths.On("GetPath", mock.Anything).Return(".", nil)
Expand All @@ -3775,9 +3834,13 @@ func TestUpdateRevisionForPaths(t *testing.T) {
{name: "NoChangesUpdateCache", fields: func() fields {
s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, paths *iomocks.TempPaths) {
gitClient.On("Init").Return(nil)
gitClient.On("IsRevisionPresent", mock.Anything).Return(false)
gitClient.On("Fetch", mock.Anything).Return(nil)
gitClient.On("Fetch", mock.Anything).Once().Return(nil)
gitClient.On("IsRevisionPresent", "632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false)
gitClient.On("Checkout", mock.Anything, mock.Anything).Return("", nil)
gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(false)
// fetch
gitClient.On("Fetch", mock.Anything).Once().Return(nil)
gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true)
gitClient.On("LsRemote", "HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil)
gitClient.On("LsRemote", "SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil)
paths.On("GetPath", mock.Anything).Return(".", nil)
Expand Down Expand Up @@ -3813,9 +3876,12 @@ func TestUpdateRevisionForPaths(t *testing.T) {
{name: "NoChangesHelmMultiSourceUpdateCache", fields: func() fields {
s, _, c := newServiceWithOpt(t, func(gitClient *gitmocks.Client, _ *helmmocks.Client, paths *iomocks.TempPaths) {
gitClient.On("Init").Return(nil)
gitClient.On("IsRevisionPresent", mock.Anything).Return(false)
gitClient.On("Fetch", mock.Anything).Return(nil)
gitClient.On("IsRevisionPresent", "632039659e542ed7de0c170a4fcc1c571b288fc0").Once().Return(false)
gitClient.On("Fetch", mock.Anything).Once().Return(nil)
gitClient.On("Checkout", mock.Anything, mock.Anything).Return("", nil)
// fetch
gitClient.On("IsRevisionPresent", "1e67a504d03def3a6a1125d934cb511680f72555").Once().Return(true)
gitClient.On("Fetch", mock.Anything).Once().Return(nil)
gitClient.On("LsRemote", "HEAD").Once().Return("632039659e542ed7de0c170a4fcc1c571b288fc0", nil)
gitClient.On("LsRemote", "SYNCEDHEAD").Once().Return("1e67a504d03def3a6a1125d934cb511680f72555", nil)
paths.On("GetPath", mock.Anything).Return(".", nil)
Expand Down