On Sun, Aug 14, 2022 at 8:30 PM Numan Siddique <[email protected]> wrote: > > On Fri, Aug 12, 2022 at 12:21 AM Ales Musil <[email protected]> wrote: > > > > On Thu, Aug 11, 2022 at 2:45 PM Dumitru Ceara <[email protected]> wrote: > > > > > On 8/10/22 18:37, Han Zhou wrote: > > > > On Tue, Aug 9, 2022 at 11:22 PM Ales Musil <[email protected]> wrote: > > > >> > > > >> > > > >> > > > >> On Wed, Aug 10, 2022 at 8:04 AM Han Zhou <[email protected]> wrote: > > > >> > > > >> Hi Han, > > > >> > > > >> thank you for the review, see my reply inline below. > > > >> > > > >>> On Tue, Aug 9, 2022 at 4:05 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. > > > >>>> 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; > > > > > > Nit: indentation. > > > > > > >>>> + 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(); > > > >>> > > > >>> Why would we immediately wake up? Wouldn't it cause busy-loop? Shall > > > we > > > > use poll_timer_wait_until? Sorry if I missed anything here and please > > > > correct me. > > > >> > > > >> > > > >> That's because of the dependency of the nodes, we need to run the waker > > > > that will schedule the sleep. > > > >> We could probably use poll_timer_wait_until as well and it should have > > > > the same effect. > > > >> > > > > Thanks, I think I did misunderstand a little and now I got the idea of > > > the > > > > waker node. It is an input node just to trigger the timer, which wakes up > > > > the aging processing when needed and at the same time avoids running the > > > > aging scanning when the process is woken up for other reasons. So the > > > > poll_immediate_wake() here won't cause busy-loop. However, it does wake > > > up > > > > the process at least one extra time for each run, although the cost may > > > be > > > > small. I'd still use poll_timer_wait_until() here since we already know > > > the > > > > next_wake_msec. > > > > > > > > In addition, it is a little weird from I-P engine's point of view that > > > the > > > > output node execution updates the input node's data, but for this timer > > > use > > > > case I think it should work and I haven't come up with anything better. > > > It > > > > would be helpful to leave some comments for the waker node to explain the > > > > motivation and make this part easier to understand. > > > > > > > > > > +1 for both points (comment + poll_timer_wait_until()). > > > > > > Also, can we at least use engine_get_input_data() to get the input > > > node's data? This will make it more clear that we're using data that's > > > related to the waker node. The 'waker' variable would have to be moved > > > inside the en_mac_binding_aging_waker data. > > > > > > > Thanks, > > > > Han > > > > > > > >>> > > > >>> > > > >>>> + } 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. */ > > > >>> > > > >>> This should work, but it is like a hack. Ideally we should allow the > > > I-P > > > > engine to have multiple "root" nodes. This can be a future improvement > > > > though. Maybe add a "XXX" comment here (a TODO item). > > > >> > > > >> > > > >> Yes unfortunately this is not the cleanest solution, but I did not come > > > > with a better one. I will add a TODO comment here about moving it once we > > > > allow multiple root nodes. > > > >> > > > >>> > > > >>> > > > >>> Thanks, > > > >>> Han > > > >>> > > > >> > > > >> Thanks, > > > >> Ales > > > >> > > > >>> > > > >>> > > > >>>> + 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 > > > > > > Nit: I'd use 'seconds'. > > > > > > Thanks, > > > Dumitru > > > > > > >>>> + exceeding this timeout will be automatically removed. The > > > > value > > > >>>> + defaults to 0, which means disabled. > > > >>>> + </column> > > > >>>> </group> > > > >>>> > > > >>>> <group title="Common Columns"> > > > >>>> -- > > > >>>> 2.37.1 > > > >>>> > > > >> > > > >> > > > >> > > > >> -- > > > >> > > > >> Ales Musil > > > >> > > > >> Senior Software Engineer - OVN Core > > > >> > > > >> Red Hat EMEA > > > >> > > > >> [email protected] IM: amusil > > > > > > > > > > > > Thank you for the reviews. > > > > v5 should address most of the comments except the open one from Numan about > > upgrade. > > > > IMO upgrading one is very important. As I mentioned earlier, > openstack neutron or ovn-kubernetes would not know > which OVN version is being run and if all the ovn-controllers are > upgraded to the version which has the mac_binding timestamp > support. This config option is not something which an administrator > can enable or disable depending on the upgrade status. > > There have been a few upgrade issues with OVN and because of which > openstack CMS had to do special handling. > This patch series can potentially affect the existing datapath traffic > after partial upgrades. So I think its better we handle this > properly. >
Hi Numan, I agree with you that upgrading is important. But for this case, maybe I didn't understand the problem. The feature is disabled by default, so as long as the CMS doesn't enable the aging setting for any LR before the upgrade is completed, it should be fine, right? This in practice just means two separate changes to the production environment: 1. upgrade the OVN version, both central and controller components 2. change the aging setting to desired value for some (or all) LRs Thanks, Han > Numan > > > 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 > > > _______________________________________________ > 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
