[ovs-dev] [PATCH ovn v3 3/3] ovn-controller: Fix incremental processing for logical port references.

Han Zhou hzhou at ovn.org
Thu Jun 24 17:20:13 UTC 2021


On Thu, Jun 24, 2021 at 7:46 AM Numan Siddique <numans at ovn.org> wrote:
>
> On Wed, Jun 23, 2021 at 8:10 PM Han Zhou <hzhou at ovn.org> wrote:
> >
> > On Wed, Jun 23, 2021 at 7:48 AM Numan Siddique <numans at ovn.org> wrote:
> > >
> > > On Mon, Jun 21, 2021 at 2:52 AM Han Zhou <hzhou at ovn.org> wrote:
> > > >
> > > > If a lflow has an lport name in the match, but when the lflow is
> > > > processed the port-binding is not seen by ovn-controller, the
> > > > corresponding openflow will not be created. Later if the
port-binding is
> > > > created/monitored by ovn-controller, the lflow is not reprocessed
> > > > because the lflow didn't change and ovn-controller doesn't know
that the
> > > > port-binding affects the lflow. This patch fixes the problem by
tracking
> > > > the references when parsing the lflow, even if the port-binding is
not
> > > > found when the lflow is firstly parsed. A test case is also added to
> > > > cover the scenario.
> > > >
> > > > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > >
> > > Hi Han,
> > >
> > > Thanks for fixing these issues.   I've a few questions.  I haven't
> > > reviewed the patch completely.
> > >
> > >
> > > > ---
> > > >  controller/lflow.c          | 63
++++++++++++++++++++++++++-----------
> > > >  controller/lflow.h          |  3 ++
> > > >  controller/ovn-controller.c | 35 ++++++++++++++++-----
> > > >  include/ovn/expr.h          |  2 +-
> > > >  lib/expr.c                  | 14 +++------
> > > >  tests/ovn.at                | 47 +++++++++++++++++++++++++++
> > > >  tests/test-ovn.c            |  4 +--
> > > >  utilities/ovn-trace.c       |  2 +-
> > > >  8 files changed, 132 insertions(+), 38 deletions(-)
> > > >
> > > > diff --git a/controller/lflow.c b/controller/lflow.c
> > > > index 34eca135a..b7699a309 100644
> > > > --- a/controller/lflow.c
> > > > +++ b/controller/lflow.c
> > > > @@ -61,6 +61,7 @@ struct lookup_port_aux {
> > > >
> > > >  struct condition_aux {
> > > >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > > > +    const struct sbrec_datapath_binding *dp;
> > > >      const struct sbrec_chassis *chassis;
> > > >      const struct sset *active_tunnels;
> > > >      const struct sbrec_logical_flow *lflow;
> > > > @@ -98,6 +99,12 @@ lookup_port_cb(const void *aux_, const char
> > *port_name, unsigned int *portp)
> > > >
> > > >      const struct lookup_port_aux *aux = aux_;
> > > >
> > > > +    /* Store the name that used to lookup the lport to lflow
> > reference, so that
> > > > +     * in the future when the lport's port binding changes, the
> > logical flow
> > > > +     * that references this lport can be reprocessed. */
> > > > +    lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
> > > > +                       &aux->lflow->header_.uuid);
> > > > +
> > > >      const struct sbrec_port_binding *pb
> > > >          = lport_lookup_by_name(aux->sbrec_port_binding_by_name,
> > port_name);
> > > >      if (pb && pb->datapath == aux->dp) {
> > > > @@ -149,19 +156,18 @@ is_chassis_resident_cb(const void *c_aux_,
const
> > char *port_name)
> > > >  {
> > > >      const struct condition_aux *c_aux = c_aux_;
> > > >
> > > > +    /* Store the port name that used to lookup the lport to lflow
> > reference, so
> > > > +     * that in the future when the lport's port-binding changes the
> > logical
> > > > +     * flow that references this lport can be reprocessed. */
> > > > +    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING,
port_name,
> > > > +                       &c_aux->lflow->header_.uuid);
> > > > +
> > > >      const struct sbrec_port_binding *pb
> > > >          = lport_lookup_by_name(c_aux->sbrec_port_binding_by_name,
> > port_name);
> > > >      if (!pb) {
> > > >          return false;
> > > >      }
> > > >
> > > > -    /* Store the port_name to lflow reference. */
> > > > -    int64_t dp_id = pb->datapath->tunnel_key;
> > > > -    char buf[16];
> > > > -    get_unique_lport_key(dp_id, pb->tunnel_key, buf, sizeof(buf));
> > > > -    lflow_resource_add(c_aux->lfrr, REF_TYPE_PORTBINDING, buf,
> > > > -                       &c_aux->lflow->header_.uuid);
> > > > -
> > > >      if (strcmp(pb->type, "chassisredirect")) {
> > > >          /* for non-chassisredirect ports */
> > > >          return pb->chassis && pb->chassis == c_aux->chassis;
> > > > @@ -623,8 +629,6 @@ add_matches_to_flow_table(const struct
> > sbrec_logical_flow *lflow,
> > > >                  int64_t dp_id = dp->tunnel_key;
> > > >                  char buf[16];
> > > >                  get_unique_lport_key(dp_id, port_id, buf,
sizeof(buf));
> > > > -                lflow_resource_add(l_ctx_out->lfrr,
> > REF_TYPE_PORTBINDING, buf,
> > > > -                                   &lflow->header_.uuid);
> > > >                  if (!sset_contains(l_ctx_in->local_lport_ids,
buf)) {
> > > >                      VLOG_DBG("lflow "UUID_FMT
> > > >                               " port %s in match is not local,
skip",
> > > > @@ -788,6 +792,7 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >      };
> > > >      struct condition_aux cond_aux = {
> > > >          .sbrec_port_binding_by_name =
> > l_ctx_in->sbrec_port_binding_by_name,
> > > > +        .dp = dp,
> > > >          .chassis = l_ctx_in->chassis,
> > > >          .active_tunnels = l_ctx_in->active_tunnels,
> > > >          .lflow = lflow,
> > > > @@ -805,7 +810,6 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >      struct hmap *matches = NULL;
> > > >      size_t matches_size = 0;
> > > >
> > > > -    bool is_cr_cond_present = false;
> > > >      bool pg_addr_set_ref = false;
> > > >      uint32_t n_conjs = 0;
> > > >
> > > > @@ -843,8 +847,8 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >      case LCACHE_T_NONE:
> > > >      case LCACHE_T_CONJ_ID:
> > > >      case LCACHE_T_EXPR:
> > > > -        expr = expr_evaluate_condition(expr,
is_chassis_resident_cb,
> > &cond_aux,
> > > > -                                       &is_cr_cond_present);
> > >
> > > I'm not very clear why the variable "is_cr_cond_present" not required
any
> > more.
> > >
> > > For logical flows with "is_chassis_resident" match condition, we cache
> > > only the "expr" right ?
> > >
> > > What is the behavior after this patch ?
> > >
> > Thanks Numan for the review!
> >
> > After this patch, it is the same for "is_chassis_resident", which we
cache
> > only the "expr" just like before.
> > However, it changes the cache behavior for lflows that reference logical
> > ports. Some of the lflows may previously cache the "match", but now they
> > will cache "expr" only. This is because the logical port parsing is done
> > when converting expr to matches. So we can't cache the match. Otherwise
the
> > test case would fail.
> >
> > >
> > > > +        expr = expr_evaluate_condition(expr,
is_chassis_resident_cb,
> > > > +                                       &cond_aux);
> > > >          expr = expr_normalize(expr);
> > > >          break;
> > > >      case LCACHE_T_MATCHES:
> > > > @@ -883,7 +887,9 @@ consider_logical_flow__(const struct
> > sbrec_logical_flow *lflow,
> > > >
> > > >          /* Cache new entry if caching is enabled. */
> > > >          if (lflow_cache_is_enabled(l_ctx_out->lflow_cache)) {
> > > > -            if (cached_expr && !is_cr_cond_present) {
> > > > +            if (cached_expr
> > > > +                &&
!lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
> > > > +                                     &lflow->header_.uuid)) {
> > >
> > > This check - lflow_ref_lookup() is not very clear to me.  Can you
> > > please explain a bit about this ?
> > >
> > > From what I understand we cache the expr matches if the flow is not
> > > referenced by resources.
> > >
> > > Before this patch we were caching the 'expr matches' for a logical
> > > flow like - 'ip4 && inport == "lp1"'
> > > But now since there is a reference for this logical flow in the
> > > lflow_ref table, we only cache the expr tree.
> > > Is this intentional ?  Correct me if my understanding is wrong.
> >
> > Yes this is intentional as explained above. We can't cache the match
> > whenever a lport is referenced by the logical flow, which includes
> > "is_chassis_resident" and other lport references as well.
> > There is a performance impact of the effectiveness of lflow-cache. I
reran
> > the scale test with a setup closer to an ovn-k8s setup with 500
chassises
> > and 5 LSPs per chassis, and GRs, some ACLs with port-group/address-set
size
> > 1k, and some LBs, etc. On my test machine the time spent by
ovn-controller
> > for recompute increased from 2.9s to 3.2s, around 10% increase. (of
course
> > the memory cost is much lower, around 40% less). I didn't notice this
> > obvious performance difference from an earlier test when I was replying
to
> > Dumitru, probably because I messed up with the lflow-cache flag
settings.
> > (I reran the tests multiple times and results are consistent so I
believe
> > there were mistakes in my earlier test).
> >
> > Probably this ~10% difference is still not that much for the recompute
> > scenario from my perspective, while the scenario I am trying to fix may
not
> > be critical either, and I am not very sure if this not-so-critical fix
is
> > worth the not-so-critical performance cost. I just feel it is better to
be
> > correct, unless we know exactly what are the corner cases that would
lead
> > to a lflow being processed before the port-binding in SB is seen by the
> > ovn-controller. The test case I crafted is one but not sure if there are
> > others. Let me know your thoughts on this.
> >
> > As a note, I also retested both versions with lflow-cache disabled, and
the
> > performance of recompute with this patch is in fact slightly better ~2%
for
> > the same scenario. In our current deployment we have critical concerns
for
> > memory, so didn't enable lflow-cache for now.
>
>
>
> Thanks for the detailed explanation.
>
> So we only cache the expr tree for the flows which match on a logical port
> and that logical port is not seen yet.  I guess this should be fine.
>
This is half correct. We can't cache the match even if the logical port is
seen, because if it disappears we need to uninstall the flow. So whenever a
port-binding is added or deleted we need to reprocess the lflows that
reference the logical port. I think I can update the test case to cover
this as well.

>
> Instead of doing
>
>  if (cached_expr  && !lflow_ref_lookup(&l_ctx_out->lfrr->lflow_ref_table,
>
> &lflow->header_.uuid))
>
> We could probably do something like
>
> if (cached_expr && !is_cr_cond_present && lookup_port_succeeded) {
>    // cache expr matches ..
> } else {
>   //  cache expr tree
> }
> ..
> ..
>
>
> lookup_port_cb() can probably set lookup_port_succeeded to false if
> the lookup failed.
>
> This would avoid the lflow_ref_lookup().
>

For the reason mentioned above, we can't make this change. In fact, I
wouldn't worry much about lflow_ref_lookup()'s cost. It is O(1) operation.
If it really turns out to be a bottleneck, we could optimize the
function/data-structure, without worrying about the logic.
The real performance impact part is probably not being able to cache the
"match" for lflows that have logical port references, but I will work on
some other solutions to optimize that.

> > >
> > >
> > > >                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
> > > >                                          &lflow->header_.uuid,
matches,
> > > >                                          matches_size);
> > > > @@ -1746,21 +1752,42 @@ lflow_processing_end:
> > > >      return handled;
> > > >  }
> > > >
> > > > +/* Handles a port-binding change that is possibly related to a
lport's
> > > > + * residence status on this chassis. */
> > > >  bool
> > > >  lflow_handle_flows_for_lport(const struct sbrec_port_binding *pb,
> > > >                               struct lflow_ctx_in *l_ctx_in,
> > > >                               struct lflow_ctx_out *l_ctx_out)
> > > >  {
> > > >      bool changed;
> > > > -    int64_t dp_id = pb->datapath->tunnel_key;
> > > > -    char pb_ref_name[16];
> > > > -    get_unique_lport_key(dp_id, pb->tunnel_key, pb_ref_name,
> > > > -                         sizeof(pb_ref_name));
> > > >
> > > > -    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
pb_ref_name,
> > > > +    return lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> > pb->logical_port,
> > > >                                      l_ctx_in, l_ctx_out, &changed);
> > > >  }
> > > >
> > > > +/* Handles port-binding add/deletions. */
> > > > +bool
> > > > +lflow_handle_changed_port_bindings(struct lflow_ctx_in *l_ctx_in,
> > > > +                                   struct lflow_ctx_out *l_ctx_out)
> > > > +{
> > > > +    bool ret = true;
> > > > +    bool changed;
> > > > +    const struct sbrec_port_binding *pb;
> > > > +    SBREC_PORT_BINDING_TABLE_FOR_EACH_TRACKED (pb,
> > > > +
> > l_ctx_in->port_binding_table) {
> > > > +        if (!sbrec_port_binding_is_new(pb)
> > > > +            && !sbrec_port_binding_is_deleted(pb)) {
> > > > +            continue;
> > > > +        }
> > > > +        if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
> > pb->logical_port,
> > > > +                                      l_ctx_in, l_ctx_out,
&changed)) {
> > > > +            ret = false;
> > > > +            break;
> > > > +        }
> > > > +    }
> > > > +    return ret;
> > > > +}
> > > > +
> > >
> > > One downside of adding the port binding handler is that we will
process
> > > a logical flow 'A' twice if a port binding is created and it is
> > > claimed in the same loop
> > > as the added test case does for the port - "lp2".
> > >
> > > We do that first in lflow_handle_flows_for_lport() and then in
> > > lflow_handle_changed_port_bindings().
> > >
> > Yes, in such cases the lflow will be handled twice. However, it is a
> > general problem in I-P engine for different input changes in the same
> > iteration. Although it is not a critical penalty in most cases because
of
> > I-P, I am still trying to optimize this to make sure a lflow is handled
at
> > most once in one engine run, so that for lflows with a big
> > port-group/address-set reference won't cause more latency in I-P.
> >
> > > I'm also not very clear why we need the port_binding handler here ?
> > > Can you please give a scenario for this ?
> > > The added test case passes for me even If I make the function
> > > lflow_handle_changed_port_bindings()
> > > return at the beginning without doing anything.
> >
> > The lflow_handle_flows_for_lport() is called only when a port-binding is
> > affecting local residency, while lflow_handle_changed_port_bindings()
takes
> > care of all port-binding adding/deletions, including the ports bound on
> > other chassises. The test case I used only one HV so it didn't matter
even
> > if you returned directly. (but the current test case fails without this
> > patch). Probably I should update the test with two HVs to cover the
> > scenario better.
>
> Ok.  I think it would be great to enhance the test with two HVs.

Ok, will do.

Thanks,
han

>
> Thanks
> Numan
>
> >
> > Thanks,
> > Han
> >
> > >
> > > Thanks
> > > Numan
> > >
> > > >  bool
> > > >  lflow_handle_changed_mc_groups(struct lflow_ctx_in *l_ctx_in,
> > > >                                 struct lflow_ctx_out *l_ctx_out)
> > > > diff --git a/controller/lflow.h b/controller/lflow.h
> > > > index e98edf81d..9d8882ae5 100644
> > > > --- a/controller/lflow.h
> > > > +++ b/controller/lflow.h
> > > > @@ -130,6 +130,7 @@ struct lflow_ctx_in {
> > > >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_datapath;
> > > >      struct ovsdb_idl_index *sbrec_logical_flow_by_logical_dp_group;
> > > >      struct ovsdb_idl_index *sbrec_port_binding_by_name;
> > > > +    const struct sbrec_port_binding_table *port_binding_table;
> > > >      const struct sbrec_dhcp_options_table *dhcp_options_table;
> > > >      const struct sbrec_dhcpv6_options_table *dhcpv6_options_table;
> > > >      const struct sbrec_datapath_binding_table *dp_binding_table;
> > > > @@ -182,4 +183,6 @@ bool lflow_handle_flows_for_lport(const struct
> > sbrec_port_binding *,
> > > >                                    struct lflow_ctx_out *);
> > > >  bool lflow_handle_changed_mc_groups(struct lflow_ctx_in *,
> > > >                                      struct lflow_ctx_out *);
> > > > +bool lflow_handle_changed_port_bindings(struct lflow_ctx_in *,
> > > > +                                        struct lflow_ctx_out *);
> > > >  #endif /* controller/lflow.h */
> > > > diff --git a/controller/ovn-controller.c
b/controller/ovn-controller.c
> > > > index c28fd6f2d..8136afe5c 100644
> > > > --- a/controller/ovn-controller.c
> > > > +++ b/controller/ovn-controller.c
> > > > @@ -1966,6 +1966,10 @@ init_lflow_ctx(struct engine_node *node,
> > > >                  engine_get_input("SB_multicast_group", node),
> > > >                  "name_datapath");
> > > >
> > > > +    struct sbrec_port_binding_table *port_binding_table =
> > > > +        (struct sbrec_port_binding_table *)EN_OVSDB_GET(
> > > > +            engine_get_input("SB_port_binding", node));
> > > > +
> > > >      struct sbrec_dhcp_options_table *dhcp_table =
> > > >          (struct sbrec_dhcp_options_table *)EN_OVSDB_GET(
> > > >              engine_get_input("SB_dhcp_options", node));
> > > > @@ -2029,6 +2033,7 @@ init_lflow_ctx(struct engine_node *node,
> > > >      l_ctx_in->sbrec_logical_flow_by_logical_dp_group =
> > > >          sbrec_logical_flow_by_dp_group;
> > > >      l_ctx_in->sbrec_port_binding_by_name =
sbrec_port_binding_by_name;
> > > > +    l_ctx_in->port_binding_table = port_binding_table;
> > > >      l_ctx_in->dhcp_options_table  = dhcp_table;
> > > >      l_ctx_in->dhcpv6_options_table = dhcpv6_table;
> > > >      l_ctx_in->mac_binding_table = mac_binding_table;
> > > > @@ -2221,6 +2226,25 @@
lflow_output_sb_multicast_group_handler(struct
> > engine_node *node, void *data)
> > > >      return true;
> > > >  }
> > > >
> > > > +static bool
> > > > +lflow_output_sb_port_binding_handler(struct engine_node *node, void
> > *data)
> > > > +{
> > > > +    struct ed_type_runtime_data *rt_data =
> > > > +        engine_get_input_data("runtime_data", node);
> > > > +
> > > > +    struct ed_type_lflow_output *lfo = data;
> > > > +
> > > > +    struct lflow_ctx_in l_ctx_in;
> > > > +    struct lflow_ctx_out l_ctx_out;
> > > > +    init_lflow_ctx(node, rt_data, lfo, &l_ctx_in, &l_ctx_out);
> > > > +    if (!lflow_handle_changed_port_bindings(&l_ctx_in,
&l_ctx_out)) {
> > > > +        return false;
> > > > +    }
> > > > +
> > > > +    engine_set_node_state(node, EN_UPDATED);
> > > > +    return true;
> > > > +}
> > > > +
> > > >  static bool
> > > >  _lflow_output_resource_ref_handler(struct engine_node *node, void
> > *data,
> > > >                                    enum ref_type ref_type)
> > > > @@ -2285,8 +2309,9 @@ _lflow_output_resource_ref_handler(struct
> > engine_node *node, void *data,
> > > >              break;
> > > >
> > > >          case REF_TYPE_PORTBINDING:
> > > > -            /* This ref type is handled in the
> > > > -             * flow_output_runtime_data_handler. */
> > > > +            /* This ref type is handled in:
> > > > +             * - flow_output_runtime_data_handler
> > > > +             * - flow_output_sb_port_binding_handler. */
> > > >          case REF_TYPE_MC_GROUP:
> > > >              /* This ref type is handled in the
> > > >               * flow_output_sb_multicast_group_handler. */
> > > > @@ -2895,12 +2920,8 @@ main(int argc, char *argv[])
> > > >      engine_add_input(&en_lflow_output, &en_sb_chassis,
> > > >                       pflow_lflow_output_sb_chassis_handler);
> > > >
> > > > -    /* Any changes to the port binding, need not be handled
> > > > -     * for lflow_outout engine.  We still need sb_port_binding
> > > > -     * as input to access the port binding data in lflow.c and
> > > > -     * hence the noop handler. */
> > > >      engine_add_input(&en_lflow_output, &en_sb_port_binding,
> > > > -                     engine_noop_handler);
> > > > +                     lflow_output_sb_port_binding_handler);
> > > >
> > > >      engine_add_input(&en_lflow_output, &en_ovs_open_vswitch, NULL);
> > > >      engine_add_input(&en_lflow_output, &en_ovs_bridge, NULL);
> > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > > > index de90e87e1..3b5653f7b 100644
> > > > --- a/include/ovn/expr.h
> > > > +++ b/include/ovn/expr.h
> > > > @@ -438,7 +438,7 @@ struct expr *expr_evaluate_condition(
> > > >      struct expr *,
> > > >      bool (*is_chassis_resident)(const void *c_aux,
> > > >                                  const char *port_name),
> > > > -    const void *c_aux, bool *condition_present);
> > > > +    const void *c_aux);
> > > >  struct expr *expr_normalize(struct expr *);
> > > >
> > > >  bool expr_honors_invariants(const struct expr *);
> > > > diff --git a/lib/expr.c b/lib/expr.c
> > > > index f728f9537..e3f6bb892 100644
> > > > --- a/lib/expr.c
> > > > +++ b/lib/expr.c
> > > > @@ -2121,16 +2121,13 @@ static struct expr *
> > > >  expr_evaluate_condition__(struct expr *expr,
> > > >                            bool (*is_chassis_resident)(const void
> > *c_aux,
> > > >                                                        const char
> > *port_name),
> > > > -                          const void *c_aux, bool
*condition_present)
> > > > +                          const void *c_aux)
> > > >  {
> > > >      bool result;
> > > >
> > > >      switch (expr->cond.type) {
> > > >      case EXPR_COND_CHASSIS_RESIDENT:
> > > >          result = is_chassis_resident(c_aux, expr->cond.string);
> > > > -        if (condition_present != NULL) {
> > > > -            *condition_present = true;
> > > > -        }
> > > >          break;
> > > >
> > > >      default:
> > > > @@ -2146,7 +2143,7 @@ struct expr *
> > > >  expr_evaluate_condition(struct expr *expr,
> > > >                          bool (*is_chassis_resident)(const void
*c_aux,
> > > >                                                      const char
> > *port_name),
> > > > -                        const void *c_aux, bool *condition_present)
> > > > +                        const void *c_aux)
> > > >  {
> > > >      struct expr *sub, *next;
> > > >
> > > > @@ -2156,15 +2153,14 @@ expr_evaluate_condition(struct expr *expr,
> > > >           LIST_FOR_EACH_SAFE (sub, next, node, &expr->andor) {
> > > >              ovs_list_remove(&sub->node);
> > > >              struct expr *e = expr_evaluate_condition(sub,
> > is_chassis_resident,
> > > > -                                                     c_aux,
> > condition_present);
> > > > +                                                     c_aux);
> > > >              e = expr_fix(e);
> > > >              expr_insert_andor(expr, next, e);
> > > >          }
> > > >          return expr_fix(expr);
> > > >
> > > >      case EXPR_T_CONDITION:
> > > > -        return expr_evaluate_condition__(expr, is_chassis_resident,
> > c_aux,
> > > > -                                         condition_present);
> > > > +        return expr_evaluate_condition__(expr, is_chassis_resident,
> > c_aux);
> > > >
> > > >      case EXPR_T_CMP:
> > > >      case EXPR_T_BOOLEAN:
> > > > @@ -3517,7 +3513,7 @@ expr_parse_microflow__(struct lexer *lexer,
> > > >
> > > >      e = expr_simplify(e);
> > > >      e = expr_evaluate_condition(e,
microflow_is_chassis_resident_cb,
> > > > -                                NULL, NULL);
> > > > +                                NULL);
> > > >      e = expr_normalize(e);
> > > >
> > > >      struct match m = MATCH_CATCHALL_INITIALIZER;
> > > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > > index bc494fcad..e6d609b5b 100644
> > > > --- a/tests/ovn.at
> > > > +++ b/tests/ovn.at
> > > > @@ -26782,3 +26782,50 @@ AT_CHECK([ovs-ofctl dump-flows br-int
"nw_src=
> > 10.0.0.0/24" | \
> > > >  OVN_CLEANUP([hv1])
> > > >  AT_CLEANUP
> > > >  ])
> > > > +
> > > > +# Test when a logical port name is used in an ACL before it is
> > created. When it
> > > > +# is created later, the ACL should be programmed as openflow rules
by
> > > > +# ovn-controller. Although this is not likely to happen in real
world
> > use
> > > > +# cases, it is possible that a port-binding is observed by
> > ovn-controller AFTER
> > > > +# an lflow that references the port is processed. So this test
case is
> > to make
> > > > +# sure the incremental processing in ovn-controller reprocesses the
> > lflow when
> > > > +# the port-binding is observed.
> > > > +OVN_FOR_EACH_NORTHD([
> > > > +AT_SETUP([ovn -- ACL referencing lport before creation])
> > > > +ovn_start
> > > > +
> > > > +net_add n1
> > > > +
> > > > +sim_add hv1
> > > > +as hv1
> > > > +check ovs-vsctl add-br br-phys
> > > > +ovn_attach n1 br-phys 192.168.0.1
> > > > +
> > > > +# Bind both lp1 and lp2 on the chasis.
> > > > +check ovs-vsctl add-port br-int lp1 -- set interface lp1
> > external_ids:iface-id=lp1
> > > > +check ovs-vsctl add-port br-int lp2 -- set interface lp2
> > external_ids:iface-id=lp2
> > > > +
> > > > +# Create only lport lp1, but not lp2.
> > > > +check ovn-nbctl ls-add lsw0
> > > > +check ovn-nbctl lsp-add lsw0 lp1 \
> > > > +    -- lsp-set-addresses lp1 "f0:00:00:00:00:01 10.0.0.11"
> > > > +
> > > > +# Each lport is referenced by an ACL.
> > > > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" &&
> > ip4.src == 10.0.0.111'  allow-related
> > > > +check ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp2" &&
> > ip4.src == 10.0.0.222'  allow-related
> > > > +
> > > > +# The first ACL should be programmed.
> > > > +check ovn-nbctl --wait=hv sync
> > > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.111],
> > [0], [ignore])
> > > > +
> > > > +# Now create the lport lp2.
> > > > +check ovn-nbctl lsp-add lsw0 lp2 \
> > > > +    -- lsp-set-addresses lp2 "f0:00:00:00:00:02 10.0.0.22"
> > > > +
> > > > +check ovn-nbctl --wait=hv sync
> > > > +# Now the second ACL should be programmed.
> > > > +AT_CHECK([ovs-ofctl dump-flows br-int table=44 | grep 10.0.0.222],
> > [0], [ignore])
> > > > +
> > > > +OVN_CLEANUP([hv1])
> > > > +AT_CLEANUP
> > > > +])
> > > > diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> > > > index 98cc2c503..c6d8b287b 100644
> > > > --- a/tests/test-ovn.c
> > > > +++ b/tests/test-ovn.c
> > > > @@ -318,7 +318,7 @@ test_parse_expr__(int steps)
> > > >              if (steps > 1) {
> > > >                  expr = expr_simplify(expr);
> > > >                  expr = expr_evaluate_condition(expr,
> > is_chassis_resident_cb,
> > > > -                                               &ports, NULL);
> > > > +                                               &ports);
> > > >              }
> > > >              if (steps > 2) {
> > > >                  expr = expr_normalize(expr);
> > > > @@ -922,7 +922,7 @@ test_tree_shape_exhaustively(struct expr *expr,
> > struct shash *symtab,
> > > >              modified = expr_simplify(expr_clone(expr));
> > > >              modified = expr_evaluate_condition(
> > > >                  modified, tree_shape_is_chassis_resident_cb,
> > > > -                NULL, NULL);
> > > > +                NULL);
> > > >              ovs_assert(expr_honors_invariants(modified));
> > > >
> > > >              if (operation >= OP_NORMALIZE) {
> > > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > > > index 49463c5c2..5d016b757 100644
> > > > --- a/utilities/ovn-trace.c
> > > > +++ b/utilities/ovn-trace.c
> > > > @@ -973,7 +973,7 @@ parse_lflow_for_datapath(const struct
> > sbrec_logical_flow *sblf,
> > > >              match = expr_simplify(match);
> > > >              match = expr_evaluate_condition(match,
> > > >
> >  ovntrace_is_chassis_resident,
> > > > -                                            NULL, NULL);
> > > > +                                            NULL);
> > > >          }
> > > >
> > > >          struct ovntrace_flow *flow = xzalloc(sizeof *flow);
> > > > --
> > > > 2.30.2
> > > >
> > > > _______________________________________________
> > > > dev mailing list
> > > > dev at openvswitch.org
> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > > >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >


More information about the dev mailing list