[lxc-devel] [lxd/master] Instance: Fix deadlock in instance operationlock package

[tomponline on Github]

A non-text attachment was scrubbed...
Name: not available
Type: text/x-mailbox
Size: 446 bytes
Desc: not available
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20201215/e3cf3a4b/attachment.bin>
-------------- next part --------------
From 678cfbde4804df2c7d50f73b66c03b0be4491fb6 Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Tue, 15 Dec 2020 16:22:56 +0000
Subject: [PATCH 1/3] lxd/instance/operationlock: Fixes deadlock caused by call
 to Reset in Create

Both try to aquire lock and so can deadlock each other.

By pushing to the reset channel directly from Create we avoid the deadlock.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/instance/operationlock/operationlock.go | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lxd/instance/operationlock/operationlock.go b/lxd/instance/operationlock/operationlock.go
index faab4c5982..a3bbd74e04 100644
--- a/lxd/instance/operationlock/operationlock.go
+++ b/lxd/instance/operationlock/operationlock.go
@@ -37,7 +37,8 @@ func Create(instanceID int, action string, reusable bool, reuse bool) (*Instance
 	op := instanceOperations[instanceID]
 	if op != nil {
 		if op.reusable && reuse {
-			op.Reset()
+			// Reset operation timeout without releasing lock or deadlocking using Reset() function.
+			op.chanReset <- true
 			return op, nil
 		}
 

From b8d7f56488219f0dfd3cb0e6076ea9b3b506863e Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Tue, 15 Dec 2020 16:23:45 +0000
Subject: [PATCH 2/3] lxd/instance/operationlock: Store operation in
 instanceOperations before calling go routine

As the go routine can call functions on the operation (such as op.Done) which rely on the instanceOperations map being populated it seems appropriate to ensure it has been populated with the new operation before starting the go routine.

Even though the only current use of the operation inside the go routine is after 30s.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/instance/operationlock/operationlock.go | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lxd/instance/operationlock/operationlock.go b/lxd/instance/operationlock/operationlock.go
index a3bbd74e04..b7e6504961 100644
--- a/lxd/instance/operationlock/operationlock.go
+++ b/lxd/instance/operationlock/operationlock.go
@@ -52,6 +52,8 @@ func Create(instanceID int, action string, reusable bool, reuse bool) (*Instance
 	op.chanDone = make(chan error, 0)
 	op.chanReset = make(chan bool, 0)
 
+	instanceOperations[instanceID] = op
+
 	go func(op *InstanceOperation) {
 		for {
 			select {
@@ -64,8 +66,6 @@ func Create(instanceID int, action string, reusable bool, reuse bool) (*Instance
 		}
 	}(op)
 
-	instanceOperations[instanceID] = op
-
 	return op, nil
 }
 

From 18ace0e0607b0b0f15bf9dc28c6355f57bb66fde Mon Sep 17 00:00:00 2001
From: Thomas Parrott <thomas.parrott at canonical.com>
Date: Tue, 15 Dec 2020 16:25:13 +0000
Subject: [PATCH 3/3] lxd/instance/operationlock: Exit go routine started in
 Create when the operation is done

Otherwise I have observed that go routines can hang around for up to 30s after operation is completed.

Signed-off-by: Thomas Parrott <thomas.parrott at canonical.com>
---
 lxd/instance/operationlock/operationlock.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lxd/instance/operationlock/operationlock.go b/lxd/instance/operationlock/operationlock.go
index b7e6504961..49dab48b1a 100644
--- a/lxd/instance/operationlock/operationlock.go
+++ b/lxd/instance/operationlock/operationlock.go
@@ -57,6 +57,8 @@ func Create(instanceID int, action string, reusable bool, reuse bool) (*Instance
 	go func(op *InstanceOperation) {
 		for {
 			select {
+			case <-op.chanDone:
+				return
 			case <-op.chanReset:
 				continue
 			case <-time.After(time.Second * 30):