On Tue, Oct 25, 2022 at 4:39 PM Lorenzo Bianconi
<[email protected]> wrote:
>
> In the current codebase ct_commit {} action clears ct_state metadata of
> the incoming packet. This behaviour introduces an issue if we need to
> check the connection tracking state in the subsequent pipeline stages,
> e.g. for hairpin traffic:
>
> table=14(ls_in_pre_hairpin ), priority=100 , match=(ip && ct.trk),
> action=(reg0[6] = chk_lb_hairpin(); reg0[12] = chk_lb_hairpin_reply(); next;)
>
> Fix the issue introducing next_table option in the ct_commit {} action
> allowing the ct packet to proceed in the pipeline.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2103086
> Signed-off-by: Lorenzo Bianconi <[email protected]>
Hi Lorenzo,
Thanks for the patch. documentation is missing in ovn-northd.8.xml
for the updated logical flows.
Please see below for few comments.
> ---
> include/ovn/actions.h | 1 +
> lib/actions.c | 50 ++++++++++++++++++-----
> northd/northd.c | 8 ++--
> ovn-sb.xml | 4 +-
> tests/ovn-northd.at | 94 +++++++++++++++----------------------------
> tests/ovn.at | 4 ++
> 6 files changed, 86 insertions(+), 75 deletions(-)
>
> diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> index d7ee84dac..d96d34841 100644
> --- a/include/ovn/actions.h
> +++ b/include/ovn/actions.h
> @@ -315,6 +315,7 @@ struct ovnact_nest {
> struct ovnact ovnact;
> struct ovnact *nested;
> size_t nested_len;
> + uint8_t ltable; /* Logical table ID of next table. */
> };
>
> /* OVNACT_GET_ARP, OVNACT_GET_ND. */
> diff --git a/lib/actions.c b/lib/actions.c
> index adbb42db4..88d7eb571 100644
> --- a/lib/actions.c
> +++ b/lib/actions.c
> @@ -207,6 +207,10 @@ struct action_context {
>
> static void parse_actions(struct action_context *, enum lex_type sentinel);
>
> +static void __parse_nested_action(struct action_context *ctx,
> + enum ovnact_type type,
> + const char *prereq,
> + enum expr_write_scope scope);
> static void parse_nested_action(struct action_context *ctx,
> enum ovnact_type type,
> const char *prereq,
> @@ -764,8 +768,23 @@ 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);
> + int table = 0;
> + lexer_force_match(ctx->lexer, LEX_T_LCURLY); /* Skip '{'. */
> + if (lexer_match_id(ctx->lexer, "next_table")) {
> + lexer_match(ctx->lexer, LEX_T_SEMICOLON);
> + table = ctx->pp->cur_ltable + 1;
> + if (table >= ctx->pp->n_tables) {
> + table = 0;
> + }
> + }
Generally actions inside {} are nested actions. But this patch adds a
custom flag just before the start of the nested actions.
Also this would break the upgrades if ovn-northd is updated first.
Because the old ovn-controller will fail parsing this action.
@Mark Michelson @Dumitru Ceara @Han Zhou Is it ok for us to not
support this upgrade scenario ? i.e ovn-northd and DBs upgraded first
before the ovn-controllers ?
> + __parse_nested_action(ctx, OVNACT_CT_COMMIT_V2, "ip",
> + WR_CT_COMMIT);
> + if (ctx->lexer->error) {
> + return;
> + }
> +
> + struct ovnact_nest *on = ctx->ovnacts->header;
> + on->ltable = table;
> } else if (ctx->lexer->token.type == LEX_T_LPAREN) {
> parse_CT_COMMIT_V1(ctx);
> } else {
> @@ -775,6 +794,7 @@ parse_CT_COMMIT(struct action_context *ctx)
> OVNACT_ALIGN(sizeof *on));
> on->nested_len = 0;
> on->nested = NULL;
> + on->ltable = 0;
> }
> }
>
> @@ -872,12 +892,16 @@ format_CT_COMMIT_V2(const struct ovnact_nest *on,
> struct ds *s)
>
> static void
> encode_CT_COMMIT_V2(const struct ovnact_nest *on,
> - const struct ovnact_encode_params *ep OVS_UNUSED,
> + const struct ovnact_encode_params *ep,
> struct ofpbuf *ofpacts)
> {
> struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts);
> ct->flags = NX_CT_F_COMMIT;
> - ct->recirc_table = NX_CT_RECIRC_NONE;
> + if (on->ltable > 0) {
> + ct->recirc_table = first_ptable(ep, ep->pipeline) + on->ltable;
> + } else {
> + ct->recirc_table = NX_CT_RECIRC_NONE;
> + }
> ct->zone_src.field = ep->is_switch
> ? mf_from_id(MFF_LOG_CT_ZONE)
> : mf_from_id(MFF_LOG_DNAT_ZONE);
> @@ -1586,13 +1610,9 @@ encode_CT_CLEAR(const struct ovnact_null *null
> OVS_UNUSED,
> /* Implements the "arp", "nd_na", and "clone" actions, which execute nested
> * actions on a packet derived from the one being processed. */
> static void
> -parse_nested_action(struct action_context *ctx, enum ovnact_type type,
> - const char *prereq, enum expr_write_scope scope)
> +__parse_nested_action(struct action_context *ctx, enum ovnact_type type,
> + const char *prereq, enum expr_write_scope scope)
> {
> - if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) {
> - return;
> - }
> -
> if (ctx->depth + 1 == MAX_NESTED_ACTION_DEPTH) {
> lexer_error(ctx->lexer, "maximum depth of nested actions reached");
> return;
> @@ -1635,6 +1655,16 @@ parse_nested_action(struct action_context *ctx, enum
> ovnact_type type,
> on->nested = ofpbuf_steal_data(&nested);
> }
>
> +static void
> +parse_nested_action(struct action_context *ctx, enum ovnact_type type,
> + const char *prereq, enum expr_write_scope scope)
> +{
> + if (!lexer_force_match(ctx->lexer, LEX_T_LCURLY)) {
> + return;
> + }
> + __parse_nested_action(ctx, type, prereq, scope);
> +}
> +
> static void
> parse_ARP(struct action_context *ctx)
> {
> diff --git a/northd/northd.c b/northd/northd.c
> index 6771ccce5..7b31466ac 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -7048,8 +7048,9 @@ build_stateful(struct ovn_datapath *od,
> * We always set ct_mark.blocked to 0 here as
> * any packet that makes it this far is part of a connection we
> * want to allow to continue. */
> - ds_put_format(&actions, "ct_commit { %s = 0; "
> - "ct_label.label = " REG_LABEL "; }; next;",
> + ds_put_format(&actions,
> + "ct_commit { next_table; %s = 0; "
> + "ct_label.label = " REG_LABEL "; };",
> ct_block_action);
> ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
> REGBIT_CONNTRACK_COMMIT" == 1 && "
> @@ -7065,7 +7066,8 @@ build_stateful(struct ovn_datapath *od,
> * any packet that makes it this far is part of a connection we
> * want to allow to continue. */
> ds_clear(&actions);
> - ds_put_format(&actions, "ct_commit { %s = 0; }; next;", ct_block_action);
> + ds_put_format(&actions, "ct_commit { next_table; %s = 0; };",
> + ct_block_action);
> ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100,
> REGBIT_CONNTRACK_COMMIT" == 1 && "
> REGBIT_ACL_LABEL" == 0",
> diff --git a/ovn-sb.xml b/ovn-sb.xml
> index 315d60853..a4d3302f6 100644
> --- a/ovn-sb.xml
> +++ b/ovn-sb.xml
> @@ -1373,7 +1373,9 @@
> which will commit connection tracking state, and then drop the
> packet. This could be useful for setting <code>ct_mark</code>
> on a connection tracking entry before dropping a packet,
> - for example.
> + for example. In order to allow the packet committed to the ct
> table
> + to continue the processing in the next pipeline stage
> + <code>next_table</code> option can be used as first nested
> actions.
> </p>
> </dd>
>
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 7d879b642..c8bd9c20f 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -2961,21 +2961,13 @@ lsp1_inport=$(fetch_column Port_Binding tunnel_key
> logical_port=lsp1)
> # TCP packets should go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
Looks to me you need to enhance ovn-trace to handle this new flag ?
With this patch, there is no output to lsp2 now, which seems odd.
> ])
>
> # UDP packets should go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
> ])
>
> # Allow stateless for TCP.
> @@ -2993,11 +2985,7 @@ output("lsp2");
> # UDP packets still go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
> ])
>
> # Add a load balancer.
> @@ -3105,21 +3093,13 @@ flow_udp='udp && udp.dst == 80'
> # TCP packets should go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
> ])
>
> # UDP packets should go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
> ])
>
> # Allow stateless for TCP.
> @@ -3137,11 +3117,7 @@ output("lsp2");
> # UDP packets still go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_udp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
> ])
>
> # Add a load balancer.
> @@ -3245,11 +3221,7 @@ lsp1_inport=$(fetch_column Port_Binding tunnel_key
> logical_port=lsp1)
> # TCP packets should go to conntrack.
> flow="inport == \"lsp1\" && ${flow_eth} && ${flow_ip} && ${flow_tcp}"
> AT_CHECK_UNQUOTED([ovn_trace --ct new --ct new --minimal ls "${flow}"], [0],
> [dnl
> -ct_next(ct_state=new|trk) {
> - ct_next(ct_state=new|trk) {
> - output("lsp2");
> - };
> -};
> +ct_next(ct_state=new|trk);
> ])
>
> # Allow stateless with *lower* priority. It always beats stateful rules.
> @@ -4027,8 +3999,8 @@ check_stateful_flows() {
>
> AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed
> 's/table=../table=??/'], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
> @@ -4050,8 +4022,8 @@ check_stateful_flows() {
>
> AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
> }
>
> @@ -4091,8 +4063,8 @@ AT_CHECK([grep "ls_in_lb" sw0flows | sort | sed
> 's/table=../table=??/'], [0], [d
>
> AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed
> 's/table=../table=??/'], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CHECK([grep "ls_out_pre_lb" sw0flows | sort], [0], [dnl
> @@ -4111,8 +4083,8 @@ AT_CHECK([grep "ls_out_pre_stateful" sw0flows | sort],
> [0], [dnl
>
> AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CLEANUP
> @@ -4137,8 +4109,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 |
> sort | sed 's/table=./table
> ])
> AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed
> 's/table=../table=??/'], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> @@ -4147,8 +4119,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 |
> sort], [0], [dnl
> ])
> AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> # Add new ACL without label
> @@ -4166,8 +4138,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 |
> sort | sed 's/table=./table
> ])
> AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed
> 's/table=../table=??/'], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> @@ -4178,8 +4150,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 |
> sort], [0], [dnl
> ])
> AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> # Delete new ACL with label
> @@ -4195,8 +4167,8 @@ AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 2002 |
> sort | sed 's/table=./table
> ])
> AT_CHECK([grep "ls_in_stateful" sw0flows | sort | sed
> 's/table=../table=??/'], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 | sort], [0], [dnl
> @@ -4205,8 +4177,8 @@ AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 2002 |
> sort], [0], [dnl
> ])
> AT_CHECK([grep "ls_out_stateful" sw0flows | sort], [0], [dnl
> table=7 (ls_out_stateful ), priority=0 , match=(1), action=(next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=7 (ls_out_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
> AT_CLEANUP
> ])
> @@ -6544,8 +6516,8 @@ AT_CHECK([grep -e "ls_in_lb" lsflows | sed
> 's/table=../table=??/' | sort], [0],
>
> AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' |
> sort], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AS_BOX([Remove and add the ACLs back with the apply-after-lb option])
> @@ -6597,8 +6569,8 @@ AT_CHECK([grep -e "ls_in_lb" lsflows | sed
> 's/table=../table=??/' | sort], [0],
>
> AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' |
> sort], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AS_BOX([Remove and add the ACLs back with a few ACLs with apply-after-lb
> option])
> @@ -6650,8 +6622,8 @@ AT_CHECK([grep -e "ls_in_lb" lsflows | sed
> 's/table=../table=??/' | sort], [0],
>
> AT_CHECK([grep -e "ls_in_stateful" lsflows | sed 's/table=../table=??/' |
> sort], [0], [dnl
> table=??(ls_in_stateful ), priority=0 , match=(1), action=(next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { ct_mark.blocked = 0; }; next;)
> - table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { ct_mark.blocked = 0; ct_label.label =
> reg3; }; next;)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 0), action=(ct_commit { next_table; ct_mark.blocked = 0; };)
> + table=??(ls_in_stateful ), priority=100 , match=(reg0[[1]] == 1 &&
> reg0[[13]] == 1), action=(ct_commit { next_table; ct_mark.blocked = 0;
> ct_label.label = reg3; };)
> ])
>
> AT_CLEANUP
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f8b8db4df..b35f8981b 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -1187,6 +1187,10 @@ ct_commit { ct_mark=1; };
> formats as ct_commit { ct_mark = 1; };
> encodes as
> ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1->ct_mark))
> has prereqs ip
> +ct_commit { next_table; ct_mark=1; };
> + formats as ct_commit { ct_mark = 1; };
IMO the formatting of ct_commit{next_table, ...} should not lose the
"next_table" flag.
I think you should enhance format_CT_COMMIT_V2() to include this flag if set.
IMO it's better to add a new action to support this. Which means
ovn-northd should check if all the ovn-controllers support this before
using this new action.
I know there are some concerns about this from Han. So I'd wait for
his and other thoughts too before deciding on using a new action -
ct_commit_next{} or enhancing the existing ct_commit (which this patch
does).
Thanks
Numan
> + encodes as
> ct(commit,table=19,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 = 1/1; };
> encodes as
> ct(commit,zone=NXM_NX_REG13[0..15],exec(set_field:0x1/0x1->ct_mark))
> --
> 2.37.3
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
