Keeper priority by arssher · Pull Request #622 · sorintlab/stolon

arssher · 2019-03-11T13:06:54Z

Add --priority keeper option.

Sentinel will promote keeper with higher priority than the current one if this
is possible. In async mode this is a bit non-deterministic because we always
elect node with highest LSN, and under heavy load prioritized node might never
report LSN higher than its stronger competitors. However, if nodes are equal
this should happen at some moment. In sync mode, we can just elect any of
synchronous standbies.

Implements #492

While here, make eyeballing of sentinel tests failures easier.

arssher · 2019-03-12T09:49:37Z

semaphoreci failed at config_test.go:541 in one of the configurations, but I don't think this is related to this pull request -- seems like the database just couldn't restart in 10 seconds. Is there a way to restart the test?

rearden-steel · 2019-04-18T17:04:54Z

Any updates on this PR? Need this feature.

sgotti

@arssher Thanks for the PR and sorry for the delay. I haven't had time to deeply test it but it should be covered by the tests you added. Some comments inline.

sgotti · 2019-05-08T11:57:06Z


+// Return DB who can be new master. This function mostly takes care of
+// sync mode; in async case, new master is just first element of findBestNewMasters.
+func (s *Sentinel) findBestNewMaster(cd *cluster.ClusterData, curMasterDB *cluster.DB, logErrors bool) *cluster.DB {


findBestNewMaster returns the db who can be the new master. This function ....

sgotti · 2019-05-08T11:57:54Z

+		}
+		if bestNewMasterDB == nil {
+			if logErrors {
+				log.Errorw("cannot choose synchronous standby since there's not match between the possible masters and the usable synchronousStandbys", "reported", curMasterDB.Status.SynchronousStandbys, "spec", curMasterDB.Spec.SynchronousStandbys, "common", commonSyncStandbys, "possibleMasters", bestNewMasters)


This previously was a log.Warnf

Indeed, fixed.

sgotti · 2019-05-08T11:58:03Z

+		commonSyncStandbys := util.CommonElements(curMasterDB.Status.SynchronousStandbys, curMasterDB.Spec.SynchronousStandbys)
+		if len(commonSyncStandbys) == 0 {
+			if logErrors {
+				log.Errorw("cannot choose synchronous standby since there are no common elements between the latest master reported synchronous standbys and the db spec ones", "reported", curMasterDB.Status.SynchronousStandbys, "spec", curMasterDB.Spec.SynchronousStandbys)


This previously was a log.Warnf

Indeed, fixed.

sgotti · 2019-05-08T11:58:38Z

-					log.Infow("electing db as the new master", "db", bestNewMasterDB.UID, "keeper", bestNewMasterDB.Spec.KeeperUID)
+				// Even if current master is ok, we probably still
+				// want to change it if there is ready DB with higher
+				// keeper priority.


So the priority will be used also to changing primary also when there's no failure? This should probably be documented

Yes. I have added some words to --priority help describing this.

sgotti · 2019-05-08T12:01:12Z

 	}
-	return reflect.DeepEqual(cd1, cd2)
-
+	equal, diff := util.DeepEqualVerbose(cd1, cd2)


Since this change could be really useful (in other projects I also used github.com/google/go-cmp) to debug test failures but not strictly related to this feature, can you please remove from this PR and open a new dedicated PR?

Done so; extracted to #679 and excluded here.

dineshba · 2019-05-25T12:58:37Z

+		}
+		pi := cd.Keepers[dbs[i].Spec.KeeperUID].Status.Priority
+		pj := cd.Keepers[dbs[i].Spec.KeeperUID].Status.Priority
+		return pi < pj


In Line 700,

pj := cd.Keepers[dbs[i].Spec.KeeperUID].Status.Priority

replace dbs[i] to dbs[j]

Ups, good catch, thanks.

rearden-steel · 2019-06-26T18:09:27Z

@arssher can you update this PR?

arssher · 2019-06-26T18:58:58Z

Hi, I will try to look into this in a few days.

arssher · 2019-06-30T14:11:11Z

Sorry for the quite a delay. I have updated the PR:

Addressed comments above.
Rebased on current master.
Added facility to update keeper priority online (new command stolonctl setkeeperpriority). It allows to update priority without restarting the keeper (and underlying Postgres instance), which can be used for controlled failover. My colleague @maksm90 hinted me it would be a nice addition...

arssher · 2019-06-30T14:52:49Z

BTW, when generating docs (gen_commands_doc.sh) I have noticed that it wasn't updated for some lately added keeper options, e.g. pg-advertise-address. I have excluded those from this PR also as I believe they should go in separate commit.

Also, does it make sense to restamp all docs files with current date like "Auto generated by spf13/cobra on 30-Jun-2019"? I can revert them, leaving only ones who have really changed.

arssher · 2019-07-01T06:23:57Z

(Made a small cleanup just now, used DefaultPriority const and purged obsolete NotSpecifiedPriority const)

arssher · 2019-07-01T08:07:56Z

One more minor fix/cleanup: forgot to add generated doc for setkeeperpriority command and used DefaultPriority const in keeper help.

Last CI build failed on Consul and passed all other stores. Looking at logs, I think this PR is not a reason of the failure again. Error was at ha_test.go:1574 in TestKeeperRemovalStolonCtl. removekeeper failed with:

    utils.go:881: executing stolonctl, args: [--cluster-name=fbfeb889-37b5-402a-89c3-e9acdaf0e248 --store-backend=consul --store-endpoints=127.0.0.1:2278 removekeeper 7c99557a]
    utils.go:897: [stolonctl]: cannot update cluster data: Unable to complete atomic operation, key modified
    ha_test.go:1576: unexpected err: exit status 1

So most probably stolonctl got interleaved with sentinel's clusterdata update.

arssher · 2019-07-01T10:47:08Z

Hm, now two builds failed. One of them is exactly as previous: TestKeeperRemovalStolonCtl at ha_test.go:1576.

Another is TestForceFailSyncReplStandbyCluster, at
ha_test.go:1893: expected master "1a1cd8ab" in cluster view
Which means sentinel failed to re-elect must after forcefail:

    utils.go:256: [sentinel dc41d2d6]: 2019-07-01T08:16:13.737Z	[34mINFO[0m	cmd/sentinel.go:1027	master db is failed	{"db": "1c4f397a", "keeper": "4a4c481b"}
    utils.go:256: [sentinel dc41d2d6]: 2019-07-01T08:16:13.738Z	[31mERROR[0m	cmd/sentinel.go:768	no eligible masters

It is unclear to me why this happened. Since I didn't touch findBestNewMasters function, most probably it is not me, but still... can we rerun the tests with debug log level enabled to see what's happening inside findBestNewMasters?

rearden-steel · 2019-09-05T17:23:46Z

@sgotti would you please check this PR?

rearden-steel · 2020-02-13T16:58:55Z

@sgotti Any updates on this?

sgotti · 2020-02-14T09:28:12Z

@rearden-steel this PR requires an update/rebase and we have to understand if this will cover also #696. Are you willing to take care of this PR? @lawrencejones @maksm90

Sentinel will promote keeper with higher priority than the current one if this is possible. In async mode this is a bit non-deterministic because we always elect node with highest LSN, and under heavy load prioritized node might never report LSN higher than its stronger competitors. However, if nodes are equal this should happen at some moment. In sync mode, we can just elect any of synchronous standbies. Priority can be set during keeper start (--priority) or later with new command 'stolonctl set keeperpriority'. The latter allows to update priority without restarting the keeper (and its Postgres instance), which can be used for controlled failover. Implements sorintlab#492

ololobus · 2020-07-10T12:51:58Z

@sgotti, I've rebased this branch. I had to modify one place because of a linter error, now all existing tests pass and

make test
INTEGRATION=1 STOLON_TEST_STORE_BACKEND=etcdv3 ./test

works well for me. As for CI, I've found it to be quite unstable these days: first it failed on build / make test with zero output; after re-pushing the brach with --force without changes all tests passed; now, I modified only a single comment and TestProxyListening failed with

TestProxyListening: utils.go:1090: err: context deadline exceeded
TestProxyListening: proxy_test.go:164: error waiting on store up: timeout
TestProxyListening: utils.go:300: stopping etcd e82d334a

which seems to be not a matter of this PR and means that etcd just failed to start before timeout fired, doesn't it?

BTW, in the #696 you mentioned that it worth to check compatibility of this PR with 05b1b0f. I had a look on it, and it seems that everything works fine even without changes, since added in this PR findBestNewMaster uses findBestNewMasters, which simply skips master candidates with --can-be-master. That way, a keeper with both --priority=0 and --can-be-master=false wouldn't be promoted.

However, it may be a bit misleading for a user, so maybe we should restrict usages of both --priority=0 and --can-be-master=false simultaneously? What do you think, @sgotti @rearden-steel @arssher?

sgotti requested changes May 8, 2019

View reviewed changes

dineshba reviewed May 25, 2019

View reviewed changes

arssher mentioned this pull request Jun 30, 2019

Print out diff in sentinel test if it fails. #679

Open

arssher force-pushed the keeper_priority branch from a3b812a to 0cce710 Compare June 30, 2019 14:07

arssher force-pushed the keeper_priority branch from 0cce710 to f39a2bd Compare June 30, 2019 14:19

arssher force-pushed the keeper_priority branch from f39a2bd to 50601c5 Compare July 1, 2019 06:14

arssher force-pushed the keeper_priority branch from 50601c5 to b9f2b74 Compare July 1, 2019 07:50

maksm90 mentioned this pull request Oct 1, 2019

[RFE] Support keepers that will never become master/sync #696

Closed

ololobus force-pushed the keeper_priority branch 2 times, most recently from 6b7fdfb to ef591f2 Compare July 9, 2020 19:45

arssher and others added 2 commits July 10, 2020 13:02

Regenerate docs for keeper --priority option.

988568e

ololobus force-pushed the keeper_priority branch from ef591f2 to 988568e Compare July 10, 2020 10:02

Conversation

arssher commented Mar 11, 2019

Uh oh!

arssher commented Mar 12, 2019

Uh oh!

rearden-steel commented Apr 18, 2019

Uh oh!

sgotti left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rearden-steel commented Jun 26, 2019

Uh oh!

arssher commented Jun 26, 2019

Uh oh!

arssher commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arssher commented Jun 30, 2019

Uh oh!

arssher commented Jul 1, 2019

Uh oh!

arssher commented Jul 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arssher commented Jul 1, 2019

Uh oh!

rearden-steel commented Sep 5, 2019

Uh oh!

rearden-steel commented Feb 13, 2020

Uh oh!

sgotti commented Feb 14, 2020

Uh oh!

ololobus commented Jul 10, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

arssher commented Jun 30, 2019 •

edited

Loading

arssher commented Jul 1, 2019 •

edited

Loading