[ovs-dev] [PATCH v3 10/16] ovn-controller: Handle only relevant ports and flows.
Mickey Spiegel
mickeys.dev at gmail.com
Mon Dec 19 02:17:15 UTC 2016
On Sun, Dec 18, 2016 at 12:18 AM, Ben Pfaff <blp at ovn.org> wrote:
> On a particular hypervisor, ovn-controller only needs to handle ports
> and datapaths that have some relationship with it, that is, the
> ports that actually reside on the hypervisor, plus all the other ports on
> those ports' datapaths, plus all of the ports and datapaths that are
> reachable from those via logical patch ports. Until now, ovn-controller
> has done a poor job of limiting what it deals with to this set. This
> commit improves the situation.
>
> This commit gets rid of the concept of a "patched_datapath" which until now
> was used to represent any datapath that contained a logical patch port.
> Previously, the concept of a "local_datapath" meant a datapath with a VIF
> that resides on the local hypervisor. This commit extends that concept to
> include any other datapath that can be reached from a VIF on the local
> hypervisor, which is a simplification that makes the code easier to
> understand in a few places.
>
> CC: Gurucharan Shetty <guru at ovn.org>
> Signed-off-by: Ben Pfaff <blp at ovn.org>
>
Acked-by: Mickey Spiegel <mickeys.dev at gmail.com>
A couple of comments below.
---
> ovn/controller/binding.c | 75 ++++++++++++++++++++++++++-----
> ovn/controller/binding.h | 7 ++-
> ovn/controller/lflow.c | 46 ++-----------------
> ovn/controller/lflow.h | 1 -
> ovn/controller/ovn-controller.c | 49 ++++++---------------
> ovn/controller/ovn-controller.h | 33 +++++++-------
> ovn/controller/patch.c | 98 ++++++++++++++++--------------
> -----------
> ovn/controller/patch.h | 5 +--
> ovn/controller/physical.c | 30 ++-----------
> ovn/controller/physical.h | 4 +-
> tests/ovn-controller.at | 19 +++++++-
> 11 files changed, 165 insertions(+), 202 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index fb76032..2759e23 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
>
<snip>
@@ -293,7 +339,8 @@ consider_local_datapath(struct controller_ctx *ctx,
> /* Add child logical port to the set of all local ports. */
> sset_add(all_lports, binding_rec->logical_port);
> }
> - add_local_datapath(local_datapaths, binding_rec);
> + 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);
> }
> @@ -328,7 +375,8 @@ consider_local_datapath(struct controller_ctx *ctx,
> }
>
> sset_add(all_lports, binding_rec->logical_port);
> - add_local_datapath(local_datapaths, binding_rec);
> + add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> + false, local_datapaths);
> if (binding_rec->chassis == chassis_rec) {
> return;
> }
> @@ -342,7 +390,8 @@ consider_local_datapath(struct controller_ctx *ctx,
> const char *chassis = smap_get(&binding_rec->options,
> "l3gateway-chassis");
> if (!strcmp(chassis, chassis_rec->name) && ctx->ovnsb_idl_txn) {
>
Not sure if you want to cover it in this patch, but since you are
making changes to add_local_datapath, I thought I would mention
it. When reviewing the datapaths of interest patch, Darrell and I
stumbled upon the realization that there is no reason for
"&& ctx->ovnsb_idl_txn" above to be part of the condition. It is
not used in the call to add_local_datapath. Similar code above
for vifs and for l2gateways has no such condition on
ctx->ovnsb_idl_txn.
> - add_local_datapath(local_datapaths, binding_rec);
> + add_local_datapath(ldatapaths, lports, binding_rec->datapath,
> + true, local_datapaths);
> }
> } else if (chassis_rec && binding_rec->chassis == chassis_rec) {
> if (ctx->ovnsb_idl_txn) {
>
<snip>
> --- a/ovn/controller/physical.c
> +++ b/ovn/controller/physical.c
> @@ -157,36 +157,13 @@ static void
> consider_port_binding(enum mf_field_id mff_ovn_geneve,
> const struct simap *ct_zones,
> struct hmap *local_datapaths,
> - struct hmap *patched_datapaths,
> const struct sbrec_port_binding *binding,
> struct ofpbuf *ofpacts_p,
> struct hmap *flow_table)
> {
> - /* Skip the port binding if the port is on a datapath that is neither
> - * local nor with any logical patch port connected, because local
> ports
> - * would never need to talk to those ports.
> - *
> - * Even with this approach there could still be unnecessary port
> - * bindings processed. A better approach would be a kind of "flood
> - * fill" algorithm:
> - *
> - * 1. Initialize set S to the logical datapaths that have a port
> - * located on the hypervisor.
> - *
> - * 2. For each patch port P in a logical datapath in S, add the
> - * logical datapath of the remote end of P to S. Iterate
> - * until S reaches a fixed point.
> - *
> - * This can be implemented in northd, which can generate the sets and
> - * save it on each port-binding record in SB, and ovn-controller can
> - * use the information directly. However, there can be update storms
> - * when a pair of patch ports are added/removed to connect/disconnect
> - * large lrouters and lswitches. This need to be studied further.
> - */
> uint32_t dp_key = binding->datapath->tunnel_key;
> uint32_t port_key = binding->tunnel_key;
> - if (!get_local_datapath(local_datapaths, dp_key)
> - && !get_patched_datapath(patched_datapaths, dp_key)) {
> + if (!get_local_datapath(local_datapaths, dp_key)) {
> return;
> }
I wonder why there is no similar check on consider_mc_group.
Is there any reason for mc_group related physical flows to be
programmed for a datapath if there are no port related physical
flows or logical flows programmed for that datapath?
I tried the following diff:
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 41673e5..6bca413 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -493,6 +493,11 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
struct ofpbuf *remote_ofpacts_p,
struct hmap *flow_table)
{
+ uint32_t dp_key = mc->datapath->tunnel_key;
+ if (!get_local_datapath(local_datapaths, dp_key)) {
+ return;
+ }
+
struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
struct match match;
It passed all automated tests except for one that seems to be
written too narrowly at line 5408 in tests/ovn.at. The test passed
when I commented out that line.
Mickey
More information about the dev
mailing list