[ovs-dev] [PATCH ovn 2/2] northd: Provide the option to not use ct.inv in lflows.

Numan Siddique numans at ovn.org
Wed Mar 24 15:58:34 UTC 2021


Hi Ben,

Need your eye on this patch for the ddlog stuff.  I was wondering
if there is a better way to do than what I've done.

Not urgent. Please take your time.

Thanks
Numan

On Mon, Mar 22, 2021 at 4:29 PM <numans at ovn.org> wrote:
>
> From: Numan Siddique <numans at ovn.org>
>
> Presently we add 65535 priority lflows in the stages -
> 'ls_in_acl' and 'ls_out_acl' to drop packets which
> match on 'ct.inv'.
>
> This patch provides an option to not use this field in the
> logical flow matches for the following
> reasons:
>
>  • Some of the smart NICs which support offloading datapath
>    flows may not support this field.  In such cases, almost
>    all the datapath flows cannot be offloaded if ACLs/load balancers
>    are configured on the logical switch datapath.
>
>  • A recent commit in kernel ovs datapath sets the committed
>    connection tracking entry to be liberal for out-of-window
>    tcp packets (nf_ct_set_tcp_be_liberal()).  Such TCP
>    packets will not be marked as invalid.
>
> There are still certain scenarios where a packet can be marked
> invalid.  Like
>  • If the tcp flags are not correct.
>
>  • If there are checksum errors.
>
> In such cases, the packet will be delivered to the destination
> port.  So CMS should evaluate their usecases before enabling
> this option.
>
> Signed-off-by: Numan Siddique <numans at ovn.org>
> ---
>  NEWS                 |  5 +++
>  northd/lswitch.dl    | 13 +++++++-
>  northd/ovn-northd.c  | 41 ++++++++++++++---------
>  northd/ovn_northd.dl | 51 +++++++++++++++++++++++------
>  ovn-nb.xml           | 11 +++++++
>  tests/ovn-northd.at  | 77 ++++++++++++++++++++++++++++++++++++++++++++
>  6 files changed, 171 insertions(+), 27 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index 530c5d42fe..3181649ba8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -7,6 +7,11 @@ Post-v21.03.0
>      (This may take testing and tuning to be effective.)  This version of OVN
>      requires DDLog 0.36.
>    - Introduce ovn-controller incremetal processing engine statistics
> +  - Add a new NB Global option - us_ct_inv_match with the default value of true.
> +    If this is set to false, then the logical field - "ct.inv" will not be
> +    used in the logical flow matches.  CMS can consider setting this to false,
> +    if they want to use smart NICs which don't support offloading datapath flows
> +    with this field used.
>
>  OVN v21.03.0 - 12 Mar 2021
>  -------------------------
> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> index 4bf8a5b907..1daf249382 100644
> --- a/northd/lswitch.dl
> +++ b/northd/lswitch.dl
> @@ -197,6 +197,7 @@ relation &Switch(
>      ipv6_prefix:       Option<in6_addr>,
>      mcast_cfg:         Ref<McastSwitchCfg>,
>      is_vlan_transparent: bool,
> +    use_ct_inv_match:  bool,
>
>      /* Does this switch have at least one port with type != "router"? */
>      has_non_router_port: bool
> @@ -223,7 +224,8 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
>          .ipv6_prefix       = ipv6_prefix,
>          .mcast_cfg         = mcast_cfg,
>          .has_non_router_port = has_non_router_port,
> -        .is_vlan_transparent = is_vlan_transparent) :-
> +        .is_vlan_transparent = is_vlan_transparent,
> +        .use_ct_inv_match = use_ct_inv_match) :-
>      nb::Logical_Switch[ls],
>      LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
>      LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
> @@ -232,6 +234,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
>      LogicalSwitchLocalnetPorts(ls._uuid, localnet_ports),
>      LogicalSwitchHasNonRouterPort(ls._uuid, has_non_router_port),
>      mcast_cfg in &McastSwitchCfg(.datapath = ls._uuid),
> +    UseCtInvMatch(use_ct_inv_match),
>      var subnet =
>          match (ls.other_config.get("subnet")) {
>              None -> None,
> @@ -744,3 +747,11 @@ function put_svc_monitor_mac(options: Map<string,string>,
>  relation SvcMonitorMac(mac: eth_addr)
>  SvcMonitorMac(get_svc_monitor_mac(options, uuid)) :-
>      nb::NB_Global(._uuid = uuid, .options = options).
> +
> +function can_use_ct_inv_match(options: Map<string,string>): bool {
> +    map_get_bool_def(options, "use_ct_inv_match", true)
> +}
> +
> +relation UseCtInvMatch(use_ct_inv_match: bool)
> +UseCtInvMatch(can_use_ct_inv_match(options)) :-
> +    nb::NB_Global(._uuid = uuid, .options = options).
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 3221fb9ff7..6baed2a418 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -4066,6 +4066,10 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct ovn_datapath *od,
>   * logical datapath only by creating a datapath group. */
>  static bool use_logical_dp_groups = false;
>
> +/* If this option is 'true' northd will make use of ct.inv match fields.
> + * Otherwise, it will avoid using it.  The default is true. */
> +static bool use_ct_inv_match = true;
> +
>  /* Adds a row with the specified contents to the Logical_Flow table. */
>  static void
>  ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
> @@ -5681,12 +5685,13 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
>           * for deletion (bit 0 of ct_label is set).
>           *
>           * This is enforced at a higher priority than ACLs can be defined. */
> +        char *match = xasprintf("%s(ct.est && ct.rpl && ct_label.blocked == 1)",
> +                                use_ct_inv_match ? "ct.inv || " : "");
>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
> -                      "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
> -                      "drop;");
> +                      match, "drop;");
>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
> -                      "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
> -                      "drop;");
> +                      match, "drop;");
> +        free(match);
>
>          /* Ingress and Egress ACL Table (Priority 65535).
>           *
> @@ -5697,14 +5702,15 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
>           * direction to hit the currently defined policy from ACLs.
>           *
>           * This is enforced at a higher priority than ACLs can be defined. */
> +        match = xasprintf("ct.est && !ct.rel && !ct.new%s && "
> +                          "ct.rpl && ct_label.blocked == 0",
> +                          use_ct_inv_match ? " && !ct.inv" : "");
> +
>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
> -                      "ct.est && !ct.rel && !ct.new && !ct.inv "
> -                      "&& ct.rpl && ct_label.blocked == 0",
> -                      "next;");
> +                      match, "next;");
>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
> -                      "ct.est && !ct.rel && !ct.new && !ct.inv "
> -                      "&& ct.rpl && ct_label.blocked == 0",
> -                      "next;");
> +                      match, "next;");
> +        free(match);
>
>          /* Ingress and Egress ACL Table (Priority 65535).
>           *
> @@ -5717,14 +5723,14 @@ build_acls(struct ovn_datapath *od, struct hmap *lflows,
>           * a dynamically negotiated FTP data channel), but will allow
>           * related traffic such as an ICMP Port Unreachable through
>           * that's generated from a non-listening UDP port.  */
> +        match = xasprintf("!ct.est && ct.rel && !ct.new%s && "
> +                          "ct_label.blocked == 0",
> +                          use_ct_inv_match ? " && !ct.inv" : "");
>          ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX,
> -                      "!ct.est && ct.rel && !ct.new && !ct.inv "
> -                      "&& ct_label.blocked == 0",
> -                      "next;");
> +                      match, "next;");
>          ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX,
> -                      "!ct.est && ct.rel && !ct.new && !ct.inv "
> -                      "&& ct_label.blocked == 0",
> -                      "next;");
> +                      match, "next;");
> +        free(match);
>
>          /* Ingress and Egress ACL Table (Priority 65535).
>           *
> @@ -12897,6 +12903,9 @@ ovnnb_db_run(struct northd_context *ctx,
>
>      use_logical_dp_groups = smap_get_bool(&nb->options,
>                                            "use_logical_dp_groups", false);
> +    use_ct_inv_match = smap_get_bool(&nb->options,
> +                                     "use_ct_inv_match", true);
> +
>      /* deprecated, use --event instead */
>      controller_event_en = smap_get_bool(&nb->options,
>                                          "controller_event", false);
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index 35e6ab76cb..9e1919cd80 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -2396,16 +2396,25 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
>           * for deletion (bit 0 of ct_label is set).
>           *
>           * This is enforced at a higher priority than ACLs can be defined. */
> +        var __match = match (sw.use_ct_inv_match) {
> +             true -> "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
> +             false -> "(ct.est && ct.rpl && ct_label.blocked == 1)"
> +        } in
>          Flow(.logical_datapath = ls._uuid,
>               .stage            = switch_stage(IN, ACL),
>               .priority         = 65535,
> -             .__match          = "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
> +             .__match          = __match,
>               .actions          = "drop;",
>               .external_ids     = map_empty());
> +
> +        var __match = match (sw.use_ct_inv_match) {
> +             true -> "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
> +             false -> "(ct.est && ct.rpl && ct_label.blocked == 1)"
> +        } in
>          Flow(.logical_datapath = ls._uuid,
>               .stage            = switch_stage(OUT, ACL),
>               .priority         = 65535,
> -             .__match          = "ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)",
> +             .__match          = __match,
>               .actions          = "drop;",
>               .external_ids     = map_empty());
>
> @@ -2418,18 +2427,29 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
>           * direction to hit the currently defined policy from ACLs.
>           *
>           * This is enforced at a higher priority than ACLs can be defined. */
> +        var __match = match (sw.use_ct_inv_match) {
> +             true -> "ct.est && !ct.rel && !ct.new && !ct.inv "
> +                     "&& ct.rpl && ct_label.blocked == 0",
> +             false -> "ct.est && !ct.rel && !ct.new "
> +                      "&& ct.rpl && ct_label.blocked == 0"
> +        } in
>          Flow(.logical_datapath = ls._uuid,
>               .stage            = switch_stage(IN, ACL),
>               .priority         = 65535,
> -             .__match          = "ct.est && !ct.rel && !ct.new && !ct.inv "
> -                                 "&& ct.rpl && ct_label.blocked == 0",
> +             .__match          = __match,
>               .actions          = "next;",
>               .external_ids     = map_empty());
> +
> +        var __match = match (sw.use_ct_inv_match) {
> +             true -> "ct.est && !ct.rel && !ct.new && !ct.inv "
> +                     "&& ct.rpl && ct_label.blocked == 0",
> +             false -> "ct.est && !ct.rel && !ct.new "
> +                      "&& ct.rpl && ct_label.blocked == 0"
> +        } in
>          Flow(.logical_datapath = ls._uuid,
>               .stage            = switch_stage(OUT, ACL),
>               .priority         = 65535,
> -             .__match          = "ct.est && !ct.rel && !ct.new && !ct.inv "
> -                                 "&& ct.rpl && ct_label.blocked == 0",
> +             .__match          = __match,
>               .actions          = "next;",
>               .external_ids     = map_empty());
>
> @@ -2444,18 +2464,29 @@ var has_stateful = sw.has_stateful_acl or sw.has_lb_vip in
>           * a dynamically negotiated FTP data channel), but will allow
>           * related traffic such as an ICMP Port Unreachable through
>           * that's generated from a non-listening UDP port.  */
> +        var __match = match (sw.use_ct_inv_match) {
> +             true -> "!ct.est && ct.rel && !ct.new && !ct.inv "
> +                     "&& ct_label.blocked == 0",
> +             false -> "!ct.est && ct.rel && !ct.new "
> +                      "&& ct_label.blocked == 0",
> +        } in
>          Flow(.logical_datapath = ls._uuid,
>               .stage            = switch_stage(IN, ACL),
>               .priority         = 65535,
> -             .__match          = "!ct.est && ct.rel && !ct.new && !ct.inv "
> -                                 "&& ct_label.blocked == 0",
> +             .__match          = __match,
>               .actions          = "next;",
>               .external_ids     = map_empty());
> +
> +        var __match = match (sw.use_ct_inv_match) {
> +             true -> "!ct.est && ct.rel && !ct.new && !ct.inv "
> +                     "&& ct_label.blocked == 0",
> +             false -> "!ct.est && ct.rel && !ct.new "
> +                      "&& ct_label.blocked == 0",
> +        } in
>          Flow(.logical_datapath = ls._uuid,
>               .stage            = switch_stage(OUT, ACL),
>               .priority         = 65535,
> -             .__match          = "!ct.est && ct.rel && !ct.new && !ct.inv "
> -                                 "&& ct_label.blocked == 0",
> +             .__match          = __match,
>               .actions          = "next;",
>               .external_ids     = map_empty());
>
> diff --git a/ovn-nb.xml b/ovn-nb.xml
> index b0a4adffe5..c3ff3b586a 100644
> --- a/ovn-nb.xml
> +++ b/ovn-nb.xml
> @@ -227,6 +227,17 @@
>          </p>
>        </column>
>
> +      <column name="options" key="use_ct_inv_match">
> +        <p>
> +          If set to false, <code>ovn-northd</code> will not use the
> +          <code>ct.inv</code> field in any of the logical flow matches.
> +          The default value is true.  CMS can consider setting this to
> +          false, in case the NIC doesn't support offloading OVS datapath
> +          flows having matches <code>ct_state(..+inv..)</code> or
> +          <code>ct_state(..-inv..)</code>.
> +        </p>
> +      </column>
> +
>        <group title="Options for configuring interconnection route advertisement">
>          <p>
>            These options control how routes are advertised between OVN
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 32f956a797..3a3a443a4e 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -3066,3 +3066,80 @@ AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
>
>  AT_CLEANUP
>  ])
> +
> +OVN_FOR_EACH_NORTHD([
> +AT_SETUP([ovn -- ct.inv usage])
> +ovn_start
> +
> +check ovn-nbctl ls-add sw0
> +check ovn-nbctl lsp-add sw0 sw0p1
> +
> +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 ip allow-related
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
> +  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> +])
> +
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
> +  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> +])
> +
> +# Disable ct.inv usage.
> +check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=false
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
> +  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
> +  table=9 (ls_in_acl          ), priority=65535, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> +])
> +
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
> +  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
> +  table=4 (ls_out_acl         ), priority=65535, match=((ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> +])
> +
> +AT_CHECK([grep -c "ct.inv" sw0flows], [1], [dnl
> +0
> +])
> +
> +# Enable ct.inv usage.
> +check ovn-nbctl --wait=sb set NB_Global . options:use_ct_inv_match=true
> +
> +ovn-sbctl dump-flows sw0 > sw0flows
> +AT_CAPTURE_FILE([sw0flows])
> +
> +AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
> +  table=9 (ls_in_acl          ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> +  table=9 (ls_in_acl          ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> +])
> +
> +AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
> +  table=4 (ls_out_acl         ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> +  table=4 (ls_out_acl         ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> +])
> +
> +AT_CHECK([grep -c "ct.inv" sw0flows], [0], [dnl
> +6
> +])
> +
> +AT_CLEANUP
> +])
> --
> 2.29.2
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev


More information about the dev mailing list