Hi Jacob, thanks for the review. I have replies below. On Tue, Oct 13, 2026 at 8:37 PM Jacob Tanenbaum <[email protected]> wrote: > > > 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?
I've added a NEWS entry that explains this. > >> >> >> 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. I've updated the language so that instead of saying "this column is ignored" it now says "this policy is skipped". > > 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 I couldn't find anywhere in the ovn-nbctl docs mentioning the nexthop column. In ovn-nb, I found one mention, and so I have updated that instance. > > >> >> + 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
