On Tue, Sep 9, 2025 at 10:31 AM Dumitru Ceara <[email protected]> wrote:
> On 9/9/25 9:09 AM, Ales Musil wrote:
> > The commit mentioned below removed bunch of features, however in
> > order for ovn-controller to work with older northd we need to
> > report them. Those features can be removed once we release the
> > next LTS. As the upgrade beyond the next LTS is not supported.
> >
> > Fixes: 92ba14e0d34e ("features: Consolidate chassis features usage,
> reporting and testing.")
> > Signed-off-by: Ales Musil <[email protected]>
> > ---
>
> Hi Ales,
>
Hi Dumitru,
>
> Ah, sorry, I should've been more thorough when checking the 24.03 code,
> I missed that these were still used there. Luckily CI caught it:
>
>
> https://github.com/ovn-org/ovn/actions/runs/17567481613/job/49911260927#step:19:43
Yes, we are both guilty. However as discussed offline I will post a patch
that should prevent this in the future. It will highlight which
feature/action
can be deprecated/removed in future releases.
>
>
> > controller/chassis.c | 6 ++++++
> > include/ovn/features.h | 6 ++++++
> > 2 files changed, 12 insertions(+)
> >
> > diff --git a/controller/chassis.c b/controller/chassis.c
> > index 118e60748..60c01eb24 100644
> > --- a/controller/chassis.c
> > +++ b/controller/chassis.c
> > @@ -408,7 +408,10 @@ chassis_build_other_config(const struct
> ovs_chassis_cfg *ovs_cfg,
> > smap_replace(config, "ovn-evpn-local-ip", ovs_cfg->evpn_local_ip);
> > smap_replace(config, "is-interconn",
> > ovs_cfg->is_interconn ? "true" : "false");
> > + smap_replace(config, OVN_FEATURE_PORT_UP_NOTIF, "true");
> > + smap_replace(config, OVN_FEATURE_CT_NO_MASKED_LABEL, "true");
> > smap_replace(config, OVN_FEATURE_MAC_BINDING_TIMESTAMP, "true");
> > + smap_replace(config, OVN_FEATURE_CT_LB_RELATED, "true");
> > smap_replace(config, OVN_FEATURE_FDB_TIMESTAMP, "true");
> > smap_replace(config, OVN_FEATURE_LS_DPG_COLUMN, "true");
> > smap_replace(config, OVN_FEATURE_CT_COMMIT_NAT_V2, "true");
> > @@ -742,7 +745,10 @@ update_supported_sset(struct sset *supported)
> > /* Internal options. */
> > sset_add(supported, "is-vtep");
> > sset_add(supported, "is-remote");
> > + sset_add(supported, OVN_FEATURE_PORT_UP_NOTIF);
> > + sset_add(supported, OVN_FEATURE_CT_NO_MASKED_LABEL);
> > sset_add(supported, OVN_FEATURE_MAC_BINDING_TIMESTAMP);
> > + sset_add(supported, OVN_FEATURE_CT_LB_RELATED);
> > sset_add(supported, OVN_FEATURE_FDB_TIMESTAMP);
> > sset_add(supported, OVN_FEATURE_LS_DPG_COLUMN);
> > sset_add(supported, OVN_FEATURE_CT_COMMIT_NAT_V2);
>
> I think unfortunately this is not enough though. We should also add
> back the part that reconciles Sb.Chassis.other_config for these
> features. Otherwise, if the SB record is changed externally
> ovn-controller won't re-enable the feature, causing old northds
> (24.03) to fall back the old way of doing things even if
> ovn-controller has been upgraded already.
>
> That's what we do for all feature flags (non-deprecated too).
>
> Something like:
> diff --git a/controller/chassis.c b/controller/chassis.c
> index 60c01eb245..07bef1c56d 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -538,12 +538,29 @@ chassis_other_config_changed(const struct
> ovs_chassis_cfg *ovs_cfg,
> return true;
> }
>
> + if (!smap_get_bool(&chassis_rec->other_config,
> OVN_FEATURE_PORT_UP_NOTIF,
> + false)) {
> + return true;
> + }
> +
> + if (!smap_get_bool(&chassis_rec->other_config,
> + OVN_FEATURE_CT_NO_MASKED_LABEL,
> + false)) {
> + return true;
> + }
> +
> if (!smap_get_bool(&chassis_rec->other_config,
> OVN_FEATURE_MAC_BINDING_TIMESTAMP,
> false)) {
> return true;
> }
>
> + if (!smap_get_bool(&chassis_rec->other_config,
> + OVN_FEATURE_CT_LB_RELATED,
> + false)) {
> + return true;
> + }
> +
> if (!smap_get_bool(&chassis_rec->other_config,
> OVN_FEATURE_FDB_TIMESTAMP,
> false)) {
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 17ec3c5d95..0b00906ae7 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -642,7 +642,9 @@ ovn_attach n1 br-phys 192.168.0.1
>
> # Wait for ovn-controller to register in the SB.
> wait_row_count Chassis 1 name=hv1
> other_config:mac-binding-timestamp="true"
> -check_row_count Chassis 1 name=hv1 other_config:fdb-timestamp="true"
> +check_row_count Chassis 1 name=hv1 other_config:port-up-notif="true"
> +check_row_count Chassis 1 name=hv1 other_config:ct-no-masked-label="true"
> +check_row_count Chassis 1 name=hv1 other_config:ovn-ct-lb-related="true"
> check_row_count Chassis 1 name=hv1 other_config:ls-dpg-column="true"
> check_row_count Chassis 1 name=hv1 other_config:ct-commit-nat-v2="true"
> check_row_count Chassis 1 name=hv1 other_config:ct-commit-to-zone="true"
> ---
>
> With that addressed, and assuming CI is green:
>
> Acked-by: Dumitru Ceara <[email protected]>
>
I have added the following diff and merged this into main.
>
> Regards,
> Dumitru
>
> > diff --git a/include/ovn/features.h b/include/ovn/features.h
> > index d5828a7e5..3caeaea25 100644
> > --- a/include/ovn/features.h
> > +++ b/include/ovn/features.h
> > @@ -31,6 +31,12 @@
> > #define OVN_FEATURE_CT_LABEL_FLUSH "ct-label-flush"
> > #define OVN_FEATURE_CT_STATE_SAVE "ct-state-save"
> >
> > +/* DEPRACATED: The following features can be removed
> > + * after the next LTS version release. */
> > +#define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif"
> > +#define OVN_FEATURE_CT_NO_MASKED_LABEL "ct-no-masked-label"
> > +#define OVN_FEATURE_CT_LB_RELATED "ovn-ct-lb-related"
> > +
> > /* OVS datapath supported features. Based on availability OVN might
> generate
> > * different types of openflows.
> > */
>
>
Thank you,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
