On Mon, Jan 22, 2024 at 6:36 AM Ales Musil <amu...@redhat.com> wrote:
>
> On Mon, Jan 22, 2024 at 9:09 AM Felix Huettner via dev <
> ovs-dev@openvswitch.org> wrote:
>
> > On Fri, Jan 19, 2024 at 04:33:28PM -0500, Mark Michelson wrote:
> > > With this change, a chassis may only update MAC Binding records that
it
> > > has created. We achieve this by adding a "chassis_name" column to the
> > > MAC_Binding table, and having the chassis insert its name into this
> > > column when creating a new MAC_Binding. The "chassis_name" is now part
> > > of the rbac_auth structure for the MAC_Binding table.
> >
> > Hi Mark,
> >
> > i am concerned that this will negatively impact MAC_Bindings for LRPs
> > with multiple gateway chassis.
> >
> > Suppose a MAC_Binding is first learned by an LRP currently residing on
> > chassis1. The LRP then failovers to chassis2 and chassis1 is potentially
> > even
> > removed completely. In this case the ovn-controller on chassis2 would no
> > longer be allowed to update the timestamp column. This would break the
> > arp refresh mechanism.
> >
> > In this case the MAC_Binding would need to expire first, causing northd
> > to removed it. Afterwards chassis2 would be allowed to insert a new
> > record with its own chassis name.
> >
> > I honestly did not try out this case so i am not fully sure if this
> > issue realy exists or if i have a missunderstanding somewhere.
> >
> > Thanks
> > Felix
> >
> >
> Hi Mark and Felix,
>
> I personally don't see the ability to not refresh as an issue, the MAC
> binding would age out and the node could create a new one. However, it
will
> still produce errors when the remote chassis tries to update the timestamp
> of MAC binding owned by someone else.
>
> There is another issue that I'm more concerned about and that's in case
the
> aging is not enabled at all. After failover the MAC binding might not be
> updated at all. Similar issue applies to MAM bindings distributed across
> many chassis. One will own it and only that chassis can update MAC address
> when anything changes which it might never do.
This is indeed a fundamental problem. Even if aging is configured it is
still a problem. Let's say aging is set as 5 min, then when the IP is moved
to a different chassis it will not work within 5 min, which is unacceptable.
In fact the below test case fails with this patch:
81. ovn.at:5232: testing IP relocation using GARP request --
parallelization=yes -- ovn_monitor_all=yes ...
>
> To solve that we would need duplicates per chassis, basically the same MAC
> binding row, but with different "owners". This goes in hand with having OF
> only for MAC bindings owned by current chassis and nothing else. Does that
> make sense?
>
If each chassis has OF only for MAC bindings owned by itself, there is no
point to have MAC binding table in SB DB, right?
But it doesn't work since the MAC binding table works as ARP cache/Neigbor
table for a distributed router, so we will need a central place to share
this information. Otherwise, when a IP moves from one chassis to another,
how would other chassis know? (the same scenario as the test case 81 above,
and similarly there will be problem for DGP failover)
However I don't have a good solution either. Need to think more about it.
Thanks,
Han
> All the above unfortunately applies also to FDB.
>
> Thanks,
> Ales
>
>
> > > ---
> > > controller/pinctrl.c | 51
++++++++++++++++++++++++++++++++------------
> > > northd/ovn-northd.c | 2 +-
> > > ovn-sb.ovsschema | 7 +++---
> > > ovn-sb.xml | 3 +++
> > > 4 files changed, 45 insertions(+), 18 deletions(-)
> > >
> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > index 4992eab08..a00cdceea 100644
> > > --- a/controller/pinctrl.c
> > > +++ b/controller/pinctrl.c
> > > @@ -180,6 +180,7 @@ struct pinctrl {
> > > bool mac_binding_can_timestamp;
> > > bool fdb_can_timestamp;
> > > bool dns_supports_ovn_owned;
> > > + bool mac_binding_has_chassis_name;
> > > };
> > >
> > > static struct pinctrl pinctrl;
> > > @@ -204,7 +205,8 @@ static void run_put_mac_bindings(
> > > struct ovsdb_idl_txn *ovnsb_idl_txn,
> > > struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> > > struct ovsdb_idl_index *sbrec_port_binding_by_key,
> > > - struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> > > + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> > > + const struct sbrec_chassis *chassis)
> > > OVS_REQUIRES(pinctrl_mutex);
> > > static void wait_put_mac_bindings(struct ovsdb_idl_txn
*ovnsb_idl_txn);
> > > static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> > > @@ -3591,6 +3593,13 @@ pinctrl_update(const struct ovsdb_idl *idl,
const
> > char *br_int_name)
> > > notify_pinctrl_handler();
> > > }
> > >
> > > + bool mac_binding_has_chassis_name =
> > > + sbrec_server_has_mac_binding_table_col_chassis_name(idl);
> > > + if (mac_binding_has_chassis_name !=
> > pinctrl.mac_binding_has_chassis_name) {
> > > + pinctrl.mac_binding_has_chassis_name =
> > mac_binding_has_chassis_name;
> > > + notify_pinctrl_handler();
> > > + }
> > > +
> > > ovs_mutex_unlock(&pinctrl_mutex);
> > > }
> > >
> > > @@ -3621,7 +3630,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > > ovs_mutex_lock(&pinctrl_mutex);
> > > run_put_mac_bindings(ovnsb_idl_txn,
sbrec_datapath_binding_by_key,
> > > sbrec_port_binding_by_key,
> > > - sbrec_mac_binding_by_lport_ip);
> > > + sbrec_mac_binding_by_lport_ip,
> > > + chassis);
> > > run_put_vport_bindings(ovnsb_idl_txn,
sbrec_datapath_binding_by_key,
> > > sbrec_port_binding_by_key, chassis);
> > > send_garp_rarp_prepare(ovnsb_idl_txn,
> > sbrec_port_binding_by_datapath,
> > > @@ -4285,7 +4295,8 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > const char *logical_port,
> > > const struct sbrec_datapath_binding *dp,
> > > struct eth_addr ea, const char *ip,
> > > - bool update_only)
> > > + bool update_only,
> > > + const struct sbrec_chassis *chassis)
> > > {
> > > /* Convert ethernet argument to string form for database. */
> > > char mac_string[ETH_ADDR_STRLEN + 1];
> > > @@ -4302,6 +4313,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > sbrec_mac_binding_set_logical_port(b, logical_port);
> > > sbrec_mac_binding_set_ip(b, ip);
> > > sbrec_mac_binding_set_datapath(b, dp);
> > > + if (pinctrl.mac_binding_has_chassis_name) {
> > > + sbrec_mac_binding_set_chassis_name(b, chassis->name);
> > > + }
> > > }
> > >
> > > if (strcmp(b->mac, mac_string)) {
> > > @@ -4323,7 +4337,8 @@ send_garp_locally(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > struct ovsdb_idl_index
*sbrec_mac_binding_by_lport_ip,
> > > const struct hmap *local_datapaths,
> > > const struct sbrec_port_binding *in_pb,
> > > - struct eth_addr ea, ovs_be32 ip)
> > > + struct eth_addr ea, ovs_be32 ip,
> > > + const struct sbrec_chassis *chassis)
> > > {
> > > if (!ovnsb_idl_txn) {
> > > return;
> > > @@ -4351,7 +4366,7 @@ send_garp_locally(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > ip_format_masked(ip, OVS_BE32_MAX, &ip_s);
> > > mac_binding_add_to_sb(ovnsb_idl_txn,
> > sbrec_mac_binding_by_lport_ip,
> > > remote->logical_port, remote->datapath,
> > > - ea, ds_cstr(&ip_s), update_only);
> > > + ea, ds_cstr(&ip_s), update_only,
chassis);
> > > ds_destroy(&ip_s);
> > > }
> > > }
> > > @@ -4361,7 +4376,8 @@ run_put_mac_binding(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > struct ovsdb_idl_index
> > *sbrec_datapath_binding_by_key,
> > > struct ovsdb_idl_index
*sbrec_port_binding_by_key,
> > > struct ovsdb_idl_index
> > *sbrec_mac_binding_by_lport_ip,
> > > - const struct mac_binding *mb)
> > > + const struct mac_binding *mb,
> > > + const struct sbrec_chassis *chassis)
> > > {
> > > /* Convert logical datapath and logical port key into lport. */
> > > const struct sbrec_port_binding *pb = lport_lookup_by_key(
> > > @@ -4384,7 +4400,7 @@ run_put_mac_binding(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > ipv6_format_mapped(&mb->ip, &ip_s);
> > > mac_binding_add_to_sb(ovnsb_idl_txn,
sbrec_mac_binding_by_lport_ip,
> > > pb->logical_port, pb->datapath, mb->mac,
> > > - ds_cstr(&ip_s), false);
> > > + ds_cstr(&ip_s), false, chassis);
> > > ds_destroy(&ip_s);
> > > }
> > >
> > > @@ -4394,7 +4410,8 @@ static void
> > > run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
> > > struct ovsdb_idl_index
> > *sbrec_datapath_binding_by_key,
> > > struct ovsdb_idl_index
*sbrec_port_binding_by_key,
> > > - struct ovsdb_idl_index
> > *sbrec_mac_binding_by_lport_ip)
> > > + struct ovsdb_idl_index
> > *sbrec_mac_binding_by_lport_ip,
> > > + const struct sbrec_chassis *chassis)
> > > OVS_REQUIRES(pinctrl_mutex)
> > > {
> > > if (!ovnsb_idl_txn) {
> > > @@ -4409,7 +4426,8 @@ run_put_mac_bindings(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > run_put_mac_binding(ovnsb_idl_txn,
> > > sbrec_datapath_binding_by_key,
> > > sbrec_port_binding_by_key,
> > > - sbrec_mac_binding_by_lport_ip, mb);
> > > + sbrec_mac_binding_by_lport_ip, mb,
> > > + chassis);
> > > ovn_mac_binding_remove(mb, &put_mac_bindings);
> > > }
> > > }
> > > @@ -4552,7 +4570,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > const struct sbrec_port_binding *binding_rec,
> > > struct shash *nat_addresses,
> > > long long int garp_max_timeout,
> > > - bool garp_continuous)
> > > + bool garp_continuous,
> > > + const struct sbrec_chassis *chassis)
> > > {
> > > volatile struct garp_rarp_data *garp_rarp = NULL;
> > >
> > > @@ -4592,7 +4611,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > send_garp_locally(ovnsb_idl_txn,
> > > sbrec_mac_binding_by_lport_ip,
> > > local_datapaths, binding_rec,
> > laddrs->ea,
> > > - laddrs->ipv4_addrs[i].addr);
> > > + laddrs->ipv4_addrs[i].addr,
> > > + chassis);
> > >
> > > }
> > > free(name);
> > > @@ -4661,7 +4681,8 @@ send_garp_rarp_update(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > binding_rec->tunnel_key);
> > > if (ip) {
> > > send_garp_locally(ovnsb_idl_txn,
> > sbrec_mac_binding_by_lport_ip,
> > > - local_datapaths, binding_rec,
laddrs.ea,
> > ip);
> > > + local_datapaths, binding_rec,
laddrs.ea,
> > ip,
> > > + chassis);
> > > }
> > >
> > > destroy_lport_addresses(&laddrs);
> > > @@ -6080,7 +6101,8 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > send_garp_rarp_update(ovnsb_idl_txn,
> > > sbrec_mac_binding_by_lport_ip,
> > > local_datapaths, pb,
&nat_addresses,
> > > - garp_max_timeout, garp_continuous);
> > > + garp_max_timeout, garp_continuous,
> > > + chassis);
> > > }
> > > }
> > >
> > > @@ -6092,7 +6114,8 @@ send_garp_rarp_prepare(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> > > if (pb) {
> > > send_garp_rarp_update(ovnsb_idl_txn,
> > sbrec_mac_binding_by_lport_ip,
> > > local_datapaths, pb,
&nat_addresses,
> > > - garp_max_timeout, garp_continuous);
> > > + garp_max_timeout, garp_continuous,
> > > + chassis);
> > > }
> > > }
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index f3868068d..f51dbecb4 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -109,7 +109,7 @@ static const char *rbac_port_binding_update[] =
> > > "options"};
> > >
> > > static const char *rbac_mac_binding_auth[] =
> > > - {""};
> > > + {"chassis_name"};
> > > static const char *rbac_mac_binding_update[] =
> > > {"logical_port", "ip", "mac", "datapath", "timestamp"};
> > >
> > > diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
> > > index 72e230b75..9cf91c8f7 100644
> > > --- a/ovn-sb.ovsschema
> > > +++ b/ovn-sb.ovsschema
> > > @@ -1,7 +1,7 @@
> > > {
> > > "name": "OVN_Southbound",
> > > - "version": "20.30.0",
> > > - "cksum": "2972392849 31172",
> > > + "version": "20.31.0",
> > > + "cksum": "3395536250 31224",
> > > "tables": {
> > > "SB_Global": {
> > > "columns": {
> > > @@ -286,7 +286,8 @@
> > > "mac": {"type": "string"},
> > > "timestamp": {"type": {"key": "integer"}},
> > > "datapath": {"type": {"key": {"type": "uuid",
> > > - "refTable":
> > "Datapath_Binding"}}}},
> > > + "refTable":
> > "Datapath_Binding"}}},
> > > + "chassis_name": {"type": "string"}},
> > > "indexes": [["logical_port", "ip"]],
> > > "isRoot": true},
> > > "DHCP_Options": {
> > > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > > index e393f92b3..411074083 100644
> > > --- a/ovn-sb.xml
> > > +++ b/ovn-sb.xml
> > > @@ -3925,6 +3925,9 @@ tcp.flags = RST;
> > > <column name="datapath">
> > > The logical datapath to which the logical port belongs.
> > > </column>
> > > + <column name="chassis_name">
> > > + The name of the chassis that inserted this record.
> > > + </column>
> > > </table>
> > >
> > > <table name="DHCP_Options" title="DHCP Options supported by native
> > OVN DHCP">
> > > --
> > > 2.40.1
> > >
> > > _______________________________________________
> > > dev mailing list
> > > d...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> >
>
> --
>
> Ales Musil
>
> Senior Software Engineer - OVN Core
>
> Red Hat EMEA <https://www.redhat.com>
>
> amu...@redhat.com
> <https://red.ht/sig>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
