[ovs-dev,v1] Add a northbound interface to program MAC_Binding table

From: Karthik Chandrashekar <karthik.c@nutanix.com>

Add a new NB MAC_Binding table. This allows programming of
static mac_bindings for a logical_router. OVN northd is
responsible for propagating these static entries to the
existing SB MAC_Binding table.

Signed-off-by: Karthik Chandrashekar <karthik.c@nutanix.com>
---
 lib/automake.mk         |   2 +
 lib/mac-binding-index.c |  43 ++++++++
 lib/mac-binding-index.h |  29 ++++++
 manpages.mk             |   1 -
 northd/ovn-northd.c     |  42 ++++++++
 northd/ovn_northd.dl    |   9 ++
 ovn-nb.ovsschema        |  17 ++-
 ovn-nb.xml              |  28 +++++
 tests/ovn-nbctl.at      |  64 ++++++++++++
 tests/ovn.at            |  26 +++++
 utilities/ovn-nbctl.c   | 222 +++++++++++++++++++++++++++++++++++++++-
 11 files changed, 478 insertions(+), 5 deletions(-)
 create mode 100644 lib/mac-binding-index.c
 create mode 100644 lib/mac-binding-index.h

References:  <20210825185229.195890-1-svc.eng.git-mail@nutanix.com>
 

Bleep bloop.  Greetings svc.eng.git-mail, 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:
WARNING: Line is 89 characters long (recommended limit is 79)
#181 FILE: northd/ovn-northd.c:8225:
                    sbrec_mac_binding_set_logical_port(mb, nb_mac_binding->logical_port);

WARNING: Line lacks whitespace around operator
#444 FILE: utilities/ovn-nbctl.c:370:
  lr-mac-binding-add ROUTER LOGICAL_PORT IP MAC\n\

WARNING: Line lacks whitespace around operator
#446 FILE: utilities/ovn-nbctl.c:372:
  lr-mac-binding-del ROUTER LOGICAL_PORT [IP]\n\

WARNING: Line lacks whitespace around operator
#449 FILE: utilities/ovn-nbctl.c:375:
  lr-mac-binding-list ROUTER        print MAC_Bindings for ROUTER\n\

WARNING: Line is 83 characters long (recommended limit is 79)
#586 FILE: utilities/ovn-nbctl.c:5723:
                nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);

WARNING: Line is 80 characters long (recommended limit is 79)
#619 FILE: utilities/ovn-nbctl.c:5756:
        ctl_error(ctx, "no matching MAC_Binding with the port (%s) and ip (%s)",

Lines checked: 691, Warnings: 6, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

Hi Karthik,

One thing that's not clear to me is why logical routers need to have 
references to the northbound MAC_Bindings. The MAC_Bindings contain a 
logical port. This means that the only valid configuration would be if 
the router that contains that logical port references the MAC_Binding. 
Further, since logical ports must all have unique names, it means it is 
not possible to have a configuration where multiple logical routers can 
have the same-named logical port on them. Therefore, it's never a valid 
configuration to have multiple logical routers referring to the same 
northbound MAC_Binding.

If I create a MAC_Binding with the properties:

ip: 10.0.0.1
mac: 00:00:00:00:00:01
logical_port: lrp1

Then ovn-northd can take that "lrp1" and find its owning logical router, 
and add/modify the southbound MAC_Binding.

Changing this would make it easier to avoid misconfigurations in the 
database, and it would simplify the code, too.

Am I missing something?

If I'm not, then the follow-up question is: why are northbound 
MAC_Bindings useful? Couldn't you just add the MAC_Bindings directly to 
the Southbound Database instead?

On 8/25/21 2:52 PM, svc.eng.git-mail@nutanix.com wrote:
> From: Karthik Chandrashekar <karthik.c@nutanix.com>
> 
> Add a new NB MAC_Binding table. This allows programming of
> static mac_bindings for a logical_router. OVN northd is
> responsible for propagating these static entries to the
> existing SB MAC_Binding table.
> 
> Signed-off-by: Karthik Chandrashekar <karthik.c@nutanix.com>
> ---
>   lib/automake.mk         |   2 +
>   lib/mac-binding-index.c |  43 ++++++++
>   lib/mac-binding-index.h |  29 ++++++
>   manpages.mk             |   1 -
>   northd/ovn-northd.c     |  42 ++++++++
>   northd/ovn_northd.dl    |   9 ++
>   ovn-nb.ovsschema        |  17 ++-
>   ovn-nb.xml              |  28 +++++
>   tests/ovn-nbctl.at      |  64 ++++++++++++
>   tests/ovn.at            |  26 +++++
>   utilities/ovn-nbctl.c   | 222 +++++++++++++++++++++++++++++++++++++++-
>   11 files changed, 478 insertions(+), 5 deletions(-)
>   create mode 100644 lib/mac-binding-index.c
>   create mode 100644 lib/mac-binding-index.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index ddfe33948..4c7974ee2 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -20,6 +20,8 @@ lib_libovn_la_SOURCES = \
>   	lib/ovn-parallel-hmap.c \
>   	lib/ip-mcast-index.c \
>   	lib/ip-mcast-index.h \
> +	lib/mac-binding-index.c \
> +	lib/mac-binding-index.h \
>   	lib/mcast-group-index.c \
>   	lib/mcast-group-index.h \
>   	lib/lex.c \
> diff --git a/lib/mac-binding-index.c b/lib/mac-binding-index.c
> new file mode 100644
> index 000000000..1d34dfbdc
> --- /dev/null
> +++ b/lib/mac-binding-index.c
> @@ -0,0 +1,43 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "lib/mac-binding-index.h"
> +#include "lib/ovn-sb-idl.h"
> +
> +struct ovsdb_idl_index *
> +mac_binding_index_create(struct ovsdb_idl *idl)
> +{
> +    return ovsdb_idl_index_create2(idl,
> +                                   &sbrec_mac_binding_col_logical_port,
> +                                   &sbrec_mac_binding_col_ip);
> +}
> +
> +const struct sbrec_mac_binding *
> +mac_binding_lookup(struct ovsdb_idl_index *mac_binding_index,
> +                   const char *logical_port, const char *ip)
> +{
> +    struct sbrec_mac_binding *target = sbrec_mac_binding_index_init_row(
> +        mac_binding_index);
> +    sbrec_mac_binding_index_set_logical_port(target, logical_port);
> +    sbrec_mac_binding_index_set_ip(target, ip);
> +
> +    struct sbrec_mac_binding *mac_binding =
> +        sbrec_mac_binding_index_find(mac_binding_index, target);
> +    sbrec_mac_binding_index_destroy_row(target);
> +
> +    return mac_binding;
> +}
> diff --git a/lib/mac-binding-index.h b/lib/mac-binding-index.h
> new file mode 100644
> index 000000000..d7434f688
> --- /dev/null
> +++ b/lib/mac-binding-index.h
> @@ -0,0 +1,29 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef OVN_MAC_BINDING_INDEX_H
> +#define OVN_MAC_BINDING_INDEX_H 1
> +
> +struct ovsdb_idl;
> +
> +struct sbrec_datapath_binding;
> +
> +struct ovsdb_idl_index *mac_binding_index_create(struct ovsdb_idl *);
> +const struct sbrec_mac_binding *mac_binding_lookup(
> +    struct ovsdb_idl_index *mac_binding_index,
> +    const char *logical_port,
> +    const char *ip);
> +
> +#endif /* lib/mac-binding-index.h */
> diff --git a/manpages.mk b/manpages.mk
> index 3334b38f9..9f7a0ced3 100644
> --- a/manpages.mk
> +++ b/manpages.mk
> @@ -9,4 +9,3 @@ utilities/ovn-detrace.1.in:
>   lib/common-syn.man:
>   lib/common.man:
>   lib/ovs.tmac:
> -
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index af413aba4..6113b1e1c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -30,6 +30,7 @@
>   #include "openvswitch/hmap.h"
>   #include "openvswitch/json.h"
>   #include "ovn/lex.h"
> +#include "lib/mac-binding-index.h"
>   #include "lib/chassis-index.h"
>   #include "lib/ip-mcast-index.h"
>   #include "lib/copp.h"
> @@ -79,6 +80,7 @@ struct northd_context {
>       struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
>       struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>       struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip;
>   };
>   
>   struct northd_state {
> @@ -8197,6 +8199,41 @@ build_bfd_table(struct northd_context *ctx, struct hmap *bfd_connections,
>       bitmap_free(bfd_src_ports);
>   }
>   
> +static void
> +build_mac_binding_table(struct northd_context *ctx, struct hmap *datapaths)
> +{
> +    const struct nbrec_logical_router *nbr;
> +    NBREC_LOGICAL_ROUTER_FOR_EACH (nbr, ctx->ovnnb_idl) {
> +        if (!lrouter_is_enabled(nbr)) {
> +            continue;
> +        }
> +
> +        struct ovn_datapath *od = ovn_datapath_find(datapaths,
> +                                                    &nbr->header_.uuid);
> +
> +        if (od && od->sb) {
> +            for (int i = 0; i < od->nbr->n_mac_bindings; i++) {
> +                const struct nbrec_mac_binding *nb_mac_binding;
> +                nb_mac_binding = od->nbr->mac_bindings[i];
> +
> +                const struct sbrec_mac_binding *mb =
> +                    mac_binding_lookup(ctx->sbrec_mac_binding_by_lport_ip,
> +                                       nb_mac_binding->logical_port,
> +                                       nb_mac_binding->ip);
> +                if (!mb) {
> +                    mb = sbrec_mac_binding_insert(ctx->ovnsb_txn);
> +                    sbrec_mac_binding_set_logical_port(mb, nb_mac_binding->logical_port);
> +                    sbrec_mac_binding_set_ip(mb, nb_mac_binding->ip);
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                    sbrec_mac_binding_set_datapath(mb, od->sb);
> +                } else {
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                }
> +            }
> +        }
> +    }
> +}
> +
>   /* Returns a string of the IP address of the router port 'op' that
>    * overlaps with 'ip_s".  If one is not found, returns NULL.
>    *
> @@ -14121,6 +14158,7 @@ ovnnb_db_run(struct northd_context *ctx,
>       build_mcast_groups(ctx, datapaths, ports, &mcast_groups, &igmp_groups);
>       build_meter_groups(ctx, &meter_groups);
>       build_bfd_table(ctx, &bfd_connections, ports);
> +    build_mac_binding_table(ctx, datapaths);
>       build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
>                    &igmp_groups, &meter_groups, &lbs, &bfd_connections,
>                    ovn_internal_version_changed);
> @@ -15272,6 +15310,9 @@ main(int argc, char *argv[])
>       struct ovsdb_idl_index *sbrec_ip_mcast_by_dp
>           = ip_mcast_index_create(ovnsb_idl_loop.idl);
>   
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip
> +        = mac_binding_index_create(ovnsb_idl_loop.idl);
> +
>       unixctl_command_register("sb-connection-status", "", 0, 0,
>                                ovn_conn_show, ovnsb_idl_loop.idl);
>   
> @@ -15317,6 +15358,7 @@ main(int argc, char *argv[])
>                   .sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
>                   .sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
>                   .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> +                .sbrec_mac_binding_by_lport_ip = sbrec_mac_binding_by_lport_ip,
>               };
>   
>               if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index dcfa1fb5d..e1b3b516c 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1010,6 +1010,15 @@ sb::Out_MAC_Binding (._uuid        = mb._uuid,
>       sb::Out_Port_Binding(.logical_port = mb.logical_port),
>       sb::Out_Datapath_Binding(._uuid = mb.datapath).
>   
> +sb::Out_MAC_Binding (._uuid        = hash,
> +                    .logical_port = nb.logical_port,
> +                    .ip           = nb.ip,
> +                    .mac          = nb.mac,
> +                    .datapath     = router._uuid) :-
> +    nb in nb::MAC_Binding(),
> +    rp in &RouterPort(.router = router, .json_name = json_string_escape(nb.logical_port)),
> +    var hash = hash128((nb.logical_port, nb.ip)).
> +
>   /*
>    * DHCP options: fixed table
>    */
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 2ac8ef3ea..734d9649e 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
> -    "version": "5.32.1",
> -    "cksum": "2805328215 29734",
> +    "version": "5.33.0",
> +    "cksum": "3031667641 30300",
>       "tables": {
>           "NB_Global": {
>               "columns": {
> @@ -325,6 +325,12 @@
>                                        "refType": "strong"},
>                                "min": 0,
>                                "max": "unlimited"}},
> +                "mac_bindings": {
> +                    "type": {"key": {"type": "uuid",
> +                                     "refTable": "MAC_Binding",
> +                                     "refType": "strong"},
> +                             "min": 0,
> +                             "max": "unlimited"}},
>                   "enabled": {"type": {"key": "boolean", "min": 0, "max": 1}},
>                   "nat": {"type": {"key": {"type": "uuid",
>                                            "refTable": "NAT",
> @@ -575,5 +581,12 @@
>                       "type": {"key": "string", "value": "string",
>                                "min": 0, "max": "unlimited"}}},
>               "indexes": [["logical_port", "dst_ip"]],
> +            "isRoot": true},
> +        "MAC_Binding": {
> +            "columns": {
> +                "logical_port": {"type": "string"},
> +                "ip": {"type": "string"},
> +                "mac": {"type": "string"}},
> +            "indexes": [["logical_port", "ip"]],
>               "isRoot": true}}
>       }
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 390cc5a44..34252581a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2043,6 +2043,10 @@
>         Zero or more routing policies for the router.
>       </column>
>   
> +    <column name="mac_bindings">
> +      Zero or more statically configured mac_bindings for the router.
> +    </column>
> +
>       <column name="enabled">
>         This column is used to administratively set router state.  If this column
>         is empty or is set to <code>true</code>, the router is enabled.  If this
> @@ -4103,4 +4107,28 @@
>         </column>
>       </group>
>     </table>
> +
> +  <table name="MAC_Binding">
> +    <p>
> +      Each record represents a MAC Binding entry.
> +    </p>
> +
> +    <group title="Configuration">
> +      <p>
> +        <code>ovn-northd</code> reads configuration from these columns.
> +      </p>
> +
> +      <column name="logical_port">
> +        The logical port on which the binding was discovered.
> +      </column>
> +
> +      <column name="ip">
> +        The bound IP address.
> +      </column>
> +
> +      <column name="mac">
> +        The Ethernet address to which the IP is bound.
> +      </column>
> +    </group>
> +  </table>
>   </database>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 9b80ae410..7f6761abd 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2057,6 +2057,70 @@ AT_CHECK([ovn-nbctl list forwarding_group], [0], [])
>   
>   dnl ---------------------------------------------------------------------
>   
> +OVN_NBCTL_TEST([ovn_nbctl_mac_binding], [lr mac_binding], [
> +
> +AT_CHECK([ovn-nbctl lr-add lr0])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 foo 00:00:44:55:66:88], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.200 foo], [1], [],
> +  [ovn-nbctl: invalid mac address foo.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77], [1], [],
> +  [ovn-nbctl: lr0-p0, 172.16.0.11: a MAC_Binding with this logical_port and ip already exists
> +])
> +
> +AT_CHECK([ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p0                   10.0.0.10                00:00:33:44:55:66
> +lr0-p0                   172.16.0.11              00:00:44:55:66:77
> +lr0-p0                   192.168.10.10            00:00:11:22:33:44
> +lr0-p0                   192.168.10.100           00:00:22:33:44:55
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0 foo], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.100], [1], [],
> +  [ovn-nbctl: no matching MAC_Binding with the port (lr0-p1) and ip (10.0.0.100)
> +])
> +
> +AT_CHECK([ovn-nbctl --if-exists lr-mac-binding-del lr0 lr0-p1 10.0.0.100])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.10])
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +])
> +
> +dnl ---------------------------------------------------------------------
> +
>   OVN_NBCTL_TEST([ovn_nbctl_negative], [basic negative tests], [
>   AT_CHECK([ovn-nbctl --id=@ls create logical_switch name=foo -- \
>             set logical_switch foo1 name=bar],
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4957a1063..3a719cb06 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -28110,3 +28110,29 @@ grep lr0-sw1], [1], [])
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([LR NB MAC_Binding table])
> +ovn_start
> +
> +# Create logical routers
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24
> +ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55
> +
> +wait_row_count nb:MAC_Binding 2 logical_port=lr0-p0
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.10 mac="00\:00\:11\:22\:33\:44"
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:44\:55"
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66
> +wait_row_count nb:MAC_Binding 1 logical_port=lr0-p1
> +wait_row_count MAC_Binding 1 logical_port=lr0-p1 ip=10.0.0.10 mac="00\:00\:33\:44\:55\:66"
> +
> +ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:55:66
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> +
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 10217dcd5..b0356d3aa 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -366,6 +366,15 @@ Policy commands:\n\
>                               remove policies from ROUTER\n\
>     lr-policy-list ROUTER     print policies for ROUTER\n\
>   \n\
> +MAC_Binding commands:\n\
> +  lr-mac-binding-add ROUTER LOGICAL_PORT IP MAC\n\
> +                                    add a MAC_Binding entry to ROUTER\n\
> +  lr-mac-binding-del ROUTER LOGICAL_PORT [IP]\n\
> +                                    remove MAC_Binding entry from ROUTER and\n\
> +                                    delete the MAC_Binding row\n\
> +  lr-mac-binding-list ROUTER        print MAC_Bindings for ROUTER\n\
> +\n\n",program_name, program_name);
> +    printf("\
>   NAT commands:\n\
>     [--stateless]\n\
>     [--portrange]\n\
> @@ -408,8 +417,7 @@ Connection commands:\n\
>     del-connection             delete the connections\n\
>     [--inactivity-probe=MSECS]\n\
>     set-connection TARGET...   set the list of connections to TARGET...\n\
> -\n\n",program_name, program_name);
> -    printf("\
> +\n\
>   SSL commands:\n\
>     get-ssl                     print the SSL configuration\n\
>     del-ssl                     delete the SSL configuration\n\
> @@ -5595,6 +5603,205 @@ nbctl_lrp_get_redirect_type(struct ctl_context *ctx)
>                     !redirect_type ? "overlay": redirect_type);
>   }
>   
> +static void
> +nbctl_pre_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    const char *logical_port = ctx->argv[2];
> +    const char *ip = ctx->argv[3];
> +    const char *mac = ctx->argv[4];
> +    char *new_ip = NULL;
> +
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const struct nbrec_logical_router_port *lrp;
> +    error = lrp_by_name_or_uuid(ctx, logical_port, true, &lrp);
> +    if (error) {
> +        ctx->error = error;
> +        goto cleanup;
> +    }
> +
> +    new_ip = normalize_addr_str(ip);
> +    if (!new_ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ip);
> +        return;
> +    }
> +
> +    struct eth_addr ea;
> +    if (!eth_addr_from_string(mac, &ea)) {
> +        ctl_error(ctx, "invalid mac address %s.", mac);
> +        goto cleanup;
> +    }
> +
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +
> +        char *old_ip;
> +        bool should_return = false;
> +        old_ip = normalize_addr_str(mac_binding->ip);
> +
> +        if (!strcmp(mac_binding->logical_port, logical_port)) {
> +            if (!strcmp(old_ip, new_ip)) {
> +                if (may_exist) {
> +                    nbrec_mac_binding_verify_mac(mac_binding);
> +                    nbrec_mac_binding_set_mac(mac_binding, mac);
> +                    should_return = true;
> +                } else {
> +                    ctl_error(ctx, "%s, %s: a MAC_Binding with this "
> +                              "logical_port and ip already "
> +                              "exists", logical_port, new_ip);
> +                    should_return = true;
> +                }
> +            }
> +        }
> +        free(old_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Create MAC_Binding entry */
> +    struct nbrec_mac_binding *mac_binding = nbrec_mac_binding_insert(ctx->txn);
> +    nbrec_mac_binding_set_logical_port(mac_binding, logical_port);
> +    nbrec_mac_binding_set_ip(mac_binding, new_ip);
> +    nbrec_mac_binding_set_mac(mac_binding, mac);
> +
> +    /* Insert MAC_Binding entry into logical router */
> +    nbrec_logical_router_update_mac_bindings_addvalue(lr, mac_binding);
> +
> +cleanup:
> +    free(new_ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const char *logical_port = ctx->argv[2];
> +    if (ctx->argc == 3) {
> +        /* Deletes all NB MAC_binding entries on the given logical port. */
> +        for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +            if (!strcmp(logical_port, lr->mac_bindings[i]->logical_port)) {
> +                struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +                nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +                nbrec_mac_binding_delete(mac_binding);
> +            }
> +        }
> +        return;
> +    }
> +
> +    char *ip = normalize_addr_str(ctx->argv[3]);
> +    if (!ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ctx->argv[3]);
> +        return;
> +    }
> +
> +    /* Remove the matching MAC_Binding. */
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        bool should_return = false;
> +        char *mac_binding_ip = normalize_addr_str(mac_binding->ip);
> +        if (!mac_binding_ip) {
> +            continue;
> +        }
> +        if (!strcmp(ip, mac_binding_ip)) {
> +            nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +            nbrec_mac_binding_delete(mac_binding);
> +            should_return = true;
> +        }
> +        free(mac_binding_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (must_exist) {
> +        ctl_error(ctx, "no matching MAC_Binding with the port (%s) and ip (%s)",
> +                  logical_port, ip);
> +    }
> +
> +cleanup:
> +    free(ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr;
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    struct smap lr_mac_bindings = SMAP_INITIALIZER(&lr_mac_bindings);
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        char *key = xasprintf("%-25s%-25s", mac_binding->logical_port,
> +                              mac_binding->ip);
> +        smap_add_format(&lr_mac_bindings, key, "%s", mac_binding->mac);
> +        free(key);
> +    }
> +
> +    const struct smap_node **nodes = smap_sort(&lr_mac_bindings);
> +    if (nodes) {
> +        ds_put_format(&ctx->output, "%-25s%-25s%s\n",
> +                      "LOGICAL_PORT", "IP", "MAC");
> +        for (size_t i = 0; i < smap_count(&lr_mac_bindings); i++) {
> +            const struct smap_node *node = nodes[i];
> +            ds_put_format(&ctx->output, "%-25s%s\n", node->key, node->value);
> +        }
> +    }
> +}
> +
>   static const struct nbrec_forwarding_group *
>   fwd_group_by_name_or_uuid(struct ctl_context *ctx, const char *id)
>   {
> @@ -6958,6 +7165,17 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>       { "lr-policy-list", 1, 1, "ROUTER", nbctl_pre_lr_policy_list,
>         nbctl_lr_policy_list, NULL, "", RO },
>   
> +    /* MAC_Binding commands */
> +    { "lr-mac-binding-add", 4, 4,
> +      "ROUTER LOGICAL_PORT IP MAC",
> +      nbctl_pre_lr_mac_binding_add, nbctl_lr_mac_binding_add, NULL,
> +      "--may-exist", RW },
> +    { "lr-mac-binding-del", 2, 3, "ROUTER LOGICAL_PORT [IP]",
> +      nbctl_pre_lr_mac_binding_del, nbctl_lr_mac_binding_del,
> +      NULL, "--if-exists", RW },
> +    { "lr-mac-binding-list", 1, 1, "ROUTER", nbctl_pre_lr_mac_binding_list,
> +      nbctl_lr_mac_binding_list, NULL, "", RO },
> +
>       /* NAT commands. */
>       { "lr-nat-add", 4, 7,
>         "ROUTER TYPE EXTERNAL_IP LOGICAL_IP"
>

Hi Mark,

Adding an NB API to configure Static MAC_binding entries is to provide a persistent configuration.
What happens if northd re-creates datapath_binding for a particular logical_router? Will this flush out the corresponding SB MAC_Binding entries?

Secondly, I don’t think we have a MAC aging mechanism today. If we ever plan to add that; having a persistent configuration coming from NB could be helpful.

Thanks,
Karthik

From: Mark Michelson <mmichels@redhat.com>
Date: Friday, August 27, 2021 at 12:34 PM
To: svc.eng.git-mail <svc.eng.git-mail@nutanix.com>, ovs-dev@openvswitch.org <ovs-dev@openvswitch.org>
Cc: Karthik Chandrashekar <karthik.c@nutanix.com>
Subject: Re: [ovs-dev] [PATCH ovn v1] Add a northbound interface to program MAC_Binding table
Hi Karthik,

One thing that's not clear to me is why logical routers need to have
references to the northbound MAC_Bindings. The MAC_Bindings contain a
logical port. This means that the only valid configuration would be if
the router that contains that logical port references the MAC_Binding.
Further, since logical ports must all have unique names, it means it is
not possible to have a configuration where multiple logical routers can
have the same-named logical port on them. Therefore, it's never a valid
configuration to have multiple logical routers referring to the same
northbound MAC_Binding.

If I create a MAC_Binding with the properties:

ip: 10.0.0.1
mac: 00:00:00:00:00:01
logical_port: lrp1

Then ovn-northd can take that "lrp1" and find its owning logical router,
and add/modify the southbound MAC_Binding.

Changing this would make it easier to avoid misconfigurations in the
database, and it would simplify the code, too.

Am I missing something?

If I'm not, then the follow-up question is: why are northbound
MAC_Bindings useful? Couldn't you just add the MAC_Bindings directly to
the Southbound Database instead?

On 8/25/21 2:52 PM, svc.eng.git-mail@nutanix.com wrote:
> From: Karthik Chandrashekar <karthik.c@nutanix.com>
>
> Add a new NB MAC_Binding table. This allows programming of
> static mac_bindings for a logical_router. OVN northd is
> responsible for propagating these static entries to the
> existing SB MAC_Binding table.
>
> Signed-off-by: Karthik Chandrashekar <karthik.c@nutanix.com>
> ---
>   lib/automake.mk         |   2 +
>   lib/mac-binding-index.c |  43 ++++++++
>   lib/mac-binding-index.h |  29 ++++++
>   manpages.mk             |   1 -
>   northd/ovn-northd.c     |  42 ++++++++
>   northd/ovn_northd.dl    |   9 ++
>   ovn-nb.ovsschema        |  17 ++-
>   ovn-nb.xml              |  28 +++++
>   tests/ovn-nbctl.at      |  64 ++++++++++++
>   tests/ovn.at            |  26 +++++
>   utilities/ovn-nbctl.c   | 222 +++++++++++++++++++++++++++++++++++++++-
>   11 files changed, 478 insertions(+), 5 deletions(-)
>   create mode 100644 lib/mac-binding-index.c
>   create mode 100644 lib/mac-binding-index.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index ddfe33948..4c7974ee2 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -20,6 +20,8 @@ lib_libovn_la_SOURCES = \
>        lib/ovn-parallel-hmap.c \
>        lib/ip-mcast-index.c \
>        lib/ip-mcast-index.h \
> +     lib/mac-binding-index.c \
> +     lib/mac-binding-index.h \
>        lib/mcast-group-index.c \
>        lib/mcast-group-index.h \
>        lib/lex.c \
> diff --git a/lib/mac-binding-index.c b/lib/mac-binding-index.c
> new file mode 100644
> index 000000000..1d34dfbdc
> --- /dev/null
> +++ b/lib/mac-binding-index.c
> @@ -0,0 +1,43 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=Rho0v37TamJ-qOZDDWo0xR-FqSZrgKABSsahe4CDK6k&m=VNuQHFSHMsMuiPKSHMSU__lk5tUES9LUhnlIkfpFF-M&s=bjaR9j-XFXLXUXpIP9moPGFazEwNJ_g0J2t5es39TUs&e=
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "lib/mac-binding-index.h"
> +#include "lib/ovn-sb-idl.h"
> +
> +struct ovsdb_idl_index *
> +mac_binding_index_create(struct ovsdb_idl *idl)
> +{
> +    return ovsdb_idl_index_create2(idl,
> +                                   &sbrec_mac_binding_col_logical_port,
> +                                   &sbrec_mac_binding_col_ip);
> +}
> +
> +const struct sbrec_mac_binding *
> +mac_binding_lookup(struct ovsdb_idl_index *mac_binding_index,
> +                   const char *logical_port, const char *ip)
> +{
> +    struct sbrec_mac_binding *target = sbrec_mac_binding_index_init_row(
> +        mac_binding_index);
> +    sbrec_mac_binding_index_set_logical_port(target, logical_port);
> +    sbrec_mac_binding_index_set_ip(target, ip);
> +
> +    struct sbrec_mac_binding *mac_binding =
> +        sbrec_mac_binding_index_find(mac_binding_index, target);
> +    sbrec_mac_binding_index_destroy_row(target);
> +
> +    return mac_binding;
> +}
> diff --git a/lib/mac-binding-index.h b/lib/mac-binding-index.h
> new file mode 100644
> index 000000000..d7434f688
> --- /dev/null
> +++ b/lib/mac-binding-index.h
> @@ -0,0 +1,29 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     https://urldefense.proofpoint.com/v2/url?u=http-3A__www.apache.org_licenses_LICENSE-2D2.0&d=DwICaQ&c=s883GpUCOChKOHiocYtGcg&r=Rho0v37TamJ-qOZDDWo0xR-FqSZrgKABSsahe4CDK6k&m=VNuQHFSHMsMuiPKSHMSU__lk5tUES9LUhnlIkfpFF-M&s=bjaR9j-XFXLXUXpIP9moPGFazEwNJ_g0J2t5es39TUs&e=
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef OVN_MAC_BINDING_INDEX_H
> +#define OVN_MAC_BINDING_INDEX_H 1
> +
> +struct ovsdb_idl;
> +
> +struct sbrec_datapath_binding;
> +
> +struct ovsdb_idl_index *mac_binding_index_create(struct ovsdb_idl *);
> +const struct sbrec_mac_binding *mac_binding_lookup(
> +    struct ovsdb_idl_index *mac_binding_index,
> +    const char *logical_port,
> +    const char *ip);
> +
> +#endif /* lib/mac-binding-index.h */
> diff --git a/manpages.mk b/manpages.mk
> index 3334b38f9..9f7a0ced3 100644
> --- a/manpages.mk
> +++ b/manpages.mk
> @@ -9,4 +9,3 @@ utilities/ovn-detrace.1.in:
>   lib/common-syn.man:
>   lib/common.man:
>   lib/ovs.tmac:
> -
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index af413aba4..6113b1e1c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -30,6 +30,7 @@
>   #include "openvswitch/hmap.h"
>   #include "openvswitch/json.h"
>   #include "ovn/lex.h"
> +#include "lib/mac-binding-index.h"
>   #include "lib/chassis-index.h"
>   #include "lib/ip-mcast-index.h"
>   #include "lib/copp.h"
> @@ -79,6 +80,7 @@ struct northd_context {
>       struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
>       struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>       struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip;
>   };
>
>   struct northd_state {
> @@ -8197,6 +8199,41 @@ build_bfd_table(struct northd_context *ctx, struct hmap *bfd_connections,
>       bitmap_free(bfd_src_ports);
>   }
>
> +static void
> +build_mac_binding_table(struct northd_context *ctx, struct hmap *datapaths)
> +{
> +    const struct nbrec_logical_router *nbr;
> +    NBREC_LOGICAL_ROUTER_FOR_EACH (nbr, ctx->ovnnb_idl) {
> +        if (!lrouter_is_enabled(nbr)) {
> +            continue;
> +        }
> +
> +        struct ovn_datapath *od = ovn_datapath_find(datapaths,
> +                                                    &nbr->header_.uuid);
> +
> +        if (od && od->sb) {
> +            for (int i = 0; i < od->nbr->n_mac_bindings; i++) {
> +                const struct nbrec_mac_binding *nb_mac_binding;
> +                nb_mac_binding = od->nbr->mac_bindings[i];
> +
> +                const struct sbrec_mac_binding *mb =
> +                    mac_binding_lookup(ctx->sbrec_mac_binding_by_lport_ip,
> +                                       nb_mac_binding->logical_port,
> +                                       nb_mac_binding->ip);
> +                if (!mb) {
> +                    mb = sbrec_mac_binding_insert(ctx->ovnsb_txn);
> +                    sbrec_mac_binding_set_logical_port(mb, nb_mac_binding->logical_port);
> +                    sbrec_mac_binding_set_ip(mb, nb_mac_binding->ip);
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                    sbrec_mac_binding_set_datapath(mb, od->sb);
> +                } else {
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                }
> +            }
> +        }
> +    }
> +}
> +
>   /* Returns a string of the IP address of the router port 'op' that
>    * overlaps with 'ip_s".  If one is not found, returns NULL.
>    *
> @@ -14121,6 +14158,7 @@ ovnnb_db_run(struct northd_context *ctx,
>       build_mcast_groups(ctx, datapaths, ports, &mcast_groups, &igmp_groups);
>       build_meter_groups(ctx, &meter_groups);
>       build_bfd_table(ctx, &bfd_connections, ports);
> +    build_mac_binding_table(ctx, datapaths);
>       build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
>                    &igmp_groups, &meter_groups, &lbs, &bfd_connections,
>                    ovn_internal_version_changed);
> @@ -15272,6 +15310,9 @@ main(int argc, char *argv[])
>       struct ovsdb_idl_index *sbrec_ip_mcast_by_dp
>           = ip_mcast_index_create(ovnsb_idl_loop.idl);
>
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip
> +        = mac_binding_index_create(ovnsb_idl_loop.idl);
> +
>       unixctl_command_register("sb-connection-status", "", 0, 0,
>                                ovn_conn_show, ovnsb_idl_loop.idl);
>
> @@ -15317,6 +15358,7 @@ main(int argc, char *argv[])
>                   .sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
>                   .sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
>                   .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> +                .sbrec_mac_binding_by_lport_ip = sbrec_mac_binding_by_lport_ip,
>               };
>
>               if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index dcfa1fb5d..e1b3b516c 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1010,6 +1010,15 @@ sb::Out_MAC_Binding (._uuid        = mb._uuid,
>       sb::Out_Port_Binding(.logical_port = mb.logical_port),
>       sb::Out_Datapath_Binding(._uuid = mb.datapath).
>
> +sb::Out_MAC_Binding (._uuid        = hash,
> +                    .logical_port = nb.logical_port,
> +                    .ip           = nb.ip,
> +                    .mac          = nb.mac,
> +                    .datapath     = router._uuid) :-
> +    nb in nb::MAC_Binding(),
> +    rp in &RouterPort(.router = router, .json_name = json_string_escape(nb.logical_port)),
> +    var hash = hash128((nb.logical_port, nb.ip)).
> +
>   /*
>    * DHCP options: fixed table
>    */
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 2ac8ef3ea..734d9649e 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>   {
>       "name": "OVN_Northbound",
> -    "version": "5.32.1",
> -    "cksum": "2805328215 29734",
> +    "version": "5.33.0",
> +    "cksum": "3031667641 30300",
>       "tables": {
>           "NB_Global": {
>               "columns": {
> @@ -325,6 +325,12 @@
>                                        "refType": "strong"},
>                                "min": 0,
>                                "max": "unlimited"}},
> +                "mac_bindings": {
> +                    "type": {"key": {"type": "uuid",
> +                                     "refTable": "MAC_Binding",
> +                                     "refType": "strong"},
> +                             "min": 0,
> +                             "max": "unlimited"}},
>                   "enabled": {"type": {"key": "boolean", "min": 0, "max": 1}},
>                   "nat": {"type": {"key": {"type": "uuid",
>                                            "refTable": "NAT",
> @@ -575,5 +581,12 @@
>                       "type": {"key": "string", "value": "string",
>                                "min": 0, "max": "unlimited"}}},
>               "indexes": [["logical_port", "dst_ip"]],
> +            "isRoot": true},
> +        "MAC_Binding": {
> +            "columns": {
> +                "logical_port": {"type": "string"},
> +                "ip": {"type": "string"},
> +                "mac": {"type": "string"}},
> +            "indexes": [["logical_port", "ip"]],
>               "isRoot": true}}
>       }
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 390cc5a44..34252581a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2043,6 +2043,10 @@
>         Zero or more routing policies for the router.
>       </column>
>
> +    <column name="mac_bindings">
> +      Zero or more statically configured mac_bindings for the router.
> +    </column>
> +
>       <column name="enabled">
>         This column is used to administratively set router state.  If this column
>         is empty or is set to <code>true</code>, the router is enabled.  If this
> @@ -4103,4 +4107,28 @@
>         </column>
>       </group>
>     </table>
> +
> +  <table name="MAC_Binding">
> +    <p>
> +      Each record represents a MAC Binding entry.
> +    </p>
> +
> +    <group title="Configuration">
> +      <p>
> +        <code>ovn-northd</code> reads configuration from these columns.
> +      </p>
> +
> +      <column name="logical_port">
> +        The logical port on which the binding was discovered.
> +      </column>
> +
> +      <column name="ip">
> +        The bound IP address.
> +      </column>
> +
> +      <column name="mac">
> +        The Ethernet address to which the IP is bound.
> +      </column>
> +    </group>
> +  </table>
>   </database>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 9b80ae410..7f6761abd 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2057,6 +2057,70 @@ AT_CHECK([ovn-nbctl list forwarding_group], [0], [])
>
>   dnl ---------------------------------------------------------------------
>
> +OVN_NBCTL_TEST([ovn_nbctl_mac_binding], [lr mac_binding], [
> +
> +AT_CHECK([ovn-nbctl lr-add lr0])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 foo 00:00:44:55:66:88], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.200 foo], [1], [],
> +  [ovn-nbctl: invalid mac address foo.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77], [1], [],
> +  [ovn-nbctl: lr0-p0, 172.16.0.11: a MAC_Binding with this logical_port and ip already exists
> +])
> +
> +AT_CHECK([ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p0                   10.0.0.10                00:00:33:44:55:66
> +lr0-p0                   172.16.0.11              00:00:44:55:66:77
> +lr0-p0                   192.168.10.10            00:00:11:22:33:44
> +lr0-p0                   192.168.10.100           00:00:22:33:44:55
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0 foo], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.100], [1], [],
> +  [ovn-nbctl: no matching MAC_Binding with the port (lr0-p1) and ip (10.0.0.100)
> +])
> +
> +AT_CHECK([ovn-nbctl --if-exists lr-mac-binding-del lr0 lr0-p1 10.0.0.100])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.10])
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +])
> +
> +dnl ---------------------------------------------------------------------
> +
>   OVN_NBCTL_TEST([ovn_nbctl_negative], [basic negative tests], [
>   AT_CHECK([ovn-nbctl --id=@ls create logical_switch name=foo -- \
>             set logical_switch foo1 name=bar],
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4957a1063..3a719cb06 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -28110,3 +28110,29 @@ grep lr0-sw1], [1], [])
>   OVN_CLEANUP([hv1])
>   AT_CLEANUP
>   ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([LR NB MAC_Binding table])
> +ovn_start
> +
> +# Create logical routers
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24
> +ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55
> +
> +wait_row_count nb:MAC_Binding 2 logical_port=lr0-p0
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.10 mac="00\:00\:11\:22\:33\:44"
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:44\:55"
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66
> +wait_row_count nb:MAC_Binding 1 logical_port=lr0-p1
> +wait_row_count MAC_Binding 1 logical_port=lr0-p1 ip=10.0.0.10 mac="00\:00\:33\:44\:55\:66"
> +
> +ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:55:66
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> +
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 10217dcd5..b0356d3aa 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -366,6 +366,15 @@ Policy commands:\n\
>                               remove policies from ROUTER\n\
>     lr-policy-list ROUTER     print policies for ROUTER\n\
>   \n\
> +MAC_Binding commands:\n\
> +  lr-mac-binding-add ROUTER LOGICAL_PORT IP MAC\n\
> +                                    add a MAC_Binding entry to ROUTER\n\
> +  lr-mac-binding-del ROUTER LOGICAL_PORT [IP]\n\
> +                                    remove MAC_Binding entry from ROUTER and\n\
> +                                    delete the MAC_Binding row\n\
> +  lr-mac-binding-list ROUTER        print MAC_Bindings for ROUTER\n\
> +\n\n",program_name, program_name);
> +    printf("\
>   NAT commands:\n\
>     [--stateless]\n\
>     [--portrange]\n\
> @@ -408,8 +417,7 @@ Connection commands:\n\
>     del-connection             delete the connections\n\
>     [--inactivity-probe=MSECS]\n\
>     set-connection TARGET...   set the list of connections to TARGET...\n\
> -\n\n",program_name, program_name);
> -    printf("\
> +\n\
>   SSL commands:\n\
>     get-ssl                     print the SSL configuration\n\
>     del-ssl                     delete the SSL configuration\n\
> @@ -5595,6 +5603,205 @@ nbctl_lrp_get_redirect_type(struct ctl_context *ctx)
>                     !redirect_type ? "overlay": redirect_type);
>   }
>
> +static void
> +nbctl_pre_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    const char *logical_port = ctx->argv[2];
> +    const char *ip = ctx->argv[3];
> +    const char *mac = ctx->argv[4];
> +    char *new_ip = NULL;
> +
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const struct nbrec_logical_router_port *lrp;
> +    error = lrp_by_name_or_uuid(ctx, logical_port, true, &lrp);
> +    if (error) {
> +        ctx->error = error;
> +        goto cleanup;
> +    }
> +
> +    new_ip = normalize_addr_str(ip);
> +    if (!new_ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ip);
> +        return;
> +    }
> +
> +    struct eth_addr ea;
> +    if (!eth_addr_from_string(mac, &ea)) {
> +        ctl_error(ctx, "invalid mac address %s.", mac);
> +        goto cleanup;
> +    }
> +
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +
> +        char *old_ip;
> +        bool should_return = false;
> +        old_ip = normalize_addr_str(mac_binding->ip);
> +
> +        if (!strcmp(mac_binding->logical_port, logical_port)) {
> +            if (!strcmp(old_ip, new_ip)) {
> +                if (may_exist) {
> +                    nbrec_mac_binding_verify_mac(mac_binding);
> +                    nbrec_mac_binding_set_mac(mac_binding, mac);
> +                    should_return = true;
> +                } else {
> +                    ctl_error(ctx, "%s, %s: a MAC_Binding with this "
> +                              "logical_port and ip already "
> +                              "exists", logical_port, new_ip);
> +                    should_return = true;
> +                }
> +            }
> +        }
> +        free(old_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Create MAC_Binding entry */
> +    struct nbrec_mac_binding *mac_binding = nbrec_mac_binding_insert(ctx->txn);
> +    nbrec_mac_binding_set_logical_port(mac_binding, logical_port);
> +    nbrec_mac_binding_set_ip(mac_binding, new_ip);
> +    nbrec_mac_binding_set_mac(mac_binding, mac);
> +
> +    /* Insert MAC_Binding entry into logical router */
> +    nbrec_logical_router_update_mac_bindings_addvalue(lr, mac_binding);
> +
> +cleanup:
> +    free(new_ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const char *logical_port = ctx->argv[2];
> +    if (ctx->argc == 3) {
> +        /* Deletes all NB MAC_binding entries on the given logical port. */
> +        for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +            if (!strcmp(logical_port, lr->mac_bindings[i]->logical_port)) {
> +                struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +                nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +                nbrec_mac_binding_delete(mac_binding);
> +            }
> +        }
> +        return;
> +    }
> +
> +    char *ip = normalize_addr_str(ctx->argv[3]);
> +    if (!ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ctx->argv[3]);
> +        return;
> +    }
> +
> +    /* Remove the matching MAC_Binding. */
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        bool should_return = false;
> +        char *mac_binding_ip = normalize_addr_str(mac_binding->ip);
> +        if (!mac_binding_ip) {
> +            continue;
> +        }
> +        if (!strcmp(ip, mac_binding_ip)) {
> +            nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +            nbrec_mac_binding_delete(mac_binding);
> +            should_return = true;
> +        }
> +        free(mac_binding_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (must_exist) {
> +        ctl_error(ctx, "no matching MAC_Binding with the port (%s) and ip (%s)",
> +                  logical_port, ip);
> +    }
> +
> +cleanup:
> +    free(ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr;
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    struct smap lr_mac_bindings = SMAP_INITIALIZER(&lr_mac_bindings);
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        char *key = xasprintf("%-25s%-25s", mac_binding->logical_port,
> +                              mac_binding->ip);
> +        smap_add_format(&lr_mac_bindings, key, "%s", mac_binding->mac);
> +        free(key);
> +    }
> +
> +    const struct smap_node **nodes = smap_sort(&lr_mac_bindings);
> +    if (nodes) {
> +        ds_put_format(&ctx->output, "%-25s%-25s%s\n",
> +                      "LOGICAL_PORT", "IP", "MAC");
> +        for (size_t i = 0; i < smap_count(&lr_mac_bindings); i++) {
> +            const struct smap_node *node = nodes[i];
> +            ds_put_format(&ctx->output, "%-25s%s\n", node->key, node->value);
> +        }
> +    }
> +}
> +
>   static const struct nbrec_forwarding_group *
>   fwd_group_by_name_or_uuid(struct ctl_context *ctx, const char *id)
>   {
> @@ -6958,6 +7165,17 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>       { "lr-policy-list", 1, 1, "ROUTER", nbctl_pre_lr_policy_list,
>         nbctl_lr_policy_list, NULL, "", RO },
>
> +    /* MAC_Binding commands */
> +    { "lr-mac-binding-add", 4, 4,
> +      "ROUTER LOGICAL_PORT IP MAC",
> +      nbctl_pre_lr_mac_binding_add, nbctl_lr_mac_binding_add, NULL,
> +      "--may-exist", RW },
> +    { "lr-mac-binding-del", 2, 3, "ROUTER LOGICAL_PORT [IP]",
> +      nbctl_pre_lr_mac_binding_del, nbctl_lr_mac_binding_del,
> +      NULL, "--if-exists", RW },
> +    { "lr-mac-binding-list", 1, 1, "ROUTER", nbctl_pre_lr_mac_binding_list,
> +      nbctl_lr_mac_binding_list, NULL, "", RO },
> +
>       /* NAT commands. */
>       { "lr-nat-add", 4, 7,
>         "ROUTER TYPE EXTERNAL_IP LOGICAL_IP"
>

> From: Karthik Chandrashekar <karthik.c@nutanix.com>
> 
> Add a new NB MAC_Binding table. This allows programming of
> static mac_bindings for a logical_router. OVN northd is
> responsible for propagating these static entries to the
> existing SB MAC_Binding table.

Hi Karthik,

thx a lot for the patch. Some comments inline.

Regards,
Lorenzo

> 
> Signed-off-by: Karthik Chandrashekar <karthik.c@nutanix.com>
> ---
>  lib/automake.mk         |   2 +
>  lib/mac-binding-index.c |  43 ++++++++
>  lib/mac-binding-index.h |  29 ++++++
>  manpages.mk             |   1 -
>  northd/ovn-northd.c     |  42 ++++++++
>  northd/ovn_northd.dl    |   9 ++
>  ovn-nb.ovsschema        |  17 ++-
>  ovn-nb.xml              |  28 +++++
>  tests/ovn-nbctl.at      |  64 ++++++++++++
>  tests/ovn.at            |  26 +++++
>  utilities/ovn-nbctl.c   | 222 +++++++++++++++++++++++++++++++++++++++-
>  11 files changed, 478 insertions(+), 5 deletions(-)
>  create mode 100644 lib/mac-binding-index.c
>  create mode 100644 lib/mac-binding-index.h
> 
> diff --git a/lib/automake.mk b/lib/automake.mk
> index ddfe33948..4c7974ee2 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -20,6 +20,8 @@ lib_libovn_la_SOURCES = \
>  	lib/ovn-parallel-hmap.c \
>  	lib/ip-mcast-index.c \
>  	lib/ip-mcast-index.h \
> +	lib/mac-binding-index.c \
> +	lib/mac-binding-index.h \
>  	lib/mcast-group-index.c \
>  	lib/mcast-group-index.h \
>  	lib/lex.c \
> diff --git a/lib/mac-binding-index.c b/lib/mac-binding-index.c
> new file mode 100644
> index 000000000..1d34dfbdc
> --- /dev/null
> +++ b/lib/mac-binding-index.c
> @@ -0,0 +1,43 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "lib/mac-binding-index.h"
> +#include "lib/ovn-sb-idl.h"
> +
> +struct ovsdb_idl_index *
> +mac_binding_index_create(struct ovsdb_idl *idl)
> +{
> +    return ovsdb_idl_index_create2(idl,
> +                                   &sbrec_mac_binding_col_logical_port,
> +                                   &sbrec_mac_binding_col_ip);
> +}
> +
> +const struct sbrec_mac_binding *
> +mac_binding_lookup(struct ovsdb_idl_index *mac_binding_index,
> +                   const char *logical_port, const char *ip)
> +{
> +    struct sbrec_mac_binding *target = sbrec_mac_binding_index_init_row(
> +        mac_binding_index);
> +    sbrec_mac_binding_index_set_logical_port(target, logical_port);
> +    sbrec_mac_binding_index_set_ip(target, ip);
> +
> +    struct sbrec_mac_binding *mac_binding =
> +        sbrec_mac_binding_index_find(mac_binding_index, target);
> +    sbrec_mac_binding_index_destroy_row(target);
> +
> +    return mac_binding;
> +}

I guess we can drop the same code in pinctrl.c now

> diff --git a/lib/mac-binding-index.h b/lib/mac-binding-index.h
> new file mode 100644
> index 000000000..d7434f688
> --- /dev/null
> +++ b/lib/mac-binding-index.h
> @@ -0,0 +1,29 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef OVN_MAC_BINDING_INDEX_H
> +#define OVN_MAC_BINDING_INDEX_H 1
> +
> +struct ovsdb_idl;
> +
> +struct sbrec_datapath_binding;
> +
> +struct ovsdb_idl_index *mac_binding_index_create(struct ovsdb_idl *);
> +const struct sbrec_mac_binding *mac_binding_lookup(
> +    struct ovsdb_idl_index *mac_binding_index,
> +    const char *logical_port,
> +    const char *ip);
> +
> +#endif /* lib/mac-binding-index.h */
> diff --git a/manpages.mk b/manpages.mk
> index 3334b38f9..9f7a0ced3 100644
> --- a/manpages.mk
> +++ b/manpages.mk
> @@ -9,4 +9,3 @@ utilities/ovn-detrace.1.in:
>  lib/common-syn.man:
>  lib/common.man:
>  lib/ovs.tmac:
> -
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index af413aba4..6113b1e1c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c

the counterpart in northd/northd.c is missing. I guess you are using an
outdated version of ovn, aren't you?

> @@ -30,6 +30,7 @@
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/json.h"
>  #include "ovn/lex.h"
> +#include "lib/mac-binding-index.h"
>  #include "lib/chassis-index.h"
>  #include "lib/ip-mcast-index.h"
>  #include "lib/copp.h"
> @@ -79,6 +80,7 @@ struct northd_context {
>      struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
>      struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip;
>  };
>  
>  struct northd_state {
> @@ -8197,6 +8199,41 @@ build_bfd_table(struct northd_context *ctx, struct hmap *bfd_connections,
>      bitmap_free(bfd_src_ports);
>  }
>  
> +static void
> +build_mac_binding_table(struct northd_context *ctx, struct hmap *datapaths)

What happen if we have an ip/mac binding collision? e.g. if ovn-controller
tries to update the same port/ip with a discovered binding? I guess static
info should not be overwritten.

> +{
> +    const struct nbrec_logical_router *nbr;
> +    NBREC_LOGICAL_ROUTER_FOR_EACH (nbr, ctx->ovnnb_idl) {
> +        if (!lrouter_is_enabled(nbr)) {
> +            continue;
> +        }
> +
> +        struct ovn_datapath *od = ovn_datapath_find(datapaths,
> +                                                    &nbr->header_.uuid);
> +
> +        if (od && od->sb) {
> +            for (int i = 0; i < od->nbr->n_mac_bindings; i++) {
> +                const struct nbrec_mac_binding *nb_mac_binding;
> +                nb_mac_binding = od->nbr->mac_bindings[i];
> +
> +                const struct sbrec_mac_binding *mb =
> +                    mac_binding_lookup(ctx->sbrec_mac_binding_by_lport_ip,
> +                                       nb_mac_binding->logical_port,
> +                                       nb_mac_binding->ip);
> +                if (!mb) {
> +                    mb = sbrec_mac_binding_insert(ctx->ovnsb_txn);
> +                    sbrec_mac_binding_set_logical_port(mb, nb_mac_binding->logical_port);
> +                    sbrec_mac_binding_set_ip(mb, nb_mac_binding->ip);
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                    sbrec_mac_binding_set_datapath(mb, od->sb);
> +                } else {
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                }
> +            }
> +        }
> +    }
> +}
> +
>  /* Returns a string of the IP address of the router port 'op' that
>   * overlaps with 'ip_s".  If one is not found, returns NULL.
>   *
> @@ -14121,6 +14158,7 @@ ovnnb_db_run(struct northd_context *ctx,
>      build_mcast_groups(ctx, datapaths, ports, &mcast_groups, &igmp_groups);
>      build_meter_groups(ctx, &meter_groups);
>      build_bfd_table(ctx, &bfd_connections, ports);
> +    build_mac_binding_table(ctx, datapaths);
>      build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
>                   &igmp_groups, &meter_groups, &lbs, &bfd_connections,
>                   ovn_internal_version_changed);
> @@ -15272,6 +15310,9 @@ main(int argc, char *argv[])
>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp
>          = ip_mcast_index_create(ovnsb_idl_loop.idl);
>  
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip
> +        = mac_binding_index_create(ovnsb_idl_loop.idl);
> +
>      unixctl_command_register("sb-connection-status", "", 0, 0,
>                               ovn_conn_show, ovnsb_idl_loop.idl);
>  
> @@ -15317,6 +15358,7 @@ main(int argc, char *argv[])
>                  .sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
>                  .sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
>                  .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> +                .sbrec_mac_binding_by_lport_ip = sbrec_mac_binding_by_lport_ip,
>              };
>  
>              if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index dcfa1fb5d..e1b3b516c 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1010,6 +1010,15 @@ sb::Out_MAC_Binding (._uuid        = mb._uuid,
>      sb::Out_Port_Binding(.logical_port = mb.logical_port),
>      sb::Out_Datapath_Binding(._uuid = mb.datapath).
>  
> +sb::Out_MAC_Binding (._uuid        = hash,
> +                    .logical_port = nb.logical_port,
> +                    .ip           = nb.ip,
> +                    .mac          = nb.mac,
> +                    .datapath     = router._uuid) :-
> +    nb in nb::MAC_Binding(),
> +    rp in &RouterPort(.router = router, .json_name = json_string_escape(nb.logical_port)),
> +    var hash = hash128((nb.logical_port, nb.ip)).
> +
>  /*
>   * DHCP options: fixed table
>   */
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 2ac8ef3ea..734d9649e 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.32.1",

This is not the latest version I guess.

> -    "cksum": "2805328215 29734",
> +    "version": "5.33.0",
> +    "cksum": "3031667641 30300",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -325,6 +325,12 @@
>                                       "refType": "strong"},
>                               "min": 0,
>                               "max": "unlimited"}},
> +                "mac_bindings": {
> +                    "type": {"key": {"type": "uuid",
> +                                     "refTable": "MAC_Binding",
> +                                     "refType": "strong"},
> +                             "min": 0,
> +                             "max": "unlimited"}},
>                  "enabled": {"type": {"key": "boolean", "min": 0, "max": 1}},
>                  "nat": {"type": {"key": {"type": "uuid",
>                                           "refTable": "NAT",
> @@ -575,5 +581,12 @@
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
>              "indexes": [["logical_port", "dst_ip"]],
> +            "isRoot": true},
> +        "MAC_Binding": {
> +            "columns": {
> +                "logical_port": {"type": "string"},
> +                "ip": {"type": "string"},
> +                "mac": {"type": "string"}},
> +            "indexes": [["logical_port", "ip"]],
>              "isRoot": true}}
>      }
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 390cc5a44..34252581a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2043,6 +2043,10 @@
>        Zero or more routing policies for the router.
>      </column>
>  
> +    <column name="mac_bindings">
> +      Zero or more statically configured mac_bindings for the router.
> +    </column>
> +
>      <column name="enabled">
>        This column is used to administratively set router state.  If this column
>        is empty or is set to <code>true</code>, the router is enabled.  If this
> @@ -4103,4 +4107,28 @@
>        </column>
>      </group>
>    </table>
> +
> +  <table name="MAC_Binding">
> +    <p>
> +      Each record represents a MAC Binding entry.
> +    </p>
> +
> +    <group title="Configuration">
> +      <p>
> +        <code>ovn-northd</code> reads configuration from these columns.
> +      </p>
> +
> +      <column name="logical_port">
> +        The logical port on which the binding was discovered.
> +      </column>
> +
> +      <column name="ip">
> +        The bound IP address.
> +      </column>
> +
> +      <column name="mac">
> +        The Ethernet address to which the IP is bound.
> +      </column>
> +    </group>
> +  </table>
>  </database>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 9b80ae410..7f6761abd 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2057,6 +2057,70 @@ AT_CHECK([ovn-nbctl list forwarding_group], [0], [])
>  
>  dnl ---------------------------------------------------------------------
>  
> +OVN_NBCTL_TEST([ovn_nbctl_mac_binding], [lr mac_binding], [
> +
> +AT_CHECK([ovn-nbctl lr-add lr0])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 foo 00:00:44:55:66:88], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.200 foo], [1], [],
> +  [ovn-nbctl: invalid mac address foo.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77], [1], [],
> +  [ovn-nbctl: lr0-p0, 172.16.0.11: a MAC_Binding with this logical_port and ip already exists
> +])
> +
> +AT_CHECK([ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p0                   10.0.0.10                00:00:33:44:55:66
> +lr0-p0                   172.16.0.11              00:00:44:55:66:77
> +lr0-p0                   192.168.10.10            00:00:11:22:33:44
> +lr0-p0                   192.168.10.100           00:00:22:33:44:55
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0 foo], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.100], [1], [],
> +  [ovn-nbctl: no matching MAC_Binding with the port (lr0-p1) and ip (10.0.0.100)
> +])
> +
> +AT_CHECK([ovn-nbctl --if-exists lr-mac-binding-del lr0 lr0-p1 10.0.0.100])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.10])
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +])
> +
> +dnl ---------------------------------------------------------------------
> +
>  OVN_NBCTL_TEST([ovn_nbctl_negative], [basic negative tests], [
>  AT_CHECK([ovn-nbctl --id=@ls create logical_switch name=foo -- \
>            set logical_switch foo1 name=bar],
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4957a1063..3a719cb06 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -28110,3 +28110,29 @@ grep lr0-sw1], [1], [])
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([LR NB MAC_Binding table])
> +ovn_start
> +
> +# Create logical routers
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24
> +ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55
> +
> +wait_row_count nb:MAC_Binding 2 logical_port=lr0-p0
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.10 mac="00\:00\:11\:22\:33\:44"
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:44\:55"
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66
> +wait_row_count nb:MAC_Binding 1 logical_port=lr0-p1
> +wait_row_count MAC_Binding 1 logical_port=lr0-p1 ip=10.0.0.10 mac="00\:00\:33\:44\:55\:66"
> +
> +ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:55:66
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> +
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 10217dcd5..b0356d3aa 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -366,6 +366,15 @@ Policy commands:\n\
>                              remove policies from ROUTER\n\
>    lr-policy-list ROUTER     print policies for ROUTER\n\
>  \n\
> +MAC_Binding commands:\n\
> +  lr-mac-binding-add ROUTER LOGICAL_PORT IP MAC\n\
> +                                    add a MAC_Binding entry to ROUTER\n\
> +  lr-mac-binding-del ROUTER LOGICAL_PORT [IP]\n\
> +                                    remove MAC_Binding entry from ROUTER and\n\
> +                                    delete the MAC_Binding row\n\
> +  lr-mac-binding-list ROUTER        print MAC_Bindings for ROUTER\n\
> +\n\n",program_name, program_name);
> +    printf("\
>  NAT commands:\n\
>    [--stateless]\n\
>    [--portrange]\n\
> @@ -408,8 +417,7 @@ Connection commands:\n\
>    del-connection             delete the connections\n\
>    [--inactivity-probe=MSECS]\n\
>    set-connection TARGET...   set the list of connections to TARGET...\n\
> -\n\n",program_name, program_name);
> -    printf("\
> +\n\
>  SSL commands:\n\
>    get-ssl                     print the SSL configuration\n\
>    del-ssl                     delete the SSL configuration\n\
> @@ -5595,6 +5603,205 @@ nbctl_lrp_get_redirect_type(struct ctl_context *ctx)
>                    !redirect_type ? "overlay": redirect_type);
>  }
>  
> +static void
> +nbctl_pre_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    const char *logical_port = ctx->argv[2];
> +    const char *ip = ctx->argv[3];
> +    const char *mac = ctx->argv[4];
> +    char *new_ip = NULL;
> +
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const struct nbrec_logical_router_port *lrp;
> +    error = lrp_by_name_or_uuid(ctx, logical_port, true, &lrp);
> +    if (error) {
> +        ctx->error = error;
> +        goto cleanup;
> +    }
> +
> +    new_ip = normalize_addr_str(ip);
> +    if (!new_ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ip);
> +        return;
> +    }
> +
> +    struct eth_addr ea;
> +    if (!eth_addr_from_string(mac, &ea)) {
> +        ctl_error(ctx, "invalid mac address %s.", mac);
> +        goto cleanup;
> +    }
> +
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +
> +        char *old_ip;
> +        bool should_return = false;
> +        old_ip = normalize_addr_str(mac_binding->ip);
> +
> +        if (!strcmp(mac_binding->logical_port, logical_port)) {
> +            if (!strcmp(old_ip, new_ip)) {
> +                if (may_exist) {
> +                    nbrec_mac_binding_verify_mac(mac_binding);
> +                    nbrec_mac_binding_set_mac(mac_binding, mac);
> +                    should_return = true;
> +                } else {
> +                    ctl_error(ctx, "%s, %s: a MAC_Binding with this "
> +                              "logical_port and ip already "
> +                              "exists", logical_port, new_ip);
> +                    should_return = true;
> +                }
> +            }
> +        }
> +        free(old_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Create MAC_Binding entry */
> +    struct nbrec_mac_binding *mac_binding = nbrec_mac_binding_insert(ctx->txn);
> +    nbrec_mac_binding_set_logical_port(mac_binding, logical_port);
> +    nbrec_mac_binding_set_ip(mac_binding, new_ip);
> +    nbrec_mac_binding_set_mac(mac_binding, mac);
> +
> +    /* Insert MAC_Binding entry into logical router */
> +    nbrec_logical_router_update_mac_bindings_addvalue(lr, mac_binding);
> +
> +cleanup:
> +    free(new_ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const char *logical_port = ctx->argv[2];
> +    if (ctx->argc == 3) {
> +        /* Deletes all NB MAC_binding entries on the given logical port. */
> +        for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +            if (!strcmp(logical_port, lr->mac_bindings[i]->logical_port)) {
> +                struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +                nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +                nbrec_mac_binding_delete(mac_binding);
> +            }
> +        }
> +        return;
> +    }
> +
> +    char *ip = normalize_addr_str(ctx->argv[3]);
> +    if (!ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ctx->argv[3]);
> +        return;
> +    }
> +
> +    /* Remove the matching MAC_Binding. */
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        bool should_return = false;
> +        char *mac_binding_ip = normalize_addr_str(mac_binding->ip);
> +        if (!mac_binding_ip) {
> +            continue;
> +        }
> +        if (!strcmp(ip, mac_binding_ip)) {
> +            nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +            nbrec_mac_binding_delete(mac_binding);
> +            should_return = true;
> +        }
> +        free(mac_binding_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (must_exist) {
> +        ctl_error(ctx, "no matching MAC_Binding with the port (%s) and ip (%s)",
> +                  logical_port, ip);
> +    }
> +
> +cleanup:
> +    free(ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr;
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    struct smap lr_mac_bindings = SMAP_INITIALIZER(&lr_mac_bindings);
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        char *key = xasprintf("%-25s%-25s", mac_binding->logical_port,
> +                              mac_binding->ip);
> +        smap_add_format(&lr_mac_bindings, key, "%s", mac_binding->mac);
> +        free(key);
> +    }
> +
> +    const struct smap_node **nodes = smap_sort(&lr_mac_bindings);
> +    if (nodes) {
> +        ds_put_format(&ctx->output, "%-25s%-25s%s\n",
> +                      "LOGICAL_PORT", "IP", "MAC");
> +        for (size_t i = 0; i < smap_count(&lr_mac_bindings); i++) {
> +            const struct smap_node *node = nodes[i];
> +            ds_put_format(&ctx->output, "%-25s%s\n", node->key, node->value);
> +        }
> +    }
> +}
> +
>  static const struct nbrec_forwarding_group *
>  fwd_group_by_name_or_uuid(struct ctl_context *ctx, const char *id)
>  {
> @@ -6958,6 +7165,17 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>      { "lr-policy-list", 1, 1, "ROUTER", nbctl_pre_lr_policy_list,
>        nbctl_lr_policy_list, NULL, "", RO },
>  
> +    /* MAC_Binding commands */
> +    { "lr-mac-binding-add", 4, 4,
> +      "ROUTER LOGICAL_PORT IP MAC",
> +      nbctl_pre_lr_mac_binding_add, nbctl_lr_mac_binding_add, NULL,
> +      "--may-exist", RW },
> +    { "lr-mac-binding-del", 2, 3, "ROUTER LOGICAL_PORT [IP]",
> +      nbctl_pre_lr_mac_binding_del, nbctl_lr_mac_binding_del,
> +      NULL, "--if-exists", RW },
> +    { "lr-mac-binding-list", 1, 1, "ROUTER", nbctl_pre_lr_mac_binding_list,
> +      nbctl_lr_mac_binding_list, NULL, "", RO },
> +
>      /* NAT commands. */
>      { "lr-nat-add", 4, 7,
>        "ROUTER TYPE EXTERNAL_IP LOGICAL_IP"
> -- 
> 2.22.3
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Thank you for the review. I’ve incorporated the changes suggested by Lorenzo and Mark and sent out v2 of the patch.

Thank You,
Karthik C

On 10/14/21, 9:55 AM, "Lorenzo Bianconi" <lorenzo.bianconi@redhat.com> wrote:
> From: Karthik Chandrashekar <karthik.c@nutanix.com<mailto:karthik.c@nutanix.com>>
>
> Add a new NB MAC_Binding table. This allows programming of
> static mac_bindings for a logical_router. OVN northd is
> responsible for propagating these static entries to the
> existing SB MAC_Binding table.

Hi Karthik,

thx a lot for the patch. Some comments inline.

Regards,
Lorenzo

>
> Signed-off-by: Karthik Chandrashekar <karthik.c@nutanix.com<mailto:karthik.c@nutanix.com>>
> ---
>  lib/automake.mk         |   2 +
>  lib/mac-binding-index.c |  43 ++++++++
>  lib/mac-binding-index.h |  29 ++++++
>  manpages.mk             |   1 -
>  northd/ovn-northd.c     |  42 ++++++++
>  northd/ovn_northd.dl    |   9 ++
>  ovn-nb.ovsschema        |  17 ++-
>  ovn-nb.xml              |  28 +++++
>  tests/ovn-nbctl.at      |  64 ++++++++++++
>  tests/ovn.at            |  26 +++++
>  utilities/ovn-nbctl.c   | 222 +++++++++++++++++++++++++++++++++++++++-
>  11 files changed, 478 insertions(+), 5 deletions(-)
>  create mode 100644 lib/mac-binding-index.c
>  create mode 100644 lib/mac-binding-index.h
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index ddfe33948..4c7974ee2 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -20,6 +20,8 @@ lib_libovn_la_SOURCES = \
>             lib/ovn-parallel-hmap.c \
>             lib/ip-mcast-index.c \
>             lib/ip-mcast-index.h \
> +          lib/mac-binding-index.c \
> +          lib/mac-binding-index.h \
>             lib/mcast-group-index.c \
>             lib/mcast-group-index.h \
>             lib/lex.c \
> diff --git a/lib/mac-binding-index.c b/lib/mac-binding-index.c
> new file mode 100644
> index 000000000..1d34dfbdc
> --- /dev/null
> +++ b/lib/mac-binding-index.c
> @@ -0,0 +1,43 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +
> +#include "lib/mac-binding-index.h"
> +#include "lib/ovn-sb-idl.h"
> +
> +struct ovsdb_idl_index *
> +mac_binding_index_create(struct ovsdb_idl *idl)
> +{
> +    return ovsdb_idl_index_create2(idl,
> +                                   &sbrec_mac_binding_col_logical_port,
> +                                   &sbrec_mac_binding_col_ip);
> +}
> +
> +const struct sbrec_mac_binding *
> +mac_binding_lookup(struct ovsdb_idl_index *mac_binding_index,
> +                   const char *logical_port, const char *ip)
> +{
> +    struct sbrec_mac_binding *target = sbrec_mac_binding_index_init_row(
> +        mac_binding_index);
> +    sbrec_mac_binding_index_set_logical_port(target, logical_port);
> +    sbrec_mac_binding_index_set_ip(target, ip);
> +
> +    struct sbrec_mac_binding *mac_binding =
> +        sbrec_mac_binding_index_find(mac_binding_index, target);
> +    sbrec_mac_binding_index_destroy_row(target);
> +
> +    return mac_binding;
> +}

I guess we can drop the same code in pinctrl.c now

> diff --git a/lib/mac-binding-index.h b/lib/mac-binding-index.h
> new file mode 100644
> index 000000000..d7434f688
> --- /dev/null
> +++ b/lib/mac-binding-index.h
> @@ -0,0 +1,29 @@
> +/* Copyright (c) 2021
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#ifndef OVN_MAC_BINDING_INDEX_H
> +#define OVN_MAC_BINDING_INDEX_H 1
> +
> +struct ovsdb_idl;
> +
> +struct sbrec_datapath_binding;
> +
> +struct ovsdb_idl_index *mac_binding_index_create(struct ovsdb_idl *);
> +const struct sbrec_mac_binding *mac_binding_lookup(
> +    struct ovsdb_idl_index *mac_binding_index,
> +    const char *logical_port,
> +    const char *ip);
> +
> +#endif /* lib/mac-binding-index.h */
> diff --git a/manpages.mk b/manpages.mk
> index 3334b38f9..9f7a0ced3 100644
> --- a/manpages.mk
> +++ b/manpages.mk
> @@ -9,4 +9,3 @@ utilities/ovn-detrace.1.in:
>  lib/common-syn.man:
>  lib/common.man:
>  lib/ovs.tmac:
> -
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index af413aba4..6113b1e1c 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c

the counterpart in northd/northd.c is missing. I guess you are using an
outdated version of ovn, aren't you?

> @@ -30,6 +30,7 @@
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/json.h"
>  #include "ovn/lex.h"
> +#include "lib/mac-binding-index.h"
>  #include "lib/chassis-index.h"
>  #include "lib/ip-mcast-index.h"
>  #include "lib/copp.h"
> @@ -79,6 +80,7 @@ struct northd_context {
>      struct ovsdb_idl_index *sbrec_ha_chassis_grp_by_name;
>      struct ovsdb_idl_index *sbrec_mcast_group_by_name_dp;
>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp;
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip;
>  };
>
>  struct northd_state {
> @@ -8197,6 +8199,41 @@ build_bfd_table(struct northd_context *ctx, struct hmap *bfd_connections,
>      bitmap_free(bfd_src_ports);
>  }
>
> +static void
> +build_mac_binding_table(struct northd_context *ctx, struct hmap *datapaths)

What happen if we have an ip/mac binding collision? e.g. if ovn-controller
tries to update the same port/ip with a discovered binding? I guess static
info should not be overwritten.

> +{
> +    const struct nbrec_logical_router *nbr;
> +    NBREC_LOGICAL_ROUTER_FOR_EACH (nbr, ctx->ovnnb_idl) {
> +        if (!lrouter_is_enabled(nbr)) {
> +            continue;
> +        }
> +
> +        struct ovn_datapath *od = ovn_datapath_find(datapaths,
> +                                                    &nbr->header_.uuid);
> +
> +        if (od && od->sb) {
> +            for (int i = 0; i < od->nbr->n_mac_bindings; i++) {
> +                const struct nbrec_mac_binding *nb_mac_binding;
> +                nb_mac_binding = od->nbr->mac_bindings[i];
> +
> +                const struct sbrec_mac_binding *mb =
> +                    mac_binding_lookup(ctx->sbrec_mac_binding_by_lport_ip,
> +                                       nb_mac_binding->logical_port,
> +                                       nb_mac_binding->ip);
> +                if (!mb) {
> +                    mb = sbrec_mac_binding_insert(ctx->ovnsb_txn);
> +                    sbrec_mac_binding_set_logical_port(mb, nb_mac_binding->logical_port);
> +                    sbrec_mac_binding_set_ip(mb, nb_mac_binding->ip);
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                    sbrec_mac_binding_set_datapath(mb, od->sb);
> +                } else {
> +                    sbrec_mac_binding_set_mac(mb, nb_mac_binding->mac);
> +                }
> +            }
> +        }
> +    }
> +}
> +
>  /* Returns a string of the IP address of the router port 'op' that
>   * overlaps with 'ip_s".  If one is not found, returns NULL.
>   *
> @@ -14121,6 +14158,7 @@ ovnnb_db_run(struct northd_context *ctx,
>      build_mcast_groups(ctx, datapaths, ports, &mcast_groups, &igmp_groups);
>      build_meter_groups(ctx, &meter_groups);
>      build_bfd_table(ctx, &bfd_connections, ports);
> +    build_mac_binding_table(ctx, datapaths);
>      build_lflows(ctx, datapaths, ports, &port_groups, &mcast_groups,
>                   &igmp_groups, &meter_groups, &lbs, &bfd_connections,
>                   ovn_internal_version_changed);
> @@ -15272,6 +15310,9 @@ main(int argc, char *argv[])
>      struct ovsdb_idl_index *sbrec_ip_mcast_by_dp
>          = ip_mcast_index_create(ovnsb_idl_loop.idl);
>
> +    struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip
> +        = mac_binding_index_create(ovnsb_idl_loop.idl);
> +
>      unixctl_command_register("sb-connection-status", "", 0, 0,
>                               ovn_conn_show, ovnsb_idl_loop.idl);
>
> @@ -15317,6 +15358,7 @@ main(int argc, char *argv[])
>                  .sbrec_ha_chassis_grp_by_name = sbrec_ha_chassis_grp_by_name,
>                  .sbrec_mcast_group_by_name_dp = sbrec_mcast_group_by_name_dp,
>                  .sbrec_ip_mcast_by_dp = sbrec_ip_mcast_by_dp,
> +                .sbrec_mac_binding_by_lport_ip = sbrec_mac_binding_by_lport_ip,
>              };
>
>              if (!state.had_lock && ovsdb_idl_has_lock(ovnsb_idl_loop.idl)) {
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index dcfa1fb5d..e1b3b516c 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -1010,6 +1010,15 @@ sb::Out_MAC_Binding (._uuid        = mb._uuid,
>      sb::Out_Port_Binding(.logical_port = mb.logical_port),
>      sb::Out_Datapath_Binding(._uuid = mb.datapath).
>
> +sb::Out_MAC_Binding (._uuid        = hash,
> +                    .logical_port = nb.logical_port,
> +                    .ip           = nb.ip,
> +                    .mac          = nb.mac,
> +                    .datapath     = router._uuid) :-
> +    nb in nb::MAC_Binding(),
> +    rp in &RouterPort(.router = router, .json_name = json_string_escape(nb.logical_port)),
> +    var hash = hash128((nb.logical_port, nb.ip)).
> +
>  /*
>   * DHCP options: fixed table
>   */
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 2ac8ef3ea..734d9649e 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.32.1",

This is not the latest version I guess.

> -    "cksum": "2805328215 29734",
> +    "version": "5.33.0",
> +    "cksum": "3031667641 30300",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -325,6 +325,12 @@
>                                       "refType": "strong"},
>                               "min": 0,
>                               "max": "unlimited"}},
> +                "mac_bindings": {
> +                    "type": {"key": {"type": "uuid",
> +                                     "refTable": "MAC_Binding",
> +                                     "refType": "strong"},
> +                             "min": 0,
> +                             "max": "unlimited"}},
>                  "enabled": {"type": {"key": "boolean", "min": 0, "max": 1}},
>                  "nat": {"type": {"key": {"type": "uuid",
>                                           "refTable": "NAT",
> @@ -575,5 +581,12 @@
>                      "type": {"key": "string", "value": "string",
>                               "min": 0, "max": "unlimited"}}},
>              "indexes": [["logical_port", "dst_ip"]],
> +            "isRoot": true},
> +        "MAC_Binding": {
> +            "columns": {
> +                "logical_port": {"type": "string"},
> +                "ip": {"type": "string"},
> +                "mac": {"type": "string"}},
> +            "indexes": [["logical_port", "ip"]],
>              "isRoot": true}}
>      }
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 390cc5a44..34252581a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -2043,6 +2043,10 @@
>        Zero or more routing policies for the router.
>      </column>
>
> +    <column name="mac_bindings">
> +      Zero or more statically configured mac_bindings for the router.
> +    </column>
> +
>      <column name="enabled">
>        This column is used to administratively set router state.  If this column
>        is empty or is set to <code>true</code>, the router is enabled.  If this
> @@ -4103,4 +4107,28 @@
>        </column>
>      </group>
>    </table>
> +
> +  <table name="MAC_Binding">
> +    <p>
> +      Each record represents a MAC Binding entry.
> +    </p>
> +
> +    <group title="Configuration">
> +      <p>
> +        <code>ovn-northd</code> reads configuration from these columns.
> +      </p>
> +
> +      <column name="logical_port">
> +        The logical port on which the binding was discovered.
> +      </column>
> +
> +      <column name="ip">
> +        The bound IP address.
> +      </column>
> +
> +      <column name="mac">
> +        The Ethernet address to which the IP is bound.
> +      </column>
> +    </group>
> +  </table>
>  </database>
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index 9b80ae410..7f6761abd 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -2057,6 +2057,70 @@ AT_CHECK([ovn-nbctl list forwarding_group], [0], [])
>
>  dnl ---------------------------------------------------------------------
>
> +OVN_NBCTL_TEST([ovn_nbctl_mac_binding], [lr mac_binding], [
> +
> +AT_CHECK([ovn-nbctl lr-add lr0])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24])
> +AT_CHECK([ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 foo 00:00:44:55:66:88], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.200 foo], [1], [],
> +  [ovn-nbctl: invalid mac address foo.
> +])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77], [1], [],
> +  [ovn-nbctl: lr0-p0, 172.16.0.11: a MAC_Binding with this logical_port and ip already exists
> +])
> +
> +AT_CHECK([ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 172.16.0.11 00:00:44:55:66:77])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66])
> +AT_CHECK([ovn-nbctl lr-mac-binding-add lr0 lr0-p1 172.16.0.11 00:00:44:55:66:88])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p0                   10.0.0.10                00:00:33:44:55:66
> +lr0-p0                   172.16.0.11              00:00:44:55:66:77
> +lr0-p0                   192.168.10.10            00:00:11:22:33:44
> +lr0-p0                   192.168.10.100           00:00:22:33:44:55
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0 foo], [1], [],
> +  [ovn-nbctl: foo: Not a valid IPv4 or IPv6 address.
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.100], [1], [],
> +  [ovn-nbctl: no matching MAC_Binding with the port (lr0-p1) and ip (10.0.0.100)
> +])
> +
> +AT_CHECK([ovn-nbctl --if-exists lr-mac-binding-del lr0 lr0-p1 10.0.0.100])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p0])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   10.0.0.10                00:00:33:44:55:66
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +AT_CHECK([ovn-nbctl lr-mac-binding-del lr0 lr0-p1 10.0.0.10])
> +AT_CHECK([ovn-nbctl lr-mac-binding-list lr0], [0], [dnl
> +LOGICAL_PORT             IP                       MAC
> +lr0-p1                   172.16.0.11              00:00:44:55:66:88
> +])
> +
> +])
> +
> +dnl ---------------------------------------------------------------------
> +
>  OVN_NBCTL_TEST([ovn_nbctl_negative], [basic negative tests], [
>  AT_CHECK([ovn-nbctl --id=@ls create logical_switch name=foo -- \
>            set logical_switch foo1 name=bar],
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 4957a1063..3a719cb06 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -28110,3 +28110,29 @@ grep lr0-sw1], [1], [])
>  OVN_CLEANUP([hv1])
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([LR NB MAC_Binding table])
> +ovn_start
> +
> +# Create logical routers
> +ovn-nbctl lr-add lr0
> +ovn-nbctl lrp-add lr0 lr0-p0 00:00:01:01:02:03 192.168.10.1/24
> +ovn-nbctl lrp-add lr0 lr0-p1 00:00:02:02:03:04 192.168.11.1/24
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.10 00:00:11:22:33:44
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:44:55
> +
> +wait_row_count nb:MAC_Binding 2 logical_port=lr0-p0
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.10 mac="00\:00\:11\:22\:33\:44"
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:44\:55"
> +
> +ovn-nbctl lr-mac-binding-add lr0 lr0-p1 10.0.0.10 00:00:33:44:55:66
> +wait_row_count nb:MAC_Binding 1 logical_port=lr0-p1
> +wait_row_count MAC_Binding 1 logical_port=lr0-p1 ip=10.0.0.10 mac="00\:00\:33\:44\:55\:66"
> +
> +ovn-nbctl --may-exist lr-mac-binding-add lr0 lr0-p0 192.168.10.100 00:00:22:33:55:66
> +wait_row_count MAC_Binding 1 logical_port=lr0-p0 ip=192.168.10.100 mac="00\:00\:22\:33\:55\:66"
> +
> +AT_CLEANUP
> +])
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index 10217dcd5..b0356d3aa 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -366,6 +366,15 @@ Policy commands:\n\
>                              remove policies from ROUTER\n\
>    lr-policy-list ROUTER     print policies for ROUTER\n\
>  \n\
> +MAC_Binding commands:\n\
> +  lr-mac-binding-add ROUTER LOGICAL_PORT IP MAC\n\
> +                                    add a MAC_Binding entry to ROUTER\n\
> +  lr-mac-binding-del ROUTER LOGICAL_PORT [IP]\n\
> +                                    remove MAC_Binding entry from ROUTER and\n\
> +                                    delete the MAC_Binding row\n\
> +  lr-mac-binding-list ROUTER        print MAC_Bindings for ROUTER\n\
> +\n\n",program_name, program_name);
> +    printf("\
>  NAT commands:\n\
>    [--stateless]\n\
>    [--portrange]\n\
> @@ -408,8 +417,7 @@ Connection commands:\n\
>    del-connection             delete the connections\n\
>    [--inactivity-probe=MSECS]\n\
>    set-connection TARGET...   set the list of connections to TARGET...\n\
> -\n\n",program_name, program_name);
> -    printf("\
> +\n\
>  SSL commands:\n\
>    get-ssl                     print the SSL configuration\n\
>    del-ssl                     delete the SSL configuration\n\
> @@ -5595,6 +5603,205 @@ nbctl_lrp_get_redirect_type(struct ctl_context *ctx)
>                    !redirect_type ? "overlay": redirect_type);
>  }
>
> +static void
> +nbctl_pre_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_add(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    const char *logical_port = ctx->argv[2];
> +    const char *ip = ctx->argv[3];
> +    const char *mac = ctx->argv[4];
> +    char *new_ip = NULL;
> +
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const struct nbrec_logical_router_port *lrp;
> +    error = lrp_by_name_or_uuid(ctx, logical_port, true, &lrp);
> +    if (error) {
> +        ctx->error = error;
> +        goto cleanup;
> +    }
> +
> +    new_ip = normalize_addr_str(ip);
> +    if (!new_ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ip);
> +        return;
> +    }
> +
> +    struct eth_addr ea;
> +    if (!eth_addr_from_string(mac, &ea)) {
> +        ctl_error(ctx, "invalid mac address %s.", mac);
> +        goto cleanup;
> +    }
> +
> +    bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL;
> +
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +
> +        char *old_ip;
> +        bool should_return = false;
> +        old_ip = normalize_addr_str(mac_binding->ip);
> +
> +        if (!strcmp(mac_binding->logical_port, logical_port)) {
> +            if (!strcmp(old_ip, new_ip)) {
> +                if (may_exist) {
> +                    nbrec_mac_binding_verify_mac(mac_binding);
> +                    nbrec_mac_binding_set_mac(mac_binding, mac);
> +                    should_return = true;
> +                } else {
> +                    ctl_error(ctx, "%s, %s: a MAC_Binding with this "
> +                              "logical_port and ip already "
> +                              "exists", logical_port, new_ip);
> +                    should_return = true;
> +                }
> +            }
> +        }
> +        free(old_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    /* Create MAC_Binding entry */
> +    struct nbrec_mac_binding *mac_binding = nbrec_mac_binding_insert(ctx->txn);
> +    nbrec_mac_binding_set_logical_port(mac_binding, logical_port);
> +    nbrec_mac_binding_set_ip(mac_binding, new_ip);
> +    nbrec_mac_binding_set_mac(mac_binding, mac);
> +
> +    /* Insert MAC_Binding entry into logical router */
> +    nbrec_logical_router_update_mac_bindings_addvalue(lr, mac_binding);
> +
> +cleanup:
> +    free(new_ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_del(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr = NULL;
> +    bool must_exist = !shash_find(&ctx->options, "--if-exists");
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    const char *logical_port = ctx->argv[2];
> +    if (ctx->argc == 3) {
> +        /* Deletes all NB MAC_binding entries on the given logical port. */
> +        for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +            if (!strcmp(logical_port, lr->mac_bindings[i]->logical_port)) {
> +                struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +                nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +                nbrec_mac_binding_delete(mac_binding);
> +            }
> +        }
> +        return;
> +    }
> +
> +    char *ip = normalize_addr_str(ctx->argv[3]);
> +    if (!ip) {
> +        ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ctx->argv[3]);
> +        return;
> +    }
> +
> +    /* Remove the matching MAC_Binding. */
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        bool should_return = false;
> +        char *mac_binding_ip = normalize_addr_str(mac_binding->ip);
> +        if (!mac_binding_ip) {
> +            continue;
> +        }
> +        if (!strcmp(ip, mac_binding_ip)) {
> +            nbrec_logical_router_update_mac_bindings_delvalue(lr, mac_binding);
> +            nbrec_mac_binding_delete(mac_binding);
> +            should_return = true;
> +        }
> +        free(mac_binding_ip);
> +        if (should_return) {
> +            goto cleanup;
> +        }
> +    }
> +
> +    if (must_exist) {
> +        ctl_error(ctx, "no matching MAC_Binding with the port (%s) and ip (%s)",
> +                  logical_port, ip);
> +    }
> +
> +cleanup:
> +    free(ip);
> +}
> +
> +static void
> +nbctl_pre_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_mac_bindings);
> +
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_logical_port);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_ip);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_mac_binding_col_mac);
> +}
> +
> +static void
> +nbctl_lr_mac_binding_list(struct ctl_context *ctx)
> +{
> +    const struct nbrec_logical_router *lr;
> +    char *error = lr_by_name_or_uuid(ctx, ctx->argv[1], true, &lr);
> +    if (error) {
> +        ctx->error = error;
> +        return;
> +    }
> +
> +    struct smap lr_mac_bindings = SMAP_INITIALIZER(&lr_mac_bindings);
> +    for (size_t i = 0; i < lr->n_mac_bindings; i++) {
> +        const struct nbrec_mac_binding *mac_binding = lr->mac_bindings[i];
> +        char *key = xasprintf("%-25s%-25s", mac_binding->logical_port,
> +                              mac_binding->ip);
> +        smap_add_format(&lr_mac_bindings, key, "%s", mac_binding->mac);
> +        free(key);
> +    }
> +
> +    const struct smap_node **nodes = smap_sort(&lr_mac_bindings);
> +    if (nodes) {
> +        ds_put_format(&ctx->output, "%-25s%-25s%s\n",
> +                      "LOGICAL_PORT", "IP", "MAC");
> +        for (size_t i = 0; i < smap_count(&lr_mac_bindings); i++) {
> +            const struct smap_node *node = nodes[i];
> +            ds_put_format(&ctx->output, "%-25s%s\n", node->key, node->value);
> +        }
> +    }
> +}
> +
>  static const struct nbrec_forwarding_group *
>  fwd_group_by_name_or_uuid(struct ctl_context *ctx, const char *id)
>  {
> @@ -6958,6 +7165,17 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>      { "lr-policy-list", 1, 1, "ROUTER", nbctl_pre_lr_policy_list,
>        nbctl_lr_policy_list, NULL, "", RO },
>
> +    /* MAC_Binding commands */
> +    { "lr-mac-binding-add", 4, 4,
> +      "ROUTER LOGICAL_PORT IP MAC",
> +      nbctl_pre_lr_mac_binding_add, nbctl_lr_mac_binding_add, NULL,
> +      "--may-exist", RW },
> +    { "lr-mac-binding-del", 2, 3, "ROUTER LOGICAL_PORT [IP]",
> +      nbctl_pre_lr_mac_binding_del, nbctl_lr_mac_binding_del,
> +      NULL, "--if-exists", RW },
> +    { "lr-mac-binding-list", 1, 1, "ROUTER", nbctl_pre_lr_mac_binding_list,
> +      nbctl_lr_mac_binding_list, NULL, "", RO },
> +
>      /* NAT commands. */
>      { "lr-nat-add", 4, 7,
>        "ROUTER TYPE EXTERNAL_IP LOGICAL_IP"
> --
> 2.22.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org<mailto:dev@openvswitch.org>
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>