config: replace fsnotify with reloading config file periodically

go.mod

@@ -6,7 +6,6 @@ require (
 	github.com/BurntSushi/toml v1.2.1
 	github.com/bahlo/generic-list-go v0.2.0
 	github.com/cenkalti/backoff/v4 v4.2.1
-	github.com/fsnotify/fsnotify v1.6.0
 	github.com/gin-contrib/pprof v1.4.0
 	github.com/gin-gonic/gin v1.8.1
 	github.com/go-mysql-org/go-mysql v1.6.0

go.sum

@@ -214,8 +214,6 @@ github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga
 github.com/flosch/pongo2 v0.0.0-20190707114632-bbf5a6c351f4/go.mod h1:T9YF2M40nIgbVgp3rreNmTged+9HrbNTIQf1PsaIiTA=
 github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
 github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ=
-github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY=
-github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw=
 github.com/gavv/httpexpect v2.0.0+incompatible/go.mod h1:x+9tiU1YnrOvnB725RkpoLv1M62hOWzwo5OXotisrKc=
 github.com/getsentry/raven-go v0.2.0/go.mod h1:KungGk8q33+aIAZUIVWZDr2OfAEBsO49PX4NzFV5kcQ=
 github.com/ghodss/yaml v1.0.0/go.mod h1:4dBDuWmgqj2HViK6kFavaiC9ZROes6MMH2rRYeMEF04=
@@ -1041,7 +1039,6 @@ golang.org/x/sys v0.0.0-20210816074244-15123e1e1f71/go.mod h1:oPkhp1MJrh7nUepCBc
 golang.org/x/sys v0.0.0-20211013075003-97ac67df715c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
-golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

pkg/manager/config/config.go

@@ -7,49 +7,24 @@ import (
 	"bytes"
 	"hash/crc32"
 	"os"
-	"time"
 
 	"github.com/BurntSushi/toml"
-	"github.com/fsnotify/fsnotify"
 	"github.com/pingcap/tiproxy/lib/config"
 	"github.com/pingcap/tiproxy/lib/util/errors"
 	"go.uber.org/zap"
 )
 
 func (e *ConfigManager) reloadConfigFile(file string) error {
-	proxyConfigData, err := os.ReadFile(file)
+	content, err := os.ReadFile(file)
 	if err != nil {
 		return errors.WithStack(err)
 	}
-
-	return e.SetTOMLConfig(proxyConfigData)
-}
-
-func (e *ConfigManager) handleFSEvent(ev fsnotify.Event, f string) {
-	switch {
-	case ev.Has(fsnotify.Create), ev.Has(fsnotify.Write), ev.Has(fsnotify.Remove), ev.Has(fsnotify.Rename):
-		// The file may be the log file, triggering reload will cause more logs and thus cause reload again,
-		// so we need to filter the wrong files.
-		// The filesystem differs from OS to OS, so don't use string comparison.
-		f1, err := os.Stat(ev.Name)
-		if err != nil {
-			break
-		}
-		f2, err := os.Stat(f)
-		if err != nil {
-			break
-		}
-		if !os.SameFile(f1, f2) {
-			break
-		}
-		if ev.Has(fsnotify.Remove) || ev.Has(fsnotify.Rename) {
-			// in case of remove/rename the file, files are not present at filesystem for a while
-			// it may be too fast to read the config file now, sleep for a while
-			time.Sleep(50 * time.Millisecond)
-		}
-		// try to reload it
-		e.logger.Info("config file reloaded", zap.Stringer("event", ev), zap.Error(e.reloadConfigFile(f)))
+	if bytes.Equal(content, e.fileContent) {
+		return nil
 	}
+	e.fileContent = content
+
+	return e.SetTOMLConfig(content)
 }
 
 // SetTOMLConfig will do partial config update. Usually, user will expect config changes

pkg/manager/config/config_test.go

@@ -7,8 +7,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"runtime"
-	"strings"
 	"testing"
 	"time"
 
@@ -156,158 +154,146 @@ func TestConfigRemove(t *testing.T) {
 }
 
 func TestFilePath(t *testing.T) {
-	var (
-		cfgmgr *ConfigManager
-		text   fmt.Stringer
-		count  int
-	)
+	for i := 0; i < 10; i++ {
+		var cfgmgr *ConfigManager
+		tmpdir := t.TempDir()
+		pdAddr1, pdAddr2, pdAddr3 := "127.0.0.1:1000", "127.0.0.1:2000", "127.0.0.1:3000"
 
-	const (
-		noReload int = iota
-		reloadWrite
-		reloadCreate
-	)
-
-	tmpdir := t.TempDir()
-	checkLog := func(reloadType int) {
-		if reloadType != noReload {
-			expectedCount := count + 1
-			// On linux, writing once will trigger 2 WRITE events. But on macOS, it only triggers once.
-			if reloadType == reloadWrite && runtime.GOOS == "linux" {
-				expectedCount++
-			}
-			var newCount int
-			require.Eventually(t, func() bool {
-				newCount = strings.Count(text.String(), "config file reloaded")
-				return newCount == expectedCount
-			}, 3*time.Second, 10*time.Millisecond, "count=%d, expected=%d, newCount=%d", count, expectedCount, newCount)
-			count = newCount
-		} else {
-			time.Sleep(100 * time.Millisecond)
-			newCount := strings.Count(text.String(), "config file reloaded")
-			require.Equal(t, count, newCount)
-		}
-	}
-
-	tests := []struct {
-		filename   string
-		createFile func()
-		cleanFile  func()
-		checker    func(filename string)
-	}{
-		{
-			// Test updating another file in the same directory won't make it reload.
-			filename: filepath.Join(tmpdir, "cfg"),
-			checker: func(filename string) {
-				tmplog := filepath.Join(tmpdir, "log")
-				f, err := os.Create(tmplog)
-				require.NoError(t, err)
-				require.NoError(t, f.Close())
-				require.NoError(t, os.WriteFile(tmplog, []byte("hello"), 0644))
-				newlog := filepath.Join(tmpdir, "log1")
-				require.NoError(t, os.Rename(tmplog, newlog))
-				require.NoError(t, os.Remove(newlog))
-				checkLog(noReload)
+		tests := []struct {
+			filename   string
+			createFile func()
+			cleanFile  func()
+			checker    func(filename string)
+		}{
+			{
+				// Test updating another file in the same directory.
+				filename: filepath.Join(tmpdir, "cfg"),
+				checker: func(filename string) {
+					tmplog := filepath.Join(tmpdir, "log")
+					f, err := os.Create(tmplog)
+					require.NoError(t, err)
+					require.NoError(t, f.Close())
+					require.NoError(t, os.WriteFile(tmplog, []byte("hello"), 0644))
+					newlog := filepath.Join(tmpdir, "log1")
+					require.NoError(t, os.Rename(tmplog, newlog))
+					require.NoError(t, os.Remove(newlog))
+					require.Equal(t, pdAddr2, cfgmgr.GetConfig().Proxy.PDAddrs)
+				},
 			},
-		},
-		{
-			// Test case-insensitive.
-			filename: filepath.Join(tmpdir, "cfg"),
-			createFile: func() {
-				f, err := os.Create(filepath.Join(tmpdir, "CFG"))
-				require.NoError(t, err)
-				require.NoError(t, f.Close())
-				// Linux is case-sensitive but macOS is case-insensitive.
-				// For linux, it creates another file. For macOS, it doesn't touch the file.
-				f, err = os.Create(filepath.Join(tmpdir, "cfg"))
-				require.NoError(t, err)
-				require.NoError(t, f.Close())
+			{
+				// Test case-insensitive.
+				filename: filepath.Join(tmpdir, "cfg"),
+				createFile: func() {
+					f, err := os.Create(filepath.Join(tmpdir, "CFG"))
+					require.NoError(t, err)
+					require.NoError(t, f.Close())
+					// Linux is case-sensitive but macOS is case-insensitive.
+					// For linux, it creates another file. For macOS, it doesn't touch the file.
+					f, err = os.Create(filepath.Join(tmpdir, "cfg"))
+					require.NoError(t, err)
+					_, err = f.WriteString(fmt.Sprintf("proxy.pd-addrs = \"%s\"", pdAddr1))
+					require.NoError(t, err)
+					require.NoError(t, f.Close())
+				},
 			},
-		},
-		{
-			// Test relative path.
-			// `event.Name` is `cfg` on MacOS, but it's `./cfg` on Linux.
-			filename: "cfg",
-		},
-		{
-			// Test relative path.
-			filename: "./cfg",
-		},
-		{
-			// Test uncleaned path.
-			filename: fmt.Sprintf("%s%c%ccfg", tmpdir, filepath.Separator, filepath.Separator),
-		},
-		{
-			// Test removing and recreating the directory.
-			filename: "_tmp/cfg",
-			createFile: func() {
-				if err := os.Mkdir("_tmp", 0755); err != nil {
-					require.ErrorIs(t, err, os.ErrExist)
-				}
-				f, err := os.Create("_tmp/cfg")
-				require.NoError(t, err)
-				require.NoError(t, f.Close())
+			{
+				// Test relative path.
+				// `event.Name` is `cfg` on MacOS, but it's `./cfg` on Linux.
+				filename: "cfg",
 			},
-			cleanFile: func() {
-				require.NoError(t, os.RemoveAll("_tmp"))
+			{
+				// Test relative path.
+				filename: "./cfg",
 			},
-			checker: func(filename string) {
-				require.NoError(t, os.RemoveAll("_tmp"))
-				// To update `count`.
-				checkLog(noReload)
-
-				require.NoError(t, os.Mkdir("_tmp", 0755))
-				f, err := os.Create("_tmp/cfg")
-				require.NoError(t, err)
-				require.NoError(t, f.Close())
-				checkLog(reloadCreate)
+			{
+				// Test uncleaned path.
+				filename: fmt.Sprintf("%s%c%ccfg", tmpdir, filepath.Separator, filepath.Separator),
 			},
-		},
-		{
-			// Test removing and recreating the file.
-			filename: "cfg",
-			checker: func(filename string) {
-				require.NoError(t, os.Remove(filename))
-				checkLog(noReload)
+			{
+				// Test removing and recreating the directory.
+				filename: "_tmp/cfg",
+				createFile: func() {
+					if err := os.Mkdir("_tmp", 0755); err != nil {
+						require.ErrorIs(t, err, os.ErrExist)
+					}
+					f, err := os.Create("_tmp/cfg")
+					require.NoError(t, err)
+					_, err = f.WriteString(fmt.Sprintf("proxy.pd-addrs = \"%s\"", pdAddr1))
+					require.NoError(t, err)
+					require.NoError(t, f.Close())
+				},
+				cleanFile: func() {
+					require.NoError(t, os.RemoveAll("_tmp"))
+				},
+				checker: func(filename string) {
+					require.NoError(t, os.RemoveAll("_tmp"))
+					t.Log("remove _tmp")
 
-				f, err := os.Create(filename)
-				require.NoError(t, err)
-				require.NoError(t, f.Close())
-				checkLog(reloadCreate)
+					require.NoError(t, os.Mkdir("_tmp", 0755))
+					f, err := os.Create("_tmp/cfg")
+					require.NoError(t, err)
+					_, err = f.WriteString(fmt.Sprintf("proxy.pd-addrs = \"%s\"", pdAddr3))
+					require.NoError(t, err)
+					require.NoError(t, f.Close())
+					t.Log("write _tmp")
+					require.Eventually(t, func() bool {
+						return pdAddr3 == cfgmgr.GetConfig().Proxy.PDAddrs
+					}, 3*time.Second, 10*time.Millisecond, cfgmgr.GetConfig().Proxy.PDAddrs)
+				},
 			},
-		},
-	}
+			{
+				// Test removing and recreating the file.
+				filename: "cfg",
+				checker: func(filename string) {
+					require.NoError(t, os.Remove(filename))
 
-	for _, test := range tests {
-		if test.createFile != nil {
-			test.createFile()
-		} else {
-			f, err := os.Create(test.filename)
-			require.NoError(t, err)
-			require.NoError(t, f.Close())
+					f, err := os.Create(filename)
+					require.NoError(t, err)
+					_, err = f.WriteString(fmt.Sprintf("proxy.pd-addrs = \"%s\"", pdAddr3))
+					require.NoError(t, err)
+					require.NoError(t, f.Close())
+					require.Eventually(t, func() bool {
+						return pdAddr3 == cfgmgr.GetConfig().Proxy.PDAddrs
+					}, 3*time.Second, 10*time.Millisecond, cfgmgr.GetConfig().Proxy.PDAddrs)
+				},
+			},
 		}
 
-		count = 0
-		cfgmgr, text, _ = testConfigManager(t, test.filename)
-		checkLog(noReload)
+		for i, test := range tests {
+			t.Logf("%dth test", i+1)
+			if test.createFile != nil {
+				test.createFile()
+			} else {
+				f, err := os.Create(test.filename)
+				require.NoError(t, err)
+				_, err = f.WriteString(fmt.Sprintf("proxy.pd-addrs = \"%s\"", pdAddr1))
+				require.NoError(t, err)
+				require.NoError(t, f.Close())
+			}
 
-		// Test write.
-		require.NoError(t, os.WriteFile(test.filename, []byte("proxy.pd-addrs = \"127.0.0.1:2379\""), 0644))
-		checkLog(reloadWrite)
+			cfgmgr, _, _ = testConfigManager(t, test.filename)
+			require.Equal(t, pdAddr1, cfgmgr.GetConfig().Proxy.PDAddrs)
 
-		// Test other.
-		if test.checker != nil {
-			test.checker(test.filename)
-		}
+			// Test write.
+			require.NoError(t, os.WriteFile(test.filename, []byte(fmt.Sprintf("proxy.pd-addrs = \"%s\"", pdAddr2)), 0644))
+			require.Eventually(t, func() bool {
+				return pdAddr2 == cfgmgr.GetConfig().Proxy.PDAddrs
+			}, 3*time.Second, 10*time.Millisecond, cfgmgr.GetConfig().Proxy.PDAddrs)
 
-		// Test remove.
-		if test.cleanFile != nil {
-			test.cleanFile()
-		} else {
-			// It doesn't matter whether it triggers reload or not.
-			require.NoError(t, os.Remove(test.filename))
+			// Test other.
+			if test.checker != nil {
+				test.checker(test.filename)
+			}
+
+			// Test remove.
+			if test.cleanFile != nil {
+				test.cleanFile()
+			} else {
+				// It doesn't matter whether it triggers reload or not.
+				require.NoError(t, os.Remove(test.filename))
+			}
+			require.NoError(t, cfgmgr.Close())
 		}
-		require.NoError(t, cfgmgr.Close())
 	}
 }