On Mon, Jul 25, 2022 at 5:53 PM Ihar Hrachyshka <[email protected]> wrote:
> On Mon, Jul 25, 2022 at 1:23 AM Ales Musil <[email protected]> wrote: > >> Hi Ihar, >> >> >> On Fri, Jul 22, 2022 at 8:38 PM Ihar Hrachyshka <[email protected]> >> wrote: >> >>> On Wed, Jul 20, 2022 at 4:50 AM Ales Musil <[email protected]> wrote: >>> > >>> > Add MAC binding aging mechanism, that utilizes >>> > the timestamp column of MAC_Binding table. >>> > When the MAC binding exceeds the threshold it is >>> > removed from SB DB, this is postponed only in case >>> > we receive update ARP with update to MAC address. >>> > >>> > The threshold is configurable via option >>> > "mac_binding_age_threshold" that can be specified >>> > for each logical router. The option is defaulting to >>> > 0 which means that by default the aging is disabled >>> > and the MAC binding rows will be persisted the same >>> > way as before. >>> > >>> > Reported-at: https://bugzilla.redhat.com/2084668 >>> > Signed-off-by: Ales Musil <[email protected]> >>> > --- >>> > v3: Rebase on top of current main. >>> > Update according to the final conclusion. >>> > --- >>> > northd/automake.mk | 2 + >>> > northd/inc-proc-northd.c | 13 ++++ >>> > northd/mac-binding-aging.c | 151 +++++++++++++++++++++++++++++++++++++ >>> > northd/mac-binding-aging.h | 33 ++++++++ >>> > ovn-nb.xml | 7 ++ >>> > 5 files changed, 206 insertions(+) >>> > create mode 100644 northd/mac-binding-aging.c >>> > create mode 100644 northd/mac-binding-aging.h >>> > >>> > diff --git a/northd/automake.mk b/northd/automake.mk >>> > index 4862ec7b7..81582867d 100644 >>> > --- a/northd/automake.mk >>> > +++ b/northd/automake.mk >>> > @@ -1,6 +1,8 @@ >>> > # ovn-northd >>> > bin_PROGRAMS += northd/ovn-northd >>> > northd_ovn_northd_SOURCES = \ >>> > + northd/mac-binding-aging.c \ >>> > + northd/mac-binding-aging.h \ >>> > northd/northd.c \ >>> > northd/northd.h \ >>> > northd/ovn-northd.c \ >>> > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >>> > index 43093cb5a..4a3699106 100644 >>> > --- a/northd/inc-proc-northd.c >>> > +++ b/northd/inc-proc-northd.c >>> > @@ -22,9 +22,11 @@ >>> > #include "ip-mcast-index.h" >>> > #include "static-mac-binding-index.h" >>> > #include "lib/inc-proc-eng.h" >>> > +#include "lib/mac-binding-index.h" >>> > #include "lib/ovn-nb-idl.h" >>> > #include "lib/ovn-sb-idl.h" >>> > #include "mcast-group-index.h" >>> > +#include "northd/mac-binding-aging.h" >>> > #include "openvswitch/poll-loop.h" >>> > #include "openvswitch/vlog.h" >>> > #include "inc-proc-northd.h" >>> > @@ -149,6 +151,8 @@ enum sb_engine_node { >>> > * avoid sparse errors. */ >>> > static ENGINE_NODE(northd, "northd"); >>> > static ENGINE_NODE(lflow, "lflow"); >>> > +static ENGINE_NODE(mac_binding_aging, "mac_binding_aging"); >>> > +static ENGINE_NODE(mac_binding_aging_waker, >>> "mac_binding_aging_waker"); >>> > >>> > void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> > struct ovsdb_idl_loop *sb) >>> > @@ -211,12 +215,16 @@ void inc_proc_northd_init(struct ovsdb_idl_loop >>> *nb, >>> > engine_add_input(&en_northd, &en_sb_load_balancer, NULL); >>> > engine_add_input(&en_northd, &en_sb_fdb, NULL); >>> > engine_add_input(&en_northd, &en_sb_static_mac_binding, NULL); >>> > + engine_add_input(&en_mac_binding_aging, &en_sb_mac_binding, NULL); >>> > + engine_add_input(&en_mac_binding_aging, &en_northd, NULL); >>> > + engine_add_input(&en_mac_binding_aging, >>> &en_mac_binding_aging_waker, NULL); >>> > engine_add_input(&en_lflow, &en_nb_bfd, NULL); >>> > engine_add_input(&en_lflow, &en_sb_bfd, NULL); >>> > engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); >>> > engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); >>> > engine_add_input(&en_lflow, &en_sb_igmp_group, NULL); >>> > engine_add_input(&en_lflow, &en_northd, NULL); >>> > + engine_add_input(&en_lflow, &en_mac_binding_aging, NULL); >>> > >>> > struct engine_arg engine_arg = { >>> > .nb_idl = nb->idl, >>> > @@ -235,6 +243,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop >>> *nb, >>> > chassis_hostname_index_create(sb->idl); >>> > struct ovsdb_idl_index *sbrec_static_mac_binding_by_lport_ip >>> > = static_mac_binding_index_create(sb->idl); >>> > + struct ovsdb_idl_index *sbrec_mac_binding_by_datapath >>> > + = mac_binding_by_datapath_index_create(sb->idl); >>> > >>> > engine_init(&en_lflow, &engine_arg); >>> > >>> > @@ -256,6 +266,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop >>> *nb, >>> > engine_ovsdb_node_add_index(&en_sb_static_mac_binding, >>> > >>> "sbrec_static_mac_binding_by_lport_ip", >>> > sbrec_static_mac_binding_by_lport_ip); >>> > + engine_ovsdb_node_add_index(&en_sb_mac_binding, >>> > + "sbrec_mac_binding_by_datapath", >>> > + sbrec_mac_binding_by_datapath); >>> > } >>> > >>> > void inc_proc_northd_run(struct ovsdb_idl_txn *ovnnb_txn, >>> > diff --git a/northd/mac-binding-aging.c b/northd/mac-binding-aging.c >>> > new file mode 100644 >>> > index 000000000..8af477ff1 >>> > --- /dev/null >>> > +++ b/northd/mac-binding-aging.c >>> > @@ -0,0 +1,151 @@ >>> > +/* Copyright (c) 2022, Red Hat, Inc. >>> > + * >>> > + * Licensed under the Apache License, Version 2.0 (the "License"); >>> > + * you may not use this file except in compliance with the License. >>> > + * You may obtain a copy of the License at: >>> > + * >>> > + * http://www.apache.org/licenses/LICENSE-2.0 >>> > + * >>> > + * Unless required by applicable law or agreed to in writing, software >>> > + * distributed under the License is distributed on an "AS IS" BASIS, >>> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> > + * See the License for the specific language governing permissions and >>> > + * limitations under the License. >>> > + */ >>> > + >>> > +#include <config.h> >>> > + >>> > +#include "lib/inc-proc-eng.h" >>> > +#include "lib/ovn-nb-idl.h" >>> > +#include "lib/ovn-sb-idl.h" >>> > +#include "lib/timeval.h" >>> > +#include "northd/mac-binding-aging.h" >>> > +#include "northd/northd.h" >>> > +#include "openvswitch/hmap.h" >>> > +#include "openvswitch/poll-loop.h" >>> > +#include "openvswitch/util.h" >>> > +#include "openvswitch/vlog.h" >>> > + >>> > +VLOG_DEFINE_THIS_MODULE(mac_binding_aging); >>> > + >>> > +struct mac_binding_waker { >>> > + bool should_schedule; >>> > + long long next_wake_msec; >>> > +}; >>> > + >>> > +static struct mac_binding_waker waker; >>> > + >>> > +static void >>> > +mac_binding_aging_run_for_datapath(const struct >>> sbrec_datapath_binding *dp, >>> > + const struct nbrec_logical_router >>> *nbr, >>> > + struct ovsdb_idl_index >>> *mb_by_datapath, >>> > + int64_t now, int64_t *wake_delay) >>> > +{ >>> > + uint64_t threshold = smap_get_uint(&nbr->options, >>> > + "mac_binding_age_threshold", >>> > + 0) * 1000; >>> > + if (!threshold) { >>> > + return; >>> > + } >>> > + >>> > + struct sbrec_mac_binding *mb_index_row = >>> > + sbrec_mac_binding_index_init_row(mb_by_datapath); >>> > + sbrec_mac_binding_index_set_datapath(mb_index_row, dp); >>> > + >>> > + const struct sbrec_mac_binding *mb; >>> > + SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, >>> mb_by_datapath) { >>> > + int64_t elapsed = now - mb->timestamp; >>> > + >>> > + if (elapsed < 0) { >>> > + return; >>> > + } else if (elapsed >= threshold) { >>> > + sbrec_mac_binding_delete(mb); >>> >>> AFAIU if mb->timestamp is not set, it defaults to 0, which will make >>> all MAC bindings removed on OVN upgrade. Is it expected? Will it not >>> affect the dataplane connectivity? >>> >> >> >> Yes this is expected, keep in mind that MAC binding aging is disabled by >> default. >> So it won't have any effect on existing/upgraded setups until someone >> enables it. >> >> > Thanks for clarification. So is the recommendation for a CMS to stage > enrollment into the new feature to spread out the impact? (I can imagine a > CMS to enable it for all existing ports on restart, e.g. neutron runs db > migration tasks on neutron-server restart. Should they avoid it?) > That might be one of the options. CMS could also change the timestamp during the upgrade, it can also be set to some time in future to spread the possible disruptions. But yeah that is definitely something that needs to be discussed before rolling the update. Unfortunately I don't have an idea what else we can do from OVN side. > > >> >> >>> >>> >>> > + } else { >>> > + *wake_delay = MIN(*wake_delay, threshold - elapsed); >>> > + } >>> > + } >>> > + sbrec_mac_binding_index_destroy_row(mb_index_row); >>> > +} >>> > + >>> > +void >>> > +en_mac_binding_aging_run(struct engine_node *node, void *data >>> OVS_UNUSED) >>> > +{ >>> > + const struct engine_context *eng_ctx = engine_get_context(); >>> > + >>> > + if (!eng_ctx->ovnsb_idl_txn) { >>> > + return; >>> > + } >>> > + >>> > + int64_t next_expire_msec = INT64_MAX; >>> > + int64_t now = time_wall_msec(); >>> > + struct northd_data *northd_data = engine_get_input_data("northd", >>> node); >>> > + struct ovsdb_idl_index *sbrec_mac_binding_by_datapath = >>> > + >>> engine_ovsdb_node_get_index(engine_get_input("SB_mac_binding", node), >>> > + "sbrec_mac_binding_by_datapath"); >>> > + >>> > + struct ovn_datapath *od; >>> > + HMAP_FOR_EACH (od, key_node, &northd_data->datapaths) { >>> > + if (od->sb && od->nbr) { >>> > + mac_binding_aging_run_for_datapath(od->sb, od->nbr, >>> > + >>> sbrec_mac_binding_by_datapath, >>> > + now, >>> &next_expire_msec); >>> > + } >>> > + } >>> > + >>> > + if (next_expire_msec < INT64_MAX) { >>> > + waker.should_schedule = true; >>> > + waker.next_wake_msec = time_msec() + next_expire_msec; >>> > + /* Run the engine right after so the waker can reflect on the >>> new wake >>> > + * time. */ >>> > + poll_immediate_wake(); >>> > + } else { >>> > + waker.should_schedule = false; >>> > + } >>> > + >>> > + /* This node is part of lflow, but lflow does not depend on it. >>> Setting >>> > + * state as unchanged does not trigger lflow node when it is not >>> needed. */ >>> > + engine_set_node_state(node, EN_UNCHANGED); >>> > +} >>> > + >>> > +void * >>> > +en_mac_binding_aging_init(struct engine_node *node OVS_UNUSED, >>> > + struct engine_arg *arg OVS_UNUSED) >>> > +{ >>> > + return NULL; >>> > +} >>> > + >>> > +void >>> > +en_mac_binding_aging_cleanup(void *data OVS_UNUSED) >>> > +{ >>> > +} >>> > + >>> > +void >>> > +en_mac_binding_aging_waker_run(struct engine_node *node, void *data >>> OVS_UNUSED) >>> > +{ >>> > + if (!waker.should_schedule) { >>> > + return; >>> > + } >>> > + >>> > + if (time_msec() >= waker.next_wake_msec) { >>> > + waker.should_schedule = false; >>> > + engine_set_node_state(node, EN_UPDATED); >>> > + return; >>> > + } >>> > + >>> > + engine_set_node_state(node, EN_UNCHANGED); >>> > + poll_timer_wait_until(waker.next_wake_msec); >>> > +} >>> > + >>> > +void * >>> > +en_mac_binding_aging_waker_init(struct engine_node *node OVS_UNUSED, >>> > + struct engine_arg *arg OVS_UNUSED) >>> > +{ >>> > + waker.should_schedule = false; >>> > + waker.next_wake_msec = 0; >>> > + return NULL; >>> > +} >>> > + >>> > +void >>> > +en_mac_binding_aging_waker_cleanup(void *data OVS_UNUSED) >>> > +{ >>> > +} >>> > diff --git a/northd/mac-binding-aging.h b/northd/mac-binding-aging.h >>> > new file mode 100644 >>> > index 000000000..296a7ab38 >>> > --- /dev/null >>> > +++ b/northd/mac-binding-aging.h >>> > @@ -0,0 +1,33 @@ >>> > +/* Copyright (c) 2022, Red Hat, Inc. >>> > + * >>> > + * Licensed under the Apache License, Version 2.0 (the "License"); >>> > + * you may not use this file except in compliance with the License. >>> > + * You may obtain a copy of the License at: >>> > + * >>> > + * http://www.apache.org/licenses/LICENSE-2.0 >>> > + * >>> > + * Unless required by applicable law or agreed to in writing, software >>> > + * distributed under the License is distributed on an "AS IS" BASIS, >>> > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or >>> implied. >>> > + * See the License for the specific language governing permissions and >>> > + * limitations under the License. >>> > + */ >>> > + >>> > +#ifndef MAC_BINDING_AGING_H >>> > +#define MAC_BINDING_AGING_H 1 >>> > + >>> > +#include "lib/inc-proc-eng.h" >>> > + >>> > +/* The MAC binding aging node functions. */ >>> > +void en_mac_binding_aging_run(struct engine_node *node, void *data); >>> > +void *en_mac_binding_aging_init(struct engine_node *node, >>> > + struct engine_arg *arg); >>> > +void en_mac_binding_aging_cleanup(void *data); >>> > + >>> > +/* The MAC binding aging waker node functions. */ >>> > +void en_mac_binding_aging_waker_run(struct engine_node *node, void >>> *data); >>> > +void *en_mac_binding_aging_waker_init(struct engine_node *node, >>> > + struct engine_arg *arg); >>> > +void en_mac_binding_aging_waker_cleanup(void *data); >>> > + >>> > +#endif /* northd/mac-binding-aging.h */ >>> > diff --git a/ovn-nb.xml b/ovn-nb.xml >>> > index e26afd83c..d3fba9bdc 100644 >>> > --- a/ovn-nb.xml >>> > +++ b/ovn-nb.xml >>> > @@ -2392,6 +2392,13 @@ >>> > and other sources. This way, OVN and the other sources can >>> make use of >>> > the same conntrack zone. >>> > </column> >>> > + >>> > + <column name="options" key="mac_binding_age_threshold" >>> > + type='{"type": "integer", "minInteger": 0, >>> "maxInteger": 4294967295}'> >>> > + MAC binding aging <code>threshold</code> value in secs. MAC >>> binding >>> > + exceeding this timeout will be automatically removed. The >>> value >>> > + defaults to 0, which means disabled. >>> > + </column> >>> > </group> >>> > >>> > <group title="Common Columns"> >>> > -- >>> > 2.35.3 >>> > >>> > _______________________________________________ >>> > dev mailing list >>> > [email protected] >>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> > >>> >>> >> Regards, >> Ales >> >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> [email protected] IM: amusil >> <https://red.ht/sig> >> > 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
