On Mon, Mar 21, 2022 at 12:26 PM Numan Siddique <[email protected]> wrote:
>
> On Mon, Mar 21, 2022 at 12:21 PM Numan Siddique <[email protected]> wrote:
> >
> > On Sun, Mar 13, 2022 at 8:01 PM Han Zhou <[email protected]> wrote:
> > >
> > > Add a new action ct_lb_mark, which is the same as ct_lb except that it
> > > internally uses ct_mark to store the NAT flag, while ct_lb uses ct_label
> > > for the same purpose. This will be used later to move the masked access
> > > of ct_label to ct_mark while keeping the backward compatibility.
> > >
> > > Signed-off-by: Han Zhou <[email protected]>
> >
> > Acked-by: Numan Siddique <[email protected]>
>
> Looks like there is a system test failure in the CI run. Can you
> please check if its because of this patch or its a flaky one.
>
>
> 97: Load balancer health checks -- ovn-northd -- dp-groups=yes FAILED
> (ovs-macros.at:255)
> 98: Load balancer health checks -- ovn-northd -- dp-groups=no FAILED
> (ovs-macros.at:255)
This test case fails for me locally too with just the first 2 patches applied.
The failure is not seen when all the patches are applied.
Numan
>
> Numan
>
> >
> > Numan
> >
> > > ---
> > > include/ovn/actions.h | 3 ++-
> > > lib/actions.c | 55 ++++++++++++++++++++++++++++++++++++-------
> > > ovn-sb.xml | 10 ++++++++
> > > tests/ovn.at | 24 +++++++++++--------
> > > tests/system-ovn.at | 8 +++----
> > > utilities/ovn-trace.c | 8 ++++++-
> > > 6 files changed, 83 insertions(+), 25 deletions(-)
> > >
> > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h
> > > index 0641b927e..8dfb6fdc5 100644
> > > --- a/include/ovn/actions.h
> > > +++ b/include/ovn/actions.h
> > > @@ -69,6 +69,7 @@ struct ovn_extend_table;
> > > OVNACT(CT_DNAT_IN_CZONE, ovnact_ct_nat) \
> > > OVNACT(CT_SNAT_IN_CZONE, ovnact_ct_nat) \
> > > OVNACT(CT_LB, ovnact_ct_lb) \
> > > + OVNACT(CT_LB_MARK, ovnact_ct_lb) \
> > > OVNACT(SELECT, ovnact_select) \
> > > OVNACT(CT_CLEAR, ovnact_null) \
> > > OVNACT(CLONE, ovnact_nest) \
> > > @@ -273,7 +274,7 @@ struct ovnact_ct_lb_dst {
> > > uint16_t port;
> > > };
> > >
> > > -/* OVNACT_CT_LB. */
> > > +/* OVNACT_CT_LB/OVNACT_CT_LB_MARK. */
> > > struct ovnact_ct_lb {
> > > struct ovnact ovnact;
> > > struct ovnact_ct_lb_dst *dsts;
> > > diff --git a/lib/actions.c b/lib/actions.c
> > > index 5d3caaf2b..1c328f88d 100644
> > > --- a/lib/actions.c
> > > +++ b/lib/actions.c
> > > @@ -1079,7 +1079,7 @@ ovnact_ct_nat_free(struct ovnact_ct_nat *ct_nat
> > > OVS_UNUSED)
> > > }
> > >
> > > static void
> > > -parse_ct_lb_action(struct action_context *ctx)
> > > +parse_ct_lb_action(struct action_context *ctx, bool ct_lb_mark)
> > > {
> > > if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
> > > lexer_error(ctx->lexer, "\"ct_lb\" action not allowed in last
> > > table.");
> > > @@ -1185,7 +1185,8 @@ parse_ct_lb_action(struct action_context *ctx)
> > > }
> > > }
> > >
> > > - struct ovnact_ct_lb *cl = ovnact_put_CT_LB(ctx->ovnacts);
> > > + struct ovnact_ct_lb *cl = ct_lb_mark ?
> > > ovnact_put_CT_LB_MARK(ctx->ovnacts)
> > > + :
> > > ovnact_put_CT_LB(ctx->ovnacts);
> > > cl->ltable = ctx->pp->cur_ltable + 1;
> > > cl->dsts = dsts;
> > > cl->n_dsts = n_dsts;
> > > @@ -1193,9 +1194,13 @@ parse_ct_lb_action(struct action_context *ctx)
> > > }
> > >
> > > static void
> > > -format_CT_LB(const struct ovnact_ct_lb *cl, struct ds *s)
> > > +format_ct_lb(const struct ovnact_ct_lb *cl, struct ds *s, bool
> > > ct_lb_mark)
> > > {
> > > - ds_put_cstr(s, "ct_lb");
> > > + if (ct_lb_mark) {
> > > + ds_put_cstr(s, "ct_lb_mark");
> > > + } else {
> > > + ds_put_cstr(s, "ct_lb");
> > > + }
> > > if (cl->n_dsts) {
> > > ds_put_cstr(s, "(backends=");
> > > for (size_t i = 0; i < cl->n_dsts; i++) {
> > > @@ -1231,9 +1236,22 @@ format_CT_LB(const struct ovnact_ct_lb *cl, struct
> > > ds *s)
> > > }
> > >
> > > static void
> > > -encode_CT_LB(const struct ovnact_ct_lb *cl,
> > > +format_CT_LB(const struct ovnact_ct_lb *cl, struct ds *s)
> > > +{
> > > + format_ct_lb(cl, s, false);
> > > +}
> > > +
> > > +static void
> > > +format_CT_LB_MARK(const struct ovnact_ct_lb *cl, struct ds *s)
> > > +{
> > > + format_ct_lb(cl, s, true);
> > > +}
> > > +
> > > +static void
> > > +encode_ct_lb(const struct ovnact_ct_lb *cl,
> > > const struct ovnact_encode_params *ep,
> > > - struct ofpbuf *ofpacts)
> > > + struct ofpbuf *ofpacts,
> > > + bool ct_lb_mark)
> > > {
> > > uint8_t recirc_table = cl->ltable + first_ptable(ep, ep->pipeline);
> > > if (!cl->n_dsts) {
> > > @@ -1302,8 +1320,9 @@ encode_CT_LB(const struct ovnact_ct_lb *cl,
> > > ds_put_format(&ds, "),commit,table=%d,zone=NXM_NX_REG%d[0..15],"
> > > "exec(set_field:"
> > > OVN_CT_MASKED_STR(OVN_CT_NATTED)
> > > - "->ct_label))",
> > > - recirc_table, zone_reg);
> > > + "->%s))",
> > > + recirc_table, zone_reg,
> > > + ct_lb_mark ? "ct_mark" : "ct_label");
> > > }
> > >
> > > table_id = ovn_extend_table_assign_id(ep->group_table, ds_cstr(&ds),
> > > @@ -1318,6 +1337,22 @@ encode_CT_LB(const struct ovnact_ct_lb *cl,
> > > og->group_id = table_id;
> > > }
> > >
> > > +static void
> > > +encode_CT_LB(const struct ovnact_ct_lb *cl,
> > > + const struct ovnact_encode_params *ep,
> > > + struct ofpbuf *ofpacts)
> > > +{
> > > + encode_ct_lb(cl, ep, ofpacts, false);
> > > +}
> > > +
> > > +static void
> > > +encode_CT_LB_MARK(const struct ovnact_ct_lb *cl,
> > > + const struct ovnact_encode_params *ep,
> > > + struct ofpbuf *ofpacts)
> > > +{
> > > + encode_ct_lb(cl, ep, ofpacts, true);
> > > +}
> > > +
> > > static void
> > > ovnact_ct_lb_free(struct ovnact_ct_lb *ct_lb)
> > > {
> > > @@ -4219,7 +4254,9 @@ parse_action(struct action_context *ctx)
> > > } else if (lexer_match_id(ctx->lexer, "ct_snat_in_czone")) {
> > > parse_CT_SNAT_IN_CZONE(ctx);
> > > } else if (lexer_match_id(ctx->lexer, "ct_lb")) {
> > > - parse_ct_lb_action(ctx);
> > > + parse_ct_lb_action(ctx, false);
> > > + } else if (lexer_match_id(ctx->lexer, "ct_lb_mark")) {
> > > + parse_ct_lb_action(ctx, true);
> > > } else if (lexer_match_id(ctx->lexer, "ct_clear")) {
> > > ovnact_put_CT_CLEAR(ctx->ovnacts);
> > > } else if (lexer_match_id(ctx->lexer, "clone")) {
> > > diff --git a/ovn-sb.xml b/ovn-sb.xml
> > > index 3afea4ed4..aa9ee0ff5 100644
> > > --- a/ovn-sb.xml
> > > +++ b/ovn-sb.xml
> > > @@ -1988,6 +1988,16 @@
> > > </p>
> > > </dd>
> > >
> > > + <dt><code>ct_lb_mark;</code></dt>
> > > +
> > > <dt><code>ct_lb_mark(backends=<var>ip</var>[:<var>port</var>][,...][;
> > > hash_fields=<var>field1</var>,<var>field2</var>,...]);</code></dt>
> > > + <dd>
> > > + <p>
> > > + Same as <code>ct_lb</code>, except that it internally uses
> > > ct_mark
> > > + to store the NAT flag, while <code>ct_lb</code> uses
> > > ct_label for
> > > + the same purpose.
> > > + </p>
> > > + </dd>
> > > +
> > > <dt>
> > > <code><var>R</var> = dns_lookup();</code>
> > > </dt>
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 9b13a980d..691f74a43 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -1075,6 +1075,10 @@ ct_lb(backends=fd0f::2,fd0f::3;
> > > hash_fields="eth_src,eth_dst,ip_src,ip_dst,sctp_
> > > uses group: id(8),
> > > name(type=select,selection_method=hash,fields(eth_src,eth_dst,ip_src,ip_dst,sctp_src,sctp_dst),bucket=bucket_id=0,weight:100,actions=ct(nat(dst=fd0f::2),commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:2/2->ct_label)),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=fd0f::3),commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:2/2->ct_label)))
> > > has prereqs ip
> > >
> > > +ct_lb_mark(backends=192.168.1.2:80,192.168.1.3:80);
> > > + encodes as group:9
> > > + uses group: id(9),
> > > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=ct(nat(dst=192.168.1.2:80),commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:2/2->ct_mark)),bucket=bucket_id=1,weight:100,actions=ct(nat(dst=192.168.1.3:80),commit,table=19,zone=NXM_NX_REG13[0..15],exec(set_field:2/2->ct_mark)))
> > > + has prereqs ip
> > > # ct_next
> > > ct_next;
> > > encodes as ct(table=19,zone=NXM_NX_REG13[0..15])
> > > @@ -1827,13 +1831,13 @@ handle_svc_check(reg0);
> > > # select
> > > reg9[16..31] = select(1=50, 2=100, 3, );
> > > formats as reg9[16..31] = select(1=50, 2=100, 3=100);
> > > - encodes as group:9
> > > - uses group: id(9),
> > > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19))
> > > + encodes as group:10
> > > + uses group: id(10),
> > > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:50,actions=load:1->xreg4[16..31],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xreg4[16..31],resubmit(,19),bucket=bucket_id=2,weight:100,actions=load:3->xreg4[16..31],resubmit(,19))
> > >
> > > reg0 = select(1, 2);
> > > formats as reg0 = select(1=100, 2=100);
> > > - encodes as group:10
> > > - uses group: id(10),
> > > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19))
> > > + encodes as group:11
> > > + uses group: id(11),
> > > name(type=select,selection_method=dp_hash,bucket=bucket_id=0,weight:100,actions=load:1->xxreg0[96..127],resubmit(,19),bucket=bucket_id=1,weight:100,actions=load:2->xxreg0[96..127],resubmit(,19))
> > >
> > > reg0 = select(1=, 2);
> > > Syntax error at `,' expecting weight.
> > > @@ -1850,12 +1854,12 @@ reg0[0..14] = select(1, 2, 3);
> > >
> > > fwd_group(liveness=true, childports="eth0", "lsp1");
> > > formats as fwd_group(liveness="true", childports="eth0", "lsp1");
> > > - encodes as group:11
> > > - uses group: id(11),
> > > name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64))
> > > + encodes as group:12
> > > + uses group: id(12),
> > > name(type=select,selection_method=dp_hash,bucket=watch_port:5,load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=watch_port:17,load=0x17->NXM_NX_REG15[0..15],resubmit(,64))
> > >
> > > fwd_group(childports="eth0", "lsp1");
> > > - encodes as group:12
> > > - uses group: id(12),
> > > name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64))
> > > + encodes as group:13
> > > + uses group: id(13),
> > > name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64))
> > >
> > > fwd_group(childports=eth0);
> > > Syntax error at `eth0' expecting logical switch port.
> > > @@ -1864,8 +1868,8 @@ fwd_group();
> > > Syntax error at `)' expecting `;'.
> > >
> > > fwd_group(childports="eth0", "lsp1");
> > > - encodes as group:12
> > > - uses group: id(12),
> > > name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64))
> > > + encodes as group:13
> > > + uses group: id(13),
> > > name(type=select,selection_method=dp_hash,bucket=load=0x5->NXM_NX_REG15[0..15],resubmit(,64),bucket=load=0x17->NXM_NX_REG15[0..15],resubmit(,64))
> > >
> > > fwd_group(liveness=xyzzy, childports="eth0", "lsp1");
> > > Syntax error at `xyzzy' expecting true or false.
> > > diff --git a/tests/system-ovn.at b/tests/system-ovn.at
> > > index c4a2c39f6..b2d976b87 100644
> > > --- a/tests/system-ovn.at
> > > +++ b/tests/system-ovn.at
> > > @@ -4418,8 +4418,8 @@ OVS_WAIT_UNTIL([test 2 = `ovn-sbctl --bare
> > > --columns status find \
> > > service_monitor | sed '/^$/d' | grep online | wc -l`])
> > >
> > > OVS_WAIT_UNTIL(
> > > - [ovn-sbctl dump-flows sw0 | grep ct_lb | grep priority=120 | grep
> > > "ip4.dst == 10.0.0.10" > lflows.txt
> > > - test 1 = `cat lflows.txt | grep
> > > "ct_lb(backends=10.0.0.3:80,20.0.0.3:80)" | wc -l`]
> > > + [ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 |
> > > grep "ip4.dst == 10.0.0.10" > lflows.txt
> > > + test 1 = `cat lflows.txt | grep
> > > "ct_lb_mark(backends=10.0.0.3:80,20.0.0.3:80)" | wc -l`]
> > > )
> > >
> > > # From sw0-p2 send traffic to vip - 10.0.0.10
> > > @@ -4444,8 +4444,8 @@ OVS_WAIT_UNTIL([test 1 = `ovn-sbctl --bare
> > > --columns status find \
> > > service_monitor logical_port=sw0-p1 | sed '/^$/d' | grep offline | wc
> > > -l`])
> > >
> > > OVS_WAIT_UNTIL(
> > > - [ovn-sbctl dump-flows sw0 | grep ct_lb | grep priority=120 | grep
> > > "ip4.dst == 10.0.0.10" > lflows.txt
> > > - test 1 = `cat lflows.txt | grep "ct_lb(backends=20.0.0.3:80)" | wc
> > > -l`]
> > > + [ovn-sbctl dump-flows sw0 | grep ct_lb_mark | grep priority=120 |
> > > grep "ip4.dst == 10.0.0.10" > lflows.txt
> > > + test 1 = `cat lflows.txt | grep "ct_lb_mark(backends=20.0.0.3:80)"
> > > | wc -l`]
> > > )
> > >
> > > ovs-appctl dpctl/flush-conntrack
> > > diff --git a/utilities/ovn-trace.c b/utilities/ovn-trace.c
> > > index ece5803f2..d6ff75886 100644
> > > --- a/utilities/ovn-trace.c
> > > +++ b/utilities/ovn-trace.c
> > > @@ -2409,7 +2409,8 @@ execute_ct_lb(const struct ovnact_ct_lb *ct_lb,
> > > }
> > >
> > > struct ovntrace_node *node = ovntrace_node_append(
> > > - super, OVNTRACE_NODE_TRANSFORMATION, "ct_lb%s",
> > > + super, OVNTRACE_NODE_TRANSFORMATION, "%s%s",
> > > + ct_lb->ovnact.type == OVNACT_CT_LB_MARK ? "ct_lb_mark" : "ct_lb",
> > > ds_cstr_ro(&comment));
> > > ds_destroy(&comment);
> > > trace__(dp, &ct_lb_flow, ct_lb->ltable, pipeline, &node->subs);
> > > @@ -2634,6 +2635,11 @@ trace_actions(const struct ovnact *ovnacts, size_t
> > > ovnacts_len,
> > > execute_ct_lb(ovnact_get_CT_LB(a), dp, uflow, pipeline,
> > > super);
> > > break;
> > >
> > > + case OVNACT_CT_LB_MARK:
> > > + execute_ct_lb(ovnact_get_CT_LB_MARK(a), dp, uflow, pipeline,
> > > + super);
> > > + break;
> > > +
> > > case OVNACT_SELECT:
> > > execute_select(ovnact_get_SELECT(a), dp, uflow,
> > > pipeline, super);
> > > --
> > > 2.30.2
> > >
> > > _______________________________________________
> > > 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
