Use consul lock properly

[5antelope]

Signed-off-by: crowu [email protected]

Description

Vitess assumes the process gets the lock if the error is nil https://github.com/vitessio/vitess/blob/main/go/vt/topo/locks.go

However, consul will return nil error when the context timeout’d before the process get the lock: https://github.com/hashicorp/consul/blob/master/api/lock.go#L185 - which means anyone will get the lock after the context timeout.

This means after the context timeout, everyone that is waiting for the lock will get one

This PR make sure we will raise error if the lost chan returned is nil, we should not assume we have the lock

Related Issue(s)

#8309

Checklist

• Tests were added or are not required
• Documentation was added or is not required

Deployment Notes

[5antelope]

Gentle reminder, thanks!

[deepthi]

Change looks good.
Is there a way to simulate the failure condition in a unit test?

[5antelope]

@deepthi it is a little bit to simulate the failure in unit test.

iiuc the test we have for topo lock are here: https://github.com/vitessio/vitess/blob/main/go/vt/topo/test/lock.go#L136

we are using same process that tries to grab the lock, the check here will consume all the time in the context: https://github.com/vitessio/vitess/blob/main/go/vt/topo/consultopo/lock.go#L72 before we jump into the line that causes the real issue here https://github.com/vitessio/vitess/blob/main/go/vt/topo/consultopo/lock.go#L88

any suggestions to set this up properly?

[deepthi]

@deepthi it is a little bit to simulate the failure in unit test.

iiuc the test we have for topo lock are here: https://github.com/vitessio/vitess/blob/main/go/vt/topo/test/lock.go#L136

we are using same process that tries to grab the lock, the check here will consume all the time in the context: https://github.com/vitessio/vitess/blob/main/go/vt/topo/consultopo/lock.go#L72 before we jump into the line that causes the real issue here https://github.com/vitessio/vitess/blob/main/go/vt/topo/consultopo/lock.go#L88

any suggestions to set this up properly?

Yeah I see the problem. I think it's ok to merge this as-is.