Hi Alexandra, thanks for the patch!
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.
Other things that could be tested for are:
1. What happens if you delete all the chassis and then add one back?
2. What if ignore_chassis_features=true? (This isn't really tested
in /tests/ so it might be good to add anyways)
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.
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 <[email protected]>
> 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])
--
Rosemarie O'Riorden
Boston & Lowell, MA, USA
[email protected]
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
