Leader election for Prometheus HA setup #37

niksajakovljevic · 2018-09-06T12:11:20Z

In HA setup two or more Prometheus instances scrape the same data and send it to adapters.
We want to prevent multiple adapters to write the same/similar data (no consistency guarantees in HA Prometheus setup) in parallel. The idea is to use leader election and allow only one adapter (leader) to write to the database. Provided leader election implementation relies on PostgreSQL advisory locks and does not provide strong data guarantees (some duplicates or data loss possible during failover). REST interface for leader election should enable plugging in any external leader election system you might use already (eg. Zookeeper, Consul...)

RobAtticus · 2018-09-06T14:43:50Z

postgresql/client.go

@@ -64,7 +64,7 @@ func ParseFlags(cfg *Config) *Config {

 // Client sends Prometheus samples to PostgreSQL
 type Client struct {
-	db  *sql.DB
+	Db  *sql.DB


i think the go linter prefers this to be DB instead of Db

util/election.go

RobAtticus · 2018-09-06T14:46:15Z

util/lock.go

+	connCheckInterval = time.Millisecond * 250
+)
+
+// One implementation of leader election protocol based on PostgreSQL advisory locks. All adapters withing a HA group are trying


Same godoc thing as above

RobAtticus · 2018-09-07T16:47:59Z

Let's start using the same standards for git commit messages as we do for main repo, so basically squash all the commits being merging.

Also maybe change the first line to Add support for leader election...?

svenklemm · 2018-09-10T14:33:21Z

util/lock.go

+		}
+		l.conn = conn
+	}
+	rows, err := l.conn.QueryContext(context.Background(), fmt.Sprintf("SELECT pg_try_advisory_lock(%d)", l.groupLockId))


Using sprintf for creating sql statements should be avoided. You should use something like the following:

QueryContext(context.Background(),"SELECT pg_try_advisory_lock(?)", l.groupLockId)

For some reason pq driver does not support that syntax when invoking SQL functions. I tried even SELECT pg_try_advisory_lock(key => $1) but no luck

I think correct syntax is SELECT pg_try_advisory_lock($1) which works for me

LeeHampton · 2018-09-10T14:51:48Z

util/lock.go

+func (l *PgAdvisoryLock) TryLock() (bool, error) {
+	l.mutex.Lock()
+	if l.closed {
+		return false, fmt.Errorf("lock is closed. please create a new lock")


Since you define the defer that unlocks l.mutex below this return, is it possible that a deadlock might be reached, or am I missing something?

Great catch! Although if the lock is closed we return an error but repeated invocation can cause the deadlock

amytai

I'm not super familiar with this codebase, but is there any way to write an regression test with 2 clients that are actually running the leader election on different processes? And making sure that 1) only one is the leader, 2) that there is proper failover when one of the clients dies / releases the lock?

amytai · 2018-09-10T16:53:27Z

util/lock.go

+// to metrics that is written (some duplicates or data loss are possible during failover)
+type PgAdvisoryLock struct {
+	conn     *sql.Conn
+	mutex    sync.RWMutex


I'm actually not sure if this mutex is doing anything. It's not like a single PgAdvisoryLock struct is going to be shared across multiple clients? So you don't need to serialize access to this struct because it's only ever access in a single-threaded manner by the client that instantiated it..?

Actually we have one lock per app and it is being shared among different go routines(http requests)

Ah, OK, this was my confusion with the test as well. I saw the test that you wrote, but that wasn't what I had in mind, because I thought you would want to test when clients are in completely different processes. But I guess that's not how the adapter is executed. I'm still not seeing how a single PgAdvisoryLock is shared across (requests?) but maybe that is code that is outside of this PR. I'll ask you offline.

IMHO having separate processes would not bring any benefit to testing but rather increase test complexity/fragility a lot. PgAdvisoryLock is being shared among go routines which are created on each request: https://golang.org/pkg/net/http/#Server.Serve

amytai · 2018-09-10T16:55:57Z

util/lock.go

+}
+
+func (l *PgAdvisoryLock) Release() error {
+	l.mutex.Lock()


Do you want to check that Assert(obtained)? Otherwise another client could accidentally (or maliciously) unlock the lock?

In that case we would return an error but I agree that it's a good practice to check the boolean state first.

niksajakovljevic · 2018-09-10T21:38:37Z

@amytai you can find an integration test here https://github.com/timescale/prometheus-postgresql-adapter/pull/37/files#diff-df609cea8c6beef224fb42b994af370bR122

erimatnor

I think this implementation would benefit from separating the two concerns of:

Setting the "leader"
Picking a leader, where using advisory locks is one particular way to set 1

Now you have a configuration parameter to set read-only mode, but no way to promote the instance at runtime to write mode. If you would make this possible, then the advisory lock stuff is just a way to toggle read vs write mode, while you could also plug in other ways to set read/write mode (preferably by external means).

The problem I have with this implementation is that the read-only mode and leader-election with advisory locks seem to be two disjoint mechanisms, while the latter, IMO, should just be a way to set or unset the former.

erimatnor · 2018-09-11T07:04:03Z

main.go

-	err := w.Write(samples)
+	shouldWrite := true
+	if election != nil {
+		isLeader, err := election.IsLeader()


isLeader seems redundant. Can't you just use shouldWrite here?

Sure, again I did it only to make it easier understanding what's going on. I am not strongly opinionated on this one, so we can remove that extra var?

Was actually confusing to me to see both, since I first tried to figure out the difference between the two variables, given their names. But this might be a personal thing.

I'm with Erik, I having both is more confusing.

erimatnor · 2018-09-11T07:15:25Z

util/lock.go

+	connCheckInterval = time.Millisecond * 250
+)
+
+// PgAdvisoryLock is implementation of leader election protocol based on PostgreSQL advisory locks. All adapters withing a HA group are trying


While not strictly wrong, I think calling this a "leader election protocol" is a bit of a stretch as I think most people would associate this with some kind of distributed algorithm (e.g., quorum-based).

I have a feeling that locks are pretty common way to coordinate and implement leader election (Consul, Zookeeper), however I am open for better wording if you have some idea?

Yes, but the point is that lock services like Zookeeper and Consul both implement a distributed leader election protocol (such as Raft or Paxos) among their respective groups/ensembles. That's what is typically meant by a "leader election protocol", specifically. I guess what trips me up a bit is the use of "protocol". Just removing "protocol" would lessen the confusion I think, e.g.,. PgAdvisoryLock is an implementation of leader election using PostgreSQL advisory locks.

erimatnor · 2018-09-11T07:19:30Z

util/lock.go

+	}
+	defer rows.Close()
+	if !rows.Next() {
+		return false, fmt.Errorf("error while trying to read reponse rows from `pg_try_advisory_lock` function")


reponse -> response

erimatnor · 2018-09-11T07:20:27Z

util/lock.go

+		return false, fmt.Errorf("error while trying to read reponse rows from `pg_try_advisory_lock` function")
+	}
+	var success bool
+	if err := rows.Scan(&success); err != nil {


can't you just pass &l.obtained to Scan? This avoids the extra variable.

Sure, I just thought that using extra var makes it more readable.

erimatnor · 2018-09-11T07:36:26Z

util/lock.go

+	return nil
+}
+
+func (l *PgAdvisoryLock) connCheck() {


Did you consider using the Listener functionality in pq instead of doing polling? It seems you can get an event notification when disconnected.

https://godoc.org/github.com/lib/pq#NewListener

Yes I did consider it, but ruled it out as not a good fit for this use case since we only need a simple health-check. Also Listener creates a new DB connection dedicated to LISTEN/NOTIFY where we only need to check the existing connection.

erimatnor · 2018-09-11T07:40:32Z

util/lock.go

+}
+
+func checkConnection(conn *sql.Conn) error {
+	_, err := conn.ExecContext(context.Background(), "SELECT 1")


At least newer versions of the sql lib has a PingContext to verify database connections. Should probably consider using that.

https://golang.org/pkg/database/sql/#Conn.PingContext

I thought about it, but apparently that's not supported by pq driver: lib/pq#533

niksajakovljevic · 2018-09-11T14:34:52Z

@erimatnor I fully understand your concerns. As we discussed the semantics of read.only is a bit different so I would keep it separate. I added additional Election implementation that provides REST interface so one can plug in any external leader election mechanism (eg. Zookeeper, Consul..)

amytai · 2018-09-11T15:26:18Z

postgresql/client_test.go

+	if !lock.Locked() {
+		t.Error("Couldn't obtain the lock")
+	}
+


Actually, now that I'm looking at this again, I think it's fine to leave out the comment I suggested. This logic tests that the PgAdvisoryLock works on the Postgres-side, which is a valid micro-test.

Instead, I would be happier if there were a test that instantiates an election and tests it with two "clients" / requests (even though these requests might be sequential). Does that make sense? Or maybe I am once again misunderstanding what's going on in Go.
It looks like you test the RestElection below, but what about the regular election you had implemented?

erimatnor · 2018-09-12T11:02:46Z

main.go

@@ -137,7 +147,8 @@ func parseFlags() *config {
 	flag.StringVar(&cfg.telemetryPath, "web.telemetry-path", "/metrics", "Address to listen on for web endpoints.")
 	flag.StringVar(&cfg.logLevel, "log.level", "debug", "The log level to use [ \"error\", \"warn\", \"info\", \"debug\" ].")
 	flag.BoolVar(&cfg.readOnly, "read.only", false, "Read-only mode. Don't write to database.")
-
+	flag.IntVar(&cfg.haGroupLockId, "ha.group-advisory-lock-id", 0, "Unique advisory lock id per adapter high-availability group. Set it if you want to use leader election implementation based on PostgreSQL advisory lock")
+	flag.BoolVar(&cfg.restElection, "rest.election", false, "Enable REST interface for the leader election")


I would group this parameter with the above one under a common prefix, e.g., leader-election.http and leader-election.advisory-lock or something. Then it is clear that these are parameter related to similar functionality.

Also, just wondering if it should be possible to have both enabled simultaneously? If not, I could imagine having one string parameter that allows leader-election=http, leader-election=lock, leader-election=off or similar.

erimatnor · 2018-09-12T11:06:07Z

main.go

-	err := w.Write(samples)
+	shouldWrite := true
+	if election != nil {
+		isLeader, err := election.IsLeader()


Was actually confusing to me to see both, since I first tried to figure out the difference between the two variables, given their names. But this might be a personal thing.

erimatnor · 2018-09-12T11:07:42Z

main.go

+				log.Error("msg", "Error occurred while trying to become a leader", "err", err)
+				return err
+			}
+			shouldWrite = isLeader


again, seems strange that this is set in both if and else case instead of outside.

I've eliminated isLeader as it seem that it doesn't make code more readable :)

erimatnor · 2018-09-12T11:15:35Z

util/election.go

+		return r.leader, nil
+	}
+	r.leader = true
+	log.Info("msg", "Instance became a leader")


FWIW, I would wrap the implementations of this interface in a higher-level object that does the logging for any implementation. Alternatively, just log where called. Then the logging is consistent across implementations.

For instance, you could have an Elector type that calls into an Election implementation and does the logging depending on return values from the specific implementation. So, if BecomeLeader() returns true, then the Elector logs "Instance became a leader"

erimatnor · 2018-09-12T11:27:09Z

util/lock.go

+	connCheckInterval = time.Millisecond * 250
+)
+
+// PgAdvisoryLock is implementation of leader election protocol based on PostgreSQL advisory locks. All adapters withing a HA group are trying


Yes, but the point is that lock services like Zookeeper and Consul both implement a distributed leader election protocol (such as Raft or Paxos) among their respective groups/ensembles. That's what is typically meant by a "leader election protocol", specifically. I guess what trips me up a bit is the use of "protocol". Just removing "protocol" would lessen the confusion I think, e.g.,. PgAdvisoryLock is an implementation of leader election using PostgreSQL advisory locks.

erimatnor · 2018-09-12T11:31:33Z

util/election.go

+		log.Debug("msg", "Instance is already a leader")
+		return r.leader, nil
+	}
+	r.leader = true


This seems weird. In the write path, you call BecomeLeader, which for this implementation will always succeed if the instance is not the leader. I would have expected that when and how to become a leader is implementation specific and not exposed/triggered on the write path. (See note also on write path code).

Agree. Removed election. BecomeLeader() from write path.

erimatnor · 2018-09-12T11:37:23Z

main.go

+		if isLeader {
+			shouldWrite = isLeader
+		} else {
+			isLeader, err := election.BecomeLeader()


It seems strange to me that "election" happens on every write in case you are not the leader. When and how to become a leader seems very implementation specific and, IMO, shouldn't be happening here. This seems wrong w.r.t., e.g., rest-based election.

I would expect the write path to simply check election.IsLeader(). A particular implementation (i.e.,. the advisory lock one) can try to become leader in the implementation of that check if it makes sense for that implementation.

So, IMO it doesn't make sense for, e.g., a REST- or Zookeeper-implementation of leader election to try and become leader here. For most implementations that happens in a side channel based on other events.

Maybe it even makes sense to split the Election interface into a read and write interface, thus only exposing the read interface here (i.e., only the IsLeader() function)?

Agree. The reason I had it here in the first place was once I implemented PgAdvisoryLock I wanted to try to obtain the lock as soon as possible (when write req happens), as opposite of having some scheduled routine trying to obtain the lock on some fixed time window. I guess that in the context of other implementations, eg. REST, that doesn't make much sense. I will perform election.IsLeader() check only and hide PgAdvisoryLock specifics.

Maybe you can do leader checks in the health-check go-routine? That makes sense to me since you are already polling there.

erimatnor · 2018-09-12T11:41:56Z

util/election.go

+				return
+			}
+			fmt.Fprintf(response, "%v", leader)
+		case http.MethodPost:


Seems a bit weird to POST something without content. Would have expected a PUT (POST typically to create a resource) where the content is the same type as what you return in the GET.

So, you, e.g., PUT a value of 1 to /admin/election/leader to become leader and a 0 to resign. A GET simply returns the current value. No resign path needed.

Otherwise it is not really a "RESTful" API.

JLockerman · 2018-09-12T15:45:05Z

util/election.go

+	defer r.mutex.Unlock()
+	if !r.leader {
+		log.Debug("msg", "Can't resign when not a leader")
+		return nil


Should this path return some information to the client so it knows it's telling a non-leader to resign?

I've increased log level to warn so the one inspecting the logs should notice a problem. I am not sure if we should return an error because from our perspective the system is in proper state, however I am not strongly opinionated here...

JLockerman · 2018-09-12T15:50:59Z

util/lock.go

+			}
+		case <-l.quit:
+			l.mutex.Lock()
+			l.closed = true


When does this release the advisory lock if we're the leader?

The lock is being released on Close(). This method is called internally

JLockerman · 2018-09-12T15:52:57Z

main.go

-	err := w.Write(samples)
+	shouldWrite := true
+	if election != nil {
+		isLeader, err := election.IsLeader()


I'm with Erik, I having both is more confusing.

erimatnor · 2018-09-13T13:56:45Z

util/lock.go

+	if l.obtained {
+		return l.obtained, nil
+	}
+	err := checkConnection(l.conn)


This check is not needed since obtained is false and l.conn doesn't have a lock. It should be enough to just grab any connection from the pool.

erimatnor

One minor nit.

erimatnor · 2018-09-14T12:08:17Z

util/election.go

+func (e *Elector) scheduledElection() {
+	for {
+		select {
+		case <-e.ticker.C:


should there be an exit case for this loop?

In HA setup two or more Prometheus instances scrape the same data and send it to adapters. We want to prevent multiple adapters to write the same/similar data (no consistency guarantees in HA Prometheus setup) in parallel. The idea is to use leader election and allow only one adapter (leader) to write to the database. Provided leader election implementation relies on PostgreSQL advisory locks and does not provide strong data guarantess (some duplicates or data loss possible during failover - btw Prometheus HA doesn't provide any guarantees that multiple Prometheus nodes would see the same data). REST interface for leader election should enable plugging in any external leader election system you might use already (eg. Zookeeper)

niksajakovljevic requested review from erimatnor, RobAtticus, davidkohn88, amytai, JLockerman, svenklemm and cevian September 6, 2018 12:12

RobAtticus suggested changes Sep 6, 2018

View reviewed changes

niksajakovljevic force-pushed the ha-leader-election branch from 13a8f10 to d6c1747 Compare September 10, 2018 10:28

svenklemm suggested changes Sep 10, 2018

View reviewed changes

LeeHampton reviewed Sep 10, 2018

View reviewed changes

amytai reviewed Sep 10, 2018

View reviewed changes

niksajakovljevic force-pushed the ha-leader-election branch from 9f05580 to 47192fd Compare September 10, 2018 22:01

LeeHampton approved these changes Sep 10, 2018

View reviewed changes

erimatnor suggested changes Sep 11, 2018

View reviewed changes

niksajakovljevic force-pushed the ha-leader-election branch 2 times, most recently from 1198aa2 to 564d9ca Compare September 11, 2018 13:55

niksajakovljevic force-pushed the ha-leader-election branch from f5b4c4e to d41f168 Compare September 11, 2018 14:39

amytai reviewed Sep 11, 2018

View reviewed changes

erimatnor suggested changes Sep 12, 2018

View reviewed changes

JLockerman reviewed Sep 12, 2018

View reviewed changes

niksajakovljevic force-pushed the ha-leader-election branch 2 times, most recently from fec05d3 to 194f4c0 Compare September 13, 2018 09:27

erimatnor reviewed Sep 13, 2018

View reviewed changes

erimatnor approved these changes Sep 14, 2018

View reviewed changes

niksajakovljevic force-pushed the ha-leader-election branch from 40631d0 to 91d54fc Compare September 14, 2018 12:26

niksajakovljevic merged commit e27a36a into timescale:master Sep 14, 2018

Leader election for Prometheus HA setup #37

Leader election for Prometheus HA setup #37

Conversation

niksajakovljevic commented Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobAtticus commented Sep 7, 2018

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

amytai 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

niksajakovljevic commented Sep 10, 2018

erimatnor left a comment • edited Loading

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

erimatnor Sep 12, 2018 • edited Loading

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

niksajakovljevic Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niksajakovljevic commented Sep 11, 2018

Choose a reason for hiding this comment

erimatnor Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erimatnor Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

erimatnor Sep 12, 2018 • edited Loading

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

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

erimatnor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niksajakovljevic commented Sep 6, 2018 •

edited

Loading

erimatnor left a comment •

edited

Loading

erimatnor Sep 12, 2018 •

edited

Loading

niksajakovljevic Sep 11, 2018 •

edited

Loading

erimatnor Sep 12, 2018 •

edited

Loading

erimatnor Sep 12, 2018 •

edited

Loading

erimatnor Sep 12, 2018 •

edited

Loading

erimatnor Sep 12, 2018 •

edited

Loading