pkg/iptables: reduce calls to iptables #116
Conversation
Currently, every time the iptables controller syncs rules, it spawns an an iptables process for every rule it checks. This causes two problems: 1. it creates unnecessary load on the system; and 2. it causes contention on the xtables lock file. This commit creates a lazy cache for iptables rules and chains that avoids spawning iptables processes. This means that each time the iptables rules are reconciled, if no rules need to be changed then at most one iptables process should be spawned to check all of the rules in a chain and at most one process should be spawned to check all of the chains in a table. Note: the success of this reduction in calls to iptables depends on a somewhat fragile comparison of iptables rule text. The text of any rule must match exactly, including the order of the flags. An improvement to come would be to implement an iptables rule parser than can be used to check semantic equivalence betweem iptables rules. Signed-off-by: Lucas Servén Marín <[email protected]>
|
Since you mention this is so fragile, is it worth adding an info log line with the amount of rules that needed updating? (but not output anything if nothing needs changing) |
|
@SerialVelocity I added the log statement to the iptables controller. PTAL 👍 |
|
Did you accidentally push this to the latest tag? The reason I ask is the latest tag was updated an hour ago but the last master commit was 2 days ago. |
|
No I didnt :/ let me see what happened, maybe CI is misconfigured |
|
The 'latest' tag has exactly the same hash as the images tagged '0d0fdda619938fc1671dadff4c55597ed1795913', and in fact they were pushed at the same time, ~8 hours ago, which corresponds to midnight UTC. This is the SHA of the latest commit on master. These images were built by the CI cron job that runs every day at midnight and rebuilds the tip of master. |
|
ping @SerialVelocity have you have any time to rebase your fork onto this branch and test it? I removed the log lines as you suggested so we should be able to see the reduction in CPU load. Thanks for your help! |
|
Oh! I didn't realise you were waiting on me! I'll check it out now. |
|
Is there a 5 second timer anywhere? Kilo seems to run the iptables commands every 5 seconds. (It's a lot less than before though!) |
|
Yes exactly: https://github.com/squat/kilo/blob/main/pkg/iptables/iptables.go#L226 |
|
If something changes and the iptables rules need updating, does it have to wait for that 5 second loop? If not, then yeah, 30s sounds much better (if not even higher) |
|
If the mesh changes, then the new rules are applied immediately rather than waiting for this timer. The 5 second timeout is to quickly catch rules that were changed by another process or someone mucking around on the host. This reconciliation is "just in case" so we can probably turn it down a bit. Ok, with that in place i think we can merge this! Thanks for your input and testing @SerialVelocity ! |
This commit adds a logger to the iptables controller using the options pattern. It also logs when the controller needs to reset rules, to be able to identify costly reconciliations. Signed-off-by: Lucas Servén Marín <[email protected]>
squat
force-pushed
the
reduce_iptables_calls
branch
from
82d17a2
to
4b32c49
Compare
|
I think having a short 5 second reconciliation loop is probably bad as people will wonder why they have small blips of network connectivity. Having this much higher means the issue will be noticed, debugged, and fixed. |
Since #116 implemented fragile comparisons of iptables rules to avoid calling the iptables binary excessively during every reconciliation, the iptables rules for IPIP encapsulation must be updated to match the expected output. One complication is that rather than returning the protocol number in the rule, iptables resolves the protocol number to a name by looking up the number in the netd protocols database. This name can vary depending on the host's environment. This commit adds two solutions for resolving the protocol name: 1. a fixed mapping to the string `ipencap`, which should always work for Kilo whenever it runs in the Alpine Linux container; and 2. a runtime lookup using the netd database, which only works if Kilo is compiled with CGO and is meant to be used only if Kilo is not running in the normal container environment. Signed-off-by: Lucas Servén Marín <[email protected]>
Currently, every time the iptables controller syncs rules, it spawns an
an iptables process for every rule it checks. This causes two problems:
This commit creates a lazy cache for iptables rules and chains that
avoids spawning iptables processes. This means that each time the
iptables rules are reconciled, if no rules need to be changed then at
most one iptables process should be spawned to check all of the rules in
a chain and at most one process should be spawned to check all of the
chains in a table.
Note: the success of this reduction in calls to iptables depends on a
somewhat fragile comparison of iptables rule text. The text of any rule
must match exactly, including the order of the flags. An improvement to
come would be to implement an iptables rule parser than can be used to
check semantic equivalence betweem iptables rules.
Signed-off-by: Lucas Servén Marín [email protected]
cc @leonnicolas @SerialVelocity
Fixes #113