Skip to content

Commit c0f4adc

Browse files
authored
config: check mod time (#445)
1 parent 457002b commit c0f4adc

File tree

8 files changed

+69
-102
lines changed

8 files changed

+69
-102
lines changed

Makefile

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ go-header:
5353
GOBIN=$(GOBIN) go install github.com/denis-tingaikin/go-header/cmd/go-header@latest
5454

5555
header: go-header
56-
$(GOBIN)/go-header $(shell find . -name "*.go" -not -path "./pkg/proxy/keepalive*")
56+
NEW_GO_FILES=$(git diff --cached --diff-filter=A --name-only | grep -E '.*\.go')
57+
[ ! $(NEW_GO_FILES) ] || $(GOBIN)/go-header $(NEW_GO_FILES)
5758

5859
lint: golangci-lint tidy header
5960
cd lib && $(GOBIN)/golangci-lint run -c ../.golangci.yaml

go.mod

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ require (
66
github.com/BurntSushi/toml v1.2.1
77
github.com/bahlo/generic-list-go v0.2.0
88
github.com/cenkalti/backoff/v4 v4.2.1
9-
github.com/fsnotify/fsnotify v1.6.0
109
github.com/gin-contrib/pprof v1.4.0
1110
github.com/gin-gonic/gin v1.8.1
1211
github.com/go-mysql-org/go-mysql v1.6.0

go.sum

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -214,8 +214,6 @@ github.com/fatih/structs v1.1.0/go.mod h1:9NiDSp5zOcgEDl+j00MP/WkGVPOlPRLejGD8Ga
214214
github.com/flosch/pongo2 v0.0.0-20190707114632-bbf5a6c351f4/go.mod h1:T9YF2M40nIgbVgp3rreNmTged+9HrbNTIQf1PsaIiTA=
215215
github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
216216
github.com/fsnotify/fsnotify v1.4.9/go.mod h1:znqG4EE+3YCdAaPaxE2ZRY/06pZUdp0tY4IgpuI1SZQ=
217-
github.com/fsnotify/fsnotify v1.6.0 h1:n+5WquG0fcWoWp6xPWfHdbskMCQaFnG6PfBrh1Ky4HY=
218-
github.com/fsnotify/fsnotify v1.6.0/go.mod h1:sl3t1tCWJFWoRz9R8WJCbQihKKwmorjAbSClcnxKAGw=
219217
github.com/gavv/httpexpect v2.0.0+incompatible/go.mod h1:x+9tiU1YnrOvnB725RkpoLv1M62hOWzwo5OXotisrKc=
220218
github.com/getsentry/raven-go v0.2.0/go.mod h1:KungGk8q33+aIAZUIVWZDr2OfAEBsO49PX4NzFV5kcQ=
221219
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
10411039
golang.org/x/sys v0.0.0-20211013075003-97ac67df715c/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
10421040
golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
10431041
golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
1044-
golang.org/x/sys v0.0.0-20220908164124-27713097b956/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
10451042
golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
10461043
golang.org/x/sys v0.8.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
10471044
golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=

lib/config/proxy.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ func (cfg *Config) Check() error {
162162
if cfg.Workdir == "" {
163163
d, err := os.Getwd()
164164
if err != nil {
165-
return err
165+
return errors.WithStack(err)
166166
}
167167
cfg.Workdir = filepath.Clean(filepath.Join(d, "work"))
168168
}
@@ -184,5 +184,5 @@ func (cfg *Config) Check() error {
184184
func (cfg *Config) ToBytes() ([]byte, error) {
185185
b := new(bytes.Buffer)
186186
err := toml.NewEncoder(b).Encode(cfg)
187-
return b.Bytes(), err
187+
return b.Bytes(), errors.WithStack(err)
188188
}

pkg/manager/config/config.go

Lines changed: 5 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7,50 +7,22 @@ import (
77
"bytes"
88
"hash/crc32"
99
"os"
10-
"time"
1110

1211
"github.com/BurntSushi/toml"
13-
"github.com/fsnotify/fsnotify"
1412
"github.com/pingcap/tiproxy/lib/config"
13+
"github.com/pingcap/tiproxy/lib/util/errors"
1514
"go.uber.org/zap"
1615
)
1716

1817
func (e *ConfigManager) reloadConfigFile(file string) error {
1918
proxyConfigData, err := os.ReadFile(file)
2019
if err != nil {
21-
return err
20+
return errors.WithStack(err)
2221
}
2322

2423
return e.SetTOMLConfig(proxyConfigData)
2524
}
2625

27-
func (e *ConfigManager) handleFSEvent(ev fsnotify.Event, f string) {
28-
switch {
29-
case ev.Has(fsnotify.Create), ev.Has(fsnotify.Write), ev.Has(fsnotify.Remove), ev.Has(fsnotify.Rename):
30-
// The file may be the log file, triggering reload will cause more logs and thus cause reload again,
31-
// so we need to filter the wrong files.
32-
// The filesystem differs from OS to OS, so don't use string comparison.
33-
f1, err := os.Stat(ev.Name)
34-
if err != nil {
35-
break
36-
}
37-
f2, err := os.Stat(f)
38-
if err != nil {
39-
break
40-
}
41-
if !os.SameFile(f1, f2) {
42-
break
43-
}
44-
if ev.Has(fsnotify.Remove) || ev.Has(fsnotify.Rename) {
45-
// in case of remove/rename the file, files are not present at filesystem for a while
46-
// it may be too fast to read the config file now, sleep for a while
47-
time.Sleep(50 * time.Millisecond)
48-
}
49-
// try to reload it
50-
e.logger.Info("config file reloaded", zap.Stringer("event", ev), zap.Error(e.reloadConfigFile(f)))
51-
}
52-
}
53-
5426
// SetTOMLConfig will do partial config update. Usually, user will expect config changes
5527
// only when they specified a config item. It is, however, impossible to tell a struct
5628
// `c.max-conns == 0` means no user-input, or it specified `0`.
@@ -73,11 +45,11 @@ func (e *ConfigManager) SetTOMLConfig(data []byte) (err error) {
7345
}
7446

7547
if err = toml.Unmarshal(data, base); err != nil {
76-
return
48+
return errors.WithStack(err)
7749
}
7850

7951
if err = toml.Unmarshal(e.overlay, base); err != nil {
80-
return
52+
return errors.WithStack(err)
8153
}
8254

8355
if err = base.Check(); err != nil {
@@ -87,7 +59,7 @@ func (e *ConfigManager) SetTOMLConfig(data []byte) (err error) {
8759
e.sts.current = base
8860
var buf bytes.Buffer
8961
if err = toml.NewEncoder(&buf).Encode(base); err != nil {
90-
return
62+
return errors.WithStack(err)
9163
}
9264
e.sts.checksum = crc32.ChecksumIEEE(buf.Bytes())
9365

pkg/manager/config/config_test.go

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,7 @@ func TestConfigReload(t *testing.T) {
123123

124124
require.NoError(t, os.WriteFile(tmpcfg, []byte(tc.postcfg), 0644), msg)
125125
if tc.postcheck != nil {
126-
require.Eventually(t, func() bool { return tc.postcheck(cfgmgr1.GetConfig()) }, time.Second, 100*time.Millisecond, msg)
126+
require.Eventually(t, func() bool { return tc.postcheck(cfgmgr1.GetConfig()) }, 3*time.Second, 100*time.Millisecond, msg)
127127
}
128128
}
129129
}
@@ -143,15 +143,15 @@ func TestConfigRemove(t *testing.T) {
143143
require.NoError(t, os.WriteFile(tmpcfg, []byte(`proxy.addr = "gg"`), 0644))
144144

145145
// check that reload still works
146-
require.Eventually(t, func() bool { return cfgmgr.GetConfig().Proxy.Addr == "gg" }, time.Second, 100*time.Millisecond)
146+
require.Eventually(t, func() bool { return cfgmgr.GetConfig().Proxy.Addr == "gg" }, 3*time.Second, 100*time.Millisecond)
147147

148148
// remove again but with a long sleep
149149
require.NoError(t, os.Remove(tmpcfg))
150150
time.Sleep(200 * time.Millisecond)
151151

152-
// but eventually re-watched the file again
152+
// but eventually reload the file again
153153
require.NoError(t, os.WriteFile(tmpcfg, []byte(`proxy.addr = "vv"`), 0644))
154-
require.Eventually(t, func() bool { return cfgmgr.GetConfig().Proxy.Addr == "vv" }, time.Second, 100*time.Millisecond)
154+
require.Eventually(t, func() bool { return cfgmgr.GetConfig().Proxy.Addr == "vv" }, 3*time.Second, 100*time.Millisecond)
155155
}
156156

157157
func TestFilePath(t *testing.T) {
@@ -163,12 +163,18 @@ func TestFilePath(t *testing.T) {
163163

164164
tmpdir := t.TempDir()
165165
checkLog := func(increased bool) {
166-
// On linux, writing once will trigger 2 WRITE events. But on macOS, it only triggers once.
167-
// So we always sleep 100ms to avoid missing any logs on the way.
168-
time.Sleep(100 * time.Millisecond)
169-
newCount := strings.Count(text.String(), "config file reloaded")
170-
require.Equal(t, increased, newCount > count, fmt.Sprintf("now: %d, was: %d", newCount, count))
171-
count = newCount
166+
if increased {
167+
var newCount int
168+
require.Eventually(t, func() bool {
169+
newCount = strings.Count(text.String(), "config file reloaded")
170+
return newCount > count
171+
}, 3*time.Second, 10*time.Millisecond, "count=%d, newCount=%d", count, newCount)
172+
count = newCount
173+
} else {
174+
time.Sleep(100 * time.Millisecond)
175+
newCount := strings.Count(text.String(), "config file reloaded")
176+
require.Equal(t, count, newCount)
177+
}
172178
}
173179

174180
tests := []struct {
@@ -271,7 +277,7 @@ func TestFilePath(t *testing.T) {
271277

272278
count = 0
273279
cfgmgr, text, _ = testConfigManager(t, test.filename)
274-
checkLog(false)
280+
checkLog(true)
275281

276282
// Test write.
277283
require.NoError(t, os.WriteFile(test.filename, []byte("proxy.pd-addrs = \"127.0.0.1:2379\""), 0644))

pkg/manager/config/manager.go

Lines changed: 41 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -5,11 +5,10 @@ package config
55

66
import (
77
"context"
8-
"path/filepath"
8+
"os"
99
"sync"
1010
"time"
1111

12-
"github.com/fsnotify/fsnotify"
1312
"github.com/pingcap/tiproxy/lib/config"
1413
"github.com/pingcap/tiproxy/lib/util/errors"
1514
"github.com/pingcap/tiproxy/lib/util/waitgroup"
@@ -22,6 +21,10 @@ const (
2221
pathPrefixConfig = "config"
2322
)
2423

24+
const (
25+
checkFileInterval = time.Second
26+
)
27+
2528
var (
2629
ErrNoResults = errors.Errorf("has no results")
2730
ErrFail2Update = errors.Errorf("failed to update")
@@ -39,9 +42,10 @@ type ConfigManager struct {
3942

4043
kv *btree.BTreeG[KVValue]
4144

42-
wch *fsnotify.Watcher
43-
overlay []byte
44-
sts struct {
45+
lastModTime time.Time
46+
checkFileInterval time.Duration
47+
overlay []byte
48+
sts struct {
4549
sync.Mutex
4650
listeners []chan<- *config.Config
4751
current *config.Config
@@ -50,7 +54,9 @@ type ConfigManager struct {
5054
}
5155

5256
func NewConfigManager() *ConfigManager {
53-
return &ConfigManager{}
57+
return &ConfigManager{
58+
checkFileInterval: checkFileInterval,
59+
}
5460
}
5561

5662
func (e *ConfigManager) Init(ctx context.Context, logger *zap.Logger, configFile string, overlay *config.Config) error {
@@ -69,69 +75,58 @@ func (e *ConfigManager) Init(ctx context.Context, logger *zap.Logger, configFile
6975
if overlay != nil {
7076
e.overlay, err = overlay.ToBytes()
7177
if err != nil {
72-
return errors.WithStack(err)
78+
return err
7379
}
7480
}
7581

7682
if configFile != "" {
77-
e.wch, err = fsnotify.NewWatcher()
78-
if err != nil {
79-
return errors.WithStack(err)
83+
if err := e.checkFileAndLoad(configFile); err != nil {
84+
return err
8085
}
81-
82-
// Watch the parent dir, because vim/k8s or other apps may not edit files in-place:
83-
// e.g. k8s configmap is a symlink of a symlink to a file, which will only trigger
84-
// a remove event for the file.
85-
parentDir := filepath.Dir(configFile)
86-
87-
if err := e.reloadConfigFile(configFile); err != nil {
88-
return errors.WithStack(err)
89-
}
90-
if err := e.wch.Add(parentDir); err != nil {
91-
return errors.WithStack(err)
92-
}
93-
9486
e.wg.Run(func() {
95-
// Some apps will trigger rename/remove events, which means they will re-create/rename
96-
// the new file to the directory. Watch possibly stopped after rename/remove events.
97-
// So, we use a tick to repeatedly add the parent dir to re-watch files.
98-
ticker := time.NewTicker(200 * time.Millisecond)
87+
var lastErr error
88+
ticker := time.NewTicker(e.checkFileInterval)
9989
for {
10090
select {
10191
case <-nctx.Done():
10292
return
103-
case err := <-e.wch.Errors:
104-
e.logger.Warn("failed to watch config file", zap.Error(err))
105-
case ev := <-e.wch.Events:
106-
e.handleFSEvent(ev, configFile)
10793
case <-ticker.C:
108-
// There may be concurrent issues:
109-
// 1. Remove the directory and the watcher removes the directory automatically
110-
// 2. Create the directory and the file again within a tick
111-
// 3. Add it to the watcher again, but the CREATE event is not sent
112-
//
113-
// Checking the watch list still have a concurrent issue because the watcher may remove the
114-
// directory between WatchList and Add. We'll fix it later because it's complex to fix it entirely.
115-
exists := len(e.wch.WatchList()) > 0
116-
if err := e.wch.Add(parentDir); err != nil {
117-
e.logger.Warn("failed to rewatch config file", zap.Error(err))
118-
} else if !exists {
119-
e.logger.Info("config file reloaded", zap.Error(e.reloadConfigFile(configFile)))
94+
// Do not report the same error to avoid log flooding.
95+
if err = e.checkFileAndLoad(configFile); err != nil && errors.Is(err, lastErr) {
96+
e.logger.Warn("reload config file failed", zap.Error(err))
12097
}
98+
lastErr = err
12199
}
122100
}
123101
})
124102
} else {
125103
if err := e.SetTOMLConfig(nil); err != nil {
126-
return errors.WithStack(err)
104+
return err
127105
}
128106
}
129107

130108
return nil
131109
}
132110

111+
func (e *ConfigManager) checkFileAndLoad(filename string) error {
112+
info, err := os.Stat(filename)
113+
if err != nil {
114+
return errors.WithStack(err)
115+
}
116+
if info.IsDir() {
117+
return errors.New("config file is a directory")
118+
}
119+
if info.ModTime() != e.lastModTime {
120+
if err = e.reloadConfigFile(filename); err != nil {
121+
return err
122+
}
123+
e.logger.Info("config file reloaded", zap.Time("file_modify_time", info.ModTime()))
124+
e.lastModTime = info.ModTime()
125+
}
126+
return nil
127+
}
128+
133129
func (e *ConfigManager) Close() error {
134-
var wcherr error
135130
if e.cancel != nil {
136131
e.cancel()
137132
e.cancel = nil
@@ -143,10 +138,5 @@ func (e *ConfigManager) Close() error {
143138
e.sts.listeners = nil
144139
e.sts.Unlock()
145140
e.wg.Wait()
146-
// close after all goroutines are done
147-
if e.wch != nil {
148-
wcherr = e.wch.Close()
149-
e.wch = nil
150-
}
151-
return wcherr
141+
return nil
152142
}

pkg/manager/config/manager_test.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77
"context"
88
"fmt"
99
"testing"
10+
"time"
1011

1112
"github.com/pingcap/tiproxy/lib/config"
1213
"github.com/pingcap/tiproxy/lib/util/logger"
@@ -22,6 +23,7 @@ func testConfigManager(t *testing.T, configFile string, overlays ...*config.Conf
2223
}
2324

2425
cfgmgr := NewConfigManager()
26+
cfgmgr.checkFileInterval = 20 * time.Millisecond
2527
require.NoError(t, cfgmgr.Init(ctx, logger, configFile, nil))
2628

2729
t.Cleanup(func() {

0 commit comments

Comments
 (0)