fix: add lock timeout to reduce lock failure cases #2287

allencloud · 2018-09-29T08:28:02Z

Signed-off-by: Allen Sun [email protected]

Ⅰ. Describe what this PR did

failed to xxx container xxxx: lock failed is a normal error when pouchd meets lots of concurrent request for the same container object, such as container.

In the original implementation, ctrd will try to get the lock for one container to continue the request's operation. While if the container's access(lock) is owned by others, then the request will not be blocked and will return a lock failure instead. This action has benefit and shortcomings as well. The benefit is that it could increase the real time of request even if it fails. The drawback is that it would depends on the client callers to retry the request.

To avoid this, we could take other ways like:

make the lock infinite blocked;
add a timeout when accessing the lock.

This pull request uses the second way to make it.

Besides the logic update for timeout(default is 5 seconds). I also changed the following things:

make the lock failure error to be more explicit, like container xxx is accessed by other request and please try again: lock failed;
remove the containerLock in ctrd since it could be replaced by kmutex pkg.

Ⅱ. Does this pull request fix one issue?

fix #2281

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

no need for this, I added more test cases for the common package knutex.

Ⅳ. Describe how to verify it

none

Ⅴ. Special notes for reviews

none

codecov · 2018-09-29T08:28:03Z

Codecov Report

Merging #2287 into master will decrease coverage by 0.04%.
The diff coverage is 63.04%.

@@            Coverage Diff             @@
##           master    #2287      +/-   ##
==========================================
- Coverage   66.97%   66.93%   -0.05%     
==========================================
  Files         211      210       -1     
  Lines       17306    17298       -8     
==========================================
- Hits        11591    11578      -13     
- Misses       4312     4313       +1     
- Partials     1403     1407       +4

Flag	Coverage Δ
#criv1alpha1test	`32.32% <41.3%> (+0.25%)`	⬆️
#criv1alpha2test	`35.78% <41.3%> (+0.18%)`	⬆️
#integrationtest	`40.24% <52.17%> (+0.06%)`	⬆️
#nodee2etest	`33.11% <52.17%> (-0.01%)`	⬇️
#unittest	`23.35% <26.08%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
pkg/kmutex/kmutex.go	`100% <100%> (+7.81%)`	⬆️
ctrd/utils.go	`65.38% <100%> (+0.67%)`	⬆️
ctrd/client.go	`57.47% <100%> (+0.4%)`	⬆️
ctrd/container.go	`59.76% <37.03%> (+1.9%)`	⬆️
cri/ocicni/cni_manager.go	`65% <0%> (-15%)`	⬇️
cri/v1alpha2/cri_wrapper.go	`60% <0%> (-2.4%)`	⬇️
cri/v1alpha2/cri.go	`64.87% <0%> (-1.46%)`	⬇️
cri/v1alpha2/cri_utils.go	`90.28% <0%> (-0.29%)`	⬇️
daemon/logger/jsonfile/utils.go	`73.17% <0%> (+1.62%)`	⬆️
daemon/mgr/container_stats.go	`12.61% <0%> (+1.8%)`	⬆️
... and 3 more

xiaoxubeii · 2018-10-09T07:08:46Z

pkg/kmutex/kmutex.go

 	if ok && len(v.c) == 0 {
+		// filled up the chan to identify caller has released the lock.
 		v.c <- struct{}{}
 	}
 }


Is it better that free unused lockedKeys in this Unlock method than using a extra go routine?
https://github.com/alibaba/pouch/blob/f56461421b3a2148db94d825a5e715fb156e2972/pkg/kmutex/kmutex.go#L41-L52

Actually this is a legacy function which is designed by @skoo87 .
Could you help explain a little bit more about the goroutine usage here?

HusterWan · 2018-10-12T02:27:04Z

ctrd/utils_test.go

@@ -5,7 +5,6 @@ import (
 	"testing"

 	"github.com/alibaba/pouch/pkg/errtypes"


please add blank line here

Signed-off-by: Allen Sun <[email protected]>

allencloud · 2018-10-15T11:58:13Z

pkg/kmutex/kmutex.go

 		return false
 	}
 }

-// LockWithTimeout trys to lock, if can't acquire the lock, will block util timeout.
+// LockWithTimeout tries to lock.
+// It can't acquire the lock, will block util timeout.
 func (m *KMutex) LockWithTimeout(k string, to time.Duration) bool {


s/LockWithTimeout/TrylockWithTimeout

allencloud · 2018-10-15T12:00:09Z

pkg/kmutex/kmutex.go

 	select {
 	case <-v.c:
+		// the locker has released the lock.
 		return true


In this line, we think there is a bug there.
If this goroutine gets the lock and returns true, there is a very tiny possibility that the GC function would delete the corresponding locked key.

Agree. These lock functions, such as Trylock, LockWithTimeout and Lock all have same problem that they didn't not get the lock after <-v.c.

allencloud · 2018-10-15T12:00:19Z

pkg/kmutex/kmutex.go

 	select {
 	case <-v.c:
+		// the locker has released the lock.
 		return true


In this line, we think there is a bug there.
If this goroutine gets the lock and returns true, there is a very tiny possibility that the GC function would delete the corresponding locked key.

allencloud · 2018-10-15T12:04:24Z

Currently block this pull request. For the issue #2281 , we will try to make the code :

if !c.Trylock(id) {
		return nil, errtypes.ErrLockfailed			
}

to be:

if !c.Trylock(id) {
      for i:= 0;i<3;i++{
            // try another 3 times.
     }			
}

xiaoxubeii · 2018-10-16T07:02:22Z

pkg/kmutex/kmutex.go

 	select {
 	case <-v.c:
+		// the locker has released the lock.
 		return true


Agree. These lock functions, such as Trylock, LockWithTimeout and Lock all have same problem that they didn't not get the lock after <-v.c.

pouchrobot · 2018-10-18T07:58:20Z

ping @allencloud
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

fuweid · 2018-10-18T15:38:17Z

this issue has been fixed by #2319

pouchrobot added kind/bug This is bug report for project size/XL labels Sep 29, 2018

allencloud force-pushed the fix-2281 branch 2 times, most recently from 1e822b7 to f8d49a0 Compare September 30, 2018 01:47

allencloud force-pushed the fix-2281 branch from f8d49a0 to f564614 Compare October 8, 2018 04:08

xiaoxubeii reviewed Oct 9, 2018

View reviewed changes

HusterWan reviewed Oct 12, 2018

View reviewed changes

fix: add lock timeout to reduce lock failure cases

5b994ef

Signed-off-by: Allen Sun <[email protected]>

allencloud force-pushed the fix-2281 branch from f564614 to 5b994ef Compare October 12, 2018 03:44

allencloud commented Oct 15, 2018

View reviewed changes

allencloud mentioned this pull request Oct 15, 2018

Failed to stop container: lock failed #2281

Closed

xiaoxubeii suggested changes Oct 16, 2018

View reviewed changes

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Oct 17, 2018

fuweid closed this Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add lock timeout to reduce lock failure cases #2287

fix: add lock timeout to reduce lock failure cases #2287

allencloud commented Sep 29, 2018

codecov bot commented Sep 29, 2018 •

edited

Loading

xiaoxubeii Oct 9, 2018

allencloud Oct 12, 2018

HusterWan Oct 12, 2018

allencloud Oct 15, 2018

allencloud Oct 15, 2018

xiaoxubeii Oct 16, 2018

allencloud Oct 15, 2018

allencloud commented Oct 15, 2018

xiaoxubeii Oct 16, 2018

pouchrobot commented Oct 18, 2018

fuweid commented Oct 18, 2018

		@@ -5,7 +5,6 @@ import (
		"testing"

		"github.com/alibaba/pouch/pkg/errtypes"

fix: add lock timeout to reduce lock failure cases #2287

fix: add lock timeout to reduce lock failure cases #2287

Conversation

allencloud commented Sep 29, 2018

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Sep 29, 2018 • edited Loading

Codecov Report

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

allencloud commented Oct 15, 2018

Choose a reason for hiding this comment

pouchrobot commented Oct 18, 2018

fuweid commented Oct 18, 2018

codecov bot commented Sep 29, 2018 •

edited

Loading