feat: Manager switching upon quorum loss #155

WithSoull · 2025-01-31T09:58:27Z

When manager lost connection, it lost quorum, than we need to make switchover

Todo`s:

Make switchover manager to any available host
Make feature tests
Make additonal logic of switching to some host, which is the nearest to master
Make feature tests again

noname0443 · 2025-01-31T10:14:19Z

internal/app/app.go

@@ -42,6 +42,8 @@ type App struct {
 	switchHelper        mysql.ISwitchHelper
 }

+var lostQuorumTime time.Time


All usage places of the variable are in App methods; I suppose it will beneficial to place it in either mysql.Cluster or App. Could we do that?

noname0443 · 2025-01-31T10:32:27Z

internal/config/config.go

@@ -66,6 +66,8 @@ type Config struct {
 	InfoFileHandlerInterval                 time.Duration                `config:"info_file_handler_interval" yaml:"info_file_handler_interval"`
 	RecoveryCheckInterval                   time.Duration                `config:"recoverycheck_interval" yaml:"recoverycheck_interval"`
 	ExternalCAFileCheckInterval             time.Duration                `config:"external_ca_file_check_interval" yaml:"external_ca_file_check_interval"`
+	WaitingRecoveryNetworkTimestamp         time.Duration                `config:"waiting_recovery_network_timestamp" yaml: "waiting_recovery_network_timestamp"`
+	DoNotAcquireLockByDisconnectedManager   time.Duration                `config:"do_not_acquire_lock_by_disconnected_manager" yaml:"do_not_acquire_lock_by_disconnected_manager"`


I believe it's more clear and understandable to use positive names, and the variable could sound like a boolean one. Could we please rename it to something like ManagerElectionDelayAfterQuorumLoss or ManagerLockAcquireDelayAfterQuorumLoss?

Ok, I will rename this variables

noname0443 · 2025-01-31T11:17:41Z

internal/app/app.go

+
+	lostQuorumDuration := time.Now().Sub(lostQuorumTime)
+	if lostQuorumDuration < app.config.WaitingRecoveryNetworkTimestamp {
+		app.logger.Debug("manager try to acquire lock")


What do you think about providing more information regarding the quorum situation in this line? When we have a non-zero lostQuorumDuration, it might be helpful to highlight that in the application logs. I suggest adding something like In the period of lost quorum <DURATION> as a suffix to the logger text, as I believe it would make the situation more clear to the reader.

noname0443 · 2025-01-31T11:19:07Z

internal/app/app.go

+	} else if lostQuorumDuration <= app.config.WaitingRecoveryNetworkTimestamp+app.config.DoNotAcquireLockByDisconnectedManager {
+		// Manager cant AcquireLock in delay
+		app.logger.Debugf(
+			"manager lost quorum for %f, manager cant acquire lock for %f delay",


Perhaps there is a typo: cant -> can't. Could we amend it?

noname0443 · 2025-01-31T11:34:45Z

internal/app/app.go

+		app.logger.Debugf(
+			"manager lost quorum for %f, manager cant acquire lock for %f delay",
+			lostQuorumDuration.Seconds(),
+			app.config.WaitingRecoveryNetworkTimestamp.Seconds()+app.config.DoNotAcquireLockByDisconnectedManager.Seconds(),


Shouldn't we consider writing something like lostQuorumDuration - app.config.WaitingRecoveryNetworkTimestamp instead? If I understand correctly, the manager can't acquire lock refers to the duration following the WaitingRecoveryNetworkTimestamp period. So, wouldn't we need to subtract WaitingRecoveryNetworkTimestamp from the total lostQuorumDuration?

I meant that by reading this log, you can understand how long the old manager will not be able to acquire lock.

No, manager can't acquire lock refers to the duration DoNotAcquireLockByDisconnectedManager

As option, we can make this:

app.logger.Debugf( "In the period of lost quorum %f: manager can`t acquire lock for %f of %f acquire cooldown", lostQuorumDuration.Seconds(), lostQuorumDuration.Seconds() - app.config.WaitingRecoveryNetworkTimestamp.Seconds(), app.config.DoNotAcquireLockByDisconnectedManager.Seconds(), )

noname0443 · 2025-01-31T11:38:11Z

internal/config/config.go

@@ -66,6 +66,8 @@ type Config struct {
 	InfoFileHandlerInterval                 time.Duration                `config:"info_file_handler_interval" yaml:"info_file_handler_interval"`
 	RecoveryCheckInterval                   time.Duration                `config:"recoverycheck_interval" yaml:"recoverycheck_interval"`
 	ExternalCAFileCheckInterval             time.Duration                `config:"external_ca_file_check_interval" yaml:"external_ca_file_check_interval"`
+	WaitingRecoveryNetworkTimestamp         time.Duration                `config:"waiting_recovery_network_timestamp" yaml:"waiting_recovery_network_timestamp"`


I think the term timestamp generally refers to a specific moment in time rather than a duration. Perhaps we could consider renaming it to NetworkRecoveryWaitingDuration or NetworkRecoveryWaitingPeriod. What do you think?

I think, "delay" is the best option

noname0443 · 2025-01-31T12:02:16Z

internal/app/app.go

+			// Lost quorum less than 15 (default WaitingRecoveryNetworkTimestamp) seconds ago
+			// Just wait manager recover connection
+			if lostQuorumDuration <= app.config.WaitingRecoveryNetworkTimestamp {
+				app.logger.Debugf("manager lost quorum for %f, wait for revery network by manager", lostQuorumDuration.Seconds())


Perhaps there is a typo: revery network -> network recovery. Could we amend it?

noname0443 · 2025-01-31T12:50:38Z

Could we create .feature tests for quorum loss scenario?

teem0n · 2025-01-31T13:50:21Z

internal/app/app.go

+	}
+
+	lostQuorumDuration := time.Since(lostQuorumTime)
+	if lostQuorumDuration < app.config.WaitingRecoveryNetworkTimestamp {


i think it will be more clear, if we read config values to local variables and use them instead of app.config....

teem0n · 2025-01-31T13:57:06Z

internal/config/config.go

@@ -66,6 +66,8 @@ type Config struct {
 	InfoFileHandlerInterval                 time.Duration                `config:"info_file_handler_interval" yaml:"info_file_handler_interval"`
 	RecoveryCheckInterval                   time.Duration                `config:"recoverycheck_interval" yaml:"recoverycheck_interval"`
 	ExternalCAFileCheckInterval             time.Duration                `config:"external_ca_file_check_interval" yaml:"external_ca_file_check_interval"`
+	WaitingRecoveryNetworkTimestamp         time.Duration                `config:"waiting_recovery_network_timestamp" yaml:"waiting_recovery_network_timestamp"`


Not "Timestamp", but "Duration"/"Delay"

teem0n · 2025-01-31T13:57:24Z

internal/config/config.go

@@ -66,6 +66,8 @@ type Config struct {
 	InfoFileHandlerInterval                 time.Duration                `config:"info_file_handler_interval" yaml:"info_file_handler_interval"`
 	RecoveryCheckInterval                   time.Duration                `config:"recoverycheck_interval" yaml:"recoverycheck_interval"`
 	ExternalCAFileCheckInterval             time.Duration                `config:"external_ca_file_check_interval" yaml:"external_ca_file_check_interval"`
+	WaitingRecoveryNetworkTimestamp         time.Duration                `config:"waiting_recovery_network_timestamp" yaml:"waiting_recovery_network_timestamp"`
+	DoNotAcquireLockByDisconnectedManager   time.Duration                `config:"do_not_acquire_lock_by_disconnected_manager" yaml:"do_not_acquire_lock_by_disconnected_manager"`


AcquireLockCooldown

teem0n · 2025-01-31T14:03:38Z

internal/app/app.go

@@ -42,6 +42,8 @@ type App struct {
 	switchHelper        mysql.ISwitchHelper
 }

+var lostQuorumTime time.Time


Let's put this inside App struct

teem0n · 2025-01-31T14:04:44Z

internal/app/app.go

@@ -607,6 +609,58 @@ func (app *App) stateManager() appState {
 		return stateManager
 	}

+	// Counting the number of HA hosts visible to the manager.
+	visibleHAHostsCount := 0


Let's make a method like "ShouldReleaseLock" or smthng like

teem0n · 2025-01-31T14:06:21Z

internal/app/app.go

+				return stateCandidate
+			}
+		}
+	} else if workingHANodesCount > 0 {


We should not check if there are working nodes here...
I guess, only lostQuorumTime.IsZero() is needed

noname0443 · 2025-02-03T11:45:50Z

I want to supplement documentation with the variables. So, what do you think about the next definitions?

ManagerElectionDelayAfterQuorumLoss is a period after quorum loss when nodes continue working in normal mode and acquire manager's lock in dcs.

ManagerLockAcquireDelayAfterQuorumLoss is a period after quorum loss when nodes wait for the old manager to start working again and can't acquire locks in dcs.

… in tests

WithSoull added 2 commits January 31, 2025 12:48

feat: implement MVP for manager switching upon quorum loss

6c54c97

lint: fix linting

15c6327

noname0443 requested changes Jan 31, 2025

View reviewed changes

teem0n requested changes Jan 31, 2025

View reviewed changes

fix: renaming variables and some logic improvment

f00f141

WithSoull force-pushed the master branch from 6b375ce to 742f7bc Compare February 3, 2025 08:39

lint: some fixes for linter

9778189

WithSoull force-pushed the master branch from 742f7bc to 9778189 Compare February 3, 2025 08:43

fix: increase delays

1a5af42

WithSoull and others added 19 commits February 3, 2025 16:02

fix: improve switch logic into app.checkQuorum()

8d1001d

tests: add test for new feature (on Actions only this test)

1c8c913

Merge branch 'master' into master

f5ca4b0

fix: add ability to write formated variables on RunCommandOnHost step…

e786b4b

… in tests

fix: rewrite feature test

14c65c2

fix: add logs

adaee1d

fix

7683e23

fix

7b0546a

fix

647a78e

fix

845ef59

fix

ef25e35

fix

edc91bd

fix

eff2871

fix

b143dad

fix

8873de9

fix

4c97d2c

fix

ad4b877

fix

e884fa0

fix

ea01e52

WithSoull added 3 commits February 7, 2025 13:33

fix

03050e9

fix

dbd5aec

fix

c307dad

WithSoull force-pushed the master branch from e8a1309 to c307dad Compare February 7, 2025 11:09

finally fix

3762e34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Manager switching upon quorum loss #155

feat: Manager switching upon quorum loss #155

WithSoull commented Jan 31, 2025 •

edited

Loading

noname0443 Jan 31, 2025

noname0443 Jan 31, 2025

WithSoull Jan 31, 2025

noname0443 Jan 31, 2025

noname0443 Jan 31, 2025

noname0443 Jan 31, 2025

WithSoull Feb 1, 2025

WithSoull Feb 1, 2025 •

edited

Loading

noname0443 Jan 31, 2025

WithSoull Feb 1, 2025

noname0443 Jan 31, 2025

noname0443 commented Jan 31, 2025

teem0n Jan 31, 2025

teem0n Jan 31, 2025

teem0n Jan 31, 2025

teem0n Jan 31, 2025

teem0n Jan 31, 2025

teem0n Jan 31, 2025

noname0443 commented Feb 3, 2025

feat: Manager switching upon quorum loss #155

Are you sure you want to change the base?

feat: Manager switching upon quorum loss #155

Conversation

WithSoull commented Jan 31, 2025 • 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

WithSoull Feb 1, 2025 • 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

noname0443 commented Jan 31, 2025

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

noname0443 commented Feb 3, 2025

WithSoull commented Jan 31, 2025 •

edited

Loading

WithSoull Feb 1, 2025 •

edited

Loading