[ovs-dev] [PATCH] ovn: Take advantage of OVSDB garbage collection in OVN_Northbound schema.

Ben Pfaff blp at nicira.com
Thu Jun 25 23:35:24 UTC 2015


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