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

Adding traffic shaping support for CNI network driver #63194

Merged
merged 2 commits into from Jul 9, 2018

Conversation

@m1093782566
Copy link
Member

m1093782566 commented Apr 26, 2018

What this PR does / why we need it:

Adding traffic shaping support for CNI network driver - it's also a sub-task of kubenet deprecation work.

Design document is available here: kubernetes/community#1893

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

/cc @freehan @jingax10 @caseydavenport @dcbw

/sig network
/sig node

Release note:

Support traffic shaping for CNI network driver
@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Apr 26, 2018

/assign @thockin

@thockin

This comment has been minimized.

Copy link
Member

thockin commented May 14, 2018

This looks OK, but I'll need someone to really try it out. What can we do about testing?

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented May 15, 2018

I'll need someone to really try it out. What can we do about testing?

My team member @Lion-Wei is working on adding some e2e test cases for this feature.

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented May 21, 2018

/assign @rramkumar1

Can you take a look when you have chance? :)

Thanks!

@dcbw

This comment has been minimized.

Copy link
Member

dcbw commented May 23, 2018

/lgtm though as noted we should really have some tests too

@dcbw

dcbw approved these changes May 23, 2018

@@ -68,6 +69,21 @@ type cniPortMapping struct {
HostIP string `json:"hostIP"`
}

// cniBandwidthEntry maps to the standard CNI bandwidth Capability
// see: https://github.com/containernetworking/cni/blob/master/CONVENTIONS.md

This comment has been minimized.

This comment has been minimized.

@m1093782566

m1093782566 Jun 4, 2018

Author Member

Done.

@Lion-Wei

This comment has been minimized.

Copy link
Contributor

Lion-Wei commented May 24, 2018

@dcbw ,I think we better have some e2e test case about bandwidth, but cni plugin v0.8 have not been released yet, so how to get bandwidth package in our test, that'a a problem. : (

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented May 24, 2018

As @Lion-Wei mentioned, my team has been working on adding e2e tests for some days but we find

  • There is no specific job for running CNI-plugins-related test cases. We need to create new test job in kubernetes/test-infra.

  • If we reuse the test jobs kubeadm-gce-cni-calico and kubeadm-gce-cni-flannel, we should add their --ginkgo.focus like [Feature: Bandwidth]. We need to update the test job in kubernetes/test-infra as well.

So, can we let this PR in first and then update the jobs in kubernetes/test-infra? My team will continue to work on it.

BTW, the functionalities of cni bandwidth plugins is already being tested in containernetworking/plugins repo, so can we just write some UTs in kubernetes repo for now?

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented May 28, 2018

@thockin @dcbw

I just added some tests, PTAL.

// cniBandwidthEntry maps to the standard CNI bandwidth Capability
// see: https://github.com/containernetworking/cni/blob/master/CONVENTIONS.md
type cniBandwidthEntry struct {
// IngressRate is the bandwidth rate in Kbps for traffic through container. 0 for no limit. If ingressRate is set, ingressBurst must also be set

This comment has been minimized.

@squeed

squeed May 30, 2018

Contributor

This was a bug in the CNI documentation; it is actually bits per second.

This comment has been minimized.

@m1093782566

m1093782566 May 31, 2018

Author Member

Done.

bandwidthParam := cniBandwidthEntry{}
if ingress != nil {
bandwidthParam.IngressRate = int(ingress.Value() / 1000)
bandwidthParam.IngressBurst = int(ingress.Value() / 1000)

This comment has been minimized.

@squeed

squeed May 30, 2018

Contributor

This value should be hard-coded to 1600 bits - it's the linux TC default and is what kubenet uses.

This comment has been minimized.

@m1093782566

m1093782566 May 31, 2018

Author Member

Fixed. Thanks!

if ingress != nil || egress != nil {
bandwidthParam := cniBandwidthEntry{}
if ingress != nil {
bandwidthParam.IngressRate = int(ingress.Value() / 1000)

This comment has been minimized.

@squeed

squeed May 30, 2018

Contributor

The documentation was wrong; the units are bits per second.

This comment has been minimized.

@m1093782566

m1093782566 May 31, 2018

Author Member

Done. Thanks!

bandwidthParam.IngressBurst = int(ingress.Value() / 1000)
}
if egress != nil {
bandwidthParam.EgressRate = int(egress.Value() / 1000)

This comment has been minimized.

@squeed

squeed May 30, 2018

Contributor

bits per second here

This comment has been minimized.

@m1093782566

m1093782566 May 31, 2018

Author Member

Done.

}
if egress != nil {
bandwidthParam.EgressRate = int(egress.Value() / 1000)
bandwidthParam.EgressBurst = int(egress.Value() / 1000)

This comment has been minimized.

@squeed

squeed May 30, 2018

Contributor

should be the default of 1600

This comment has been minimized.

@m1093782566

m1093782566 May 31, 2018

Author Member

Done.

// see: https://github.com/containernetworking/cni/blob/master/CONVENTIONS.md
type cniBandwidthEntry struct {
// IngressRate is the bandwidth rate in Kbps for traffic through container. 0 for no limit. If ingressRate is set, ingressBurst must also be set
IngressRate int `json:"ingressRate"`

This comment has been minimized.

@squeed

squeed May 30, 2018

Contributor

All of these fields should have omitEmpty

This comment has been minimized.

@m1093782566

m1093782566 May 31, 2018

Author Member

Done.

@squeed

This comment has been minimized.

Copy link
Contributor

squeed commented May 30, 2018

I did a manual end-to-end test as part of #64445 and found some bugs - left some comments.

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented May 31, 2018

@squeed

Thanks for your comments, we have addressed all of them. PTAL.

BTW, I find there are some bandwidth-related changes in #64445 as well. As my friend(@Lion-Wei) and I started this work 2 months ago and have spent lots of time doing it, can we let this PR in first and then rework #64445?

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Jul 8, 2018

@thockin @dcbw Thanks for reviewing.

I just pushed an update. PTAL.

@m1093782566 m1093782566 force-pushed the m1093782566:cni-ts branch from fff4178 to 25767e8 Jul 8, 2018

@k8s-ci-robot k8s-ci-robot added approved and removed lgtm labels Jul 8, 2018

@m1093782566 m1093782566 force-pushed the m1093782566:cni-ts branch from 25767e8 to fe163e5 Jul 8, 2018

@Lion-Wei

This comment has been minimized.

Copy link
Contributor

Lion-Wei commented Jul 9, 2018

/lgtm
/retest

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Jul 9, 2018

Thanks @Lion-Wei

/hold

for @thockin final review :)

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jul 9, 2018

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Jul 9, 2018

/hold cancel

@m1093782566 m1093782566 force-pushed the m1093782566:cni-ts branch from d5ca499 to 34d848e Jul 9, 2018

@timchenxiaoyu

This comment has been minimized.

Copy link
Contributor

timchenxiaoyu commented Jul 9, 2018

does support change bandwith dynamic when pods in runtime ?

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Jul 9, 2018

@timchenxiaoyu

does support change bandwith dynamic when pods in runtime?

Currently, it doesn't support - we configure bandwidth limit in pod start up. I think we can support it if we get better consensus.

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jul 9, 2018

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 9, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 9, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, Lion-Wei, m1093782566, thockin

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

/test all [submit-queue is verifying that this PR is safe to merge]

@m1093782566

This comment has been minimized.

Copy link
Member Author

m1093782566 commented Jul 9, 2018

THANKS @thockin

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 9, 2018

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit e943d09 into kubernetes:master Jul 9, 2018

16 of 17 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-kubemark-e2e-gce-big
Details
cla/linuxfoundation Lion-Wei authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.