This is an automated email from the ASF dual-hosted git repository.
pbacsko pushed a commit to branch branch-1.5
in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
The following commit(s) were added to refs/heads/branch-1.5 by this push:
new 34662721 [YUNIKORN-2646] Allow lock order detection to be turned off
(#877)
34662721 is described below
commit 34662721f7e1d4e810adf9161267397272a2461c
Author: Wilfred Spiegelenburg <[email protected]>
AuthorDate: Fri May 31 08:52:02 2024 +0200
[YUNIKORN-2646] Allow lock order detection to be turned off (#877)
Most of the false positive deadlock detections are linked to the
preemption cycle. In that cycle we have a lock on the application being
scheduled and acquire read locks on other applications to look for
victims.
In the next scheduling cycle the applications involved might be reversed
triggering an inconsistent lock detection to be logged. That causes log
spew.
The lock order detection can be turned off by setting an option exposed
via a new environment variable.
Simplify testing
Closes: #877
Signed-off-by: Peter Bacsko <[email protected]>
(cherry picked from commit 28ba5aea049e15c295cdd6932c43f22b9dfbf49d)
---
pkg/locking/locking.go | 60 +++++++++++++++++++++++++---------------
pkg/locking/locking_race_test.go | 37 +++++++++++++++++++++++++
pkg/locking/locking_test.go | 45 +++++++++++++++++++++++++++---
3 files changed, 116 insertions(+), 26 deletions(-)
diff --git a/pkg/locking/locking.go b/pkg/locking/locking.go
index 9c0c661a..f4d903c9 100644
--- a/pkg/locking/locking.go
+++ b/pkg/locking/locking.go
@@ -31,16 +31,22 @@ import (
"github.com/apache/yunikorn-core/pkg/log"
)
-const EnvDeadlockDetectionEnabled = "DEADLOCK_DETECTION_ENABLED"
-const EnvDeadlockTimeoutSeconds = "DEADLOCK_TIMEOUT_SECONDS"
-const EnvExitOnDeadlock = "DEADLOCK_EXIT"
+const (
+ EnvDeadlockDetectionEnabled = "DEADLOCK_DETECTION_ENABLED"
+ EnvDeadlockTimeoutSeconds = "DEADLOCK_TIMEOUT_SECONDS"
+ EnvExitOnDeadlock = "DEADLOCK_EXIT"
+ EnvDisableLockOrder = "DEADLOCK_DISABLE_LOCK_ORDER"
+)
-var once sync.Once
-var trackingEnabled atomic.Bool
-var timeoutSeconds atomic.Int32
-var deadlockDetected atomic.Bool
-var testingMode atomic.Bool
-var exitOnDeadlock bool
+var (
+ once sync.Once
+ trackingEnabled atomic.Bool
+ timeoutSeconds atomic.Int32
+ deadlockDetected atomic.Bool
+ testingMode atomic.Bool
+ exitOnDeadlock atomic.Bool
+ disableOrderDetect atomic.Bool
+)
type errorBuf struct {
data string
@@ -68,31 +74,45 @@ func reInit() {
}
trackingEnabled.Store(enabled)
- timeoutSec, err :=
strconv.ParseInt(os.Getenv(EnvDeadlockTimeoutSeconds), 10, 32)
+ var timeoutSec int64
+ timeoutSec, err =
strconv.ParseInt(os.Getenv(EnvDeadlockTimeoutSeconds), 10, 32)
if err != nil {
timeoutSec = 60
}
timeoutSeconds.Store(int32(timeoutSec))
+
+ var disableOrder bool
+ disableOrder, err = strconv.ParseBool(os.Getenv(EnvDisableLockOrder))
+ if err != nil {
+ disableOrder = false
+ }
+ disableOrderDetect.Store(disableOrder)
+
+ var exitOnDetect bool
+ exitOnDetect, err = strconv.ParseBool(os.Getenv(EnvExitOnDeadlock))
+ if err != nil {
+ exitOnDetect = false
+ }
+ exitOnDeadlock.Store(exitOnDetect)
+
+ // set deadlock detection options
godeadlock.Opts.Disable = !enabled
godeadlock.Opts.DeadlockTimeout = time.Duration(timeoutSec) *
time.Second
godeadlock.Opts.LogBuf = &errorBuf{}
godeadlock.Opts.OnPotentialDeadlock = onPotentialDeadlock
- if exitEnv, err := strconv.ParseBool(os.Getenv(EnvExitOnDeadlock)); err
!= nil {
- exitOnDeadlock = false
- } else {
- exitOnDeadlock = exitEnv
- }
+ godeadlock.Opts.DisableLockOrderDetection = disableOrder
if enabled {
- // We want to ensure that we write this before any other
subsystem is initialized, including logging which may also use locks.
- fmt.Fprintf(os.Stderr, "=== Deadlock detection enabled
(timeout: %d seconds, exit on deadlock: %v) ===\n", timeoutSec, exitOnDeadlock)
+ // We want to ensure that we write this before any other
subsystem is initialized, including logging which may also use locks.
+ // no way to handle errors just ignore
+ _, _ = fmt.Fprintf(os.Stderr, "=== Deadlock detection enabled
(timeout: %d seconds, exit on deadlock: %t, locking order disabled: %t) ===\n",
timeoutSec, exitOnDetect, disableOrder)
}
}
func onPotentialDeadlock() {
deadlockDetected.Store(true)
printBufContents()
- if exitOnDeadlock && !testingMode.Load() {
+ if exitOnDeadlock.Load() && !testingMode.Load() {
os.Exit(1)
}
}
@@ -109,10 +129,6 @@ func printBufContents() {
buf.data = ""
}
-func SetTrackingEnabled(enabled bool) {
- trackingEnabled.Store(enabled)
-}
-
func IsTrackingEnabled() bool {
return trackingEnabled.Load()
}
diff --git a/pkg/locking/locking_race_test.go b/pkg/locking/locking_race_test.go
index c1930150..e5b7be45 100644
--- a/pkg/locking/locking_race_test.go
+++ b/pkg/locking/locking_race_test.go
@@ -43,3 +43,40 @@ func TestDeadlockDetection(t *testing.T) {
mutex.Unlock() // will unwind first lock
assert.Assert(t, IsDeadlockDetected(), "Deadlock should have been
detected")
}
+
+// TestLockOrderDetection
+// lock order detection looks at the ordering of the same mutexes in different
go routines
+// if the order changes (for two different go routines) then that could be a
potential deadlock
+// this case happens in preemption when looking for victims when queues hover
around guaranteed
+func TestLockOrderDetection(t *testing.T) {
+ var tests = []struct {
+ name string
+ disable bool
+ }{
+ {"ordered", false},
+ {"no order", true},
+ }
+ for _, tt := range tests {
+ t.Run(tt.name, func(t *testing.T) {
+ enableTrackingWithOrder(tt.disable)
+ deadlockDetected.Store(false)
+ defer disableTracking()
+
+ var a, b RWMutex
+ // lock ordering: a, b, b, a
+ a.Lock()
+ b.RLock()
+ b.RUnlock()
+ a.Unlock()
+
+ // lock ordering: b, a, a, b
+ b.Lock()
+ a.RLock()
+ a.RUnlock()
+ b.Unlock()
+
+ // detection is based on the tracking order enabled or
not
+ assert.Assert(t, IsDeadlockDetected() == tt.disable,
"Deadlock detected not as expected")
+ })
+ }
+}
diff --git a/pkg/locking/locking_test.go b/pkg/locking/locking_test.go
index 34993505..b50f1a91 100644
--- a/pkg/locking/locking_test.go
+++ b/pkg/locking/locking_test.go
@@ -20,6 +20,7 @@
package locking
import (
+ "fmt"
"os"
"sync"
"sync/atomic"
@@ -32,15 +33,21 @@ import (
)
func disableTracking() {
- os.Unsetenv(EnvDeadlockDetectionEnabled)
- os.Unsetenv(EnvDeadlockTimeoutSeconds)
+ _ = os.Unsetenv(EnvDeadlockDetectionEnabled)
+ _ = os.Unsetenv(EnvDeadlockTimeoutSeconds)
+ _ = os.Unsetenv(EnvDisableLockOrder)
testingMode.Store(false)
reInit()
}
func enableTracking() {
- os.Setenv(EnvDeadlockDetectionEnabled, "true")
- os.Setenv(EnvDeadlockTimeoutSeconds, "1")
+ enableTrackingWithOrder(true)
+}
+
+func enableTrackingWithOrder(enableOrder bool) {
+ _ = os.Setenv(EnvDeadlockDetectionEnabled, "true")
+ _ = os.Setenv(EnvDeadlockTimeoutSeconds, "1")
+ _ = os.Setenv(EnvDisableLockOrder, fmt.Sprint(!enableOrder))
testingMode.Store(true)
reInit()
}
@@ -155,6 +162,36 @@ func BenchmarkTrackedRWMutexWrite(b *testing.B) {
}
}
+func BenchmarkTrackedNoOrderMutex(b *testing.B) {
+ enableTrackingWithOrder(false)
+ defer disableTracking()
+ var lock Mutex
+ for i := 0; i < b.N; i++ {
+ lock.Lock()
+ lock.Unlock()
+ }
+}
+
+func BenchmarkTrackedNoOrderRWMutexRead(b *testing.B) {
+ enableTrackingWithOrder(false)
+ defer disableTracking()
+ var lock RWMutex
+ for i := 0; i < b.N; i++ {
+ lock.RLock()
+ lock.RUnlock()
+ }
+}
+
+func BenchmarkTrackedNoOrderRWMutexWrite(b *testing.B) {
+ enableTrackingWithOrder(false)
+ defer disableTracking()
+ var lock RWMutex
+ for i := 0; i < b.N; i++ {
+ lock.Lock()
+ lock.Unlock()
+ }
+}
+
func TestMutex(t *testing.T) {
var mutex Mutex
var result atomic.Int32
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
