From 273e457da4ec971c658282f1869bd54e24b962fc Mon Sep 17 00:00:00 2001 From: Dmitry Vorobev Date: Mon, 11 Jul 2016 16:24:54 +0200 Subject: [PATCH] web: return status code and error message for config resource --- cmd/prometheus/main.go | 34 +++++++++++++++++++++++----------- notifier/notifier.go | 5 ++--- retrieval/targetmanager.go | 5 ++--- rules/manager.go | 12 +++++------- storage/remote/remote.go | 5 ++--- web/web.go | 18 ++++++++++-------- 6 files changed, 44 insertions(+), 35 deletions(-) diff --git a/cmd/prometheus/main.go b/cmd/prometheus/main.go index 8bf2643b1..c5b9b6435 100644 --- a/cmd/prometheus/main.go +++ b/cmd/prometheus/main.go @@ -116,7 +116,8 @@ func Main() int { reloadables = append(reloadables, targetManager, ruleManager, webHandler, notifier) - if !reloadConfig(cfg.configFile, reloadables...) { + if err := reloadConfig(cfg.configFile, reloadables...); err != nil { + log.Errorf("Error loading config: %s", err) return 1 } @@ -131,9 +132,17 @@ func Main() int { for { select { case <-hup: - case <-webHandler.Reload(): + if err := reloadConfig(cfg.configFile, reloadables...); err != nil { + log.Errorf("Error reloading config: %s", err) + } + case rc := <-webHandler.Reload(): + if err := reloadConfig(cfg.configFile, reloadables...); err != nil { + log.Errorf("Error reloading config: %s", err) + rc <- err + } else { + rc <- nil + } } - reloadConfig(cfg.configFile, reloadables...) } }() @@ -199,13 +208,13 @@ func Main() int { // Reloadable things can change their internal state to match a new config // and handle failure gracefully. type Reloadable interface { - ApplyConfig(*config.Config) bool + ApplyConfig(*config.Config) error } -func reloadConfig(filename string, rls ...Reloadable) (success bool) { +func reloadConfig(filename string, rls ...Reloadable) (err error) { log.Infof("Loading configuration file %s", filename) defer func() { - if success { + if err == nil { configSuccess.Set(1) configSuccessTime.Set(float64(time.Now().Unix())) } else { @@ -215,13 +224,16 @@ func reloadConfig(filename string, rls ...Reloadable) (success bool) { conf, err := config.LoadFile(filename) if err != nil { - log.Errorf("Couldn't load configuration (-config.file=%s): %v", filename, err) - return false + return fmt.Errorf("couldn't load configuration (-config.file=%s): %v", filename, err) } - success = true + // Apply all configs and return the first error if there were any. for _, rl := range rls { - success = success && rl.ApplyConfig(conf) + if err != nil { + err = rl.ApplyConfig(conf) + } else { + rl.ApplyConfig(conf) + } } - return success + return err } diff --git a/notifier/notifier.go b/notifier/notifier.go index b54ccd1fb..4ab8e13e7 100644 --- a/notifier/notifier.go +++ b/notifier/notifier.go @@ -131,13 +131,12 @@ func New(o *Options) *Notifier { } // ApplyConfig updates the status state as the new config requires. -// Returns true on success. -func (n *Notifier) ApplyConfig(conf *config.Config) bool { +func (n *Notifier) ApplyConfig(conf *config.Config) error { n.mtx.Lock() defer n.mtx.Unlock() n.opts.ExternalLabels = conf.GlobalConfig.ExternalLabels - return true + return nil } const maxBatchSize = 64 diff --git a/retrieval/targetmanager.go b/retrieval/targetmanager.go index 4788f27dc..846bbe5f6 100644 --- a/retrieval/targetmanager.go +++ b/retrieval/targetmanager.go @@ -160,8 +160,7 @@ func (tm *TargetManager) Pools() map[string]Targets { // ApplyConfig resets the manager's target providers and job configurations as defined // by the new cfg. The state of targets that are valid in the new configuration remains unchanged. -// Returns true on success. -func (tm *TargetManager) ApplyConfig(cfg *config.Config) bool { +func (tm *TargetManager) ApplyConfig(cfg *config.Config) error { tm.mtx.Lock() defer tm.mtx.Unlock() @@ -170,7 +169,7 @@ func (tm *TargetManager) ApplyConfig(cfg *config.Config) bool { if tm.ctx != nil { tm.reload() } - return true + return nil } // targetSet holds several TargetProviders for which the same scrape configuration diff --git a/rules/manager.go b/rules/manager.go index 75977dd1e..8bd79a7ff 100644 --- a/rules/manager.go +++ b/rules/manager.go @@ -373,8 +373,8 @@ func (m *Manager) Stop() { } // ApplyConfig updates the rule manager's state as the config requires. If -// loading the new rules failed the old rule set is restored. Returns true on success. -func (m *Manager) ApplyConfig(conf *config.Config) bool { +// loading the new rules failed the old rule set is restored. +func (m *Manager) ApplyConfig(conf *config.Config) error { m.mtx.Lock() defer m.mtx.Unlock() @@ -384,16 +384,14 @@ func (m *Manager) ApplyConfig(conf *config.Config) bool { fs, err := filepath.Glob(pat) if err != nil { // The only error can be a bad pattern. - log.Errorf("Error retrieving rule files for %s: %s", pat, err) - return false + return fmt.Errorf("error retrieving rule files for %s: %s", pat, err) } files = append(files, fs...) } groups, err := m.loadGroups(files...) if err != nil { - log.Errorf("Error loading rules, previous rule set restored: %s", err) - return false + return fmt.Errorf("error loading rules, previous rule set restored: %s", err) } var wg sync.WaitGroup @@ -433,7 +431,7 @@ func (m *Manager) ApplyConfig(conf *config.Config) bool { wg.Wait() m.groups = groups - return true + return nil } // loadGroups reads groups from a list of files. diff --git a/storage/remote/remote.go b/storage/remote/remote.go index 6c0ddba9d..e4b222837 100644 --- a/storage/remote/remote.go +++ b/storage/remote/remote.go @@ -37,13 +37,12 @@ type Storage struct { } // ApplyConfig updates the status state as the new config requires. -// Returns true on success. -func (s *Storage) ApplyConfig(conf *config.Config) bool { +func (s *Storage) ApplyConfig(conf *config.Config) error { s.mtx.Lock() defer s.mtx.Unlock() s.externalLabels = conf.GlobalConfig.ExternalLabels - return true + return nil } // New returns a new remote Storage. diff --git a/web/web.go b/web/web.go index 338d6f0f5..dab5ca509 100644 --- a/web/web.go +++ b/web/web.go @@ -62,7 +62,7 @@ type Handler struct { router *route.Router listenErrCh chan error quitCh chan struct{} - reloadCh chan struct{} + reloadCh chan chan error options *Options configString string versionInfo *PrometheusVersion @@ -74,15 +74,14 @@ type Handler struct { } // ApplyConfig updates the status state as the new config requires. -// Returns true on success. -func (h *Handler) ApplyConfig(conf *config.Config) bool { +func (h *Handler) ApplyConfig(conf *config.Config) error { h.mtx.Lock() defer h.mtx.Unlock() h.externalLabels = conf.GlobalConfig.ExternalLabels h.configString = conf.String() - return true + return nil } // PrometheusVersion contains build information about Prometheus. @@ -124,7 +123,7 @@ func New( router: router, listenErrCh: make(chan error), quitCh: make(chan struct{}), - reloadCh: make(chan struct{}), + reloadCh: make(chan chan error), options: o, versionInfo: version, birth: time.Now(), @@ -225,7 +224,7 @@ func (h *Handler) Quit() <-chan struct{} { } // Reload returns the receive-only channel that signals configuration reload requests. -func (h *Handler) Reload() <-chan struct{} { +func (h *Handler) Reload() <-chan chan error { return h.reloadCh } @@ -352,8 +351,11 @@ func (h *Handler) quit(w http.ResponseWriter, r *http.Request) { } func (h *Handler) reload(w http.ResponseWriter, r *http.Request) { - fmt.Fprintf(w, "Reloading configuration file...") - h.reloadCh <- struct{}{} + rc := make(chan error) + h.reloadCh <- rc + if err := <-rc; err != nil { + http.Error(w, fmt.Sprintf("failed to reload config: %s", err), http.StatusInternalServerError) + } } func (h *Handler) consolesPath() string {