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
> ---
> 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
