On Wed, Aug 10, 2022 at 3:22 PM Ales Musil <[email protected]> wrote:
>
> On Wed, Aug 10, 2022 at 3:20 AM Numan Siddique <[email protected]> wrote:
>
> > On Tue, Aug 9, 2022 at 9:06 PM 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]>
> >
> > Hi Ales,
> >
> > Thanks for v4. I see a couple of problems during upgrades.
> >
>
> Hi Numan,
>
> 1. What happens to the existing mac_binding entries ? Can you please
> > test this scenario out ?
> > I think when the dbs are upgraded to the new schema, all the mac
> > binding entries will have the timestamp value set to 0.
> > And ovn-northd would delete all the existing mac binding entries
> > of a datapath if mac_binding_age_threshold is configured.
> > Perhaps this is not a big issue as the existing entries would have
> > to be deleted anyway.
> >
>
> we have actually discussed it with Ihar earlier, yes the upgraded will have
> timestamp=0, I still think that this is up to CMS to handle it. The feature
> is disabled by
> default. As long as we communicate how it behaves on upgrade we should be
> fine IMO.
> I could put a description of this behavior into ovn-architecture.7.xml or
> any other better place, WDYT?
>
>
Sorry. I missed out on the discussion earlier.
My concern is that this option is not a global config, but per logical
router config.
And CMS like openstack neutron or ovn-kubernetes would definitely not know if
all the computer/worker nodes have been upgraded or not before setting this
option in the desired logical routers.
@Daniel Alvarez Sanchez - Do you have any comments on this ?
We already have similar functionality in ovn-northd to check if all
ovn-controllers support a particular
feature or not. I think we should use that here too.
ovn-northd can detect that and just disable the mac binging until all
ovn-controllers report that
they support setting timestamp in mac binding rows.
If we have to document, ovn-nb.xml seems to be the right place.
Thanks
Numan
> >
> > 2. What happens when ovn-northd and dbs are upgraded but
> > ovn-controllers are not yet upgraded ?
> > I tested this scenario. When ovn-controllers learn a mac binding
> > entry, the timestamp will be 0 and ovn-northd will immediately delete
> > the learnt mac_binding entry.
> > This scenario is very likely to happen and this behaviour seems to
> > be problematic.
> > Either ovn-northd should disable this mac binding aging feature
> > until all the ovn-controllers are upgraded or ovn-northd should not
> > delete mac binding entries if timestamp is 0.
> > If we take the latter approach we may not be able to age the mac
> > binding entries learnt before the upgrade to this feature.
> >
> >
> I think the same as before applies here, it should be up to CMS to enable
> it when
> it is comfortable.
>
>
> >
> > Thoughts ?
> >
> > Numan
> >
> >
> >
> Thanks,
> Ales
>
>
> >
> > > ---
> > > v3: Rebase on top of current main.
> > > Update according to the final conclusion.
> > > v4: Rebase on top of current main.
> > > Skip one row instead of all if time elapsed is negative.
> > > Add note to NEWS.
> > > ---
> > > NEWS | 3 +
> > > 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 ++
> > > 6 files changed, 209 insertions(+)
> > > create mode 100644 northd/mac-binding-aging.c
> > > create mode 100644 northd/mac-binding-aging.h
> > >
> > > diff --git a/NEWS b/NEWS
> > > index 20cea579e..eff8e472a 100644
> > > --- a/NEWS
> > > +++ b/NEWS
> > > @@ -14,6 +14,9 @@ Post v22.06.0
> > > NAT-T UDP encapsulation. Requires OVS support for IPsec custom
> > tunnel
> > > options (which will be available in OVS 2.18).
> > > - Removed possibility of disabling logical datapath groups.
> > > + - Added MAC binding aging mechanism, that is disabled by default.
> > > + It can be enabled per logical router with option
> > > + "mac_binding_age_threshold".
> > >
> > > OVN v22.06.0 - 03 Jun 2022
> > > --------------------------
> > > 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..e8217e8bc
> > > --- /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) {
> > > + continue;
> > > + } else if (elapsed >= threshold) {
> > > + sbrec_mac_binding_delete(mb);
> > > + } 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.37.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > [email protected]
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > >
> >
> >
>
> --
>
> 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
