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