unsubscribe
----邮件原文---- 发件人：ovs-dev-request 收件人：ovs-dev 抄 送: (无) 发送时间：2026-10-20 09:17:18 主题：dev Digest, Vol 207, Issue 278 Send dev mailing list submissions to [email protected] To subscribe or unsubscribe via the World Wide Web, visit https://mail.openvswitch.org/mailman/listinfo/ovs-dev or, via email, send a message with subject or body 'help' to [email protected] You can reach the person managing the list at [email protected] When replying, please edit your Subject line so it is more specific than "Re: Contents of dev digest..." Today's Topics: 1. [PATCH ovn v2 17/17] en-route-policies: Get rid of en-northd change handler. (Mark Michelson) 2. Re: [PATCH ovn] northd: Reset chassis features on recompute. (Rosemarie O'Riorden) ---------------------------------------------------------------------- Message: 1 Date: Mon, 19 Oct 2026 16:51:55 -0400 From: Mark Michelson To: [email protected] Subject: [ovs-dev] [PATCH ovn v2 17/17] en-route-policies: Get rid of en-northd change handler. Message-ID: <[email protected]> Content-Type: text/plain charset="US-ASCII" x-default=true Prior to this commit, any time that en-northd needed to recompute, it would also trigger a recompute of en-route-policies. The vast majority of reasons that en-northd could recompute is irrelevant to logical router policy processing. The only change that actually makes a difference is if the logical router ports change, and there are "reroute" policies installed on the logical router. In this commit, we now check for logical router port changes in our handler for synced logical routers. If we detect port changes, then we will rebuild the policies for the logical router incrementally. This allows us to remove the handler for en-northd entirely. Now, if en-northd recomputes, en-route-policies does not care and likely can incrementally handle whatever caused en-northd to recompute. Reported-at: https://redhat.atlassian.net/browse/FDP-3984 Signed-off-by: Mark Michelson --- v1 -> v2: * Enhanced the test case a bit more so that we are ensuring reroute policies are incrementally handled properly. --- northd/en-route-policies.c | 57 ++++++++++++++++++++------------ northd/en-route-policies.h | 3 -- northd/inc-proc-northd.c | 7 ++-- tests/ovn-inc-proc-graph-dump.at | 2 +- tests/ovn-northd.at | 46 ++++++++++++++++++++++++-- 5 files changed, 84 insertions(+), 31 deletions(-) diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c index aabf95b52..814a46bef 100644 --- a/northd/en-route-policies.c +++ b/northd/en-route-policies.c @@ -556,27 +556,6 @@ en_route_policies_cleanup(void *data) route_policies_destroy(data) } -enum engine_input_handler_result -route_policies_northd_change_handler(struct engine_node *node, - void *data OVS_UNUSED) -{ - struct northd_data *northd_data = engine_get_input_data("northd", node) - if (!northd_has_tracked_data(&northd_data->trk_data)) { - return EN_UNHANDLED - } - - /* This node uses the below data from the en_northd engine node. - * See (lr_stateful_get_input_data()) - * 1. northd_data->lr_datapaths - * This data gets updated when a logical router or logical router port - * is created or deleted. - * Northd engine node presently falls back to full recompute when - * this happens and so does this node. - */ - - return EN_HANDLED_UNCHANGED -} - static bool logical_router_policies_updated(const struct nbrec_logical_router *lr) { @@ -595,6 +574,39 @@ logical_router_policies_updated(const struct nbrec_logical_router *lr) return false } +static bool +logical_router_ports_updated(const struct nbrec_logical_router *lr) +{ + /* We only care if ports change if the lr has policies with a + * "reroute" action. Otherwise, we don't care about the ports. + */ + bool has_reroute = false + for (size_t i = 0 i < lr->n_policies i++) { + const struct nbrec_logical_router_policy *rule = lr->policies[i] + if (!strcmp(rule->action, "reroute")) { + has_reroute = true + break + } + } + if (!has_reroute) { + return false + } + + if (nbrec_logical_router_is_updated(lr, + NBREC_LOGICAL_ROUTER_COL_PORTS)) { + return true + } + for (size_t i = 0 i < lr->n_ports i++) { + const struct nbrec_logical_router_port *lrp = lr->ports[i] + if (nbrec_logical_router_port_row_get_seqno( + lrp, OVSDB_IDL_CHANGE_MODIFY) > 0) { + return true + } + } + + return false +} + enum engine_input_handler_result route_policies_datapath_synced_logical_router_handler(struct engine_node *node, void *data) @@ -673,7 +685,8 @@ route_policies_datapath_synced_logical_router_handler(struct engine_node *node, HMAPX_FOR_EACH (lr_node, &synced_lrs->updated) { const struct ovn_synced_logical_router *lr = lr_node->data - if (!logical_router_policies_updated(lr->nb)) { + if (!logical_router_policies_updated(lr->nb) && + !logical_router_ports_updated(lr->nb)) { continue } struct ovn_datapath *od = diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h index 0c93d5a0f..1990507f9 100644 --- a/northd/en-route-policies.h +++ b/northd/en-route-policies.h @@ -100,9 +100,6 @@ struct route_policies_data { void en_route_policies_cleanup(void *data) enum engine_input_handler_result -route_policies_northd_change_handler(struct engine_node *node, - void *data OVS_UNUSED) -enum engine_input_handler_result route_policies_datapath_synced_logical_router_handler(struct engine_node *node, void *data) enum engine_node_state en_route_policies_run(struct engine_node *node, diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 603590697..49348e42f 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -331,8 +331,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_route_policies, &en_datapath_synced_logical_router, route_policies_datapath_synced_logical_router_handler) - engine_add_input(&en_route_policies, &en_northd, - route_policies_northd_change_handler) + /* en_route_policies uses data from en_northd, but it makes the + * determination about whether to recompute based on synced logical + * routers. + */ + engine_add_input(&en_route_policies, &en_northd, engine_noop_handler) engine_add_input(&en_routes, &en_northd, routes_northd_change_handler) diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ovn-inc-proc-graph-dump.at index 8e32e5e6b..0d355ecb1 100644 --- a/tests/ovn-inc-proc-graph-dump.at +++ b/tests/ovn-inc-proc-graph-dump.at @@ -152,7 +152,7 @@ digraph "Incremental-Processing-Engine" { NB_logical_router_static_route -> routes [[label="routes_static_route_change_handler"]] route_policies [[style=filled, shape=box, fillcolor=white, label="route_policies"]] datapath_synced_logical_router -> route_policies [[label="route_policies_datapath_synced_logical_router_handler"]] - northd -> route_policies [[label="route_policies_northd_change_handler"]] + northd -> route_policies [[label="engine_noop_handler"]] bfd_sync [[style=filled, shape=box, fillcolor=white, label="bfd_sync"]] NB_bfd -> bfd_sync [[label=""]] SB_bfd -> bfd_sync [[label=""]] diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 03bea5217..b6eb3437e 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -16523,7 +16523,7 @@ check_engine_stats lflow recompute nocompute # For the below engine nodes, en_northd is input. So check # their engine status. check_engine_stats lr_stateful recompute nocompute -check_engine_stats route_policies recompute nocompute +check_engine_stats route_policies norecompute compute check_engine_stats routes recompute nocompute check_engine_stats bfd_sync recompute nocompute check_engine_stats sync_to_sb_lb recompute nocompute @@ -25112,15 +25112,55 @@ check_engine_stats bfd_sync norecompute compute check_engine_stats lflow norecompute compute CHECK_NO_CHANGE_AFTER_RECOMPUTE +# Add a port to lr1. This should trigger a recompute in northd, +# but route_policies should be able to handle the change incrementally. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lrp-add lr1 new_port 00:00:00:00:00:05 20.0.0.1/24 +check ovn-nbctl --wait=sb sync + +check_engine_stats northd recompute compute +check_engine_stats route_policies norecompute compute +# bfd_sync has to recompute because northd recomputed. +# Since bfd_sync recomputes, so does lflow. +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Add a reroute policy to lr1. Like other policy additions, this should +# be handled incrementally everywhere now. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lr-policy-add lr1 101 "ip4.src == 10.0.0.1" reroute 20.0.0.10 + +check_engine_stats northd norecompute compute +check_engine_stats route_policies norecompute compute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Add another port to lr1. Like before, we should be able to handle this +# incrementally, even though northd has to recompute. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lrp-add lr1 newer_port 00:00:00:00:00:06 30.0.0.1/24 + +check_engine_stats northd recompute compute +check_engine_stats route_policies norecompute compute +# bfd_sync has to recompute because northd recomputed. +# Since bfd_sync recomputes, so does lflow. +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + # Delete the logical router that has policies on it. # This should also not require a recompute in route_policies check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats check ovn-nbctl --wait=sb lr-del lr1 -check_engine_stats northd norecompute compute +# northd will recompute when this router is deleted since it +# had ports. +check_engine_stats northd recompute compute check_engine_stats route_policies norecompute compute check_engine_stats bfd_sync recompute nocompute -check_engine_stats lflow norecompute compute +check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE OVN_CLEANUP_NORTHD -- 2.55.0 ------------------------------ Message: 2 Date: Mon, 19 Oct 2026 17:02:35 -0400 From: Rosemarie O'Riorden To: Rukomoinikova Aleksandra , "[email protected]" Cc: [email protected] Subject: Re: [ovs-dev] [PATCH ovn] northd: Reset chassis features on recompute. Message-ID: Content-Type: text/plain charset=UTF-8 format=flowed Ack, thanks Sasha! On 10/12/26 6:10 PM, Rukomoinikova Aleksandra wrote: > On 02.10.2026 22:09, Rosemarie O'Riorden wrote: >> Hi Alexandra, thanks for the patch! > Hi Rosemarie, > > Thanks for the review and the ideas! >> >> This is a very simple fix so it looks good to me, and the only thing I'm >> wondering is if the test could catch more edge-cases so it's more >> sensitive to any changes that affect this area in the future. >> >> Something I think could make the test more robust is checking for all of >> the features instead of just ct_state_save, in case any individual >> feature is affected. > I'd prefer to keep this to a single feature. The bug isn't tied to any > particular feature, it's in the common mechanism: the full recompute path > didn't call northd_enable_all_features() before build_chassis_features(), > which affects all flags the same way. Comparing the full > debug/chassis-features-list output would mean updating the test every > time a new feature is added, which isn't related to what the test is > actually checking. >> >> Other things that could be tested for are: >> 1. What happens if you delete all the chassis and then add one back? > This goes through the same full recompute path as deleting a single > chassis, so it doesn't add much coverage. The only new case is an empty > Chassis table, where all features should be enabled. I can add that > check if you think it's useful. >> 2. What if ignore_chassis_features=true? (This isn't really tested >> in /tests/ so it might be good to add anyways) > Good idea, especially since the patch touches exactly that branch and > feature values aren't currently checked with this option set. I'll add > it in v2: with a chassis that lacks the feature, set the option and > check that the feature is enabled, then remove the option and check > that it's disabled again. >> 3. What if ct-state-save's value is changed during the middle of the >> test? As far as I understand, adding or deleting a chassis triggers a >> full recompute but just altering the other_config is handled >> incrementally, so since different paths are taken it could be useful >> to check to prevent regressions. > You're right that this is the incremental path. It already called > northd_enable_all_features(), so it wasn't affected by the bug, but it's > still a useful regression check, especially together with verifying > that the change was handled incrementally rather than by a recompute. > I'll add it. > > I'd also like to add an explicit "inc-engine/recompute". It tests the > fix directly: with the old code the feature stayed disabled after any > full recompute, not only after a chassis deletion. > > I'll send a v2 with these changes. > > Thanks, Sasha. >> >> >> I'm not sure how important any of these ideas are, and the test is >> probably fine as-is. These are just random thoughts, so if you or anyone >> else have input LMK :) >> >> >> On 10/1/26 4:16 AM, Alexandra Rukomoinikova via dev wrote: >>> build_chassis_features() can only clear feature flags, but >>> en_global_config_run() called it without first enabling all features. >>> A feature disabled by a chassis therefore stayed disabled after that >>> chassis was deleted, since deletion triggers a full recompute. >>> >>> Fixes: aed60eea33f2 ("northd: Add I-P for NB_Global and SB_Global.") >>> Signed-off-by: Alexandra Rukomoinikova >>> Assisted-by: Claude Opus 5.5 >>> --- >>> northd/en-global-config.c | 7 ++++--- >>> tests/ovn-northd.at | 32 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 36 insertions(+), 3 deletions(-) >>> >>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >>> index 6dd5e0977..ecaf998b2 100644 >>> --- a/northd/en-global-config.c >>> +++ b/northd/en-global-config.c >>> @@ -217,9 +217,10 @@ en_global_config_run(struct engine_node *node , void *data) >>> nbrec_nb_global_set_options(nb, options) >>> } >>> >>> - if (smap_get_bool(&nb->options, "ignore_chassis_features", false)) { >>> - northd_enable_all_features(config_data) >>> - } else { >>> + /* Enable all features before calling build_chassis_features() as >>> + * build_chassis_features() only sets the feature flags to false. */ >>> + northd_enable_all_features(config_data) >>> + if (!smap_get_bool(&nb->options, "ignore_chassis_features", false)) { >>> build_chassis_features(sbrec_chassis_table, &config_data->features) >>> } >>> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 6572b1318..431649425 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -11651,6 +11651,38 @@ OVN_CLEANUP_NORTHD >>> AT_CLEANUP >>> ]) >>> >>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>> +AT_SETUP([Chassis-feature compatibility - chassis deletion]) >>> +ovn_start >>> + >>> +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 \ >>> + -- set chassis hv1 other_config:ct-state-save=true >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([as northd ovn-appctl -t ovn-northd debug/chassis-features-list | grep "ct_state_save"], [0], [dnl >>> +ct_state_save: true >>> +]) >>> + >>> +AS_BOX([Add chassis without the feature]) >>> +check ovn-sbctl chassis-add hv2 geneve 127.0.0.2 >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([as northd ovn-appctl -t ovn-northd debug/chassis-features-list | grep "ct_state_save"], [0], [dnl >>> +ct_state_save: false >>> +]) >>> + >>> +AS_BOX([Delete chassis without the feature]) >>> +check ovn-sbctl chassis-del hv2 >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([as northd ovn-appctl -t ovn-northd debug/chassis-features-list | grep "ct_state_save"], [0], [dnl >>> +ct_state_save: true >>> +]) >>> + >>> +OVN_CLEANUP_NORTHD >>> +AT_CLEANUP >>> +]) >>> + >>> OVN_FOR_EACH_NORTHD_NO_HV([ >>> AT_SETUP([check OVN QoS]) >>> AT_KEYWORDS([OVN-QoS]) > ------------------------------ Subject: Digest Footer _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev ------------------------------ End of dev Digest, Vol 207, Issue 278 ************************************* Subject：dev Digest, Vol 207, Issue 278 Send dev mailing list submissions to [email protected] To subscribe or unsubscribe via the World Wide Web, visit https://mail.openvswitch.org/mailman/listinfo/ovs-dev or, via email, send a message with subject or body 'help' to [email protected] You can reach the person managing the list at [email protected] When replying, please edit your Subject line so it is more specific than "Re: Contents of dev digest..." Today's Topics: 1. [PATCH ovn v2 17/17] en-route-policies: Get rid of en-northd change handler. (Mark Michelson) 2. Re: [PATCH ovn] northd: Reset chassis features on recompute. (Rosemarie O'Riorden) ---------------------------------------------------------------------- Message: 1 Date: Mon, 19 Oct 2026 16:51:55 -0400 From: Mark Michelson To: [email protected] Subject: [ovs-dev] [PATCH ovn v2 17/17] en-route-policies: Get rid of en-northd change handler. Message-ID: <[email protected]> Content-Type: text/plain charset="US-ASCII" x-default=true Prior to this commit, any time that en-northd needed to recompute, it would also trigger a recompute of en-route-policies. The vast majority of reasons that en-northd could recompute is irrelevant to logical router policy processing. The only change that actually makes a difference is if the logical router ports change, and there are "reroute" policies installed on the logical router. In this commit, we now check for logical router port changes in our handler for synced logical routers. If we detect port changes, then we will rebuild the policies for the logical router incrementally. This allows us to remove the handler for en-northd entirely. Now, if en-northd recomputes, en-route-policies does not care and likely can incrementally handle whatever caused en-northd to recompute. Reported-at: https://redhat.atlassian.net/browse/FDP-3984 Signed-off-by: Mark Michelson --- v1 -> v2: * Enhanced the test case a bit more so that we are ensuring reroute policies are incrementally handled properly. --- northd/en-route-policies.c | 57 ++++++++++++++++++++------------ northd/en-route-policies.h | 3 -- northd/inc-proc-northd.c | 7 ++-- tests/ovn-inc-proc-graph-dump.at | 2 +- tests/ovn-northd.at | 46 ++++++++++++++++++++++++-- 5 files changed, 84 insertions(+), 31 deletions(-) diff --git a/northd/en-route-policies.c b/northd/en-route-policies.c index aabf95b52..814a46bef 100644 --- a/northd/en-route-policies.c +++ b/northd/en-route-policies.c @@ -556,27 +556,6 @@ en_route_policies_cleanup(void *data) route_policies_destroy(data) } -enum engine_input_handler_result -route_policies_northd_change_handler(struct engine_node *node, - void *data OVS_UNUSED) -{ - struct northd_data *northd_data = engine_get_input_data("northd", node) - if (!northd_has_tracked_data(&northd_data->trk_data)) { - return EN_UNHANDLED - } - - /* This node uses the below data from the en_northd engine node. - * See (lr_stateful_get_input_data()) - * 1. northd_data->lr_datapaths - * This data gets updated when a logical router or logical router port - * is created or deleted. - * Northd engine node presently falls back to full recompute when - * this happens and so does this node. - */ - - return EN_HANDLED_UNCHANGED -} - static bool logical_router_policies_updated(const struct nbrec_logical_router *lr) { @@ -595,6 +574,39 @@ logical_router_policies_updated(const struct nbrec_logical_router *lr) return false } +static bool +logical_router_ports_updated(const struct nbrec_logical_router *lr) +{ + /* We only care if ports change if the lr has policies with a + * "reroute" action. Otherwise, we don't care about the ports. + */ + bool has_reroute = false + for (size_t i = 0 i < lr->n_policies i++) { + const struct nbrec_logical_router_policy *rule = lr->policies[i] + if (!strcmp(rule->action, "reroute")) { + has_reroute = true + break + } + } + if (!has_reroute) { + return false + } + + if (nbrec_logical_router_is_updated(lr, + NBREC_LOGICAL_ROUTER_COL_PORTS)) { + return true + } + for (size_t i = 0 i < lr->n_ports i++) { + const struct nbrec_logical_router_port *lrp = lr->ports[i] + if (nbrec_logical_router_port_row_get_seqno( + lrp, OVSDB_IDL_CHANGE_MODIFY) > 0) { + return true + } + } + + return false +} + enum engine_input_handler_result route_policies_datapath_synced_logical_router_handler(struct engine_node *node, void *data) @@ -673,7 +685,8 @@ route_policies_datapath_synced_logical_router_handler(struct engine_node *node, HMAPX_FOR_EACH (lr_node, &synced_lrs->updated) { const struct ovn_synced_logical_router *lr = lr_node->data - if (!logical_router_policies_updated(lr->nb)) { + if (!logical_router_policies_updated(lr->nb) && + !logical_router_ports_updated(lr->nb)) { continue } struct ovn_datapath *od = diff --git a/northd/en-route-policies.h b/northd/en-route-policies.h index 0c93d5a0f..1990507f9 100644 --- a/northd/en-route-policies.h +++ b/northd/en-route-policies.h @@ -100,9 +100,6 @@ struct route_policies_data { void en_route_policies_cleanup(void *data) enum engine_input_handler_result -route_policies_northd_change_handler(struct engine_node *node, - void *data OVS_UNUSED) -enum engine_input_handler_result route_policies_datapath_synced_logical_router_handler(struct engine_node *node, void *data) enum engine_node_state en_route_policies_run(struct engine_node *node, diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index 603590697..49348e42f 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -331,8 +331,11 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_route_policies, &en_datapath_synced_logical_router, route_policies_datapath_synced_logical_router_handler) - engine_add_input(&en_route_policies, &en_northd, - route_policies_northd_change_handler) + /* en_route_policies uses data from en_northd, but it makes the + * determination about whether to recompute based on synced logical + * routers. + */ + engine_add_input(&en_route_policies, &en_northd, engine_noop_handler) engine_add_input(&en_routes, &en_northd, routes_northd_change_handler) diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ovn-inc-proc-graph-dump.at index 8e32e5e6b..0d355ecb1 100644 --- a/tests/ovn-inc-proc-graph-dump.at +++ b/tests/ovn-inc-proc-graph-dump.at @@ -152,7 +152,7 @@ digraph "Incremental-Processing-Engine" { NB_logical_router_static_route -> routes [[label="routes_static_route_change_handler"]] route_policies [[style=filled, shape=box, fillcolor=white, label="route_policies"]] datapath_synced_logical_router -> route_policies [[label="route_policies_datapath_synced_logical_router_handler"]] - northd -> route_policies [[label="route_policies_northd_change_handler"]] + northd -> route_policies [[label="engine_noop_handler"]] bfd_sync [[style=filled, shape=box, fillcolor=white, label="bfd_sync"]] NB_bfd -> bfd_sync [[label=""]] SB_bfd -> bfd_sync [[label=""]] diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 03bea5217..b6eb3437e 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -16523,7 +16523,7 @@ check_engine_stats lflow recompute nocompute # For the below engine nodes, en_northd is input. So check # their engine status. check_engine_stats lr_stateful recompute nocompute -check_engine_stats route_policies recompute nocompute +check_engine_stats route_policies norecompute compute check_engine_stats routes recompute nocompute check_engine_stats bfd_sync recompute nocompute check_engine_stats sync_to_sb_lb recompute nocompute @@ -25112,15 +25112,55 @@ check_engine_stats bfd_sync norecompute compute check_engine_stats lflow norecompute compute CHECK_NO_CHANGE_AFTER_RECOMPUTE +# Add a port to lr1. This should trigger a recompute in northd, +# but route_policies should be able to handle the change incrementally. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lrp-add lr1 new_port 00:00:00:00:00:05 20.0.0.1/24 +check ovn-nbctl --wait=sb sync + +check_engine_stats northd recompute compute +check_engine_stats route_policies norecompute compute +# bfd_sync has to recompute because northd recomputed. +# Since bfd_sync recomputes, so does lflow. +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Add a reroute policy to lr1. Like other policy additions, this should +# be handled incrementally everywhere now. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lr-policy-add lr1 101 "ip4.src == 10.0.0.1" reroute 20.0.0.10 + +check_engine_stats northd norecompute compute +check_engine_stats route_policies norecompute compute +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow norecompute compute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Add another port to lr1. Like before, we should be able to handle this +# incrementally, even though northd has to recompute. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl lrp-add lr1 newer_port 00:00:00:00:00:06 30.0.0.1/24 + +check_engine_stats northd recompute compute +check_engine_stats route_policies norecompute compute +# bfd_sync has to recompute because northd recomputed. +# Since bfd_sync recomputes, so does lflow. +check_engine_stats bfd_sync recompute nocompute +check_engine_stats lflow recompute nocompute +CHECK_NO_CHANGE_AFTER_RECOMPUTE + # Delete the logical router that has policies on it. # This should also not require a recompute in route_policies check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats check ovn-nbctl --wait=sb lr-del lr1 -check_engine_stats northd norecompute compute +# northd will recompute when this router is deleted since it +# had ports. +check_engine_stats northd recompute compute check_engine_stats route_policies norecompute compute check_engine_stats bfd_sync recompute nocompute -check_engine_stats lflow norecompute compute +check_engine_stats lflow recompute nocompute CHECK_NO_CHANGE_AFTER_RECOMPUTE OVN_CLEANUP_NORTHD -- 2.55.0 ------------------------------ Message: 2 Date: Mon, 19 Oct 2026 17:02:35 -0400 From: Rosemarie O'Riorden To: Rukomoinikova Aleksandra , "[email protected]" Cc: [email protected] Subject: Re: [ovs-dev] [PATCH ovn] northd: Reset chassis features on recompute. Message-ID: Content-Type: text/plain charset=UTF-8 format=flowed Ack, thanks Sasha! On 10/12/26 6:10 PM, Rukomoinikova Aleksandra wrote: > On 02.10.2026 22:09, Rosemarie O'Riorden wrote: >> Hi Alexandra, thanks for the patch! > Hi Rosemarie, > > Thanks for the review and the ideas! >> >> This is a very simple fix so it looks good to me, and the only thing I'm >> wondering is if the test could catch more edge-cases so it's more >> sensitive to any changes that affect this area in the future. >> >> Something I think could make the test more robust is checking for all of >> the features instead of just ct_state_save, in case any individual >> feature is affected. > I'd prefer to keep this to a single feature. The bug isn't tied to any > particular feature, it's in the common mechanism: the full recompute path > didn't call northd_enable_all_features() before build_chassis_features(), > which affects all flags the same way. Comparing the full > debug/chassis-features-list output would mean updating the test every > time a new feature is added, which isn't related to what the test is > actually checking. >> >> Other things that could be tested for are: >> 1. What happens if you delete all the chassis and then add one back? > This goes through the same full recompute path as deleting a single > chassis, so it doesn't add much coverage. The only new case is an empty > Chassis table, where all features should be enabled. I can add that > check if you think it's useful. >> 2. What if ignore_chassis_features=true? (This isn't really tested >> in /tests/ so it might be good to add anyways) > Good idea, especially since the patch touches exactly that branch and > feature values aren't currently checked with this option set. I'll add > it in v2: with a chassis that lacks the feature, set the option and > check that the feature is enabled, then remove the option and check > that it's disabled again. >> 3. What if ct-state-save's value is changed during the middle of the >> test? As far as I understand, adding or deleting a chassis triggers a >> full recompute but just altering the other_config is handled >> incrementally, so since different paths are taken it could be useful >> to check to prevent regressions. > You're right that this is the incremental path. It already called > northd_enable_all_features(), so it wasn't affected by the bug, but it's > still a useful regression check, especially together with verifying > that the change was handled incrementally rather than by a recompute. > I'll add it. > > I'd also like to add an explicit "inc-engine/recompute". It tests the > fix directly: with the old code the feature stayed disabled after any > full recompute, not only after a chassis deletion. > > I'll send a v2 with these changes. > > Thanks, Sasha. >> >> >> I'm not sure how important any of these ideas are, and the test is >> probably fine as-is. These are just random thoughts, so if you or anyone >> else have input LMK :) >> >> >> On 10/1/26 4:16 AM, Alexandra Rukomoinikova via dev wrote: >>> build_chassis_features() can only clear feature flags, but >>> en_global_config_run() called it without first enabling all features. >>> A feature disabled by a chassis therefore stayed disabled after that >>> chassis was deleted, since deletion triggers a full recompute. >>> >>> Fixes: aed60eea33f2 ("northd: Add I-P for NB_Global and SB_Global.") >>> Signed-off-by: Alexandra Rukomoinikova >>> Assisted-by: Claude Opus 5.5 >>> --- >>> northd/en-global-config.c | 7 ++++--- >>> tests/ovn-northd.at | 32 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 36 insertions(+), 3 deletions(-) >>> >>> diff --git a/northd/en-global-config.c b/northd/en-global-config.c >>> index 6dd5e0977..ecaf998b2 100644 >>> --- a/northd/en-global-config.c >>> +++ b/northd/en-global-config.c >>> @@ -217,9 +217,10 @@ en_global_config_run(struct engine_node *node , void *data) >>> nbrec_nb_global_set_options(nb, options) >>> } >>> >>> - if (smap_get_bool(&nb->options, "ignore_chassis_features", false)) { >>> - northd_enable_all_features(config_data) >>> - } else { >>> + /* Enable all features before calling build_chassis_features() as >>> + * build_chassis_features() only sets the feature flags to false. */ >>> + northd_enable_all_features(config_data) >>> + if (!smap_get_bool(&nb->options, "ignore_chassis_features", false)) { >>> build_chassis_features(sbrec_chassis_table, &config_data->features) >>> } >>> >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 6572b1318..431649425 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -11651,6 +11651,38 @@ OVN_CLEANUP_NORTHD >>> AT_CLEANUP >>> ]) >>> >>> +OVN_FOR_EACH_NORTHD_NO_HV([ >>> +AT_SETUP([Chassis-feature compatibility - chassis deletion]) >>> +ovn_start >>> + >>> +check ovn-sbctl chassis-add hv1 geneve 127.0.0.1 \ >>> + -- set chassis hv1 other_config:ct-state-save=true >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([as northd ovn-appctl -t ovn-northd debug/chassis-features-list | grep "ct_state_save"], [0], [dnl >>> +ct_state_save: true >>> +]) >>> + >>> +AS_BOX([Add chassis without the feature]) >>> +check ovn-sbctl chassis-add hv2 geneve 127.0.0.2 >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([as northd ovn-appctl -t ovn-northd debug/chassis-features-list | grep "ct_state_save"], [0], [dnl >>> +ct_state_save: false >>> +]) >>> + >>> +AS_BOX([Delete chassis without the feature]) >>> +check ovn-sbctl chassis-del hv2 >>> +check ovn-nbctl --wait=sb sync >>> + >>> +AT_CHECK([as northd ovn-appctl -t ovn-northd debug/chassis-features-list | grep "ct_state_save"], [0], [dnl >>> +ct_state_save: true >>> +]) >>> + >>> +OVN_CLEANUP_NORTHD >>> +AT_CLEANUP >>> +]) >>> + >>> OVN_FOR_EACH_NORTHD_NO_HV([ >>> AT_SETUP([check OVN QoS]) >>> AT_KEYWORDS([OVN-QoS]) > ------------------------------ Subject: Digest Footer _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev ------------------------------ End of dev Digest, Vol 207, Issue 278 ************************************* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
