On Tue, Aug 16, 2022 at 1:07 PM Dumitru Ceara <[email protected]> wrote:
> On 8/16/22 08:33, Ales Musil wrote: > > It can happen that northd and SB DB are updated before ovn-controller > > in that case the new MAC binding would be added with timestamp=0. > > In combination with enabled MAC binding aging, the affected rows > > would be deleted over and over until the controller is upgraded. > > > > To prevent the before mentioned issue add indication if > > the controller supports MAC binding timestamps. > > > > Signed-off-by: Ales Musil <[email protected]> > > --- > > controller/chassis.c | 7 +++++++ > > include/ovn/features.h | 1 + > > northd/mac-binding-aging.c | 5 +++-- > > northd/northd.c | 21 +++++++++++++++------ > > northd/northd.h | 1 + > > 5 files changed, 27 insertions(+), 8 deletions(-) > > > > diff --git a/controller/chassis.c b/controller/chassis.c > > index 3d75f10ad..685d9b2ae 100644 > > --- a/controller/chassis.c > > +++ b/controller/chassis.c > > @@ -351,6 +351,7 @@ chassis_build_other_config(const struct > ovs_chassis_cfg *ovs_cfg, > > 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"); > > } > > > > /* > > @@ -462,6 +463,12 @@ chassis_other_config_changed(const struct > ovs_chassis_cfg *ovs_cfg, > > return true; > > } > > > > + if (!smap_get_bool(&chassis_rec->other_config, > > + OVN_FEATURE_MAC_BINDING_TIMESTAMP, > > + false)) { > > + return true; > > + } > > + > > return false; > > } > > > > diff --git a/include/ovn/features.h b/include/ovn/features.h > > index 8fbdbf19a..679f67457 100644 > > --- a/include/ovn/features.h > > +++ b/include/ovn/features.h > > @@ -23,6 +23,7 @@ > > /* ovn-controller supported feature names. */ > > #define OVN_FEATURE_PORT_UP_NOTIF "port-up-notif" > > #define OVN_FEATURE_CT_NO_MASKED_LABEL "ct-no-masked-label" > > +#define OVN_FEATURE_MAC_BINDING_TIMESTAMP "mac-binding-timestamp" > > > > /* OVS datapath supported features. Based on availability OVN might > generate > > * different types of openflows. > > diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c > > index 36d0a6fd7..4a2dfbbf8 100644 > > --- a/northd/mac-binding-aging.c > > +++ b/northd/mac-binding-aging.c > > @@ -90,8 +90,10 @@ void > > en_mac_binding_aging_run(struct engine_node *node, void *data > OVS_UNUSED) > > { > > const struct engine_context *eng_ctx = engine_get_context(); > > + struct northd_data *northd_data = engine_get_input_data("northd", > node); > > > > - if (!eng_ctx->ovnsb_idl_txn) { > > + if (!eng_ctx->ovnsb_idl_txn || > > + !northd_data->features.mac_binding_timestamp) { > > return; > > } > > > > @@ -99,7 +101,6 @@ en_mac_binding_aging_run(struct engine_node *node, > void *data OVS_UNUSED) > > int64_t now = time_wall_msec(); > > uint32_t removal_limit = get_removal_limit(node); > > uint32_t removed_n = 0; > > - struct northd_data *northd_data = engine_get_input_data("northd", > node); > > struct mac_binding_waker *waker = > > engine_get_input_data("mac_binding_aging_waker", node); > > struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = > > diff --git a/northd/northd.c b/northd/northd.c > > index 76d79c5ce..684313a18 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -407,14 +407,23 @@ build_chassis_features(const struct northd_input > *input_data, > > const struct sbrec_chassis *chassis; > > > > SBREC_CHASSIS_TABLE_FOR_EACH (chassis, input_data->sbrec_chassis) { > > - if (!smap_get_bool(&chassis->other_config, > > - OVN_FEATURE_CT_NO_MASKED_LABEL, > > - false)) { > > + bool ct_no_masked_label = > > + smap_get_bool(&chassis->other_config, > > + OVN_FEATURE_CT_NO_MASKED_LABEL, > > + false); > > + if (!ct_no_masked_label && > chassis_features->ct_no_masked_label) { > > chassis_features->ct_no_masked_label = false; > > - return; > > + } > > + > > + bool mac_binding_timestamp = > > + smap_get_bool(&chassis->other_config, > > + OVN_FEATURE_MAC_BINDING_TIMESTAMP, > > + false); > > + if (!mac_binding_timestamp && > > + chassis_features->mac_binding_timestamp) { > > + chassis_features->mac_binding_timestamp = false; > > } > > } > > - chassis_features->ct_no_masked_label = true; > > To make it explicit, please move the initialization part to the > beginning of the function or to northd_init(). We can use designated > initializers: > > *chassis_features = (struct chassis_features) { > .ct_no_masked_label = true, > .mac_binding_timestamp = true, > }; > Makes sense, it is done in northd_init. > > > } > > > > struct ovn_chassis_qdisc_queues { > > @@ -15202,7 +15211,7 @@ northd_init(struct northd_data *data) > > hmap_init(&data->lbs); > > hmap_init(&data->bfd_connections); > > ovs_list_init(&data->lr_list); > > - memset(&data->features, 0, sizeof data->features); > > + memset(&data->features, true, sizeof data->features); > > This looks a bit weird to me, 'data->features' is not an "array of > bool". I think we either leave this as it was, zeroed by default, and > set what's needed to true/false in build_chassis_features() or > initialize all the fields to true explicitly here (using designated > initializers). > > > data->ovn_internal_version_changed = false; > > } > > > > diff --git a/northd/northd.h b/northd/northd.h > > index dcbed1b1f..d9856af97 100644 > > --- a/northd/northd.h > > +++ b/northd/northd.h > > @@ -64,6 +64,7 @@ struct northd_input { > > > > struct chassis_features { > > bool ct_no_masked_label; > > + bool mac_binding_timestamp; > > }; > > > > struct northd_data { > > Thanks, > Dumitru > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
