[ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
Aaron Rosen
arosen at vmware.com
Fri Jun 26 17:20:16 UTC 2015
Hi Ben,
I just tested out this patch and it seems to work as expected for me. I do have a few quick questions/thoughts.
- One thing I noticed is if I create a logical_port but don't append the port.uuid to the Logical_Switch.ports column it seems like the port isn't showing up in ovsdb. I'm not sure if this is expected or I'm incorrect.
- Another thing I was thinking about is if requiring the caller to query ovsdb for the logical_switch to obtain the switch_ports and then appending the newly create logical port could be problematic. For one it seems like we need to lock ovsdb for concurrent operations here. I was wondering if it would be possible to keep previous schema where the logical_port stores a reference to it's logical_switch. Though, when the logical_switch is deleted we would cascade the deletes of the logical_ports on that switch? I also think it feels more natural to have a reference from the logical_port to logical_switch because now it seems like the schema allows one to create a logical_port without a logical_switch? Perhaps ovsdb can't currently work in this way which you were explaining in your commit message though I just wanted to make sure.
Thanks,
Aaron
________________________________________
From: Ben Pfaff <blp at nicira.com>
Sent: Thursday, June 25, 2015 4:57 PM
To: Aaron Rosen
Cc: dev at openvswitch.org
Subject: Re: [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.
Thanks. Of course this patch needs review on the OVS side as well; that
might take a few days.
On Thu, Jun 25, 2015 at 11:51:57PM +0000, Aaron Rosen wrote:
> Awesome thanks Ben! I'll update neutron tomorrow morning to work with this.
>
>
> > On Jun 25, 2015, at 5:35 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > Until now, the OVN_Northbound schema has been designed to sidestep a
> > weakness in the OVSDB protocol when a column has a great deal of data in
> > it. In the current OVSDB protocol, whenever a column changes, the entire
> > new value of the column is sent to all of the clients that are monitoring
> > that column. That means that adding or removing a small amount of data,
> > say 1 element in a set, requires sending all of the data, which is
> > expensive if the column has a lot of data.
> >
> > One example of a column with potential to have a lot of data is the set of
> > ports within a logical switch, if a logical switch has a large number of
> > ports. Thus, the existing OVN_Northbound schema has each Logical_Port
> > point to its containing Logical_Switch instead of the other way around.
> > This sidesteps the problem because it does not use any large columns.
> >
> > The tradeoff that this forces, however, is that the schema cannot take
> > advantage of OVSDB's garbage collection feature, where it automatically
> > deletes rows that are unreferenced. That's a problem for Neutron because
> > of Neutron-internal races between deletion of a Logical_Switch and
> > creation of new Logical_Ports on the switch being deleted. When such a
> > race happens, OVSDB refuses to delete the Logical_Switch because of
> > references to it from the newly created Logical_Port (although Neutron
> > does delete the pre-existing logical ports).
> >
> > To solve the problem, this commit changes the OVN_Northbound schema to
> > use a set of ports within Logical_Switch. That will lead to large columns
> > for large logical switches; I plan to address that (though I don't have
> > code written) by enhancing the OVSDB protocol. With this commit applied,
> > the database will automatically cascade deleting a logical switch row to
> > delete all of its ports, ACLs, and its router port (if any).
> >
> > This commit makes some pretty pervasive changes to ovn-northd, but they
> > are mostly beneficial to the code readability because now it becomes
> > possible to trivially iterate through the ports that belong to a switch,
> > which was difficult before the schema change.
> >
> > This commit will break the Neutron integration until that is changed to
> > handle the new database schema.
> >
> > CC: Aaron Rosen <arosen at vmware.com>
> > Signed-off-by: Ben Pfaff <blp at nicira.com>
> > ---
> > ovn/northd/ovn-northd.c | 318 ++++++++++++++++++++++--------------------------
> > ovn/ovn-nb.ovsschema | 39 +++---
> > ovn/ovn-nb.xml | 74 ++++++-----
> > ovn/ovn-nbctl.c | 116 ++++++++++--------
> > 4 files changed, 282 insertions(+), 265 deletions(-)
> >
> > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> > index f37df77..c790a90 100644
> > --- a/ovn/northd/ovn-northd.c
> > +++ b/ovn/northd/ovn-northd.c
> > @@ -281,139 +281,116 @@ build_pipeline(struct northd_context *ctx)
> > }
> >
> > /* Table 0: Ingress port security. */
> > - const struct nbrec_logical_port *lport;
> > - NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > - struct ds match = DS_EMPTY_INITIALIZER;
> > - ds_put_cstr(&match, "inport == ");
> > - json_string_escape(lport->name, &match);
> > - build_port_security("eth.src",
> > - lport->port_security, lport->n_port_security,
> > - &match);
> > - pipeline_add(&pc, lport->lswitch, 0, 50, ds_cstr(&match),
> > - lport_is_enabled(lport) ? "next;" : "drop;");
> > - ds_destroy(&match);
> > - }
> > -
> > - /* Table 1: Destination lookup, broadcast and multicast handling (priority
> > - * 100). */
> > NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > - struct ds actions;
> > -
> > - ds_init(&actions);
> > - NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > - if (lport->lswitch == lswitch && lport_is_enabled(lport)) {
> > - ds_put_cstr(&actions, "outport = ");
> > - json_string_escape(lport->name, &actions);
> > - ds_put_cstr(&actions, "; next; ");
> > - }
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + const struct nbrec_logical_port *lport = lswitch->ports[i];
> > + struct ds match = DS_EMPTY_INITIALIZER;
> > + ds_put_cstr(&match, "inport == ");
> > + json_string_escape(lport->name, &match);
> > + build_port_security("eth.src",
> > + lport->port_security, lport->n_port_security,
> > + &match);
> > + pipeline_add(&pc, lswitch, 0, 50, ds_cstr(&match),
> > + lport_is_enabled(lport) ? "next;" : "drop;");
> > + ds_destroy(&match);
> > }
> > - ds_chomp(&actions, ' ');
> > -
> > - pipeline_add(&pc, lswitch, 1, 100, "eth.dst[40]", ds_cstr(&actions));
> > - ds_destroy(&actions);
> > }
> >
> > - /* Table 1: Destination lookup, unicast handling (priority 50), */
> > - struct unknown_actions {
> > - struct hmap_node hmap_node;
> > - const struct nbrec_logical_switch *ls;
> > - struct ds actions;
> > - };
> > - struct hmap unknown_actions = HMAP_INITIALIZER(&unknown_actions);
> > - NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > - lswitch = lport->lswitch;
> > - for (size_t i = 0; i < lport->n_macs; i++) {
> > - uint8_t mac[ETH_ADDR_LEN];
> > -
> > - if (eth_addr_from_string(lport->macs[i], mac)) {
> > - struct ds match, actions;
> > -
> > - ds_init(&match);
> > - ds_put_format(&match, "eth.dst == %s", lport->macs[i]);
> > -
> > - ds_init(&actions);
> > - ds_put_cstr(&actions, "outport = ");
> > - json_string_escape(lport->name, &actions);
> > - ds_put_cstr(&actions, "; next;");
> > - pipeline_add(&pc, lswitch, 1, 50,
> > - ds_cstr(&match), ds_cstr(&actions));
> > - ds_destroy(&actions);
> > - ds_destroy(&match);
> > - } else if (!strcmp(lport->macs[i], "unknown")) {
> > - const struct uuid *uuid = &lswitch->header_.uuid;
> > - struct unknown_actions *ua = NULL;
> > - struct unknown_actions *iter;
> > - HMAP_FOR_EACH_WITH_HASH (iter, hmap_node, uuid_hash(uuid),
> > - &unknown_actions) {
> > - if (uuid_equals(&iter->ls->header_.uuid, uuid)) {
> > - ua = iter;
> > - break;
> > - }
> > - }
> > - if (!ua) {
> > - ua = xmalloc(sizeof *ua);
> > - hmap_insert(&unknown_actions, &ua->hmap_node,
> > - uuid_hash(uuid));
> > - ua->ls = lswitch;
> > - ds_init(&ua->actions);
> > + /* Table 1: Destination lookup:
> > + *
> > + * - Broadcast and multicast handling (priority 100).
> > + * - Unicast handling (priority 50).
> > + * - Unknown unicast address handling (priority 0).
> > + * */
> > + NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > + struct ds bcast; /* Actions for broadcast on 'lswitch'. */
> > + struct ds unknown; /* Actions for unknown MACs on 'lswitch'. */
> > +
> > + ds_init(&bcast);
> > + ds_init(&unknown);
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + const struct nbrec_logical_port *lport = lswitch->ports[i];
> > +
> > + ds_put_cstr(&bcast, "outport = ");
> > + json_string_escape(lport->name, &bcast);
> > + ds_put_cstr(&bcast, "; next; ");
> > +
> > + for (size_t j = 0; j < lport->n_macs; j++) {
> > + const char *s = lport->macs[j];
> > + uint8_t mac[ETH_ADDR_LEN];
> > +
> > + if (eth_addr_from_string(s, mac)) {
> > + struct ds match, unicast;
> > +
> > + ds_init(&match);
> > + ds_put_format(&match, "eth.dst == %s", s);
> > +
> > + ds_init(&unicast);
> > + ds_put_cstr(&unicast, "outport = ");
> > + json_string_escape(lport->name, &unicast);
> > + ds_put_cstr(&unicast, "; next;");
> > + pipeline_add(&pc, lswitch, 1, 50,
> > + ds_cstr(&match), ds_cstr(&unicast));
> > + ds_destroy(&unicast);
> > + ds_destroy(&match);
> > + } else if (!strcmp(s, "unknown")) {
> > + ds_put_cstr(&bcast, "outport = ");
> > + json_string_escape(lport->name, &unknown);
> > + ds_put_cstr(&bcast, "; next; ");
> > } else {
> > - ds_put_char(&ua->actions, ' ');
> > - }
> > -
> > - ds_put_cstr(&ua->actions, "outport = ");
> > - json_string_escape(lport->name, &ua->actions);
> > - ds_put_cstr(&ua->actions, "; next;");
> > - } else {
> > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> >
> > - VLOG_INFO_RL(&rl, "%s: invalid syntax '%s' in macs column",
> > - lport->name, lport->macs[i]);
> > + VLOG_INFO_RL(&rl, "%s: invalid syntax '%s' in macs column",
> > + lport->name, s);
> > + }
> > }
> > }
> > - }
> >
> > - /* Table 1: Destination lookup for unknown MACs (priority 0). */
> > - struct unknown_actions *ua, *next_ua;
> > - HMAP_FOR_EACH_SAFE (ua, next_ua, hmap_node, &unknown_actions) {
> > - pipeline_add(&pc, ua->ls, 1, 0, "1", ds_cstr(&ua->actions));
> > - hmap_remove(&unknown_actions, &ua->hmap_node);
> > - ds_destroy(&ua->actions);
> > - free(ua);
> > + ds_chomp(&bcast, ' ');
> > + pipeline_add(&pc, lswitch, 1, 100, "eth.dst[40]", ds_cstr(&bcast));
> > + ds_destroy(&bcast);
> > +
> > + ds_chomp(&unknown, ' ');
> > + pipeline_add(&pc, lswitch, 1, 0, "1", ds_cstr(&unknown));
> > + ds_destroy(&unknown);
> > }
> > - hmap_destroy(&unknown_actions);
> >
> > /* Table 2: ACLs. */
> > - const struct nbrec_acl *acl;
> > - NBREC_ACL_FOR_EACH (acl, ctx->ovnnb_idl) {
> > - const char *action;
> > -
> > - action = (!strcmp(acl->action, "allow") ||
> > - !strcmp(acl->action, "allow-related"))
> > - ? "next;" : "drop;";
> > - pipeline_add(&pc, acl->lswitch, 2, acl->priority, acl->match, action);
> > - }
> > NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > + for (size_t i = 0; i < lswitch->n_acls; i++) {
> > + const struct nbrec_acl *acl = lswitch->acls[i];
> > +
> > + NBREC_ACL_FOR_EACH (acl, ctx->ovnnb_idl) {
> > + pipeline_add(&pc, lswitch, 2, acl->priority, acl->match,
> > + (!strcmp(acl->action, "allow") ||
> > + !strcmp(acl->action, "allow-related")
> > + ? "next;" : "drop;"));
> > + }
> > + }
> > +
> > pipeline_add(&pc, lswitch, 2, 0, "1", "next;");
> > }
> >
> > /* Table 3: Egress port security. */
> > NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > pipeline_add(&pc, lswitch, 3, 100, "eth.dst[40]", "output;");
> > - }
> > - NBREC_LOGICAL_PORT_FOR_EACH (lport, ctx->ovnnb_idl) {
> > - struct ds match;
> >
> > - ds_init(&match);
> > - ds_put_cstr(&match, "outport == ");
> > - json_string_escape(lport->name, &match);
> > - build_port_security("eth.dst",
> > - lport->port_security, lport->n_port_security,
> > - &match);
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + const struct nbrec_logical_port *lport = lswitch->ports[i];
> > + struct ds match;
> >
> > - pipeline_add(&pc, lport->lswitch, 3, 50, ds_cstr(&match),
> > - lport_is_enabled(lport) ? "output;" : "drop;");
> > + ds_init(&match);
> > + ds_put_cstr(&match, "outport == ");
> > + json_string_escape(lport->name, &match);
> > + build_port_security("eth.dst",
> > + lport->port_security, lport->n_port_security,
> > + &match);
> >
> > - ds_destroy(&match);
> > + pipeline_add(&pc, lswitch, 3, 50, ds_cstr(&match),
> > + lport_is_enabled(lport) ? "output;" : "drop;");
> > +
> > + ds_destroy(&match);
> > + }
> > }
> >
> > /* Delete any existing Pipeline rows that were not re-generated. */
> > @@ -504,7 +481,6 @@ static void
> > set_bindings(struct northd_context *ctx)
> > {
> > const struct sbrec_binding *binding;
> > - const struct nbrec_logical_port *lport;
> >
> > /*
> > * We will need to look up a binding for every logical port. We don't want
> > @@ -533,72 +509,74 @@ set_bindings(struct northd_context *ctx)
> > hash_int(binding->tunnel_key, 0));
> > }
> >
> > - NBREC_LOGICAL_PORT_FOR_EACH(lport, ctx->ovnnb_idl) {
> > - struct binding_hash_node *hash_node;
> > - binding = NULL;
> > - HMAP_FOR_EACH_WITH_HASH(hash_node, lp_node,
> > - hash_string(lport->name, 0), &lp_hmap) {
> > - if (!strcmp(lport->name, hash_node->binding->logical_port)) {
> > - binding = hash_node->binding;
> > - break;
> > + const struct nbrec_logical_switch *lswitch;
> > + NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, ctx->ovnnb_idl) {
> > + const struct uuid *logical_datapath = &lswitch->header_.uuid;
> > +
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + const struct nbrec_logical_port *lport = lswitch->ports[i];
> > + struct binding_hash_node *hash_node;
> > + binding = NULL;
> > + HMAP_FOR_EACH_WITH_HASH(hash_node, lp_node,
> > + hash_string(lport->name, 0), &lp_hmap) {
> > + if (!strcmp(lport->name, hash_node->binding->logical_port)) {
> > + binding = hash_node->binding;
> > + break;
> > + }
> > }
> > - }
> >
> > - struct uuid logical_datapath;
> > - if (lport->lswitch) {
> > - logical_datapath = lport->lswitch->header_.uuid;
> > - } else {
> > - uuid_zero(&logical_datapath);
> > - }
> > + if (binding) {
> > + /* We found an existing binding for this logical port. Update
> > + * its contents. */
> >
> > - if (binding) {
> > - /* We found an existing binding for this logical port. Update its
> > - * contents. */
> > + hmap_remove(&lp_hmap, &hash_node->lp_node);
> >
> > - hmap_remove(&lp_hmap, &hash_node->lp_node);
> > + if (!macs_equal(binding->mac, binding->n_mac,
> > + lport->macs, lport->n_macs)) {
> > + sbrec_binding_set_mac(binding, (const char **) lport->macs,
> > + lport->n_macs);
> > + }
> > + if (!parents_equal(binding, lport)) {
> > + sbrec_binding_set_parent_port(binding, lport->parent_name);
> > + }
> > + if (!tags_equal(binding, lport)) {
> > + sbrec_binding_set_tag(binding, lport->tag, lport->n_tag);
> > + }
> > + if (!uuid_equals(&binding->logical_datapath,
> > + logical_datapath)) {
> > + sbrec_binding_set_logical_datapath(binding,
> > + *logical_datapath);
> > + }
> > + } else {
> > + /* There is no binding for this logical port, so create one. */
> >
> > - if (!macs_equal(binding->mac, binding->n_mac,
> > - lport->macs, lport->n_macs)) {
> > - sbrec_binding_set_mac(binding,
> > - (const char **) lport->macs, lport->n_macs);
> > - }
> > - if (!parents_equal(binding, lport)) {
> > - sbrec_binding_set_parent_port(binding, lport->parent_name);
> > - }
> > - if (!tags_equal(binding, lport)) {
> > - sbrec_binding_set_tag(binding, lport->tag, lport->n_tag);
> > - }
> > - if (!uuid_equals(&binding->logical_datapath, &logical_datapath)) {
> > - sbrec_binding_set_logical_datapath(binding,
> > - logical_datapath);
> > - }
> > - } else {
> > - /* There is no binding for this logical port, so create one. */
> > + uint16_t tunnel_key = choose_tunnel_key(&tk_hmap);
> > + if (!tunnel_key) {
> > + continue;
> > + }
> >
> > - uint16_t tunnel_key = choose_tunnel_key(&tk_hmap);
> > - if (!tunnel_key) {
> > - continue;
> > - }
> > + binding = sbrec_binding_insert(ctx->ovnsb_txn);
> > + sbrec_binding_set_logical_port(binding, lport->name);
> > + sbrec_binding_set_mac(binding, (const char **) lport->macs,
> > + lport->n_macs);
> > + if (lport->parent_name && lport->n_tag > 0) {
> > + sbrec_binding_set_parent_port(binding, lport->parent_name);
> > + sbrec_binding_set_tag(binding, lport->tag, lport->n_tag);
> > + }
> >
> > - binding = sbrec_binding_insert(ctx->ovnsb_txn);
> > - sbrec_binding_set_logical_port(binding, lport->name);
> > - sbrec_binding_set_mac(binding,
> > - (const char **) lport->macs, lport->n_macs);
> > - if (lport->parent_name && lport->n_tag > 0) {
> > - sbrec_binding_set_parent_port(binding, lport->parent_name);
> > - sbrec_binding_set_tag(binding, lport->tag, lport->n_tag);
> > + sbrec_binding_set_tunnel_key(binding, tunnel_key);
> > + sbrec_binding_set_logical_datapath(binding, *logical_datapath);
> > +
> > + /* Add the tunnel key to the tk_hmap so that we don't try to
> > + * use it for another port. (We don't want it in the lp_hmap
> > + * because that would just get the Binding record deleted
> > + * later.) */
> > + struct binding_hash_node *hash_node
> > + = xzalloc(sizeof *hash_node);
> > + hash_node->binding = binding;
> > + hmap_insert(&tk_hmap, &hash_node->tk_node,
> > + hash_int(binding->tunnel_key, 0));
> > }
> > -
> > - sbrec_binding_set_tunnel_key(binding, tunnel_key);
> > - sbrec_binding_set_logical_datapath(binding, logical_datapath);
> > -
> > - /* Add the tunnel key to the tk_hmap so that we don't try to use it
> > - * for another port. (We don't want it in the lp_hmap because that
> > - * would just get the Binding record deleted later.) */
> > - struct binding_hash_node *hash_node = xzalloc(sizeof *hash_node);
> > - hash_node->binding = binding;
> > - hmap_insert(&tk_hmap, &hash_node->tk_node,
> > - hash_int(binding->tunnel_key, 0));
> > }
> > }
> >
> > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> > index bcbd94b..d7d8ac0 100644
> > --- a/ovn/ovn-nb.ovsschema
> > +++ b/ovn/ovn-nb.ovsschema
> > @@ -4,18 +4,26 @@
> > "Logical_Switch": {
> > "columns": {
> > "name": {"type": "string"},
> > + "ports": {"type": {"key": {"type": "uuid",
> > + "refTable": "Logical_Port",
> > + "refType": "strong"},
> > + "min": 0,
> > + "max": "unlimited"}},
> > + "acls": {"type": {"key": {"type": "uuid",
> > + "refTable": "ACL",
> > + "refType": "strong"},
> > + "min": 0,
> > + "max": "unlimited"}},
> > "router_port": {"type": {"key": {"type": "uuid",
> > "refTable": "Logical_Router_Port",
> > "refType": "strong"},
> > "min": 0, "max": 1}},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > - "min": 0, "max": "unlimited"}}}},
> > + "min": 0, "max": "unlimited"}}},
> > + "isRoot": true},
> > "Logical_Port": {
> > "columns": {
> > - "lswitch": {"type": {"key": {"type": "uuid",
> > - "refTable": "Logical_Switch",
> > - "refType": "strong"}}},
> > "name": {"type": "string"},
> > "parent_name": {"type": {"key": "string", "min": 0, "max": 1}},
> > "tag": {
> > @@ -34,12 +42,10 @@
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > "min": 0, "max": "unlimited"}}},
> > - "indexes": [["name"]]},
> > + "indexes": [["name"]],
> > + "isRoot": false},
> > "ACL": {
> > "columns": {
> > - "lswitch": {"type": {"key": {"type": "uuid",
> > - "refTable": "Logical_Switch",
> > - "refType": "strong"}}},
> > "priority": {"type": {"key": {"type": "integer",
> > "minInteger": 1,
> > "maxInteger": 65535}}},
> > @@ -49,22 +55,27 @@
> > "log": {"type": "boolean"},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > - "min": 0, "max": "unlimited"}}}},
> > + "min": 0, "max": "unlimited"}}},
> > + "isRoot": false},
> > "Logical_Router": {
> > "columns": {
> > + "ports": {"type": {"key": {"type": "uuid",
> > + "refTable": "Logical_Router_Port",
> > + "refType": "weak"},
> > + "min": 0,
> > + "max": "unlimited"}},
> > "ip": {"type": "string"},
> > "default_gw": {"type": {"key": "string", "min": 0, "max": 1}},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > - "min": 0, "max": "unlimited"}}}},
> > + "min": 0, "max": "unlimited"}}},
> > + "isRoot": true},
> > "Logical_Router_Port": {
> > "columns": {
> > - "router": {"type": {"key": {"type": "uuid",
> > - "refTable": "Logical_Router",
> > - "refType": "strong"}}},
> > "network": {"type": "string"},
> > "mac": {"type": "string"},
> > "external_ids": {
> > "type": {"key": "string", "value": "string",
> > - "min": 0, "max": "unlimited"}}}}},
> > + "min": 0, "max": "unlimited"}}},
> > + "isRoot": false}},
> > "version": "1.0.0"}
> > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> > index a74bf4d..cafba14 100644
> > --- a/ovn/ovn-nb.xml
> > +++ b/ovn/ovn-nb.xml
> > @@ -32,9 +32,7 @@
> >
> > <table name="Logical_Switch" title="L2 logical switch">
> > <p>
> > - Each row represents one L2 logical switch. A given switch's ports are
> > - the <ref table="Logical_Port"/> rows whose <ref table="Logical_Port"
> > - column="lswitch"/> column points to its row.
> > + Each row represents one L2 logical switch.
> > </p>
> >
> > <column name="name">
> > @@ -46,6 +44,17 @@
> > </p>
> > </column>
> >
> > + <column name="ports">
> > + <p>
> > + The logical ports connected to the logical switch.
> > + </p>
> > +
> > + <p>
> > + It is an error for multiple logical switches to include the same
> > + logical port.
> > + </p>
> > + </column>
> > +
> > <column name="router_port">
> > <p>
> > The router port to which this logical switch is connected, or empty if
> > @@ -54,6 +63,15 @@
> > restriction because logical routers may be connected into arbitrary
> > topologies.
> > </p>
> > +
> > + <p>
> > + It is an error for multiple logical switches to refer to the same
> > + router port.
> > + </p>
> > + </column>
> > +
> > + <column name="acls">
> > + Access control rules that apply to packets within the logical switch.
> > </column>
> >
> > <group title="Common Columns">
> > @@ -68,10 +86,6 @@
> > A port within an L2 logical switch.
> > </p>
> >
> > - <column name="lswitch">
> > - The logical switch to which the logical port is connected.
> > - </column>
> > -
> > <column name="name">
> > <p>
> > The logical port name.
> > @@ -170,21 +184,15 @@
> >
> > <table name="ACL" title="Access Control List (ACL) rule">
> > <p>
> > - Each row in this table represents one ACL rule for the logical switch in
> > - its <ref column="lswitch"/> column. The <ref column="action"/> column for
> > - the highest-<ref column="priority"/> matching row in this table
> > - determines a packet's treatment. If no row matches, packets are allowed
> > - by default. (Default-deny treatment is possible: add a rule with <ref
> > - column="priority"/> 1, <code>1</code> as <ref column="match"/>, and
> > - <code>deny</code> as <ref column="action"/>.)
> > + Each row in this table represents one ACL rule for a logical switch
> > + that points to it through its <ref column="acls"/> column. The <ref
> > + column="action"/> column for the highest-<ref column="priority"/>
> > + matching row in this table determines a packet's treatment. If no row
> > + matches, packets are allowed by default. (Default-deny treatment is
> > + possible: add a rule with <ref column="priority"/> 1, <code>1</code> as
> > + <ref column="match"/>, and <code>deny</code> as <ref column="action"/>.)
> > </p>
> >
> > - <column name="lswitch">
> > - The switch to which the ACL rule applies. The expression in the
> > - <ref column="match"/> column may match against logical ports
> > - within this switch.
> > - </column>
> > -
> > <column name="priority">
> > The ACL rule's priority. Rules with numerically higher priority take
> > precedence over those with lower. If two ACL rules with the same
> > @@ -255,11 +263,15 @@
> >
> > <table name="Logical_Router" title="L3 logical router">
> > <p>
> > - Each row represents one L3 logical router. A given router's ports are
> > - the <ref table="Logical_Router_Port"/> rows whose <ref
> > - table="Logical_Router_Port" column="router"/> column points to its row.
> > + Each row represents one L3 logical router.
> > </p>
> >
> > + <column name="ports">
> > + The router's ports. This is a set of weak references, so a <ref
> > + table="Logical_Switch"/> must also refer to any given <ref
> > + table="Logical_Router_Port"/> or it will automatically be deleted.
> > + </column>
> > +
> > <column name="ip">
> > The logical router's own IP address. The logical router uses this
> > address for ICMP replies (e.g. network unreachable messages) and other
> > @@ -284,16 +296,16 @@
> > </p>
> >
> > <p>
> > - A router port is always attached to a switch port. The connection can be
> > - identified by following the <ref column="router_port"
> > - table="Logical_Port"/> column from an appropriate <ref
> > - table="Logical_Port"/> row.
> > + A router port is always attached to a logical switch and to a logical
> > + router. The former attachment, which is enforced by the database schema,
> > + can be identified by finding the <ref table="Logical_Switch"/> row whose
> > + <ref column="router_port" table="Logical_Switch"/> column points to the
> > + router port. The latter attachment, which the database schema does not
> > + enforce, can be identified by finding the <ref table="Logical_Router"/>
> > + row whose <ref column="ports" table="Logical_Router"/> column includes
> > + the router port.
> > </p>
> >
> > - <column name="router">
> > - The router to which the port belongs.
> > - </column>
> > -
> > <column name="network">
> > The IP network and netmask of the network on the router port. Used for
> > routing.
> > diff --git a/ovn/ovn-nbctl.c b/ovn/ovn-nbctl.c
> > index 6a35a1a..bcc5028 100644
> > --- a/ovn/ovn-nbctl.c
> > +++ b/ovn/ovn-nbctl.c
> > @@ -139,30 +139,25 @@ lswitch_by_name_or_uuid(struct nbctl_context *nb_ctx, const char *id)
> > }
> >
> > static void
> > -print_lswitch(const struct nbctl_context *nb_ctx,
> > - const struct nbrec_logical_switch *lswitch)
> > +print_lswitch(const struct nbrec_logical_switch *lswitch)
> > {
> > - const struct nbrec_logical_port *lport;
> > -
> > printf(" lswitch "UUID_FMT" (%s)\n",
> > UUID_ARGS(&lswitch->header_.uuid), lswitch->name);
> >
> > - NBREC_LOGICAL_PORT_FOR_EACH(lport, nb_ctx->idl) {
> > - int i;
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + const struct nbrec_logical_port *lport = lswitch->ports[i];
> >
> > - if (lport->lswitch == lswitch) {
> > - printf(" lport %s\n", lport->name);
> > - if (lport->parent_name && lport->n_tag) {
> > - printf(" parent: %s, tag:%"PRIu64"\n",
> > - lport->parent_name, lport->tag[0]);
> > - }
> > - if (lport->n_macs) {
> > - printf(" macs:");
> > - for (i=0; i < lport->n_macs; i++) {
> > - printf(" %s", lport->macs[i]);
> > - }
> > - printf("\n");
> > + printf(" lport %s\n", lport->name);
> > + if (lport->parent_name && lport->n_tag) {
> > + printf(" parent: %s, tag:%"PRIu64"\n",
> > + lport->parent_name, lport->tag[0]);
> > + }
> > + if (lport->n_macs) {
> > + printf(" macs:");
> > + for (size_t j = 0; j < lport->n_macs; j++) {
> > + printf(" %s", lport->macs[j]);
> > }
> > + printf("\n");
> > }
> > }
> > }
> > @@ -176,11 +171,11 @@ do_show(struct ovs_cmdl_context *ctx)
> > if (ctx->argc == 2) {
> > lswitch = lswitch_by_name_or_uuid(nb_ctx, ctx->argv[1]);
> > if (lswitch) {
> > - print_lswitch(nb_ctx, lswitch);
> > + print_lswitch(lswitch);
> > }
> > } else {
> > NBREC_LOGICAL_SWITCH_FOR_EACH(lswitch, nb_ctx->idl) {
> > - print_lswitch(nb_ctx, lswitch);
> > + print_lswitch(lswitch);
> > }
> > }
> > }
> > @@ -323,7 +318,7 @@ do_lport_add(struct ovs_cmdl_context *ctx)
> > }
> >
> > if (ctx->argc != 3 && ctx->argc != 5) {
> > - /* If a parent_name is specififed, a tag must be specified as well. */
> > + /* If a parent_name is specified, a tag must be specified as well. */
> > VLOG_WARN("Invalid arguments to lport-add.");
> > return;
> > }
> > @@ -336,14 +331,44 @@ do_lport_add(struct ovs_cmdl_context *ctx)
> > }
> > }
> >
> > - /* Finally, create the transaction. */
> > + /* Create the logical port. */
> > lport = nbrec_logical_port_insert(nb_ctx->txn);
> > nbrec_logical_port_set_name(lport, ctx->argv[2]);
> > - nbrec_logical_port_set_lswitch(lport, lswitch);
> > if (ctx->argc == 5) {
> > nbrec_logical_port_set_parent_name(lport, ctx->argv[3]);
> > nbrec_logical_port_set_tag(lport, &tag, 1);
> > }
> > +
> > + /* Insert the logical port into the logical switch. */
> > + nbrec_logical_switch_verify_ports(lswitch);
> > + struct nbrec_logical_port **new_ports = xmalloc(sizeof *new_ports *
> > + (lswitch->n_ports + 1));
> > + memcpy(new_ports, lswitch->ports, sizeof *new_ports * lswitch->n_ports);
> > + new_ports[lswitch->n_ports] = lport;
> > + nbrec_logical_switch_set_ports(lswitch, new_ports, lswitch->n_ports + 1);
> > + free(new_ports);
> > +}
> > +
> > +/* Removes lport 'lswitch->ports[idx]'. */
> > +static void
> > +remove_lport(const struct nbrec_logical_switch *lswitch, size_t idx)
> > +{
> > + const struct nbrec_logical_port *lport = lswitch->ports[idx];
> > +
> > + /* First remove 'lport' from the array of ports. This is what will
> > + * actually causing the logical port to be deleted when the transaction is
> > + * sent to the database server (due to garbage collection). */
> > + struct nbrec_logical_port **new_ports
> > + = xmemdup(lswitch->ports, sizeof *new_ports * lswitch->n_ports);
> > + new_ports[idx] = new_ports[lswitch->n_ports - 1];
> > + nbrec_logical_switch_verify_ports(lswitch);
> > + nbrec_logical_switch_set_ports(lswitch, new_ports, lswitch->n_ports + 1);
> > + free(new_ports);
> > +
> > + /* Delete 'lport' from the IDL. This won't have a real effect on the
> > + * database server (the IDL will suppress it in fact) but it means that it
> > + * won't show up when we iterate with NBREC_LOGICAL_PORT_FOR_EACH later. */
> > + nbrec_logical_port_delete(lport);
> > }
> >
> > static void
> > @@ -357,44 +382,35 @@ do_lport_del(struct ovs_cmdl_context *ctx)
> > return;
> > }
> >
> > - nbrec_logical_port_delete(lport);
> > -}
> > -
> > -static bool
> > -is_lswitch(const struct nbrec_logical_switch *lswitch,
> > - struct uuid *lswitch_uuid, const char *name)
> > -{
> > - if (lswitch_uuid) {
> > - return uuid_equals(lswitch_uuid, &lswitch->header_.uuid);
> > - } else {
> > - return !strcmp(lswitch->name, name);
> > + /* Find the switch that contains 'lport', then delete it. */
> > + const struct nbrec_logical_switch *lswitch;
> > + NBREC_LOGICAL_SWITCH_FOR_EACH (lswitch, nb_ctx->idl) {
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + if (lswitch->ports[i] == lport) {
> > + remove_lport(lswitch, i);
> > + return;
> > + }
> > + }
> > }
> > -}
> >
> > + VLOG_WARN("logical port %s is not part of any logical switch",
> > + ctx->argv[1]);
> > +}
> >
> > static void
> > do_lport_list(struct ovs_cmdl_context *ctx)
> > {
> > struct nbctl_context *nb_ctx = ctx->pvt;
> > const char *id = ctx->argv[1];
> > - const struct nbrec_logical_port *lport;
> > - bool is_uuid = false;
> > - struct uuid lswitch_uuid;
> > + const struct nbrec_logical_switch *lswitch;
> >
> > - if (uuid_from_string(&lswitch_uuid, id)) {
> > - is_uuid = true;
> > + lswitch = lswitch_by_name_or_uuid(nb_ctx, id);
> > + if (!lswitch) {
> > + return;
> > }
> >
> > - NBREC_LOGICAL_PORT_FOR_EACH(lport, nb_ctx->idl) {
> > - bool match;
> > - if (is_uuid) {
> > - match = is_lswitch(lport->lswitch, &lswitch_uuid, NULL);
> > - } else {
> > - match = is_lswitch(lport->lswitch, NULL, id);
> > - }
> > - if (!match) {
> > - continue;
> > - }
> > + for (size_t i = 0; i < lswitch->n_ports; i++) {
> > + const struct nbrec_logical_port *lport = lswitch->ports[i];
> > printf(UUID_FMT " (%s)\n",
> > UUID_ARGS(&lport->header_.uuid), lport->name);
> > }
> > --
> > 2.1.3
> >
More information about the dev
mailing list