> On Jun 10, 2016, at 7:07 PM, Ben Pfaff <b...@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 <b...@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)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
