web: return status code and error message for config resource

cookingcodewithme · Jul 15, 2016 · 273e457 · 273e457
1 parent 4d0c697
commit 273e457
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 35 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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 {