[ovs-dev,v4,1/3] actions: add sample action

sample ovn action encodes into the OFPACT_SAMPLE ovs action.

OVN action allows the following parameters:

- obs_domain_id: 8-bit integer that identifies the sampling application.
  This value will be combined with the datapath's tunnel_id to form the
  final observation_domain_id that will be used in the OVS action.

- obs_point_id: a 32-bit integer or the $cookie macro that will be
  expanded into the first 32 bits of the lflow's UUID.

- probability: a 16-bit integer that specifies the sampling probability.
  Specifying 0 has no effect and 65535 means sampling all packets.

- collector_set: the 32-bit id that has to be configured in OVS's
  Flow_Sample_Collector_Set table in order to configure IPFIX sampling.

Signed-off-by: Adrian Moreno <amorenoz@redhat.com>
---
 controller/lflow.c    |   1 +
 include/ovn/actions.h |  16 ++++++
 lib/actions.c         | 120 ++++++++++++++++++++++++++++++++++++++++++
 tests/ovn.at          |  25 +++++++++
 tests/test-ovn.c      |   3 ++
 utilities/ovn-trace.c |   2 +
 6 files changed, 167 insertions(+)

On Mon, Oct 17, 2022 at 9:14 AM Adrian Moreno <amorenoz@redhat.com> wrote:
>
> sample ovn action encodes into the OFPACT_SAMPLE ovs action.
>
> OVN action allows the following parameters:
>
> - obs_domain_id: 8-bit integer that identifies the sampling application.
>   This value will be combined with the datapath's tunnel_id to form the
>   final observation_domain_id that will be used in the OVS action.
>
> - obs_point_id: a 32-bit integer or the $cookie macro that will be
>   expanded into the first 32 bits of the lflow's UUID.
>
> - probability: a 16-bit integer that specifies the sampling probability.
>   Specifying 0 has no effect and 65535 means sampling all packets.
>
> - collector_set: the 32-bit id that has to be configured in OVS's
>   Flow_Sample_Collector_Set table in order to configure IPFIX sampling.
>
> Signed-off-by: Adrian Moreno <amorenoz@redhat.com>

Thanks Adrian for working on this patch series and for the patience.

The patch overall looks good to me.

Can you please add documentation for the new action "sample"  in
ovn-sb.xml  here
https://github.com/ovn-org/ovn/blob/main/ovn-sb.xml#L1133 ?


Thanks
Numan

> ---
>  controller/lflow.c    |   1 +
>  include/ovn/actions.h |  16 ++++++
>  lib/actions.c         | 120 ++++++++++++++++++++++++++++++++++++++++++
>  tests/ovn.at          |  25 +++++++++
>  tests/test-ovn.c      |   3 ++
>  utilities/ovn-trace.c |   2 +
>  6 files changed, 167 insertions(+)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index cc0f31db0..ad316c17f 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -1007,6 +1007,7 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
>          .group_table = l_ctx_out->group_table,
>          .meter_table = l_ctx_out->meter_table,
>          .lflow_uuid = lflow->header_.uuid,
> +        .dp_key = ldp->datapath->tunnel_key,
>
>          .pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
>          .ingress_ptable = OFTABLE_LOG_INGRESS_PIPELINE,
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index d7ee84dac..009487cfc 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -121,6 +121,7 @@ struct ovn_extend_table;
>      OVNACT(COMMIT_ECMP_NH,    ovnact_commit_ecmp_nh)  \
>      OVNACT(CHK_ECMP_NH_MAC,   ovnact_result)          \
>      OVNACT(CHK_ECMP_NH,       ovnact_result)          \
> +    OVNACT(SAMPLE,            ovnact_sample)          \
>
>  /* enum ovnact_type, with a member OVNACT_<ENUM> for each action. */
>  enum OVS_PACKED_ENUM ovnact_type {
> @@ -456,6 +457,18 @@ struct ovnact_lookup_fdb {
>      struct expr_field dst;     /* 1-bit destination field. */
>  };
>
> +/* OVNACT_SAMPLE */
> +struct ovnact_sample {
> +    struct ovnact ovnact;
> +    uint16_t probability;       /* probability over UINT16_MAX. */
> +    uint8_t obs_domain_id;      /* most significant byte of the
> +                                   observation domain id. The other 24 bits
> +                                   will come from the datapath's tunnel key. */
> +    uint32_t collector_set_id;  /* colector_set_id. */
> +    uint32_t obs_point_id;      /* observation point id. */
> +    bool use_cookie;            /* use cookie as obs_point_id */
> +};
> +
>  /* OVNACT_COMMIT_ECMP_NH. */
>  struct ovnact_commit_ecmp_nh {
>      struct ovnact ovnact;
> @@ -785,6 +798,9 @@ struct ovnact_encode_params {
>      /* The logical flow uuid that drove this action. */
>      struct uuid lflow_uuid;
>
> +    /* The datapath key. */
> +    uint32_t dp_key;
> +
>      /* OVN maps each logical flow table (ltable), one-to-one, onto a physical
>       * OpenFlow flow table (ptable).  A number of parameters describe this
>       * mapping and data related to flow tables:
> diff --git a/lib/actions.c b/lib/actions.c
> index adbb42db4..cef626f84 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -4279,6 +4279,124 @@ encode_CHECK_OUT_PORT_SEC(const struct ovnact_result *dl,
>                             MLF_CHECK_PORT_SEC_BIT, ofpacts);
>  }
>
> +static void
> +format_SAMPLE(const struct ovnact_sample *sample, struct ds *s)
> +{
> +    ds_put_format(s, "sample(probability=%"PRId16, sample->probability);
> +
> +    ds_put_format(s, ",collector_set=%"PRId32, sample->collector_set_id);
> +    ds_put_format(s, ",obs_domain=%"PRId8, sample->obs_domain_id);
> +    if (sample->use_cookie) {
> +        ds_put_cstr(s, ",obs_point=$cookie");
> +    } else {
> +        ds_put_format(s, ",obs_point=%"PRId32, sample->obs_point_id);
> +    }
> +    ds_put_format(s, ");");
> +}
> +
> +static void
> +encode_SAMPLE(const struct ovnact_sample *sample,
> +              const struct ovnact_encode_params *ep,
> +              struct ofpbuf *ofpacts)
> +{
> +    struct ofpact_sample *os = ofpact_put_SAMPLE(ofpacts);
> +    os->probability = sample->probability;
> +    os->collector_set_id = sample->collector_set_id;
> +    os->obs_domain_id =
> +        (sample->obs_domain_id << 24) | (ep->dp_key & 0xFFFFFF);
> +
> +    if (sample->use_cookie) {
> +        os->obs_point_id = ep->lflow_uuid.parts[0];
> +    } else {
> +        os->obs_point_id = sample->obs_point_id;
> +    }
> +    os->sampling_port = OFPP_NONE;
> +}
> +
> +static void
> +parse_sample_arg(struct action_context *ctx, struct ovnact_sample *sample)
> +{
> +    if (lexer_match_id(ctx->lexer, "probability")) {
> +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +            return;
> +        }
> +        if (ctx->lexer->token.type == LEX_T_INTEGER
> +            && ctx->lexer->token.format == LEX_F_DECIMAL) {
> +            if (!action_parse_uint16(ctx, &sample->probability,
> +                                     "probability")) {
> +                return;
> +            }
> +        }
> +    } else if (lexer_match_id(ctx->lexer, "obs_point")) {
> +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +            return;
> +        }
> +        if (ctx->lexer->token.type == LEX_T_MACRO &&
> +            !strcmp(ctx->lexer->token.s, "cookie")) {
> +            sample->use_cookie = true;
> +            lexer_get(ctx->lexer);
> +        } else if (ctx->lexer->token.type == LEX_T_INTEGER
> +                && ctx->lexer->token.format == LEX_F_DECIMAL) {
> +            sample->obs_point_id = ntohll(ctx->lexer->token.value.integer);
> +            lexer_get(ctx->lexer);
> +        } else {
> +            lexer_syntax_error(ctx->lexer,
> +                               "malformed sample observation_point_id");
> +        }
> +    } else if (lexer_match_id(ctx->lexer, "obs_domain")) {
> +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +            return;
> +        }
> +        if (ctx->lexer->token.type == LEX_T_INTEGER
> +            && ctx->lexer->token.format == LEX_F_DECIMAL) {
> +            uint32_t obs_domain = ntohll(ctx->lexer->token.value.integer);
> +            if (obs_domain > UINT8_MAX) {
> +                lexer_syntax_error(ctx->lexer,
> +                     "obs_domain must be 8-bit long");
> +                return;
> +            }
> +            sample->obs_domain_id = obs_domain;
> +        }
> +        lexer_get(ctx->lexer);
> +    } else if (lexer_match_id(ctx->lexer, "collector_set")) {
> +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +            return;
> +        }
> +        if (ctx->lexer->token.type == LEX_T_INTEGER
> +            && ctx->lexer->token.format == LEX_F_DECIMAL) {
> +            sample->collector_set_id = ntohll(ctx->lexer->token.value.integer);
> +        }
> +        lexer_get(ctx->lexer);
> +    } else {
> +        lexer_syntax_error(ctx->lexer, "unknown argument");
> +    }
> +}
> +
> +static void
> +parse_sample(struct action_context *ctx)
> +{
> +    struct ovnact_sample * sample = ovnact_put_SAMPLE(ctx->ovnacts);
> +
> +    if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> +        while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> +            parse_sample_arg(ctx, sample);
> +            if (ctx->lexer->error) {
> +                return;
> +            }
> +            lexer_match(ctx->lexer, LEX_T_COMMA);
> +        }
> +    }
> +    if (!sample->probability) {
> +        lexer_error(ctx->lexer, "probability must be greater than zero");
> +        return;
> +    }
> +}
> +
> +static void
> +ovnact_sample_free(struct ovnact_sample *sample OVS_UNUSED)
> +{
> +}
> +
>  static void
>  parse_commit_ecmp_nh(struct action_context *ctx,
>                       struct ovnact_commit_ecmp_nh *ecmp_nh)
> @@ -4790,6 +4908,8 @@ parse_action(struct action_context *ctx)
>          parse_put_fdb(ctx, ovnact_put_PUT_FDB(ctx->ovnacts));
>      } else if (lexer_match_id(ctx->lexer, "commit_ecmp_nh")) {
>          parse_commit_ecmp_nh(ctx, ovnact_put_COMMIT_ECMP_NH(ctx->ovnacts));
> +    } else if (lexer_match_id(ctx->lexer, "sample")) {
> +        parse_sample(ctx);
>      } else {
>          lexer_syntax_error(ctx->lexer, "expecting action");
>      }
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f8b8db4df..95176f131 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -2136,6 +2136,31 @@ pop(eth.type);
>  push(abc);
>      Syntax error at `abc' expecting field name.
>
> +# sample
> +sample(probability=100,collector_set=200,obs_domain=0,obs_point=1000);
> +    encodes as sample(probability=100,collector_set_id=200,obs_domain_id=11259375,obs_point_id=1000)
> +
> +# sample with obs_domain = 10. Final obs_domain is 0xA << 24 | 0xABCDEF.
> +sample(probability=100,collector_set=200,obs_domain=10,obs_point=$cookie);
> +    encodes as sample(probability=100,collector_set_id=200,obs_domain_id=179031535,obs_point_id=2863311530)
> +
> +sample(probability=10);
> +    formats as sample(probability=10,collector_set=0,obs_domain=0,obs_point=0);
> +    encodes as sample(probability=10,collector_set_id=0,obs_domain_id=11259375,obs_point_id=0)
> +
> +sample(probability=0,collector_set=200,obs_domain=0,obs_point=1000);
> +    probability must be greater than zero
> +
> +sample(probability=0,collector_set=200,obs_domain=0,obs_point=foo);
> +    Syntax error at `foo' malformed sample observation_point_id.
> +
> +sample(probability=0,collector_set=200,obs_domain=300,obs_point=foo);
> +    Syntax error at `300' obs_domain must be 8-bit long.
> +
> +sample(probability=10,foo=bar,obs_domain=0,obs_point=1000);
> +    Syntax error at `foo' unknown argument.
> +
> +
>  # Miscellaneous negative tests.
>  ;
>      Syntax error at `;'.
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index a241f150d..fd580b5df 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1355,6 +1355,9 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
>                  .common_nat_ct_zone = MFF_LOG_DNAT_ZONE,
>                  .in_port_sec_ptable = OFTABLE_CHK_IN_PORT_SEC,
>                  .out_port_sec_ptable = OFTABLE_CHK_OUT_PORT_SEC,
> +                .lflow_uuid.parts =
> +                    { 0xaaaaaaaa, 0xbbbbbbbb, 0xcccccccc, 0xdddddddd},
> +                .dp_key = 0xabcdef,
>              };
>              struct ofpbuf ofpacts;
>              ofpbuf_init(&ofpacts, 0);
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index d9e7129d9..fbe548605 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -3298,6 +3298,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>              break;
>          case OVNACT_CHK_ECMP_NH:
>              break;
> +        case OVNACT_SAMPLE:
> +            break;
>          }
>      }
>      ofpbuf_uninit(&stack);
> --
> 2.37.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>