On Tue, Oct 6, 2026 at 1:50 PM Mark Michelson via dev < [email protected]> wrote:
> In the current code, the map of chain IDs is kept in a global simap. > This means that when a particular router policy is encountered, it will > always be given the same numerical ID given its string chain ID. > > This commit changes the code so that the numerical IDs are only > consistent for a logical router policy within a particular logical > router. This does not require any changes to logical flows, since the > logical flows already discriminate based on datapath tunnel key. > > First, this prevents users from jumping from a policy attached to > logical router A to a policy attached to logical router B. You may only > jump to policies that are attached to the same logical router. > > Second, this allows for more chain IDs to be used. Previously, the global > simap would only allow for 65535 total chain IDs to be used. Now, 65535 > chain IDs can be used per logical router. This is not likely to be > useful, but it's worth noting. > > Finally, this change makes logical flow generation more consistent in > the case where chain IDs are used only on particular datapaths. The code > would set a policy's chain_id to -1 if the global simap of chain_ids is > empty. However, if chain_ids are not empty, and a policy does not > specify a chain_id, then the chain_id is set to 0. Let's picture a > scenario where logical router A does not use chain_ids or jump_chains, > and logical router B uses chain_ids and jump_chains. If the route policy > code evaluates A before B, then all of A's policies will have chain_id > set to -1. This results in the matches for logical router policies to > look like this: > > match = (<policy match>) > > If the route policy code evaluates B before A, then all of > A's policies will have chain_ids set to 0. This results in the matches > for logical router policies to look like this: > > match = (REG_POLICY_CHAIN_ID == 0 && (<policy_match>)) > > The resulting behavior ends up being the same for router A's policies, > since REG_POLICY_CHAIN_ID is never explicitly set and defaults to 0. > With this change, now router A's policies will be consistently set to > -1, no matter whether they are evaluated before or after B's policies. > Should this get a NEWS entry? it doesn't change how the feature works but if a user upgrades and looks at the flows they are now different. > > Signed-off-by: Mark Michelson <[email protected]> > --- > northd/en-route-policies.c | 7 ++-- > northd/en-route-policies.h | 2 -- > tests/ovn-northd.at | 71 ++++++++++++++++++++++++++++++++++++++ > 3 files changed, 75 insertions(+), 5 deletions(-) > > diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c > index dd6d182e8..9597980ec 100644 > --- a/northd/en-route-policies.c > +++ b/northd/en-route-policies.c nit: northd/en-route-policies.c should add #include "lib/simap.h", this commit removes the include from the header but the source still uses simap functions. This compiles because northd/en-route-policies.c includes northd.h which includes lib/simap.h but it should be explicitly included here in case that changes. > @@ -288,7 +288,6 @@ route_policies_init(struct route_policies_data *data) > { > hmap_init(&data->route_policies); > hmap_init(&data->bfd_active_connections); > - simap_init(&data->chain_ids); > } > > static void > @@ -301,7 +300,6 @@ route_policies_destroy(struct route_policies_data > *data) > }; > hmap_destroy(&data->route_policies); > bfd_destroy(&data->bfd_active_connections); > - simap_destroy(&data->chain_ids); > } > > enum engine_node_state > @@ -316,10 +314,13 @@ en_route_policies_run(struct engine_node *node, void > *data) > > struct ovn_datapath *od; > HMAP_FOR_EACH (od, key_node, &northd_data->lr_datapaths.datapaths) { > + struct simap chain_ids = SIMAP_INITIALIZER(&chain_ids); > + > build_route_policies(od, &bfd_data->bfd_connections, > &route_policies_data->route_policies, > &route_policies_data->bfd_active_connections, > - &route_policies_data->chain_ids); > + &chain_ids); > + simap_destroy(&chain_ids); > } > > return EN_UPDATED; > diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h > index 1d2288e46..1b13eb4cf 100644 > --- a/northd/en-route-policies.h > +++ b/northd/en-route-policies.h > @@ -19,7 +19,6 @@ > > #include "inc-proc-eng.h" > > -#include "lib/simap.h" > #include "openvswitch/hmap.h" > > /* Represents the data associated with an instance of a northbound > @@ -38,7 +37,6 @@ struct route_policy { > struct route_policies_data { > struct hmap route_policies; > struct hmap bfd_active_connections; > - struct simap chain_ids; > }; > > void en_route_policies_cleanup(void *data); > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 0a488ed64..ef56a2882 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -24725,3 +24725,74 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Router policy invalid chain IDs]) > +ovn_start > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lr-add lr1 > + > +# Set up a policy on lr0 to jump to a policy on chain lr1 > +check ovn-nbctl lr-policy-add lr0 102 "ip4.src == 10.0.0.0/24" jump > inbound \ > + -- --chain=inbound lr-policy-add lr1 201 "1" allow > +check ovn-nbctl --wait=sb sync > + > +# The policy on lr0 should not be installed since it jumps to > +# a chain on a different router. > +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | > ovn_strip_lflows | sort], [0], [dnl > + table=??(lr_in_policy ), priority=0 , match=(1), > action=(reg8[[0..15]] = 0; next;) > +]) > + > +# Double-check that we see the expected warning in the logs. > +check grep -qE "Logical router: lr0, policy action 'jump' follows to > non-existent chain inbound" northd/ovn-northd.log > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Router policy chain IDs consistent lflows]) > +ovn_start > + > +# For this test, we create two logical routers, one with policies with > jump chains, and one with > +# simple allow policies. We then will do the same but swap the policies > so that they are on the > +# opposite routers. We want to ensure that in both scenarios, the > generated flows are consistent > +# for the router without the jump chains. > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lr-add lr1 > + > +# First, let's put the jump policy on lr0 > +check ovn-nbctl lr-policy-add lr0 102 "ip4.src == 10.0.0.0/24" jump > inbound \ > + -- --chain=inbound lr-policy-add lr0 201 "1" allow > + > +# lr1 just gets an allow policy. > +check ovn-nbctl lr-policy-add lr1 102 "ip4.src == 10.0.0.0/24" allow > +check ovn-nbctl --wait=sb sync > + > +# The flow on lr1 should not check the chain ID register as part of its > match. > +AT_CHECK([ovn-sbctl lflow-list lr1 | grep "lr_in_policy[[^_]]" | > ovn_strip_lflows | sort], [0], [dnl > + table=??(lr_in_policy ), priority=0 , match=(1), > action=(reg8[[0..15]] = 0; next;) > + table=??(lr_in_policy ), priority=102 , match=(ip4.src == > 10.0.0.0/24), action=(reg8[[0..15]] = 0; next;) > +]) > + > +# Delete the policies from both routers > +check ovn-nbctl lr-policy-del lr0 > +check ovn-nbctl lr-policy-del lr1 > + > +# Install the same policies, but on the opposite routers. > +check ovn-nbctl lr-policy-add lr1 102 "ip4.src == 10.0.0.0/24" jump > inbound \ > + -- --chain=inbound lr-policy-add lr1 201 "1" allow > +check ovn-nbctl lr-policy-add lr0 102 "ip4.src == 10.0.0.0/24" allow > +check ovn-nbctl --wait=sb sync > + > +# The flow on lr0 should not check the chain ID register as part of its > match. > +AT_CHECK([ovn-sbctl lflow-list lr0 | grep "lr_in_policy[[^_]]" | > ovn_strip_lflows | sort], [0], [dnl > + table=??(lr_in_policy ), priority=0 , match=(1), > action=(reg8[[0..15]] = 0; next;) > + table=??(lr_in_policy ), priority=102 , match=(ip4.src == > 10.0.0.0/24), action=(reg8[[0..15]] = 0; next;) > +]) + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > neither of these two tests asserts that the numbering of chains is independent accross routers could you add a quick test that directly asserts routers independantly number there own chains? > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks, Jacob Tanenbaum _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
