diff mbox series

[ovs-dev] Fix OVN update issue when ovn-controller is updated first from 20.06 to 20.09.

Message ID 20201120105253.2160203-1-numans@ovn.org
State Accepted
Headers show
Series [ovs-dev] Fix OVN update issue when ovn-controller is updated first from 20.06 to 20.09. | expand

Commit Message

Numan Siddique Nov. 20, 2020, 10:52 a.m. UTC
From: Numan Siddique <numans@ovn.org>

The commit in the Fixes tag changed the ct_commit signature from
ct_commit(..) to ct_commit{ ..}.  When ovn-controllers are updated
to a vesion which has this change and if ovn-northd is still not
udated to this version, then the logical flow with the action
ct_commit(..) will be rejected by ovn-controllers resulting in
datapath disruptions.  OVN recommends ovn-controller updates before
ovn-northd, but it is broken now.  This patch fixes this issue by
adding back the support for the old ct_commit(..).  We now support
both the versions of ct_commit.

Fixes: 6cfb44a76c61("Used nested actions in ct_commit)
CC: Mark Michelson <mmichels@redhat.com>
Signed-off-by: Numan Siddique <numans@ovn.org>
---
 include/ovn/actions.h |  10 ++-
 lib/actions.c         | 137 ++++++++++++++++++++++++++++++++++++++++--
 tests/ovn.at          |  44 ++++++++++++++
 utilities/ovn-trace.c |   3 +-
 4 files changed, 186 insertions(+), 8 deletions(-)

Comments

Flavio Fernandes Nov. 22, 2020, 10:50 p.m. UTC | #1
Acked-by: Flavio Fernandes <flavio@flaviof.com>


> On Nov 20, 2020, at 5:52 AM, numans@ovn.org wrote:
> 
> From: Numan Siddique <numans@ovn.org>
> 
> The commit in the Fixes tag changed the ct_commit signature from
> ct_commit(..) to ct_commit{ ..}.  When ovn-controllers are updated
> to a vesion which has this change and if ovn-northd is still not
> udated to this version, then the logical flow with the action
> ct_commit(..) will be rejected by ovn-controllers resulting in
> datapath disruptions.  OVN recommends ovn-controller updates before
> ovn-northd, but it is broken now.  This patch fixes this issue by
> adding back the support for the old ct_commit(..).  We now support
> both the versions of ct_commit.
> 
> Fixes: 6cfb44a76c61("Used nested actions in ct_commit)
> CC: Mark Michelson <mmichels@redhat.com>
> Signed-off-by: Numan Siddique <numans@ovn.org>
> ---
> include/ovn/actions.h |  10 ++-
> lib/actions.c         | 137 ++++++++++++++++++++++++++++++++++++++++--
> tests/ovn.at          |  44 ++++++++++++++
> utilities/ovn-trace.c |   3 +-
> 4 files changed, 186 insertions(+), 8 deletions(-)
> 
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index 630bbe79e..7bec6a6f8 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -57,7 +57,8 @@ struct ovn_extend_table;
>     OVNACT(EXCHANGE,          ovnact_move)            \
>     OVNACT(DEC_TTL,           ovnact_null)            \
>     OVNACT(CT_NEXT,           ovnact_ct_next)         \
> -    OVNACT(CT_COMMIT,         ovnact_nest)            \
> +    OVNACT(CT_COMMIT_V1,      ovnact_ct_commit_v1)    \
> +    OVNACT(CT_COMMIT_V2,      ovnact_nest)            \
>     OVNACT(CT_DNAT,           ovnact_ct_nat)          \
>     OVNACT(CT_SNAT,           ovnact_ct_nat)          \
>     OVNACT(CT_LB,             ovnact_ct_lb)           \
> @@ -226,6 +227,13 @@ struct ovnact_ct_next {
>     uint8_t ltable;                /* Logical table ID of next table. */
> };
> 
> +/* OVNACT_CT_COMMIT_V1. */
> +struct ovnact_ct_commit_v1 {
> +    struct ovnact ovnact;
> +    uint32_t ct_mark, ct_mark_mask;
> +    ovs_be128 ct_label, ct_label_mask;
> +};
> +
> /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
> struct ovnact_ct_nat {
>     struct ovnact ovnact;
> diff --git a/lib/actions.c b/lib/actions.c
> index 82b35ccf9..36c8078b0 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -627,16 +627,75 @@ ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED)
> {
> }
> 
> +static void
> +parse_ct_commit_v1_arg(struct action_context *ctx,
> +                       struct ovnact_ct_commit_v1 *cc)
> +{
> +    if (lexer_match_id(ctx->lexer, "ct_mark")) {
> +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +            return;
> +        }
> +        if (ctx->lexer->token.type == LEX_T_INTEGER) {
> +            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
> +            cc->ct_mark_mask = UINT32_MAX;
> +        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
> +            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
> +            cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);
> +        } else {
> +            lexer_syntax_error(ctx->lexer, "expecting integer");
> +            return;
> +        }
> +        lexer_get(ctx->lexer);
> +    } else if (lexer_match_id(ctx->lexer, "ct_label")) {
> +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> +            return;
> +        }
> +        if (ctx->lexer->token.type == LEX_T_INTEGER) {
> +            cc->ct_label = ctx->lexer->token.value.be128_int;
> +            cc->ct_label_mask = OVS_BE128_MAX;
> +        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
> +            cc->ct_label = ctx->lexer->token.value.be128_int;
> +            cc->ct_label_mask = ctx->lexer->token.mask.be128_int;
> +        } else {
> +            lexer_syntax_error(ctx->lexer, "expecting integer");
> +            return;
> +        }
> +        lexer_get(ctx->lexer);
> +    } else {
> +        lexer_syntax_error(ctx->lexer, NULL);
> +    }
> +}
> +
> +static void
> +parse_CT_COMMIT_V1(struct action_context *ctx)
> +{
> +    add_prerequisite(ctx, "ip");
> +
> +    struct ovnact_ct_commit_v1 *ct_commit =
> +        ovnact_put_CT_COMMIT_V1(ctx->ovnacts);
> +    if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> +        while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> +            parse_ct_commit_v1_arg(ctx, ct_commit);
> +            if (ctx->lexer->error) {
> +                return;
> +            }
> +            lexer_match(ctx->lexer, LEX_T_COMMA);
> +        }
> +    }
> +}
> +
> static void
> parse_CT_COMMIT(struct action_context *ctx)
> {
>     if (ctx->lexer->token.type == LEX_T_LCURLY) {
> -        parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip",
> +        parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",
>                             WR_CT_COMMIT);
> +    } else if (ctx->lexer->token.type == LEX_T_LPAREN) {
> +        parse_CT_COMMIT_V1(ctx);
>     } else {
>         /* Add an empty nested action to allow for "ct_commit;" syntax */
>         add_prerequisite(ctx, "ip");
> -        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT,
> +        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT_V2,
>                                             OVNACT_ALIGN(sizeof *on));
>         on->nested_len = 0;
>         on->nested = NULL;
> @@ -644,7 +703,73 @@ parse_CT_COMMIT(struct action_context *ctx)
> }
> 
> static void
> -format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)
> +format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, struct ds *s)
> +{
> +    ds_put_cstr(s, "ct_commit(");
> +    if (cc->ct_mark_mask) {
> +        ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark);
> +        if (cc->ct_mark_mask != UINT32_MAX) {
> +            ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask);
> +        }
> +    }
> +    if (!ovs_be128_is_zero(cc->ct_label_mask)) {
> +        if (ds_last(s) != '(') {
> +            ds_put_cstr(s, ", ");
> +        }
> +
> +        ds_put_format(s, "ct_label=");
> +        ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label);
> +        if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) {
> +            ds_put_char(s, '/');
> +            ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask);
> +        }
> +    }
> +    if (!ds_chomp(s, '(')) {
> +        ds_put_char(s, ')');
> +    }
> +    ds_put_char(s, ';');
> +}
> +
> +static void
> +encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc,
> +                    const struct ovnact_encode_params *ep OVS_UNUSED,
> +                    struct ofpbuf *ofpacts)
> +{
> +    struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> +    ct->flags = NX_CT_F_COMMIT;
> +    ct->recirc_table = NX_CT_RECIRC_NONE;
> +    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> +    ct->zone_src.ofs = 0;
> +    ct->zone_src.n_bits = 16;
> +
> +    size_t set_field_offset = ofpacts->size;
> +    ofpbuf_pull(ofpacts, set_field_offset);
> +
> +    if (cc->ct_mark_mask) {
> +        const ovs_be32 value = htonl(cc->ct_mark);
> +        const ovs_be32 mask = htonl(cc->ct_mark_mask);
> +        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, &mask);
> +    }
> +
> +    if (!ovs_be128_is_zero(cc->ct_label_mask)) {
> +        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,
> +                             &cc->ct_label_mask);
> +    }
> +
> +    ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
> +    ct = ofpacts->header;
> +    ofpact_finish(ofpacts, &ct->ofpact);
> +}
> +
> +static void
> +ovnact_ct_commit_v1_free(struct ovnact_ct_commit_v1 *cc OVS_UNUSED)
> +{
> +}
> +
> +
> +
> +static void
> +format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s)
> {
>     if (on->nested_len) {
>         format_nested_action(on, "ct_commit", s);
> @@ -654,9 +779,9 @@ format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)
> }
> 
> static void
> -encode_CT_COMMIT(const struct ovnact_nest *on,
> -                 const struct ovnact_encode_params *ep OVS_UNUSED,
> -                 struct ofpbuf *ofpacts)
> +encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> +                    const struct ovnact_encode_params *ep OVS_UNUSED,
> +                    struct ofpbuf *ofpacts)
> {
>     struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
>     ct->flags = NX_CT_F_COMMIT;
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 2253d0bde..e2568bb66 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1102,6 +1102,50 @@ ct_commit { ct_label=0x181716151413121110090807060504030201; };
> ct_commit { ip4.dst = 192.168.0.1; };
>     Field ip4.dst is not modifiable.
> 
> +# Legact ct_commit_v1 action.
> +ct_commit();
> +    formats as ct_commit;
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15])
> +    has prereqs ip
> +ct_commit(ct_mark=1);
> +    formats as ct_commit(ct_mark=0x1);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
> +    has prereqs ip
> +ct_commit(ct_mark=1/1);
> +    formats as ct_commit(ct_mark=0x1/0x1);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))
> +    has prereqs ip
> +ct_commit(ct_label=1);
> +    formats as ct_commit(ct_label=0x1);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_label))
> +    has prereqs ip
> +ct_commit(ct_label=1/1);
> +    formats as ct_commit(ct_label=0x1/0x1);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_label))
> +    has prereqs ip
> +ct_commit(ct_mark=1, ct_label=2);
> +    formats as ct_commit(ct_mark=0x1, ct_label=0x2);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
> +    has prereqs ip
> +
> +ct_commit(ct_label=0x01020304050607080910111213141516);
> +    formats as ct_commit(ct_label=0x1020304050607080910111213141516);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label))
> +    has prereqs ip
> +ct_commit(ct_label=0x181716151413121110090807060504030201);
> +    formats as ct_commit(ct_label=0x16151413121110090807060504030201);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x16151413121110090807060504030201->ct_label))
> +    has prereqs ip
> +ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label))
> +    has prereqs ip
> +ct_commit(ct_label=18446744073709551615);
> +    formats as ct_commit(ct_label=0xffffffffffffffff);
> +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xffffffffffffffff->ct_label))
> +    has prereqs ip
> +ct_commit(ct_label=18446744073709551616);
> +    Decimal constants must be less than 2**64.
> +
> ct_mark = 12345
>     Field ct_mark is not modifiable.
> ct_label = 0xcafe
> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> index 5d92188ab..8eb7263b3 100644
> --- a/utilities/ovn-trace.c
> +++ b/utilities/ovn-trace.c
> @@ -2336,7 +2336,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
>             execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, super);
>             break;
> 
> -        case OVNACT_CT_COMMIT:
> +        case OVNACT_CT_COMMIT_V1:
> +        case OVNACT_CT_COMMIT_V2:
>             /* Nothing to do. */
>             break;
> 
> --
> 2.28.0
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Numan Siddique Nov. 23, 2020, 6:49 a.m. UTC | #2
On Mon, Nov 23, 2020 at 4:21 AM Flavio Fernandes <flavio@flaviof.com> wrote:
>
> Acked-by: Flavio Fernandes <flavio@flaviof.com>

Thanks Flavio for the reviews. I applied this patch to master. I think
this should also be backported
to branch-20.09. I will do it in sometime.

Thanks
Numan

>
>
> > On Nov 20, 2020, at 5:52 AM, numans@ovn.org wrote:
> >
> > From: Numan Siddique <numans@ovn.org>
> >
> > The commit in the Fixes tag changed the ct_commit signature from
> > ct_commit(..) to ct_commit{ ..}.  When ovn-controllers are updated
> > to a vesion which has this change and if ovn-northd is still not
> > udated to this version, then the logical flow with the action
> > ct_commit(..) will be rejected by ovn-controllers resulting in
> > datapath disruptions.  OVN recommends ovn-controller updates before
> > ovn-northd, but it is broken now.  This patch fixes this issue by
> > adding back the support for the old ct_commit(..).  We now support
> > both the versions of ct_commit.
> >
> > Fixes: 6cfb44a76c61("Used nested actions in ct_commit)
> > CC: Mark Michelson <mmichels@redhat.com>
> > Signed-off-by: Numan Siddique <numans@ovn.org>
> > ---
> > include/ovn/actions.h |  10 ++-
> > lib/actions.c         | 137 ++++++++++++++++++++++++++++++++++++++++--
> > tests/ovn.at          |  44 ++++++++++++++
> > utilities/ovn-trace.c |   3 +-
> > 4 files changed, 186 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > index 630bbe79e..7bec6a6f8 100644
> > --- a/include/ovn/actions.h
> > +++ b/include/ovn/actions.h
> > @@ -57,7 +57,8 @@ struct ovn_extend_table;
> >     OVNACT(EXCHANGE,          ovnact_move)            \
> >     OVNACT(DEC_TTL,           ovnact_null)            \
> >     OVNACT(CT_NEXT,           ovnact_ct_next)         \
> > -    OVNACT(CT_COMMIT,         ovnact_nest)            \
> > +    OVNACT(CT_COMMIT_V1,      ovnact_ct_commit_v1)    \
> > +    OVNACT(CT_COMMIT_V2,      ovnact_nest)            \
> >     OVNACT(CT_DNAT,           ovnact_ct_nat)          \
> >     OVNACT(CT_SNAT,           ovnact_ct_nat)          \
> >     OVNACT(CT_LB,             ovnact_ct_lb)           \
> > @@ -226,6 +227,13 @@ struct ovnact_ct_next {
> >     uint8_t ltable;                /* Logical table ID of next table. */
> > };
> >
> > +/* OVNACT_CT_COMMIT_V1. */
> > +struct ovnact_ct_commit_v1 {
> > +    struct ovnact ovnact;
> > +    uint32_t ct_mark, ct_mark_mask;
> > +    ovs_be128 ct_label, ct_label_mask;
> > +};
> > +
> > /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
> > struct ovnact_ct_nat {
> >     struct ovnact ovnact;
> > diff --git a/lib/actions.c b/lib/actions.c
> > index 82b35ccf9..36c8078b0 100644
> > --- a/lib/actions.c
> > +++ b/lib/actions.c
> > @@ -627,16 +627,75 @@ ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED)
> > {
> > }
> >
> > +static void
> > +parse_ct_commit_v1_arg(struct action_context *ctx,
> > +                       struct ovnact_ct_commit_v1 *cc)
> > +{
> > +    if (lexer_match_id(ctx->lexer, "ct_mark")) {
> > +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> > +            return;
> > +        }
> > +        if (ctx->lexer->token.type == LEX_T_INTEGER) {
> > +            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
> > +            cc->ct_mark_mask = UINT32_MAX;
> > +        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
> > +            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
> > +            cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);
> > +        } else {
> > +            lexer_syntax_error(ctx->lexer, "expecting integer");
> > +            return;
> > +        }
> > +        lexer_get(ctx->lexer);
> > +    } else if (lexer_match_id(ctx->lexer, "ct_label")) {
> > +        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
> > +            return;
> > +        }
> > +        if (ctx->lexer->token.type == LEX_T_INTEGER) {
> > +            cc->ct_label = ctx->lexer->token.value.be128_int;
> > +            cc->ct_label_mask = OVS_BE128_MAX;
> > +        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
> > +            cc->ct_label = ctx->lexer->token.value.be128_int;
> > +            cc->ct_label_mask = ctx->lexer->token.mask.be128_int;
> > +        } else {
> > +            lexer_syntax_error(ctx->lexer, "expecting integer");
> > +            return;
> > +        }
> > +        lexer_get(ctx->lexer);
> > +    } else {
> > +        lexer_syntax_error(ctx->lexer, NULL);
> > +    }
> > +}
> > +
> > +static void
> > +parse_CT_COMMIT_V1(struct action_context *ctx)
> > +{
> > +    add_prerequisite(ctx, "ip");
> > +
> > +    struct ovnact_ct_commit_v1 *ct_commit =
> > +        ovnact_put_CT_COMMIT_V1(ctx->ovnacts);
> > +    if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
> > +        while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
> > +            parse_ct_commit_v1_arg(ctx, ct_commit);
> > +            if (ctx->lexer->error) {
> > +                return;
> > +            }
> > +            lexer_match(ctx->lexer, LEX_T_COMMA);
> > +        }
> > +    }
> > +}
> > +
> > static void
> > parse_CT_COMMIT(struct action_context *ctx)
> > {
> >     if (ctx->lexer->token.type == LEX_T_LCURLY) {
> > -        parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip",
> > +        parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",
> >                             WR_CT_COMMIT);
> > +    } else if (ctx->lexer->token.type == LEX_T_LPAREN) {
> > +        parse_CT_COMMIT_V1(ctx);
> >     } else {
> >         /* Add an empty nested action to allow for "ct_commit;" syntax */
> >         add_prerequisite(ctx, "ip");
> > -        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT,
> > +        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT_V2,
> >                                             OVNACT_ALIGN(sizeof *on));
> >         on->nested_len = 0;
> >         on->nested = NULL;
> > @@ -644,7 +703,73 @@ parse_CT_COMMIT(struct action_context *ctx)
> > }
> >
> > static void
> > -format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)
> > +format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, struct ds *s)
> > +{
> > +    ds_put_cstr(s, "ct_commit(");
> > +    if (cc->ct_mark_mask) {
> > +        ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark);
> > +        if (cc->ct_mark_mask != UINT32_MAX) {
> > +            ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask);
> > +        }
> > +    }
> > +    if (!ovs_be128_is_zero(cc->ct_label_mask)) {
> > +        if (ds_last(s) != '(') {
> > +            ds_put_cstr(s, ", ");
> > +        }
> > +
> > +        ds_put_format(s, "ct_label=");
> > +        ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label);
> > +        if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) {
> > +            ds_put_char(s, '/');
> > +            ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask);
> > +        }
> > +    }
> > +    if (!ds_chomp(s, '(')) {
> > +        ds_put_char(s, ')');
> > +    }
> > +    ds_put_char(s, ';');
> > +}
> > +
> > +static void
> > +encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc,
> > +                    const struct ovnact_encode_params *ep OVS_UNUSED,
> > +                    struct ofpbuf *ofpacts)
> > +{
> > +    struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> > +    ct->flags = NX_CT_F_COMMIT;
> > +    ct->recirc_table = NX_CT_RECIRC_NONE;
> > +    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
> > +    ct->zone_src.ofs = 0;
> > +    ct->zone_src.n_bits = 16;
> > +
> > +    size_t set_field_offset = ofpacts->size;
> > +    ofpbuf_pull(ofpacts, set_field_offset);
> > +
> > +    if (cc->ct_mark_mask) {
> > +        const ovs_be32 value = htonl(cc->ct_mark);
> > +        const ovs_be32 mask = htonl(cc->ct_mark_mask);
> > +        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, &mask);
> > +    }
> > +
> > +    if (!ovs_be128_is_zero(cc->ct_label_mask)) {
> > +        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,
> > +                             &cc->ct_label_mask);
> > +    }
> > +
> > +    ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
> > +    ct = ofpacts->header;
> > +    ofpact_finish(ofpacts, &ct->ofpact);
> > +}
> > +
> > +static void
> > +ovnact_ct_commit_v1_free(struct ovnact_ct_commit_v1 *cc OVS_UNUSED)
> > +{
> > +}
> > +
> > +
> > +
> > +static void
> > +format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s)
> > {
> >     if (on->nested_len) {
> >         format_nested_action(on, "ct_commit", s);
> > @@ -654,9 +779,9 @@ format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)
> > }
> >
> > static void
> > -encode_CT_COMMIT(const struct ovnact_nest *on,
> > -                 const struct ovnact_encode_params *ep OVS_UNUSED,
> > -                 struct ofpbuf *ofpacts)
> > +encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> > +                    const struct ovnact_encode_params *ep OVS_UNUSED,
> > +                    struct ofpbuf *ofpacts)
> > {
> >     struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> >     ct->flags = NX_CT_F_COMMIT;
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 2253d0bde..e2568bb66 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1102,6 +1102,50 @@ ct_commit { ct_label=0x181716151413121110090807060504030201; };
> > ct_commit { ip4.dst = 192.168.0.1; };
> >     Field ip4.dst is not modifiable.
> >
> > +# Legact ct_commit_v1 action.
> > +ct_commit();
> > +    formats as ct_commit;
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15])
> > +    has prereqs ip
> > +ct_commit(ct_mark=1);
> > +    formats as ct_commit(ct_mark=0x1);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
> > +    has prereqs ip
> > +ct_commit(ct_mark=1/1);
> > +    formats as ct_commit(ct_mark=0x1/0x1);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))
> > +    has prereqs ip
> > +ct_commit(ct_label=1);
> > +    formats as ct_commit(ct_label=0x1);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_label))
> > +    has prereqs ip
> > +ct_commit(ct_label=1/1);
> > +    formats as ct_commit(ct_label=0x1/0x1);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_label))
> > +    has prereqs ip
> > +ct_commit(ct_mark=1, ct_label=2);
> > +    formats as ct_commit(ct_mark=0x1, ct_label=0x2);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
> > +    has prereqs ip
> > +
> > +ct_commit(ct_label=0x01020304050607080910111213141516);
> > +    formats as ct_commit(ct_label=0x1020304050607080910111213141516);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label))
> > +    has prereqs ip
> > +ct_commit(ct_label=0x181716151413121110090807060504030201);
> > +    formats as ct_commit(ct_label=0x16151413121110090807060504030201);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x16151413121110090807060504030201->ct_label))
> > +    has prereqs ip
> > +ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label))
> > +    has prereqs ip
> > +ct_commit(ct_label=18446744073709551615);
> > +    formats as ct_commit(ct_label=0xffffffffffffffff);
> > +    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xffffffffffffffff->ct_label))
> > +    has prereqs ip
> > +ct_commit(ct_label=18446744073709551616);
> > +    Decimal constants must be less than 2**64.
> > +
> > ct_mark = 12345
> >     Field ct_mark is not modifiable.
> > ct_label = 0xcafe
> > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > index 5d92188ab..8eb7263b3 100644
> > --- a/utilities/ovn-trace.c
> > +++ b/utilities/ovn-trace.c
> > @@ -2336,7 +2336,8 @@ trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
> >             execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, super);
> >             break;
> >
> > -        case OVNACT_CT_COMMIT:
> > +        case OVNACT_CT_COMMIT_V1:
> > +        case OVNACT_CT_COMMIT_V2:
> >             /* Nothing to do. */
> >             break;
> >
> > --
> > 2.28.0
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
diff mbox series

Patch

diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index 630bbe79e..7bec6a6f8 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -57,7 +57,8 @@  struct ovn_extend_table;
     OVNACT(EXCHANGE,          ovnact_move)            \
     OVNACT(DEC_TTL,           ovnact_null)            \
     OVNACT(CT_NEXT,           ovnact_ct_next)         \
-    OVNACT(CT_COMMIT,         ovnact_nest)            \
+    OVNACT(CT_COMMIT_V1,      ovnact_ct_commit_v1)    \
+    OVNACT(CT_COMMIT_V2,      ovnact_nest)            \
     OVNACT(CT_DNAT,           ovnact_ct_nat)          \
     OVNACT(CT_SNAT,           ovnact_ct_nat)          \
     OVNACT(CT_LB,             ovnact_ct_lb)           \
@@ -226,6 +227,13 @@  struct ovnact_ct_next {
     uint8_t ltable;                /* Logical table ID of next table. */
 };
 
+/* OVNACT_CT_COMMIT_V1. */
+struct ovnact_ct_commit_v1 {
+    struct ovnact ovnact;
+    uint32_t ct_mark, ct_mark_mask;
+    ovs_be128 ct_label, ct_label_mask;
+};
+
 /* OVNACT_CT_DNAT, OVNACT_CT_SNAT. */
 struct ovnact_ct_nat {
     struct ovnact ovnact;
diff --git a/lib/actions.c b/lib/actions.c
index 82b35ccf9..36c8078b0 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -627,16 +627,75 @@  ovnact_ct_next_free(struct ovnact_ct_next *a OVS_UNUSED)
 {
 }
 
+static void
+parse_ct_commit_v1_arg(struct action_context *ctx,
+                       struct ovnact_ct_commit_v1 *cc)
+{
+    if (lexer_match_id(ctx->lexer, "ct_mark")) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+            return;
+        }
+        if (ctx->lexer->token.type == LEX_T_INTEGER) {
+            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
+            cc->ct_mark_mask = UINT32_MAX;
+        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
+            cc->ct_mark = ntohll(ctx->lexer->token.value.integer);
+            cc->ct_mark_mask = ntohll(ctx->lexer->token.mask.integer);
+        } else {
+            lexer_syntax_error(ctx->lexer, "expecting integer");
+            return;
+        }
+        lexer_get(ctx->lexer);
+    } else if (lexer_match_id(ctx->lexer, "ct_label")) {
+        if (!lexer_force_match(ctx->lexer, LEX_T_EQUALS)) {
+            return;
+        }
+        if (ctx->lexer->token.type == LEX_T_INTEGER) {
+            cc->ct_label = ctx->lexer->token.value.be128_int;
+            cc->ct_label_mask = OVS_BE128_MAX;
+        } else if (ctx->lexer->token.type == LEX_T_MASKED_INTEGER) {
+            cc->ct_label = ctx->lexer->token.value.be128_int;
+            cc->ct_label_mask = ctx->lexer->token.mask.be128_int;
+        } else {
+            lexer_syntax_error(ctx->lexer, "expecting integer");
+            return;
+        }
+        lexer_get(ctx->lexer);
+    } else {
+        lexer_syntax_error(ctx->lexer, NULL);
+    }
+}
+
+static void
+parse_CT_COMMIT_V1(struct action_context *ctx)
+{
+    add_prerequisite(ctx, "ip");
+
+    struct ovnact_ct_commit_v1 *ct_commit =
+        ovnact_put_CT_COMMIT_V1(ctx->ovnacts);
+    if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
+        while (!lexer_match(ctx->lexer, LEX_T_RPAREN)) {
+            parse_ct_commit_v1_arg(ctx, ct_commit);
+            if (ctx->lexer->error) {
+                return;
+            }
+            lexer_match(ctx->lexer, LEX_T_COMMA);
+        }
+    }
+}
+
 static void
 parse_CT_COMMIT(struct action_context *ctx)
 {
     if (ctx->lexer->token.type == LEX_T_LCURLY) {
-        parse_nested_action(ctx, OVNACT_CT_COMMIT, "ip",
+        parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",
                             WR_CT_COMMIT);
+    } else if (ctx->lexer->token.type == LEX_T_LPAREN) {
+        parse_CT_COMMIT_V1(ctx);
     } else {
         /* Add an empty nested action to allow for "ct_commit;" syntax */
         add_prerequisite(ctx, "ip");
-        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT,
+        struct ovnact_nest *on = ovnact_put(ctx->ovnacts, OVNACT_CT_COMMIT_V2,
                                             OVNACT_ALIGN(sizeof *on));
         on->nested_len = 0;
         on->nested = NULL;
@@ -644,7 +703,73 @@  parse_CT_COMMIT(struct action_context *ctx)
 }
 
 static void
-format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)
+format_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc, struct ds *s)
+{
+    ds_put_cstr(s, "ct_commit(");
+    if (cc->ct_mark_mask) {
+        ds_put_format(s, "ct_mark=%#"PRIx32, cc->ct_mark);
+        if (cc->ct_mark_mask != UINT32_MAX) {
+            ds_put_format(s, "/%#"PRIx32, cc->ct_mark_mask);
+        }
+    }
+    if (!ovs_be128_is_zero(cc->ct_label_mask)) {
+        if (ds_last(s) != '(') {
+            ds_put_cstr(s, ", ");
+        }
+
+        ds_put_format(s, "ct_label=");
+        ds_put_hex(s, &cc->ct_label, sizeof cc->ct_label);
+        if (!ovs_be128_equals(cc->ct_label_mask, OVS_BE128_MAX)) {
+            ds_put_char(s, '/');
+            ds_put_hex(s, &cc->ct_label_mask, sizeof cc->ct_label_mask);
+        }
+    }
+    if (!ds_chomp(s, '(')) {
+        ds_put_char(s, ')');
+    }
+    ds_put_char(s, ';');
+}
+
+static void
+encode_CT_COMMIT_V1(const struct ovnact_ct_commit_v1 *cc,
+                    const struct ovnact_encode_params *ep OVS_UNUSED,
+                    struct ofpbuf *ofpacts)
+{
+    struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
+    ct->flags = NX_CT_F_COMMIT;
+    ct->recirc_table = NX_CT_RECIRC_NONE;
+    ct->zone_src.field = mf_from_id(MFF_LOG_CT_ZONE);
+    ct->zone_src.ofs = 0;
+    ct->zone_src.n_bits = 16;
+
+    size_t set_field_offset = ofpacts->size;
+    ofpbuf_pull(ofpacts, set_field_offset);
+
+    if (cc->ct_mark_mask) {
+        const ovs_be32 value = htonl(cc->ct_mark);
+        const ovs_be32 mask = htonl(cc->ct_mark_mask);
+        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_MARK), &value, &mask);
+    }
+
+    if (!ovs_be128_is_zero(cc->ct_label_mask)) {
+        ofpact_put_set_field(ofpacts, mf_from_id(MFF_CT_LABEL), &cc->ct_label,
+                             &cc->ct_label_mask);
+    }
+
+    ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset);
+    ct = ofpacts->header;
+    ofpact_finish(ofpacts, &ct->ofpact);
+}
+
+static void
+ovnact_ct_commit_v1_free(struct ovnact_ct_commit_v1 *cc OVS_UNUSED)
+{
+}
+
+
+
+static void
+format_CT_COMMIT_V2(const struct ovnact_nest *on, struct ds *s)
 {
     if (on->nested_len) {
         format_nested_action(on, "ct_commit", s);
@@ -654,9 +779,9 @@  format_CT_COMMIT(const struct ovnact_nest *on, struct ds *s)
 }
 
 static void
-encode_CT_COMMIT(const struct ovnact_nest *on,
-                 const struct ovnact_encode_params *ep OVS_UNUSED,
-                 struct ofpbuf *ofpacts)
+encode_CT_COMMIT_V2(const struct ovnact_nest *on,
+                    const struct ovnact_encode_params *ep OVS_UNUSED,
+                    struct ofpbuf *ofpacts)
 {
     struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
     ct->flags = NX_CT_F_COMMIT;
diff --git a/tests/ovn.at b/tests/ovn.at
index 2253d0bde..e2568bb66 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -1102,6 +1102,50 @@  ct_commit { ct_label=0x181716151413121110090807060504030201; };
 ct_commit { ip4.dst = 192.168.0.1; };
     Field ip4.dst is not modifiable.
 
+# Legact ct_commit_v1 action.
+ct_commit();
+    formats as ct_commit;
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15])
+    has prereqs ip
+ct_commit(ct_mark=1);
+    formats as ct_commit(ct_mark=0x1);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
+    has prereqs ip
+ct_commit(ct_mark=1/1);
+    formats as ct_commit(ct_mark=0x1/0x1);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))
+    has prereqs ip
+ct_commit(ct_label=1);
+    formats as ct_commit(ct_label=0x1);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_label))
+    has prereqs ip
+ct_commit(ct_label=1/1);
+    formats as ct_commit(ct_label=0x1/0x1);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_label))
+    has prereqs ip
+ct_commit(ct_mark=1, ct_label=2);
+    formats as ct_commit(ct_mark=0x1, ct_label=0x2);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark,set_field:0x2->ct_label))
+    has prereqs ip
+
+ct_commit(ct_label=0x01020304050607080910111213141516);
+    formats as ct_commit(ct_label=0x1020304050607080910111213141516);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1020304050607080910111213141516->ct_label))
+    has prereqs ip
+ct_commit(ct_label=0x181716151413121110090807060504030201);
+    formats as ct_commit(ct_label=0x16151413121110090807060504030201);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x16151413121110090807060504030201->ct_label))
+    has prereqs ip
+ct_commit(ct_label=0x1000000000000000000000000000000/0x1000000000000000000000000000000);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1000000000000000000000000000000/0x1000000000000000000000000000000->ct_label))
+    has prereqs ip
+ct_commit(ct_label=18446744073709551615);
+    formats as ct_commit(ct_label=0xffffffffffffffff);
+    encodes as ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0xffffffffffffffff->ct_label))
+    has prereqs ip
+ct_commit(ct_label=18446744073709551616);
+    Decimal constants must be less than 2**64.
+
 ct_mark = 12345
     Field ct_mark is not modifiable.
 ct_label = 0xcafe
diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
index 5d92188ab..8eb7263b3 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -2336,7 +2336,8 @@  trace_actions(const struct ovnact *ovnacts, size_t ovnacts_len,
             execute_ct_next(ovnact_get_CT_NEXT(a), dp, uflow, pipeline, super);
             break;
 
-        case OVNACT_CT_COMMIT:
+        case OVNACT_CT_COMMIT_V1:
+        case OVNACT_CT_COMMIT_V2:
             /* Nothing to do. */
             break;