When old master restores, it should be marked as unhealthy

Related to https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2571

After https://gitlab.com/gitlab-org/omnibus-gitlab/merge_requests/1800

Few times I've hit a case where an old master returning from being down still gets marked as master. This leads to having multiple masters and causes issues. Good thing is that we automatically stop pgbouncer from running when we see more than one master so rule out any damage. The bad thing is that this basically causes downtime. We should look whether we can mark an old master unhealthy when it returns to rotation.

/cc @ibaum @jtevnan

mentioned in merge request !1800 (merged)

marked this issue as related to #2571

mentioned in issue #2696 (closed)

So currently, the script to check if a node is the master runs the query SELECT name FROM repmgr_gitlab_cluster.repl_nodes WHERE type='master' AND active != 'f' against the gitlab_repmgr db.

It does look like if a failed master is brought back online, the active column does not get set to t. So while gitlab-ctl repmgr cluster show will show two masters, there will in fact be only one.

I'll triple check to confirm that the script works as we expect, then make a note on the documentation that this is the behavior users can expect to see.

Right, it doesn't work, because the old master doesn't have the updated database when it comes back online.

Output from gitlab-ctl repmgr cluster show

On a failed master with postgresql running:

Role      | Name     | Upstream | Connection String
----------+----------|----------|----------------------------------------------------------------
* master  | db_one   |          | host=db_one port=5432 user=gitlab_repmgr dbname=gitlab_repmgr
* master  | db_two   | db_one   | host=db_two port=5432 user=gitlab_repmgr dbname=gitlab_repmgr
  standby | db_three | db_one   | host=db_three port=5432 user=gitlab_repmgr dbname=gitlab_repmgr

On the new master:

Role      | Name     | Upstream | Connection String
----------+----------|----------|----------------------------------------------------------------
* master  | db_one   |          | host=db_one port=5432 user=gitlab_repmgr dbname=gitlab_repmgr
* master  | db_two   |          | host=db_two port=5432 user=gitlab_repmgr dbname=gitlab_repmgr
  standby | db_three | db_two   | host=db_three port=5432 user=gitlab_repmgr dbname=gitlab_repmgr

So the old master sees that the new node is now a master, but doesn't have that information stored locally.

Contents of repmgr_gitlab_cluster.repl_nodes

On the failed master (or, a totally healthy cluster)

gitlab_repmgr=# select * from repmgr_gitlab_cluster.repl_nodes;
 id |  type   | upstream_node_id |    cluster     |   name   |                            conninfo                             | slot_name | priority | active

----+---------+------------------+----------------+----------+-----------------------------------------------------------------+-----------+----------+-------
-
  1 | master  |                  | gitlab_cluster | db_one   | host=db_one port=5432 user=gitlab_repmgr dbname=gitlab_repmgr   |           |      100 | t
  2 | standby |                1 | gitlab_cluster | db_two   | host=db_two port=5432 user=gitlab_repmgr dbname=gitlab_repmgr   |           |      100 | t
  3 | standby |                1 | gitlab_cluster | db_three | host=db_three port=5432 user=gitlab_repmgr dbname=gitlab_repmgr |           |      100 | t
(3 rows)

On the new master

gitlab_repmgr=# select * from repmgr_gitlab_cluster.repl_nodes;
 id |  type   | upstream_node_id |    cluster     |   name   |                            conninfo                             | slot_name | priority | active

----+---------+------------------+----------------+----------+-----------------------------------------------------------------+-----------+----------+-------
-
  1 | master  |                  | gitlab_cluster | db_one   | host=db_one port=5432 user=gitlab_repmgr dbname=gitlab_repmgr   |           |      100 | f
  2 | master  |                  | gitlab_cluster | db_two   | host=db_two port=5432 user=gitlab_repmgr dbname=gitlab_repmgr   |           |      100 | t
  3 | standby |                2 | gitlab_cluster | db_three | host=db_three port=5432 user=gitlab_repmgr dbname=gitlab_repmgr |           |      100 | t
(3 rows)

So my goal is that each node is capable of independently determining whether it's the master or not.

Currently we're just basing that off of the local contents of repmgr_gitlab_cluster.repl_nodes.

I think if we add a check on the output from gitlab-ctl repmgr cluster show, that should do it. If it shows only one master, then good. If it shows two masters, and we see active = 't' for both, then it can't be a master.

So now I'm thinking repmgr's event notification may be the way to go on this.

We could setup a script to unregister the old master on the standby_promote event.

/cc @_stark

marked this issue as related to #2776 (closed)

mentioned in issue #2776 (closed)

created branch 2697-when-old-master-restores-it-should-be-marked-as-unhealthy

mentioned in merge request !1929

So in thinking about this, it definitely isn't an easy problem. I still think event notification is the way to go, but it does leave us with a similar problem for failover. Who is responsible for removing, and how do we ensure it happens.

I think adding another watcher to the db nodes consul agents is the way to go. When the repmgr event occurs, we write an entry in the consul k/v store. The watcher on the remaining db nodes will see the new value, try for a lock, delete the node from the cluster (and the consul service) and then delete the value when successful. If it fails, release the lock but don't delete the value. All operations should be idempotent.

I think having all nodes try and write to the k/v is the only loophole here. It's tough to guarantee that at least one will succeed. But since they write to the local consul agent, I think it is an acceptable risk.

Just remembered I mentioned this on the call and planned to drop this note in the issue but forgot:

We currently cannot use pg_rewind in production because we don't have checksums enabled and don't have wal_log_hints enabled. To my surprise checksums are actually disabled on even new installs (at least looking at the GDK database).

I would suggest we should enabl enable checksums on new databases. This will increase the volume of wal generated but to me it seems supporting many client installs on hardware we don't control is exactly the kind of situation where checksums are most valuable. The busiest instances where the extra wal volume would hurt the most are also exactly the instances where the extra costs would be easily justified.

And I would suggest we should enable wal_log_hints at least on databases where HA is set up. The benefit of being able to use pg_rewind if we wish to -- which I believe repmgr is capable of doing automatically -- seems compelling even if we don't make it the normal path.

As an aside I think Stephen Frost was suggesting relaxing our WAL settings to allow longer time between checkpoints. If we did the two at the same time that would probably more than make up the difference leaving users with less WAL overall (though not as much savings as they would have had without having wal_log_hints enabled).

@_stark Those topics are kind of separate from what we're trying to address here. I've opened separate issues for them #2819 and #2820

mentioned in issue #2819

mentioned in issue #2820

I think if we add a check on the output from gitlab-ctl repmgr cluster show, that should do it. If it shows only one master, then good. If it shows two masters, and we see active = 't' for both, then it can't be a master.

@ibaum is it possible for the failed master to detect this itself? The failed master boots itself up, checks whether t stands next to its name. If it has f, demote itself? This way we limit the responsibility to the node that is in inconsistent state and leave the rest of the cluster out of this decision.

@marin The data a failed master will have when it comes back online will be from when it was the active master. So unfortunately the other nodes do need to be involved.

In that case, https://gitlab.com/gitlab-org/omnibus-gitlab/issues/2697#note_40783608 is the only way to move forward.

mentioned in merge request !1980 (merged)

Played around a bit with the consul kv store today.

• On a repmgr promotion event, the remaining nodes should put into the kv something like gitlab/postgresql/$failed_node_name with no value. Consider looping until it succeeds. For a basic put, writing data which is already there is considered a success, so no harm in having all nodes try.

• We can then use a keyprefix watch on gitlab/postgresql

• Since there is no builtin repmgr command to remove a master, (see !1980 (merged)), the actual DELETE query needs to happen on the new master node. Need to do some testing on this. When a watch picks up an event, the new master should be ready, and we can execute the remove command there. But there should be some mechanism in place to have it retry on failure, it should not wait for a new event to occur.

mentioned in merge request !1987