From 7f8aad23e6978f3bfe08f5eefb6153ff1f384bba Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 24 Nov 2022 17:21:58 -0800 Subject: [PATCH 1/2] e2e: fix sync_fetch_skip_depth_1 e2e: fix sync_fetch_skip_depth_1 The improvements in e2e perf broke this test case. Make it more explicit - this is not a success (triggering touch), but not really a failure either. Now it will not touch the touch-file. --- cmd/git-sync/main.go | 47 ++++++++++++++++++++++++-------------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index bd0be47..c5e5099 100644 --- a/cmd/git-sync/main.go +++ b/cmd/git-sync/main.go @@ -1088,8 +1088,10 @@ func (git *repoSync) CleanupWorkTree(ctx context.Context, gitRoot, worktree stri return nil } -// AddWorktreeAndSwap creates a new worktree and calls UpdateSymlink to swap the symlink to point to the new worktree -func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error { +// AddWorktreeAndSwap creates a new worktree and calls UpdateSymlink to swap +// the symlink to point to the new worktree. This returns 1) whether the link +// has changed; 2) what the new hash is; 3) was there an error to be reported. +func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) (bool, string, error) { git.log.V(0).Info("syncing git", "rev", git.rev, "hash", hash) args := []string{"fetch", "-f", "--tags"} @@ -1104,15 +1106,18 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error // Update from the remote. if _, err := git.run.Run(ctx, git.root, nil, git.cmd, args...); err != nil { - return err + return false, "", err } // With shallow fetches, it's possible to race with the upstream repo and // end up NOT fetching the hash we wanted. If we can't resolve that hash // to a commit we can just end early and leave it for the next sync period. + // This probably SHOULD be an error, but it can be a problem for startup + // and first-sync-must-succeed semantics. If/when we fix that this can be + // fixed. if _, err := git.ResolveRef(ctx, hash); err != nil { git.log.Error(err, "can't resolve commit, will retry", "rev", git.rev, "hash", hash) - return nil + return false, "", nil } // Make a worktree for this exact git hash. @@ -1123,13 +1128,13 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error // create the worktree and bails out. This manifests as: // "fatal: '/repo/root/rev-nnnn' already exists" if err := git.CleanupWorkTree(ctx, git.root, worktreePath); err != nil { - return err + return false, "", err } git.log.V(0).Info("adding worktree", "path", worktreePath, "hash", hash) _, err := git.run.Run(ctx, git.root, nil, git.cmd, "worktree", "add", "--detach", worktreePath, hash, "--no-checkout") if err != nil { - return err + return false, "", err } // The .git file in the worktree directory holds a reference to @@ -1138,11 +1143,11 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error // mount name. worktreePathRelative, err := filepath.Rel(git.root, worktreePath) if err != nil { - return err + return false, "", err } gitDirRef := []byte(filepath.Join("gitdir: ../.git/worktrees", worktreePathRelative) + "\n") if err = os.WriteFile(filepath.Join(worktreePath, ".git"), gitDirRef, 0644); err != nil { - return err + return false, "", err } // If sparse checkout is requested, configure git for it. @@ -1157,7 +1162,7 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error source, err := os.Open(checkoutFile) if err != nil { - return err + return false, "", err } defer source.Close() @@ -1165,32 +1170,32 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error fileMode := os.FileMode(int(0755)) err := os.Mkdir(gitInfoPath, fileMode) if err != nil { - return err + return false, "", err } } destination, err := os.Create(gitSparseConfigPath) if err != nil { - return err + return false, "", err } defer destination.Close() _, err = io.Copy(destination, source) if err != nil { - return err + return false, "", err } args := []string{"sparse-checkout", "init"} _, err = git.run.Run(ctx, worktreePath, nil, git.cmd, args...) if err != nil { - return err + return false, "", err } } // Reset the worktree's working copy to the specific rev. _, err = git.run.Run(ctx, worktreePath, nil, git.cmd, "reset", "--hard", hash, "--") if err != nil { - return err + return false, "", err } git.log.V(0).Info("reset worktree to hash", "path", worktreePath, "hash", hash) @@ -1207,7 +1212,7 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error } _, err = git.run.Run(ctx, worktreePath, nil, git.cmd, submodulesArgs...) if err != nil { - return err + return false, "", err } } @@ -1217,7 +1222,7 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error git.log.V(0).Info("changing file permissions", "mode", mode) _, err = git.run.Run(ctx, "", nil, "chmod", "-R", mode, worktreePath) if err != nil { - return err + return false, "", err } } @@ -1225,14 +1230,14 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error // Use --soft so we do not check out files into the root. _, err = git.run.Run(ctx, git.root, nil, git.cmd, "reset", "--soft", hash, "--") if err != nil { - return err + return false, "", err } git.log.V(0).Info("reset root to hash", "path", git.root, "hash", hash) // Flip the symlink. oldWorktree, err := git.UpdateSymlink(ctx, worktreePath) if err != nil { - return err + return false, "", err } setRepoReady() @@ -1263,9 +1268,9 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error } if len(cleanupErrs) > 0 { - return cleanupErrs + return true, hash, cleanupErrs } - return nil + return true, hash, nil } type multiError []error @@ -1446,7 +1451,7 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con hash = remote } - return true, hash, git.AddWorktreeAndSwap(ctx, hash) + return git.AddWorktreeAndSwap(ctx, hash) } func (git *repoSync) ensureLocalHashForRev(ctx context.Context, rev string) (string, error) { From 59e2d9e97c8d50523808232024e2aef6a4cb5484 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Thu, 24 Nov 2022 17:47:45 -0800 Subject: [PATCH 2/2] e2e: fix exechook_fail_retry Previous e2e perf changes caused this to fail. --- test_e2e.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_e2e.sh b/test_e2e.sh index 492d687..0d1577a 100755 --- a/test_e2e.sh +++ b/test_e2e.sh @@ -177,7 +177,7 @@ EXECHOOK_COMMAND_SLEEPY=/test_exechook_command_with_sleep.sh EXECHOOK_COMMAND_FAIL_SLEEPY=/test_exechook_command_fail_with_sleep.sh EXECHOOK_ENVKEY=ENVKEY EXECHOOK_ENVVAL=envval -RUNLOG="$DIR/runlog.exechook-fail-retry" +RUNLOG="$DIR/runlog" rm -f $RUNLOG touch $RUNLOG @@ -1390,7 +1390,7 @@ function e2e::exechook_fail_retry() { --exechook-command="$EXECHOOK_COMMAND_FAIL" \ --exechook-backoff=1s \ >> "$1" 2>&1 & - wait_for_sync 3 + sleep 3 # give it time to retry # Check that exechook was called RUNS=$(cat "$RUNLOG" | wc -l)