| Message ID | 20240719133721.1043327-1-martin.kalcok@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [ovs-dev,v2] actions: Explicitly finish CT actions. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
Regarding the failed CI run, I suspect that there might be some test instability. All tests passed successfully in my branch [0] for this patch. Martin. [0] https://github.com/mkalcok/ovn/actions/runs/10009002777 On Fri, 2024-07-19 at 15:37 +0200, Martin Kalcok wrote: > As discussed here [0], a couple of functions that encode > CT-related actions were using older, manual, way of finishing > the action. > > As amusil mentioned here [1], there's a shorter and more explicit > way of doing it. This change replaces manual way with the more > explicit aproach. > > [0] > https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html > [1] > https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html > > Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> > --- > Thank you for the review, Ales. I added places that I missed > originally. > For the unit tests, I copied your suggestion for 'ct_lb' action, and > for the rest of the actions, I just pre-pended them with `ct_clear`. > I hope > that's enough. > > Martin. > > controller/lflow.c | 11 +++++----- > lib/actions.c | 52 +++++++++++++++----------------------------- > -- > tests/ovn.at | 22 ++++++++++++++++++++ > 3 files changed, 44 insertions(+), 41 deletions(-) > > diff --git a/controller/lflow.c b/controller/lflow.c > index b4c379044..aa77ed631 100644 > --- a/controller/lflow.c > +++ b/controller/lflow.c > @@ -1795,6 +1795,7 @@ add_lb_ct_snat_hairpin_vip_flow(const struct > ovn_controller_lb *lb, > { > uint64_t stub[1024 / 8]; > struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); > + const size_t ct_offset = ofpacts.size; > > uint8_t address_family; > if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > @@ -1811,10 +1812,6 @@ add_lb_ct_snat_hairpin_vip_flow(const struct > ovn_controller_lb *lb, > ct->flags = NX_CT_F_COMMIT; > ct->alg = 0; > > - size_t nat_offset; > - nat_offset = ofpacts.size; > - ofpbuf_pull(&ofpacts, nat_offset); > - > struct ofpact_nat *nat = ofpact_put_NAT(&ofpacts); > nat->flags = NX_NAT_F_SRC; > nat->range_af = address_family; > @@ -1828,8 +1825,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct > ovn_controller_lb *lb, > ? lb- > >hairpin_snat_ips.ipv6_addrs[0].addr > : lb_vip->vip; > } > - ofpacts.header = ofpbuf_push_uninit(&ofpacts, nat_offset); > - ofpact_finish(&ofpacts, &ct->ofpact); > + > + ct = ofpbuf_at_assert(&ofpacts, ct_offset, sizeof *ct); > + ofpacts.header = ct; > + ofpact_finish_CT(&ofpacts, &ct); > > struct match match = MATCH_CATCHALL_INITIALIZER; > > diff --git a/lib/actions.c b/lib/actions.c > index e8cc0994d..9d19dd2dc 100644 > --- a/lib/actions.c > +++ b/lib/actions.c > @@ -715,13 +715,18 @@ encode_CT_NEXT(const struct ovnact_ct_next > *ct_next, > const struct ovnact_encode_params *ep, > struct ofpbuf *ofpacts) > { > + size_t ct_offset = ofpacts->size; > + > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next- > >ltable; > ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) > : mf_from_id(MFF_LOG_DNAT_ZONE); > ct->zone_src.ofs = 0; > ct->zone_src.n_bits = 16; > - ofpact_finish(ofpacts, &ct->ofpact); > + > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > } > > static void > @@ -761,7 +766,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > struct ofpbuf *ofpacts) > { > size_t ct_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, ct_offset); > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->flags = NX_CT_F_COMMIT; > @@ -776,25 +780,17 @@ encode_CT_COMMIT_V2(const struct ovnact_nest > *on, > * 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); > - > ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); > - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > - ct = ofpacts->header; > - ofpact_finish(ofpacts, &ct->ofpact); > - ofpbuf_push_uninit(ofpacts, ct_offset); > + > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > } > > static void > @@ -1027,20 +1023,16 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > enum mf_field_id zone_src, struct ofpbuf *ofpacts) > { > const size_t ct_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, ct_offset); > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); > ct->zone_src.field = mf_from_id(zone_src); > ct->zone_src.ofs = 0; > ct->zone_src.n_bits = 16; > - ct->flags = 0; > + ct->flags = cn->commit ? NX_CT_F_COMMIT : 0; > ct->alg = 0; > > struct ofpact_nat *nat; > - size_t nat_offset; > - nat_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, nat_offset); > > nat = ofpact_put_NAT(ofpacts); > nat->range_af = cn->family; > @@ -1081,13 +1073,9 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > } > } > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > - ct = ofpacts->header; > - if (cn->commit) { > - ct->flags |= NX_CT_F_COMMIT; > - } > - ofpact_finish(ofpacts, &ct->ofpact); > - ofpbuf_push_uninit(ofpacts, ct_offset); > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > } > > static void > @@ -1383,11 +1371,9 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > /* ct_lb without any destinations means that this is an > established > * connection and we just need to do a NAT. */ > const size_t ct_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, ct_offset); > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > struct ofpact_nat *nat; > - size_t nat_offset; > ct->zone_src.field = ep->is_switch ? > mf_from_id(MFF_LOG_CT_ZONE) > : mf_from_id(MFF_LOG_DNAT_ZONE); > ct->zone_src.ofs = 0; > @@ -1396,17 +1382,13 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > ct->recirc_table = recirc_table; > ct->alg = 0; > > - nat_offset = ofpacts->size; > - ofpbuf_pull(ofpacts, nat_offset); > - > nat = ofpact_put_NAT(ofpacts); > nat->flags = 0; > nat->range_af = AF_UNSPEC; > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > - ct = ofpacts->header; > - ofpact_finish(ofpacts, &ct->ofpact); > - ofpbuf_push_uninit(ofpacts, ct_offset); > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > + ofpacts->header = ct; > + ofpact_finish_CT(ofpacts, &ct); > return; > } > > diff --git a/tests/ovn.at b/tests/ovn.at > index 185ba4a21..adeff95a9 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -1169,6 +1169,9 @@ ct_lb(); > formats as ct_lb; > encodes as > ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat) > has prereqs ip > +ct_clear; ct_lb; reg8[[7]] = 1; > + encodes as > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat),set_ > field:0x8000000000/0x8000000000->xreg4 > + has prereqs ip > ct_lb(192.168.1.2:80, 192.168.1.3:80); > Syntax error at `192.168.1.2' expecting backends. > ct_lb(backends=192.168.1.2:80,192.168.1.3:80); > @@ -1263,6 +1266,9 @@ > ct_lb_mark(backends=192.168.1.2:80,192.168.1.3:80; > hash_fields="eth_src,eth_dst, > ct_next; > encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > has prereqs ip > +ct_clear; ct_next; > + encodes as > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > + has prereqs ip > > # ct_commit > ct_commit; > @@ -1318,6 +1324,10 @@ ct_commit { ip4.dst = 192.168.0.1; }; > reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > encodes as set_field:0x2000000000000/0x2000000000000- > >xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1- > >ct_mark)) > has prereqs ip > +ct_clear; ct_commit { }; next; > + formats as ct_clear; ct_commit; next; > + encodes as > ct_clear,ct(commit,zone=NXM_NX_REG13[[0..15]]),resubmit(,oflow_in_tab > le) > + has prereqs ip > > # ct_commit_to_zone > ct_commit_to_zone(dnat); > @@ -1381,6 +1391,9 @@ ct_dnat(192.168.1.2, 1-3000); > formats as ct_dnat(192.168.1.2,1-3000); > encodes as > ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(dst=192 > .168.1.2:1-3000,random)) > has prereqs ip > +ct_clear; ct_dnat; > + encodes as > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) > + has prereqs ip > > ct_dnat(192.168.1.2, 192.168.1.3); > Syntax error at `192.168.1.3' expecting Integer for port range. > @@ -1415,6 +1428,9 @@ ct_dnat_in_czone(192.168.1.2, 1-3000); > formats as ct_dnat_in_czone(192.168.1.2,1-3000); > encodes as > ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(dst=192 > .168.1.2:1-3000,random)) > has prereqs ip > +ct_clear; ct_dnat_in_czone; > + encodes as > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) > + has prereqs ip > > ct_dnat_in_czone(192.168.1.2, 192.168.1.3); > Syntax error at `192.168.1.3' expecting Integer for port range. > @@ -1449,6 +1465,9 @@ ct_snat(192.168.1.2, 1-3000); > formats as ct_snat(192.168.1.2,1-3000); > encodes as > ct(commit,table=oflow_in_table,zone=NXM_NX_REG12[[0..15]],nat(src=192 > .168.1.2:1-3000,random)) > has prereqs ip > +ct_clear; ct_snat; > + encodes as > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG12[[0..15]],nat) > + has prereqs ip > > ct_snat(192.168.1.2, 192.168.1.3); > Syntax error at `192.168.1.3' expecting Integer for port range. > @@ -1483,6 +1502,9 @@ ct_snat_in_czone(192.168.1.2, 1-3000); > formats as ct_snat_in_czone(192.168.1.2,1-3000); > encodes as > ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(src=192 > .168.1.2:1-3000,random)) > has prereqs ip > +ct_clear; ct_snat_in_czone; > + encodes as > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) > + has prereqs ip > > ct_snat_in_czone(192.168.1.2, 192.168.1.3); > Syntax error at `192.168.1.3' expecting Integer for port range.
On Mon, Jul 22, 2024 at 11:05 AM <martin.kalcok@canonical.com> wrote: > Regarding the failed CI run, I suspect that there might be some test > instability. All tests passed successfully in my branch [0] for this > patch. > > Martin. > > [0] https://github.com/mkalcok/ovn/actions/runs/10009002777 Hi Martin, the incremental processing test is a bit flaky as your patch has nothing to do with that area, it should be fine. > > On Fri, 2024-07-19 at 15:37 +0200, Martin Kalcok wrote: > > As discussed here [0], a couple of functions that encode > > CT-related actions were using older, manual, way of finishing > > the action. > > > > As amusil mentioned here [1], there's a shorter and more explicit > > way of doing it. This change replaces manual way with the more > > explicit aproach. > > > > [0] > > https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html > > [1] > > https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html > > > > Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> > > --- > > Thank you for the review, Ales. I added places that I missed > > originally. > > For the unit tests, I copied your suggestion for 'ct_lb' action, and > > for the rest of the actions, I just pre-pended them with `ct_clear`. > > I hope > > that's enough. > > > > Martin. > > > > controller/lflow.c | 11 +++++----- > > lib/actions.c | 52 +++++++++++++++----------------------------- > > -- > > tests/ovn.at | 22 ++++++++++++++++++++ > > 3 files changed, 44 insertions(+), 41 deletions(-) > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > index b4c379044..aa77ed631 100644 > > --- a/controller/lflow.c > > +++ b/controller/lflow.c > > @@ -1795,6 +1795,7 @@ add_lb_ct_snat_hairpin_vip_flow(const struct > > ovn_controller_lb *lb, > > { > > uint64_t stub[1024 / 8]; > > struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); > > + const size_t ct_offset = ofpacts.size; > > > > uint8_t address_family; > > if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > > @@ -1811,10 +1812,6 @@ add_lb_ct_snat_hairpin_vip_flow(const struct > > ovn_controller_lb *lb, > > ct->flags = NX_CT_F_COMMIT; > > ct->alg = 0; > > > > - size_t nat_offset; > > - nat_offset = ofpacts.size; > > - ofpbuf_pull(&ofpacts, nat_offset); > > - > > struct ofpact_nat *nat = ofpact_put_NAT(&ofpacts); > > nat->flags = NX_NAT_F_SRC; > > nat->range_af = address_family; > > @@ -1828,8 +1825,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct > > ovn_controller_lb *lb, > > ? lb- > > >hairpin_snat_ips.ipv6_addrs[0].addr > > : lb_vip->vip; > > } > > - ofpacts.header = ofpbuf_push_uninit(&ofpacts, nat_offset); > > - ofpact_finish(&ofpacts, &ct->ofpact); > > + > > + ct = ofpbuf_at_assert(&ofpacts, ct_offset, sizeof *ct); > > + ofpacts.header = ct; > > + ofpact_finish_CT(&ofpacts, &ct); > > > > struct match match = MATCH_CATCHALL_INITIALIZER; > > > > diff --git a/lib/actions.c b/lib/actions.c > > index e8cc0994d..9d19dd2dc 100644 > > --- a/lib/actions.c > > +++ b/lib/actions.c > > @@ -715,13 +715,18 @@ encode_CT_NEXT(const struct ovnact_ct_next > > *ct_next, > > const struct ovnact_encode_params *ep, > > struct ofpbuf *ofpacts) > > { > > + size_t ct_offset = ofpacts->size; > > + > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > > ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next- > > >ltable; > > ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) > > : mf_from_id(MFF_LOG_DNAT_ZONE); > > ct->zone_src.ofs = 0; > > ct->zone_src.n_bits = 16; > > - ofpact_finish(ofpacts, &ct->ofpact); > > + > > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > > + ofpacts->header = ct; > > + ofpact_finish_CT(ofpacts, &ct); > > } > > > > static void > > @@ -761,7 +766,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, > > struct ofpbuf *ofpacts) > > { > > size_t ct_offset = ofpacts->size; > > - ofpbuf_pull(ofpacts, ct_offset); > > > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > > ct->flags = NX_CT_F_COMMIT; > > @@ -776,25 +780,17 @@ encode_CT_COMMIT_V2(const struct ovnact_nest > > *on, > > * 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); > > - > > ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); > > - ct = ofpacts->header; > > - ofpact_finish(ofpacts, &ct->ofpact); > > - ofpbuf_push_uninit(ofpacts, ct_offset); > > + > > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > > + ofpacts->header = ct; > > + ofpact_finish_CT(ofpacts, &ct); > > } > > > > static void > > @@ -1027,20 +1023,16 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > > enum mf_field_id zone_src, struct ofpbuf *ofpacts) > > { > > const size_t ct_offset = ofpacts->size; > > - ofpbuf_pull(ofpacts, ct_offset); > > > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > > ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); > > ct->zone_src.field = mf_from_id(zone_src); > > ct->zone_src.ofs = 0; > > ct->zone_src.n_bits = 16; > > - ct->flags = 0; > > + ct->flags = cn->commit ? NX_CT_F_COMMIT : 0; > > ct->alg = 0; > > > > struct ofpact_nat *nat; > > - size_t nat_offset; > > - nat_offset = ofpacts->size; > > - ofpbuf_pull(ofpacts, nat_offset); > > > > nat = ofpact_put_NAT(ofpacts); > > nat->range_af = cn->family; > > @@ -1081,13 +1073,9 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > > } > > } > > > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > > - ct = ofpacts->header; > > - if (cn->commit) { > > - ct->flags |= NX_CT_F_COMMIT; > > - } > > - ofpact_finish(ofpacts, &ct->ofpact); > > - ofpbuf_push_uninit(ofpacts, ct_offset); > > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > > + ofpacts->header = ct; > > + ofpact_finish_CT(ofpacts, &ct); > > } > > > > static void > > @@ -1383,11 +1371,9 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > > /* ct_lb without any destinations means that this is an > > established > > * connection and we just need to do a NAT. */ > > const size_t ct_offset = ofpacts->size; > > - ofpbuf_pull(ofpacts, ct_offset); > > > > struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); > > struct ofpact_nat *nat; > > - size_t nat_offset; > > ct->zone_src.field = ep->is_switch ? > > mf_from_id(MFF_LOG_CT_ZONE) > > : mf_from_id(MFF_LOG_DNAT_ZONE); > > ct->zone_src.ofs = 0; > > @@ -1396,17 +1382,13 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, > > ct->recirc_table = recirc_table; > > ct->alg = 0; > > > > - nat_offset = ofpacts->size; > > - ofpbuf_pull(ofpacts, nat_offset); > > - > > nat = ofpact_put_NAT(ofpacts); > > nat->flags = 0; > > nat->range_af = AF_UNSPEC; > > > > - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); > > - ct = ofpacts->header; > > - ofpact_finish(ofpacts, &ct->ofpact); > > - ofpbuf_push_uninit(ofpacts, ct_offset); > > + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); > > + ofpacts->header = ct; > > + ofpact_finish_CT(ofpacts, &ct); > > return; > > } > > > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 185ba4a21..adeff95a9 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -1169,6 +1169,9 @@ ct_lb(); > > formats as ct_lb; > > encodes as > > ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat) > > has prereqs ip > > +ct_clear; ct_lb; reg8[[7]] = 1; > > + encodes as > > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat),set_ > > field:0x8000000000/0x8000000000->xreg4 > > + has prereqs ip > > ct_lb(192.168.1.2:80, 192.168.1.3:80); > > Syntax error at `192.168.1.2' expecting backends. > > ct_lb(backends=192.168.1.2:80,192.168.1.3:80); > > @@ -1263,6 +1266,9 @@ > > ct_lb_mark(backends=192.168.1.2:80,192.168.1.3:80; > > hash_fields="eth_src,eth_dst, > > ct_next; > > encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > > has prereqs ip > > +ct_clear; ct_next; > > + encodes as > > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) > > + has prereqs ip > > > > # ct_commit > > ct_commit; > > @@ -1318,6 +1324,10 @@ ct_commit { ip4.dst = 192.168.0.1; }; > > reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; > > encodes as set_field:0x2000000000000/0x2000000000000- > > >xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1- > > >ct_mark)) > > has prereqs ip > > +ct_clear; ct_commit { }; next; > > + formats as ct_clear; ct_commit; next; > > + encodes as > > ct_clear,ct(commit,zone=NXM_NX_REG13[[0..15]]),resubmit(,oflow_in_tab > > le) > > + has prereqs ip > > > > # ct_commit_to_zone > > ct_commit_to_zone(dnat); > > @@ -1381,6 +1391,9 @@ ct_dnat(192.168.1.2, 1-3000); > > formats as ct_dnat(192.168.1.2,1-3000); > > encodes as > > ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(dst=192 > > .168.1.2:1-3000,random)) > > has prereqs ip > > +ct_clear; ct_dnat; > > + encodes as > > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) > > + has prereqs ip > > > > ct_dnat(192.168.1.2, 192.168.1.3); > > Syntax error at `192.168.1.3' expecting Integer for port range. > > @@ -1415,6 +1428,9 @@ ct_dnat_in_czone(192.168.1.2, 1-3000); > > formats as ct_dnat_in_czone(192.168.1.2,1-3000); > > encodes as > > ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(dst=192 > > .168.1.2:1-3000,random)) > > has prereqs ip > > +ct_clear; ct_dnat_in_czone; > > + encodes as > > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) > > + has prereqs ip > > > > ct_dnat_in_czone(192.168.1.2, 192.168.1.3); > > Syntax error at `192.168.1.3' expecting Integer for port range. > > @@ -1449,6 +1465,9 @@ ct_snat(192.168.1.2, 1-3000); > > formats as ct_snat(192.168.1.2,1-3000); > > encodes as > > ct(commit,table=oflow_in_table,zone=NXM_NX_REG12[[0..15]],nat(src=192 > > .168.1.2:1-3000,random)) > > has prereqs ip > > +ct_clear; ct_snat; > > + encodes as > > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG12[[0..15]],nat) > > + has prereqs ip > > > > ct_snat(192.168.1.2, 192.168.1.3); > > Syntax error at `192.168.1.3' expecting Integer for port range. > > @@ -1483,6 +1502,9 @@ ct_snat_in_czone(192.168.1.2, 1-3000); > > formats as ct_snat_in_czone(192.168.1.2,1-3000); > > encodes as > > ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(src=192 > > .168.1.2:1-3000,random)) > > has prereqs ip > > +ct_clear; ct_snat_in_czone; > > + encodes as > > ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) > > + has prereqs ip > > > > ct_snat_in_czone(192.168.1.2, 192.168.1.3); > > Syntax error at `192.168.1.3' expecting Integer for port range. > > The patch looks good to me, thanks. Acked-by: Ales Musil <amusil@redhat.com>
diff --git a/controller/lflow.c b/controller/lflow.c index b4c379044..aa77ed631 100644 --- a/controller/lflow.c +++ b/controller/lflow.c @@ -1795,6 +1795,7 @@ add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb, { uint64_t stub[1024 / 8]; struct ofpbuf ofpacts = OFPBUF_STUB_INITIALIZER(stub); + const size_t ct_offset = ofpacts.size; uint8_t address_family; if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { @@ -1811,10 +1812,6 @@ add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb, ct->flags = NX_CT_F_COMMIT; ct->alg = 0; - size_t nat_offset; - nat_offset = ofpacts.size; - ofpbuf_pull(&ofpacts, nat_offset); - struct ofpact_nat *nat = ofpact_put_NAT(&ofpacts); nat->flags = NX_NAT_F_SRC; nat->range_af = address_family; @@ -1828,8 +1825,10 @@ add_lb_ct_snat_hairpin_vip_flow(const struct ovn_controller_lb *lb, ? lb->hairpin_snat_ips.ipv6_addrs[0].addr : lb_vip->vip; } - ofpacts.header = ofpbuf_push_uninit(&ofpacts, nat_offset); - ofpact_finish(&ofpacts, &ct->ofpact); + + ct = ofpbuf_at_assert(&ofpacts, ct_offset, sizeof *ct); + ofpacts.header = ct; + ofpact_finish_CT(&ofpacts, &ct); struct match match = MATCH_CATCHALL_INITIALIZER; diff --git a/lib/actions.c b/lib/actions.c index e8cc0994d..9d19dd2dc 100644 --- a/lib/actions.c +++ b/lib/actions.c @@ -715,13 +715,18 @@ encode_CT_NEXT(const struct ovnact_ct_next *ct_next, const struct ovnact_encode_params *ep, struct ofpbuf *ofpacts) { + size_t ct_offset = ofpacts->size; + struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->recirc_table = first_ptable(ep, ep->pipeline) + ct_next->ltable; ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) : mf_from_id(MFF_LOG_DNAT_ZONE); ct->zone_src.ofs = 0; ct->zone_src.n_bits = 16; - ofpact_finish(ofpacts, &ct->ofpact); + + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); } static void @@ -761,7 +766,6 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, struct ofpbuf *ofpacts) { size_t ct_offset = ofpacts->size; - ofpbuf_pull(ofpacts, ct_offset); struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->flags = NX_CT_F_COMMIT; @@ -776,25 +780,17 @@ encode_CT_COMMIT_V2(const struct ovnact_nest *on, * 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); - ovnacts_encode(on->nested, on->nested_len, ep, ofpacts); - ofpacts->header = ofpbuf_push_uninit(ofpacts, set_field_offset); - ct = ofpacts->header; - ofpact_finish(ofpacts, &ct->ofpact); - ofpbuf_push_uninit(ofpacts, ct_offset); + + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); } static void @@ -1027,20 +1023,16 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, enum mf_field_id zone_src, struct ofpbuf *ofpacts) { const size_t ct_offset = ofpacts->size; - ofpbuf_pull(ofpacts, ct_offset); struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); ct->recirc_table = cn->ltable + first_ptable(ep, ep->pipeline); ct->zone_src.field = mf_from_id(zone_src); ct->zone_src.ofs = 0; ct->zone_src.n_bits = 16; - ct->flags = 0; + ct->flags = cn->commit ? NX_CT_F_COMMIT : 0; ct->alg = 0; struct ofpact_nat *nat; - size_t nat_offset; - nat_offset = ofpacts->size; - ofpbuf_pull(ofpacts, nat_offset); nat = ofpact_put_NAT(ofpacts); nat->range_af = cn->family; @@ -1081,13 +1073,9 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, } } - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); - ct = ofpacts->header; - if (cn->commit) { - ct->flags |= NX_CT_F_COMMIT; - } - ofpact_finish(ofpacts, &ct->ofpact); - ofpbuf_push_uninit(ofpacts, ct_offset); + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); } static void @@ -1383,11 +1371,9 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, /* ct_lb without any destinations means that this is an established * connection and we just need to do a NAT. */ const size_t ct_offset = ofpacts->size; - ofpbuf_pull(ofpacts, ct_offset); struct ofpact_conntrack *ct = ofpact_put_CT(ofpacts); struct ofpact_nat *nat; - size_t nat_offset; ct->zone_src.field = ep->is_switch ? mf_from_id(MFF_LOG_CT_ZONE) : mf_from_id(MFF_LOG_DNAT_ZONE); ct->zone_src.ofs = 0; @@ -1396,17 +1382,13 @@ encode_ct_lb(const struct ovnact_ct_lb *cl, ct->recirc_table = recirc_table; ct->alg = 0; - nat_offset = ofpacts->size; - ofpbuf_pull(ofpacts, nat_offset); - nat = ofpact_put_NAT(ofpacts); nat->flags = 0; nat->range_af = AF_UNSPEC; - ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset); - ct = ofpacts->header; - ofpact_finish(ofpacts, &ct->ofpact); - ofpbuf_push_uninit(ofpacts, ct_offset); + ct = ofpbuf_at_assert(ofpacts, ct_offset, sizeof *ct); + ofpacts->header = ct; + ofpact_finish_CT(ofpacts, &ct); return; } diff --git a/tests/ovn.at b/tests/ovn.at index 185ba4a21..adeff95a9 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -1169,6 +1169,9 @@ ct_lb(); formats as ct_lb; encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat) has prereqs ip +ct_clear; ct_lb; reg8[[7]] = 1; + encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]],nat),set_field:0x8000000000/0x8000000000->xreg4 + has prereqs ip ct_lb(192.168.1.2:80, 192.168.1.3:80); Syntax error at `192.168.1.2' expecting backends. ct_lb(backends=192.168.1.2:80,192.168.1.3:80); @@ -1263,6 +1266,9 @@ ct_lb_mark(backends=192.168.1.2:80,192.168.1.3:80; hash_fields="eth_src,eth_dst, ct_next; encodes as ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) has prereqs ip +ct_clear; ct_next; + encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG13[[0..15]]) + has prereqs ip # ct_commit ct_commit; @@ -1318,6 +1324,10 @@ ct_commit { ip4.dst = 192.168.0.1; }; reg8[[17]] = 1; ct_commit { ct_mark.blocked = 1; }; encodes as set_field:0x2000000000000/0x2000000000000->xreg4,ct(commit,zone=NXM_NX_REG13[[0..15]],exec(set_field:0x1/0x1->ct_mark)) has prereqs ip +ct_clear; ct_commit { }; next; + formats as ct_clear; ct_commit; next; + encodes as ct_clear,ct(commit,zone=NXM_NX_REG13[[0..15]]),resubmit(,oflow_in_table) + has prereqs ip # ct_commit_to_zone ct_commit_to_zone(dnat); @@ -1381,6 +1391,9 @@ ct_dnat(192.168.1.2, 1-3000); formats as ct_dnat(192.168.1.2,1-3000); encodes as ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(dst=192.168.1.2:1-3000,random)) has prereqs ip +ct_clear; ct_dnat; + encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) + has prereqs ip ct_dnat(192.168.1.2, 192.168.1.3); Syntax error at `192.168.1.3' expecting Integer for port range. @@ -1415,6 +1428,9 @@ ct_dnat_in_czone(192.168.1.2, 1-3000); formats as ct_dnat_in_czone(192.168.1.2,1-3000); encodes as ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(dst=192.168.1.2:1-3000,random)) has prereqs ip +ct_clear; ct_dnat_in_czone; + encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) + has prereqs ip ct_dnat_in_czone(192.168.1.2, 192.168.1.3); Syntax error at `192.168.1.3' expecting Integer for port range. @@ -1449,6 +1465,9 @@ ct_snat(192.168.1.2, 1-3000); formats as ct_snat(192.168.1.2,1-3000); encodes as ct(commit,table=oflow_in_table,zone=NXM_NX_REG12[[0..15]],nat(src=192.168.1.2:1-3000,random)) has prereqs ip +ct_clear; ct_snat; + encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG12[[0..15]],nat) + has prereqs ip ct_snat(192.168.1.2, 192.168.1.3); Syntax error at `192.168.1.3' expecting Integer for port range. @@ -1483,6 +1502,9 @@ ct_snat_in_czone(192.168.1.2, 1-3000); formats as ct_snat_in_czone(192.168.1.2,1-3000); encodes as ct(commit,table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat(src=192.168.1.2:1-3000,random)) has prereqs ip +ct_clear; ct_snat_in_czone; + encodes as ct_clear,ct(table=oflow_in_table,zone=NXM_NX_REG11[[0..15]],nat) + has prereqs ip ct_snat_in_czone(192.168.1.2, 192.168.1.3); Syntax error at `192.168.1.3' expecting Integer for port range.
As discussed here [0], a couple of functions that encode CT-related actions were using older, manual, way of finishing the action. As amusil mentioned here [1], there's a shorter and more explicit way of doing it. This change replaces manual way with the more explicit aproach. [0] https://mail.openvswitch.org/pipermail/ovs-dev/2024-June/414667.html [1] https://mail.openvswitch.org/pipermail/ovs-dev/2024-April/413317.html Signed-off-by: Martin Kalcok <martin.kalcok@canonical.com> --- Thank you for the review, Ales. I added places that I missed originally. For the unit tests, I copied your suggestion for 'ct_lb' action, and for the rest of the actions, I just pre-pended them with `ct_clear`. I hope that's enough. Martin. controller/lflow.c | 11 +++++----- lib/actions.c | 52 +++++++++++++++------------------------------- tests/ovn.at | 22 ++++++++++++++++++++ 3 files changed, 44 insertions(+), 41 deletions(-)