On 11/4/24 10:44, Ales Musil wrote:
> On Fri, Nov 1, 2024 at 2:29 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
>> SB.MAC_Binding doesn't store a strong reference to SB.Port_Binding.
>> Instead it stores the port binding 'logical_port' name. As the
>> mac-binding cache in ovn-controller stored entries indexed by
>> [datapath-key, port-key, ip] it meant that when processing
>> SB.MAC_Binding changes (including deletion) it had to first lookup the
>> corresponding SB.Port_Binding.
>>
>> If the logical port associated with the MAC binding is deleted in the
>> same transaction then looking it up in the IDL index fails (deleted IDL
>> records are not indexed). This leads to stale entries staying in the
>> mac cache and in consequence to potential use after free (of the SB
>> MAC_Binding record) or to crashes if the statctrl module tries to update
>> timestamps of MAC_Bindings that happened to have been removed in the
>> current iteration (but still linger in the cache).
>>
>> In practice we don't need the port-key we can simply use the port
>> binding UUID as unique index. However, the same struct mac_binding type
>> is used by pinctrl to temporarily efficiently store buffered packets for
>> yet to be resolved MAC bindings. In those cases there's no SB
>> MAC_Binding record yet so the logical ingress port tunnel_key is used as
>> index.
>>
>> The two ways of using the MAC binding maps are orthogonal, that is, we
>> will never use a single map to store both types of bindings.
>>
>> In order to avoid issues we now include the SB MAC_Binding UUID into the
>> key of the struct mac_binding_data.
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-906
>> Fixes: b57f60a617b4 ("controller: Add MAC cache I-P node")
>> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
>> ---
>>
>
> Hi Dumitru,
>
Hi Ales,
> thank you for the fix. There is just one small typo down below.
> We should probably think about a way of decoupling pinctrl and
> ovn-controller mac-cache in the future as it unfortunately doesn't
> work well together.
>
I agree, it would be nice to follow up and split the two mac-caches.
> controller/mac-cache.c | 77 ++++++++++++++++++-------
>> controller/mac-cache.h | 30 +++++++---
>> controller/ovn-controller.c | 67 ++++++++++------------
>> controller/pinctrl.c | 24 +++++---
>> tests/ovn.at | 110 ++++++++++++++++++++++++++++++++++++
>> 5 files changed, 236 insertions(+), 72 deletions(-)
>>
>> diff --git a/controller/mac-cache.c b/controller/mac-cache.c
>> index de45d7a6a5..d6ba9e20ee 100644
>> --- a/controller/mac-cache.c
>> +++ b/controller/mac-cache.c
>> @@ -142,21 +142,18 @@ mac_cache_thresholds_clear(struct mac_cache_data
>> *data)
>> }
>>
>> /* MAC binding. */
>> -struct mac_binding *
>> +void
>> mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
>> long long timestamp) {
>>
>> struct mac_binding *mb = mac_binding_find(map, &mb_data);
>> if (!mb) {
>> mb = xmalloc(sizeof *mb);
>> - mb->sbrec_mb = NULL;
>> hmap_insert(map, &mb->hmap_node, mac_binding_data_hash(&mb_data));
>> }
>>
>> mb->data = mb_data;
>> mb->timestamp = timestamp;
>> -
>> - return mb;
>> }
>>
>> void
>> @@ -181,24 +178,37 @@ mac_binding_find(const struct hmap *map,
>> }
>>
>> bool
>> -mac_binding_data_from_sbrec(struct mac_binding_data *data,
>> - const struct sbrec_mac_binding *mb,
>> - struct ovsdb_idl_index *sbrec_pb_by_name)
>> +mac_binding_data_parse(struct mac_binding_data *data,
>> + uint32_t dp_key, uint32_t port_key,
>> + const char *ip_str, const char *mac_str)
>> {
>> - const struct sbrec_port_binding *pb =
>> - lport_lookup_by_name(sbrec_pb_by_name, mb->logical_port);
>> + struct eth_addr mac;
>> + struct in6_addr ip;
>>
>> - if (!pb || !pb->datapath || !ip46_parse(mb->ip, &data->ip) ||
>> - !eth_addr_from_string(mb->mac, &data->mac)) {
>> + if (!ip46_parse(ip_str, &ip) || !eth_addr_from_string(mac_str, &mac))
>> {
>> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1);
>> - VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s, "
>> - "logical_port=%s", mb->ip, mb->mac,
>> mb->logical_port);
>> + VLOG_WARN_RL(&rl, "Couldn't parse MAC binding: ip=%s, mac=%s",
>> + ip_str, mac_str);
>> return false;
>> }
>>
>> - data->dp_key = mb->datapath->tunnel_key;
>> - data->port_key = pb->tunnel_key;
>> + mac_binding_data_init(data, dp_key, port_key, ip, mac);
>> + return true;
>> +}
>> +
>> +bool
>> +mac_binding_data_from_sbrec(struct mac_binding_data *data,
>> + const struct sbrec_mac_binding *mb)
>> +{
>> + /* This explicitly sets the port_key to 0 as port_binding tunnel_keys
>> + * can change. Instead use add the SB.MAC_Binding UUID as key; this
>> + * makes the mac_binding_data key unique. */
>> + if (!mac_binding_data_parse(data, mb->datapath->tunnel_key, 0,
>> + mb->ip, mb->mac)) {
>> + return false;
>> + }
>>
>> + data->sbrec = mb;
>> return true;
>> }
>>
>> @@ -211,6 +221,30 @@ mac_bindings_clear(struct hmap *map)
>> }
>> }
>>
>> +void
>> +mac_bindings_to_string(const struct hmap *map, struct ds *out_data)
>> +{
>> + struct mac_binding *mb;
>> + HMAP_FOR_EACH (mb, hmap_node, map) {
>> + char ip[INET6_ADDRSTRLEN];
>> +
>> + if (!ipv6_string_mapped(ip, &mb->data.ip)) {
>> + continue;
>> + }
>> + if (mb->data.sbrec) {
>> + ds_put_format(out_data, "SB UUID: " UUID_FMT ", ",
>> + UUID_ARGS(&mb->data.sbrec->header_.uuid));
>> + } else {
>> + ds_put_cstr(out_data, "SB UUID: <none>, ");
>> + }
>> + ds_put_format(out_data, "datapath-key: %"PRIu32", "
>> + "port-key: %"PRIu32", "
>> + "ip: %s, mac: " ETH_ADDR_FMT "\n",
>> + mb->data.dp_key, mb->data.port_key,
>> + ip, ETH_ADDR_ARGS(mb->data.mac));
>> + }
>> +}
>> +
>> bool
>> sb_mac_binding_updated(const struct sbrec_mac_binding *mb)
>> {
>> @@ -382,9 +416,9 @@ mac_binding_stats_run(struct ovs_list *stats_list,
>> uint64_t *req_delay,
>> * used on this chassis. Also make sure that we don't update the
>> * timestamp more than once during the dump period. */
>> if (stats->idle_age_ms < threshold->value &&
>> - (timewall_now - mb->sbrec_mb->timestamp) >=
>> + (timewall_now - mb->data.sbrec->timestamp) >=
>> threshold->dump_period) {
>> - sbrec_mac_binding_set_timestamp(mb->sbrec_mb, timewall_now);
>> + sbrec_mac_binding_set_timestamp(mb->data.sbrec, timewall_now);
>> }
>>
>> free(stats);
>> @@ -608,7 +642,9 @@ mac_cache_stats_destroy(struct ovs_list *stats_list)
>> static uint32_t
>> mac_binding_data_hash(const struct mac_binding_data *mb_data)
>> {
>> - uint32_t hash = 0;
>> + uint32_t hash = mb_data->sbrec
>> + ? uuid_hash(&mb_data->sbrec->header_.uuid)
>> + : 0;
>>
>> hash = hash_add(hash, mb_data->port_key);
>> hash = hash_add(hash, mb_data->dp_key);
>> @@ -621,9 +657,10 @@ static inline bool
>> mac_binding_data_equals(const struct mac_binding_data *a,
>> const struct mac_binding_data *b)
>> {
>> - return a->port_key == b->port_key &&
>> + return a->sbrec == b->sbrec &&
>> + a->port_key == b->port_key &&
>> a->dp_key == b->dp_key &&
>> - ipv6_addr_equals(&a->ip, &b->ip);
>> + ipv6_addr_equals(&a->ip, &b->ip);
>> }
>>
>> static uint32_t
>> diff --git a/controller/mac-cache.h b/controller/mac-cache.h
>> index b94e7a1cf2..df84ef072c 100644
>> --- a/controller/mac-cache.h
>> +++ b/controller/mac-cache.h
>> @@ -52,6 +52,7 @@ struct mac_cache_threshold {
>>
>> struct mac_binding_data {
>> /* Keys. */
>> + const struct sbrec_mac_binding * sbrec; /* Only the UUID is used as
>> key.*/
>>
>
> nit: Space after *.
>
Ah, true, I fixed it before applying the patch.
>
>> uint32_t port_key;
>> uint32_t dp_key;
>> struct in6_addr ip;
>> @@ -63,8 +64,6 @@ struct mac_binding {
>> struct hmap_node hmap_node;
>> /* Common data to identify MAC binding. */
>> struct mac_binding_data data;
>> - /* Reference to the SB MAC binding record (Might be NULL). */
>> - const struct sbrec_mac_binding *sbrec_mb;
>> /* User specified timestamp (in ms) */
>> long long timestamp;
>> };
>> @@ -128,20 +127,37 @@ void mac_cache_thresholds_sync(struct mac_cache_data
>> *data,
>> void mac_cache_thresholds_clear(struct mac_cache_data *data);
>>
>> /* MAC binding. */
>> -struct mac_binding *mac_binding_add(struct hmap *map,
>> - struct mac_binding_data mb_data,
>> - long long timestamp);
>> +
>> +static inline void
>> +mac_binding_data_init(struct mac_binding_data *data,
>> + uint32_t dp_key, uint32_t port_key,
>> + struct in6_addr ip, struct eth_addr mac)
>> +{
>> + *data = (struct mac_binding_data) {
>> + .sbrec = NULL,
>> + .dp_key = (dp_key),
>> + .port_key = (port_key),
>> + .ip = (ip),
>> + .mac = (mac),
>> + };
>> +}
>> +
>> +void mac_binding_add(struct hmap *map, struct mac_binding_data mb_data,
>> + long long timestamp);
>>
>> void mac_binding_remove(struct hmap *map, struct mac_binding *mb);
>>
>> struct mac_binding *mac_binding_find(const struct hmap *map,
>> const struct mac_binding_data
>> *mb_data);
>>
>> +bool mac_binding_data_parse(struct mac_binding_data *data,
>> + uint32_t dp_key, uint32_t port_key,
>> + const char *ip_str, const char *mac_str);
>> bool mac_binding_data_from_sbrec(struct mac_binding_data *data,
>> - const struct sbrec_mac_binding *mb,
>> - struct ovsdb_idl_index
>> *sbrec_pb_by_name);
>> + const struct sbrec_mac_binding *mb);
>>
>> void mac_bindings_clear(struct hmap *map);
>> +void mac_bindings_to_string(const struct hmap *map, struct ds *out_data);
>>
>> bool sb_mac_binding_updated(const struct sbrec_mac_binding *mb);
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index e87274121c..ab95938502 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -101,6 +101,7 @@ static unixctl_cb_func debug_status_execution;
>> static unixctl_cb_func debug_dump_local_bindings;
>> static unixctl_cb_func debug_dump_related_lports;
>> static unixctl_cb_func debug_dump_local_template_vars;
>> +static unixctl_cb_func debug_dump_local_mac_bindings;
>> static unixctl_cb_func debug_dump_lflow_conj_ids;
>> static unixctl_cb_func lflow_cache_flush_cmd;
>> static unixctl_cb_func lflow_cache_show_stats_cmd;
>> @@ -2932,26 +2933,22 @@ en_lb_data_cleanup(void *data)
>>
>> static void
>> mac_binding_add_sb(struct mac_cache_data *data,
>> - const struct sbrec_mac_binding *smb,
>> - struct ovsdb_idl_index *sbrec_pb_by_name)
>> + const struct sbrec_mac_binding *smb)
>> {
>> struct mac_binding_data mb_data;
>> - if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
>> + if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>> return;
>> }
>>
>> - struct mac_binding *mb = mac_binding_add(&data->mac_bindings,
>> mb_data, 0);
>> -
>> - mb->sbrec_mb = smb;
>> + mac_binding_add(&data->mac_bindings, mb_data, 0);
>> }
>>
>> static void
>> mac_binding_remove_sb(struct mac_cache_data *data,
>> - const struct sbrec_mac_binding *smb,
>> - struct ovsdb_idl_index *sbrec_pb_by_name)
>> + const struct sbrec_mac_binding *smb)
>> {
>> struct mac_binding_data mb_data;
>> - if (!mac_binding_data_from_sbrec(&mb_data, smb, sbrec_pb_by_name)) {
>> + if (!mac_binding_data_from_sbrec(&mb_data, smb)) {
>> return;
>> }
>>
>> @@ -2995,8 +2992,7 @@ fdb_remove_sb(struct mac_cache_data *data, const
>> struct sbrec_fdb *sfdb)
>> static void
>> mac_cache_mb_handle_for_datapath(struct mac_cache_data *data,
>> const struct sbrec_datapath_binding *dp,
>> - struct ovsdb_idl_index *sbrec_mb_by_dp,
>> - struct ovsdb_idl_index *sbrec_pb_by_name)
>> + struct ovsdb_idl_index *sbrec_mb_by_dp)
>> {
>> bool has_threshold = mac_cache_threshold_find(data, dp->tunnel_key);
>>
>> @@ -3007,9 +3003,9 @@ mac_cache_mb_handle_for_datapath(struct
>> mac_cache_data *data,
>> const struct sbrec_mac_binding *mb;
>> SBREC_MAC_BINDING_FOR_EACH_EQUAL (mb, mb_index_row, sbrec_mb_by_dp) {
>> if (has_threshold) {
>> - mac_binding_add_sb(data, mb, sbrec_pb_by_name);
>> + mac_binding_add_sb(data, mb);
>> } else {
>> - mac_binding_remove_sb(data, mb, sbrec_pb_by_name);
>> + mac_binding_remove_sb(data, mb);
>> }
>> }
>>
>> @@ -3064,10 +3060,6 @@ en_mac_cache_run(struct engine_node *node, void
>> *data)
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_mac_binding", node),
>> "datapath");
>> - struct ovsdb_idl_index *sbrec_pb_by_name =
>> - engine_ovsdb_node_get_index(
>> - engine_get_input("SB_port_binding", node),
>> - "name");
>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_fdb", node),
>> @@ -3086,7 +3078,7 @@ en_mac_cache_run(struct engine_node *node, void
>> *data)
>>
>> mac_cache_threshold_add(cache_data, sbrec_dp);
>> mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
>> - sbrec_mb_by_dp,
>> sbrec_pb_by_name);
>> + sbrec_mb_by_dp);
>> mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>> sbrec_fdb_by_dp_key);
>> }
>> @@ -3102,11 +3094,6 @@ mac_cache_sb_mac_binding_handler(struct engine_node
>> *node, void *data)
>> engine_get_input_data("runtime_data", node);
>> const struct sbrec_mac_binding_table *mb_table =
>> EN_OVSDB_GET(engine_get_input("SB_mac_binding", node));
>> - struct ovsdb_idl_index *sbrec_pb_by_name =
>> - engine_ovsdb_node_get_index(
>> - engine_get_input("SB_port_binding", node),
>> - "name");
>> -
>> size_t previous_size = hmap_count(&cache_data->mac_bindings);
>>
>> const struct sbrec_mac_binding *sbrec_mb;
>> @@ -3116,8 +3103,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node
>> *node, void *data)
>> }
>>
>> if (!sbrec_mac_binding_is_new(sbrec_mb)) {
>> - mac_binding_remove_sb(cache_data, sbrec_mb,
>> - sbrec_pb_by_name);
>> + mac_binding_remove_sb(cache_data, sbrec_mb);
>> }
>>
>> if (sbrec_mac_binding_is_deleted(sbrec_mb) ||
>> @@ -3128,7 +3114,7 @@ mac_cache_sb_mac_binding_handler(struct engine_node
>> *node, void *data)
>>
>> if (mac_cache_threshold_find(cache_data,
>> sbrec_mb->datapath->tunnel_key)) {
>> - mac_binding_add_sb(cache_data, sbrec_mb, sbrec_pb_by_name);
>> + mac_binding_add_sb(cache_data, sbrec_mb);
>> }
>> }
>>
>> @@ -3189,10 +3175,6 @@ mac_cache_runtime_data_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_mac_binding", node),
>> "datapath");
>> - struct ovsdb_idl_index *sbrec_pb_by_name =
>> - engine_ovsdb_node_get_index(
>> - engine_get_input("SB_port_binding", node),
>> - "name");
>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_fdb", node),
>> @@ -3217,7 +3199,7 @@ mac_cache_runtime_data_handler(struct engine_node
>> *node, void *data OVS_UNUSED)
>>
>> HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) {
>> mac_cache_mb_handle_for_datapath(cache_data, tdp->dp,
>> - sbrec_mb_by_dp,
>> sbrec_pb_by_name);
>> + sbrec_mb_by_dp);
>>
>> mac_cache_fdb_handle_for_datapath(cache_data, tdp->dp,
>> sbrec_fdb_by_dp_key);
>> @@ -3243,10 +3225,6 @@ mac_cache_sb_datapath_binding_handler(struct
>> engine_node *node, void *data)
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_mac_binding", node),
>> "datapath");
>> - struct ovsdb_idl_index *sbrec_pb_by_name =
>> - engine_ovsdb_node_get_index(
>> - engine_get_input("SB_port_binding", node),
>> - "name");
>> struct ovsdb_idl_index *sbrec_fdb_by_dp_key =
>> engine_ovsdb_node_get_index(
>> engine_get_input("SB_fdb", node),
>> @@ -3274,7 +3252,7 @@ mac_cache_sb_datapath_binding_handler(struct
>> engine_node *node, void *data)
>>
>> SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (sbrec_dp, dp_table) {
>> mac_cache_mb_handle_for_datapath(cache_data, sbrec_dp,
>> - sbrec_mb_by_dp,
>> sbrec_pb_by_name);
>> + sbrec_mb_by_dp);
>>
>> mac_cache_fdb_handle_for_datapath(cache_data, sbrec_dp,
>> sbrec_fdb_by_dp_key);
>> @@ -5399,6 +5377,10 @@ main(int argc, char *argv[])
>> debug_dump_local_template_vars,
>> &template_vars_data->local_templates);
>>
>> + unixctl_command_register("debug/dump-mac-bindings", "", 0, 0,
>> + debug_dump_local_mac_bindings,
>> + &mac_cache_data->mac_bindings);
>> +
>> unixctl_command_register("debug/ignore-startup-delay", "", 0, 0,
>> debug_ignore_startup_delay, NULL);
>>
>> @@ -6356,6 +6338,19 @@ debug_dump_local_template_vars(struct unixctl_conn
>> *conn, int argc OVS_UNUSED,
>> ds_destroy(&tv_str);
>> }
>>
>> +static void
>> +debug_dump_local_mac_bindings(struct unixctl_conn *conn, int argc
>> OVS_UNUSED,
>> + const char *argv[] OVS_UNUSED,
>> + void *mac_bindings)
>> +{
>> + struct ds mb_str = DS_EMPTY_INITIALIZER;
>> +
>> + ds_put_cstr(&mb_str, "Local MAC bindings:\n");
>> + mac_bindings_to_string(mac_bindings, &mb_str);
>> + unixctl_command_reply(conn, ds_cstr(&mb_str));
>> + ds_destroy(&mb_str);
>> +}
>> +
>> static void
>> debug_ignore_startup_delay(struct unixctl_conn *conn, int argc OVS_UNUSED,
>> const char *argv[] OVS_UNUSED, void *arg
>> OVS_UNUSED)
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index dfb3130aa2..ca880a1da6 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -1539,19 +1539,20 @@ pinctrl_handle_buffered_packets(const struct
>> ofputil_packet_in *pin,
>> OVS_REQUIRES(pinctrl_mutex)
>> {
>> const struct match *md = &pin->flow_metadata;
>> - struct mac_binding_data mb_data = (struct mac_binding_data) {
>> - .dp_key = ntohll(md->flow.metadata),
>> - .port_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
>> - .mac = eth_addr_zero,
>> - };
>> + struct mac_binding_data mb_data;
>> + struct in6_addr ip;
>>
>> if (is_arp) {
>> - mb_data.ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>> + ip = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>> } else {
>> ovs_be128 ip6 = hton128(flow_get_xxreg(&md->flow, 0));
>> - memcpy(&mb_data.ip, &ip6, sizeof mb_data.ip);
>> + memcpy(&ip, &ip6, sizeof ip);
>> }
>>
>> + mac_binding_data_init(&mb_data, ntohll(md->flow.metadata),
>> + md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0],
>> + ip, eth_addr_zero);
>> +
>> struct buffered_packets *bp =
>> buffered_packets_add(&buffered_packets_ctx,
>> mb_data);
>> if (!bp) {
>> @@ -4959,9 +4960,14 @@ run_buffered_binding(const struct
>> sbrec_mac_binding_table *mac_binding_table,
>> const struct sbrec_port_binding *pb = lport_lookup_by_name(
>> sbrec_port_binding_by_name, smb->logical_port);
>>
>> + if (!pb || !pb->datapath) {
>> + continue;
>> + }
>> +
>> struct mac_binding_data mb_data;
>> - if (!mac_binding_data_from_sbrec(&mb_data, smb,
>> - sbrec_port_binding_by_name)) {
>> +
>> + if (!mac_binding_data_parse(&mb_data, smb->datapath->tunnel_key,
>> + pb->tunnel_key, smb->ip, smb->mac)) {
>> continue;
>> }
>>
>> diff --git a/tests/ovn.at b/tests/ovn.at
>> index 10cd7a79b9..44d58ca160 100644
>> --- a/tests/ovn.at
>> +++ b/tests/ovn.at
>> @@ -35168,6 +35168,116 @@ OVN_CLEANUP([hv1], [hv2])
>> AT_CLEANUP
>> ])
>>
>> +OVN_FOR_EACH_NORTHD([
>> +AT_SETUP([MAC binding aging - port deletion])
>> +AT_SKIP_IF([test $HAVE_SCAPY = no])
>> +ovn_start
>> +
>> +net_add n1
>> +sim_add hv1
>> +as hv1
>> +check ovs-vsctl add-br br-phys
>> +ovn_attach n1 br-phys 192.168.0.1
>> +
>> +check ovn-nbctl \
>> + -- ls-add ls1 \
>> + -- ls-add ls2 \
>> + -- lr-add lr \
>> + -- set logical_router lr options:mac_binding_age_threshold=3600 \
>> + -- lrp-add lr lr-ls1 00:00:00:00:10:00 192.168.10.1/24 \
>> + -- lrp-add lr lr-ls2 00:00:00:00:20:00 192.168.20.1/24 \
>> + -- lsp-add ls1 ls1-lr \
>> + -- lsp-set-type ls1-lr router \
>> + -- lsp-set-addresses ls1-lr router \
>> + -- lsp-set-options ls1-lr router-port=lr-ls1 \
>> + -- lsp-add ls1 vif1 \
>> + -- lsp-set-addresses vif1 "00:00:00:00:10:10 192.168.10.10" \
>> + -- lsp-add ls2 ls2-lr \
>> + -- lsp-set-type ls2-lr router \
>> + -- lsp-set-addresses ls2-lr router \
>> + -- lsp-set-options ls2-lr router-port=lr-ls2 \
>> + -- lsp-add ls2 vif2 \
>> + -- lsp-set-addresses vif2 "00:00:00:00:20:10 192.168.20.10"
>> +
>> +check ovs-vsctl \
>> + -- add-port br-int vif1 \
>> + -- set interface vif1 external-ids:iface-id=vif1 \
>> + -- add-port br-int vif2 \
>> + -- set interface vif2 external-ids:iface-id=vif2
>> +
>> +OVN_POPULATE_ARP
>> +wait_for_ports_up
>> +check ovn-nbctl --wait=hv sync
>> +
>> +dnl Wait for pinctrl thread to be connected.
>> +OVS_WAIT_UNTIL([grep pinctrl hv1/ovn-controller.log | grep -q connected])
>> +
>> +send_garp() {
>> + hv=$1
>> + dev=$2
>> + mac=$3
>> + ip=$4
>> +
>> + packet=$(fmt_pkt "Ether(dst='ff:ff:ff:ff:ff:ff', src='${mac}')/ \
>> + ARP(op=2, hwsrc='${mac}', hwdst='${mac}', \
>> + psrc='${ip}', pdst='${ip}')")
>> + as $hv ovs-appctl netdev-dummy/receive $dev $packet
>> +}
>> +
>> +AS_BOX([Remove LRP with learnt MAC_Binding])
>> +dnl Populate MAC Binding entry.
>> +send_garp hv1 vif1 00:00:00:00:10:10 192.168.10.10
>> +wait_row_count mac_binding 1 ip="192.168.10.10" logical_port="lr-ls1"
>> +
>> +dnl Check local mac binding cache.
>> +lr_key=$(fetch_column datapath_binding tunnel_key external_ids:name=lr)
>> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls1)
>> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller
>> debug/dump-mac-bindings], [0], [dnl
>> +Local MAC bindings:
>> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.10.10,
>> mac: 00:00:00:00:10:10
>> +])
>> +
>> +dnl Remove router port. This deletes the mac binding in the same
>> transaction.
>> +check ovn-nbctl --wait=hv lrp-del lr-ls1
>> +
>> +dnl Check local mac binding cache - it should be empty now.
>> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings],
>> [0], [dnl
>> +Local MAC bindings:
>> +])
>> +
>> +AS_BOX([Change LRP tunnel-key with learnt MAC_Binding and remove LRP])
>> +dnl Re-add port.
>> +check ovn-nbctl \
>> + -- set logical_router_port lr-ls2 options:requested-tnl-key=42
>> +check ovn-nbctl --wait=hv sync
>> +check_column 42 Port_Binding tunnel_key logical_port=lr-ls2
>> +
>> +dnl Populate MAC Binding entry.
>> +send_garp hv1 vif2 00:00:00:00:20:10 192.168.20.10
>> +wait_row_count mac_binding 1 ip="192.168.20.10" logical_port="lr-ls2"
>> +
>> +dnl Check local mac binding cache.
>> +mb_uuid=$(fetch_column mac_binding _uuid logical_port=lr-ls2)
>> +OVS_WAIT_FOR_OUTPUT_UNQUOTED([as hv1 ovn-appctl -t ovn-controller
>> debug/dump-mac-bindings], [0], [dnl
>> +Local MAC bindings:
>> +SB UUID: $mb_uuid, datapath-key: $lr_key, port-key: 0, ip: 192.168.20.10,
>> mac: 00:00:00:00:20:10
>> +])
>> +
>> +dnl Change LRP tunnel key.
>> +check ovn-nbctl --wait=hv set logical_router_port lr-ls2
>> options:requested-tnl-key=43
>> +check_column 43 Port_Binding tunnel_key logical_port=lr-ls2
>> +
>> +dnl Remove router port. This deletes the mac binding in the same
>> transaction.
>> +check ovn-nbctl --wait=hv lrp-del lr-ls2
>> +dnl Check local mac binding cache - it should be empty now.
>> +AT_CHECK([as hv1 ovn-appctl -t ovn-controller debug/dump-mac-bindings],
>> [0], [dnl
>> +Local MAC bindings:
>> +])
>> +
>> +OVN_CLEANUP([hv1])
>> +AT_CLEANUP
>> +])
>> +
>> OVN_FOR_EACH_NORTHD([
>> AT_SETUP([router port type update and then remove])
>> ovn_start
>> --
>> 2.46.2
>>
>>
> Other than that the fix looks good.
>
> Acked-by: Ales Musil <amu...@redhat.com>
>
> Thanks,
> Ales
Thanks,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
