diff mbox series

[ovs-dev,v2] ovn-nbctl: add the capability to specify CoPP UUID or CoPP name

Message ID 9f54fbecc2e8647fb5218fcadf89bbd810662b1f.1642764193.git.lorenzo.bianconi@redhat.com
State Changes Requested
Headers show
Series [ovs-dev,v2] ovn-nbctl: add the capability to specify CoPP UUID or CoPP name | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Lorenzo Bianconi Jan. 21, 2022, 11:25 a.m. UTC
Introduce the capability to specify CoPP UUID or CoPP name in order to
reuse the same CoPP reference on multiple datapaths.
Introduce logical_router and logical_switches columns in CoPP table in
order to specify datapaths where CoPP is installed.
Add external_ids column in CoPP table.

Reported-ad: https://bugzilla.redhat.com/show_bug.cgi?id=2040852
Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
---
Changes since v1:
- Use name as table index
- make name mandatory
- add external_ids column
---
 ovn-nb.ovsschema          | 21 +++++++--
 ovn-nb.xml                | 12 +++++
 tests/ovn-controller.at   |  6 +--
 tests/ovn-northd.at       | 58 +++++++++++++++++++----
 tests/ovn.at              |  2 +-
 tests/system-ovn.at       |  8 ++--
 utilities/ovn-nbctl.8.xml | 12 +++--
 utilities/ovn-nbctl.c     | 99 +++++++++++++++++++++++++++++++++------
 8 files changed, 179 insertions(+), 39 deletions(-)

Comments

Dumitru Ceara Jan. 24, 2022, 4:08 p.m. UTC | #1
On 1/21/22 12:25, Lorenzo Bianconi wrote:
> Introduce the capability to specify CoPP UUID or CoPP name in order to
> reuse the same CoPP reference on multiple datapaths.
> Introduce logical_router and logical_switches columns in CoPP table in
> order to specify datapaths where CoPP is installed.
> Add external_ids column in CoPP table.
> 
> Reported-ad: https://bugzilla.redhat.com/show_bug.cgi?id=2040852
> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com>
> ---

Hi Lorenzo,

> Changes since v1:
> - Use name as table index
> - make name mandatory
> - add external_ids column
> ---
>  ovn-nb.ovsschema          | 21 +++++++--
>  ovn-nb.xml                | 12 +++++
>  tests/ovn-controller.at   |  6 +--
>  tests/ovn-northd.at       | 58 +++++++++++++++++++----
>  tests/ovn.at              |  2 +-
>  tests/system-ovn.at       |  8 ++--
>  utilities/ovn-nbctl.8.xml | 12 +++--
>  utilities/ovn-nbctl.c     | 99 +++++++++++++++++++++++++++++++++------
>  8 files changed, 179 insertions(+), 39 deletions(-)
> 
> diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
> index 55977339a..192036ad5 100644
> --- a/ovn-nb.ovsschema
> +++ b/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>      "name": "OVN_Northbound",
> -    "version": "5.34.1",
> -    "cksum": "2177334725 30782",
> +    "version": "5.35.0",

This is a backwards-incompatible change, I think we should change the
version to:
6.0.0

> +    "cksum": "3892116111 31629",
>      "tables": {
>          "NB_Global": {
>              "columns": {
> @@ -32,11 +32,26 @@
>              "isRoot": true},
>          "Copp": {
>              "columns": {
> +                "name": {"type": "string"},
> +                "logical_switch": {"type": {"key": {"type": "uuid",
> +                                          "refTable": "Logical_Switch",
> +                                          "refType": "strong"},
> +                                  "min": 0,
> +                                  "max": "unlimited"}},
> +                "logical_router": {"type": {"key": {"type": "uuid",
> +                                          "refTable": "Logical_Router",
> +                                          "refType": "strong"},
> +                                  "min": 0,
> +                                  "max": "unlimited"}},
>                  "meters": {
>                      "type": {"key": "string",
>                               "value": "string",
>                               "min": 0,
> -                             "max": "unlimited"}}},
> +                             "max": "unlimited"}},
> +                "external_ids": {
> +                    "type": {"key": "string", "value": "string",
> +                             "min": 0, "max": "unlimited"}}},
> +            "indexes": [["name"]],
>              "isRoot": true},
>          "Logical_Switch": {
>              "columns": {
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index 6a6972856..e327c92ee 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -360,6 +360,15 @@
>        associate entries from table <ref table="Meter"/> to control protocol
>        names.
>      </p>
> +    <column name="name">
> +      CoPP name.
> +    </column>
> +    <column name="logical_switch">
> +      Reference to <ref table="Logical_Switch"/> where the CoPP is installed.
> +    </column>
> +    <column name="logical_router">
> +      Reference to <ref table="Logical_Router"/> where the CoPP is installed.
> +    </column>
>      <column name="meters" key="arp">
>        Rate limiting meter for ARP packets (request/reply) used for learning
>        neighbors.
> @@ -417,6 +426,9 @@
>      <column name="meters" key="reject">
>        Rate limiting meter for packets that trigger a reject action
>      </column>
> +    <column name="external_ids">
> +      See <em>External IDs</em> at the beginning of this document.
> +    </column>
>    </table>
>  
>    <table name="Logical_Switch" title="L2 logical switch">
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 2f39e5f3e..6a5880501 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -713,19 +713,19 @@ check ovn-nbctl ls-lb-add ls1 lb1
>  
>  # controller-event metering
>  check ovn-nbctl meter-add event-elb drop 100 pktps 10
> -check ovn-nbctl --wait=hv ls-copp-add ls1 event-elb event-elb
> +check ovn-nbctl --wait=hv ls-copp-add copp0 ls1 event-elb event-elb
>  
>  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.0f | grep -q meter_id=1])
>  
>  # reject metering
>  check ovn-nbctl meter-add acl-meter drop 1 pktps 0
> -check ovn-nbctl ls-copp-add ls1 reject acl-meter
> +check ovn-nbctl ls-copp-add copp1 ls1 reject acl-meter
>  check ovn-nbctl --wait=hv acl-add ls1 from-lport 1002 'inport == "lsp1" && ip && udp' reject
>  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.16 | grep -q meter_id=2])
>  
>  # arp metering
>  check ovn-nbctl meter-add arp-meter drop 200 pktps 0
> -check ovn-nbctl --wait=hv lr-copp-add lr1 arp-resolve arp-meter
> +check ovn-nbctl --wait=hv lr-copp-add copp2 lr1 arp-resolve arp-meter
>  AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.00 | grep -q meter_id=3])
>  
>  OVN_CLEANUP([hv1])
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 652903761..5c7587c3e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3287,7 +3287,7 @@ check ovn-nbctl --wait=sb lsp-set-addresses sw1-r0 00:00:00:00:00:01
>  check ovn-nbctl --event lb-add lb0 192.168.1.100:80 ""
>  check ovn-nbctl ls-lb-add sw1 lb0
>  check ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10
> -check ovn-nbctl --wait=hv ls-copp-add sw1 event-elb meter0
> +check ovn-nbctl --wait=hv ls-copp-add copp0 sw1 event-elb meter0
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  event-elb: meter0
>  ])
> @@ -3304,7 +3304,7 @@ AT_CHECK([ovn-nbctl meter-list |grep meter1 -A 1], [0], [dnl
>  meter1: bands:
>    drop: 200 pktps, 10 packet burst
>  ])
> -check ovn-nbctl --wait=hv lr-copp-add r0 arp meter1
> +check ovn-nbctl --wait=hv lr-copp-add copp1 r0 arp meter1
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  arp: meter1
>  ])
> @@ -3318,7 +3318,7 @@ AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  AT_CHECK([ovn-sbctl list logical_flow | grep arp -A 2 | grep -q meter1],[1])
>  
>  check ovn-nbctl --wait=hv meter-add meter2 drop 400 pktps 10
> -check ovn-nbctl --wait=hv lr-copp-add r0 icmp4-error meter2
> +check ovn-nbctl --wait=hv lr-copp-add copp2 r0 icmp4-error meter2
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  icmp4-error: meter2
>  ])
> @@ -3329,7 +3329,7 @@ check ovn-nbctl --wait=hv lr-copp-del r0 icmp4-error
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  ])
>  
> -check ovn-nbctl --wait=hv lr-copp-add r0 icmp6-error meter2
> +check ovn-nbctl --wait=hv lr-copp-add copp3 r0 icmp6-error meter2
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  icmp6-error: meter2
>  ])
> @@ -3340,7 +3340,7 @@ check ovn-nbctl --wait=hv lr-copp-del r0 icmp6-error
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  ])
>  
> -check ovn-nbctl --wait=hv lr-copp-add r0 tcp-reset meter2
> +check ovn-nbctl --wait=hv lr-copp-add copp4 r0 tcp-reset meter2
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  tcp-reset: meter2
>  ])
> @@ -3358,21 +3358,21 @@ AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  AT_CHECK([ovn-sbctl list logical_flow | grep trigger_event -A 2 | grep -q meter0],[1])
>  
>  # let's try to add an usupported protocol "dhcp"
> -AT_CHECK([ovn-nbctl --wait=hv ls-copp-add sw1 dhcp meter1],[1],[],[dnl
> +AT_CHECK([ovn-nbctl --wait=hv ls-copp-add copp5 sw1 dhcp meter1],[1],[],[dnl
>  ovn-nbctl: Invalid control protocol. Allowed values: arp, arp-resolve, dhcpv4-opts, dhcpv6-opts, dns, event-elb, icmp4-error, icmp6-error, igmp, nd-na, nd-ns, nd-ns-resolve, nd-ra-opts, tcp-reset, bfd, reject.
>  ])
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  ])
>  
>  #Let's try to add a valid protocol to an unknown datapath
> -AT_CHECK([ovn-nbctl --wait=hv ls-copp-add sw10 arp meter1],[1],[],[dnl
> +AT_CHECK([ovn-nbctl --wait=hv ls-copp-add copp6 sw10 arp meter1],[1],[],[dnl
>  ovn-nbctl: sw10: switch name not found
>  ])
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  ])
>  
>  check ovn-nbctl --bfd lr-route-add r0 240.0.0.0/8 192.168.50.2 r0-sw1
> -check ovn-nbctl --wait=hv lr-copp-add r0 bfd meter0
> +check ovn-nbctl --wait=hv lr-copp-add copp7 r0 bfd meter0
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  bfd: meter0
>  ])
> @@ -3381,14 +3381,14 @@ AT_CHECK([ovn-sbctl list logical_flow | grep bfd -A 2 | grep -q meter0])
>  check ovn-nbctl --wait=hv set Logical_Switch sw1 \
>      other_config:mcast_querier="false" \
>      other_config:mcast_snoop="true"
> -check ovn-nbctl --wait=hv ls-copp-add sw1 igmp meter1
> +check ovn-nbctl --wait=hv ls-copp-add copp8 sw1 igmp meter1
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  igmp: meter1
>  ])
>  AT_CHECK([ovn-sbctl list logical_flow | grep igmp -A 2 | grep -q meter1])
>  
>  # let's add igmp meter1 twice
> -AT_CHECK([ovn-nbctl --wait=hv ls-copp-add sw1 igmp meter1])
> +AT_CHECK([ovn-nbctl --wait=hv ls-copp-add copp9 sw1 igmp meter1])
>  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
>  igmp: meter1
>  ])
> @@ -3403,6 +3403,44 @@ check ovn-nbctl lr-copp-del r0
>  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
>  ])
>  
> +check ovn-nbctl ls-copp-del sw1
> +AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
> +])
> +
> +check ovn-nbctl --wait=hv lr-copp-add copp0 r0 arp meter0
> +AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
> +arp: meter0
> +])
> +
> +AT_CHECK([fetch_column nb:CoPP name], [0], [dnl
> +copp0
> +])
> +
> +lr_uuid=$(fetch_column nb:Logical_Router _uuid)
> +copp_lr_uuid=$(fetch_column nb:CoPP logical_router)
> +AT_CHECK([test "$lr_uuid" = "$copp_lr_uuid"])
> +
> +copp_uuid=$(fetch_column nb:CoPP _uuid)
> +check ovn-nbctl --wait=hv ls-copp-add $copp_uuid sw1 arp meter0
> +
> +ls_uuid=$(fetch_column nb:Logical_Switch _uuid)
> +copp_ls_uuid=$(fetch_column nb:CoPP logical_switch)
> +AT_CHECK([test "$ls_uuid" = "$copp_ls_uuid"])
> +
> +ls_copp_uuid=$(fetch_column nb:Logical_Switch copp)
> +AT_CHECK([test "$ls_copp_uuid" = "$copp_uuid"])
> +
> +check ovn-nbctl --wait=sb lrp-add r0 r0-sw2 00:00:00:00:00:03 192.168.2.1/24
> +check ovn-nbctl --wait=sb ls-add sw2
> +check ovn-nbctl --wait=sb lsp-add sw2 sw2-r0
> +check ovn-nbctl --wait=sb lsp-set-type sw2-r0 router
> +check ovn-nbctl --wait=sb lsp-set-options sw2-r0 router-port=r0-sw2
> +check ovn-nbctl --wait=sb lsp-set-addresses sw2-r0 00:00:00:00:00:02
> +
> +check ovn-nbctl --wait=hv ls-copp-add copp0 sw2 event-elb meter0
> +ls2_copp_uuid=$(ovn-nbctl get Logical_Switch sw2 copp)
> +AT_CHECK([test "$ls2_copp_uuid" = "$copp_uuid"])
> +
>  AT_CLEANUP
>  ])
>  
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 92e284e8a..8de540e57 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -18932,7 +18932,7 @@ ovn-nbctl ls-lb-add sw0 lb2
>  uuid_lb2=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb2)
>  
>  ovn-nbctl --wait=hv meter-add event-elb drop 100 pktps 10
> -ovn-nbctl --wait=hv ls-copp-add sw0 event-elb event-elb
> +ovn-nbctl --wait=hv ls-copp-add copp0 sw0 event-elb event-elb
>  
>  OVN_POPULATE_ARP
>  wait_for_ports_up
> diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> index 3ae812296..b384a2204 100644
> --- a/tests/system-ovn.at
> +++ b/tests/system-ovn.at
> @@ -6642,7 +6642,7 @@ check ovn-nbctl lsp-add public public1 \
>  
>  NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
>  check ovn-nbctl meter-add acl-meter drop 1 pktps 0
> -check ovn-nbctl --wait=hv ls-copp-add sw0 reject acl-meter
> +check ovn-nbctl --wait=hv ls-copp-add copp0 sw0 reject acl-meter
>  check ovn-nbctl acl-add sw0 from-lport 1002 'inport == "sw01" && ip && udp' reject
>  
>  AT_CHECK([ovn-nbctl ls-copp-list sw0], [0], [dnl
> @@ -6678,7 +6678,7 @@ kill $(pidof tcpdump)
>  
>  NS_EXEC([server], [tcpdump -l -n -i s1 arp[[24:4]]=0xac100164 > arp.pcap &])
>  check ovn-nbctl meter-add arp-meter drop 1 pktps 0
> -check ovn-nbctl --wait=hv lr-copp-add R1 arp-resolve arp-meter
> +check ovn-nbctl --wait=hv lr-copp-add copp1 R1 arp-resolve arp-meter
>  AT_CHECK([ovn-nbctl lr-copp-list R1], [0], [dnl
>  arp-resolve: arp-meter
>  ])
> @@ -6696,7 +6696,7 @@ OVS_WAIT_UNTIL([
>  kill $(pidof tcpdump)
>  
>  check ovn-nbctl meter-add icmp-meter drop 1 pktps 0
> -check ovn-nbctl --wait=hv lr-copp-add R1 icmp4-error icmp-meter
> +check ovn-nbctl --wait=hv lr-copp-add copp2 R1 icmp4-error icmp-meter
>  AT_CHECK([ovn-nbctl lr-copp-list R1 |grep icmp4-error], [0], [dnl
>  icmp4-error: icmp-meter
>  ])
> @@ -6715,7 +6715,7 @@ OVS_WAIT_UNTIL([
>  kill $(pidof tcpdump)
>  
>  check ovn-nbctl meter-add bfd-meter drop 1 pktps 0
> -check ovn-nbctl --wait=hv lr-copp-add R1 bfd bfd-meter
> +check ovn-nbctl --wait=hv lr-copp-add copp3 R1 bfd bfd-meter
>  AT_CHECK([ovn-nbctl lr-copp-list R1 |grep bfd], [0], [dnl
>  bfd: bfd-meter
>  ])
> diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
> index 80a564660..b944d7e08 100644
> --- a/utilities/ovn-nbctl.8.xml
> +++ b/utilities/ovn-nbctl.8.xml
> @@ -1474,13 +1474,15 @@
>      </p>
>  
>      <dl>
> -      <dt><code>ls-copp-add</code> <var>switch</var> <var>proto</var>
> -      <var>meter</var></dt>
> +      <dt><code>ls-copp-add</code> [<var>UUID</var>|<var>name</var>]
> +      <var>switch</var> <var>proto</var> <var>meter</var></dt>
>        <dd>
>          Adds the control <code>proto</code> to <code>meter</code> mapping
>          to the <code>switch</code> control plane protection policy. If no
>          policy exists yet, it creates one. If a mapping already existed for
>          <code>proto</code>, this will overwrite it.
> +        If <var>UUID</var> <var>name</var> is provided, the already installed

Nit: "If UUID or name is provided.."

> +        CoPP will be reused.
>        </dd>
>  
>        <dt><code>ls-copp-del</code> <var>switch</var> [<var>proto</var>]</dt>
> @@ -1497,13 +1499,15 @@
>          <code>switch</code>.
>        </dd>
>  
> -      <dt><code>lr-copp-add</code> <var>router</var> <var>proto</var>
> -      <var>meter</var></dt>
> +      <dt><code>lr-copp-add</code> [<var>UUID</var>|<var>name</var>]
> +      <var>router</var> <var>proto</var> <var>meter</var></dt>
>        <dd>
>          Adds the control <code>proto</code> to <code>meter</code> mapping
>          to the <code>router</code> control plane protection policy. If no
>          policy exists yet, it creates one. If a mapping already existed for
>          <code>proto</code>, this will overwrite it.
> +        If <var>UUID</var> <var>name</var> is provided, the already installed
> +        CoPP will be reused.

Same here.

>        </dd>
>  
>        <dt><code>lr-copp-del</code> <var>router</var> [<var>proto</var>]</dt>
> diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
> index d67d2db65..3a89da030 100644
> --- a/utilities/ovn-nbctl.c
> +++ b/utilities/ovn-nbctl.c
> @@ -437,7 +437,7 @@ chassis with mandatory PRIORITY to the HA chassis group GRP\n\
>  CHASSIS from the HA chassis group GRP\n\
>  \n\
>  Control Plane Protection Policy commands:\n\
> -  ls-copp-add SWITCH PROTO METER\n\
> +  ls-copp-add [UUID|NAME] SWITCH PROTO METER\n\
>                              Add a copp policy for PROTO packets on SWITCH\n\
>                              based on an existing METER.\n\
>    ls-copp-del SWITCH [PROTO]\n\
> @@ -447,7 +447,7 @@ Control Plane Protection Policy commands:\n\
>    ls-copp-list SWITCH\n\
>                              List all copp policies defined for control\n\
>                              protocols on SWITCH.\n\
> -  lr-copp-add ROUTER PROTO METER\n\
> +  lr-copp-add [UUID|NAME] ROUTER PROTO METER\n\
>                              Add a copp policy for PROTO packets on ROUTER\n\
>                              based on an existing METER.\n\
>    lr-copp-del ROUTER [PROTO]\n\
> @@ -6278,6 +6278,9 @@ nbctl_pre_copp(struct ctl_context *ctx)
>  {
>      nbctl_pre_context(ctx);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_meters);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_logical_switch);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_logical_router);
> +    ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_name);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_col_copp);
>      ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_copp);
>  }
> @@ -6285,9 +6288,29 @@ nbctl_pre_copp(struct ctl_context *ctx)
>  static void
>  nbctl_ls_copp_add(struct ctl_context *ctx)
>  {
> -    const char *ls_name = ctx->argv[1];
> -    const char *proto_name = ctx->argv[2];
> -    const char *meter = ctx->argv[3];
> +    const struct nbrec_copp *copp = NULL;
> +    const char *proto_name = ctx->argv[3];
> +    const char *ls_name = ctx->argv[2];
> +    const char *meter = ctx->argv[4];
> +    const char *copp_name = NULL;
> +    struct uuid uuid;
> +
> +    if (uuid_from_string(&uuid, ctx->argv[1])) {
> +        copp = nbrec_copp_get_for_uuid(ctx->idl, &uuid);
> +        if (!copp) {
> +            ctx->error = xasprintf("copp %s not found.", ctx->argv[1]);
> +            return;
> +        }
> +    } else {
> +        copp_name = ctx->argv[1];
> +        const struct nbrec_copp *iter;
> +        NBREC_COPP_FOR_EACH (iter, ctx->idl) {
> +            if (!strcmp(iter->name, copp_name)) {
> +                copp = iter;
> +                break;
> +            }
> +        }
> +    }

We can factor out this part into a helper function, e.g.,
copp_by_name_or_uuid(), similar to ls_by_name_or_uuid().

>  
>      char *error = copp_proto_validate(proto_name);
>      if (error) {
> @@ -6302,9 +6325,23 @@ nbctl_ls_copp_add(struct ctl_context *ctx)
>          return;
>      }
>  
> -    const struct nbrec_copp *copp =
> -        copp_meter_add(ctx, ls->copp, proto_name, meter);
> +    if (!copp) {
> +        copp = copp_meter_add(ctx, ls->copp, proto_name, meter);
> +        if (copp_name) {
> +            nbrec_copp_set_name(copp, copp_name);
> +        }
> +    }
>      nbrec_logical_switch_set_copp(ls, copp);
> +
> +    size_t n_logical_switch = copp->n_logical_switch + 1;
> +    struct nbrec_logical_switch **ls_list =
> +        xmalloc(n_logical_switch * sizeof *ls_list);
> +    for (int i = 0; i < copp->n_logical_switch; i++) {
> +        ls_list[i] = copp->logical_switch[i];
> +    }
> +    ls_list[copp->n_logical_switch] = (struct nbrec_logical_switch *)ls;
> +    nbrec_copp_set_logical_switch(copp, ls_list, n_logical_switch);

We can use nbrec_logical_switch_update_copp_addvalue() instead.

> +    free(ls_list);
>  }
>  
>  static void
> @@ -6351,9 +6388,29 @@ nbctl_ls_copp_list(struct ctl_context *ctx)
>  static void
>  nbctl_lr_copp_add(struct ctl_context *ctx)
>  {
> -    const char *lr_name = ctx->argv[1];
> -    const char *proto_name = ctx->argv[2];
> -    const char *meter = ctx->argv[3];
> +    const struct nbrec_copp *copp = NULL;
> +    const char *proto_name = ctx->argv[3];
> +    const char *lr_name = ctx->argv[2];
> +    const char *meter = ctx->argv[4];
> +    const char *copp_name = NULL;
> +    struct uuid uuid;
> +
> +    if (uuid_from_string(&uuid, ctx->argv[1])) {
> +        copp = nbrec_copp_get_for_uuid(ctx->idl, &uuid);
> +        if (!copp) {
> +            ctx->error = xasprintf("copp %s not found.", ctx->argv[1]);
> +            return;
> +        }
> +    } else {
> +        copp_name = ctx->argv[1];
> +        const struct nbrec_copp *iter;
> +        NBREC_COPP_FOR_EACH (iter, ctx->idl) {
> +            if (!strcmp(iter->name, copp_name)) {
> +                copp = iter;
> +                break;
> +            }
> +        }
> +    }

We could use copp_by_name_or_uuid() here.

>  
>      char *error = copp_proto_validate(proto_name);
>      if (error) {
> @@ -6368,9 +6425,23 @@ nbctl_lr_copp_add(struct ctl_context *ctx)
>          return;
>      }
>  
> -    const struct nbrec_copp *copp =
> -        copp_meter_add(ctx, lr->copp, proto_name, meter);
> +    if (!copp) {
> +        copp = copp_meter_add(ctx, lr->copp, proto_name, meter);
> +        if (copp_name) {
> +            nbrec_copp_set_name(copp, copp_name);
> +        }
> +    }
>      nbrec_logical_router_set_copp(lr, copp);
> +
> +    size_t n_logical_router = copp->n_logical_router + 1;
> +    struct nbrec_logical_router **lr_list =
> +        xmalloc(n_logical_router * sizeof *lr_list);
> +    for (int i = 0; i < copp->n_logical_router; i++) {
> +        lr_list[i] = copp->logical_router[i];
> +    }
> +    lr_list[copp->n_logical_router] = (struct nbrec_logical_router *)lr;
> +    nbrec_copp_set_logical_router(copp, lr_list, n_logical_router);
> +    free(lr_list);
>  }

We can use nbrec_logical_router_update_copp_addvalue() instead.

>  
>  static void
> @@ -7177,13 +7248,13 @@ static const struct ctl_command_syntax nbctl_commands[] = {
>       NULL, "", RO },
>  
>      /* Control plane protection commands */
> -    {"ls-copp-add", 3, 3, "SWITCH PROTO METER", nbctl_pre_copp,
> +    {"ls-copp-add", 4, 4, "SWITCH PROTO METER", nbctl_pre_copp,
>        nbctl_ls_copp_add, NULL, "", RW},
>      {"ls-copp-del", 1, 2, "SWITCH [PROTO]", nbctl_pre_copp,
>        nbctl_ls_copp_del, NULL, "", RW},
>      {"ls-copp-list", 1, 1, "SWITCH", nbctl_pre_copp, nbctl_ls_copp_list,
>        NULL, "", RO},
> -    {"lr-copp-add", 3, 3, "ROUTER PROTO METER", nbctl_pre_copp,
> +    {"lr-copp-add", 4, 4, "ROUTER PROTO METER", nbctl_pre_copp,
>       nbctl_lr_copp_add, NULL, "", RW},
>      {"lr-copp-del", 1, 2, "ROUTER [PROTO]", nbctl_pre_copp,
>       nbctl_lr_copp_del, NULL, "", RW},
> 

Thanks,
Dumitru
diff mbox series

Patch

diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema
index 55977339a..192036ad5 100644
--- a/ovn-nb.ovsschema
+++ b/ovn-nb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Northbound",
-    "version": "5.34.1",
-    "cksum": "2177334725 30782",
+    "version": "5.35.0",
+    "cksum": "3892116111 31629",
     "tables": {
         "NB_Global": {
             "columns": {
@@ -32,11 +32,26 @@ 
             "isRoot": true},
         "Copp": {
             "columns": {
+                "name": {"type": "string"},
+                "logical_switch": {"type": {"key": {"type": "uuid",
+                                          "refTable": "Logical_Switch",
+                                          "refType": "strong"},
+                                  "min": 0,
+                                  "max": "unlimited"}},
+                "logical_router": {"type": {"key": {"type": "uuid",
+                                          "refTable": "Logical_Router",
+                                          "refType": "strong"},
+                                  "min": 0,
+                                  "max": "unlimited"}},
                 "meters": {
                     "type": {"key": "string",
                              "value": "string",
                              "min": 0,
-                             "max": "unlimited"}}},
+                             "max": "unlimited"}},
+                "external_ids": {
+                    "type": {"key": "string", "value": "string",
+                             "min": 0, "max": "unlimited"}}},
+            "indexes": [["name"]],
             "isRoot": true},
         "Logical_Switch": {
             "columns": {
diff --git a/ovn-nb.xml b/ovn-nb.xml
index 6a6972856..e327c92ee 100644
--- a/ovn-nb.xml
+++ b/ovn-nb.xml
@@ -360,6 +360,15 @@ 
       associate entries from table <ref table="Meter"/> to control protocol
       names.
     </p>
+    <column name="name">
+      CoPP name.
+    </column>
+    <column name="logical_switch">
+      Reference to <ref table="Logical_Switch"/> where the CoPP is installed.
+    </column>
+    <column name="logical_router">
+      Reference to <ref table="Logical_Router"/> where the CoPP is installed.
+    </column>
     <column name="meters" key="arp">
       Rate limiting meter for ARP packets (request/reply) used for learning
       neighbors.
@@ -417,6 +426,9 @@ 
     <column name="meters" key="reject">
       Rate limiting meter for packets that trigger a reject action
     </column>
+    <column name="external_ids">
+      See <em>External IDs</em> at the beginning of this document.
+    </column>
   </table>
 
   <table name="Logical_Switch" title="L2 logical switch">
diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
index 2f39e5f3e..6a5880501 100644
--- a/tests/ovn-controller.at
+++ b/tests/ovn-controller.at
@@ -713,19 +713,19 @@  check ovn-nbctl ls-lb-add ls1 lb1
 
 # controller-event metering
 check ovn-nbctl meter-add event-elb drop 100 pktps 10
-check ovn-nbctl --wait=hv ls-copp-add ls1 event-elb event-elb
+check ovn-nbctl --wait=hv ls-copp-add copp0 ls1 event-elb event-elb
 
 AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.0f | grep -q meter_id=1])
 
 # reject metering
 check ovn-nbctl meter-add acl-meter drop 1 pktps 0
-check ovn-nbctl ls-copp-add ls1 reject acl-meter
+check ovn-nbctl ls-copp-add copp1 ls1 reject acl-meter
 check ovn-nbctl --wait=hv acl-add ls1 from-lport 1002 'inport == "lsp1" && ip && udp' reject
 AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.16 | grep -q meter_id=2])
 
 # arp metering
 check ovn-nbctl meter-add arp-meter drop 200 pktps 0
-check ovn-nbctl --wait=hv lr-copp-add lr1 arp-resolve arp-meter
+check ovn-nbctl --wait=hv lr-copp-add copp2 lr1 arp-resolve arp-meter
 AT_CHECK([as hv1 ovs-ofctl dump-flows br-int | grep controller | grep userdata=00.00.00.00 | grep -q meter_id=3])
 
 OVN_CLEANUP([hv1])
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 652903761..5c7587c3e 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3287,7 +3287,7 @@  check ovn-nbctl --wait=sb lsp-set-addresses sw1-r0 00:00:00:00:00:01
 check ovn-nbctl --event lb-add lb0 192.168.1.100:80 ""
 check ovn-nbctl ls-lb-add sw1 lb0
 check ovn-nbctl --wait=hv meter-add meter0 drop 100 pktps 10
-check ovn-nbctl --wait=hv ls-copp-add sw1 event-elb meter0
+check ovn-nbctl --wait=hv ls-copp-add copp0 sw1 event-elb meter0
 AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
 event-elb: meter0
 ])
@@ -3304,7 +3304,7 @@  AT_CHECK([ovn-nbctl meter-list |grep meter1 -A 1], [0], [dnl
 meter1: bands:
   drop: 200 pktps, 10 packet burst
 ])
-check ovn-nbctl --wait=hv lr-copp-add r0 arp meter1
+check ovn-nbctl --wait=hv lr-copp-add copp1 r0 arp meter1
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 arp: meter1
 ])
@@ -3318,7 +3318,7 @@  AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 AT_CHECK([ovn-sbctl list logical_flow | grep arp -A 2 | grep -q meter1],[1])
 
 check ovn-nbctl --wait=hv meter-add meter2 drop 400 pktps 10
-check ovn-nbctl --wait=hv lr-copp-add r0 icmp4-error meter2
+check ovn-nbctl --wait=hv lr-copp-add copp2 r0 icmp4-error meter2
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 icmp4-error: meter2
 ])
@@ -3329,7 +3329,7 @@  check ovn-nbctl --wait=hv lr-copp-del r0 icmp4-error
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 ])
 
-check ovn-nbctl --wait=hv lr-copp-add r0 icmp6-error meter2
+check ovn-nbctl --wait=hv lr-copp-add copp3 r0 icmp6-error meter2
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 icmp6-error: meter2
 ])
@@ -3340,7 +3340,7 @@  check ovn-nbctl --wait=hv lr-copp-del r0 icmp6-error
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 ])
 
-check ovn-nbctl --wait=hv lr-copp-add r0 tcp-reset meter2
+check ovn-nbctl --wait=hv lr-copp-add copp4 r0 tcp-reset meter2
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 tcp-reset: meter2
 ])
@@ -3358,21 +3358,21 @@  AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
 AT_CHECK([ovn-sbctl list logical_flow | grep trigger_event -A 2 | grep -q meter0],[1])
 
 # let's try to add an usupported protocol "dhcp"
-AT_CHECK([ovn-nbctl --wait=hv ls-copp-add sw1 dhcp meter1],[1],[],[dnl
+AT_CHECK([ovn-nbctl --wait=hv ls-copp-add copp5 sw1 dhcp meter1],[1],[],[dnl
 ovn-nbctl: Invalid control protocol. Allowed values: arp, arp-resolve, dhcpv4-opts, dhcpv6-opts, dns, event-elb, icmp4-error, icmp6-error, igmp, nd-na, nd-ns, nd-ns-resolve, nd-ra-opts, tcp-reset, bfd, reject.
 ])
 AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
 ])
 
 #Let's try to add a valid protocol to an unknown datapath
-AT_CHECK([ovn-nbctl --wait=hv ls-copp-add sw10 arp meter1],[1],[],[dnl
+AT_CHECK([ovn-nbctl --wait=hv ls-copp-add copp6 sw10 arp meter1],[1],[],[dnl
 ovn-nbctl: sw10: switch name not found
 ])
 AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
 ])
 
 check ovn-nbctl --bfd lr-route-add r0 240.0.0.0/8 192.168.50.2 r0-sw1
-check ovn-nbctl --wait=hv lr-copp-add r0 bfd meter0
+check ovn-nbctl --wait=hv lr-copp-add copp7 r0 bfd meter0
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 bfd: meter0
 ])
@@ -3381,14 +3381,14 @@  AT_CHECK([ovn-sbctl list logical_flow | grep bfd -A 2 | grep -q meter0])
 check ovn-nbctl --wait=hv set Logical_Switch sw1 \
     other_config:mcast_querier="false" \
     other_config:mcast_snoop="true"
-check ovn-nbctl --wait=hv ls-copp-add sw1 igmp meter1
+check ovn-nbctl --wait=hv ls-copp-add copp8 sw1 igmp meter1
 AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
 igmp: meter1
 ])
 AT_CHECK([ovn-sbctl list logical_flow | grep igmp -A 2 | grep -q meter1])
 
 # let's add igmp meter1 twice
-AT_CHECK([ovn-nbctl --wait=hv ls-copp-add sw1 igmp meter1])
+AT_CHECK([ovn-nbctl --wait=hv ls-copp-add copp9 sw1 igmp meter1])
 AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
 igmp: meter1
 ])
@@ -3403,6 +3403,44 @@  check ovn-nbctl lr-copp-del r0
 AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
 ])
 
+check ovn-nbctl ls-copp-del sw1
+AT_CHECK([ovn-nbctl ls-copp-list sw1], [0], [dnl
+])
+
+check ovn-nbctl --wait=hv lr-copp-add copp0 r0 arp meter0
+AT_CHECK([ovn-nbctl lr-copp-list r0], [0], [dnl
+arp: meter0
+])
+
+AT_CHECK([fetch_column nb:CoPP name], [0], [dnl
+copp0
+])
+
+lr_uuid=$(fetch_column nb:Logical_Router _uuid)
+copp_lr_uuid=$(fetch_column nb:CoPP logical_router)
+AT_CHECK([test "$lr_uuid" = "$copp_lr_uuid"])
+
+copp_uuid=$(fetch_column nb:CoPP _uuid)
+check ovn-nbctl --wait=hv ls-copp-add $copp_uuid sw1 arp meter0
+
+ls_uuid=$(fetch_column nb:Logical_Switch _uuid)
+copp_ls_uuid=$(fetch_column nb:CoPP logical_switch)
+AT_CHECK([test "$ls_uuid" = "$copp_ls_uuid"])
+
+ls_copp_uuid=$(fetch_column nb:Logical_Switch copp)
+AT_CHECK([test "$ls_copp_uuid" = "$copp_uuid"])
+
+check ovn-nbctl --wait=sb lrp-add r0 r0-sw2 00:00:00:00:00:03 192.168.2.1/24
+check ovn-nbctl --wait=sb ls-add sw2
+check ovn-nbctl --wait=sb lsp-add sw2 sw2-r0
+check ovn-nbctl --wait=sb lsp-set-type sw2-r0 router
+check ovn-nbctl --wait=sb lsp-set-options sw2-r0 router-port=r0-sw2
+check ovn-nbctl --wait=sb lsp-set-addresses sw2-r0 00:00:00:00:00:02
+
+check ovn-nbctl --wait=hv ls-copp-add copp0 sw2 event-elb meter0
+ls2_copp_uuid=$(ovn-nbctl get Logical_Switch sw2 copp)
+AT_CHECK([test "$ls2_copp_uuid" = "$copp_uuid"])
+
 AT_CLEANUP
 ])
 
diff --git a/tests/ovn.at b/tests/ovn.at
index 92e284e8a..8de540e57 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -18932,7 +18932,7 @@  ovn-nbctl ls-lb-add sw0 lb2
 uuid_lb2=$(ovn-nbctl --bare --columns=_uuid find load_balancer name=lb2)
 
 ovn-nbctl --wait=hv meter-add event-elb drop 100 pktps 10
-ovn-nbctl --wait=hv ls-copp-add sw0 event-elb event-elb
+ovn-nbctl --wait=hv ls-copp-add copp0 sw0 event-elb event-elb
 
 OVN_POPULATE_ARP
 wait_for_ports_up
diff --git a/tests/system-ovn.at b/tests/system-ovn.at
index 3ae812296..b384a2204 100644
--- a/tests/system-ovn.at
+++ b/tests/system-ovn.at
@@ -6642,7 +6642,7 @@  check ovn-nbctl lsp-add public public1 \
 
 NS_EXEC([sw01], [tcpdump -l -n -i sw01 icmp -Q in > reject.pcap &])
 check ovn-nbctl meter-add acl-meter drop 1 pktps 0
-check ovn-nbctl --wait=hv ls-copp-add sw0 reject acl-meter
+check ovn-nbctl --wait=hv ls-copp-add copp0 sw0 reject acl-meter
 check ovn-nbctl acl-add sw0 from-lport 1002 'inport == "sw01" && ip && udp' reject
 
 AT_CHECK([ovn-nbctl ls-copp-list sw0], [0], [dnl
@@ -6678,7 +6678,7 @@  kill $(pidof tcpdump)
 
 NS_EXEC([server], [tcpdump -l -n -i s1 arp[[24:4]]=0xac100164 > arp.pcap &])
 check ovn-nbctl meter-add arp-meter drop 1 pktps 0
-check ovn-nbctl --wait=hv lr-copp-add R1 arp-resolve arp-meter
+check ovn-nbctl --wait=hv lr-copp-add copp1 R1 arp-resolve arp-meter
 AT_CHECK([ovn-nbctl lr-copp-list R1], [0], [dnl
 arp-resolve: arp-meter
 ])
@@ -6696,7 +6696,7 @@  OVS_WAIT_UNTIL([
 kill $(pidof tcpdump)
 
 check ovn-nbctl meter-add icmp-meter drop 1 pktps 0
-check ovn-nbctl --wait=hv lr-copp-add R1 icmp4-error icmp-meter
+check ovn-nbctl --wait=hv lr-copp-add copp2 R1 icmp4-error icmp-meter
 AT_CHECK([ovn-nbctl lr-copp-list R1 |grep icmp4-error], [0], [dnl
 icmp4-error: icmp-meter
 ])
@@ -6715,7 +6715,7 @@  OVS_WAIT_UNTIL([
 kill $(pidof tcpdump)
 
 check ovn-nbctl meter-add bfd-meter drop 1 pktps 0
-check ovn-nbctl --wait=hv lr-copp-add R1 bfd bfd-meter
+check ovn-nbctl --wait=hv lr-copp-add copp3 R1 bfd bfd-meter
 AT_CHECK([ovn-nbctl lr-copp-list R1 |grep bfd], [0], [dnl
 bfd: bfd-meter
 ])
diff --git a/utilities/ovn-nbctl.8.xml b/utilities/ovn-nbctl.8.xml
index 80a564660..b944d7e08 100644
--- a/utilities/ovn-nbctl.8.xml
+++ b/utilities/ovn-nbctl.8.xml
@@ -1474,13 +1474,15 @@ 
     </p>
 
     <dl>
-      <dt><code>ls-copp-add</code> <var>switch</var> <var>proto</var>
-      <var>meter</var></dt>
+      <dt><code>ls-copp-add</code> [<var>UUID</var>|<var>name</var>]
+      <var>switch</var> <var>proto</var> <var>meter</var></dt>
       <dd>
         Adds the control <code>proto</code> to <code>meter</code> mapping
         to the <code>switch</code> control plane protection policy. If no
         policy exists yet, it creates one. If a mapping already existed for
         <code>proto</code>, this will overwrite it.
+        If <var>UUID</var> <var>name</var> is provided, the already installed
+        CoPP will be reused.
       </dd>
 
       <dt><code>ls-copp-del</code> <var>switch</var> [<var>proto</var>]</dt>
@@ -1497,13 +1499,15 @@ 
         <code>switch</code>.
       </dd>
 
-      <dt><code>lr-copp-add</code> <var>router</var> <var>proto</var>
-      <var>meter</var></dt>
+      <dt><code>lr-copp-add</code> [<var>UUID</var>|<var>name</var>]
+      <var>router</var> <var>proto</var> <var>meter</var></dt>
       <dd>
         Adds the control <code>proto</code> to <code>meter</code> mapping
         to the <code>router</code> control plane protection policy. If no
         policy exists yet, it creates one. If a mapping already existed for
         <code>proto</code>, this will overwrite it.
+        If <var>UUID</var> <var>name</var> is provided, the already installed
+        CoPP will be reused.
       </dd>
 
       <dt><code>lr-copp-del</code> <var>router</var> [<var>proto</var>]</dt>
diff --git a/utilities/ovn-nbctl.c b/utilities/ovn-nbctl.c
index d67d2db65..3a89da030 100644
--- a/utilities/ovn-nbctl.c
+++ b/utilities/ovn-nbctl.c
@@ -437,7 +437,7 @@  chassis with mandatory PRIORITY to the HA chassis group GRP\n\
 CHASSIS from the HA chassis group GRP\n\
 \n\
 Control Plane Protection Policy commands:\n\
-  ls-copp-add SWITCH PROTO METER\n\
+  ls-copp-add [UUID|NAME] SWITCH PROTO METER\n\
                             Add a copp policy for PROTO packets on SWITCH\n\
                             based on an existing METER.\n\
   ls-copp-del SWITCH [PROTO]\n\
@@ -447,7 +447,7 @@  Control Plane Protection Policy commands:\n\
   ls-copp-list SWITCH\n\
                             List all copp policies defined for control\n\
                             protocols on SWITCH.\n\
-  lr-copp-add ROUTER PROTO METER\n\
+  lr-copp-add [UUID|NAME] ROUTER PROTO METER\n\
                             Add a copp policy for PROTO packets on ROUTER\n\
                             based on an existing METER.\n\
   lr-copp-del ROUTER [PROTO]\n\
@@ -6278,6 +6278,9 @@  nbctl_pre_copp(struct ctl_context *ctx)
 {
     nbctl_pre_context(ctx);
     ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_meters);
+    ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_logical_switch);
+    ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_logical_router);
+    ovsdb_idl_add_column(ctx->idl, &nbrec_copp_col_name);
     ovsdb_idl_add_column(ctx->idl, &nbrec_logical_switch_col_copp);
     ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_copp);
 }
@@ -6285,9 +6288,29 @@  nbctl_pre_copp(struct ctl_context *ctx)
 static void
 nbctl_ls_copp_add(struct ctl_context *ctx)
 {
-    const char *ls_name = ctx->argv[1];
-    const char *proto_name = ctx->argv[2];
-    const char *meter = ctx->argv[3];
+    const struct nbrec_copp *copp = NULL;
+    const char *proto_name = ctx->argv[3];
+    const char *ls_name = ctx->argv[2];
+    const char *meter = ctx->argv[4];
+    const char *copp_name = NULL;
+    struct uuid uuid;
+
+    if (uuid_from_string(&uuid, ctx->argv[1])) {
+        copp = nbrec_copp_get_for_uuid(ctx->idl, &uuid);
+        if (!copp) {
+            ctx->error = xasprintf("copp %s not found.", ctx->argv[1]);
+            return;
+        }
+    } else {
+        copp_name = ctx->argv[1];
+        const struct nbrec_copp *iter;
+        NBREC_COPP_FOR_EACH (iter, ctx->idl) {
+            if (!strcmp(iter->name, copp_name)) {
+                copp = iter;
+                break;
+            }
+        }
+    }
 
     char *error = copp_proto_validate(proto_name);
     if (error) {
@@ -6302,9 +6325,23 @@  nbctl_ls_copp_add(struct ctl_context *ctx)
         return;
     }
 
-    const struct nbrec_copp *copp =
-        copp_meter_add(ctx, ls->copp, proto_name, meter);
+    if (!copp) {
+        copp = copp_meter_add(ctx, ls->copp, proto_name, meter);
+        if (copp_name) {
+            nbrec_copp_set_name(copp, copp_name);
+        }
+    }
     nbrec_logical_switch_set_copp(ls, copp);
+
+    size_t n_logical_switch = copp->n_logical_switch + 1;
+    struct nbrec_logical_switch **ls_list =
+        xmalloc(n_logical_switch * sizeof *ls_list);
+    for (int i = 0; i < copp->n_logical_switch; i++) {
+        ls_list[i] = copp->logical_switch[i];
+    }
+    ls_list[copp->n_logical_switch] = (struct nbrec_logical_switch *)ls;
+    nbrec_copp_set_logical_switch(copp, ls_list, n_logical_switch);
+    free(ls_list);
 }
 
 static void
@@ -6351,9 +6388,29 @@  nbctl_ls_copp_list(struct ctl_context *ctx)
 static void
 nbctl_lr_copp_add(struct ctl_context *ctx)
 {
-    const char *lr_name = ctx->argv[1];
-    const char *proto_name = ctx->argv[2];
-    const char *meter = ctx->argv[3];
+    const struct nbrec_copp *copp = NULL;
+    const char *proto_name = ctx->argv[3];
+    const char *lr_name = ctx->argv[2];
+    const char *meter = ctx->argv[4];
+    const char *copp_name = NULL;
+    struct uuid uuid;
+
+    if (uuid_from_string(&uuid, ctx->argv[1])) {
+        copp = nbrec_copp_get_for_uuid(ctx->idl, &uuid);
+        if (!copp) {
+            ctx->error = xasprintf("copp %s not found.", ctx->argv[1]);
+            return;
+        }
+    } else {
+        copp_name = ctx->argv[1];
+        const struct nbrec_copp *iter;
+        NBREC_COPP_FOR_EACH (iter, ctx->idl) {
+            if (!strcmp(iter->name, copp_name)) {
+                copp = iter;
+                break;
+            }
+        }
+    }
 
     char *error = copp_proto_validate(proto_name);
     if (error) {
@@ -6368,9 +6425,23 @@  nbctl_lr_copp_add(struct ctl_context *ctx)
         return;
     }
 
-    const struct nbrec_copp *copp =
-        copp_meter_add(ctx, lr->copp, proto_name, meter);
+    if (!copp) {
+        copp = copp_meter_add(ctx, lr->copp, proto_name, meter);
+        if (copp_name) {
+            nbrec_copp_set_name(copp, copp_name);
+        }
+    }
     nbrec_logical_router_set_copp(lr, copp);
+
+    size_t n_logical_router = copp->n_logical_router + 1;
+    struct nbrec_logical_router **lr_list =
+        xmalloc(n_logical_router * sizeof *lr_list);
+    for (int i = 0; i < copp->n_logical_router; i++) {
+        lr_list[i] = copp->logical_router[i];
+    }
+    lr_list[copp->n_logical_router] = (struct nbrec_logical_router *)lr;
+    nbrec_copp_set_logical_router(copp, lr_list, n_logical_router);
+    free(lr_list);
 }
 
 static void
@@ -7177,13 +7248,13 @@  static const struct ctl_command_syntax nbctl_commands[] = {
      NULL, "", RO },
 
     /* Control plane protection commands */
-    {"ls-copp-add", 3, 3, "SWITCH PROTO METER", nbctl_pre_copp,
+    {"ls-copp-add", 4, 4, "SWITCH PROTO METER", nbctl_pre_copp,
       nbctl_ls_copp_add, NULL, "", RW},
     {"ls-copp-del", 1, 2, "SWITCH [PROTO]", nbctl_pre_copp,
       nbctl_ls_copp_del, NULL, "", RW},
     {"ls-copp-list", 1, 1, "SWITCH", nbctl_pre_copp, nbctl_ls_copp_list,
       NULL, "", RO},
-    {"lr-copp-add", 3, 3, "ROUTER PROTO METER", nbctl_pre_copp,
+    {"lr-copp-add", 4, 4, "ROUTER PROTO METER", nbctl_pre_copp,
      nbctl_lr_copp_add, NULL, "", RW},
     {"lr-copp-del", 1, 2, "ROUTER [PROTO]", nbctl_pre_copp,
      nbctl_lr_copp_del, NULL, "", RW},