kubenet: yield lock while executing CNI plugin. #54800
Conversation
The CNI plugin can take up to 3 seconds to execute. CNI plugins can safely be executed in parallel, so yield the lock to speed up pod creation. Fixes: kubernetes#54651
k8s-ci-robot
added
release-note-none
|
Hi @squeed. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
needs-ok-to-test
|
/ok-to-test |
k8s-ci-robot
removed
the
needs-ok-to-test
|
@kubernetes/sig-network-pr-reviews @kubernetes/sig-node-pr-reviews |
k8s-ci-robot
added
sig/network
|
This seems fine to me.. Ideally I'd like to see the lock held only when modifying state that is actually meant to be protected by it (pod IP map, net config, shaper init according to the comments). At a glance, it seems we're holding this lock in a whole bunch of places that we don't need to, but I'm happy enough to get this in as a quick fix. |
|
@caseydavenport yeah, I initially approached that, but ultimately decided not to. It was going to be a fairly invasive change, given that the rest of that module is assuming it has the lock. Mutations are spread throughout, so I wasn't confident I could avoid introducing bugs. |
|
ping @freehan for approval |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @thockin |
|
/approve This is a quick-and-dirty fix. But okay. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, freehan, squeed Associated issue: 54651 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-github-robot
added
the
approved
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here. |
9 participants
The CNI plugin can take up to 3 seconds to execute. CNI plugins can safely be
executed in parallel, so yield the lock to speed up pod creation.
This caused problems with the pod latency tests - previously, CNI plugins executed
in under 20ms. Now they must wait for DAD to finish and addresses to leave
tentative state.
Fixes: #54651
What this PR does / why we need it:
After upgrading CNI plugins to v0.6 in #51250, the pod latency tests began failing. This is because the plugins, in order to support IPv6, need to wait for DAD to finish. Because this
delay is while the kubenet lock is held, it significantly slows down the pod creation rate.
Special notes for your reviewer:
The CNI plugins also do locking for their critical paths, so it is safe to run them concurrently.
Release note: