Re: [ovs-dev] [PATCH ovn 1/4] rbac: MAC_Bindings can only be updated by the inserting chassis.

Thu, 25 Jan 2024 19:11:54 -0800 

On 1/22/24 09:35, Ales Musil wrote:




On Mon, Jan 22, 2024 at 9:09 AM Felix Huettner via dev 
<ovs-dev@openvswitch.org <mailto: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.



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?


All the above unfortunately applies also to FDB.

Thanks,
Ales



I spent some time today trying to fix the problem in your first 
paragraph: chassis may attempt to update timestamps on mac bindings they 
do not own. I tried to fix this by making the chassis name part of the 
lookup criteria for SB MAC bindings.



This turned out to be a problem. I needed to change the index we use for 
MAC binding lookups to use the logical port, IP address, and chassis 
name. However, if the chassis name column isn't present, then I need the 
index to behave like it currently does and use only the logical port and 
IP address. IDL indexes must be created prior to the first call to 
ovsdb_idl_run(). This means that if ovn-controller connects to an old SB 
DB that doesn't have the chassis name column in the MAC Binding table, 
then it can never create an index that includes the chassis name. Even 
if the SB DB were updated to have the chassis name, we couldn't look up 
MAC bindings properly unless we also restart ovn-controller so that we 
create the proper index.



That doesn't even touch on the issue you've brought up about old MAC 
bindings never aging out if the binding moves to a new chassis.



And finally, if I factor in Han's reply, I think the appropriate action 
at this time is to drop this patch from the series. I will upload a v2 
tomorrow that only has patches 2-4.




     > ---
     >  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 <mailto:d...@openvswitch.org>
     > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
    <https://mail.openvswitch.org/mailman/listinfo/ovs-dev>
    _______________________________________________
    dev mailing list
    d...@openvswitch.org <mailto:d...@openvswitch.org>
    https://mail.openvswitch.org/mailman/listinfo/ovs-dev
    <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 <mailto:amu...@redhat.com>

<https://red.ht/sig>



_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev























					Reply via email to