-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
dockershim/network: pass ipRange CNI capabilities #64445
dockershim/network: pass ipRange CNI capabilities #64445
Conversation
/sig network |
/ok-to-test |
c412449
to
131f76c
Compare
/retest |
CI is all green :-) |
@kubernetes/sig-network-pr-reviews this is ready for review. |
@squeed: Reiterating the mentions to trigger a notification: In response to this:
Instructions 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. |
Will this allow scheduler to consider ip available on a node when making scheduling decisions |
@krmayankk no, this doesn't change anything with the scheduler. It just makes it easier for CNI plugins to get the PodCIDR. Right now, CNI plugins have to contact the apiserver; this removes that round-trip. |
It seems that bandwidth shaping has also been added in #63194 - I'll rework this PR. |
Still waiting for #63194 to be merged; CI is green, it's just pending review. |
131f76c
to
bd6bfbe
Compare
OK, blocker has been merged; this PR is ready for review and merge again. |
bd6bfbe
to
e1933b5
Compare
Flake: #65466 /retest |
/lgtm |
/retest |
Paging @dcbw - can I get an approval? |
hack/local-up-cluster.sh
Outdated
@@ -638,7 +638,7 @@ function start_apiserver { | |||
|
|||
function start_controller_manager { | |||
node_cidr_args="" | |||
if [[ "${NET_PLUGIN}" == "kubenet" ]]; then | |||
if [[ "${NET_PLUGIN}" == "kubenet" || "${NET_PLUGIN}" == "cni" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not all CNI plugins will actually want --allocate-node-cidrs, it really depends on what happens to be in /etc/cni/net.d or what happens to get written there when the net plugin's daemonset starts. So I don't think we can assume CNI==allocate-node-cidrs. Maybe have another environment variable for local-up-cluster.sh that turns this option on if NET_PLUGIN=cni?
e1933b5
to
85d93a6
Compare
Update PR to remove change to This is ready for final review and merge (whenever the flaky tests pass). |
/retest Has CI been green in months? |
CNI now supports passing ipRanges dynamically. Pass podCIDR so that plugins no longer have to look it up.
85d93a6
to
5d9ec20
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dcbw, m1093782566, squeed The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
@squeed: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions 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. |
Automatic merge from submit-queue (batch tested with PRs 64445, 67459, 67434). If you want to cherry-pick this change to another branch, please follow the instructions here. |
What this PR does / why we need it:
Updates the dynamic (capability args) passed from Kubernetes to the CNI plugin. This means CNI plugin authors can offer more features and / or reduce their dependency on the APIServer.
Currently, we only pass the
portMappings
capability. CNI now supportsbandwidth
for bandwidth limiting andipRanges
for preferred IP blocks. This PR adds support for these two new capabilities.Bandwidth limits are provided - as implemented in kubenet - via the pod annotations
kubernetes.io/ingress-bandwidth
andkubernetes.io/egress-bandwidth
.The ipRanges field simply passes the PodCIDR. This does mean that we need to change the NodeReady algorithm. Previously, we would only set NodeNotReady on missing PodCIDR when using Kubenet. Now, if the CNI configuration includes the
ipRanges
capability, we need to do the same.Which issue(s) this PR fixes:
Fixes #64393
Release note: