From 224b2ba1c53279f33eafe76ac46ffdf347f8a077 Mon Sep 17 00:00:00 2001
From: Numan Siddique <numans@ovn.org>
Date: Fri, 20 Nov 2020 16:22:53 +0530
Subject: [PATCH] Fix OVN update issue when ovn-controller is updated first
from 20.06 to 20.09.
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>
Acked-by: Flavio Fernandes <flavio@flaviof.com>
---
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 7ba24cd60..9c1ebf4aa 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -61,7 +61,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) \
@@ -230,6 +231,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 3219ab3be..4a9813218 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 f56f8a696..396e60eeb 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 cc1cd1b16..8421c0682 100644
--- a/utilities/ovn-trace.c
+++ b/utilities/ovn-trace.c
@@ -2334,7 +2334,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