network limit tbf #350

vincent178 · 2020-03-18T21:13:53Z

What problem does this PR solve?

This PR is adding bandwidth limit feature for network chaos which specify in #303, which use tbf qdisc under the hood.

What is changed and how does it work?

proto add message SetTbf and DeleteTbf
NetworkChaosSpec add BandwidthSpec
Add tbf controller which reconciler when k8s config
chaosdaemon implement SetTbf and DeleteTbf grpc function
Add related document
Add related example

Some notable things I would like to work next is

refactor networkchaos netem and tbf with a more general implementation, which is related to 2
make netem and tbf work together

Check List

Tests

Unit test
Manual test (add detailed scripts or steps below)

manually run kubectl apply -f network-chaos.yaml and kubectl delete -f network-chaos.yaml below.

apiVersion: pingcap.com/v1alpha1
kind: NetworkChaos
metadata:
  name: tbf-test
  namespace: default
spec:
  action: bandwidth
  mode: one
  selector:
    labelSelectors:
      "app": "tbf-test"
  bandwidth:
    rate: 10000kbps
    limit: 1000
    buffer: 10000

screenshot below

Code changes

Has Go code change

Side effects

n/a

Related changes

Need to update the documentation
done

Does this PR introduce a user-facing change?:

Add new action bandwidth in network chaos to support bandwidth limit.

CLAassistant · 2020-03-18T21:13:57Z

All committers have signed the CLA.

codecov-io · 2020-03-19T05:18:08Z

Codecov Report

Merging #350 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #350   +/-   ##
=======================================
  Coverage   58.63%   58.63%           
=======================================
  Files          60       60           
  Lines        3626     3626           
=======================================
  Hits         2126     2126           
  Misses       1332     1332           
  Partials      168      168

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ed2d27...8ed2d27. Read the comment docs.

controllers/networkchaos/tbf/types.go

YangKeao

Rest LGTM.

@cwen0 PTAL

pkg/chaosdaemon/netlink_linux.go

api/v1alpha1/networkchaos_types.go

examples/network-bandwidth-example.yaml

api/v1alpha1/networkchaos_types.go

YangKeao · 2020-03-24T14:07:02Z

Please also document that minikube doesn't support this feature, as CONFIG_NET_SCH_TBF was disabled in minikube's image.

I will try to add this kernel option for minikube later.

api/v1alpha1/networkchaos_types.go

doc/network_chaos.md

YangKeao

LGTM

@cwen0 PTAL

cwen0

Rest LGTM

api/v1alpha1/networkchaos_types.go

controllers/networkchaos/types.go

cwen0 · 2020-03-26T16:00:23Z

examples/network-bandwidth-example.yaml

+      "app.kubernetes.io/component": "tikv"
+  bandwidth:
+    rate: 100kbps
+    limit: 100


What will happen if we set this value to "0"?

Good question, string "0" will be fail for type checking error, and int 0 will fail to apply qdisc as limit is required for tbf (in the background). One thing could improve this is add a validation before actually apply this.

pkg/chaosdaemon/netlink_linux.go

cwen0 · 2020-03-26T16:12:35Z

pkg/chaosdaemon/netlink_linux.go

+}
+
+// TODO(vincent178): this is ok if we only apply either netem or tbf, revisit this if we want both working
+func qdiscExists(qdisc netlink.Qdisc, handler *netlink.Handle, link netlink.Link) (bool, error) {


Add some unit tests for this.

I think this function is simple and straightforward, but I do find the comment is not valid anymore, so I remove it. Thank you!

AngleNet · 2020-03-27T00:26:50Z

@vincent178 Is this PR ready for another round of review now ?

vincent178 · 2020-03-27T00:30:05Z

@AngleNet I'm adding validation for api object, should be quick though. Will let you guys know after that.

vincent178 · 2020-03-27T01:15:43Z

$ kubectl apply -f network-chaos.yaml
The NetworkChaos "tbf-test" is invalid:
* spec.bandwidth.buffer: Invalid value: 1: spec.bandwidth.buffer in body should be greater than or equal to 1
* spec.bandwidth.limit: Invalid value: 1: spec.bandwidth.limit in body should be greater than or equal to 1

vincent178 · 2020-03-27T01:21:46Z

Hi @AngleNet @YangKeao @cwen0 the code is ready for review now. Thanks

pkg/chaosdaemon/tbf_darwin.go

api/v1alpha1/networkchaos_types.go

This reverts commit f7877b7.

cwen0

LGTM

cwen0 · 2020-03-27T10:22:08Z

@YangKeao @fewdan PTAL

YangKeao

LGTM

YangKeao · 2020-03-27T10:46:46Z

/merge

sre-bot · 2020-03-27T10:49:16Z

/run-all-tests

hound · 2020-03-27T10:51:29Z

controllers/test/types.go

+	return nil, mockError("SetTbf")
+}
+
+func (c *MockChaosDaemonClient) DeleteTbf(ctx context.Context, in *chaosdaemon.TbfRequest, opts ...grpc.CallOption) (*empty.Empty, error) {


exported method MockChaosDaemonClient.DeleteTbf should have comment or be unexported

hound · 2020-03-27T10:51:29Z

controllers/test/types.go

@@ -43,6 +43,14 @@ func (c *MockChaosDaemonClient) DeleteNetem(ctx context.Context, in *chaosdaemon
 	return nil, mockError("DeleteNetem")
 }

+func (c *MockChaosDaemonClient) SetTbf(ctx context.Context, in *chaosdaemon.TbfRequest, opts ...grpc.CallOption) (*empty.Empty, error) {


exported method MockChaosDaemonClient.SetTbf should have comment or be unexported

hound · 2020-03-27T10:51:29Z

controllers/networkchaos/tbf/types.go

+	networkTbfActionMsg = "network tbf action duration %s"
+)
+
+type Reconciler struct {


exported type Reconciler should have comment or be unexported

sre-bot · 2020-03-27T10:53:23Z

/run-all-tests

sre-bot · 2020-03-27T10:55:25Z

@vincent178 merge failed.

* proto and tbf server apply logic * refactor common logic to netlink_linux and apply it in tbf_server * [temp] remove limit in networkchaos * make fmt and make yaml * fix var * add mock implementation * fix missing grpc option * rough implementation of API spec and controller * implement InnerReconciler * make fmt * rename to bandwidth * make * fix api zero value parse issue * fix remove tbf issue * adjust type for bandwidth * commit yaml and test * fix tbf server test * add unit support for rate * add network bandwidth example * add documentation * make yaml * make fmt * update pb * make yaml * update log * refactor netem with toQdiscFunc * update based on comments * update based on comments * optional * fix nil pointer dereference * fix typo * update document * update document * update based on comments * add more doc * add unit test and add license comment * add comments for tbf * fix comment * add license comment * fix ut * add validation * update licensed * update code * Revert "update licensed" This reverts commit f7877b7. * update license file to 2020 Co-authored-by: pingcap-github-bot <[email protected]>

sre-bot added the contribution label Mar 18, 2020

vincent178 force-pushed the network-limit-tbf branch from b6927f0 to 9e0f82a Compare March 18, 2020 21:39

vincent178 force-pushed the network-limit-tbf branch 4 times, most recently from b523ffc to 2004080 Compare March 23, 2020 23:42

vincent178 changed the title ~~[WIP] network limit tbf~~ network limit tbf Mar 24, 2020

YangKeao reviewed Mar 24, 2020

View reviewed changes

controllers/networkchaos/tbf/types.go Outdated Show resolved Hide resolved

YangKeao reviewed Mar 24, 2020

View reviewed changes

pkg/chaosdaemon/netlink_linux.go Show resolved Hide resolved

api/v1alpha1/networkchaos_types.go Show resolved Hide resolved

api/v1alpha1/networkchaos_types.go Outdated Show resolved Hide resolved

vincent178 force-pushed the network-limit-tbf branch from 4d49072 to 9b251c2 Compare March 24, 2020 12:26

hound bot deleted a comment from YangKeao Mar 24, 2020

YangKeao reviewed Mar 24, 2020

View reviewed changes

examples/network-bandwidth-example.yaml Outdated Show resolved Hide resolved

YangKeao reviewed Mar 24, 2020

View reviewed changes

api/v1alpha1/networkchaos_types.go Show resolved Hide resolved

api/v1alpha1/networkchaos_types.go Outdated Show resolved Hide resolved

AngleNet reviewed Mar 25, 2020

View reviewed changes

api/v1alpha1/networkchaos_types.go Outdated Show resolved Hide resolved

doc/network_chaos.md Outdated Show resolved Hide resolved

doc/network_chaos.md Show resolved Hide resolved

vincent178 force-pushed the network-limit-tbf branch from 65d2520 to 2e59872 Compare March 26, 2020 05:08

YangKeao previously approved these changes Mar 26, 2020

View reviewed changes

YangKeao mentioned this pull request Mar 26, 2020

Add sch_tbf and extend filter ipset kernel module for bandwidth shaping kubernetes/minikube#7255

Merged

cwen0 reviewed Mar 26, 2020

View reviewed changes

vincent178 dismissed YangKeao’s stale review via 92e7329 March 26, 2020 20:39

vincent178 force-pushed the network-limit-tbf branch from d9ca093 to d15bbe4 Compare March 26, 2020 20:53

cwen0 reviewed Mar 27, 2020

View reviewed changes

pkg/chaosdaemon/tbf_darwin.go Outdated Show resolved Hide resolved

fewdan reviewed Mar 27, 2020

View reviewed changes

api/v1alpha1/networkchaos_types.go Outdated Show resolved Hide resolved

api/v1alpha1/networkchaos_types.go Outdated Show resolved Hide resolved

proto and tbf server apply logic

e013bdf

vincent178 added 13 commits March 27, 2020 15:57

fix nil pointer dereference

873f823

fix typo

4869db6

update document

8b41594

update document

1d3fb45

update based on comments

7ebcd41

add more doc

4fdfa37

add unit test and add license comment

fc02c7a

add comments for tbf

76a2b06

fix comment

6fb9e13

add license comment

04aa217

fix ut

e58285c

add validation

180e0a3

update licensed

f7877b7

vincent178 force-pushed the network-limit-tbf branch from 8ed2d27 to f7877b7 Compare March 27, 2020 07:57

vincent178 added 3 commits March 27, 2020 16:03

update code

15a47fe

Revert "update licensed"

7f1d0e7

This reverts commit f7877b7.

update license file to 2020

08694b6

cwen0 approved these changes Mar 27, 2020

View reviewed changes

YangKeao approved these changes Mar 27, 2020

View reviewed changes

sre-bot added the status/can-merge label Mar 27, 2020

Merge branch 'master' into network-limit-tbf

35892c1

Merge branch 'master' into network-limit-tbf

5fd765a

hound bot reviewed Mar 27, 2020

View reviewed changes

YangKeao merged commit e0bfd44 into chaos-mesh:master Mar 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network limit tbf #350

network limit tbf #350

vincent178 commented Mar 18, 2020 •

edited

Loading

CLAassistant commented Mar 18, 2020 •

edited

Loading

codecov-io commented Mar 19, 2020 •

edited

Loading

YangKeao left a comment

YangKeao commented Mar 24, 2020

YangKeao left a comment

cwen0 left a comment

cwen0 Mar 26, 2020

vincent178 Mar 26, 2020 •

edited

Loading

cwen0 Mar 26, 2020

vincent178 Mar 26, 2020

AngleNet commented Mar 27, 2020

vincent178 commented Mar 27, 2020

vincent178 commented Mar 27, 2020

vincent178 commented Mar 27, 2020

cwen0 left a comment

cwen0 commented Mar 27, 2020

YangKeao left a comment

YangKeao commented Mar 27, 2020

sre-bot commented Mar 27, 2020

hound bot Mar 27, 2020

hound bot Mar 27, 2020

hound bot Mar 27, 2020

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

network limit tbf #350

network limit tbf #350

Conversation

vincent178 commented Mar 18, 2020 • edited Loading

What problem does this PR solve?

What is changed and how does it work?

Check List

Does this PR introduce a user-facing change?:

CLAassistant commented Mar 18, 2020 • edited Loading

codecov-io commented Mar 19, 2020 • edited Loading

Codecov Report

YangKeao left a comment

Choose a reason for hiding this comment

YangKeao commented Mar 24, 2020

YangKeao left a comment

Choose a reason for hiding this comment

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 Mar 26, 2020

Choose a reason for hiding this comment

vincent178 Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

cwen0 Mar 26, 2020

Choose a reason for hiding this comment

vincent178 Mar 26, 2020

Choose a reason for hiding this comment

AngleNet commented Mar 27, 2020

vincent178 commented Mar 27, 2020

vincent178 commented Mar 27, 2020

vincent178 commented Mar 27, 2020

cwen0 left a comment

Choose a reason for hiding this comment

cwen0 commented Mar 27, 2020

YangKeao left a comment

Choose a reason for hiding this comment

YangKeao commented Mar 27, 2020

sre-bot commented Mar 27, 2020

hound bot Mar 27, 2020

Choose a reason for hiding this comment

hound bot Mar 27, 2020

Choose a reason for hiding this comment

hound bot Mar 27, 2020

Choose a reason for hiding this comment

sre-bot commented Mar 27, 2020

sre-bot commented Mar 27, 2020

vincent178 commented Mar 18, 2020 •

edited

Loading

CLAassistant commented Mar 18, 2020 •

edited

Loading

codecov-io commented Mar 19, 2020 •

edited

Loading

vincent178 Mar 26, 2020 •

edited

Loading