Hi Jacob, thanks for the review. I have replies below On Tue, Oct 13, 2026 at 1:39 PM Jacob Tanenbaum <[email protected]> wrote: > > > > 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. >
No, I don't think this is NEWS-worthy. This doesn't introduce any behavior changes, even though flows may be structured differently. >> >> >> 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. There's a patch later in the series that re-adds an simap into northd/en-route-policies.h and also re-adds the #include of the simap.h header file. To make things easier, I have just left the #include in place now. > >> >> @@ -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? I know that internally, the chains start numbering at 1 and move up from there, so I could theoretically create two routers with identical policies and test that the flows for each router use 1 as the jump chain value for their respective policies. Done! In practice, I don't like a test like this because it's working based on internal knowledge, plus it's testing assumptions that aren't guaranteed to be true. Yes, this commit changes the logic so that numbering of chain IDs is independent for each router, but there are no other guarantees besides that. There is no guarantee that chain IDs will always start at 1 for each router. There is no guarantee that having two routers with identical policies will definitely result in the same chain IDs being generated for each router's policies. Because of that, it makes the tests vulnerable to breaking if the internal algorithm for chain ID generation changes. In tests that operate based on internally-assigned values like datapath or port tunnel keys, we sometimes will query the southbound row to get the generated tunnel key, then test flows (or something else) to ensure that the key was used where expected. However, there are no tests that try to verify that the generated tunnel keys themselves follow a specific algorithm. In this particular case, the chain ID is not placed into any sort of database or publicly accessible area. I could add an ovn-appctl debug command to get internal router policy information, then use that to test the veracity of the generated logical flows. That way I could at least get around the assumption that chain IDs start generating at 1 and increment from there. However, that still does not get around the other assumption, which is that identical policies on different logical routers will always generate the same chain ID values. > >> >> -- >> 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
