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 <[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])
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
