This is a good direction to go with the deprecated nexthop column deprecation. I have a few comments below
On Tue, Oct 6, 2026 at 1:51 PM Mark Michelson via dev < [email protected]> wrote: > The "nexthop" column in the northbound database is deprecated in favor > of the "nexthops" column. However, the "nexthops" column only matters if > the policy is a "reroute" policy. Therefore, we can ignore the fact that > a deprecated column has its value set if the policy type does not rely > on nexthops at all. > I am not sure if this is pedantic or not but doesn't this change do more than just "stop emitting a warning"? Before this patch ANY policy with the deprecated column was skipped and produced no lflows. After this patch any allow/jump/drop policies that a CMS created with a stale nexthop will be enforced, that is the correct behavior but should there be a NEWS entry or a more strongly worded commit message? > > Signed-off-by: Mark Michelson <[email protected]> > --- > northd/en-route-policies.c | 17 +++++----- > tests/ovn-northd.at | 66 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 75 insertions(+), 8 deletions(-) > > diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c > index 9597980ec..1288a4a93 100644 > --- a/northd/en-route-policies.c > +++ b/northd/en-route-policies.c > @@ -176,14 +176,6 @@ build_route_policies(struct ovn_datapath *od, > for (int i = 0; i < od->nbr->n_policies; i++) { > const struct nbrec_logical_router_policy *rule = > od->nbr->policies[i]; > > - if (rule->nexthop && rule->nexthop[0]) { > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); > - VLOG_WARN_RL(&rl, "Logical router: %s, policy uses deprecated" > - " column \"nexthop\", this column is ignored. > Please" > - "use \"nexthops\" column instead.", > od->nbr->name); > - continue; > - } > - > size_t n_valid_nexthops = 0; > char **valid_nexthops = NULL; > uint32_t chain_id = 0; > @@ -224,6 +216,15 @@ build_route_policies(struct ovn_datapath *od, > } > > if (!strcmp(rule->action, "reroute")) { > + if (rule->nexthop && rule->nexthop[0]) { > + static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 1); > + VLOG_WARN_RL(&rl, "Logical router: %s, policy uses > deprecated" > + " column \"nexthop\", this column is > ignored. " > + "Please use \"nexthops\" column instead.", > + od->nbr->name); > I know this is a reusing what the WARN message was but it isn't entirely accurate, it isn't only the nexthop column that gets ignored. The whole policy is ignored, even if valid nexthops column is set. Could something be added to reflect that? That the policy is ignored due to the use of deprecated nexthop column. It might even be worth updating the ovn-nb/ovn-nbctl docs. They still reference that nexthop can be used. I know this is not the point of this patch set but since this patch is specifically about this the work could be done here. Could be done in a follow up if not here > + continue; > + } > + > if (rule->output_port && rule->n_nexthops != 1) { > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); > VLOG_WARN_RL(&rl, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index ef56a2882..192c74f1b 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -24796,3 +24796,69 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Router policy selective nexthop deprecation warning]) > +ovn_start > + > +check ovn-nbctl lr-add lr0 > +check ovn-nbctl lrp-add lr0 lr0-p1 00:00:00:00:00:01 172.16.1.1/24 > + > +# Add a policy of each type to this router > +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 10.0.0.0/24" allow > +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 20.0.0.0/24" drop > +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 30.0.0.0/24" reroute > 172.16.1.50 > +check ovn-nbctl lr-policy-add lr0 100 "ip4.src == 40.0.0.0/24" jump > target > +check ovn-nbctl --chain=target lr-policy-add lr0 200 "1" allow > +check ovn-nbctl --wait=sb sync > + > +# Let's double-check that all of the expected flows are present > +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=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 10.0.0.0/24)), action=(reg8[[0..15]] = 0; next;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 20.0.0.0/24)), action=(drop;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 30.0.0.0/24)), action=(reg0 = 172.16.1.50; reg5 = > 172.16.1.1; eth.src = 00:00:00:00:00:01; outport = "lr0-p1"; flags.loopback > = 1; reg8[[0..15]] = 0; reg9[[9]] = 1; next;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 40.0.0.0/24)), action=(reg9[[16..31]]=1; > next(pipeline=ingress, table=??);) > + table=??(lr_in_policy ), priority=200 , match=(reg9[[16..31]] == > 1 && (1)), action=(reg8[[0..15]] = 0; next;) > +]) > + > +allow_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 > action=allow) > +drop_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 > action=drop) > +jump_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 > action=jump) > +reroute_uuid=$(fetch_column nb:Logical_Router_Policy _uuid priority=100 > action=reroute) > + > +# For the allow, drop, and jump policies, we'll set the "nexthop" column. > The policies > +# should remain unchanged, and we shouldn't see a warning in the log. > +check ovn-nbctl set Logical_Router_Policy $allow_uuid nexthop=50.0.0.50 > +check ovn-nbctl set Logical_Router_Policy $drop_uuid nexthop=50.0.0.50 > +check ovn-nbctl set Logical_Router_Policy $jump_uuid nexthop=50.0.0.50 > +check ovn-nbctl --wait=sb sync > + > +AT_CHECK([grep -qE "policy uses deprecated" northd/ovn-northd.log], [1]) > + > +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=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 10.0.0.0/24)), action=(reg8[[0..15]] = 0; next;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 20.0.0.0/24)), action=(drop;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 30.0.0.0/24)), action=(reg0 = 172.16.1.50; reg5 = > 172.16.1.1; eth.src = 00:00:00:00:00:01; outport = "lr0-p1"; flags.loopback > = 1; reg8[[0..15]] = 0; reg9[[9]] = 1; next;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 40.0.0.0/24)), action=(reg9[[16..31]]=1; > next(pipeline=ingress, table=??);) > + table=??(lr_in_policy ), priority=200 , match=(reg9[[16..31]] == > 1 && (1)), action=(reg8[[0..15]] = 0; next;) > +]) > + > +# Now let's set the "nexthop" column on the reroute policy. This should > result in the > +# warning appearing and the reroute policy not showing up in the lflows. > +check ovn-nbctl set Logical_Router_Policy $reroute_uuid nexthop=50.0.0.50 > +check ovn-nbctl --wait=sb sync > + > +AT_CHECK([grep -qE "policy uses deprecated" northd/ovn-northd.log], [0]) > +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=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 10.0.0.0/24)), action=(reg8[[0..15]] = 0; next;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 20.0.0.0/24)), action=(drop;) > + table=??(lr_in_policy ), priority=100 , match=(reg9[[16..31]] == > 0 && (ip4.src == 40.0.0.0/24)), action=(reg9[[16..31]]=1; > next(pipeline=ingress, table=??);) > + table=??(lr_in_policy ), priority=200 , match=(reg9[[16..31]] == > 1 && (1)), action=(reg8[[0..15]] = 0; next;) > +]) > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > +]) > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev Thanks, Jacob _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
