Awesome thanks Ben! I'll update neutron tomorrow morning to work with this.
> On Jun 25, 2015, at 5:35 PM, Ben Pfaff <b...@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 <aro...@vmware.com>
> Signed-off-by: Ben Pfaff <b...@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
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
