On Fri, Apr 21, 2017 at 10:11 AM, Daniel Alvarez <dalva...@redhat.com> wrote: > This patch introduces a new type of OVN ports called "localport". > These ports will be present in every hypervisor and may have the > same IP/MAC addresses. They are not bound to any chassis and traffic > to these ports will never go through a tunnel. > > Its main use case is the OpenStack metadata API support which relies > on a local agent running on every hypervisor and serving metadata to > VM's locally. This service is described in detail at [0]. > > TODO: tests
will also need to document the new port type in ovn-nb.xml and ovn-sb.xml. ovn-controller.xml should be updated documenting the new external-id set on rows in the open_vswitch db. > > [0] https://review.openstack.org/#/c/452811/ > > Signed-off-by: Daniel Alvarez <dalva...@redhat.com> > --- > ovn/controller/binding.c | 87 > +++++++++++++++++++++++++++++++++++++-------- > ovn/controller/physical.c | 53 +++++++++++++++++++++++++++ > ovn/northd/ovn-northd.8.xml | 4 +-- > ovn/northd/ovn-northd.c | 6 ++-- > ovn/ovn-architecture.7.xml | 20 ++++++++--- > 5 files changed, 147 insertions(+), 23 deletions(-) > > diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c > index 95e9deb..f96127c 100644 > --- a/ovn/controller/binding.c > +++ b/ovn/controller/binding.c > @@ -352,6 +352,43 @@ setup_qos(const char *egress_iface, struct hmap > *queue_map) > hmap_destroy(&consistent_queues); > netdev_close(netdev_phy); > } > +static bool > +set_ovsport_external_ids(struct controller_ctx *ctx, > + const struct ovsrec_bridge *bridge, > + const struct ovsrec_interface *iface_rec, > + const char *key, > + const char *value) > +{ > + if (!ctx->ovs_idl_txn) { > + return false; > + } > + > + /* Find the port with this interface and add the key/value pair to its > + * external_ids. Assume 1-1 relationship between port and interface. */ > + int i; > + for (i = 0; i < bridge->n_ports; i++) { > + const struct ovsrec_port *port_rec = bridge->ports[i]; > + int j; > + > + if (!strcmp(port_rec->name, bridge->name)) { > + continue; > + } > + > + for (j = 0; j < port_rec->n_interfaces; j++) { > + if (port_rec->interfaces[j] == iface_rec) { > + struct smap new_ids; > + smap_clone(&new_ids, &port_rec->external_ids); > + smap_replace(&new_ids, key, value); > + ovsrec_port_verify_external_ids(port_rec); > + ovsrec_port_set_external_ids(port_rec, &new_ids); > + smap_destroy(&new_ids); > + return true; > + } > + } > + } > + return false; > +} > + > > static void > consider_local_datapath(struct controller_ctx *ctx, > @@ -359,6 +396,7 @@ consider_local_datapath(struct controller_ctx *ctx, > const struct lport_index *lports, > const struct sbrec_chassis *chassis_rec, > const struct sbrec_port_binding *binding_rec, > + const struct ovsrec_bridge *bridge, > struct hmap *qos_map, > struct hmap *local_datapaths, > struct shash *lport_to_iface, > @@ -368,19 +406,40 @@ consider_local_datapath(struct controller_ctx *ctx, > = shash_find_data(lport_to_iface, binding_rec->logical_port); > > bool our_chassis = false; > - if (iface_rec > - || (binding_rec->parent_port && binding_rec->parent_port[0] && > - sset_contains(local_lports, binding_rec->parent_port))) { > - if (binding_rec->parent_port && binding_rec->parent_port[0]) { > - /* Add child logical port to the set of all local ports. */ > - sset_add(local_lports, binding_rec->logical_port); > - } > - add_local_datapath(ldatapaths, lports, binding_rec->datapath, > - false, local_datapaths); > - if (iface_rec && qos_map && ctx->ovs_idl_txn) { > - get_qos_params(binding_rec, qos_map); > + > + if (iface_rec && !strcmp(binding_rec->type, "localport")) { > + /* Make sure localport external_id is present, otherwise set it > + * for both interface and port. This should only happen the first > + * time. We set the value on both the interface and port because it's > + * most convenient to check for the value here on the interface. > Later, > + * we need to read this value in physical.c, and expect to read it > from > + * the port there.*/ > + if (!smap_get(&iface_rec->external_ids, "ovn-localport-port")) { > + if (set_ovsport_external_ids(ctx, bridge, iface_rec, > + "ovn-localport-port", > + binding_rec->logical_port)) { > + struct smap new_ids; > + smap_clone(&new_ids, &iface_rec->external_ids); > + smap_replace(&new_ids, "ovn-localport-port", > + binding_rec->logical_port); > + ovsrec_interface_verify_external_ids(iface_rec); > + ovsrec_interface_set_external_ids(iface_rec, &new_ids); > + smap_destroy(&new_ids); > + } > } > - our_chassis = true; > + } else if (iface_rec > + || (binding_rec->parent_port && binding_rec->parent_port[0] && > + sset_contains(local_lports, binding_rec->parent_port))) { > + if (binding_rec->parent_port && binding_rec->parent_port[0]) { > + /* Add child logical port to the set of all local ports. > */ > + sset_add(local_lports, binding_rec->logical_port); > + } > + add_local_datapath(ldatapaths, lports, binding_rec->datapath, > + false, local_datapaths); > + if (iface_rec && qos_map && ctx->ovs_idl_txn) { > + get_qos_params(binding_rec, qos_map); > + } > + our_chassis = true; > } else if (!strcmp(binding_rec->type, "l2gateway")) { > const char *chassis_id = smap_get(&binding_rec->options, > "l2gateway-chassis"); > @@ -468,8 +527,8 @@ binding_run(struct controller_ctx *ctx, const struct > ovsrec_bridge *br_int, > consider_local_datapath(ctx, ldatapaths, lports, > chassis_rec, binding_rec, > sset_is_empty(&egress_ifaces) ? NULL : > - &qos_map, local_datapaths, &lport_to_iface, > - local_lports); > + br_int, &qos_map, local_datapaths, > + &lport_to_iface, local_lports); > > } > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index 0f1aa63..a64ac4b 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -59,6 +59,8 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl) > static struct simap localvif_to_ofport = > SIMAP_INITIALIZER(&localvif_to_ofport); > static struct hmap tunnels = HMAP_INITIALIZER(&tunnels); > +static struct simap localport_to_ofport = > + SIMAP_INITIALIZER(&localport_to_ofport); > > /* Maps from a chassis to the OpenFlow port number of the tunnel that can be > * used to reach that chassis. */ > @@ -601,6 +603,27 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve, > } else { > /* Remote port connected by tunnel */ > > + /* Table 32, priority 150. > + * ======================= > + * > + * Drop traffic originated from a localport to a remote destination. > + */ > + struct simap_node *localport; > + SIMAP_FOR_EACH (localport, &localport_to_ofport) { > + int inport; > + if ((inport = simap_get(&localport_to_ofport, localport->name))) > { > + match_init_catchall(&match); > + ofpbuf_clear(ofpacts_p); > + /* Match localport in_port. */ > + match_set_in_port(&match, inport); > + /* Match MFF_LOG_DATAPATH, MFF_LOG_OUTPORT. */ > + match_set_metadata(&match, htonll(dp_key)); > + match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, port_key); > + ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 150, 0, > + &match, ofpacts_p); > + } > + } > + > /* Table 32, priority 100. > * ======================= > * > @@ -768,6 +791,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id > mff_ovn_geneve, > SIMAP_INITIALIZER(&new_localvif_to_ofport); > struct simap new_tunnel_to_ofport = > SIMAP_INITIALIZER(&new_tunnel_to_ofport); > + struct simap new_localport_to_ofport = > + SIMAP_INITIALIZER(&new_localport_to_ofport); > + > for (int i = 0; i < br_int->n_ports; i++) { > const struct ovsrec_port *port_rec = br_int->ports[i]; > if (!strcmp(port_rec->name, br_int->name)) { > @@ -784,6 +810,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id > mff_ovn_geneve, > "ovn-localnet-port"); > const char *l2gateway = smap_get(&port_rec->external_ids, > "ovn-l2gateway-port"); > + const char *localport = smap_get(&port_rec->external_ids, > + "ovn-localport-port"); > > for (int j = 0; j < port_rec->n_interfaces; j++) { > const struct ovsrec_interface *iface_rec = > port_rec->interfaces[j]; > @@ -848,6 +876,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id > mff_ovn_geneve, > "iface-id"); > if (iface_id) { > simap_put(&new_localvif_to_ofport, iface_id, ofport); > + if (localport) { > + simap_put(&new_localport_to_ofport, localport, > ofport); > + } > } > } > } > @@ -884,6 +915,27 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > physical_map_changed = true; > } > } > + > + /* Do the same for all localports. */ > + SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, &localport_to_ofport) { > + int newport; > + if ((newport = simap_get(&new_localport_to_ofport, vif_name->name))) > { > + if (newport != simap_get(&localport_to_ofport, vif_name->name)) { > + simap_put(&localport_to_ofport, vif_name->name, newport); > + physical_map_changed = true; > + } > + } else { > + simap_find_and_delete(&localport_to_ofport, vif_name->name); > + physical_map_changed = true; > + } > + } > + SIMAP_FOR_EACH (vif_name, &new_localport_to_ofport) { > + if (!simap_get(&localport_to_ofport, vif_name->name)) { > + simap_put(&localport_to_ofport, vif_name->name, > + simap_get(&new_localport_to_ofport, vif_name->name)); > + physical_map_changed = true; > + } > + } It looks like this block of code is nearly a duplicate of the code above it, right? How about turning it into a function that takes &new_localport_to_ofport and &localport_to_ofport as arguments? > if (physical_map_changed) { > /* Reprocess logical flow table immediately. */ > poll_immediate_wake(); > @@ -1043,4 +1095,5 @@ physical_run(struct controller_ctx *ctx, enum > mf_field_id mff_ovn_geneve, > > simap_destroy(&new_localvif_to_ofport); > simap_destroy(&new_tunnel_to_ofport); > + simap_destroy(&new_localport_to_ofport); > } > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml > index ab8fd88..db28736 100644 > --- a/ovn/northd/ovn-northd.8.xml > +++ b/ovn/northd/ovn-northd.8.xml > @@ -521,8 +521,8 @@ output; > </pre> > > <p> > - These flows are omitted for logical ports (other than router ports) > - that are down. > + These flows are omitted for logical ports (other than router > ports > + or <code>localport</code> ports) that are down. I believe this comment needs to be updated in two places: once for the ARP responder flows and again for the IPv6 ND flows. > </p> > </li> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 5a2e5ab..09f55c8 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -3166,9 +3166,11 @@ build_lswitch_flows(struct hmap *datapaths, struct > hmap *ports, > /* > * Add ARP/ND reply flows if either the > * - port is up or > - * - port type is router > + * - port type is router or > + * - port type is localport > */ > - if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router")) { > + if (!lsp_is_up(op->nbsp) && strcmp(op->nbsp->type, "router") && > + strcmp(op->nbsp->type, "localport")) { > continue; > } > > diff --git a/ovn/ovn-architecture.7.xml b/ovn/ovn-architecture.7.xml > index d8114f1..2866666 100644 > --- a/ovn/ovn-architecture.7.xml > +++ b/ovn/ovn-architecture.7.xml > @@ -409,6 +409,15 @@ > logical patch ports at each such point of connectivity, one on > each side. > </li> > + <li> > + <dfn>Localport ports</dfn> represent the points of local > + connectivity between logical switches and VIFs. These ports are > + present in every chassis (not bound to any particular one) and > + traffic from them will never go through a tunnel. A > + <code>localport</code> is expected to only generate traffic > destined > + for a local destination, typically in response to a request it > + received. > + </li> > </ul> > </li> > </ul> > @@ -986,11 +995,12 @@ > hypervisor. Each flow's actions implement sending a packet to the > port > it matches. For unicast logical output ports on remote hypervisors, > the actions set the tunnel key to the correct value, then send the > - packet on the tunnel port to the correct hypervisor. (When the > remote > - hypervisor receives the packet, table 0 there will recognize it as a > - tunneled packet and pass it along to table 33.) For multicast > logical > - output ports, the actions send one copy of the packet to each remote > - hypervisor, in the same way as for unicast destinations. If a > + packet on the tunnel port to the correct hypervisor (unless the > packet > + comes from a localport, in which case it will be dropped). (When the > + remote hypervisor receives the packet, table 0 there will recognize > it > + as a tunneled packet and pass it along to table 33.) For multicast > + logical output ports, the actions send one copy of the packet to each > + remote hypervisor, in the same way as for unicast destinations. If a > multicast group includes a logical port or ports on the local > hypervisor, then its actions also resubmit to table 33. Table 32 > also > includes a fallback flow that resubmits to table 33 if there is no > -- > 1.8.3.1 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev -- Russell Bryant _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
