*: Introduce config coordinator bundling config specific logic #1744

mxinden · 2019-02-06T18:14:53Z

Instead of handling all config specific logic inside
Alertmangaer.main(), this patch introduces the config coordinator
component.

Tasks of the config coordinator:

Load and parse configuration
Notify subscribers on configuration changes
Register and manage configuration specific metrics

This patch still has a couple of TODOs. I am opening a pull request anyways, to get early feedback whether a refactoring like this is wanted.

beorn7 · 2019-02-06T18:38:39Z

This is acting on precisely the same code @stuartnelson3 and I are currently working on.
Can we perhaps postpone the refactoring until we are done with that?

beorn7 · 2019-02-06T18:39:14Z

See #1743 and #1733 .

mxinden · 2019-02-06T18:46:45Z

This is acting on precisely the same code @stuartnelson3 and I are currently working on.
Can we perhaps postpone the refactoring until we are done with that?

@beorn7 absolutely. The two other patches have precedence.

beorn7 · 2019-02-06T19:45:45Z

In case I sounded too negative: I like the refactoring in general. :o)

beorn7 · 2019-02-19T15:06:09Z

I think it's now “safe” to continue with this refactoring. Perhaps you should sync with @stuartnelson3 about basic ideas what should be in main and what should be pushed into packages, cf. his comment.

I'll now work on implementing my ideas about making the silence and inhibition checks less expensive. But that should not touch the configuration logic too much.

beorn7

Looks good to me. But the AM experts should have their say.

beorn7 · 2019-02-20T16:50:31Z

config/coordinator.go

+}
+
+// NewCoordinator returns a new coordinator with the given configuration file
+// path.


Perhaps mention that the config file is not loaded yet.

Good point. Added.

beorn7 · 2019-02-20T16:51:10Z

config/coordinator.go

+
+// Coordinator coordinates Alertmanager configurations beyond the lifetime of a
+// single configuration.
+// TODO: Make Coordinator thread safe


Is that necessary? Isn't it the advantage of the coordinator with the subscription model that you don't need that anymore?

Say there are two reloads triggered via the API, this would require write access to config *Config in a concurrent fassion. That should be synchronized, right?

Yes. "Make Coordinator thread safe" made me think you want to somehow enable concurrent reloads.

I think we either can protect the Reload method by a mutex, or we can document that Reload must only be called from a single goroutine. (I could imagine the latter is naturally the case, but if not, a mutex might be the most convenient solution.) If that's what you meant anyway, I'd suggest to move this TODO to the Reload method, reworded as "protect this method with a mutex" and add to the doc comment (for now) that Reload is not concurrency-safe.

Given that configuration reloading is not a super time sensitive thing and does not need to be parallelized I added Mutex to protect subscribers and config. Let me know what you think.

beorn7 · 2019-02-20T16:58:12Z

The failed CircleCI check succeeded when I reran it. Flaky test...

simonpasquier · 2019-02-21T14:44:38Z

config/coordinator.go

+	"time"
+
+	"github.com/prometheus/client_golang/prometheus"
+


(nit) can be removed.

simonpasquier · 2019-02-21T14:47:52Z

config/coordinator.go

+		"file", c.cfgFilePath,
+	)
+
+	conf, plainCFG, err := LoadFile(c.cfgFilePath)


(nit) s/plainCFG/plainCfg/

In general, Go would use plainCFG see: https://github.com/golang/go/wiki/CodeReviewComments#initialisms.

I decided to replace all cfg with config to stay consistent with config.go, let me know is you think differently.

Hmm, I wouldn't consider CFG as an initialism or acronym but I'm not a linguistic expert :-)

simonpasquier · 2019-02-21T14:59:02Z

cmd/alertmanager/main.go

 		tmpl, err = template.FromGlobs(conf.Templates...)
 		if err != nil {
-			return err
+			level.Error(logger).Log("err", fmt.Errorf("failed to parse templates: %v", err.Error()))


If we enter this branch, the error wouldn't bubble up anymore. Is that intended? Wouldn't it make sense for Coordinator.Subscribe to accept func(*Config) error?

Right, that got me thinking as well. If Coordinator accepts func(*Config) error, it would need to handle errors returned by other components logic. What should it do with such error? It can't know what kind of errors are possible, hence it doesn't know how to handle them, right? How would you handle the errors in the Coordinator code?

In the Prometheus code, we apply all "reloaders" (equivalent to subscribers), log an error message for any that fails and return a generic error message tot the caller in case one reloader failed.

https://github.com/prometheus/prometheus/blob/e4a741cb7dfa24978097971c2c8d5c8cc12e6cef/cmd/prometheus/main.go#L714-L744

Ok, good point. Did the relevant changes. Instead of returning a generic error message, I am returning and logging a specific one. Let me know if that is ok for you. Should be better for debugging.

beorn7

LGTM. But as I'm not the domain expert for AM, let's wait for another approve?

stuartnelson3 · 2019-02-24T13:24:50Z

config/coordinator.go

+	}
+
+	err = c.notifySubscribers()
+	if err != nil {


any reason for not having the if err := foo.Thing(); err != nil pattern?

Oh, right. That looks better. Adjusted above as well.

Instead of handling all config specific logic inside Alertmangaer.main(), this patch introduces the config coordinator component. Tasks of the config coordinator: - Load and parse configuration - Notify subscribers on configuration changes - Register and manage configuration specific metrics Signed-off-by: Max Leonard Inden <IndenML@gmail.com>

mxinden · 2019-02-25T11:01:19Z

Given that you both approved I will merge. Let me know if you want me to do any follow ups.

simonpasquier self-requested a review February 7, 2019 15:57

beorn7 mentioned this pull request Feb 15, 2019

Introduce concurrency limit for GET requests and a general timeout for HTTP #1743

Merged

mxinden force-pushed the introduce-config-coordinator branch from 738bd85 to b5f772f Compare February 20, 2019 12:46

mxinden changed the title ~~[WIP] *: Introduce config coordinator bundling config specific logic~~ *: Introduce config coordinator bundling config specific logic Feb 20, 2019

mxinden requested review from stuartnelson3 and beorn7 February 20, 2019 12:46

mxinden force-pushed the introduce-config-coordinator branch from b5f772f to a795818 Compare February 20, 2019 12:49

beorn7 reviewed Feb 20, 2019

View reviewed changes

simonpasquier reviewed Feb 21, 2019

View reviewed changes

mxinden force-pushed the introduce-config-coordinator branch 3 times, most recently from 8f2bce0 to 6a94cba Compare February 22, 2019 15:00

beorn7 approved these changes Feb 22, 2019

View reviewed changes

stuartnelson3 approved these changes Feb 24, 2019

View reviewed changes

mxinden force-pushed the introduce-config-coordinator branch from 6a94cba to d0cd5a0 Compare February 25, 2019 10:26

mxinden merged commit 6d55530 into prometheus:master Feb 25, 2019

simonpasquier mentioned this pull request Feb 26, 2019

No metrics for failed notifications due to a non-existent receiver #1759

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Introduce config coordinator bundling config specific logic #1744

*: Introduce config coordinator bundling config specific logic #1744

mxinden commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

mxinden commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 19, 2019

beorn7 left a comment

beorn7 Feb 20, 2019

mxinden Feb 22, 2019

beorn7 Feb 20, 2019

mxinden Feb 22, 2019

beorn7 Feb 22, 2019

mxinden Feb 22, 2019

beorn7 commented Feb 20, 2019

simonpasquier Feb 21, 2019

simonpasquier Feb 21, 2019

mxinden Feb 22, 2019

simonpasquier Feb 22, 2019

simonpasquier Feb 21, 2019

mxinden Feb 22, 2019

simonpasquier Feb 22, 2019

mxinden Feb 22, 2019

beorn7 left a comment

stuartnelson3 Feb 24, 2019

mxinden Feb 25, 2019

mxinden commented Feb 25, 2019

*: Introduce config coordinator bundling config specific logic #1744

*: Introduce config coordinator bundling config specific logic #1744

Conversation

mxinden commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 6, 2019

mxinden commented Feb 6, 2019

beorn7 commented Feb 6, 2019

beorn7 commented Feb 19, 2019

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 commented Feb 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

beorn7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Feb 25, 2019