On Thu, Jul 11, 2024 at 12:32:06AM GMT, Ilya Maximets wrote:
> On 7/10/24 23:38, Adrián Moreno wrote:
> > On Wed, Jul 10, 2024 at 11:00:43PM GMT, Ilya Maximets wrote:
> >> On 7/7/24 22:09, Adrian Moreno wrote:
> >>> When sample action gets used as a way of sampling traffic with
> >>> controller-generated metadata (i.e: obs_domain_id and obs_point_id),
> >>> the controller will have to increase the number of flows to ensure each
> >>> part of the pipeline contains the right metadata.
> >>>
> >>> As an example, if the controller decides to sample stateful traffic, it
> >>> could store the computed metadata for each connection in the conntrack
> >>> label. However, for established connections, a flow must be created for
> >>> each different ct_label value with a sample action that contains a
> >>> different hardcoded obs_domain and obs_point id.
> >>>
> >>> This patch adds a new version of the NXAST_RAW_SAMPLE* action (number 4)
> >>> that supports specifying the observation point and domain using an
> >>> OpenFlow field reference, so now the controller can express:
> >>>
> >>> sample(...
> >>> obs_domain_id=NXM_NX_CT_LABEL[0..31],
> >>> obs_point_id=NXM_NX_CT_LABEL[32..63]
> >>> ...
> >>> )
> >>>
> >>> Signed-off-by: Adrian Moreno <amore...@redhat.com>
> >>> ---
> >>> include/openvswitch/ofp-actions.h | 8 +-
> >>> lib/ofp-actions.c | 249 +++++++++++++++++++++++++++---
> >>> ofproto/ofproto-dpif-xlate.c | 55 ++++++-
> >>> python/ovs/flow/ofp.py | 8 +-
> >>> python/ovs/flow/ofp_act.py | 4 +-
> >>> tests/ofp-actions.at | 5 +
> >>> tests/ofproto-dpif.at | 41 +++++
> >>> tests/system-traffic.at | 74 +++++++++
> >>> 8 files changed, 405 insertions(+), 39 deletions(-)
> >>
> >> Not a full review, it's a complicated change. See a few comments below.
> >>
> >>>
> >>> diff --git a/include/openvswitch/ofp-actions.h
> >>> b/include/openvswitch/ofp-actions.h
> >>> index 7b57e49ad..56dc2c147 100644
> >>> --- a/include/openvswitch/ofp-actions.h
> >>> +++ b/include/openvswitch/ofp-actions.h
> >>> @@ -1015,14 +1015,16 @@ enum nx_action_sample_direction {
> >>>
> >>> /* OFPACT_SAMPLE.
> >>> *
> >>> - * Used for NXAST_SAMPLE, NXAST_SAMPLE2, and NXAST_SAMPLE3. */
> >>> + * Used for NXAST_SAMPLE, NXAST_SAMPLE2, NXAST_SAMPLE3 and
> >>> NXAST_SAMPLE4. */
> >>> struct ofpact_sample {
> >>> OFPACT_PADDED_MEMBERS(
> >>> struct ofpact ofpact;
> >>> uint16_t probability; /* Always positive. */
> >>> uint32_t collector_set_id;
> >>> - uint32_t obs_domain_id;
> >>> - uint32_t obs_point_id;
> >>> + uint32_t obs_domain_imm;
> >>> + struct mf_subfield obs_domain_src;
> >>> + uint32_t obs_point_imm;
> >>> + struct mf_subfield obs_point_src;
> >>> ofp_port_t sampling_port;
> >>> enum nx_action_sample_direction direction;
> >>> );
> >>> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> >>> index da7b1dd31..e329a7e3f 100644
> >>> --- a/lib/ofp-actions.c
> >>> +++ b/lib/ofp-actions.c
> >>> @@ -330,6 +330,8 @@ enum ofp_raw_action_type {
> >>> NXAST_RAW_SAMPLE2,
> >>> /* NX1.0+(41): struct nx_action_sample2. */
> >>> NXAST_RAW_SAMPLE3,
> >>> + /* NX1.0+(51): struct nx_action_sample4, ... VLMFF */
> >>
> >> Why the dots are here? The structure doesn't seem to have
> >> extra fields at the end.
> >
> > I missread the description then. I thought it was just about alignment.
> >
> >>
> >>> + NXAST_RAW_SAMPLE4,
> >>>
> >>> /* NX1.0+(34): struct nx_action_conjunction. */
> >>> NXAST_RAW_CONJUNCTION,
> >>> @@ -6188,6 +6190,34 @@ struct nx_action_sample2 {
> >>> };
> >>> OFP_ASSERT(sizeof(struct nx_action_sample2) == 32);
> >>>
> >>> +/* Action structure for NXAST_SAMPLE4
> >>> + *
> >>> + * NXAST_SAMPLE4 was added in Open vSwitch 3.4.0. Compared to
> >>> NXAST_SAMPLE3,
> >>> + * it adds support for using field specifiers for observation_domain_id
> >>> and
> >>> + * observation_point_id. */
> >>> +struct nx_action_sample4 {
> >>> + ovs_be16 type; /* OFPAT_VENDOR. */
> >>> + ovs_be16 len; /* Length is 32. */
> >>
> >> Is the length 32? 40, I suppose.
> >
> > Yep
> >
> >>
> >>> + ovs_be32 vendor; /* NX_VENDOR_ID. */
> >>> + ovs_be16 subtype; /* NXAST_SAMPLE. */
> >>
> >> NXAST_SAMPLE4 ?
> >
> > Ack.
> >
> >>
> >>> + ovs_be16 probability; /* Fraction of packets to sample. */
> >>> + ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */
> >>> + ovs_be32 obs_domain_src; /* Source of the
> >>> observation_domain_id. */
> >>> + union {
> >>> + ovs_be16 obs_domain_ofs_nbits; /* Range to use from source
> >>> field. */
> >>> + ovs_be32 obs_domain_imm; /* Immediate value for domain
> >>> id. */
> >>> + };
> >>> + ovs_be32 obs_point_src; /* Source of the
> >>> observation_point_id */
> >>> + union {
> >>> + ovs_be16 obs_point_ofs_nbits; /* Range to use from source
> >>> field. */
> >>> + ovs_be32 obs_point_imm; /* Immediate value for point id.
> >>> */
> >>> + };
> >>> + ovs_be16 sampling_port; /* Sampling port. */
> >>> + uint8_t direction; /* Sampling direction. */
> >>> + uint8_t zeros[5]; /* Pad to a multiple of 8 bytes */
> >>
> >> Double spaces are a little strage.
> >>
> >>> + };
> >>> + OFP_ASSERT(sizeof(struct nx_action_sample4) == 40);
> >>> +
> >>> static enum ofperr
> >>> decode_NXAST_RAW_SAMPLE(const struct nx_action_sample *nas,
> >>> enum ofp_version ofp_version OVS_UNUSED,
> >>> @@ -6199,11 +6229,14 @@ decode_NXAST_RAW_SAMPLE(const struct
> >>> nx_action_sample *nas,
> >>> sample->ofpact.raw = NXAST_RAW_SAMPLE;
> >>> sample->probability = ntohs(nas->probability);
> >>> sample->collector_set_id = ntohl(nas->collector_set_id);
> >>> - sample->obs_domain_id = ntohl(nas->obs_domain_id);
> >>> - sample->obs_point_id = ntohl(nas->obs_point_id);
> >>> + sample->obs_domain_imm = ntohl(nas->obs_domain_id);
> >>> + sample->obs_domain_src.field = NULL;
> >>> + sample->obs_point_imm = ntohl(nas->obs_point_id);
> >>> + sample->obs_point_src.field = NULL;
> >>> sample->sampling_port = OFPP_NONE;
> >>> sample->direction = NX_ACTION_SAMPLE_DEFAULT;
> >>> -
> >>> + sample->obs_domain_src.field = NULL;
> >>> + sample->obs_point_src.field = NULL;
> >>> if (sample->probability == 0) {
> >>> return OFPERR_OFPBAC_BAD_ARGUMENT;
> >>> }
> >>> @@ -6220,8 +6253,10 @@ decode_SAMPLE2(const struct nx_action_sample2 *nas,
> >>> sample->ofpact.raw = raw;
> >>> sample->probability = ntohs(nas->probability);
> >>> sample->collector_set_id = ntohl(nas->collector_set_id);
> >>> - sample->obs_domain_id = ntohl(nas->obs_domain_id);
> >>> - sample->obs_point_id = ntohl(nas->obs_point_id);
> >>> + sample->obs_domain_imm = ntohl(nas->obs_domain_id);
> >>> + sample->obs_domain_src.field = NULL;
> >>> + sample->obs_point_imm = ntohl(nas->obs_point_id);
> >>> + sample->obs_point_src.field = NULL;
> >>> sample->sampling_port = u16_to_ofp(ntohs(nas->sampling_port));
> >>> sample->direction = direction;
> >>>
> >>> @@ -6241,41 +6276,174 @@ decode_NXAST_RAW_SAMPLE2(const struct
> >>> nx_action_sample2 *nas,
> >>> ofpact_put_SAMPLE(out));
> >>> }
> >>>
> >>> +static int
> >>> +check_sample_direction(enum nx_action_sample_direction direction)
> >>> +{
> >>> + if (direction != NX_ACTION_SAMPLE_DEFAULT &&
> >>> + direction != NX_ACTION_SAMPLE_INGRESS &&
> >>> + direction != NX_ACTION_SAMPLE_EGRESS) {
> >>> + VLOG_WARN_RL(&rl, "invalid sample direction %"PRIu8, direction);
> >>> + return OFPERR_OFPBAC_BAD_ARGUMENT;
> >>> + }
> >>> + return 0;
> >>> +}
> >>> static enum ofperr
> >>> decode_NXAST_RAW_SAMPLE3(const struct nx_action_sample2 *nas,
> >>> enum ofp_version ofp_version OVS_UNUSED,
> >>> struct ofpbuf *out)
> >>> {
> >>> struct ofpact_sample *sample = ofpact_put_SAMPLE(out);
> >>> + int err;
> >>> +
> >>> if (!is_all_zeros(nas->zeros, sizeof nas->zeros)) {
> >>> return OFPERR_NXBRC_MUST_BE_ZERO;
> >>> }
> >>> - if (nas->direction != NX_ACTION_SAMPLE_DEFAULT &&
> >>> - nas->direction != NX_ACTION_SAMPLE_INGRESS &&
> >>> - nas->direction != NX_ACTION_SAMPLE_EGRESS) {
> >>> - VLOG_WARN_RL(&rl, "invalid sample direction %"PRIu8,
> >>> nas->direction);
> >>> - return OFPERR_OFPBAC_BAD_ARGUMENT;
> >>> + err = check_sample_direction(nas->direction);
> >>> + if (err) {
> >>> + return err;
> >>> }
> >>> return decode_SAMPLE2(nas, NXAST_RAW_SAMPLE3, nas->direction,
> >>> sample);
> >>> }
> >>>
> >>> +static int
> >>> +decode_sample_obs_id(ovs_be32 src, ovs_be16 ofs_nbits, ovs_be32 imm,
> >>> + const struct vl_mff_map *vl_mff_map, uint64_t
> >>> *tlv_bitmap,
> >>> + struct mf_subfield *src_out, uint32_t *imm_out)
> >>> +{
> >>> + if (src) {
> >>> + enum ofperr error;
> >>> +
> >>> + src_out->ofs = nxm_decode_ofs(ofs_nbits);
> >>> + src_out->n_bits = nxm_decode_n_bits(ofs_nbits);
> >>> + error = mf_vl_mff_mf_from_nxm_header(ntohl(src),
> >>> + vl_mff_map, &src_out->field,
> >>> + tlv_bitmap);
> >>> + if (error) {
> >>> + return error;
> >>> + }
> >>> +
> >>> + error = mf_check_src(src_out, NULL);
> >>> + if (error) {
> >>> + return error;
> >>> + }
> >>> +
> >>> + if (src_out->n_bits != 32) {
> >>> + VLOG_WARN_RL(&rl, "field associated to sample observation id
> >>> is "
> >>> + "not 32-bit but %d", src_out->n_bits);
> >>
> >> Why it has to be exactly 32 bits? Seems like an unnecessary limitation.
> >> It should be possible to use any number of bits up to 32.
> >>
> >
> > I wanted to be consistent with other "move" operations.
> > If you try something like "move:reg0[0..1]->eth_src", the action fails
> > with:
> >
> > "ovs-ofctl: reg0[0..7]->eth_src: source field is 8 bits wide but
> > destination is 48 bits wide"
>
> Such restrictions are typically applied when the destination
> is part of the packet or metadata. I'd treat this case more
> like the output:reg case.
>
The difference is that output has clear integer semantics where as my
impression (polluted by the way OVN will use these fields) is that this
is more a metadata field where bits can be split in arbitrary ways (see
8/24 split of obs_domain_id in OVN).
Besides, I don't see a strong limitation since you can use registers to
build your 32bit value, e.g:
move:in_port->reg0[0..15],sample(...obs_domain_id=NXM_NX_REG0).
Having said that, I don't see a big practical problem in allowing
smaller subfields, other than feeling a bit weird. So if you feel strong
about this I'll change it.
> And I'm actually not sure why ct zone requires 16 bits...
>
>
> >
> >
> >>> + return OFPERR_OFPBAC_BAD_SET_LEN;
> >>
> >> This is not an appropriate error code. BAD_SET_* codes are specific
> >> to SET_FIELD action. ct zone code is wrong to use it.
> >> It should be OFPBAC_BAD_ARGUMENT instead.
> >>
> >
> > I did consider it but I chose a more concrete error code to try to
> > increase expressiveness. The comment in above the error code did not
> > give the impression of being specific to SET action:
> > "
> > /* NX1.0-1.1(1,524), OF1.2+(2,14). Action references past the end of an
> > * OXM or NXM field, or uses a length of zero. */
> > OFPERR_OFPBAC_BAD_SET_LEN,
> > "
>
> The spec defines it as:
>
> OFPBAC_BAD_SET_LEN = 14, /* Length problem in SET_FIELD action. */
>
> The description in our header maybe slightly incomplete. Either way
> the action doesn't reference outside of the field in this case.
> mf_check_src() will actually return BAD_SET_LEN if that was true.
I wasn't aware of that. Thanks
>
> >
> >>> + }
> >>> + } else {
> >>> + src_out->field = NULL;
> >>> + *imm_out = ntohl(imm);
> >>> + }
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >>> +static enum ofperr
> >>> +decode_NXAST_RAW_SAMPLE4(const struct nx_action_sample4 *nas,
> >>> + enum ofp_version ofp_version OVS_UNUSED,
> >>> + const struct vl_mff_map *vl_mff_map,
> >>> + uint64_t *tlv_bitmap,
> >>> + struct ofpbuf *out)
> >>> +{
> >>> + struct ofpact_sample *sample = ofpact_put_SAMPLE(out);
> >>> + int err;
> >>> +
> >>> + if (!is_all_zeros(nas->zeros, sizeof nas->zeros)) {
> >>> + return OFPERR_NXBRC_MUST_BE_ZERO;
> >>> + }
> >>> +
> >>> + err = check_sample_direction(nas->direction);
> >>> + if (err) {
> >>> + return err;
> >>> + }
> >>> +
> >>> + sample->ofpact.raw = NXAST_RAW_SAMPLE4;
> >>> + sample->probability = ntohs(nas->probability);
> >>> + sample->collector_set_id = ntohl(nas->collector_set_id);
> >>> + sample->sampling_port = u16_to_ofp(ntohs(nas->sampling_port));
> >>> + sample->direction = nas->direction;
> >>> +
> >>> + if (sample->probability == 0) {
> >>> + return OFPERR_OFPBAC_BAD_ARGUMENT;
> >>> + }
> >>> +
> >>> + err = decode_sample_obs_id(nas->obs_domain_src,
> >>> + nas->obs_domain_ofs_nbits,
> >>> + nas->obs_domain_imm,
> >>> + vl_mff_map, tlv_bitmap,
> >>> + &sample->obs_domain_src,
> >>> + &sample->obs_domain_imm);
> >>> + if (err) {
> >>> + return err;
> >>> + }
> >>> +
> >>> + err = decode_sample_obs_id(nas->obs_point_src,
> >>> + nas->obs_point_ofs_nbits,
> >>> + nas->obs_point_imm,
> >>> + vl_mff_map, tlv_bitmap,
> >>> + &sample->obs_point_src,
> >>> + &sample->obs_point_imm);
> >>> + if (err) {
> >>> + return err;
> >>> + }
> >>> +
> >>> + return 0;
> >>
> >> Can just return err here and avoid the if.
> >>
> >
> > Ack.
> >
> >>> +}
> >>> +
> >>> static void
> >>> encode_SAMPLE2(const struct ofpact_sample *sample,
> >>> struct nx_action_sample2 *nas)
> >>> {
> >>> nas->probability = htons(sample->probability);
> >>> nas->collector_set_id = htonl(sample->collector_set_id);
> >>> - nas->obs_domain_id = htonl(sample->obs_domain_id);
> >>> - nas->obs_point_id = htonl(sample->obs_point_id);
> >>> + nas->obs_domain_id = htonl(sample->obs_domain_imm);
> >>> + nas->obs_point_id = htonl(sample->obs_point_imm);
> >>> + nas->sampling_port = htons(ofp_to_u16(sample->sampling_port));
> >>> + nas->direction = sample->direction;
> >>> +}
> >>> +
> >>> +static void
> >>> +encode_SAMPLE4(const struct ofpact_sample *sample,
> >>> + struct nx_action_sample4 *nas)
> >>> +{
> >>> + nas->probability = htons(sample->probability);
> >>> + nas->collector_set_id = htonl(sample->collector_set_id);
> >>> nas->sampling_port = htons(ofp_to_u16(sample->sampling_port));
> >>> nas->direction = sample->direction;
> >>> +
> >>> + if (sample->obs_domain_src.field) {
> >>> + nas->obs_domain_src =
> >>> + htonl(nxm_header_from_mff(sample->obs_domain_src.field));
> >>> + nas->obs_domain_ofs_nbits =
> >>> + nxm_encode_ofs_nbits(sample->obs_domain_src.ofs,
> >>> + sample->obs_domain_src.n_bits);
> >>> + } else {
> >>> + nas->obs_domain_src = htonl(0);
> >>> + nas->obs_domain_imm = htonl(sample->obs_domain_imm);
> >>> + }
> >>> + if (sample->obs_point_src.field) {
> >>> + nas->obs_point_src =
> >>> + htonl(nxm_header_from_mff(sample->obs_point_src.field));
> >>> + nas->obs_point_ofs_nbits =
> >>> + nxm_encode_ofs_nbits(sample->obs_point_src.ofs,
> >>> + sample->obs_point_src.n_bits);
> >>> + } else {
> >>> + nas->obs_point_src = htonl(0);
> >>> + nas->obs_point_imm = htonl(sample->obs_point_imm);
> >>> + }
> >>> }
> >>>
> >>> static void
> >>> encode_SAMPLE(const struct ofpact_sample *sample,
> >>> enum ofp_version ofp_version OVS_UNUSED, struct ofpbuf
> >>> *out)
> >>> {
> >>> - if (sample->ofpact.raw == NXAST_RAW_SAMPLE3
> >>> + if (sample->ofpact.raw == NXAST_RAW_SAMPLE4 ||
> >>> + sample->obs_domain_src.field ||
> >>> + sample->obs_point_src.field) {
> >>> + encode_SAMPLE4(sample, put_NXAST_SAMPLE4(out));
> >>> + } else if (sample->ofpact.raw == NXAST_RAW_SAMPLE3
> >>> || sample->direction != NX_ACTION_SAMPLE_DEFAULT) {
> >>> encode_SAMPLE2(sample, put_NXAST_SAMPLE3(out));
> >>> } else if (sample->ofpact.raw == NXAST_RAW_SAMPLE2
> >>> @@ -6285,8 +6453,8 @@ encode_SAMPLE(const struct ofpact_sample *sample,
> >>> struct nx_action_sample *nas = put_NXAST_SAMPLE(out);
> >>> nas->probability = htons(sample->probability);
> >>> nas->collector_set_id = htonl(sample->collector_set_id);
> >>> - nas->obs_domain_id = htonl(sample->obs_domain_id);
> >>> - nas->obs_point_id = htonl(sample->obs_point_id);
> >>> + nas->obs_domain_id = htonl(sample->obs_domain_imm);
> >>> + nas->obs_point_id = htonl(sample->obs_point_imm);
> >>> }
> >>> }
> >>>
> >>> @@ -6314,9 +6482,35 @@ parse_SAMPLE(char *arg, const struct
> >>> ofpact_parse_params *pp)
> >>> } else if (!strcmp(key, "collector_set_id")) {
> >>> error = str_to_u32(value, &os->collector_set_id);
> >>> } else if (!strcmp(key, "obs_domain_id")) {
> >>> - error = str_to_u32(value, &os->obs_domain_id);
> >>> + error = str_to_u32(value, &os->obs_domain_imm);
> >>> +
> >>> + if (error) {
> >>> + free(error);
> >>> + error = mf_parse_subfield(&os->obs_domain_src, value);
> >>> + if (error) {
> >>> + return error;
> >>> + }
> >>> + if (os->obs_domain_src.n_bits != 32) {
> >>
> >> Same here.
> >>
> >>> + error = xasprintf("invalid length of obs_domain_id
> >>> field "
> >>> + "(%d). Must be 32",
> >>> + os->obs_domain_src.n_bits);
> >>> + }
> >>> + }
> >>> } else if (!strcmp(key, "obs_point_id")) {
> >>> - error = str_to_u32(value, &os->obs_point_id);
> >>> + error = str_to_u32(value, &os->obs_point_imm);
> >>> +
> >>> + if (error) {
> >>> + free(error);
> >>> + error = mf_parse_subfield(&os->obs_point_src, value);
> >>> + if (error) {
> >>> + return error;
> >>> + }
> >>> + if (os->obs_point_src.n_bits != 32) {
> >>
> >> And here.
> >>
> >>> + error = xasprintf("invalid length of obs_point_id
> >>> field "
> >>> + "(%d). Must be 32",
> >>> + os->obs_point_src.n_bits);
> >>> + }
> >>> + }
> >>> } else if (!strcmp(key, "sampling_port")) {
> >>> if (!ofputil_port_from_string(value, pp->port_map,
> >>> &os->sampling_port)) {
> >>> @@ -6346,14 +6540,23 @@ format_SAMPLE(const struct ofpact_sample *a,
> >>> const struct ofpact_format_params *fp)
> >>> {
> >>> ds_put_format(fp->s, "%ssample(%s%sprobability=%s%"PRIu16
> >>> - ",%scollector_set_id=%s%"PRIu32
> >>> - ",%sobs_domain_id=%s%"PRIu32
> >>> - ",%sobs_point_id=%s%"PRIu32,
> >>> + ",%scollector_set_id=%s%"PRIu32,
> >>> colors.paren, colors.end,
> >>> colors.param, colors.end, a->probability,
> >>> - colors.param, colors.end, a->collector_set_id,
> >>> - colors.param, colors.end, a->obs_domain_id,
> >>> - colors.param, colors.end, a->obs_point_id);
> >>> + colors.param, colors.end, a->collector_set_id);
> >>> +
> >>> + ds_put_format(fp->s, ",%sobs_domain_id=%s", colors.param,
> >>> colors.end);
> >>> + if (a->obs_domain_src.field) {
> >>> + mf_format_subfield(&a->obs_domain_src, fp->s);
> >>> + } else {
> >>> + ds_put_format(fp->s, "%"PRIu32, a->obs_domain_imm);
> >>> + }
> >>> + ds_put_format(fp->s, ",%sobs_point_id=%s", colors.param, colors.end);
> >>> + if (a->obs_point_src.field) {
> >>> + mf_format_subfield(&a->obs_point_src, fp->s);
> >>> + } else {
> >>> + ds_put_format(fp->s, "%"PRIu32, a->obs_point_imm);
> >>> + }
> >>> if (a->sampling_port != OFPP_NONE) {
> >>> ds_put_format(fp->s, ",%ssampling_port=%s", colors.param,
> >>> colors.end);
> >>> ofputil_format_port(a->sampling_port, fp->port_map, fp->s);
> >>> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >>> index 323a58cbf..2aff48f5e 100644
> >>> --- a/ofproto/ofproto-dpif-xlate.c
> >>> +++ b/ofproto/ofproto-dpif-xlate.c
> >>> @@ -5909,6 +5909,44 @@ xlate_fin_timeout(struct xlate_ctx *ctx,
> >>> }
> >>> }
> >>>
> >>> +static uint32_t
> >>> +ofpact_sample_get_domain(struct xlate_ctx *ctx,
> >>> + const struct ofpact_sample *os)
> >>> +{
> >>> + if (os->obs_domain_src.field) {
> >>> + union mf_subvalue *value = xmalloc(sizeof *value);
> >>> + uint32_t obs_domain_id;
> >>> +
> >>> + memset(value, 0xff, sizeof *value);
> >>> + obs_domain_id = mf_get_subfield(&os->obs_domain_src,
> >>> &ctx->xin->flow);
> >>> + mf_write_subfield_flow(&os->obs_domain_src, value,
> >>> &ctx->wc->masks);
> >>> +
> >>> + free(value);
> >>> + return obs_domain_id;
> >>> + } else {
> >>> + return os->obs_domain_imm;
> >>> + }
> >>> +}
> >>> +
> >>> +static uint32_t
> >>> +ofpact_sample_get_point(struct xlate_ctx *ctx,
> >>> + const struct ofpact_sample *os)
> >>> +{
> >>> + if (os->obs_point_src.field) {
> >>> + union mf_subvalue *value = xmalloc(sizeof *value);
> >>> + uint32_t obs_point_id;
> >>> +
> >>> + memset(value, 0xff, sizeof *value);
> >>> + obs_point_id = mf_get_subfield(&os->obs_point_src,
> >>> &ctx->xin->flow);
> >>> + mf_write_subfield_flow(&os->obs_point_src, value,
> >>> &ctx->wc->masks);
> >>> +
> >>> + free(value);
> >>> + return obs_point_id;
> >>> + } else {
> >>> + return os->obs_point_imm;
> >>> + }
> >>> +}
> >>> +
> >>> static void
> >>> xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
> >>> const struct ofpact_sample *os,
> >>> @@ -5975,8 +6013,10 @@ xlate_fill_ipfix_sample(struct xlate_ctx *ctx,
> >>> userspace->cookie.ofproto_uuid = ctx->xbridge->ofproto->uuid;
> >>> userspace->cookie.flow_sample.probability = os->probability;
> >>> userspace->cookie.flow_sample.collector_set_id =
> >>> os->collector_set_id;
> >>> - userspace->cookie.flow_sample.obs_domain_id = os->obs_domain_id;
> >>> - userspace->cookie.flow_sample.obs_point_id = os->obs_point_id;
> >>> + userspace->cookie.flow_sample.obs_domain_id =
> >>> + ofpact_sample_get_domain(ctx, os);
> >>> + userspace->cookie.flow_sample.obs_point_id =
> >>> + ofpact_sample_get_point(ctx, os);
> >>> userspace->cookie.flow_sample.output_odp_port = output_odp_port;
> >>> userspace->cookie.flow_sample.direction = os->direction;
> >>> userspace->include_actions = false;
> >>> @@ -5987,7 +6027,8 @@ xlate_sample_action(struct xlate_ctx *ctx,
> >>> const struct ofpact_sample *os,
> >>> bool last)
> >>> {
> >>> - uint8_t cookie_buf[sizeof(os->obs_domain_id) +
> >>> sizeof(os->obs_point_id)];
> >>> + uint8_t cookie_buf[sizeof(os->obs_domain_imm) +
> >>> + sizeof(os->obs_point_imm)];
> >>> struct dpif_lsample *lsample = ctx->xbridge->lsample;
> >>> struct dpif_ipfix *ipfix = ctx->xbridge->ipfix;
> >>> struct compose_sample_args compose_args = {};
> >>> @@ -6018,12 +6059,12 @@ xlate_sample_action(struct xlate_ctx *ctx,
> >>> ofpbuf_use_stub(&psample.cookie, cookie_buf, sizeof cookie_buf);
> >>>
> >>> data = ofpbuf_put_uninit(&psample.cookie,
> >>> - sizeof(os->obs_domain_id));
> >>> - *data = htonl(os->obs_domain_id);
> >>> + sizeof(os->obs_domain_imm));
> >>> + *data = htonl(ofpact_sample_get_domain(ctx, os));
> >>>
> >>> data = ofpbuf_put_uninit(&psample.cookie,
> >>> - sizeof(os->obs_point_id));
> >>> - *data = htonl(os->obs_point_id);
> >>> + sizeof(os->obs_point_imm));
> >>> + *data = htonl(ofpact_sample_get_point(ctx, os));
> >>>
> >>> compose_args.psample = &psample;
> >>>
> >>> diff --git a/python/ovs/flow/ofp.py b/python/ovs/flow/ofp.py
> >>> index 3d3226c91..f011b0460 100644
> >>> --- a/python/ovs/flow/ofp.py
> >>> +++ b/python/ovs/flow/ofp.py
> >>> @@ -30,7 +30,7 @@ from ovs.flow.ofp_act import (
> >>> decode_move_field,
> >>> decode_dec_ttl,
> >>> decode_chk_pkt_larger,
> >>> - decode_zone,
> >>> + decode_field_or_int,
> >>> decode_learn,
> >>> )
> >>>
> >>> @@ -330,7 +330,7 @@ class OFPFlow(Flow):
> >>> KVDecoders(
> >>> {
> >>> "commit": decode_flag,
> >>> - "zone": decode_zone,
> >>> + "zone": decode_field_or_int,
> >>> "table": decode_int,
> >>> "nat": decode_nat,
> >>> "force": decode_flag,
> >>> @@ -426,8 +426,8 @@ class OFPFlow(Flow):
> >>> {
> >>> "probability": decode_int,
> >>> "collector_set_id": decode_int,
> >>> - "obs_domain_id": decode_int,
> >>> - "obs_point_id": decode_int,
> >>> + "obs_domain_id": decode_field_or_int,
> >>> + "obs_point_id": decode_field_or_int,
> >>> "sampling_port": decode_default,
> >>> "ingress": decode_flag,
> >>> "egress": decode_flag,
> >>> diff --git a/python/ovs/flow/ofp_act.py b/python/ovs/flow/ofp_act.py
> >>> index 2c85076a3..32c5a8e17 100644
> >>> --- a/python/ovs/flow/ofp_act.py
> >>> +++ b/python/ovs/flow/ofp_act.py
> >>> @@ -246,8 +246,7 @@ def decode_chk_pkt_larger(value):
> >>> return {"pkt_len": pkt_len, "dst": dst}
> >>>
> >>>
> >>> -# CT decoders
> >>> -def decode_zone(value):
> >>> +def decode_field_or_int(value):
> >>> """Decodes the value of the 'zone' keyword (part of the ct
> >>> action)."""
> >>> try:
> >>> return int(value, 0)
> >>> @@ -256,6 +255,7 @@ def decode_zone(value):
> >>> return decode_field(value)
> >>>
> >>>
> >>> +# CT decoders
> >>> def decode_learn(action_decoders):
> >>> """Create the decoder to be used to decode the 'learn' action.
> >>>
> >>> diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
> >>> index 40a23bb15..fc531bc9f 100644
> >>> --- a/tests/ofp-actions.at
> >>> +++ b/tests/ofp-actions.at
> >>> @@ -489,6 +489,9 @@ ffff 0020 00002320 0015 000500000000 80003039005A02fd
> >>> 0400000000000000
> >>> #
> >>> actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=34567,obs_point_id=45678)
> >>> ffff 0018 00002320 001d 3039 00005BA0 00008707 0000B26E
> >>>
> >>> +#
> >>> actions=sample(probability=12345,collector_set_id=23456,obs_domain_id=NXM_NX_CT_LABEL[0..31],obs_point_id=NXM_NX_CT_LABEL[32..63],sampling_port=0)
> >>> +ffff 0028 00002320 0033 3039 00005ba0 0001d810 001f0000 0001d810
> >>> 081f0000 0000 000000000000
> >>> +
> >>> # bad OpenFlow11 actions: OFPBAC_BAD_OUT_PORT
> >>> & ofp_actions|WARN|bad action at offset 0 (OFPBAC_BAD_OUT_PORT):
> >>> & 00000000 00 00 00 10 ff ff ff ff-00 00 00 00 00 00 00 00
> >>> @@ -1121,6 +1124,8 @@ bad_action 'unroll_xlate' "UNROLL is an internal
> >>> action that shouldn't be used v
> >>> # sample
> >>> bad_action 'sample(probability=0)' 'invalid probability value "0"'
> >>> bad_action 'sample(sampling_port=asdf)' 'asdf: unknown port'
> >>> +bad_action
> >>> 'sample(probability=12345,obs_domain_id=NXM_NX_CT_LABEL[[0..30]])'
> >>> 'invalid length of obs_domain_id field (31). Must be 32'
> >>> +bad_action
> >>> 'sample(probability=12345,obs_point_id=NXM_NX_CT_LABEL[[0..32]])'
> >>> 'invalid length of obs_point_id field (33). Must be 32'
> >>> bad_action 'sample(foo=bar)' 'invalid key "foo" in "sample" argument'
> >>> bad_action 'sample' 'non-zero "probability" must be specified on sample'
> >>>
> >>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> >>> index ba8f3b69c..5d9a6e651 100644
> >>> --- a/tests/ofproto-dpif.at
> >>> +++ b/tests/ofproto-dpif.at
> >>> @@ -8304,6 +8304,47 @@ AT_CHECK([ovs-vsctl destroy
> >>> Flow_Sample_Collector_Set 1], [0], [ignore])
> >>> OVS_VSWITCHD_STOP
> >>> AT_CLEANUP
> >>>
> >>> +AT_SETUP([ofproto-dpif - Flow IPFIX sanity check - from field])
> >>> +OVS_VSWITCHD_START
> >>> +add_of_ports br0 1 2
> >>> +
> >>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> >>> + -- --id=@ipfix create IPFIX
> >>> targets=\"127.0.0.1:5500\" \
> >>> + -- --id=@cs create Flow_Sample_Collector_Set id=0
> >>> bridge=@br0 ipfix=@ipfix],
> >>> + [0], [ignore])
> >>> +
> >>> +m4_define([SAMPLE_ACTION],
> >>> +
> >>> [sample(probability=65535,collector_set_id=1,obs_domain_id=NXM_NX_REG0,obs_point_id=NXM_NX_REG1)]dnl
> >>> +)
> >>> +
> >>> +dnl Store in_port in obs_domain_id and dp_hash in the obs_point_id.
> >>> +AT_DATA([flows.txt], [dnl
> >>> +table=0,priority=100,arp,action=normal
> >>> +table=0,priority=10,ip actions=move:in_port->reg0[[0..15]],
> >>> move:dp_hash->reg1, SAMPLE_ACTION, goto_table:1
> >>> +table=1,in_port=1 actions=2
> >>> +table=1,in_port=2 actions=1
> >>> +])
> >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt], [0], [ignore])
> >>> +
> >>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy \
> >>> +
> >>> "in_port(1),dp_hash(45),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),\
> >>> +
> >>> ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)"],
> >>> [0], [stdout])
> >>> +
> >>> +AT_CHECK([tail -1 stdout], [0], [dnl
> >>> +Datapath actions:
> >>> userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=1,obs_point_id=45,output_port=4294967295)),2
> >>> +])
> >>> +
> >>> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy \
> >>> +
> >>> "in_port(2),dp_hash(59),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),\
> >>> +
> >>> ipv4(src=10.10.10.2,dst=10.10.10.1,proto=1,tos=1,ttl=128,frag=no),icmp(type=8,code=0)"],
> >>> [0], [stdout])
> >>> +
> >>> +AT_CHECK([tail -1 stdout], [0], [dnl
> >>> +Datapath actions:
> >>> userspace(pid=0,flow_sample(probability=65535,collector_set_id=1,obs_domain_id=2,obs_point_id=59,output_port=4294967295)),1
> >>> +])
> >>
> >> It's not enough to only check the actions, we need to verify that
> >> fields are getting added to the match as well.
> >>
> >
> > Right. I do verify that in the next test but not in this one.
> >
> >>> +
> >>> +OVS_VSWITCHD_STOP
> >>> +AT_CLEANUP
> >>> +
> >>> AT_SETUP([ofproto-dpif - clone action])
> >>> OVS_VSWITCHD_START
> >>> add_of_ports br0 1 2 3 4
> >>> diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> >>> index ddab4ece3..2ed425029 100644
> >>> --- a/tests/system-traffic.at
> >>> +++ b/tests/system-traffic.at
> >>> @@ -9388,3 +9388,77 @@ dnl OVS will fail to send IPFIX packets because
> >>> the target is localhost
> >>> dnl and the port is closed. Ignore the message it generates.
> >>> OVS_TRAFFIC_VSWITCHD_STOP(["/sending to collector failed/d"])
> >>> AT_CLEANUP
> >>> +
> >>> +AT_SETUP([psample - from ct label])
> >>> +CHECK_CONNTRACK()
> >>> +OVS_TRAFFIC_VSWITCHD_START()
> >>> +OVS_CHECK_PSAMPLE()
> >>> +
> >>> +ADD_NAMESPACES(at_ns0, at_ns1)
> >>> +NS_CHECK_EXEC([at_ns0], [sysctl -w net.ipv6.conf.all.disable_ipv6=1],
> >>> [0], [ignore])
> >>> +NS_CHECK_EXEC([at_ns1], [sysctl -w net.ipv6.conf.all.disable_ipv6=1],
> >>> [0], [ignore])
> >>> +
> >>> +ADD_VETH(p0, at_ns0, br0, "10.1.1.1/24", "e4:11:22:33:44:55")
> >>> +ADD_VETH(p1, at_ns1, br0, "10.1.1.2/24", "e4:11:22:33:44:66")
> >>> +
> >>> +AT_CHECK([ovs-vsctl -- --id=@br0 get Bridge br0 \
> >>> + -- --id=@ipfix create IPFIX
> >>> targets=\"127.0.0.1:4739\" \
> >>> + -- create Flow_Sample_Collector_Set id=1 bridge=@br0
> >>> ipfix=@ipfix, local_group_id=10 \
> >>> + -- create Flow_Sample_Collector_Set id=2 bridge=@br0
> >>> ipfix=@ipfix, local_group_id=12],
> >>> + [0], [ignore])
> >>> +
> >>> +
> >>> +m4_define([CT_STORE_ACT],
> >>> +
> >>> [ct(zone=5,commit,exec(load:0x0bb102030->NXM_NX_CT_LABEL[[0..31]],load:0xbb405060->NXM_NX_CT_LABEL[[32..63]]))])
> >>> +
> >>> +AT_DATA([flows.txt], [dnl
> >>> +priority=100,ip actions=ct(zone=5, table=10)
> >>> +priority=0 actions=NORMAL
> >>> +table=10,priority=100,ip,ct_state=+trk+new action=SAMPLE_ACTION(1,
> >>> 2853183536, 2856341600),CT_STORE_ACT,NORMAL
> >>> +table=10,priority=100,ip,ct_state=+trk-new action=SAMPLE_ACTION(2,
> >>> NXM_NX_CT_LABEL[[[0..31]]], NXM_NX_CT_LABEL[[[32..63]]]),NORMAL
> >>> +table=10, priority=50, ip, actions=DROP
> >>> +])
> >>> +
> >>> +AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
> >>> +
> >>> +OVS_DAEMONIZE([ovstest test-psample > psample.out], [psample1.pid])
> >>> +
> >>> +NS_CHECK_EXEC([at_ns0], [ping -q -c 1 10.1.1.2 | FORMAT_PING], [0], [dnl
> >>> +1 packets transmitted, 1 received, 0% packet loss, time 0ms
> >>> +])
> >>> +
> >>> +m4_define([SAMPLE1], [m4_join([ ],
> >>> + [group_id=0xa],
> >>> + [obs_domain=0xaa102030,obs_point=0xaa405060],
> >>> + [.*icmp.*nw_src=10.1.1.1,nw_dst=10.1.1.2])])
> >>> +
> >>> +m4_define([SAMPLE2], [m4_join([ ],
> >>> + [group_id=0xc],
> >>> + [obs_domain=0xbb102030,obs_point=0xbb405060],
> >>> + [.*icmp.*nw_src=10.1.1.2,nw_dst=10.1.1.1])])
> >>> +AT_CHECK([grep -qE 'SAMPLE1' psample.out])
> >>> +AT_CHECK([grep -qE 'SAMPLE2' psample.out])
> >>> +
> >>> +m4_define([FLOW_MATCH], [m4_join([],
> >>> + [ct_label(0xbb405060bb102030/0xffffffffffffffff).*actions:],
> >>> + [actions:psample(group=12,cookie=0xbb102030bb405060),],
> >>> +
> >>> [userspace(pid=[[0-9]]+,flow_sample(.*obs_domain_id=3138396208,obs_point_id=3141554272.*))]
> >>> +)])
> >>> +
> >>> +AT_CHECK([ovs-appctl dpctl/dump-flows --names filter=in_port=ovs-p1 dnl
> >>> + | grep -qE 'FLOW_MATCH' ], [0], [])
> >>> +
> >>> +AT_CHECK([ovs-appctl lsample/show br0], [0], [dnl
> >>> +Local sample statistics for bridge "br0":
> >>> +Collector Set ID: 1:
> >>> + Group ID : 10
> >>> + Total packets: 1
> >>> + Total bytes : 98
> >>> +
> >>> +Collector Set ID: 2:
> >>> + Group ID : 12
> >>> + Total packets: 1
> >>> + Total bytes : 98
> >>> +])
> >>> +
> >>> +AT_CLEANUP
> >>
> >
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
