Merge fcbb1d1603 into d499de1839

_test_tools/exechook_command_with_sleep.sh

@@ -19,4 +19,6 @@
 
 sleep 3
 cat file > exechook
+cat exechook
+if [[ "$(pwd)" != "$(pwd -P)" ]]; then echo "true" > delaycheck; fi
 echo "ENVKEY=$ENVKEY" > exechook-env

main.go

@@ -207,7 +207,8 @@ func main() {
 	flGroupWrite := pflag.Bool("group-write",
 		envBool(false, "GITSYNC_GROUP_WRITE", "GIT_SYNC_GROUP_WRITE"),
 		"ensure that all data (repo, worktrees, etc.) is group writable")
-	flStaleWorktreeTimeout := pflag.Duration("stale-worktree-timeout", envDuration(0, "GITSYNC_STALE_WORKTREE_TIMEOUT"),
+	flStaleWorktreeTimeout := pflag.Duration("stale-worktree-timeout",
+		envDuration(0, "GITSYNC_STALE_WORKTREE_TIMEOUT"),
 		"how long to retain non-current worktrees")
 
 	flExechookCommand := pflag.String("exechook-command",
@@ -236,6 +237,13 @@ func main() {
 		envDuration(3*time.Second, "GITSYNC_WEBHOOK_BACKOFF", "GIT_SYNC_WEBHOOK_BACKOFF"),
 		"the time to wait before retrying a failed webhook")
 
+	flHooksAsync := pflag.Bool("hooks-async",
+		envBool(true, "GITSYNC_HOOKS_ASYNC", "GIT_SYNC_HOOKS_ASYNC"),
+		"run hooks asynchronously")
+	flHooksBeforeSymlink := pflag.Bool("hooks-before-symlink",
+		envBool(false, "GITSYNC_HOOKS_BEFORE_SYMLINK", "GIT_SYNC_HOOKS_BEFORE_SYMLINK"),
+		"run hooks before creating the symlink (defaults to false)")
+
 	flUsername := pflag.String("username",
 		envString("", "GITSYNC_USERNAME", "GIT_SYNC_USERNAME"),
 		"the username to use for git auth")
@@ -844,6 +852,7 @@ func main() {
 			hook.NewHookData(),
 			log,
 			*flOneTime,
+			*flHooksAsync,
 		)
 		go webhookRunner.Run(context.Background())
 	}
@@ -868,10 +877,30 @@ func main() {
 			hook.NewHookData(),
 			log,
 			*flOneTime,
+			*flHooksAsync,
 		)
 		go exechookRunner.Run(context.Background())
 	}
 
+	runHooks := func(hash string) error {
+		var err error
+		if exechookRunner != nil {
+			log.V(3).Info("sending exechook")
+			err = exechookRunner.Send(hash)
+			if err != nil {
+				return err
+			}
+		}
+		if webhookRunner != nil {
+			log.V(3).Info("sending webhook")
+			err = webhookRunner.Send(hash)
+		}
+		if err != nil {
+			return err
+		}
+		return nil
+	}
+
 	// Setup signal notify channel
 	sigChan := make(chan os.Signal, 1)
 	if syncSig != 0 {
@@ -912,11 +941,12 @@ func main() {
 
 	failCount := 0
 	syncCount := uint64(0)
+
 	for {
 		start := time.Now()
 		ctx, cancel := context.WithTimeout(context.Background(), *flSyncTimeout)
 
-		if changed, hash, err := git.SyncRepo(ctx, refreshCreds); err != nil {
+		if changed, hash, err := git.SyncRepo(ctx, refreshCreds, runHooks, *flHooksBeforeSymlink); err != nil {
 			failCount++
 			updateSyncMetrics(metricKeyError, start)
 			if *flMaxFailures >= 0 && failCount >= *flMaxFailures {
@@ -937,11 +967,10 @@ func main() {
 						log.V(3).Info("touched touch-file", "path", absTouchFile)
 					}
 				}
-				if webhookRunner != nil {
+				// if --hooks-before-symlink is set, these will have already been sent and completed.
-					webhookRunner.Send(hash)
+				// otherwise, we send them now.
-				}
+				if !*flHooksBeforeSymlink {
-				if exechookRunner != nil {
+					runHooks(hash)
-					exechookRunner.Send(hash)
 				}
 				updateSyncMetrics(metricKeySuccess, start)
 			} else {
@@ -961,14 +990,17 @@ func main() {
 				// Assumes that if hook channels are not nil, they will have at
 				// least one value before getting closed
 				exitCode := 0 // is 0 if all hooks succeed, else is 1
-				if exechookRunner != nil {
+				// This will not be needed if async == false, because the Send func for the hookRunners will wait
-					if err := exechookRunner.WaitForCompletion(); err != nil {
+				if *flHooksAsync {
-						exitCode = 1
+					if exechookRunner != nil {
+						if err := exechookRunner.WaitForCompletion(); err != nil {
+							exitCode = 1
+						}
 					}
-				}
+					if webhookRunner != nil {
-				if webhookRunner != nil {
+						if err := webhookRunner.WaitForCompletion(); err != nil {
-					if err := webhookRunner.WaitForCompletion(); err != nil {
+							exitCode = 1
-						exitCode = 1
+						}
 					}
 				}
 				log.DeleteErrorFile()
@@ -1716,7 +1748,7 @@ func (git *repoSync) currentWorktree() (worktree, error) {
 // SyncRepo syncs the repository to the desired ref, publishes it via the link,
 // and tries to clean up any detritus.  This function returns whether the
 // current hash has changed and what the new hash is.
-func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error) (bool, string, error) {
+func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Context) error, runHooks func(hash string) error, flHooksBeforeSymlink bool) (bool, string, error) {
 	git.log.V(3).Info("syncing", "repo", redactURL(git.repo))
 
 	if err := refreshCreds(ctx); err != nil {
@@ -1771,6 +1803,11 @@ func (git *repoSync) SyncRepo(ctx context.Context, refreshCreds func(context.Con
 	// path was different.
 	changed := (currentHash != remoteHash) || (currentWorktree != git.worktreeFor(currentHash))
 
+	// Fire hooks if needed.
+	if flHooksBeforeSymlink {
+		runHooks(remoteHash)
+	}
+
 	// We have to do at least one fetch, to ensure that parameters like depth
 	// are set properly.  This is cheap when we already have the target hash.
 	if changed || git.syncCount == 0 {
@@ -2537,6 +2574,13 @@ OPTIONS
     -?, -h, --help
             Print help text and exit.
 
+	--hooks-async, $GITSYNC_HOOKS_ASYNC
+			Whether to run the --exechook-command asynchronously.
+
+	--hooks-before-symlink, $GITSYNC_HOOKS_BEFORE_SYMLINK
+			Whether to run the --exechook-command before updating the symlink. Use in combination with --hooks-async set
+			to false if you need the hook to finish before the symlink is updated. 
+
     --http-bind <string>, $GITSYNC_HTTP_BIND
             The bind address (including port) for git-sync's HTTP endpoint.
             The '/' URL of this endpoint is suitable for Kubernetes startup and

pkg/hook/hook.go

@@ -89,10 +89,10 @@ func (d *hookData) send(newHash string) {
 }
 
 // NewHookRunner returns a new HookRunner.
-func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log logintf, oneTime bool) *HookRunner {
+func NewHookRunner(hook Hook, backoff time.Duration, data *hookData, log logintf, oneTime bool, async bool) *HookRunner {
-	hr := &HookRunner{hook: hook, backoff: backoff, data: data, log: log}
+	hr := &HookRunner{hook: hook, backoff: backoff, data: data, log: log, oneTime: oneTime, async: async}
-	if oneTime {
+	if oneTime || !async {
-		hr.oneTimeResult = make(chan bool, 1)
+		hr.result = make(chan bool, 1)
 	}
 	return hr
 }
@@ -107,9 +107,13 @@ type HookRunner struct {
 	data *hookData
 	// Logger
 	log logintf
-	// Used to send a status result when running in one-time mode.
+	// Used to send a status result when running in one-time or non-async mode.
 	// Should be initialised to a buffered channel of size 1.
-	oneTimeResult chan bool
+	result chan bool
+	// Bool for whether this is a one-time hook or not.
+	oneTime bool
+	// Bool for whether this is an async hook or not.
+	async bool
 }
 
 // Just the logr methods we need in this package.
@@ -120,8 +124,17 @@ type logintf interface {
 }
 
 // Send sends hash to hookdata.
-func (r *HookRunner) Send(hash string) {
+func (r *HookRunner) Send(hash string) error {
 	r.data.send(hash)
+	if !r.async {
+		r.log.V(1).Info("waiting for completion", "hash", hash, "name", r.hook.Name())
+		err := r.WaitForCompletion()
+		r.log.V(1).Info("hook completed", "hash", hash, "err", err, "name", r.hook.Name())
+		if err != nil {
+			return err
+		}
+	}
+	return nil
 }
 
 // Run waits for trigger events from the channel, and run hook when triggered.
@@ -144,45 +157,49 @@ func (r *HookRunner) Run(ctx context.Context) {
 				r.log.Error(err, "hook failed", "hash", hash, "retry", r.backoff)
 				updateHookRunCountMetric(r.hook.Name(), "error")
 				// don't want to sleep unnecessarily terminating anyways
-				r.sendOneTimeResultAndTerminate(false)
+				r.sendResult(false)
 				time.Sleep(r.backoff)
 			} else {
 				updateHookRunCountMetric(r.hook.Name(), "success")
 				lastHash = hash
-				r.sendOneTimeResultAndTerminate(true)
+				r.sendResult(true)
 				break
 			}
 		}
 	}
 }
 
-// If oneTimeResult is nil, does nothing. Otherwise, forwards the caller
+func (r *HookRunner) sendResult(completedSuccessfully bool) {
-// provided success status (as a boolean) of HookRunner to receivers of
+	// if onetime is true, we send the result then exit
-// oneTimeResult, closes said chanel, and terminates this goroutine.
+	if r.oneTime {
-// Using this function to write to oneTimeResult ensures it is only ever
+		r.result <- completedSuccessfully
-// written to once.
+		close(r.result)
-func (r *HookRunner) sendOneTimeResultAndTerminate(completedSuccessfully bool) {
-	if r.oneTimeResult != nil {
-		r.oneTimeResult <- completedSuccessfully
-		close(r.oneTimeResult)
 		runtime.Goexit()
+	} else if !r.async {
+		// if onetime is false, and we've set non-async we send but don't exit.
+		r.result <- completedSuccessfully
 	}
+	// if neither oneTime nor !async, we do nothing here.
 }
 
 // WaitForCompletion waits for HookRunner to send completion message to
 // calling thread and returns either true if HookRunner executed successfully
 // and some error otherwise.
-// Assumes that r.oneTimeResult is not nil, but if it is, returns an error.
+// Assumes that either r.oneTime or !r.async, otherwise returns an error.
 func (r *HookRunner) WaitForCompletion() error {
 	// Make sure function should be called
-	if r.oneTimeResult == nil {
+	if r.result == nil {
 		return fmt.Errorf("HookRunner.WaitForCompletion called on async runner")
 	}
 
-	// Perform wait on HookRunner
+	// If oneTimeResult is not nil, we wait for its result.
-	hookRunnerFinishedSuccessfully := <-r.oneTimeResult
+	if r.result != nil {
-	if !hookRunnerFinishedSuccessfully {
+		hookRunnerFinishedSuccessfully := <-r.result
-		return fmt.Errorf("hook completed with error")
+		r.log.V(1).Info("hook completed", "success", hookRunnerFinishedSuccessfully,
+			"oneTime", r.oneTime, "async", r.async, "name", r.hook.Name())
+		if !hookRunnerFinishedSuccessfully {
+			return fmt.Errorf("hook completed with error")
+		}
 	}
 
 	return nil

test_e2e.sh

@@ -96,7 +96,7 @@ function assert_file_eq() {
     if [[ $(cat "$1") == "$2" ]]; then
         return
     fi
-    fail "$1 does not contain '$2': $(cat "$1")"
+    fail "$1 does not equal '$2': $(cat "$1")"
 }
 
 function assert_file_contains() {
@@ -2406,7 +2406,7 @@ function e2e::exechook_fail_retry() {
         --exechook-command="/$EXECHOOK_COMMAND_FAIL" \
         --exechook-backoff=1s \
         &
-    sleep 3 # give it time to retry
+    sleep 4 # give it time to retry
 
     # Check that exechook was called
     assert_file_lines_ge "$RUNLOG" 2
@@ -2486,6 +2486,55 @@ function e2e::exechook_startup_after_crash() {
     assert_file_lines_eq "$RUNLOG" 1
 }
 
+##############################################
+# Test exechook-success with --hooks-async=false
+##############################################
+function e2e::exechook_success_hooks_non_async() {
+    cat /dev/null > "$RUNLOG"
+
+    GIT_SYNC \
+        --hooks-async=false \
+        --period=100ms \
+        --repo="file://$REPO" \
+        --root="$ROOT" \
+        --link="link" \
+        --exechook-command="/$EXECHOOK_COMMAND_SLEEPY" \
+        &
+    sleep 5 # give it time to run
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_exists "$ROOT/link/exechook"
+    assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook-env" "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL"
+}
+
+##############################################
+# Test exechook-success with --hooks-async=false and --hooks-before-symlink
+##############################################
+function e2e::exechook_success_hooks_before_symlink_non_async() {
+    cat /dev/null > "$RUNLOG"
+
+    GIT_SYNC \
+        --hooks-async=false \
+        --hooks-before-symlink \
+        --period=100ms \
+        --repo="file://$REPO" \
+        --root="$ROOT" \
+        --link="link" \
+        --exechook-command="/$EXECHOOK_COMMAND_SLEEPY" \
+        --exechook-backoff=1s \
+        &
+    sleep 5 # give it time to run
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_exists "$ROOT/link/exechook"
+    assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook" "${FUNCNAME[0]}"
+    assert_file_eq "$ROOT/link/exechook-env" "$EXECHOOK_ENVKEY=$EXECHOOK_ENVVAL"
+    assert_file_absent "$ROOT/link/delaycheck"
+}
+
 ##############################################
 # Test webhook success
 ##############################################