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

Han Zhou hzhou at ovn.org
Fri Jun 11 19:37:02 UTC 2021


On Fri, Jun 4, 2021 at 6:56 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 5/28/21 9:23 PM, Han Zhou 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>
> > ---
> >  controller/lflow.c          | 66 +++++++++++++++++++++++++++----------
> >  controller/lflow.h          |  3 ++
> >  controller/ovn-controller.c | 17 ++++++++--
> >  tests/ovn.at                | 48 +++++++++++++++++++++++++++
> >  4 files changed, 114 insertions(+), 20 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 0abb6eb96..fa102e8d0 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,
> > @@ -883,7 +888,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)) {
> >                  lflow_cache_add_matches(l_ctx_out->lflow_cache,
> >                                          &lflow->header_.uuid, matches,
> >                                          matches_size);
> > @@ -1746,19 +1753,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,
> > -                                    l_ctx_in, l_ctx_out, &changed);
> > +    bool changed, ret = true;
>
> Same nit as in patch 1, I'd personally split this in two declarations.
>
Ack

> > +
> > +    if (!lflow_handle_changed_ref(REF_TYPE_PORTBINDING,
pb->logical_port,
> > +                                  l_ctx_in, l_ctx_out, &changed)) {
> > +        ret = false;
> > +    }
> > +    return ret;
> > +}
> > +
> > +/* Handles port-binding add/deletions. */
> > +bool
> > +lflow_handle_port_binding_add_del(struct lflow_ctx_in *l_ctx_in,
> > +                                  struct lflow_ctx_out *l_ctx_out)
>
> For consistency with other functions that perform I-P in lflow.c we
> should probably rename this to lflow_handle_changed_port_bindings().

Ack

>
> > +{
> > +    bool changed, ret = true;
>
> Same nit as in patch 1, I'd personally split this in two declarations.

Ack
>
> > +    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;
> >  }
> >
> >  bool
> > diff --git a/controller/lflow.h b/controller/lflow.h
> > index 07263ff47..1ae32b9c5 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;
> > @@ -180,6 +181,8 @@ bool lflow_add_flows_for_datapath(const struct
sbrec_datapath_binding *,
> >  bool lflow_handle_flows_for_lport(const struct sbrec_port_binding *,
> >                                    struct lflow_ctx_in *,
> >                                    struct lflow_ctx_out *);
> > +bool lflow_handle_port_binding_add_del(struct lflow_ctx_in *,
> > +                                       struct lflow_ctx_out *);
> >  bool lflow_handle_mc_group_changes(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 949137c0a..6566f44be 100644
> > --- a/controller/ovn-controller.c
> > +++ b/controller/ovn-controller.c
> > @@ -2000,6 +2000,10 @@ static void 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));
> > @@ -2063,6 +2067,7 @@ static void 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;
> > @@ -2253,6 +2258,13 @@ flow_output_sb_port_binding_handler(struct
engine_node *node,
> >      struct ed_type_flow_output *fo = data;
> >      struct ovn_desired_flow_table *flow_table = &fo->flow_table;
> >
> > +    struct lflow_ctx_in l_ctx_in;
> > +    struct lflow_ctx_out l_ctx_out;
> > +    init_lflow_ctx(node, rt_data, fo, &l_ctx_in, &l_ctx_out);
> > +    if (!lflow_handle_port_binding_add_del(&l_ctx_in, &l_ctx_out)) {
> > +        return false;
> > +    }
> > +
> >      struct physical_ctx p_ctx;
> >      init_physical_ctx(node, rt_data, &p_ctx);
> >
> > @@ -2356,8 +2368,9 @@ _flow_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. */
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index a21dbadac..5efd32d82 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -26749,3 +26749,51 @@ 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
> > +ovs-vsctl add-br br-phys
> > +ovn_attach n1 br-phys 192.168.0.1
> > +
> > +# Bind both lp1 and lp2 on the chasis.
> > +ovs-vsctl add-port br-int lp1 -- set interface lp1
external_ids:iface-id=lp1
> > +ovs-vsctl add-port br-int lp2 -- set interface lp2
external_ids:iface-id=lp2
>
> It's, probably, good to prepend "check " to all ovs-*ctl/ovn-*ctl calls.
> They might catch unexpected issues in the future.

Ack.

Thanks,
Han

>
> > +
> > +# Create only lport lp1, but not lp2.
> > +ovn-nbctl ls-add lsw0
> > +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.
> > +ovn-nbctl acl-add lsw0 to-lport 1002 'outport == "lp1" && ip4.src ==
10.0.0.111'  allow-related
> > +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.
> > +ovn-nbctl ls-add lsw0
> > +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
> > +])
> >
>
> Thanks,
> Dumitru
>


More information about the dev mailing list