bhouse-nexthop commented on code in PR #14350:
URL: https://github.com/apache/cloudstack/pull/14350#discussion_r4273529803
##########
systemvm/debian/opt/cloud/bin/configure.py:
##########
@@ -583,16 +592,21 @@ def process(self, direction, rule_list, base, is_routed):
count = base
for i in rule_list:
- ruleData = copy.copy(i)
- cidr = ruleData['cidr']
+ cidr = i['cidr']
if cidr is not None and cidr != "":
cidr = removeUndesiredCidrs(cidr, 6)
if cidr is None or cidr == "":
continue
- ruleData['cidr'] = cidr
- r = self.AclRule(direction, self, ruleData, self.config, count)
- r.create()
- count += 1
+ # iptables expands "-s a,b" into one rule per address, but
CsNetfilters
+ # counts each entry as a single rule when placing the next ACL
rule ahead
+ # of the DROP. Emit one entry per CIDR so the two stay in step.
+ cidrs = [c for c in cidr.split(",") if c] if cidr else [cidr]
+ for c in cidrs:
Review Comment:
Yes, I measured it. I fed the rules `AclDevice` generates through the
unmodified `CsNetfilters.compare()`, against real iptables (1.8.10, nf_tables
backend) in a throwaway network namespace. The test ACL has one TCP rule with N
CIDRs, followed by a single-CIDR allow and a deny. Times are wall clock, with
the number of `iptables` processes run:
| N | before, first apply | before, re-apply | after, first apply | after,
re-apply |
|---|---|---|---|---|
| 100 | 0.02s / 8 | 1.5s / 106 | 0.3s / 107 | 2.0s / 205 |
| 300 | 0.03s / 8 | 5.1s / 306 | 1.0s / 307 | 6.6s / 605 |
| 1000 | rule not applied (`iptables: Message too long`) | - | 9.8s / 1007 |
43.9s / 2005 |
Generating the rules in Python takes about 1ms either way.
Most of this cost was already there before the PR. `has_rule()` never
matches a rule that has a position, so on every apply each ACL rule is inserted
again and its old copy is deleted as unseen. Those old copies were already one
rule per CIDR, because iptables had expanded `-s a,b,c`. So a re-apply was
already 1 insert + N deletes. With this PR it is N inserts + N deletes. The
deletes are the slow part: about 17ms each at N=300, against about 3ms per
insert, because each call reads the whole ruleset. At N=300 this PR adds
roughly 1 to 1.5s to an apply that already took 5s. That is also exactly what
the current workaround costs, since N single-CIDR rules produce the same
iptables calls.
The real fix is to batch the changes through `iptables-restore --noflush`.
That way, 300 inserts plus 300 deletes take about 0.02s. It would change how
every iptables rule on the VR is applied, though:
- Failure handling changes: one bad line fails the whole batch.
- It needs chunking: a 1000-rule batch also fails with `Message too long`.
So I'd rather keep it out of this bug fix and do it as a separate PR. I'm
happy to open one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
