Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC]refactor: refact the container lock #2756

Merged
merged 1 commit into from
Apr 3, 2019

Conversation

HusterWan
Copy link
Contributor

@HusterWan HusterWan commented Mar 17, 2019

Signed-off-by: Michael Wan zirenwan@gmail.com

Ⅰ. Describe what this PR did

Since the single container APIs have not occurred performance problem and the Fine-grained locking mechanism has occurred many concurrency problems, so we decide to change to Coarse-grained locking mechanism that will lock all operations in one API function.

Of course, we should make sure this change has no influence on the legacy containers and the CRI interfaces, so we will first make all test cases happy, then consider to merge this pr.

Ⅱ. 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
Copy link

codecov bot commented Mar 17, 2019

Codecov Report

Merging #2756 into master will decrease coverage by 0.08%.
The diff coverage is 86.15%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #2756      +/-   ##
=========================================
- Coverage   69.39%   69.3%   -0.09%     
=========================================
  Files         277     277              
  Lines       17435   17347      -88     
=========================================
- Hits        12099   12023      -76     
+ Misses       4008    3993      -15     
- Partials     1328    1331       +3
Flag Coverage Δ
#criv1alpha2_test 39.32% <40%> (-0.03%) ⬇️
#integration_test_0 36.48% <55.38%> (-0.16%) ⬇️
#integration_test_1 35.29% <55.38%> (-0.14%) ⬇️
#integration_test_2 36.38% <40%> (-0.04%) ⬇️
#integration_test_3 35.37% <41.53%> (-0.18%) ⬇️
#node_e2e_test 35.1% <27.69%> (-0.14%) ⬇️
#unittest 28.67% <0%> (+0.14%) ⬆️
Impacted Files Coverage Δ
daemon/mgr/container_storage.go 60.24% <ø> (-0.17%) ⬇️
daemon/mgr/container_types.go 70.49% <ø> (-2.37%) ⬇️
daemon/mgr/spec.go 62.5% <ø> (-4.17%) ⬇️
daemon/mgr/container_checkpoint.go 68.67% <0%> (ø) ⬆️
daemon/mgr/container_stats.go 70.52% <100%> (-1.32%) ⬇️
daemon/mgr/container_state.go 100% <100%> (+8.08%) ⬆️
daemon/mgr/container_logs.go 84.41% <100%> (ø) ⬆️
daemon/mgr/container_exec.go 79.67% <100%> (ø) ⬆️
daemon/mgr/container_upgrade.go 65.09% <100%> (ø) ⬆️
daemon/mgr/container.go 61.24% <83.67%> (+0.4%) ⬆️
... and 7 more

@Ace-Tang
Copy link
Contributor

Maybe get deadlock somewhere or other reason, ci hang long make test fail

@HusterWan
Copy link
Contributor Author

Maybe get deadlock somewhere or other reason, ci hang long make test fail

@Ace-Tang yes, I will figure it out

@HusterWan HusterWan force-pushed the zr/fix-lock branch 2 times, most recently from 62b1d65 to 15bcd29 Compare March 18, 2019 02:17
@Ace-Tang
Copy link
Contributor

@HusterWan , does ths ready for review now?

@HusterWan
Copy link
Contributor Author

@HusterWan , does ths ready for review now?

Yes, you can review now.

@wangforthinker
Copy link
Contributor

@HusterWan As we know, read ops are more than write ops, so rwlock is a good suggestion.

@HusterWan
Copy link
Contributor Author

@HusterWan As we know, read ops are more than write ops, so rwlock is a good suggestion.

@wangforthinker agree with you. actually, we only have one container read ops, just Get api. and we do not add lock for this api.

@HusterWan HusterWan changed the title [WIP]refactor: refact the container lock [RFC]refactor: refact the container lock Mar 18, 2019
@fuweid
Copy link
Contributor

fuweid commented Mar 19, 2019

I think we should add lock for Create. Since we set the container name at the ending of function, it might create two containers with the same name. WDYT?

@HusterWan
Copy link
Contributor Author

I think we should add lock for Create. Since we set the container name at the ending of function, it might create two containers with the same name. WDYT?

I think we should change the time when to put name->id to the cache in case we may create containers with the same name. Using container Lock cannot fix the problem.

@HusterWan
Copy link
Contributor Author

I think we should add lock for Create. Since we set the container name at the ending of function, it might create two containers with the same name. WDYT?

I think we should change the time when to put name->id to the cache in case we may create containers with the same name. Using container Lock cannot fix the problem.

I can create a new pr to fix this problem.

@Ace-Tang
Copy link
Contributor

@HusterWan , please rebase, since I fix a bug in #2757
and parallel test is pass.

@HusterWan
Copy link
Contributor Author

@HusterWan , please rebase, since I fix a bug in #2757
and parallel test is pass.

done

@allencloud
Copy link
Collaborator

Please solve the conflict and sync the latest code, and then we can move to consider merging. @HusterWan

… api

Signed-off-by: Michael Wan <zirenwan@gmail.com>
Copy link
Contributor

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@fuweid fuweid merged commit a6c5274 into AliyunContainerService:master Apr 3, 2019
rudyfly pushed a commit to rudyfly/pouch that referenced this pull request Apr 12, 2019
refactor: refact the container lock, put a big lock for all container api

Signed-off-by: Michael Wan <zirenwan@gmail.com>

cherry-pick from github AliyunContainerService#2756

See merge request !304119
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants