| Message ID | 20240119213331.454896-1-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/4] rbac: MAC_Bindings can only be updated by the inserting chassis. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Whoops, I forgot to add Reported-at: https://issues.redhat.com/browse/FDP-155 to this series. I can add this in v2. I'll wait to post a v2 until I get some feedback on this series. On 1/19/24 16:33, 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. > --- > 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">
Bleep bloop. Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.
I encountered some error that I wasn't expecting. See the details below.
checkpatch:
ERROR: Author Mark Michelson <mmichels@redhat.com> needs to sign off.
WARNING: Line is 80 characters long (recommended limit is 79)
#225 FILE: ovn-sb.ovsschema:289:
"refTable": "Datapath_Binding"}}},
Lines checked: 247, Warnings: 1, Errors: 1
Please check this out. If you feel there has been an error, please email aconole@redhat.com
Thanks,
0-day Robot
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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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. 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 > > --- > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > >
On Mon, Jan 22, 2024 at 6:36 AM Ales Musil <amusil@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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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 > > dev@openvswitch.org <mailto:dev@openvswitch.org> > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > <https://mail.openvswitch.org/mailman/listinfo/ovs-dev> > _______________________________________________ > dev mailing list > dev@openvswitch.org <mailto:dev@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> > > amusil@redhat.com <mailto:amusil@redhat.com> > > <https://red.ht/sig> >
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">