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

Dev/exclude subnets from traffic shaping2 #921

Conversation

oOraph
Copy link
Contributor

@oOraph oOraph commented Jul 6, 2023

bandwidth: possibility to exclude some subnets from traffic shapping

what changed:

we had to refactor the bandwidth plugin and switch from a classless qdisc (tbf)
to a classful qdisc (htb).

subnets are to be provided in config or runtimeconfig just like other parameters

unit and integration tests were also adapted in consequence

unrelated changes:

test fixes: the most important tests were just silently skipped due to ginkgo Measure deprecation
(the ones actually checking the effectiveness of the traffic control)

@oOraph oOraph marked this pull request as draft July 6, 2023 09:58
@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from 956ad1c to c1af94f Compare July 6, 2023 09:58
@oOraph oOraph marked this pull request as ready for review July 6, 2023 09:59
@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch 6 times, most recently from 2424028 to 644ebf7 Compare July 7, 2023 12:35
@squeed
Copy link
Member

squeed commented Jul 21, 2023

@oOraph thanks for this! And very good catch w.r.t. Ginkgo Measure.

Can you check the lint error?

@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from 644ebf7 to 6021f78 Compare July 21, 2023 12:14
@oOraph
Copy link
Contributor Author

oOraph commented Jul 21, 2023

Can you check the lint error?

@squeed Done :)

Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

Hi @oOraph I have a question about the design of the PR.

As far as I understood, this PR is going to introduce folloiwng tc filtering config.

tc qdisc add dev <interfaceName> root handle 1: htb default 30
tc class add dev <interfaceName> parent 1: classid 1:30 htb rate <rateInBits> burst <burstInBits>
tc class add dev <interfaceName> parent 1: classid 1:1 htb rate 100000000000
tc filter add dev <interfaceName> parent 1: protocol <protocol> prio 16 u32 match ip dst <subnet> flowid 1:1

It might be good to have, however, on the otherside, user may want to do opposite configurations (adding 'target CIDR' for rate limit), such as

tc qdisc add dev <interfaceName> root handle 1: htb default 30
tc class add dev <interfaceName> parent 1: classid 1:1 htb rate <rateInBits> burst <burstInBits>
tc class add dev <interfaceName> parent 1: classid 1:30 htb rate 100000000000
tc filter add dev <interfaceName> parent 1: protocol <protocol> prio 16 u32 match ip dst <subnet> flowid 1:1

Your proposed config may not fit second one. Is there any way to enhance your desin to support second config and more flexible tc configurations?

for _, subnet := range subnets {
_, _, err := net.ParseCIDR(subnet)
if err != nil {
return fmt.Errorf("bad subnet provided %s, details %s", subnet, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

how about to use %q for subnet (to double quote) and %v for error, to fit to other Errorf in the file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure. done

Comment on lines 191 to 210
err = validateSubnets(bandwidth.NonShapedSubnets)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but we could change as other code does

if err := json.Unmarshal(stdin, &conf); err != nil {
return nil, fmt.Errorf("failed to parse network configuration: %v", err)
}

	if err = validateSubnets(bandwidth.NonShapedSubnets); err != nil {
		return err
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


}

protocol := syscall.ETH_P_ALL
Copy link
Contributor

Choose a reason for hiding this comment

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

why protocol is all, instead of ip' or ipv6' (as you commented just above?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from 6021f78 to 729c1d1 Compare July 27, 2023 07:44
@oOraph
Copy link
Contributor Author

oOraph commented Jul 27, 2023

@s1061123 , about #921 (review)
Done as you wished :). now you can either set unshappedSubnets or shappedSubnets to specify subnets to be excluded from/included in traffic shapping. Both parameters are mutually exclusive and will result in error when simultaneously defined

@s1061123
Copy link
Contributor

@oOraph Thank you to incorporate my comments! I will review code tomorrow again!

@oOraph oOraph requested a review from s1061123 August 4, 2023 08:49
Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

Hi, almost code is reasonable to me. BTW, could you provide document for that, in https://github.com/containernetworking/cni.dev ?

var err error

name := "myBWnet"
Copy link
Contributor

Choose a reason for hiding this comment

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

how about to embedded the name in cni config directly (in line 47)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

It("Should fail if specified ShapedSubnets are not valid CIDRs", func() {
err := validateSubnets([]string{}, []string{"10.0.0.0/8", "hello"})
Expect(err).To(HaveOccurred())
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for improve unit tests. However, on the other side, this file is now the biggest file in the repository. Could you split into several _test.go file for ease of maintenance, if you don't mind it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, split the file in three:
gathered all "functional" tests in one file (tests checking the effectiveness of the shaping)
all config related tests in another
left all other tests together in the original test file


// cmd = exec.Command("/usr/sbin/tc", "filter", "add", "dev", interfaceName, "parent", "1:", "protocol", protocol,
// "prio", "16", "u32", "match", "ip", "dst", subnet, "flowid", "1:1")

Copy link
Contributor

Choose a reason for hiding this comment

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

May remove this empty line (line:225)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

}

// Now add filters to redirect subnets to the class 1 if excluded instead of the default one (30)

Copy link
Contributor

Choose a reason for hiding this comment

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

May remove this empty line (line:220)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

keepBytes = 4
// prio/pref needs to be changed if we change the protocol, looks like we cannot mix protocols with the same pref
prio = 16

Copy link
Contributor

Choose a reason for hiding this comment

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

May remove this empty line (line:249)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@oOraph
Copy link
Contributor Author

oOraph commented Aug 10, 2023

Hi, almost code is reasonable to me. BTW, could you provide document for that, in https://github.com/containernetworking/cni.dev ?

done -> containernetworking/cni.dev#130

@oOraph oOraph requested a review from s1061123 August 10, 2023 08:03
@oOraph oOraph changed the title Dev/exclude subnets from traffic shapping2 Dev/exclude subnets from traffic shaping2 Aug 10, 2023
@oOraph
Copy link
Contributor Author

oOraph commented Sep 7, 2023

Hello @s1061123 @MikeZappa87 :). Do you still see anything wrong or missing with this pr ?

Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

Hi @oOraph thank you for the great work. Almost Okey to me. I give two minor comment to fix year. That's all. Thanks again!

@@ -0,0 +1,563 @@
// Copyright 2018 CNI authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix copyright year: 2018 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@@ -0,0 +1,824 @@
// Copyright 2018 CNI authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix copyright year: 2018 -> 2023

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from 2e3f7d6 to 3d93935 Compare September 11, 2023 12:02
@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from 3d93935 to 21cabd1 Compare October 17, 2023 14:07
Copy link
Contributor

@s1061123 s1061123 left a comment

Choose a reason for hiding this comment

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

/lgtm

Could you please address doc changes in another PR in doc repo, https://github.com/containernetworking/cni.dev?

@oOraph
Copy link
Contributor Author

oOraph commented Nov 13, 2023

/lgtm

Could you please address doc changes in another PR in doc repo, https://github.com/containernetworking/cni.dev?

Sure it's already done here:
containernetworking/cni.dev#130

@oOraph oOraph force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from 21cabd1 to adcf445 Compare November 16, 2023 17:42
@oOraph
Copy link
Contributor Author

oOraph commented Nov 16, 2023

just rebased on upstream/main to fix lint issue

@MikeZappa87
Copy link
Contributor

This looks good. I did not run the plugin though.

oOraph and others added 5 commits April 8, 2024 15:39
what changed:

we had to refactor the bandwidth plugin and switch from a classless qdisc (tbf)
to a classful qdisc (htb).

subnets are to be provided in config or runtimeconfig just like other parameters

unit and integration tests were also adapted in consequence

unrelated changes:

test fixes: the most important tests were just silently skipped due to ginkgo Measure deprecation
(the ones actually checking the effectiveness of the traffic control)

Signed-off-by: Raphael <oOraph@users.noreply.github.com>
…rom shaping

Signed-off-by: Raphael <oOraph@users.noreply.github.com>
Signed-off-by: Raphael <oOraph@users.noreply.github.com>
even if json unmarshalling in golang with the standard libs is case unsensitive regarding the keys

Signed-off-by: Raphael <oOraph@users.noreply.github.com>
Signed-off-by: Tomofumi Hayashi <tohayash@redhat.com>
@s1061123 s1061123 force-pushed the dev/exclude_subnets_from_traffic_shapping2 branch from adcf445 to ccc1cfa Compare April 8, 2024 06:42
@s1061123 s1061123 merged commit ef076af into containernetworking:main Apr 8, 2024
5 checks passed
@nayihz
Copy link

nayihz commented Apr 19, 2024

Hi all, I am curious why we need to create ifb devices and tc rules in host-side network namespace, rather than in container-side.

@oOraph
Copy link
Contributor Author

oOraph commented Apr 23, 2024

Hi all, I am curious why we need to create ifb devices and tc rules in host-side network namespace, rather than in container-side.

Hello @nayihz :). If I understand correctly what you mean we need to apply tc rules on host side because the container side is the user side and we do not want them to arbitrarily edit/cancel them.

@oOraph
Copy link
Contributor Author

oOraph commented Apr 23, 2024

@s1061123 @MikeZappa87 thanks for merging. Maybe we should also merge the doc wdyt ?
containernetworking/cni.dev#130

h0nIg added a commit to h0nIg/plugins that referenced this pull request Oct 11, 2024
…clude_subnets_from_traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.
h0nIg added a commit to h0nIg/plugins that referenced this pull request Oct 11, 2024
…clude_subnets_from_traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.
h0nIg added a commit to h0nIg/plugins that referenced this pull request Oct 11, 2024
…clude_subnets_from_traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.

Signed-off-by: h0nIg <h0nIg@users.noreply.github.com>
h0nIg added a commit to h0nIg/plugins that referenced this pull request Oct 11, 2024
…clude_subnets_from_traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.

Signed-off-by: h0nIg <h0nIg@users.noreply.github.com>
h0nIg added a commit to h0nIg/plugins that referenced this pull request Oct 11, 2024
…clude_subnets_from_traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.

Signed-off-by: h0nIg <h0nIg@users.noreply.github.com>
h0nIg added a commit to h0nIg/plugins that referenced this pull request Oct 11, 2024
…clude_subnets_from_traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.

Signed-off-by: h0nIg <h0nIg@users.noreply.github.com>
squeed pushed a commit that referenced this pull request Oct 14, 2024
…traffic_shapping2"

This reverts commit ef076af, reversing
changes made to 5974089.

Signed-off-by: h0nIg <h0nIg@users.noreply.github.com>
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.

6 participants