[viper WatchConfig] platform-specific write to ensure callback fires …

…exactly once (#13627)

Signed-off-by: Andrew Mason <[email protected]>

go/viperutil/internal/sync/sync_darwin_test.go

@@ -0,0 +1,34 @@
+/*
+Copyright 2023 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package sync_test
+
+import "os"
+
+// atomicWrite overwrites a file in such a way as to produce exactly one
+// filesystem event of the type CREATE or WRITE (which are tracked by viper)
+// without producing any REMOVE events.
+//
+// At time of writing, this produces the following on darwin:
+// CHMOD => WRITE => CHMOD.
+func atomicWrite(path string, data []byte) error {
+	stat, err := os.Stat(path)
+	if err != nil {
+		return err
+	}
+
+	return os.WriteFile(path, data, stat.Mode())
+}

go/viperutil/internal/sync/sync_linux_test.go

@@ -0,0 +1,39 @@
+/*
+Copyright 2023 The Vitess Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+	http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package sync_test
+
+import "os"
+
+// atomicWrite overwrites a file in such a way as to produce exactly one
+// filesystem event of the type CREATE or WRITE (which are tracked by viper)
+// without producing any REMOVE events.
+//
+// At time of writing, this produces the following on x86_64 linux:
+// CREATE.
+func atomicWrite(path string, data []byte) error {
+	stat, err := os.Stat(path)
+	if err != nil {
+		return err
+	}
+
+	tmp := path + ".tmp"
+	if err := os.WriteFile(tmp, data, stat.Mode()); err != nil {
+		return err
+	}
+
+	return os.Rename(tmp, path)
+}

go/viperutil/internal/sync/sync_test.go

@@ -19,16 +19,12 @@ package sync_test
 import (
 	"context"
 	"encoding/json"
-	"fmt"
-	"math"
 	"math/rand"
 	"os"
 	"sync"
 	"testing"
 	"time"
 
-	"vitess.io/vitess/go/vt/log"
-
 	"github.com/fsnotify/fsnotify"
 	"github.com/spf13/viper"
 	"github.com/stretchr/testify/require"
@@ -44,36 +40,17 @@ func TestWatchConfig(t *testing.T) {
 	}
 
 	writeConfig := func(tmp *os.File, a, b int) error {
-		stat, err := os.Stat(tmp.Name())
-		if err != nil {
-			return err
-		}
-
 		data, err := json.Marshal(&config{A: a, B: b})
 		if err != nil {
 			return err
 		}
 
-		err = os.WriteFile(tmp.Name(), data, stat.Mode())
-		if err != nil {
-			return err
-		}
-
-		data, err = os.ReadFile(tmp.Name())
-		if err != nil {
-			return err
-		}
-
-		var cfg config
-		if err := json.Unmarshal(data, &cfg); err != nil {
-			return err
-		}
-
-		if cfg.A != a || cfg.B != b {
-			return fmt.Errorf("config did not persist; want %+v got %+v", config{A: a, B: b}, cfg)
-		}
-
-		return nil
+		// In order to guarantee viper's watcher detects exactly one config
+		// change, we perform a write specific to the platform we're executing
+		// on.
+		//
+		// Consequently, this test only supports linux and macos for now.
+		return atomicWrite(tmp.Name(), data)
 	}
 	writeRandomConfig := func(tmp *os.File) error {
 		a, b := rand.Intn(100), rand.Intn(100)
@@ -109,19 +86,8 @@ func TestWatchConfig(t *testing.T) {
 	require.NoError(t, writeConfig(tmp, a+1, b+1))
 	<-wCh // wait for the update to finish
 
-	// temporary hack to fix flakiness where we seem to miss one update.
-	const permittedVariance = 1
-	closeEnoughTo := func(want, got int) bool {
-		if math.Abs(float64(want-got)) <= permittedVariance {
-			return true
-		}
-		log.Infof("TestWatchConfig: count not close enough: want %d, got %d, permitted variance %d",
-			want, got, permittedVariance)
-		return false
-	}
-
-	require.True(t, closeEnoughTo(a+1, v.GetInt("a")))
-	require.True(t, closeEnoughTo(b+1, v.GetInt("b")))
+	require.Equal(t, a+1, v.GetInt("a"))
+	require.Equal(t, b+1, v.GetInt("b"))
 
 	rCh <- struct{}{}