On 7/11/24 09:28, Adrián Moreno wrote:
> 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).
I think, obs_domain_id has a clear integer semantics as well and it is
not part of the packet metadata, it's just an argument of an action.
>
> 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).
Registers are a scarce resource. There may be no free register to use.
OVN itself is running out of registers to use these days. Other SDN
controllers may not have regissters to spare as well. The fact that you
can do that also highlights the arbitrary nature of the limitation.
One more thing about 'move' is that it is getting translated into
OpenFlow's COPY_FIELD actions, which contains two field offsets and
a single n_bits field. That maybe the reason why move requires the
sizes to be the same.
>
> 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.
I think, we should 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
