[FIX BRANCH — DO NOT MERGE] ACL warm-restart staging over PR #4799 by courtland · Pull Request #27 · courtland/faucet

courtland · 2026-05-09T16:45:49Z

Warm-reload for changes to a port or VLAN's acls_in list. Adopts the
per-ACL design @gizmoguy proposed in his
2026-05-01 review of #4733;
supersedes faucetsdn#4733 with @hieunt79 co-authored.

For each port/VLAN whose only change is acls_in, iterate the old
and new ACL lists and call del/add per-ACL with a cold-start
priority decrement. remove_overlap_ofmsgs cancels unchanged
del+add pairs at flow-emit.

add_port_acl / del_port_acl: new priority kwarg, defaults to
auth_priority (dot1x callers unchanged).
build_acl_port_of_msgs: derives allow/force_port_vlan instructions
from the pipeline so per-ACL installs respect force_port_vlan: 1.
add_vlan_acl / del_vlan_acl: new methods symmetric to the port
helpers, missing per @gizmoguy's review.
del_port: priority-less flowdel so remove_overlap_ofmsgs doesn't
cancel it against add_port's wildcard (the @hieunt79 fix from VLAN ACL change detection to be able to warm reload faucetsdn/faucet#4733).

@hieunt79's two integration tests are included verbatim;
FaucetConfigReloadPortAclRemoveAllTest and a unit-level
ValveRemoveAllPortACLsTestCase cover the wildcard-cancellation case.
Adding or removing the first/last ACL on the DP still
cold-starts because the port_acl table itself is conditionally
allocated.

If merged, faucetsdn#4733 can close.

courtland · 2026-05-19T15:27:41Z

+    def add_vlan_acl(self, acl, vlan, priority=None):
+        """Create ACL openflow rules for a single VLAN ACL."""
+        if not acl.rules:
+            return []
+        if priority is None:
+            priority = self.acl_priority
+        return build_acl_ofmsgs(
+            [acl],
+            self.vlan_acl_table,
+            self.pipeline.accept_to_classification(),
+            self.pipeline.accept_to_l2_forwarding(),
+            priority,
+            acl.meter,
+            acl.exact_match,
+            vlan_vid=vlan.vid,
+        )
+
+    def del_vlan_acl(self, acl, vlan, priority=None):
+        """Delete ACL rules for a single VLAN ACL."""
+        flowmods = self.add_vlan_acl(acl, vlan, priority=priority)
+        return [
+            self.vlan_acl_table.flowdel(match=fm.match, priority=fm.priority)
+            for fm in flowmods
+        ]
+


Ensure these extra methods are necessary and don't exist as existing helpoers

courtland · 2026-05-19T15:31:37Z

+        if self.acl_manager:
            for port_num in changed_acl_ports:
-                port = self.dp.ports[port_num]
-                ofmsgs.extend(self.acl_manager.cold_start_port(port))
+                old = old_port_acls[port_num]
+                new = list(self.dp.ports[port_num].acls_in or [])
+                if not old or not new:
+                    ofmsgs.extend(
+                        self.acl_manager.cold_start_port(self.dp.ports[port_num])
+                    )
+                    continue
+                priority = self.acl_manager.acl_priority
+                for acl in old:
+                    ofmsgs.extend(
+                        self.acl_manager.del_port_acl(acl, port_num, priority=priority)
+                    )
+                    priority -= len(acl.rules)
+                priority = self.acl_manager.acl_priority
+                for acl in new:
+                    ofmsgs.extend(
+                        self.acl_manager.add_port_acl(acl, port_num, priority=priority)
+                    )
+                    priority -= len(acl.rules)
+            for vid in changed_acl_vlans:
+                vlan = self.dp.vlans[vid]
+                old = old_vlan_acls[vid]
+                new = list(vlan.acls_in or [])
+                priority = self.acl_manager.acl_priority
+                for acl in old:
+                    ofmsgs.extend(
+                        self.acl_manager.del_vlan_acl(acl, vlan, priority=priority)
+                    )
+                    priority -= len(acl.rules)
+                priority = self.acl_manager.acl_priority
+                for acl in new:
+                    ofmsgs.extend(
+                        self.acl_manager.add_vlan_acl(acl, vlan, priority=priority)
+                    )
+                    priority -= len(acl.rules)


Explain this change and consider adding more helpful comments

courtland · 2026-05-19T15:32:11Z

-def build_acl_port_of_msgs(acl, vid, port_num, acl_table, goto_table, priority):
+def build_acl_port_of_msgs(acl, vid, port_num, acl_table, pipeline, priority):
    """A Helper function for building Openflow Mod Messages for Port ACLs"""
    ofmsgs = None
    if acl.rules:
        ofmsgs = build_acl_ofmsgs(
            [acl],
            acl_table,
-            [valve_of.goto_table(goto_table)],
-            [valve_of.goto_table(goto_table)],
+            pipeline.accept_to_vlan(),
+            pipeline.accept_to_l2_forwarding(),


was changing this signature necessary?

courtland · 2026-05-19T15:33:08Z

            None,
            port_num,
            self.port_acl_table,
-            pipeline_vlan_table,


why was this replaced with self.pipeline instead of self.pipeline.vlan_table

@gizmoguy

Adopts the per-ACL design @gizmoguy proposed in his 2026-05-01 review of faucetsdn#4733: for each port/VLAN whose only change is acls_in, iterate the old and new ACL lists and call del/add per-ACL with a cold-start priority decrement. remove_overlap_ofmsgs in valve_flowreorder cancels unchanged del+add pairs at flow-emit, so unchanged ACL rules don't churn on the wire. Inserting an ACL mid-list shifts subsequent priorities -- those re-emit (acceptable per gizmoguy's fallback in the linked review). dp.py - _get_vlan_config_changes returns a new changed_acl_vlans set; ACL-only VLAN changes are no longer folded into changed_vlans so they warm-reload instead of cold valve.py _apply_config_changes - Snapshot the old acls_in lists before dp_init swaps self.dp - Per-ACL del+add loop replaces the prior cold_start_port handling for changed_acl_ports, and adds the equivalent for changed_acl_vlans - Empty<->non-empty port transitions need the wildcard rule flipped, so fall back to cold_start_port valve_acl.py - add_port_acl / del_port_acl get a priority kwarg defaulting to self.auth_priority so dot1x callers are unchanged - build_acl_port_of_msgs derives allow/force_port_vlan instructions from the pipeline (matching cold-start build_acl_ofmsgs), so rules with force_port_vlan: 1 land in the right table; dot1x ACLs never carry force_port_vlan: 1 so its behavior is preserved - New add_vlan_acl / del_vlan_acl symmetric to the port helpers (the methods gizmoguy noted were missing for per-ACL VLAN reload) - del_port's flowdel is now priority-less so its flowmodkey differs from the acl_priority wildcard that add_port emits for a port with no acls_in; without this, remove_overlap_ofmsgs cancels the delete and old ACL flows stay on the switch (the @hieunt79 fix from faucetsdn#4733) Tests - FaucetConfigReloadVlanAclChangeTest and FaucetConfigReloadPortAclRemoveTest come from @hieunt79's faucetsdn#4733 (the latter exercises the bug gizmoguy reproduced reviewing faucetsdn#4733: removing one ACL from a port that has multiple ACLs must leave the others installed) - FaucetConfigReloadPortAclRemoveAllTest covers removing the last ACL from a port; without the del_port fix above this hits the remove_overlap_ofmsgs cancellation and the old flow stays stale. Both ports share the same ACL so removing it from one port doesn't change the overall pipeline match set (which would otherwise force cold-start for table reconfiguration) - ValveRemoveAllPortACLsTestCase covers the same scenario at unit level - test_vlan_acl_update now expects warm reload (was cold) - ValveChangeVLANACLTestCase.test_change_vlan_acl now expects warm Co-Authored-By: hieunt79 <nguyenhieu264996@gmail.com>

courtland force-pushed the warm-restart-vlan-vip-fix branch 3 times, most recently from d1de3fe to d979072 Compare May 9, 2026 17:39

courtland changed the base branch from warm-restart-vlan-vip to main May 9, 2026 17:39

courtland changed the base branch from main to warm-restart-vlan-vip May 9, 2026 17:41

courtland changed the base branch from warm-restart-vlan-vip to main May 9, 2026 17:41

courtland force-pushed the warm-restart-vlan-vip-fix branch 9 times, most recently from 3cf420c to 2c70afc Compare May 12, 2026 17:59

courtland force-pushed the warm-restart-vlan-vip-fix branch 10 times, most recently from e0c21c3 to a67541d Compare May 19, 2026 15:05

courtland commented May 19, 2026

View reviewed changes

courtland force-pushed the warm-restart-vlan-vip-fix branch from a67541d to 9cd0f2c Compare May 19, 2026 15:47

courtland force-pushed the warm-restart-vlan-vip-fix branch from 9cd0f2c to b202eee Compare May 19, 2026 17:21

courtland closed this Jun 22, 2026

Navigation Menu

[FIX BRANCH — DO NOT MERGE] ACL warm-restart staging over PR #4799 - #27

[FIX BRANCH — DO NOT MERGE] ACL warm-restart staging over PR #4799#27
courtland wants to merge 1 commit into
mainfrom
warm-restart-vlan-vip-fix

courtland commented May 9, 2026 •

edited

Loading

Uh oh!

courtland May 19, 2026

Uh oh!

courtland May 19, 2026

Uh oh!

courtland May 19, 2026

Uh oh!

courtland May 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

courtland commented May 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

courtland May 19, 2026

Choose a reason for hiding this comment

Uh oh!

courtland May 19, 2026

Choose a reason for hiding this comment

Uh oh!

courtland May 19, 2026

Choose a reason for hiding this comment

Uh oh!

courtland May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

courtland commented May 9, 2026 •

edited

Loading