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.

Sign up for GitHub

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

Jump to bottom

Add support for network device limits.priority option #12135

Merged
merged 7 commits into from
Sep 25, 2023
Merged

Add support for network device limits.priority option #12135

merged 7 commits into from
Sep 25, 2023

Conversation

mihalicyn
Copy link
Member

@mihalicyn mihalicyn commented Aug 10, 2023 •
edited
Loading

lxd/device: add support for network device limits.priority option

This is a replacement feature for per-instance limits.network.priority option.
New approach does not require netprio cgroup to be suppored (it's from legacy
cgroup v1) and also it allows to set priority for virtual machine instances.

The recommended Linux kernel version (for full functionality) is 5.16.

@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch from d252419 to f5a7b8a Compare August 10, 2023 12:06
roosterfish
roosterfish previously requested changes Aug 16, 2023
View reviewed changes
Copy link
Contributor

@roosterfish roosterfish left a comment

Choose a reason for hiding this comment

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

Looks good, just a few questions and remarks :)

lxd/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
lxd/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
lxd/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
lxd/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
@mihalicyn
Copy link
Member Author

Looks good, just a few questions and remarks :)

Thanks for your review, Julian! ;-)

masnax
masnax previously requested changes Aug 17, 2023
View reviewed changes
Copy link
Contributor

@masnax masnax left a comment

Choose a reason for hiding this comment

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

lgtm!, just a question and one small nit :)

lxd/instance/drivers/driver_lxc.go Outdated Show resolved Hide resolved
@tomponline tomponline self-requested a review August 21, 2023 11:30
@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch from f5a7b8a to aaeb652 Compare August 21, 2023 12:29
@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch from aaeb652 to d514ab6 Compare August 21, 2023 12:47
tomponline
tomponline reviewed Aug 22, 2023
View reviewed changes
lxd/devices.go Outdated Show resolved Hide resolved
@tomponline
Copy link
Member

@mihalicyn whilst we think about the netfilter netprio stuff, do you want to split the bug fixes for the cgroup functionality into a separate PR we can merge for LXD 5.17?

@mihalicyn
Copy link
Member Author

@mihalicyn whilst we think about the netfilter netprio stuff, do you want to split the bug fixes for the cgroup functionality into a separate PR we can merge for LXD 5.17?

sure as you say. I just thought that this thing is simple enough to be merged at once.

@mihalicyn mihalicyn mentioned this pull request Aug 22, 2023
Merged
@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch from d514ab6 to 67bcf99 Compare August 22, 2023 08:48
@tomponline
Copy link
Member

@mihalicyn when you're ready for another review please can you click the refresh review button on my name top right so that I get a notification and it'll appear on my todo list on https://github.com/pulls/review-requested

Thanks

@github-actions github-actions bot added the Documentation Documentation needs updating label Sep 13, 2023
@mihalicyn mihalicyn changed the title Support "limits.network.priority" config using nftables/xtables Add support for network device limits.priority option Sep 13, 2023
@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch 2 times, most recently from fdfa52e to 5bc9328 Compare September 14, 2023 08:14
@mihalicyn
Copy link
Member Author

@mihalicyn if you can fix the spelling mistake urgently "cgroupv" then I can merge this.

sure, but I also wanted to write extra tests with lxd-ci and test everything with different (3) NIC types.

@tomponline
Copy link
Member

tomponline commented Sep 21, 2023 •
edited
Loading

@mihalicyn if you can fix the spelling mistake urgently "cgroupv" then I can merge this.

sure, but I also wanted to write extra tests with lxd-ci and test everything with different (3) NIC types.

OK I'll hold off then. Thanks!

@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch 2 times, most recently from 3677e9a to 192b7fb Compare September 21, 2023 09:37
ru-fu
ru-fu previously approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@ru-fu ru-fu left a comment

Choose a reason for hiding this comment

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

Doc update looks good now. :)

@mihalicyn mihalicyn force-pushed the netprio_cgroup_v2_replacement branch 7 times, most recently from 74194ec to 7db74f6 Compare September 21, 2023 13:44
@mihalicyn
Copy link
Member Author

Ok. Now limits.priority works for bridged NICs too. This feature is only available for nftables firewall drivers because xtables lacks of having egress hooks for device or bridge-level hook that allows us filtering based on the input device name. After discussion with Tom we've decided to add special check to the limits.priority handler that prevents this option from being silently ignored if something is not supported.

Taking this change into account now I'm not sure that I need to change anything in the https://github.com/canonical/lxd-ci/blob/main/tests/network-bridge-firewall test because this test is about checking that everything works equally for nftables/xtables. For limits.priority it's not the case, unfortunately.

@mihalicyn
lxd/firewall: add support for InstanceSetupNetPrio/InstanceClearNetPrio
cd02832 
For xtables we use xt_CLASSIFY target to set skb->priority and
NF_INET_FORWARD hook. Which is not an ideal solution because
it won't work in bridged configuration. But we can do nothing with that.

With nftables we use netdev family and egress hook introduced in
torvalds/linux@42df6e1
(starting from Linux kernel v5.16)
it allows to catch and process all skb's which go out from the container's veth device.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
lxd/device: extend networkSetupHostVethLimits signature
d23780c 
Make it to take *deviceCommon, oldConfig and "is bridged" flag

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
lxd/device: add support for network device limits.priority option
daef95d 
This is a replacement feature for per-instance limits.network.priority option.
New approach does not require netprio cgroup to be suppored (it's from legacy
cgroup v1) and also it allows to set priority for virtual machine instances.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
test: add test for limits.priority per-device setting
a1d670f 
Add nic_bridged test for limits.priority per-device option.

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
api: instances_nic_limits_priority
e570caa 
Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
lxc/cgroup/init: change warning that network priority will be ignored…
51c6b6f 
… without NetPrio cgroup

Currently, we issue the following warning in the logs:

Couldn't find the CGroup network priority controller, network priority will be ignored

Let's change text to:

Couldn't find the CGroup network priority controller, per-instance network priority will be ignored. Please use per-device limits.priority instead

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
doc: clarify that limits.network.priority option is deprecated
47d6d6f 
Update limits.network.priority documentation description and
mention that option is deprecated and replaced by per-NIC
limits.priority option and run:
$ make update-metadata

Signed-off-by: Alexander Mikhalitsyn <aleksandr.mikhalitsyn@canonical.com>
@mihalicyn
Copy link
Member Author

I've rebased PR and resolved conflicts.

tomponline
tomponline approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@tomponline tomponline left a comment

Choose a reason for hiding this comment

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

LGTM!

@tomponline
Copy link
Member

@mihalicyn we should just add a check for nftables in lxd-ci bridge firewall now.

@tomponline tomponline dismissed stale reviews from masnax and roosterfish September 25, 2023 09:56

changes applied

@tomponline tomponline merged commit a700a2f into canonical:main Sep 25, 2023
23 of 24 checks passed
@mihalicyn
Copy link
Member Author

@mihalicyn we should just add a check for nftables in lxd-ci bridge firewall now.

sure, I think we discuss approach to this thing during our regular call ;-)

@mihalicyn mihalicyn mentioned this pull request Sep 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomponline tomponline tomponline approved these changes

@ru-fu ru-fu ru-fu left review comments

@masnax masnax Awaiting requested review from masnax

@roosterfish roosterfish Awaiting requested review from roosterfish

Assignees
No one assigned
Labels
API Changes to the REST API Documentation Documentation needs updating
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mihalicyn @tomponline @escabo @roosterfish @ru-fu @masnax