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

Use ChainExists to reduce memory footprint #2458

Merged
merged 1 commit into from
Jul 26, 2021

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented Jul 23, 2021

ListChains was used to check if a chain exists. The method
used "iptables -t TABLE -S" to list all rules and filter chains from
the output. If there are massive iptables chains and rules configured,
it would cause a lot of space allocated.

benchmark comparison (with 15K iptables rules configured for 1000
services, each of which has 3 endpoints):

name            old time/op    new time/op    delta
EnsureChain-48    78.2ms ± 3%    27.0ms ± 1%  -65.43%  (p=0.008 n=5+5)

name            old alloc/op   new alloc/op   delta
EnsureChain-48    6.06MB ± 0%    0.01MB ± 0%  -99.84%  (p=0.016 n=4+5)

name            old allocs/op  new allocs/op  delta
EnsureChain-48     4.16k ± 0%     0.04k ± 0%     ~     (p=0.079 n=4+5)

Signed-off-by: Quan Tian qtian@vmware.com

For #2457

@tnqn
Copy link
Member Author

tnqn commented Jul 23, 2021

/test-all

1 similar comment
@tnqn
Copy link
Member Author

tnqn commented Jul 23, 2021

/test-all

ListChains was used to check if a chain exists. The method
used "iptables -t TABLE -S" to list all rules and filter chains from
the output. If there are massive iptables chains and rules configured,
it would cause a lot of space allocated.

benchmark comparison (with 15K iptables rules configured for 1000
services, each of which has 3 endpoints):

name            old time/op    new time/op    delta
EnsureChain-48    78.2ms ± 3%    27.0ms ± 1%  -65.43%  (p=0.008 n=5+5)

name            old alloc/op   new alloc/op   delta
EnsureChain-48    6.06MB ± 0%    0.01MB ± 0%  -99.84%  (p=0.016 n=4+5)

name            old allocs/op  new allocs/op  delta
EnsureChain-48     4.16k ± 0%     0.04k ± 0%     ~     (p=0.079 n=4+5)

Signed-off-by: Quan Tian <qtian@vmware.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #2458 (5c59c36) into main (9adf9c9) will increase coverage by 5.18%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2458      +/-   ##
==========================================
+ Coverage   59.79%   64.98%   +5.18%     
==========================================
  Files         284      284              
  Lines       22171    25413    +3242     
==========================================
+ Hits        13258    16515    +3257     
+ Misses       7490     7353     -137     
- Partials     1423     1545     +122     
Flag Coverage Δ
e2e-tests 55.99% <42.85%> (?)
kind-e2e-tests 47.05% <42.85%> (+<0.01%) ⬆️
unit-tests 42.01% <0.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/agent/util/iptables/iptables.go 47.22% <42.85%> (+8.33%) ⬆️
pkg/controller/egress/ipallocator/allocator.go 67.82% <0.00%> (-15.16%) ⬇️
pkg/controller/networkpolicy/endpoint_querier.go 77.64% <0.00%> (-13.79%) ⬇️
pkg/apis/controlplane/v1beta1/conversion.go 72.44% <0.00%> (-11.89%) ⬇️
pkg/legacyapis/core/v1alpha2/register.go 69.23% <0.00%> (-10.77%) ⬇️
pkg/controller/egress/controller.go 76.76% <0.00%> (-10.44%) ⬇️
pkg/apis/stats/register.go 71.42% <0.00%> (-10.39%) ⬇️
pkg/legacyapis/stats/register.go 71.42% <0.00%> (-10.39%) ⬇️
pkg/ovs/openflow/ofctrl_meter.go 33.84% <0.00%> (-10.16%) ⬇️
pkg/legacyapis/security/v1alpha1/register.go 73.33% <0.00%> (-10.00%) ⬇️
... and 269 more

@tnqn tnqn force-pushed the improve-ensure-chain branch from 85e2eef to 5c59c36 Compare July 23, 2021 15:55
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

Is the github.com/coreos/go-iptables upgrade opportunistic or is it required to use ChainExists (to be clear I am fine with the upgrade even if it's opportunistic...)

I think this should also be backported...

@tnqn
Copy link
Member Author

tnqn commented Jul 23, 2021

Is the github.com/coreos/go-iptables upgrade opportunistic or is it required to use ChainExists (to be clear I am fine with the upgrade even if it's opportunistic...)

I think this should also be backported...

Thanks for the quick review.

go-iptables added ChainExists in 0.5.0 coreos/go-iptables@b15012c, I bumped up to latest version which includes a fix for timeout.

Sure, will backport it.

@tnqn
Copy link
Member Author

tnqn commented Jul 23, 2021

/test-all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants