[ovs-dev] [PATCH 3/8] ovn-nbctl: Update logical router port commands.

[Justin Pettit]

> On Jun 10, 2016, at 7:07 PM, Ben Pfaff <blp at ovn.org> wrote:
> 
> On Fri, Jun 10, 2016 at 05:03:31PM -0700, Justin Pettit wrote:
>> 
>>> On Jun 9, 2016, at 5:09 PM, Ben Pfaff <blp at ovn.org> wrote:
>>> 
>>> I think that lrp-add with --may-exist should report an error if the MAC
>>> or NETWORK or [PEER] differ from the existing port's configuration.
>> 
>> Currently, adding the port fails if the port belongs to a different
>> router.  I assume you are suggesting that we report an error if those
>> fields are changing when re-adding to the same router, correct?  
> 
> Yes.
> 
> The idea of --may-exist is to make the command idempotent: if the same
> command has already been executed, then it turns it into a no-op.  But
> if a different command has been executed (with different MAC or NETWORK
> or PEER), it's not what the user has asked for.
> 
> This follows the precedent set by the add-port command in ovs-vsctl,
> which with --may-exist complains if, for example, the interfaces
> specified for a bond are different from the ones that actually exist.
> 
> I think there is at least one other precedent, but that's the one that
> comes to mind.

Got it.  I've added that check and a couple of checks to the unit tests.  I've attached a diff below.

--Justin


-=-=-=-=-=-=-=-

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 493f1e8..ac4f941 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -1334,6 +1334,10 @@ nbctl_lrp_add(struct ctl_context *ctx)
     lr = lr_by_name_or_uuid(ctx, ctx->argv[1], true);
 
     const char *lrp_name = ctx->argv[2];
+    const char *mac = ctx->argv[3];
+    const char *network = ctx->argv[4];
+    const char *peer = (ctx->argc == 6) ? ctx->argv[5] : NULL;
+
     const struct nbrec_logical_router_port *lrp;
     lrp = lrp_by_name_or_uuid(ctx, lrp_name, false);
     if (lrp) {
@@ -1350,34 +1354,50 @@ nbctl_lrp_add(struct ctl_context *ctx)
                       lr_get_name(bound_lr, uuid_s, sizeof uuid_s));
         }
 
+        if (strcmp(mac, lrp->mac)) {
+            ctl_fatal("%s: port already exists with mac %s", lrp_name,
+                      lrp->mac);
+        }
+
+        if (strcmp(network, lrp->network)) {
+            ctl_fatal("%s: port already exists with network %s", lrp_name,
+                      lrp->network);
+        }
+
+        if ((!peer != !lrp->peer) ||
+                (lrp->peer && strcmp(peer, lrp->peer))) {
+            ctl_fatal("%s: port already exists with mismatching peer",
+                      lrp_name);
+        }
+
         return;
     }
 
     struct eth_addr ea;
-    if (!ovs_scan(ctx->argv[3], ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
-        ctl_fatal("%s: invalid mac address.", ctx->argv[3]);
+    if (!ovs_scan(mac, ETH_ADDR_SCAN_FMT, ETH_ADDR_SCAN_ARGS(ea))) {
+        ctl_fatal("%s: invalid mac address.", mac);
     }
 
     ovs_be32 ip;
     unsigned int plen;
-    char *error = ip_parse_cidr(ctx->argv[4], &ip, &plen);
+    char *error = ip_parse_cidr(network, &ip, &plen);
     if (error) {
         free(error);
         struct in6_addr ipv6;
-        error = ipv6_parse_cidr(ctx->argv[4], &ipv6, &plen);
+        error = ipv6_parse_cidr(network, &ipv6, &plen);
         if (error) {
             free(error);
-            ctl_fatal("%s: invalid network address.", ctx->argv[4]);
+            ctl_fatal("%s: invalid network address.", network);
         }
     }
 
     /* Create the logical port. */
     lrp = nbrec_logical_router_port_insert(ctx->txn);
     nbrec_logical_router_port_set_name(lrp, lrp_name);
-    nbrec_logical_router_port_set_mac(lrp, ctx->argv[3]);
-    nbrec_logical_router_port_set_network(lrp, ctx->argv[4]);
-    if (ctx->argc == 6) {
-        nbrec_logical_router_port_set_peer(lrp, ctx->argv[5]);
+    nbrec_logical_router_port_set_mac(lrp, mac);
+    nbrec_logical_router_port_set_network(lrp, network);
+    if (peer) {
+        nbrec_logical_router_port_set_peer(lrp, peer);
     }
 
     /* Insert the logical port into the logical router. */
diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
index 2e8fc17..db40abf 100644
--- a/tests/ovn-nbctl.at
+++ b/tests/ovn-nbctl.at
@@ -305,7 +305,7 @@ AT_CHECK([ovn-nbctl lrp-list lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <0> (lrp0)
 ])
 
-AT_CHECK([ovn-nbctl lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24])
+AT_CHECK([ovn-nbctl lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24 lrp1-peer])
 AT_CHECK([ovn-nbctl lrp-list lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <0> (lrp0)
 <1> (lrp1)
@@ -315,10 +315,21 @@ AT_CHECK([ovn-nbctl lr-add lr1])
 AT_CHECK([ovn-nbctl lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24], [1], [],
   [ovn-nbctl: lrp1: a port with this name already exists
 ])
+
 AT_CHECK([ovn-nbctl --may-exist lrp-add lr1 lrp1 00:00:00:01:02:03 192.168.1.1/24], [1], [],
   [ovn-nbctl: lrp1: port already exists but in router lr0
 ])
 
+AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:04:05:06 192.168.1.1/24], [1], [],
+  [ovn-nbctl: lrp1: port already exists with mac 00:00:00:01:02:03
+])
+
+AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24], [1], [],
+  [ovn-nbctl: lrp1: port already exists with mismatching peer
+])
+
+AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24 lrp1-peer])
+
 AT_CHECK([ovn-nbctl lrp-del lrp1])
 AT_CHECK([ovn-nbctl lrp-list lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl
 <0> (lrp0)