Change time-related flags to durations

Add '--period' to replace '--wait', which is now obsolete. Add '--sync-timeout' to replace '--timeout', which is now obsolete. Both of these new flags take a Go-style time string, rather than a bare number. For example "1s" for 1 second or "1m" for one minute. The old flags have been kept and will take precedence if specified.
kubernetes · Oct 31, 2020 · 7e9a07d · 7e9a07d
1 parent 2069668
commit 7e9a07d
Show file tree

Hide file tree

Showing 4 changed files with 72 additions and 55 deletions.
diff --git a/README.md b/README.md
@@ -48,7 +48,7 @@ docker run -d \
     registry/git-sync:tag \
         --repo=https://github.com/kubernetes/git-sync \
         --branch=master \
-        --wait=30
+        --period=30s
 
 # run an nginx container to serve the content
 docker run -d \
@@ -74,7 +74,7 @@ docker run -d \
     registry/git-sync:tag \
         --repo=https://github.com/kubernetes/git-sync \
         --branch=master \
-        --wait=30 \
+        --period=30s \
         --webhook-url="http://localhost:9090/-/reload"
 ```
 
@@ -89,8 +89,8 @@ docker run -d \
 | GIT_SYNC_SUBMODULES             | `--submodules`             | git submodule behavior: one of 'recursive', 'shallow', or 'off'                                                                                                                                                                               | recursive                     |
 | GIT_SYNC_ROOT                   | `--root`                   | the root directory for git-sync operations, under which --dest will be created                                                                                                                                                                | "$HOME/git"                   |
 | GIT_SYNC_DEST                   | `--dest`                   | the name of (a symlink to) a directory in which to check-out files under --root (defaults to the leaf dir of --repo)                                                                                                                          | ""                            |
-| GIT_SYNC_WAIT                   | `--wait`                   | the number of seconds between syncs                                                                                                                                                                                                           | 1 (second)                    |
-| GIT_SYNC_TIMEOUT                | `--timeout`                | the max number of seconds allowed for a complete sync                                                                                                                                                                                         | 120                           |
+| GIT_SYNC_PERIOD                 | `--period`                 | how long to wait between syncs, must be >= 10ms                                                                                                                                                                                               | "10s"                         |
+| GIT_SYNC_SYNC_TIMEOUT           | `--sync-timeout`           | the total time allowed for one complete sync, must be >= 10ms                                                                                                                                                                                 | "120s"                        |
 | GIT_SYNC_ONE_TIME               | `--one-time`               | exit after the first sync                                                                                                                                                                                                                     | false                         |
 | GIT_SYNC_MAX_SYNC_FAILURES      | `--max-sync-failures`      | the number of consecutive failures allowed before aborting (the first sync must succeed, -1 will retry forever after the initial sync)                                                                                                        | 0                             |
 | GIT_SYNC_PERMISSIONS            | `--change-permissions`     | the file permissions to apply to the checked-out files (0 will not change permissions at all)                                                                                                                                                 | 0                             |

diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go
@@ -66,10 +66,10 @@ var flRoot = pflag.String("root", envString("GIT_SYNC_ROOT", envString("HOME", "
 	"the root directory for git-sync operations, under which --dest will be created")
 var flDest = pflag.String("dest", envString("GIT_SYNC_DEST", ""),
 	"the name of (a symlink to) a directory in which to check-out files under --root (defaults to the leaf dir of --repo)")
-var flWait = pflag.Float64("wait", envFloat("GIT_SYNC_WAIT", 1),
-	"the number of seconds between syncs")
-var flSyncTimeout = pflag.Int("timeout", envInt("GIT_SYNC_TIMEOUT", 120),
-	"the number of seconds allowed for a complete sync")
+var flPeriod = pflag.Duration("period", envDuration("GIT_SYNC_PERIOD", 10*time.Second),
+	"how long to wait between syncs, must be >= 10ms; --wait overrides this")
+var flSyncTimeout = pflag.Duration("sync-timeout", envDuration("GIT_SYNC_SYNC_TIMEOUT", 120*time.Second),
+	"the total time allowed for one complete sync, must be >= 10ms; --timeout overrides this")
 var flOneTime = pflag.Bool("one-time", envBool("GIT_SYNC_ONE_TIME", false),
 	"exit after the first sync")
 var flMaxSyncFailures = pflag.Int("max-sync-failures", envInt("GIT_SYNC_MAX_SYNC_FAILURES", 0),
@@ -123,6 +123,17 @@ var flHTTPMetrics = pflag.Bool("http-metrics", envBool("GIT_SYNC_HTTP_METRICS",
 var flHTTPprof = pflag.Bool("http-pprof", envBool("GIT_SYNC_HTTP_PPROF", false),
 	"enable the pprof debug endpoints on git-sync's HTTP endpoint")
 
+// Obsolete flags, kept for compat.
+var flWait = pflag.Float64("wait", envFloat("GIT_SYNC_WAIT", 0),
+	"DEPRECATED: use --period instead")
+var flTimeout = pflag.Int("timeout", envInt("GIT_SYNC_TIMEOUT", 0),
+	"DEPRECATED: use --sync-timeout instead")
+
+func init() {
+	pflag.CommandLine.MarkDeprecated("wait", "use --period instead")
+	pflag.CommandLine.MarkDeprecated("timeout", "use --sync-timeout instead")
+}
+
 var log = glogr.New()
 
 // Total pull/error, summary on pull duration
@@ -150,9 +161,6 @@ const (
 	metricKeyNoOp    = "noop"
 )
 
-// initTimeout is a timeout for initialization, like git credentials setup.
-const initTimeout = time.Second * 30
-
 const (
 	submodulesRecursive = "recursive"
 	submodulesShallow   = "shallow"
@@ -308,14 +316,20 @@ func main() {
 		os.Exit(1)
 	}
 
-	if *flWait < 0 {
-		fmt.Fprintf(os.Stderr, "ERROR: --wait must be greater than or equal to 0\n")
+	if *flWait != 0 {
+		*flPeriod = time.Duration(int(*flWait*1000)) * time.Millisecond
+	}
+	if *flPeriod < 10*time.Millisecond {
+		fmt.Fprintf(os.Stderr, "ERROR: --period must be at least 10ms\n")
 		pflag.Usage()
 		os.Exit(1)
 	}
 
-	if *flSyncTimeout < 0 {
-		fmt.Fprintf(os.Stderr, "ERROR: --timeout must be greater than 0\n")
+	if *flTimeout != 0 {
+		*flSyncTimeout = time.Duration(*flTimeout) * time.Second
+	}
+	if *flSyncTimeout < 10*time.Millisecond {
+		fmt.Fprintf(os.Stderr, "ERROR: --sync-timeout must be at least 10ms\n")
 		pflag.Usage()
 		os.Exit(1)
 	}
@@ -382,8 +396,8 @@ func main() {
 	}
 
 	// This context is used only for git credentials initialization. There are no long-running operations like
-	// `git clone`, so initTimeout set to 30 seconds should be enough.
-	ctx, cancel := context.WithTimeout(context.Background(), initTimeout)
+	// `git clone`, so hopefully 30 seconds will be enough.
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
 
 	if *flUsername != "" && *flPassword != "" {
 		if err := setupGitAuth(ctx, *flUsername, *flPassword, *flRepo); err != nil {
@@ -471,7 +485,7 @@ func main() {
 	failCount := 0
 	for {
 		start := time.Now()
-		ctx, cancel := context.WithTimeout(context.Background(), time.Second*time.Duration(*flSyncTimeout))
+		ctx, cancel := context.WithTimeout(context.Background(), *flSyncTimeout)
 		if changed, hash, err := syncRepo(ctx, *flRepo, *flBranch, *flRev, *flDepth, *flRoot, *flDest, *flAskPassURL, *flSubmodules); err != nil {
 			updateSyncMetrics(metricKeyError, start)
 			if *flMaxSyncFailures != -1 && failCount >= *flMaxSyncFailures {
@@ -482,9 +496,9 @@ func main() {
 
 			failCount++
 			log.Error(err, "unexpected error syncing repo, will retry")
-			log.V(0).Info("waiting before retrying", "waitTime", waitTime(*flWait))
+			log.V(0).Info("waiting before retrying", "waitTime", flPeriod.String())
 			cancel()
-			time.Sleep(waitTime(*flWait))
+			time.Sleep(*flPeriod)
 			continue
 		} else if changed {
 			if webhook != nil {
@@ -510,9 +524,9 @@ func main() {
 		}
 
 		failCount = 0
-		log.V(1).Info("next sync", "wait_time", waitTime(*flWait))
+		log.V(1).Info("next sync", "waitTime", flPeriod.String())
 		cancel()
-		time.Sleep(waitTime(*flWait))
+		time.Sleep(*flPeriod)
 	}
 }
 
@@ -521,10 +535,6 @@ func updateSyncMetrics(key string, start time.Time) {
 	syncCount.WithLabelValues(key).Inc()
 }
 
-func waitTime(seconds float64) time.Duration {
-	return time.Duration(int(seconds*1000)) * time.Millisecond
-}
-
 // Do no work, but don't do something that triggers go's runtime into thinking
 // it is deadlocked.
 func sleepForever() {
@@ -1113,6 +1123,11 @@ OPTIONS
             users should prefer the environment variable for specifying the
             password.
 
+    --period <duration>, $GIT_SYNC_PERIOD
+            How long to wait between sync attempts.  This must be at least
+            10ms.  This flag obsoletes --wait, but if --wait is specifed, it
+            will take precedence. (default: 10s)
+
     --repo <string>, $GIT_SYNC_REPO
             The git repository to sync.
 
@@ -1145,11 +1160,13 @@ OPTIONS
             An optional command to be executed after syncing a new hash of the
             remote repository.  This command does not take any arguments and
             executes with the synced repo as its working directory.  The
-            execution is subject to the overall --timeout flag and will extend
-            the period between syncs attempts.
+            execution is subject to the overall --sync-timeout flag and will
+            extend the effective period between syncs attempts.
 
-    --timeout <int>, $GIT_SYNC_TIMEOUT
-            The number of seconds allowed for a complete sync. (default: 120)
+    --sync-timeout <duration>, $GIT_SYNC_SYNC_TIMEOUT
+            The total time allowed for one complete sync.  This must be at least
+            10ms.  This flag obsoletes --timeout, but if --timeout is specified,
+            it will take precedence. (default: 120s)
 
     --username <string>, $GIT_SYNC_USERNAME
             The username to use for git authentication (see --password).
@@ -1161,9 +1178,6 @@ OPTIONS
     --version
             Print the version and exit.
 
-    --wait <float>, $GIT_SYNC_WAIT
-            The number of seconds between sync attempts. (default: 1)
-
     --webhook-backoff <duration>, $GIT_SYNC_WEBHOOK_BACKOFF
             The time to wait before retrying a failed --webhook-url).
             (default: 3s)
@@ -1188,7 +1202,7 @@ EXAMPLE USAGE
         --repo=https://github.com/kubernetes/git-sync \
         --branch=master \
         --rev=HEAD \
-        --wait=10 \
+        --period=10s \
         --root=/mnt/git
 
 AUTHENTICATION

diff --git a/cmd/git-sync/webhook.go b/cmd/git-sync/webhook.go
@@ -81,7 +81,7 @@ func (w *Webhook) Do(hash string) error {
 	defer cancel()
 	req = req.WithContext(ctx)
 
-	log.V(0).Info("sending webhook", "hash", hash, "url", w.URL, "method", w.Method, "timeout", w.Timeout)
+	log.V(0).Info("sending webhook", "hash", hash, "url", w.URL, "method", w.Method, "timeout", w.Timeout.String())
 	resp, err := http.DefaultClient.Do(req)
 	if err != nil {
 		return err
@@ -113,7 +113,7 @@ func (w *Webhook) run() {
 			}
 
 			if err := w.Do(hash); err != nil {
-				log.Error(err, "webhook failed", "url", w.URL, "method", w.Method, "timeout", w.Timeout)
+				log.Error(err, "webhook failed", "url", w.URL, "method", w.Method, "timeout", w.Timeout.String())
 				time.Sleep(w.Backoff)
 			} else {
 				lastHash = hash

diff --git a/test_e2e.sh b/test_e2e.sh
@@ -172,7 +172,7 @@ testcase "default-sync"
 echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
@@ -205,7 +205,7 @@ testcase "head-sync"
 echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --branch=master \
     --rev=HEAD \
@@ -243,7 +243,7 @@ echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 git -C "$REPO" checkout -q master
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --branch="$BRANCH" \
     --root="$ROOT" \
@@ -283,7 +283,7 @@ echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 git -C "$REPO" tag -f "$TAG" >/dev/null
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --rev="$TAG" \
     --root="$ROOT" \
@@ -328,7 +328,7 @@ echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 git -C "$REPO" tag -af "$TAG" -m "$TESTCASE 1" >/dev/null
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --rev="$TAG" \
     --root="$ROOT" \
@@ -372,7 +372,7 @@ echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 REV=$(git -C "$REPO" rev-list -n1 HEAD)
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --rev="$REV" \
     --root="$ROOT" \
@@ -460,7 +460,7 @@ git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
     --git="$SLOW_GIT" \
     --one-time \
-    --timeout=1 \
+    --sync-timeout=1s \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
@@ -470,8 +470,8 @@ assert_file_absent "$ROOT"/link/file
 # run with slow_git but without timing out
 GIT_SYNC \
     --git="$SLOW_GIT" \
-    --wait=0.1 \
-    --timeout=16 \
+    --period=100ms \
+    --sync-timeout=16s \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
@@ -499,7 +499,7 @@ echo "$TESTCASE 1" > "$REPO"/file
 expected_depth="1"
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --depth="$expected_depth" \
     --root="$ROOT" \
@@ -634,7 +634,7 @@ testcase "sync_hook_command"
 echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
@@ -667,6 +667,7 @@ freencport
 echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --webhook-url="http://127.0.0.1:$NCPORT" \
@@ -709,6 +710,7 @@ freencport
 echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --webhook-url="http://127.0.0.1:$NCPORT" \
@@ -735,6 +737,7 @@ echo "$TESTCASE 1" > "$REPO"/file
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
     --git="$SLOW_GIT" \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --http-bind=":$BINDPORT" \
@@ -795,7 +798,7 @@ git -C "$NESTED_SUBMODULE" commit -aqm "init nested-submodule file"
 git -C "$REPO" submodule add -q file://$SUBMODULE
 git -C "$REPO" commit -aqm "add submodule"
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
@@ -882,7 +885,7 @@ git -C "$REPO" submodule add -q file://$SUBMODULE
 git -C "$REPO" config -f "$REPO"/.gitmodules submodule.$SUBMODULE_REPO_NAME.shallow true
 git -C "$REPO" commit -qam "$TESTCASE 1"
 GIT_SYNC \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --depth="$expected_depth" \
     --root="$ROOT" \
@@ -957,11 +960,11 @@ git -C "$REPO" submodule add -q file://$SUBMODULE
 git -C "$REPO" commit -aqm "add submodule"
 
 GIT_SYNC \
-    --submodules=off \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
+    --submodules=off \
     > "$DIR"/log."$TESTCASE" 2>&1 &
 sleep 3
 assert_file_absent "$ROOT"/link/$SUBMODULE_REPO_NAME/submodule
@@ -999,11 +1002,11 @@ git -C "$REPO" submodule add -q file://$SUBMODULE
 git -C "$REPO" commit -aqm "add submodule"
 
 GIT_SYNC \
-    --submodules=shallow \
-    --wait=0.1 \
+    --period=100ms \
     --repo="file://$REPO" \
     --root="$ROOT" \
     --dest="link" \
+    --submodules=shallow \
     > "$DIR"/log."$TESTCASE" 2>&1 &
 sleep 3
 assert_link_exists "$ROOT"/link
@@ -1032,13 +1035,13 @@ IP=$(docker inspect "$CTR" | jq -r .[0].NetworkSettings.IPAddress)
 git -C "$REPO" commit -qam "$TESTCASE"
 GIT_SYNC \
     --one-time \
-    --ssh \
-    --ssh-known-hosts=false \
     --repo="test@$IP:/src" \
     --branch=master \
     --rev=HEAD \
     --root="$ROOT" \
     --dest="link" \
+    --ssh \
+    --ssh-known-hosts=false \
     > "$DIR"/log."$TESTCASE" 2>&1
 assert_link_exists "$ROOT"/link
 assert_file_exists "$ROOT"/link/file