diff --git a/cmd/query/app/mocks/Watcher.go b/cmd/query/app/mocks/Watcher.go new file mode 100644 index 00000000000..37b1c9b8736 --- /dev/null +++ b/cmd/query/app/mocks/Watcher.go @@ -0,0 +1,74 @@ +// Code generated by mockery v1.0.0. DO NOT EDIT. + +// Copyright (c) 2021 The Jaeger 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 mocks + +import ( + fsnotify "github.com/fsnotify/fsnotify" + mock "github.com/stretchr/testify/mock" +) + +// Watcher is an autogenerated mock type for the Watcher type +type Watcher struct { + mock.Mock +} + +// Add provides a mock function with given fields: name +func (_m *Watcher) Add(name string) error { + ret := _m.Called(name) + + var r0 error + if rf, ok := ret.Get(0).(func(string) error); ok { + r0 = rf(name) + } else { + r0 = ret.Error(0) + } + + return r0 +} + +// Errors provides a mock function with given fields: +func (_m *Watcher) Errors() chan error { + ret := _m.Called() + + var r0 chan error + if rf, ok := ret.Get(0).(func() chan error); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(chan error) + } + } + + return r0 +} + +// Events provides a mock function with given fields: +func (_m *Watcher) Events() chan fsnotify.Event { + ret := _m.Called() + + var r0 chan fsnotify.Event + if rf, ok := ret.Get(0).(func() chan fsnotify.Event); ok { + r0 = rf() + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).(chan fsnotify.Event) + } + } + + return r0 +} diff --git a/cmd/query/app/static_handler.go b/cmd/query/app/static_handler.go index 5f8e9709bd6..bf8632c1859 100644 --- a/cmd/query/app/static_handler.go +++ b/cmd/query/app/static_handler.go @@ -30,6 +30,7 @@ import ( "go.uber.org/zap" "github.com/jaegertracing/jaeger/cmd/query/app/ui" + "github.com/jaegertracing/jaeger/pkg/fswatcher" "github.com/jaegertracing/jaeger/pkg/version" ) @@ -60,9 +61,10 @@ func RegisterStaticHandler(r *mux.Router, logger *zap.Logger, qOpts *QueryOption // StaticAssetsHandler handles static assets type StaticAssetsHandler struct { - options StaticAssetsHandlerOptions - indexHTML atomic.Value // stores []byte - assetsFS http.FileSystem + options StaticAssetsHandlerOptions + indexHTML atomic.Value // stores []byte + assetsFS http.FileSystem + newWatcher func() (fswatcher.Watcher, error) } // StaticAssetsHandlerOptions defines options for NewStaticAssetsHandler @@ -70,6 +72,7 @@ type StaticAssetsHandlerOptions struct { BasePath string UIConfigPath string Logger *zap.Logger + NewWatcher func() (fswatcher.Watcher, error) } // NewStaticAssetsHandler returns a StaticAssetsHandler @@ -83,14 +86,19 @@ func NewStaticAssetsHandler(staticAssetsRoot string, options StaticAssetsHandler options.Logger = zap.NewNop() } + if options.NewWatcher == nil { + options.NewWatcher = fswatcher.NewWatcher + } + indexHTML, err := loadAndEnrichIndexHTML(assetsFS.Open, options) if err != nil { return nil, err } h := &StaticAssetsHandler{ - options: options, - assetsFS: assetsFS, + options: options, + assetsFS: assetsFS, + newWatcher: options.NewWatcher, } h.indexHTML.Store(indexHTML) @@ -134,10 +142,10 @@ func loadAndEnrichIndexHTML(open func(string) (http.File, error), options Static return indexBytes, nil } -func (sH *StaticAssetsHandler) configListener(watcher *fsnotify.Watcher) { +func (sH *StaticAssetsHandler) configListener(watcher fswatcher.Watcher) { for { select { - case event := <-watcher.Events: + case event := <-watcher.Events(): // ignore if the event filename is not the UI configuration if filepath.Base(event.Name) != filepath.Base(sH.options.UIConfigPath) { continue @@ -157,7 +165,8 @@ func (sH *StaticAssetsHandler) configListener(watcher *fsnotify.Watcher) { sH.options.Logger.Error("error while reloading the UI config", zap.Error(err)) } sH.indexHTML.Store(content) - case err, ok := <-watcher.Errors: + sH.options.Logger.Info("reloaded UI config", zap.String("filename", sH.options.UIConfigPath)) + case err, ok := <-watcher.Errors(): if !ok { return } @@ -168,10 +177,11 @@ func (sH *StaticAssetsHandler) configListener(watcher *fsnotify.Watcher) { func (sH *StaticAssetsHandler) watch() { if sH.options.UIConfigPath == "" { + sH.options.Logger.Info("UI config path not provided, config file will not be watched") return } - watcher, err := fsnotify.NewWatcher() + watcher, err := sH.newWatcher() if err != nil { sH.options.Logger.Error("failed to create a new watcher for the UI config", zap.Error(err)) return @@ -181,16 +191,14 @@ func (sH *StaticAssetsHandler) watch() { sH.configListener(watcher) }() - err = watcher.Add(sH.options.UIConfigPath) - if err != nil { + if err := watcher.Add(sH.options.UIConfigPath); err != nil { sH.options.Logger.Error("error adding watcher to file", zap.String("file", sH.options.UIConfigPath), zap.Error(err)) } else { sH.options.Logger.Info("watching", zap.String("file", sH.options.UIConfigPath)) } dir := filepath.Dir(sH.options.UIConfigPath) - err = watcher.Add(dir) - if err != nil { + if err := watcher.Add(dir); err != nil { sH.options.Logger.Error("error adding watcher to dir", zap.String("dir", dir), zap.Error(err)) } else { sH.options.Logger.Info("watching", zap.String("dir", dir)) diff --git a/cmd/query/app/static_handler_test.go b/cmd/query/app/static_handler_test.go index 97bdc50cd15..0e047fea9c4 100644 --- a/cmd/query/app/static_handler_test.go +++ b/cmd/query/app/static_handler_test.go @@ -25,13 +25,22 @@ import ( "testing" "time" + "github.com/fsnotify/fsnotify" "github.com/gorilla/mux" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "go.uber.org/zap" + "go.uber.org/zap/zapcore" + "go.uber.org/zap/zaptest/observer" + "github.com/jaegertracing/jaeger/cmd/query/app/mocks" + "github.com/jaegertracing/jaeger/pkg/fswatcher" "github.com/jaegertracing/jaeger/pkg/testutils" ) +//go:generate mockery -all -dir ../../../pkg/fswatcher + func TestNotExistingUiConfig(t *testing.T) { handler, err := NewStaticAssetsHandler("/foo/bar", StaticAssetsHandlerOptions{}) require.Error(t, err) @@ -115,50 +124,137 @@ func TestNewStaticAssetsHandlerErrors(t *testing.T) { } } -// This test is potentially intermittent +func TestWatcherError(t *testing.T) { + const totalWatcherAddCalls = 2 + + for _, tc := range []struct { + name string + errorOnNthAdd int + newWatcherErr error + watcherAddErr error + wantWatcherAddCalls int + }{ + { + name: "NewWatcher error", + newWatcherErr: fmt.Errorf("new watcher error"), + }, + { + name: "Watcher.Add first call error", + errorOnNthAdd: 0, + watcherAddErr: fmt.Errorf("add first error"), + wantWatcherAddCalls: 2, + }, + { + name: "Watcher.Add second call error", + errorOnNthAdd: 1, + watcherAddErr: fmt.Errorf("add second error"), + wantWatcherAddCalls: 2, + }, + } { + t.Run(tc.name, func(t *testing.T) { + // Prepare + zcore, logObserver := observer.New(zapcore.InfoLevel) + logger := zap.New(zcore) + defer func() { + if r := recover(); r != nil { + // Select loop exits without logging error, only containing previous error log. + assert.Equal(t, logObserver.FilterMessage("event").Len(), 1) + assert.Equal(t, "send on closed channel", fmt.Sprint(r)) + } + }() + + watcher := &mocks.Watcher{} + for i := 0; i < totalWatcherAddCalls; i++ { + var err error + if i == tc.errorOnNthAdd { + err = tc.watcherAddErr + } + watcher.On("Add", mock.Anything).Return(err).Once() + } + watcher.On("Events").Return(make(chan fsnotify.Event)) + errChan := make(chan error) + watcher.On("Errors").Return(errChan) + + // Test + _, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{ + UIConfigPath: "fixture/ui-config-hotreload.json", + NewWatcher: func() (fswatcher.Watcher, error) { + return watcher, tc.newWatcherErr + }, + Logger: logger, + }) + + // Validate + + // Error logged but not returned + assert.NoError(t, err) + if tc.newWatcherErr != nil { + assert.Equal(t, logObserver.FilterField(zap.Error(tc.newWatcherErr)).Len(), 1) + } else { + assert.Zero(t, logObserver.FilterField(zap.Error(tc.newWatcherErr)).Len()) + } + + if tc.watcherAddErr != nil { + assert.Equal(t, logObserver.FilterField(zap.Error(tc.watcherAddErr)).Len(), 1) + } else { + assert.Zero(t, logObserver.FilterField(zap.Error(tc.watcherAddErr)).Len()) + } + + watcher.AssertNumberOfCalls(t, "Add", tc.wantWatcherAddCalls) + + // Validate Events and Errors channels + if tc.newWatcherErr == nil { + errChan <- fmt.Errorf("first error") + + waitUntil(t, func() bool { + return logObserver.FilterMessage("event").Len() > 0 + }, 100, 10*time.Millisecond, "timed out waiting for error") + assert.Equal(t, logObserver.FilterMessage("event").Len(), 1) + + close(errChan) + errChan <- fmt.Errorf("second error on closed chan") + } + }) + } +} + func TestHotReloadUIConfigTempFile(t *testing.T) { - tmpfile, err := ioutil.TempFile("", "ui-config-hotreload.*.json") - assert.NoError(t, err) + dir, err := ioutil.TempDir("", "ui-config-hotreload-*") + require.NoError(t, err) + defer os.RemoveAll(dir) + tmpfile, err := ioutil.TempFile(dir, "*.json") + require.NoError(t, err) tmpFileName := tmpfile.Name() - defer os.Remove(tmpFileName) content, err := ioutil.ReadFile("fixture/ui-config-hotreload.json") - assert.NoError(t, err) + require.NoError(t, err) - err = ioutil.WriteFile(tmpFileName, content, 0644) - assert.NoError(t, err) + err = syncWrite(tmpFileName, content, 0644) + require.NoError(t, err) + zcore, logObserver := observer.New(zapcore.InfoLevel) + logger := zap.New(zcore) h, err := NewStaticAssetsHandler("fixture", StaticAssetsHandlerOptions{ UIConfigPath: tmpFileName, + Logger: logger, }) - assert.NoError(t, err) + require.NoError(t, err) c := string(h.indexHTML.Load().([]byte)) assert.Contains(t, c, "About Jaeger") newContent := strings.Replace(string(content), "About Jaeger", "About a new Jaeger", 1) - err = ioutil.WriteFile(tmpFileName, []byte(newContent), 0644) - assert.NoError(t, err) - - done := make(chan bool) - go func() { - for { - i := string(h.indexHTML.Load().([]byte)) + err = syncWrite(tmpFileName, []byte(newContent), 0644) + require.NoError(t, err) - if strings.Contains(i, "About a new Jaeger") { - done <- true - } - time.Sleep(10 * time.Millisecond) - } - }() + waitUntil(t, func() bool { + return logObserver.FilterMessage("reloaded UI config"). + FilterField(zap.String("filename", tmpFileName)).Len() > 0 + }, 100, 10*time.Millisecond, "timed out waiting for the hot reload to kick in") - select { - case <-done: - assert.Contains(t, string(h.indexHTML.Load().([]byte)), "About a new Jaeger") - case <-time.After(time.Second): - assert.Fail(t, "timed out waiting for the hot reload to kick in") - } + i := string(h.indexHTML.Load().([]byte)) + assert.Contains(t, i, "About a new Jaeger", logObserver.All()) } func TestLoadUIConfig(t *testing.T) { @@ -225,3 +321,27 @@ func TestLoadIndexHTMLReadError(t *testing.T) { _, err := loadIndexHTML(open) require.Error(t, err) } + +func waitUntil(t *testing.T, f func() bool, iterations int, sleepInterval time.Duration, timeoutErrMsg string) { + for i := 0; i < iterations; i++ { + if f() { + return + } + time.Sleep(sleepInterval) + } + require.Fail(t, timeoutErrMsg) +} + +// syncWrite ensures data is written to the given filename and flushed to disk. +// This ensures that any watchers looking for file system changes can be reliably alerted. +func syncWrite(filename string, data []byte, perm os.FileMode) error { + f, err := os.OpenFile(filename, os.O_WRONLY|os.O_CREATE|os.O_TRUNC|os.O_SYNC, perm) + if err != nil { + return err + } + defer f.Close() + if _, err = f.Write(data); err != nil { + return err + } + return f.Sync() +} diff --git a/pkg/fswatcher/fs_watcher.go b/pkg/fswatcher/fs_watcher.go new file mode 100644 index 00000000000..7c9c9aeec5b --- /dev/null +++ b/pkg/fswatcher/fs_watcher.go @@ -0,0 +1,51 @@ +// Copyright (c) 2021 The Jaeger 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 fswatcher + +import "github.com/fsnotify/fsnotify" + +// Watcher watches for Events and Errors once a resource is added to the watch list. +// Primarily used for mocking the fsnotify lib. +type Watcher interface { + Add(name string) error + Events() chan fsnotify.Event + Errors() chan error +} + +// fsnotifyWatcherWrapper wraps the fsnotify.Watcher and implements Watcher. +type fsnotifyWatcherWrapper struct { + fsnotifyWatcher *fsnotify.Watcher +} + +// Add adds the filename to watch. +func (f *fsnotifyWatcherWrapper) Add(name string) error { + return f.fsnotifyWatcher.Add(name) +} + +// Events returns the fsnotify.Watcher's Events chan. +func (f *fsnotifyWatcherWrapper) Events() chan fsnotify.Event { + return f.fsnotifyWatcher.Events +} + +// Errors returns the fsnotify.Watcher's Errors chan. +func (f *fsnotifyWatcherWrapper) Errors() chan error { + return f.fsnotifyWatcher.Errors +} + +// NewWatcher creates a new fsnotifyWatcherWrapper, wrapping the fsnotify.Watcher. +func NewWatcher() (Watcher, error) { + w, err := fsnotify.NewWatcher() + return &fsnotifyWatcherWrapper{fsnotifyWatcher: w}, err +} diff --git a/pkg/fswatcher/fs_watcher_test.go b/pkg/fswatcher/fs_watcher_test.go new file mode 100644 index 00000000000..e4e630f8b10 --- /dev/null +++ b/pkg/fswatcher/fs_watcher_test.go @@ -0,0 +1,40 @@ +// Copyright (c) 2021 The Jaeger 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 fswatcher + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestFsWatcher(t *testing.T) { + w, err := NewWatcher() + require.NoError(t, err) + assert.IsType(t, &fsnotifyWatcherWrapper{}, w) + + err = w.Add("foo") + assert.Error(t, err) + + err = w.Add("../../cmd/query/app/fixture/ui-config.json") + assert.NoError(t, err) + + events := w.Events() + assert.NotZero(t, events) + + errs := w.Errors() + assert.NotZero(t, errs) +}