On Tue, Jul 26, 2022 at 1:37 AM Ales Musil <[email protected]> wrote:
> > > > 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. > > Thank you for the answer. I think it's fair that if CMS wants to opt in this behavior, they can stage migration in phases / pre-timestamp bindings before enabling cleanup threshold. > >> >>> >>> >>>> >>>> >>>> > + } 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
