Skip to content

Commit d240074

Browse files
andaaronandaaron
authored
fix(cli): stop test server leaks and initialize Trivy logger safely (#4331)
Make HotReloader stoppable, shut down CLI test servers after each case, and route Trivy logs through zot's slog handler to avoid DeferredHandler data races. This is to fix the race in https://github.com/project-zot/zot/actions/runs/32109063736/job/95624416506?pr=4330 Signed-off-by: Andrei Aaron <andreifdaaron@gmail.com>
1 parent 5fca8a6 commit d240074

7 files changed

Lines changed: 314 additions & 193 deletions

File tree

‎pkg/cli/server/config_reloader.go‎

Lines changed: 31 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"errors"
55
"os"
66
"os/signal"
7+
"sync"
78
"syscall"
89

910
"github.com/fsnotify/fsnotify"
@@ -19,6 +20,9 @@ type HotReloader struct {
1920
ldapCredentialsPath string
2021
ctlr *api.Controller
2122
logger log.Logger
23+
24+
done chan struct{}
25+
stopOnce sync.Once
2226
}
2327

2428
func NewHotReloader(ctlr *api.Controller, filePath, ldapCredentialsPath string) (*HotReloader, error) {
@@ -34,25 +38,27 @@ func NewHotReloader(ctlr *api.Controller, filePath, ldapCredentialsPath string)
3438
ldapCredentialsPath: ldapCredentialsPath,
3539
ctlr: ctlr,
3640
logger: log.NewLogger("info", ""),
41+
done: make(chan struct{}),
3742
}
3843

3944
return hotReloader, nil
4045
}
4146

42-
func signalHandler(ctlr *api.Controller, sigCh chan os.Signal) {
47+
func signalHandler(ctlr *api.Controller, hr *HotReloader, sigCh chan os.Signal) {
4348
// if signal then shutdown
4449
if sig, ok := <-sigCh; ok {
4550
ctlr.Log.Info().Interface("signal", sig).Msg("received signal")
4651

52+
hr.Stop()
4753
// gracefully shutdown http server
4854
ctlr.Shutdown() //nolint: contextcheck
4955
}
5056
}
5157

52-
func initShutDownRoutine(ctlr *api.Controller) {
58+
func initShutDownRoutine(ctlr *api.Controller, hr *HotReloader) {
5359
sigCh := make(chan os.Signal, 1)
5460

55-
go signalHandler(ctlr, sigCh)
61+
go signalHandler(ctlr, hr, sigCh)
5662

5763
// block all async signals to this server
5864
signal.Ignore()
@@ -61,18 +67,32 @@ func initShutDownRoutine(ctlr *api.Controller) {
6167
signal.Notify(sigCh, syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP)
6268
}
6369

64-
func (hr *HotReloader) Start() {
65-
done := make(chan bool)
70+
func (hr *HotReloader) Stop() {
71+
hr.stopOnce.Do(func() {
72+
if hr.done != nil {
73+
close(hr.done)
74+
}
75+
if hr.watcher != nil {
76+
_ = hr.watcher.Close()
77+
}
78+
})
79+
}
6680

81+
func (hr *HotReloader) Start() {
6782
// run watcher
6883
go func() {
6984
defer hr.watcher.Close()
7085

7186
go func() {
7287
for {
7388
select {
89+
case <-hr.done:
90+
return
7491
// watch for events
75-
case event := <-hr.watcher.Events:
92+
case event, ok := <-hr.watcher.Events:
93+
if !ok {
94+
return
95+
}
7696
if event.Op == fsnotify.Write {
7797
hr.logger.Info().Msg("config file changed, trying to reload config")
7898

@@ -110,7 +130,10 @@ func (hr *HotReloader) Start() {
110130
hr.ctlr.StartBackgroundTasks()
111131
}
112132
// watch for errors
113-
case err := <-hr.watcher.Errors:
133+
case err, ok := <-hr.watcher.Errors:
134+
if !ok {
135+
return
136+
}
114137
hr.logger.Panic().Err(err).Str("config", hr.configPath).Msg("fsnotfy error while watching config")
115138
}
116139
}
@@ -127,6 +150,6 @@ func (hr *HotReloader) Start() {
127150
}
128151
}
129152

130-
<-done
153+
<-hr.done
131154
}()
132155
}

‎pkg/cli/server/config_reloader_internal_test.go‎

Lines changed: 141 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,141 @@
1+
package server //nolint:testpackage // white-box tests for unexported signalHandler and watcher close
2+
3+
import (
4+
"os"
5+
"path/filepath"
6+
"syscall"
7+
"testing"
8+
"time"
9+
10+
. "github.com/smartystreets/goconvey/convey"
11+
12+
"zotregistry.dev/zot/v2/pkg/api"
13+
"zotregistry.dev/zot/v2/pkg/api/config"
14+
)
15+
16+
func TestHotReloaderStop(t *testing.T) {
17+
Convey("Stop is safe with nil done and watcher", t, func() {
18+
reloader := &HotReloader{}
19+
So(func() { reloader.Stop() }, ShouldNotPanic)
20+
So(func() { reloader.Stop() }, ShouldNotPanic)
21+
})
22+
23+
Convey("SIGTERM stops the reloader and shuts down the controller", t, func() {
24+
reloader := newTestHotReloader(t)
25+
reloader.Start()
26+
So(waitForWatch(reloader, 2*time.Second), ShouldBeTrue)
27+
28+
sigCh := make(chan os.Signal, 1)
29+
finished := make(chan struct{})
30+
31+
go func() {
32+
signalHandler(reloader.ctlr, reloader, sigCh)
33+
close(finished)
34+
}()
35+
36+
sigCh <- syscall.SIGTERM
37+
So(waitChan(finished, 2*time.Second), ShouldBeTrue)
38+
So(isClosed(reloader.done), ShouldBeTrue)
39+
})
40+
41+
Convey("closed signal channel returns without stopping the reloader", t, func() {
42+
reloader := newTestHotReloader(t)
43+
t.Cleanup(reloader.Stop)
44+
45+
sigCh := make(chan os.Signal)
46+
finished := make(chan struct{})
47+
48+
go func() {
49+
signalHandler(reloader.ctlr, reloader, sigCh)
50+
close(finished)
51+
}()
52+
53+
close(sigCh)
54+
So(waitChan(finished, 2*time.Second), ShouldBeTrue)
55+
So(isClosed(reloader.done), ShouldBeFalse)
56+
})
57+
58+
Convey("closed watcher events channel exits the watch loop", t, func() {
59+
reloader := newTestHotReloader(t)
60+
// Block the Errors case so select must take Events once the watcher is closed.
61+
reloader.watcher.Errors = nil
62+
startReloaderAndCloseWatcher(t, reloader)
63+
})
64+
65+
Convey("closed watcher errors channel exits the watch loop", t, func() {
66+
reloader := newTestHotReloader(t)
67+
// Block the Events case so select must take Errors once the watcher is closed.
68+
reloader.watcher.Events = nil
69+
startReloaderAndCloseWatcher(t, reloader)
70+
})
71+
}
72+
73+
func testServerConfig(t *testing.T) *config.Config {
74+
t.Helper()
75+
76+
conf := config.New()
77+
conf.HTTP.Address = "127.0.0.1"
78+
conf.HTTP.Port = "0"
79+
conf.Storage.RootDirectory = t.TempDir()
80+
81+
return conf
82+
}
83+
84+
func newTestHotReloader(t *testing.T) *HotReloader {
85+
t.Helper()
86+
87+
configPath := filepath.Join(t.TempDir(), "zot.json")
88+
err := os.WriteFile(configPath, []byte(`{}`), 0o600)
89+
So(err, ShouldBeNil)
90+
91+
reloader, err := NewHotReloader(api.NewController(testServerConfig(t)), configPath, "")
92+
So(err, ShouldBeNil)
93+
94+
return reloader
95+
}
96+
97+
func startReloaderAndCloseWatcher(t *testing.T, reloader *HotReloader) {
98+
t.Helper()
99+
100+
reloader.Start()
101+
So(waitForWatch(reloader, 2*time.Second), ShouldBeTrue)
102+
103+
err := reloader.watcher.Close()
104+
So(err, ShouldBeNil)
105+
106+
// Let the watch loop observe the closed channel before Stop() also unblocks <-done.
107+
time.Sleep(100 * time.Millisecond)
108+
reloader.Stop()
109+
}
110+
111+
func waitForWatch(reloader *HotReloader, timeout time.Duration) bool {
112+
deadline := time.Now().Add(timeout)
113+
114+
for time.Now().Before(deadline) {
115+
if len(reloader.watcher.WatchList()) > 0 {
116+
return true
117+
}
118+
119+
time.Sleep(10 * time.Millisecond)
120+
}
121+
122+
return false
123+
}
124+
125+
func waitChan(done <-chan struct{}, timeout time.Duration) bool {
126+
select {
127+
case <-done:
128+
return true
129+
case <-time.After(timeout):
130+
return false
131+
}
132+
}
133+
134+
func isClosed(done <-chan struct{}) bool {
135+
select {
136+
case <-done:
137+
return true
138+
default:
139+
return false
140+
}
141+
}

‎pkg/cli/server/config_reloader_test.go‎

Lines changed: 10 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -11,16 +11,11 @@ import (
1111

1212
. "github.com/smartystreets/goconvey/convey"
1313

14-
cli "zotregistry.dev/zot/v2/pkg/cli/server"
1514
test "zotregistry.dev/zot/v2/pkg/test/common"
1615
)
1716

1817
func TestConfigReloader(t *testing.T) {
19-
oldArgs := os.Args
20-
21-
defer func() { os.Args = oldArgs }()
22-
23-
Convey("reload access control config", t, func(conveyCtx C) {
18+
Convey("reload access control config", t, func() {
2419
logPath := test.MakeTempFilePath(t, "zot-log.txt")
2520

2621
username := "alice"
@@ -72,15 +67,7 @@ func TestConfigReloader(t *testing.T) {
7267
_, err := cfgfile.WriteString(content)
7368
So(err, ShouldBeNil)
7469

75-
os.Args = []string{"cli_test", "serve", cfgfile.Name()}
76-
77-
go func() {
78-
err = cli.NewServerRootCmd().Execute()
79-
conveyCtx.So(err, ShouldBeNil)
80-
}()
81-
82-
baseURL := test.WaitForKernelChosenPortBaseURL(logPath)
83-
test.WaitTillServerReady(baseURL)
70+
So(startServerFromConfigFile(t, cfgfile.Name()), ShouldBeNil)
8471

8572
// verify initial startup authentication logs
8673
initialData, err := os.ReadFile(logPath)
@@ -171,7 +158,7 @@ func TestConfigReloader(t *testing.T) {
171158
})
172159
})
173160

174-
Convey("reload gc config", t, func(ctx C) {
161+
Convey("reload gc config", t, func() {
175162
logFile := test.MakeTempFile(t, "zot-log.txt")
176163
defer logFile.Close()
177164

@@ -205,15 +192,7 @@ func TestConfigReloader(t *testing.T) {
205192
_, err := cfgfile.WriteString(content)
206193
So(err, ShouldBeNil)
207194

208-
os.Args = []string{"cli_test", "serve", cfgfile.Name()}
209-
210-
go func() {
211-
err = cli.NewServerRootCmd().Execute()
212-
ctx.So(err, ShouldBeNil)
213-
}()
214-
215-
baseURL := test.WaitForKernelChosenPortBaseURL(logFile.Name())
216-
test.WaitTillServerReady(baseURL)
195+
So(startServerFromConfigFile(t, cfgfile.Name()), ShouldBeNil)
217196

218197
// verify initial startup authentication logs (no auth configured)
219198
initialData, err := os.ReadFile(logFile.Name())
@@ -296,7 +275,7 @@ func TestConfigReloader(t *testing.T) {
296275
})
297276
})
298277

299-
Convey("reload sync config", t, func(ctx C) {
278+
Convey("reload sync config", t, func() {
300279
logPath := test.MakeTempFilePath(t, "zot-log.txt")
301280

302281
content := fmt.Sprintf(`{
@@ -341,15 +320,7 @@ func TestConfigReloader(t *testing.T) {
341320
_, err := cfgfile.WriteString(content)
342321
So(err, ShouldBeNil)
343322

344-
os.Args = []string{"cli_test", "serve", cfgfile.Name()}
345-
346-
go func() {
347-
err = cli.NewServerRootCmd().Execute()
348-
ctx.So(err, ShouldBeNil)
349-
}()
350-
351-
baseURL := test.WaitForKernelChosenPortBaseURL(logPath)
352-
test.WaitTillServerReady(baseURL)
323+
So(startServerFromConfigFile(t, cfgfile.Name()), ShouldBeNil)
353324

354325
// verify initial startup authentication logs (no auth configured)
355326
initialData, err := os.ReadFile(logPath)
@@ -444,7 +415,7 @@ func TestConfigReloader(t *testing.T) {
444415
})
445416
})
446417

447-
Convey("reload scrub and CVE config", t, func(ctx C) {
418+
Convey("reload scrub and CVE config", t, func() {
448419
logPath := test.MakeTempFilePath(t, "zot-log.txt")
449420

450421
content := fmt.Sprintf(`{
@@ -482,15 +453,7 @@ func TestConfigReloader(t *testing.T) {
482453
_, err := cfgfile.WriteString(content)
483454
So(err, ShouldBeNil)
484455

485-
os.Args = []string{"cli_test", "serve", cfgfile.Name()}
486-
487-
go func() {
488-
err = cli.NewServerRootCmd().Execute()
489-
ctx.So(err, ShouldBeNil)
490-
}()
491-
492-
baseURL := test.WaitForKernelChosenPortBaseURL(logPath)
493-
test.WaitTillServerReady(baseURL)
456+
So(startServerFromConfigFile(t, cfgfile.Name()), ShouldBeNil)
494457

495458
// verify initial startup authentication logs (no auth configured)
496459
initialData, err := os.ReadFile(logPath)
@@ -583,7 +546,7 @@ func TestConfigReloader(t *testing.T) {
583546
So(found, ShouldBeTrue)
584547
})
585548

586-
Convey("reload bad config", t, func(conveyCtx C) {
549+
Convey("reload bad config", t, func() {
587550
logPath := test.MakeTempFilePath(t, "zot-log.txt")
588551

589552
content := fmt.Sprintf(`{
@@ -628,15 +591,7 @@ func TestConfigReloader(t *testing.T) {
628591
_, err := cfgfile.WriteString(content)
629592
So(err, ShouldBeNil)
630593

631-
os.Args = []string{"cli_test", "serve", cfgfile.Name()}
632-
633-
go func() {
634-
err = cli.NewServerRootCmd().Execute()
635-
conveyCtx.So(err, ShouldBeNil)
636-
}()
637-
638-
baseURL := test.WaitForKernelChosenPortBaseURL(logPath)
639-
test.WaitTillServerReady(baseURL)
594+
So(startServerFromConfigFile(t, cfgfile.Name()), ShouldBeNil)
640595

641596
content = "[]"
642597

0 commit comments

Comments
 (0)