MySQL HA: Return an error if we fail to get a lock on standby #8229

catsby · 2020-01-23T20:25:47Z

On a standby node, attempting to grab a lock with NewMySQLLock can fail if the MySQL fails to respond (in this case, is completely down), and return a nil MySQLBackend. If the method returns an error, we pass it on to the failLock channel which returns the error, but at present we don't abort the internal attemptLock method, instead we continue on an eventually get a nil pointer deference panic as shown in #8203.

In this PR, if NewMySQLLock returns an error we send on the failLock channel as normal, but we also call return. I checked the Postgresql storage backend and it too returns after failed lock:

vault/physical/postgresql/postgresql.go

Lines 420 to 435 in ac33c32

    
           for { 
        
           	select { 
        
           	case <-stop: 
        
           		return 
        
           	case <-ticker.C: 
        
           		gotlock, err := l.writeItem() 
        
           		switch { 
        
           		case err != nil: 
        
           			errors <- err 
        
           			return 
        
           		case gotlock: 
        
           			close(success) 
        
           			return 
        
           		} 
        
           	} 
        
           }

Fixes #8203

michelvocks

LGTM. Not important but we could potentially test this.

…nto b-fix-mysql-lock-panic * 'b-fix-mysql-lock-panic' of github.com:hashicorp/vault: (52 commits) Fix minor typo in doc string (#8277) Update gen_openapi.sh (#8273) update dependencies (#8271) docs: update vault k8s to 0.2.0 (#8269) Fix flaky test of api renewer by moving away from legacy api. (#8265) Clean AlibabaCloud physical backend code (#8186) Update GH issue template to point to forum (#8226) Fix default max_open_connections for db plugins (#8262) Fix broken link (#8259) Removing timing-dependent aspects of test. (#8261) Changelog++ Added flag to disable X-Vault-Token header proxy if client passes the token (#8101) changelog++ changelog++ test: fix TestAgent_Template_Basic (#8257) docs: fix api path for merge entity identity doc (#8258) Bump etcd client API dep (#8037) Add Consul TLS options to access API endpoint (#8253) Docs: Add nomad TLS options (#8254) Update CHANGELOG.md ...

catsby · 2020-02-04T17:26:14Z

LGTM. Not important but we could potentially test this.

@michelvocks you're totally right, a regression test was easier than I expected and added in 8ea43ab , please take another look when you can! Thanks

michelvocks

LGTM!

catsby · 2020-02-05T20:08:36Z

Commit 16e3711 dismissed @michelvocks' approval, however that commit simply fixes a race in the test so I'm going to merge as is

* return an error if we fail to get a lock on standby * Add regression test * minor refactoring to remove a race condition in the test

…#8300) * return an error if we fail to get a lock on standby * Add regression test * minor refactoring to remove a race condition in the test

sksingh20 · 2021-05-13T12:34:12Z

if this created new probleb with mysql Mysql Hashicorp #10318
Mysql Hashicorp Vault in HA leaving multiple sleep connections with Mysql

sksingh20 · 2021-05-13T12:37:28Z

Standby node leaving all connection with mysql as established while trying to acquire lock

pi@serlog:~ $ sudo netstat tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:57480 tcp 0 0 serlog:mysql tcp 0 0 serlog:57482 tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql tcp 0 0 serlog:46002 tcp 0 0 serlog:mysql tcp 0 0 serlog:mysql pi@serlog:~ $ | grep mysql |grep -i serlog|grep -i established
192.168.0.213:58760 ESTABLISHED
192.168.0.213:58762 ESTABLISHED
192.168.0.213:58834 ESTABLISHED
192.168.0.213:58818 ESTABLISHED
192.168.0.213:58822 ESTABLISHED
192.168.0.213:58694 ESTABLISHED
192.168.0.213:58728 ESTABLISHED
192.168.0.213:58790 ESTABLISHED
192.168.0.213:58736 ESTABLISHED
192.168.0.213:58668 ESTABLISHED
192.168.0.213:58688 ESTABLISHED
192.168.0.213:58750 ESTABLISHED
serlog:mysql ESTABLISHED
192.168.0.213:58714 ESTABLISHED
serlog:mysql ESTABLISHED
serlog:46002 ESTABLISHED
192.168.0.213:58800 ESTABLISHED
192.168.0.213:58734 ESTABLISHED
192.168.0.213:58784 ESTABLISHED
192.168.0.213:58708 ESTABLISHED
192.168.0.213:58842 ESTABLISHED
serlog:57482 ESTABLISHED
192.168.0.213:58840 ESTABLISHED
192.168.0.213:58686 ESTABLISHED
192.168.0.213:58792 ESTABLISHED
192.168.0.213:58812 ESTABLISHED
192.168.0.213:58810 ESTABLISHED
192.168.0.213:58700 ESTABLISHED
192.168.0.213:58774 ESTABLISHED
192.168.0.213:58742 ESTABLISHED
192.168.0.213:58756 ESTABLISHED
192.168.0.213:58768 ESTABLISHED
192.168.0.213:58744 ESTABLISHED
192.168.0.213:58780 ESTABLISHED
192.168.0.213:58680 ESTABLISHED
192.168.0.213:58766 ESTABLISHED
192.168.0.213:58832 ESTABLISHED
192.168.0.213:58816 ESTABLISHED
192.168.0.213:58786 ESTABLISHED
192.168.0.213:58806 ESTABLISHED
192.168.0.213:58838 ESTABLISHED
192.168.0.213:58682 ESTABLISHED
192.168.0.213:58716 ESTABLISHED
192.168.0.213:58828 ESTABLISHED
192.168.0.213:58676 ESTABLISHED
192.168.0.213:58722 ESTABLISHED
serlog:mysql ESTABLISHED
192.168.0.213:58710 ESTABLISHED
serlog:57480 ESTABLISHED

On standby vault node I have following output when I trace a connection

pi@vaultf2:~ $ sudo lsof -i tcp:59860
COMMAND PID USER FD TYPE DEVICE SIZE/OFF NODE NAME
vault 9180 root 78u IPv4 63158 0t0 TCP 192.168.0.213:59860->192.168.0.212:mysql (ESTABLISHED)
pi@vaultf2:~ $

pi@vaultf2:~ $ ps -ef|grep -i 9180
root 9180 1 2 18:27 ? 00:00:38 /usr/bin/vault server -config=/etc/vault/config.hcl
pi 10527 664 0 18:52 pts/0 00:00:00 grep --color=auto -i 9180
pi@vaultf2:~ $

return an error if we fail to get a lock on standby

f18031d

catsby added the storage/mysql label Jan 23, 2020

catsby mentioned this pull request Jan 23, 2020

mysql backend HA lock handling is unsafe #8203

Closed

michelvocks previously approved these changes Jan 24, 2020

View reviewed changes

catsby and others added 3 commits February 3, 2020 06:31

Merge branch 'master' into b-fix-mysql-lock-panic

6641c89

Add regression test

8ea43ab

catsby dismissed michelvocks’s stale review via 575c967 February 4, 2020 17:24

catsby requested a review from michelvocks February 4, 2020 17:26

michelvocks previously approved these changes Feb 5, 2020

View reviewed changes

minor refactoring to remove a race condition in the test

16e3711

catsby dismissed michelvocks’s stale review via 16e3711 February 5, 2020 14:49

catsby merged commit 66bf106 into master Feb 5, 2020

catsby deleted the b-fix-mysql-lock-panic branch February 5, 2020 20:08

catsby added a commit that referenced this pull request Feb 5, 2020

MySQL HA: Return an error if we fail to get a lock on standby (#8229)

b5511ac

* return an error if we fail to get a lock on standby * Add regression test * minor refactoring to remove a race condition in the test

catsby mentioned this pull request Feb 5, 2020

Backport 1.3.x: MySQL HA: Return an error if we fail to get a lock on standby (#8229) #8300

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MySQL HA: Return an error if we fail to get a lock on standby #8229

MySQL HA: Return an error if we fail to get a lock on standby #8229

catsby commented Jan 23, 2020 •

edited

Loading

michelvocks left a comment

catsby commented Feb 4, 2020 •

edited

Loading

michelvocks left a comment

catsby commented Feb 5, 2020

sksingh20 commented May 13, 2021

sksingh20 commented May 13, 2021 •

edited

Loading

	for {
	select {
	case <-stop:
	return
	case <-ticker.C:
	gotlock, err := l.writeItem()
	switch {
	case err != nil:
	errors <- err
	return
	case gotlock:
	close(success)
	return
	}
	}
	}

MySQL HA: Return an error if we fail to get a lock on standby #8229

MySQL HA: Return an error if we fail to get a lock on standby #8229

Conversation

catsby commented Jan 23, 2020 • edited Loading

michelvocks left a comment

Choose a reason for hiding this comment

catsby commented Feb 4, 2020 • edited Loading

michelvocks left a comment

Choose a reason for hiding this comment

catsby commented Feb 5, 2020

sksingh20 commented May 13, 2021

sksingh20 commented May 13, 2021 • edited Loading

catsby commented Jan 23, 2020 •

edited

Loading

catsby commented Feb 4, 2020 •

edited

Loading

sksingh20 commented May 13, 2021 •

edited

Loading