On 9 Jul 2024, at 16:17, Adrián Moreno wrote:
> On Tue, Jul 09, 2024 at 11:46:12AM GMT, Eelco Chaudron wrote: >> On 7 Jul 2024, at 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> >> >> See some comments inline. I’m missing the documentation update. >> > > Yes. I noticed I missed both the NEWS and documentation update. > >> Cheers, >> >> Eelco >> >>> --- >>> 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(-) >>> >>> 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 */ >>> + 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. */ >>> + ovs_be32 vendor; /* NX_VENDOR_ID. */ >>> + ovs_be16 subtype; /* NXAST_SAMPLE. */ >>> + 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 */ >>> + }; >> >> Maybe align all comments, and make sure all end with a dot. >> >> struct nx_action_sample4 { >> ovs_be16 type; /* OFPAT_VENDOR. */ >> ovs_be16 len; /* Length is 32. */ >> ovs_be32 vendor; /* NX_VENDOR_ID. */ >> ovs_be16 subtype; /* NXAST_SAMPLE. */ >> ovs_be16 probability; /* Fraction of packets to sample. */ >> ovs_be32 collector_set_id; /* ID of collector set in OVSDB. */ >> ovs_be32 obs_domain_src; /* The observation_domain_id source. >> */ >> 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; /* The observation_point_id source. */ >> 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. */ >> }; >> >> >>> + 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; >>> +} >> >> Newline between functions. >> > > Ack > >>> 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); >>> + return OFPERR_OFPBAC_BAD_SET_LEN; >>> + } >>> + } 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; >>> +} >>> + >>> 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) { >>> + 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) { >>> + 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); >> >> Would it be better to just put these 128 bytes on the stack to avoid >> memory fragmentation? Same for the next function. >> >> Or maybe something better, we could do something like we did for >> exact_match_mask? >> >> const union mf_value exact_match_mask = MF_VALUE_EXACT_INITIALIZER; >> extern const union mf_value exact_match_mask; >> > > A quick look seemed to indicate dynamic memory was preferred in this > module so I assumed there was a reason for it (I do remember stack size > being a problem at some point, not sure if it was here). > > I'll look at the exact_match_mask case. I guess this function is not called recursively, so we should be ok with the stack usage. But a const variable might be better (and we can fix the other place(s) where you copied this code from ;). >>> + 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) >> >> Fix alignment of const to the previous line. >> > > Ack. > >>> +{ >>> + 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).""" >> >> You need to update the help text also. >> > > Yep. > >>> 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 >>> +]) >>> + >>> +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 \ >> >> Maybe wrap to 80 chars? >> >> -- create Flow_Sample_Collector_Set id=1 bridge=@br0 \ >> ipfix=@ipfix, local_group_id=10 \ >> > > Ack. > >>> + -- 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])]) >> >> New line? >> > > Ack. > >>> +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 >> >> Maybe use \ instead of dnl, as you do in other places for AT_CHECK()? >> > > Ack. > >>> + | 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 >>> +]) >>> + >> >> Missing OVS_VSWITCHD_STOP()? >> > > Ack. > >>> +AT_CLEANUP >>> -- >>> 2.45.2 >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev