| Message ID | 20240402153134.261811-1-dceara@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | Dumitru Ceara |
| Headers | show |
| Series | [ovs-dev] actions: Remove ct_commit_v1. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On Tue, Apr 2, 2024 at 5:31 PM Dumitru Ceara <dceara@redhat.com> wrote: > It's not used by any logical flow since v20.12.0. It was kept for > backwards compatibility reasons in 4799f73fd6ed ("Fix OVN update > issue when ovn-controller is updated first from 20.06 to 20.09.") but > now there's no supported release that uses it. > > Therefore the action is safe to be removed. Update the tests to reflect > the fact that the action is not supported anymore. > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > include/ovn/actions.h | 9 +-- > lib/actions.c | 141 ------------------------------------------ > tests/ovn.at | 41 ++++-------- > utilities/ovn-trace.c | 1 - > 4 files changed, 12 insertions(+), 180 deletions(-) > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index dcacbb1fff..8e794450cf 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -66,7 +66,7 @@ struct collector_set_ids; > OVNACT(EXCHANGE, ovnact_move) \ > OVNACT(DEC_TTL, ovnact_null) \ > OVNACT(CT_NEXT, ovnact_ct_next) \ > - OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ > + /* CT_COMMIT_V1 is not supported anymore. */ \ > OVNACT(CT_COMMIT_V2, ovnact_nest) \ > OVNACT(CT_DNAT, ovnact_ct_nat) \ > OVNACT(CT_SNAT, ovnact_ct_nat) \ > @@ -260,13 +260,6 @@ 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; > -}; > - > /* Type of NAT used for the particular action. > * UNSPEC translates to applying NAT that works for both directions. */ > enum ovnact_ct_nat_type { > diff --git a/lib/actions.c b/lib/actions.c > index 71615fc53c..7c20535717 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -729,71 +729,12 @@ 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_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"); > @@ -804,88 +745,6 @@ parse_CT_COMMIT(struct action_context *ctx) > } > } > > -static void > -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; > - > - /* If the datapath supports all-zero SNAT then use it to avoid tuple > - * collisions at commit time between NATed and firewalled-only > sessions. > - */ > - > - if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { > - size_t nat_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, nat_offset); > - > - struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); > - nat->flags = 0; > - nat->range_af = AF_UNSPEC; > - nat->flags |= NX_NAT_F_SRC; > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > - ct = ofpacts->header; > - } > - > - 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) > { > diff --git a/tests/ovn.at b/tests/ovn.at > index dd2ebce98e..f05437b9cd 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1318,47 +1318,28 @@ ct_commit { ip4.dst = 192.168.0.1; }; > > # Legact ct_commit_v1 action. > ct_commit(); > - formats as ct_commit; > - encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) > - has prereqs ip > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > > 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 > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > > 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 > + Syntax error at `(' expecting `;'. > 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 > + Syntax error at `(' expecting `;'. > ct_commit(ct_label=18446744073709551616); > - Decimal constants must be less than 2**64. > + Syntax error at `(' expecting `;'. > > ct_mark = 12345 > Field ct_mark is not modifiable. > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c > index e0f1c3ec9a..5e55fbbcc0 100644 > --- a/utilities/ovn-trace.c > +++ b/utilities/ovn-trace.c > @@ -3107,7 +3107,6 @@ 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_V1: > case OVNACT_CT_COMMIT_V2: > /* Nothing to do. */ > break; > -- > 2.44.0 > > Makes sense, thanks! Acked-by: Ales Musil <amusil@redhat.com>
LGTM, thanks for helping with this clean-up. On Wed, Apr 3, 2024 at 7:43 AM Ales Musil <amusil@redhat.com> wrote: > > > On Tue, Apr 2, 2024 at 5:31 PM Dumitru Ceara <dceara@redhat.com> wrote: > >> It's not used by any logical flow since v20.12.0. It was kept for >> backwards compatibility reasons in 4799f73fd6ed ("Fix OVN update >> issue when ovn-controller is updated first from 20.06 to 20.09.") but >> now there's no supported release that uses it. >> >> Therefore the action is safe to be removed. Update the tests to reflect >> the fact that the action is not supported anymore. >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> include/ovn/actions.h | 9 +-- >> lib/actions.c | 141 ------------------------------------------ >> tests/ovn.at | 41 ++++-------- >> utilities/ovn-trace.c | 1 - >> 4 files changed, 12 insertions(+), 180 deletions(-) >> >> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >> index dcacbb1fff..8e794450cf 100644 >> --- a/include/ovn/actions.h >> +++ b/include/ovn/actions.h >> @@ -66,7 +66,7 @@ struct collector_set_ids; >> OVNACT(EXCHANGE, ovnact_move) \ >> OVNACT(DEC_TTL, ovnact_null) \ >> OVNACT(CT_NEXT, ovnact_ct_next) \ >> - OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ >> + /* CT_COMMIT_V1 is not supported anymore. */ \ >> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >> OVNACT(CT_DNAT, ovnact_ct_nat) \ >> OVNACT(CT_SNAT, ovnact_ct_nat) \ >> @@ -260,13 +260,6 @@ 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; >> -}; >> - >> /* Type of NAT used for the particular action. >> * UNSPEC translates to applying NAT that works for both directions. */ >> enum ovnact_ct_nat_type { >> diff --git a/lib/actions.c b/lib/actions.c >> index 71615fc53c..7c20535717 100644 >> --- a/lib/actions.c >> +++ b/lib/actions.c >> @@ -729,71 +729,12 @@ 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_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"); >> @@ -804,88 +745,6 @@ parse_CT_COMMIT(struct action_context *ctx) >> } >> } >> >> -static void >> -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; >> - >> - /* If the datapath supports all-zero SNAT then use it to avoid tuple >> - * collisions at commit time between NATed and firewalled-only >> sessions. >> - */ >> - >> - if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { >> - size_t nat_offset = ofpacts->size; >> - ofpbuf_pull(ofpacts, nat_offset); >> - >> - struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); >> - nat->flags = 0; >> - nat->range_af = AF_UNSPEC; >> - nat->flags |= NX_NAT_F_SRC; >> - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); >> - ct = ofpacts->header; >> - } >> - >> - 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) >> { >> diff --git a/tests/ovn.at b/tests/ovn.at >> index dd2ebce98e..f05437b9cd 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -1318,47 +1318,28 @@ ct_commit { ip4.dst = 192.168.0.1; }; >> >> # Legact ct_commit_v1 action. >> ct_commit(); >> - formats as ct_commit; >> - encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >> - has prereqs ip >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> >> 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 >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> >> 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 >> + Syntax error at `(' expecting `;'. >> 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 >> + Syntax error at `(' expecting `;'. >> ct_commit(ct_label=18446744073709551616); >> - Decimal constants must be less than 2**64. >> + Syntax error at `(' expecting `;'. >> >> ct_mark = 12345 >> Field ct_mark is not modifiable. >> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >> index e0f1c3ec9a..5e55fbbcc0 100644 >> --- a/utilities/ovn-trace.c >> +++ b/utilities/ovn-trace.c >> @@ -3107,7 +3107,6 @@ 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_V1: >> case OVNACT_CT_COMMIT_V2: >> /* Nothing to do. */ >> break; >> -- >> 2.44.0 >> >> > Makes sense, thanks! > > Acked-by: Ales Musil <amusil@redhat.com> > > -- > > Ales Musil > > Senior Software Engineer - OVN Core > > Red Hat EMEA <https://www.redhat.com> > > amusil@redhat.com > <https://red.ht/sig> >
On 4/9/24 14:38, Martin Kalcok wrote: > LGTM, thanks for helping with this clean-up. > > On Wed, Apr 3, 2024 at 7:43 AM Ales Musil <amusil@redhat.com> wrote: > >> >> >> On Tue, Apr 2, 2024 at 5:31 PM Dumitru Ceara <dceara@redhat.com> wrote: >> >>> It's not used by any logical flow since v20.12.0. It was kept for >>> backwards compatibility reasons in 4799f73fd6ed ("Fix OVN update >>> issue when ovn-controller is updated first from 20.06 to 20.09.") but >>> now there's no supported release that uses it. >>> >>> Therefore the action is safe to be removed. Update the tests to reflect >>> the fact that the action is not supported anymore. >>> >>> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >>> --- >>> include/ovn/actions.h | 9 +-- >>> lib/actions.c | 141 ------------------------------------------ >>> tests/ovn.at | 41 ++++-------- >>> utilities/ovn-trace.c | 1 - >>> 4 files changed, 12 insertions(+), 180 deletions(-) >>> >>> diff --git a/include/ovn/actions.h b/include/ovn/actions.h >>> index dcacbb1fff..8e794450cf 100644 >>> --- a/include/ovn/actions.h >>> +++ b/include/ovn/actions.h >>> @@ -66,7 +66,7 @@ struct collector_set_ids; >>> OVNACT(EXCHANGE, ovnact_move) \ >>> OVNACT(DEC_TTL, ovnact_null) \ >>> OVNACT(CT_NEXT, ovnact_ct_next) \ >>> - OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ >>> + /* CT_COMMIT_V1 is not supported anymore. */ \ >>> OVNACT(CT_COMMIT_V2, ovnact_nest) \ >>> OVNACT(CT_DNAT, ovnact_ct_nat) \ >>> OVNACT(CT_SNAT, ovnact_ct_nat) \ >>> @@ -260,13 +260,6 @@ 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; >>> -}; >>> - >>> /* Type of NAT used for the particular action. >>> * UNSPEC translates to applying NAT that works for both directions. */ >>> enum ovnact_ct_nat_type { >>> diff --git a/lib/actions.c b/lib/actions.c >>> index 71615fc53c..7c20535717 100644 >>> --- a/lib/actions.c >>> +++ b/lib/actions.c >>> @@ -729,71 +729,12 @@ 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_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"); >>> @@ -804,88 +745,6 @@ parse_CT_COMMIT(struct action_context *ctx) >>> } >>> } >>> >>> -static void >>> -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; >>> - >>> - /* If the datapath supports all-zero SNAT then use it to avoid tuple >>> - * collisions at commit time between NATed and firewalled-only >>> sessions. >>> - */ >>> - >>> - if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { >>> - size_t nat_offset = ofpacts->size; >>> - ofpbuf_pull(ofpacts, nat_offset); >>> - >>> - struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); >>> - nat->flags = 0; >>> - nat->range_af = AF_UNSPEC; >>> - nat->flags |= NX_NAT_F_SRC; >>> - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); >>> - ct = ofpacts->header; >>> - } >>> - >>> - 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) >>> { >>> diff --git a/tests/ovn.at b/tests/ovn.at >>> index dd2ebce98e..f05437b9cd 100644 >>> --- a/tests/ovn.at >>> +++ b/tests/ovn.at >>> @@ -1318,47 +1318,28 @@ ct_commit { ip4.dst = 192.168.0.1; }; >>> >>> # Legact ct_commit_v1 action. >>> ct_commit(); >>> - formats as ct_commit; >>> - encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) >>> - has prereqs ip >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> >>> 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 >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> >>> 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 >>> + Syntax error at `(' expecting `;'. >>> 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 >>> + Syntax error at `(' expecting `;'. >>> ct_commit(ct_label=18446744073709551616); >>> - Decimal constants must be less than 2**64. >>> + Syntax error at `(' expecting `;'. >>> >>> ct_mark = 12345 >>> Field ct_mark is not modifiable. >>> diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c >>> index e0f1c3ec9a..5e55fbbcc0 100644 >>> --- a/utilities/ovn-trace.c >>> +++ b/utilities/ovn-trace.c >>> @@ -3107,7 +3107,6 @@ 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_V1: >>> case OVNACT_CT_COMMIT_V2: >>> /* Nothing to do. */ >>> break; >>> -- >>> 2.44.0 >>> >>> >> Makes sense, thanks! >> >> Acked-by: Ales Musil <amusil@redhat.com> >> Thanks, Ales and Martin! I applied this to main with the following minor change (documentation parts that I had forgotten to change initially): diff --git a/northd/northd.c b/northd/northd.c index 02cf5b2344..51181e08aa 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -6568,7 +6568,7 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, * that this connection is set for deletion. By not * specifying "next;", we implicitly drop the packet after * updating conntrack state. We would normally defer - * ct_commit() to the "stateful" stage, but since we're + * ct_commit to the "stateful" stage, but since we're * rejecting/dropping the packet, we go ahead and do it here. */ ds_truncate(match, match_tier_len); @@ -6883,7 +6883,7 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, * set on them. That's a connection that was disallowed, but is * now allowed by policy again since it hit this default-allow flow. * We need to set ct_mark.blocked=0 to let the connection continue, - * which will be done by ct_commit() in the "stateful" stage. + * which will be done by ct_commit in the "stateful" stage. * Subsequent packets will hit the flow at priority 0 that just * uses "next;". */ ds_clear(&match); diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml index 90ce0de3f9..b14a302852 100644 --- a/northd/ovn-northd.8.xml +++ b/northd/ovn-northd.8.xml @@ -724,7 +724,7 @@ </li> <li> <code>allow-related</code> ACLs translate into logical flows that set - the allow bit and additionally have <code>ct_commit(ct_label=0/1); + the allow bit and additionally have <code>ct_commit { ct_label=0/1; }; next;</code> actions for new connections and <code>reg0[1] = 1; next;</code> for existing connections. In case the <code>ACL</code> has a label then <code>reg3</code> is loaded with the label value and @@ -746,9 +746,9 @@ <li> Other ACLs set the drop bit and advance to the next table for new or untracked connections. For known connections, they set the drop bit, - as well as running the <code>ct_commit(ct_label=1/1);</code> action. - Setting <code>ct_label</code> marks a connection as one that was - previously allowed, but should no longer be allowed due to a policy + as well as running the <code>ct_commit { ct_label=1/1; };</code> + action. Setting <code>ct_label</code> marks a connection as one that + was previously allowed, but should no longer be allowed due to a policy change. </li> </ul> @@ -1218,10 +1218,11 @@ </li> <li> <code>allow-related</code> apply-after-lb ACLs translate into logical - flows that set the allow bit and run the <code>ct_commit(ct_label=0/1); - next;</code> actions for new connections and <code>reg0[1] = 1; - next;</code> for existing connections. In case the <code>ACL</code> - has a label then <code>reg3</code> is loaded with the label value and + flows that set the allow bit and run the + <code>ct_commit {ct_label=0/1; }; next;</code> actions for new + connections and <code>reg0[1] = 1; next;</code> for existing + connections. In case the <code>ACL</code> has a label then + <code>reg3</code> is loaded with the label value and <code>reg0[13]</code> bit is set to 1 (which acts as a hint for the next tables to commit the label to conntrack). </li> @@ -1240,7 +1241,7 @@ </li> <li> Other apply-after-lb ACLs set the drop bit for new or untracked - connections and <code>ct_commit(ct_label=1/1);</code> for known + connections and <code>ct_commit { ct_label=1/1; }</code> for known connections. Setting <code>ct_label</code> marks a connection as one that was previously allowed, but should no longer be allowed due to a policy change. --- Regards, Dumitru
diff --git a/include/ovn/actions.h b/include/ovn/actions.h index dcacbb1fff..8e794450cf 100644 --- a/include/ovn/actions.h +++ b/include/ovn/actions.h @@ -66,7 +66,7 @@ struct collector_set_ids; OVNACT(EXCHANGE, ovnact_move) \ OVNACT(DEC_TTL, ovnact_null) \ OVNACT(CT_NEXT, ovnact_ct_next) \ - OVNACT(CT_COMMIT_V1, ovnact_ct_commit_v1) \ + /* CT_COMMIT_V1 is not supported anymore. */ \ OVNACT(CT_COMMIT_V2, ovnact_nest) \ OVNACT(CT_DNAT, ovnact_ct_nat) \ OVNACT(CT_SNAT, ovnact_ct_nat) \ @@ -260,13 +260,6 @@ 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; -}; - /* Type of NAT used for the particular action. * UNSPEC translates to applying NAT that works for both directions. */ enum ovnact_ct_nat_type { diff --git a/lib/actions.c b/lib/actions.c index 71615fc53c..7c20535717 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -729,71 +729,12 @@ 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_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"); @@ -804,88 +745,6 @@ parse_CT_COMMIT(struct action_context *ctx) } } -static void -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; - - /* If the datapath supports all-zero SNAT then use it to avoid tuple - * collisions at commit time between NATed and firewalled-only sessions. - */ - - if (ovs_feature_is_supported(OVS_CT_ZERO_SNAT_SUPPORT)) { - size_t nat_offset = ofpacts->size; - ofpbuf_pull(ofpacts, nat_offset); - - struct ofpact_nat *nat = ofpact_put_NAT(ofpacts); - nat->flags = 0; - nat->range_af = AF_UNSPEC; - nat->flags |= NX_NAT_F_SRC; - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); - ct = ofpacts->header; - } - - 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) { diff --git a/tests/ovn.at b/tests/ovn.at index dd2ebce98e..f05437b9cd 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1318,47 +1318,28 @@ ct_commit { ip4.dst = 192.168.0.1; }; # Legact ct_commit_v1 action. ct_commit(); - formats as ct_commit; - encodes as ct(commit,zone=NXM_NX_REG13[[0..15]]) - has prereqs ip + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. 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 + Syntax error at `(' expecting `;'. ct_commit(ct_label=18446744073709551616); - Decimal constants must be less than 2**64. + Syntax error at `(' expecting `;'. ct_mark = 12345 Field ct_mark is not modifiable. diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c index e0f1c3ec9a..5e55fbbcc0 100644 --- a/utilities/ovn-trace.c +++ b/utilities/ovn-trace.c @@ -3107,7 +3107,6 @@ 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_V1: case OVNACT_CT_COMMIT_V2: /* Nothing to do. */ break;
It's not used by any logical flow since v20.12.0. It was kept for backwards compatibility reasons in 4799f73fd6ed ("Fix OVN update issue when ovn-controller is updated first from 20.06 to 20.09.") but now there's no supported release that uses it. Therefore the action is safe to be removed. Update the tests to reflect the fact that the action is not supported anymore. Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- include/ovn/actions.h | 9 +-- lib/actions.c | 141 ------------------------------------------ tests/ovn.at | 41 ++++-------- utilities/ovn-trace.c | 1 - 4 files changed, 12 insertions(+), 180 deletions(-)