Re: [ovs-dev] [OVN v13] OVN - Add Support for Remote Port Mirroring
The code is now closer to final, so I've drilled down and listed a number of nits and style changes recommended. These are not complete and you should apply similar changes to the whole body of the patch. I reviewed all the code except test suite changes since you were going to reshuffle the tests, esp. bulk tests; I will wait for v14 for this. I definitely see a number of logical bugs in the code that syncs mirror rules to vswitch db (as well as to sbdb). A general note on test cases: while they seem more complete than before, there are more places for improvement: bulk test scenarios will need to expand to cover cases where the same port is attached and detached to a different number of mirrors in the same transaction; each new field of mirror resource (sink, type etc.) should be checked to make sure updates to OVN db propagate to SB and finally to vswitchd. You can probably identify other**lacunae better than me since you wrote the code. ** One aspect of the patch that I'm not sure about is I-P engine updates. Someone more knowledgeable better review this part, not just me. Looking forward to see v14 with updated test suite changes. Thanks, Ihar On 11/4/22 3:09 PM, Abhiram R N wrote: Mirror creation just creates the mirror. The lsp-attach-mirror triggers the sequence to create Mirror in OVS DB on compute node. OVS already supports Port Mirroring. Note: This is targeted to mirror to destinations anywhere outside the cluster where the analyser resides and it need not be an OVN node. Example commands are as below: Mirror creation ovn-nbctl mirror-add mirror1 gre 0 from-lport 10.10.10.2 Attach a logical port to the mirror. ovn-nbctl lsp-attach-mirror sw0-port1 mirror1 Detach a source from Mirror ovn-nbctl lsp-detach-mirror sw0-port1 mirror1 Mirror deletion ovn-nbctl mirror-del mirror1 Co-authored-by: Veda Barrenkala Signed-off-by: Veda Barrenkala Signed-off-by: Abhiram R N --- v12 --> V13: Made each of bulk test cases(in ovn.at) as separate test to make it pass consistently. V11 --> V12: Minor fix in ovn.at to solve intermittent failures V10 --> V11: Addressed review comments from V10 by Ihar i) Expanded bulk updates test cases in ovn.at Overall below cases are covered a) Attaches multiple mirrors (new) b) Equal detaches and attaches (same as V10) c) Detaches more than attaches (new) d) Attaches more than detaches (new) e) Detaches all (new) ii) Addressed the detach all case in mirror.c iii) Minor correction in NEWS iv) Added invalid mirror attach case in ovn-nbctl.at Files modified (V10 --> V11): Code --> mirror.c Test --> ovn.at, ovn-nbctl.at Misc --> NEWS Ihar, Regarding mirror_delete function param delete_all it is wrt the port binding and if a port binding is removed we delete all its attachment. Already that use case is covered in ovn.at. Having said that the detaches all had issue in mirror_delete which I have addressed. With all the above cases added now in bulk updates hope it should give good assurance. NEWS| 1 + controller/automake.mk | 4 +- controller/mirror.c | 538 + controller/mirror.h | 53 +++ controller/ovn-controller.c | 266 ++-- northd/en-northd.c | 4 + northd/inc-proc-northd.c| 4 + northd/northd.c | 172 northd/northd.h | 2 + ovn-nb.ovsschema| 31 +- ovn-nb.xml | 63 +++ ovn-sb.ovsschema| 26 +- ovn-sb.xml | 50 +++ tests/ovn-nbctl.at | 120 ++ tests/ovn-northd.at | 102 + tests/ovn.at| 778 utilities/ovn-nbctl.c | 357 + utilities/ovn-sbctl.c | 4 + 18 files changed, 2547 insertions(+), 28 deletions(-) create mode 100644 controller/mirror.c create mode 100644 controller/mirror.h diff --git a/NEWS b/NEWS index 224a7b83e..84b22abdb 100644 --- a/NEWS +++ b/NEWS @@ -25,6 +25,7 @@ OVN v22.09.0 - 16 Sep 2022 any of LR's LRP IP, there is no need to create SNAT entry. Now such traffic destined to LRP IP is not dropped. - Bump python version required for building OVN to 3.6. + - Added Support for Remote Port Mirroring. OVN v22.06.0 - 03 Jun 2022 -- diff --git a/controller/automake.mk b/controller/automake.mk index c2ab1bbe6..334672b4d 100644 --- a/controller/automake.mk +++ b/controller/automake.mk @@ -41,7 +41,9 @@ controller_ovn_controller_SOURCES = \ controller/ovsport.h \ controller/ovsport.c \ controller/vif-plug.h \ - controller/vif-plug.c + controller/vif-plug.c \ + controller/mirror.h \ + controller/mirror.c controller_ovn_controller_LDADD = lib/libovn.la
Re: [ovs-dev] [PATCH net-next 5/5] net: move the nat function to nf_nat_core for ovs and tc
On Wed, Nov 16, 2022 at 4:54 PM Pablo Neira Ayuso wrote:
>
> On Tue, Nov 15, 2022 at 10:50:57AM -0500, Xin Long wrote:
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index e29e4ccb5c5a..1c72b8caa24e 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -784,6 +784,137 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb,
> > }
> > EXPORT_SYMBOL_GPL(nf_nat_inet_fn);
> >
> > +/* Modelled after nf_nat_ipv[46]_fn().
> > + * range is only used for new, uninitialized NAT state.
> > + * Returns either NF_ACCEPT or NF_DROP.
> > + */
> > +static int nf_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> > + enum ip_conntrack_info ctinfo, int *action,
> > + const struct nf_nat_range2 *range,
> > + enum nf_nat_manip_type maniptype)
> > +{
> > + __be16 proto = skb_protocol(skb, true);
> > + int hooknum, err = NF_ACCEPT;
> > +
> > + /* See HOOK2MANIP(). */
> > + if (maniptype == NF_NAT_MANIP_SRC)
> > + hooknum = NF_INET_LOCAL_IN; /* Source NAT */
> > + else
> > + hooknum = NF_INET_LOCAL_OUT; /* Destination NAT */
> > +
> > + switch (ctinfo) {
> > + case IP_CT_RELATED:
> > + case IP_CT_RELATED_REPLY:
> > + if (proto == htons(ETH_P_IP) &&
> > + ip_hdr(skb)->protocol == IPPROTO_ICMP) {
> > + if (!nf_nat_icmp_reply_translation(skb, ct, ctinfo,
> > +hooknum))
> > + err = NF_DROP;
> > + goto out;
> > + } else if (IS_ENABLED(CONFIG_IPV6) && proto ==
> > htons(ETH_P_IPV6)) {
> > + __be16 frag_off;
> > + u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> > + int hdrlen = ipv6_skip_exthdr(skb,
> > + sizeof(struct ipv6hdr),
> > + , _off);
> > +
> > + if (hdrlen >= 0 && nexthdr == IPPROTO_ICMPV6) {
> > + if (!nf_nat_icmpv6_reply_translation(skb, ct,
> > + ctinfo,
> > + hooknum,
> > + hdrlen))
> > + err = NF_DROP;
> > + goto out;
> > + }
> > + }
> > + /* Non-ICMP, fall thru to initialize if needed. */
> > + fallthrough;
> > + case IP_CT_NEW:
> > + /* Seen it before? This can happen for loopback, retrans,
> > + * or local packets.
> > + */
> > + if (!nf_nat_initialized(ct, maniptype)) {
> > + /* Initialize according to the NAT action. */
> > + err = (range && range->flags & NF_NAT_RANGE_MAP_IPS)
> > + /* Action is set up to establish a new
> > + * mapping.
> > + */
> > + ? nf_nat_setup_info(ct, range, maniptype)
> > + : nf_nat_alloc_null_binding(ct, hooknum);
> > + if (err != NF_ACCEPT)
> > + goto out;
> > + }
> > + break;
> > +
> > + case IP_CT_ESTABLISHED:
> > + case IP_CT_ESTABLISHED_REPLY:
> > + break;
> > +
> > + default:
> > + err = NF_DROP;
> > + goto out;
> > + }
> > +
> > + err = nf_nat_packet(ct, ctinfo, hooknum, skb);
> > + if (err == NF_ACCEPT)
> > + *action |= (1 << maniptype);
> > +out:
> > + return err;
> > +}
> > +
> > +int nf_ct_nat(struct sk_buff *skb, struct nf_conn *ct,
> > + enum ip_conntrack_info ctinfo, int *action,
> > + const struct nf_nat_range2 *range, bool commit)
> > +{
> > + enum nf_nat_manip_type maniptype;
> > + int err, ct_action = *action;
> > +
> > + *action = 0;
> > +
> > + /* Add NAT extension if not confirmed yet. */
> > + if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> > + return NF_ACCEPT; /* Can't NAT. */
> > +
> > + if (ctinfo != IP_CT_NEW && (ct->status & IPS_NAT_MASK) &&
> > + (ctinfo != IP_CT_RELATED || commit)) {
> > + /* NAT an established or related connection like before. */
> > + if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY)
> > + /* This is the REPLY direction for a connection
> > + * for which NAT was applied in the forward
> > + * direction. Do the reverse NAT.
> > + */
> > + maniptype = ct->status & IPS_SRC_NAT
> > +
Re: [ovs-dev] [PATCH net-next 5/5] net: move the nat function to nf_nat_core for ovs and tc
On Wed, Nov 16, 2022 at 4:05 PM Aaron Conole wrote:
>
> Hi Xin,
>
> Xin Long writes:
>
> > There are two nat functions are nearly the same in both OVS and
> > TC code, (ovs_)ct_nat_execute() and ovs_ct_nat/tcf_ct_act_nat().
> >
> > This patch is to move them to netfilter nf_nat_core and export
> > nf_ct_nat() so that it can be shared by both OVS and TC, and
> > keep the nat (type) check and nat flag update in OVS and TC's
> > own place, as these parts are different between OVS and TC.
> >
> > Note that in OVS nat function it was using skb->protocol to get
> > the proto as it already skips vlans in key_extract(), while it
> > doesn't in TC, and TC has to call skb_protocol() to get proto.
> > So in nf_ct_nat_execute(), we keep using skb_protocol() which
> > works for both OVS and TC.
> >
> > Signed-off-by: Xin Long
> > ---
> > include/net/netfilter/nf_nat.h | 4 +
> > net/netfilter/nf_nat_core.c| 131 +++
> > net/openvswitch/conntrack.c| 137 +++--
> > net/sched/act_ct.c | 136 +++-
> > 4 files changed, 156 insertions(+), 252 deletions(-)
> >
> > diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
> > index e9eb01e99d2f..9877f064548a 100644
> > --- a/include/net/netfilter/nf_nat.h
> > +++ b/include/net/netfilter/nf_nat.h
> > @@ -104,6 +104,10 @@ unsigned int
> > nf_nat_inet_fn(void *priv, struct sk_buff *skb,
> > const struct nf_hook_state *state);
> >
> > +int nf_ct_nat(struct sk_buff *skb, struct nf_conn *ct,
> > + enum ip_conntrack_info ctinfo, int *action,
> > + const struct nf_nat_range2 *range, bool commit);
> > +
> > static inline int nf_nat_initialized(const struct nf_conn *ct,
> >enum nf_nat_manip_type manip)
> > {
> > diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> > index e29e4ccb5c5a..1c72b8caa24e 100644
> > --- a/net/netfilter/nf_nat_core.c
> > +++ b/net/netfilter/nf_nat_core.c
> > @@ -784,6 +784,137 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb,
> > }
> > EXPORT_SYMBOL_GPL(nf_nat_inet_fn);
> >
> > +/* Modelled after nf_nat_ipv[46]_fn().
> > + * range is only used for new, uninitialized NAT state.
> > + * Returns either NF_ACCEPT or NF_DROP.
> > + */
> > +static int nf_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> > + enum ip_conntrack_info ctinfo, int *action,
> > + const struct nf_nat_range2 *range,
> > + enum nf_nat_manip_type maniptype)
> > +{
> > + __be16 proto = skb_protocol(skb, true);
> > + int hooknum, err = NF_ACCEPT;
> > +
> > + /* See HOOK2MANIP(). */
> > + if (maniptype == NF_NAT_MANIP_SRC)
> > + hooknum = NF_INET_LOCAL_IN; /* Source NAT */
> > + else
> > + hooknum = NF_INET_LOCAL_OUT; /* Destination NAT */
> > +
> > + switch (ctinfo) {
> > + case IP_CT_RELATED:
> > + case IP_CT_RELATED_REPLY:
> > + if (proto == htons(ETH_P_IP) &&
> > + ip_hdr(skb)->protocol == IPPROTO_ICMP) {
> > + if (!nf_nat_icmp_reply_translation(skb, ct, ctinfo,
> > +hooknum))
> > + err = NF_DROP;
> > + goto out;
> > + } else if (IS_ENABLED(CONFIG_IPV6) && proto ==
> > htons(ETH_P_IPV6)) {
> > + __be16 frag_off;
> > + u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> > + int hdrlen = ipv6_skip_exthdr(skb,
> > + sizeof(struct ipv6hdr),
> > + , _off);
> > +
> > + if (hdrlen >= 0 && nexthdr == IPPROTO_ICMPV6) {
> > + if (!nf_nat_icmpv6_reply_translation(skb, ct,
> > + ctinfo,
> > + hooknum,
> > + hdrlen))
> > + err = NF_DROP;
> > + goto out;
> > + }
> > + }
> > + /* Non-ICMP, fall thru to initialize if needed. */
> > + fallthrough;
> > + case IP_CT_NEW:
> > + /* Seen it before? This can happen for loopback, retrans,
> > + * or local packets.
> > + */
> > + if (!nf_nat_initialized(ct, maniptype)) {
> > + /* Initialize according to the NAT action. */
> > + err = (range && range->flags & NF_NAT_RANGE_MAP_IPS)
> > + /* Action is set up to establish a new
> > + * mapping.
> > + */
> > +
Re: [ovs-dev] [PATCH net-next 5/5] net: move the nat function to nf_nat_core for ovs and tc
On Tue, Nov 15, 2022 at 10:50:57AM -0500, Xin Long wrote:
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index e29e4ccb5c5a..1c72b8caa24e 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -784,6 +784,137 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb,
> }
> EXPORT_SYMBOL_GPL(nf_nat_inet_fn);
>
> +/* Modelled after nf_nat_ipv[46]_fn().
> + * range is only used for new, uninitialized NAT state.
> + * Returns either NF_ACCEPT or NF_DROP.
> + */
> +static int nf_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> + enum ip_conntrack_info ctinfo, int *action,
> + const struct nf_nat_range2 *range,
> + enum nf_nat_manip_type maniptype)
> +{
> + __be16 proto = skb_protocol(skb, true);
> + int hooknum, err = NF_ACCEPT;
> +
> + /* See HOOK2MANIP(). */
> + if (maniptype == NF_NAT_MANIP_SRC)
> + hooknum = NF_INET_LOCAL_IN; /* Source NAT */
> + else
> + hooknum = NF_INET_LOCAL_OUT; /* Destination NAT */
> +
> + switch (ctinfo) {
> + case IP_CT_RELATED:
> + case IP_CT_RELATED_REPLY:
> + if (proto == htons(ETH_P_IP) &&
> + ip_hdr(skb)->protocol == IPPROTO_ICMP) {
> + if (!nf_nat_icmp_reply_translation(skb, ct, ctinfo,
> +hooknum))
> + err = NF_DROP;
> + goto out;
> + } else if (IS_ENABLED(CONFIG_IPV6) && proto ==
> htons(ETH_P_IPV6)) {
> + __be16 frag_off;
> + u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> + int hdrlen = ipv6_skip_exthdr(skb,
> + sizeof(struct ipv6hdr),
> + , _off);
> +
> + if (hdrlen >= 0 && nexthdr == IPPROTO_ICMPV6) {
> + if (!nf_nat_icmpv6_reply_translation(skb, ct,
> + ctinfo,
> + hooknum,
> + hdrlen))
> + err = NF_DROP;
> + goto out;
> + }
> + }
> + /* Non-ICMP, fall thru to initialize if needed. */
> + fallthrough;
> + case IP_CT_NEW:
> + /* Seen it before? This can happen for loopback, retrans,
> + * or local packets.
> + */
> + if (!nf_nat_initialized(ct, maniptype)) {
> + /* Initialize according to the NAT action. */
> + err = (range && range->flags & NF_NAT_RANGE_MAP_IPS)
> + /* Action is set up to establish a new
> + * mapping.
> + */
> + ? nf_nat_setup_info(ct, range, maniptype)
> + : nf_nat_alloc_null_binding(ct, hooknum);
> + if (err != NF_ACCEPT)
> + goto out;
> + }
> + break;
> +
> + case IP_CT_ESTABLISHED:
> + case IP_CT_ESTABLISHED_REPLY:
> + break;
> +
> + default:
> + err = NF_DROP;
> + goto out;
> + }
> +
> + err = nf_nat_packet(ct, ctinfo, hooknum, skb);
> + if (err == NF_ACCEPT)
> + *action |= (1 << maniptype);
> +out:
> + return err;
> +}
> +
> +int nf_ct_nat(struct sk_buff *skb, struct nf_conn *ct,
> + enum ip_conntrack_info ctinfo, int *action,
> + const struct nf_nat_range2 *range, bool commit)
> +{
> + enum nf_nat_manip_type maniptype;
> + int err, ct_action = *action;
> +
> + *action = 0;
> +
> + /* Add NAT extension if not confirmed yet. */
> + if (!nf_ct_is_confirmed(ct) && !nf_ct_nat_ext_add(ct))
> + return NF_ACCEPT; /* Can't NAT. */
> +
> + if (ctinfo != IP_CT_NEW && (ct->status & IPS_NAT_MASK) &&
> + (ctinfo != IP_CT_RELATED || commit)) {
> + /* NAT an established or related connection like before. */
> + if (CTINFO2DIR(ctinfo) == IP_CT_DIR_REPLY)
> + /* This is the REPLY direction for a connection
> + * for which NAT was applied in the forward
> + * direction. Do the reverse NAT.
> + */
> + maniptype = ct->status & IPS_SRC_NAT
> + ? NF_NAT_MANIP_DST : NF_NAT_MANIP_SRC;
> + else
> + maniptype = ct->status & IPS_SRC_NAT
> + ? NF_NAT_MANIP_SRC : NF_NAT_MANIP_DST;
> + } else if (ct_action & (1 << NF_NAT_MANIP_SRC)) {
> + maniptype
Re: [ovs-dev] [PATCH ovn v2 5/5] tutorial: Add scripts to simulate node-port ovn-k8s services.
Acked-by: Mark Michelson
I have some python-related nits below, but I don't think any of them is
strong enough to prevent this from being merged.
On 11/4/22 18:11, Dumitru Ceara wrote:
In a sandbox run:
$ ./ovn-lb-benchmark.sh
to simulate an ovn-k8s-like topology with N nodes, VIPS NodePort services
applied to all nodes. Each service has BACKENDS backends.
If USE_TEMPLATES is "yes" then the configuration will be optimized to use
Chassis_Template_Vars. Otherwise it will create N LBs per service, one
for every node.
Signed-off-by: Dumitru Ceara
---
tutorial/automake.mk |4 +
tutorial/ovn-gen-lb-template-vars.py | 116 ++
tutorial/ovn-lb-benchmark.sh | 110
3 files changed, 229 insertions(+), 1 deletion(-)
create mode 100755 tutorial/ovn-gen-lb-template-vars.py
create mode 100755 tutorial/ovn-lb-benchmark.sh
diff --git a/tutorial/automake.mk b/tutorial/automake.mk
index 046962c000..171da8de66 100644
--- a/tutorial/automake.mk
+++ b/tutorial/automake.mk
@@ -1,6 +1,8 @@
EXTRA_DIST += \
tutorial/ovs-sandbox \
- tutorial/ovn-setup.sh
+ tutorial/ovn-setup.sh \
+ tutorial/ovn-lb-benchmark.sh \
+ tutorial/ovn-gen-lb-template-vars.py
sandbox: all
cd $(srcdir)/tutorial && MAKE=$(MAKE) HAVE_OPENSSL=$(HAVE_OPENSSL) \
./ovs-sandbox -b $(abs_builddir) --ovs-src $(ovs_srcdir)
--ovs-build $(ovs_builddir) $(SANDBOXFLAGS)
diff --git a/tutorial/ovn-gen-lb-template-vars.py
b/tutorial/ovn-gen-lb-template-vars.py
new file mode 100755
index 00..fe7e6b93f6
--- /dev/null
+++ b/tutorial/ovn-gen-lb-template-vars.py
@@ -0,0 +1,116 @@
+import getopt
+import os
+import re
+import sys
+import uuid
+
+import ovs.db.idl
+import ovs.db.schema
+import ovs.db.types
+import ovs.ovsuuid
+import ovs.poller
+import ovs.stream
+import ovs.util
+import ovs.vlog
+from ovs.db import data
+from ovs.db import error
+from ovs.db.idl import _row_to_uuid as row_to_uuid
+from ovs.fatal_signal import signal_alarm
The following imports are not used:
* os
* re
* uuid
* ovs.db.schema
* ovs.db.types
* ovs.ovsuuid
* from ovs.db import data
* from ovs.db.idl import _row_to_uuid as row_to_uuid
+
+vlog = ovs.vlog.Vlog("template-lb-stress")
+vlog.set_levels_from_string("console:info")
+vlog.init(None)
+
+SCHEMA = '../ovn-nb.ovsschema'
+
+
+def add_chassis_template_vars(idl, n, n_vips, n_backends):
+for i in range(1, n + 1):
+print(f'ADDING LBs for node {i}')
+txn = ovs.db.idl.Transaction(idl)
Would it make sense to create the transaction outside this for loop and
then commit just one big transaction at the end?
+tv = txn.insert(idl.tables["Chassis_Template_Var"])
+tv.chassis = f'chassis-{i}'
+tv.setkey('variables', 'vip', f'42.42.42.{i}')
+
+for j in range(1, n_vips + 1):
+backends = ''
+for k in range(0, n_backends):
+j1 = j // 250
+j2 = j % 250
The j1 and j2 calculations can be moved out of this loop since they are
constant across all values of k.
If you do that, then you can rewrite this inner loop as a list
comprehension:
backends = [f'42.{k}.{j1}.{j2}:{j}' for k in range(n_backends)]
(note that when using range(), you can omit the '0' as the first
parameter and just put the upper bound)
Then when you need to stringify backends, use
','.join(backends)
+backends = f'42.{k}.{j1}.{j2}:{j},{backends}'
+tv.setkey('variables', f'backends{j}', backends)
+status = txn.commit_block()
+sys.stdout.write(
+f'commit status =
{ovs.db.idl.Transaction.status_to_string(status)}\n'
+)
+
+
+def run(remote, n, n_vips, n_backends):
+schema_helper = ovs.db.idl.SchemaHelper(SCHEMA)
+schema_helper.register_all()
+idl = ovs.db.idl.Idl(remote, schema_helper, leader_only=False)
+
+seqno = 0
+
+error, stream = ovs.stream.Stream.open_block(
+ovs.stream.Stream.open(remote), 2000
+)
+if error:
+sys.stderr.write(f'failed to connect to \"{remote}\"')
+sys.exit(1)
+
+if not stream:
+sys.stderr.write(f'failed to connect to \"{remote}\"')
+sys.exit(1)
+rpc = ovs.jsonrpc.Connection(stream)
+
+while idl.change_seqno == seqno and not idl.run():
+rpc.run()
+
+poller = ovs.poller.Poller()
+idl.wait(poller)
+rpc.wait(poller)
+poller.block()
+
+add_chassis_template_vars(idl, n, n_vips, n_backends)
+
+
+def main(argv):
+try:
+options, args = getopt.gnu_getopt(
+argv[1:], 'n:v:b:r:', ['vips', 'backends', 'remote']
+)
+except getopt.GetoptError as geo:
+sys.stderr.write(f'{ovs.util.PROGRAM_NAME}: {geo.msg}\n')
+sys.exit(1)
+
+n = None
+vips = None
+backends = None
+remote = None
+for key, value in options:
+if key ==
Re: [ovs-dev] [PATCH ovn] northd: bypass connection tracking for stateless flows when there are LB flows present
On Wed, Nov 16, 2022 at 3:05 PM Venugopal Iyer via dev
wrote:
>
> Thanks for looking at this, Numan. I haven't worked with the system tests
> before, and am having some
> issues getting it to run. So, let me get back to you on this.
>
Sure. Please make sure that the test case checks that there is no
conntrack entry for the stateless traffic.
Existing system tests should give you a good reference. Let me know
if you have any questions.
Numan
> regards,
>
> -venu
>
>
> From: Numan Siddique
> Sent: Monday, November 14, 2022 9:17 AM
> To: Han Zhou
> Cc: Venugopal Iyer; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH ovn] northd: bypass connection tracking for
> stateless flows when there are LB flows present
>
> External email: Use caution opening links or attachments
>
>
> On Wed, Nov 9, 2022 at 4:15 PM Numan Siddique wrote:
> >
> > On Wed, Nov 9, 2022 at 4:11 PM Han Zhou wrote:
> > >
> > > On Tue, Nov 8, 2022 at 7:51 AM venu.iyer wrote:
> > > >
> > > > Currently, even stateless flows are subject to connection tracking when
> > > there are
> > > > LB rules (for DNAT). However, if a flow needs to be subjected to LB,
> > > > then
> > > it shouldn't
> > > > be configured as stateless.
> > > >
> > > > A stateless flow means we should not track it, and this change exempts
> > > stateless
> > > > flows from being tracked regardless of whether LB rules are present or
> > > not.
> > > >
> > > > Signed-off-by: venu.iyer
> > > > Acked-by: Han Zhou
>
> I had a quick look and it looks ok to me.
> Can you please add system tests to cover some scenarios for this use
> case to avoid future regressions ?
>
> Thanks
> Numan
>
>
>
>
> > > > ---
> > > > northd/northd.c | 24 +-
> > > > northd/ovn-northd.8.xml | 56 ++---
> > > > ovn-nb.xml | 3 +++
> > > > tests/ovn-northd.at | 48 ---
> > > > tests/ovn.at| 4 +--
> > > > 5 files changed, 69 insertions(+), 66 deletions(-)
> > > >
> > > > diff --git a/northd/northd.c b/northd/northd.c
> > > > index b7388afc5..da4beede6 100644
> > > > --- a/northd/northd.c
> > > > +++ b/northd/northd.c
> > > > @@ -137,8 +137,8 @@ enum ovn_stage {
> > > > PIPELINE_STAGE(SWITCH, IN, L2_UNKNOWN,24, "ls_in_l2_unknown")
> > > \
> > > >
> > > \
> > > > /* Logical switch egress stages. */
> > > \
> > > > -PIPELINE_STAGE(SWITCH, OUT, PRE_LB, 0, "ls_out_pre_lb")
> > > \
> > > > -PIPELINE_STAGE(SWITCH, OUT, PRE_ACL, 1, "ls_out_pre_acl")
> > > \
> > > > +PIPELINE_STAGE(SWITCH, OUT, PRE_ACL, 0, "ls_out_pre_acl")
> > > \
> > > > +PIPELINE_STAGE(SWITCH, OUT, PRE_LB, 1, "ls_out_pre_lb")
> > > \
> > > > PIPELINE_STAGE(SWITCH, OUT, PRE_STATEFUL, 2, "ls_out_pre_stateful")
> > > \
> > > > PIPELINE_STAGE(SWITCH, OUT, ACL_HINT, 3, "ls_out_acl_hint")
> > > \
> > > > PIPELINE_STAGE(SWITCH, OUT, ACL, 4, "ls_out_acl")
> > > \
> > > > @@ -210,6 +210,7 @@ enum ovn_stage {
> > > > #define REGBIT_ACL_LABEL "reg0[13]"
> > > > #define REGBIT_FROM_RAMP "reg0[14]"
> > > > #define REGBIT_PORT_SEC_DROP "reg0[15]"
> > > > +#define REGBIT_ACL_STATELESS "reg0[16]"
> > > >
> > > > #define REG_ORIG_DIP_IPV4 "reg1"
> > > > #define REG_ORIG_DIP_IPV6 "xxreg1"
> > > > @@ -271,7 +272,7 @@ enum ovn_stage {
> > > > * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | |
> > > |
> > > > * || REGBIT_{HAIRPIN/HAIRPIN_REPLY} | |
> > > |
> > > > * || REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | |
> > > |
> > > > - * || REGBIT_ACL_LABEL | X |
> > > |
> > > > + * || REGBIT_ACL_{LABEL/STATELESS} | X |
> > > |
> > > > * ++--+ X |
> > > |
> > > > * | R1 | ORIG_DIP_IPV4 (>= IN_PRE_STATEFUL) | R |
> > > |
> > > > * ++--+ E |
> > > |
> > > > @@ -5677,17 +5678,18 @@ build_stateless_filter(struct ovn_datapath *od,
> > > > const struct nbrec_acl *acl,
> > > > struct hmap *lflows)
> > > > {
> > > > +const char *action = REGBIT_ACL_STATELESS" = 1; next;";
> > > > if (!strcmp(acl->direction, "from-lport")) {
> > > > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL,
> > > > acl->priority + OVN_ACL_PRI_OFFSET,
> > > > acl->match,
> > > > -"next;",
> > > > +action,
> > > > >header_);
> > > > } else {
> > > > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL,
> > > > acl->priority + OVN_ACL_PRI_OFFSET,
> > > >
Re: [ovs-dev] [PATCH ovn v2 2/5] Add NB and SB Template_Var tables.
Hi Dumitru, I have a few comments below.
On 11/4/22 18:11, Dumitru Ceara wrote:
Propagate the contents of the NB table to the Southbound.
Signed-off-by: Dumitru Ceara
---
Note:
- ovn-trace doesn't support template variables (yet).
V2:
- Fixed TEMPLATE_VAR_TABLE_INITIALIZER definition so that GCC doesn't
complain anymore.
- Addressed Han's comments:
- Rename tables to Chassis_Template_Var.
- Fix man page.
- Simplify function prototypes.
- Changed schema as suggested by Ilya.
---
northd/automake.mk |4 ++
northd/en-northd.c |4 ++
northd/inc-proc-northd.c |8 -
northd/northd.c | 41 +
northd/northd.h |4 ++
northd/template-var.c| 74 ++
northd/template-var.h| 58
ovn-nb.ovsschema | 17 +--
ovn-nb.xml | 29 ++
ovn-sb.ovsschema | 12 ++-
ovn-sb.xml | 15 +
tests/ovn-northd.at | 35 ++
utilities/ovn-nbctl.c|3 ++
utilities/ovn-sbctl.c|3 ++
14 files changed, 299 insertions(+), 8 deletions(-)
create mode 100644 northd/template-var.c
create mode 100644 northd/template-var.h
diff --git a/northd/automake.mk b/northd/automake.mk
index 81582867dc..31134bc329 100644
--- a/northd/automake.mk
+++ b/northd/automake.mk
@@ -13,7 +13,9 @@ northd_ovn_northd_SOURCES = \
northd/inc-proc-northd.c \
northd/inc-proc-northd.h \
northd/ipam.c \
- northd/ipam.h
+ northd/ipam.h \
+ northd/template-var.c \
+ northd/template-var.h
northd_ovn_northd_LDADD = \
lib/libovn.la \
$(OVSDB_LIBDIR)/libovsdb.la \
diff --git a/northd/en-northd.c b/northd/en-northd.c
index 7fe83db642..030ee25d8f 100644
--- a/northd/en-northd.c
+++ b/northd/en-northd.c
@@ -80,6 +80,8 @@ void en_northd_run(struct engine_node *node, void *data)
EN_OVSDB_GET(engine_get_input("NB_acl", node));
input_data.nbrec_static_mac_binding_table =
EN_OVSDB_GET(engine_get_input("NB_static_mac_binding", node));
+input_data.nbrec_chassis_template_var_table =
+EN_OVSDB_GET(engine_get_input("NB_chassis_template_var", node));
input_data.sbrec_sb_global_table =
EN_OVSDB_GET(engine_get_input("SB_sb_global", node));
@@ -113,6 +115,8 @@ void en_northd_run(struct engine_node *node, void *data)
EN_OVSDB_GET(engine_get_input("SB_chassis_private", node));
input_data.sbrec_static_mac_binding_table =
EN_OVSDB_GET(engine_get_input("SB_static_mac_binding", node));
+input_data.sbrec_chassis_template_var_table =
+EN_OVSDB_GET(engine_get_input("SB_chassis_template_var", node));
northd_run(_data, data,
eng_ctx->ovnnb_idl_txn,
diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
index 54e0ad3b05..da791f035d 100644
--- a/northd/inc-proc-northd.c
+++ b/northd/inc-proc-northd.c
@@ -64,7 +64,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
NB_NODE(ha_chassis_group, "ha_chassis_group") \
NB_NODE(ha_chassis, "ha_chassis") \
NB_NODE(bfd, "bfd") \
-NB_NODE(static_mac_binding, "static_mac_binding")
+NB_NODE(static_mac_binding, "static_mac_binding") \
+NB_NODE(chassis_template_var, "chassis_template_var")
enum nb_engine_node {
#define NB_NODE(NAME, NAME_STR) NB_##NAME,
@@ -114,7 +115,8 @@ VLOG_DEFINE_THIS_MODULE(inc_proc_northd);
SB_NODE(load_balancer, "load_balancer") \
SB_NODE(bfd, "bfd") \
SB_NODE(fdb, "fdb") \
-SB_NODE(static_mac_binding, "static_mac_binding")
+SB_NODE(static_mac_binding, "static_mac_binding") \
+SB_NODE(chassis_template_var, "chassis_template_var")
enum sb_engine_node {
#define SB_NODE(NAME, NAME_STR) SB_##NAME,
@@ -186,6 +188,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _nb_ha_chassis_group, NULL);
engine_add_input(_northd, _nb_ha_chassis, NULL);
engine_add_input(_northd, _nb_static_mac_binding, NULL);
+engine_add_input(_northd, _nb_chassis_template_var, NULL);
engine_add_input(_northd, _sb_sb_global, NULL);
engine_add_input(_northd, _sb_chassis, NULL);
@@ -215,6 +218,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
engine_add_input(_northd, _sb_load_balancer, NULL);
engine_add_input(_northd, _sb_fdb, NULL);
engine_add_input(_northd, _sb_static_mac_binding, NULL);
+engine_add_input(_northd, _sb_chassis_template_var, NULL);
engine_add_input(_mac_binding_aging, _nb_nb_global, NULL);
engine_add_input(_mac_binding_aging, _sb_mac_binding, NULL);
engine_add_input(_mac_binding_aging, _northd, NULL);
diff --git a/northd/northd.c b/northd/northd.c
index b7388afc58..170b4f95c8 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -51,6 +51,7 @@
#include
Re: [ovs-dev] [PATCH ovn v2 3/5] controller: Add support for templated actions and matches.
Hi Dumitru, I have a few comments down below
On 11/4/22 18:11, Dumitru Ceara wrote:
Expand SB.Template_Var records in two stages:
1. first expand them to local values in match/action strings
2. then reparse the expanded strings
For the case when a lflow references a Chassis_Template_Var
also track references (similar to the ones maintained for
multicast groups, address sets, port_groups, port bindings).
Signed-off-by: Dumitru Ceara
---
V2:
- Fix GCC build due to missing newline.
- Handle SB table rename Template_Var -> Chassis_Template_Var.
- Address Han's comments:
- Add new function to parse lflow actions.
- Move xstrdup inside lexer_parse_template_string() and execute it only
if needed.
- Match template vars only by chassis name.
- Change local_templates table to a plain smap.
- Some indentation updates.
- Use NULL template variable change handlers for chassis/Open_vSwitch
changes.
- Added more tests.
- Fix tracking of template references in lflow actions.
---
controller/lflow.c | 136 ++--
controller/lflow.h |1
controller/ofctrl.c | 11 +-
controller/ofctrl.h |3
controller/ovn-controller.c | 289 +++
include/ovn/expr.h |4 -
include/ovn/lex.h | 15 ++
lib/actions.c |9 +
lib/expr.c | 18 ++-
lib/lex.c | 57
lib/objdep.c|1
lib/objdep.h|1
tests/ovn-controller.at | 50 +++
tests/ovn.at| 80
tests/test-ovn.c| 17 ++-
utilities/ovn-trace.c | 33 -
16 files changed, 658 insertions(+), 67 deletions(-)
diff --git a/controller/lflow.c b/controller/lflow.c
index d4434bdee8..fc4371d0df 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -81,6 +81,8 @@ convert_match_to_expr(const struct sbrec_logical_flow *,
const struct local_datapath *ldp,
struct expr **prereqs, const struct shash *addr_sets,
const struct shash *port_groups,
+ const struct smap *template_vars,
+ struct sset *template_vars_ref,
struct objdep_mgr *, bool *pg_addr_set_ref);
static void
add_matches_to_flow_table(const struct sbrec_logical_flow *,
@@ -297,6 +299,43 @@ as_info_from_expr_const(const char *as_name, const union
expr_constant *c,
return true;
}
+static bool
+lflow_parse_actions(const struct sbrec_logical_flow *lflow,
+const struct lflow_ctx_in *l_ctx_in,
+struct sset *template_vars_ref,
+struct ofpbuf *ovnacts_out,
+struct expr **prereqs_out)
+{
+bool ingress = !strcmp(lflow->pipeline, "ingress");
+struct ovnact_parse_params pp = {
+.symtab = ,
+.dhcp_opts = l_ctx_in->dhcp_opts,
+.dhcpv6_opts = l_ctx_in->dhcpv6_opts,
+.nd_ra_opts = l_ctx_in->nd_ra_opts,
+.controller_event_opts = l_ctx_in->controller_event_opts,
+
+.pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
+.n_tables = LOG_PIPELINE_LEN,
+.cur_ltable = lflow->table_id,
+};
+
+char *actions_expanded_s = NULL;
+const char *actions_s =
+lexer_parse_template_string(lflow->actions, l_ctx_in->template_vars,
+template_vars_ref, _expanded_s);
+char *error = ovnacts_parse_string(actions_s, ,
+ ovnacts_out, prereqs_out);
+free(actions_expanded_s);
+if (error) {
+static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
+VLOG_WARN_RL(, "error parsing actions \"%s\": %s",
+ lflow->actions, error);
+free(error);
+return false;
+}
+return true;
+}
+
/* Parses the lflow regarding the changed address set 'as_name', and generates
* ovs flows for the newly added addresses in 'as_diff_added' only. It is
* similar to consider_logical_flow__, with the below differences:
@@ -347,27 +386,14 @@ consider_lflow_for_added_as_ips__(
uint64_t ovnacts_stub[1024 / 8];
struct ofpbuf ovnacts = OFPBUF_STUB_INITIALIZER(ovnacts_stub);
-struct ovnact_parse_params pp = {
-.symtab = ,
-.dhcp_opts = l_ctx_in->dhcp_opts,
-.dhcpv6_opts = l_ctx_in->dhcpv6_opts,
-.nd_ra_opts = l_ctx_in->nd_ra_opts,
-.controller_event_opts = l_ctx_in->controller_event_opts,
-.pipeline = ingress ? OVNACT_P_INGRESS : OVNACT_P_EGRESS,
-.n_tables = LOG_PIPELINE_LEN,
-.cur_ltable = lflow->table_id,
-};
+struct sset template_vars_ref = SSET_INITIALIZER(_vars_ref);
struct expr *prereqs = NULL;
-char *error;
-error = ovnacts_parse_string(lflow->actions, , , );
-if (error) {
-static
Re: [ovs-dev] [PATCH ovn v2 1/5] lflow: Factor out the lflow reference handling code into a new module.
Acked-by: Mark Michelson
On 11/4/22 18:11, Dumitru Ceara wrote:
This makes it easier to have an overview of what the code does and at the
same time it allows multiple users to define and manage
"resource <-> object" dependencies.
Acked-by: Han Zhou
Signed-off-by: Dumitru Ceara
---
V2:
- Addressed Mark's comments:
- Fixed typos in comments in objdep.h.
- Made objdep_change_handler return bool (handled successfully or not).
- Reverted some unrelated style changes.
- Fixed cast style.
- Addressed Han's comments:
- Removed superfluous 'type' argument name in prototypes.
- Added Han's ack.
---
controller/lflow.c | 330 ++-
controller/lflow.h | 67 +
controller/ovn-controller.c | 55 +--
lib/automake.mk |2
lib/objdep.c| 260 ++
lib/objdep.h| 122
6 files changed, 508 insertions(+), 328 deletions(-)
create mode 100644 lib/objdep.c
create mode 100644 lib/objdep.h
diff --git a/controller/lflow.c b/controller/lflow.c
index cc0f31db06..d4434bdee8 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -61,7 +61,7 @@ struct lookup_port_aux {
struct ovsdb_idl_index *sbrec_port_binding_by_name;
const struct sbrec_datapath_binding *dp;
const struct sbrec_logical_flow *lflow;
-struct lflow_resource_ref *lfrr;
+struct objdep_mgr *deps_mgr;
const struct hmap *chassis_tunnels;
};
@@ -72,8 +72,8 @@ struct condition_aux {
const struct sset *active_tunnels;
const struct sbrec_logical_flow *lflow;
/* Resource reference to store the port name referenced
- * in is_chassis_resident() to the logical flow. */
-struct lflow_resource_ref *lfrr;
+ * in is_chassis_resident() to the object (logical flow). */
+struct objdep_mgr *deps_mgr;
};
static struct expr *
@@ -81,7 +81,7 @@ convert_match_to_expr(const struct sbrec_logical_flow *,
const struct local_datapath *ldp,
struct expr **prereqs, const struct shash *addr_sets,
const struct shash *port_groups,
- struct lflow_resource_ref *, bool *pg_addr_set_ref);
+ struct objdep_mgr *, bool *pg_addr_set_ref);
static void
add_matches_to_flow_table(const struct sbrec_logical_flow *,
const struct local_datapath *,
@@ -94,17 +94,6 @@ consider_logical_flow(const struct sbrec_logical_flow *lflow,
bool is_recompute,
struct lflow_ctx_in *l_ctx_in,
struct lflow_ctx_out *l_ctx_out);
-static void lflow_resource_add(struct lflow_resource_ref *, enum ref_type,
- const char *ref_name, const struct uuid *,
- size_t ref_count);
-static struct ref_lflow_node *ref_lflow_lookup(struct hmap *ref_lflow_table,
- enum ref_type,
- const char *ref_name);
-static struct lflow_ref_node *lflow_ref_lookup(struct hmap *lflow_ref_table,
- const struct uuid *lflow_uuid);
-static void ref_lflow_node_destroy(struct ref_lflow_node *);
-static void lflow_resource_destroy_lflow(struct lflow_resource_ref *,
- const struct uuid *lflow_uuid);
static void add_port_sec_flows(const struct shash *binding_lports,
const struct sbrec_chassis *,
@@ -125,8 +114,8 @@ lookup_port_cb(const void *aux_, const char *port_name,
unsigned int *portp)
/* Store the name that used to lookup the lport to lflow reference, so
that
* in the future when the lport's port binding changes, the logical flow
* that references this lport can be reprocessed. */
-lflow_resource_add(aux->lfrr, REF_TYPE_PORTBINDING, port_name,
- >lflow->header_.uuid, 0);
+objdep_mgr_add(aux->deps_mgr, OBJDEP_TYPE_PORTBINDING, port_name,
+ >lflow->header_.uuid);
const struct sbrec_port_binding *pb
= lport_lookup_by_name(aux->sbrec_port_binding_by_name, port_name);
@@ -141,8 +130,8 @@ lookup_port_cb(const void *aux_, const char *port_name,
unsigned int *portp)
* this multicast group can be reprocessed. */
struct ds mg_key = DS_EMPTY_INITIALIZER;
get_mc_group_key(port_name, aux->dp->tunnel_key, _key);
-lflow_resource_add(aux->lfrr, REF_TYPE_MC_GROUP, ds_cstr(_key),
- >lflow->header_.uuid, 0);
+objdep_mgr_add(aux->deps_mgr, OBJDEP_TYPE_MC_GROUP, ds_cstr(_key),
+ >lflow->header_.uuid);
ds_destroy(_key);
const struct sbrec_multicast_group *mg = mcgroup_lookup_by_dp_name(
@@ -180,11 +169,11 @@ is_chassis_resident_cb(const void *c_aux_, const char
Re: [ovs-dev] [PATCH net-next 5/5] net: move the nat function to nf_nat_core for ovs and tc
Hi Xin,
Xin Long writes:
> There are two nat functions are nearly the same in both OVS and
> TC code, (ovs_)ct_nat_execute() and ovs_ct_nat/tcf_ct_act_nat().
>
> This patch is to move them to netfilter nf_nat_core and export
> nf_ct_nat() so that it can be shared by both OVS and TC, and
> keep the nat (type) check and nat flag update in OVS and TC's
> own place, as these parts are different between OVS and TC.
>
> Note that in OVS nat function it was using skb->protocol to get
> the proto as it already skips vlans in key_extract(), while it
> doesn't in TC, and TC has to call skb_protocol() to get proto.
> So in nf_ct_nat_execute(), we keep using skb_protocol() which
> works for both OVS and TC.
>
> Signed-off-by: Xin Long
> ---
> include/net/netfilter/nf_nat.h | 4 +
> net/netfilter/nf_nat_core.c| 131 +++
> net/openvswitch/conntrack.c| 137 +++--
> net/sched/act_ct.c | 136 +++-
> 4 files changed, 156 insertions(+), 252 deletions(-)
>
> diff --git a/include/net/netfilter/nf_nat.h b/include/net/netfilter/nf_nat.h
> index e9eb01e99d2f..9877f064548a 100644
> --- a/include/net/netfilter/nf_nat.h
> +++ b/include/net/netfilter/nf_nat.h
> @@ -104,6 +104,10 @@ unsigned int
> nf_nat_inet_fn(void *priv, struct sk_buff *skb,
> const struct nf_hook_state *state);
>
> +int nf_ct_nat(struct sk_buff *skb, struct nf_conn *ct,
> + enum ip_conntrack_info ctinfo, int *action,
> + const struct nf_nat_range2 *range, bool commit);
> +
> static inline int nf_nat_initialized(const struct nf_conn *ct,
>enum nf_nat_manip_type manip)
> {
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index e29e4ccb5c5a..1c72b8caa24e 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -784,6 +784,137 @@ nf_nat_inet_fn(void *priv, struct sk_buff *skb,
> }
> EXPORT_SYMBOL_GPL(nf_nat_inet_fn);
>
> +/* Modelled after nf_nat_ipv[46]_fn().
> + * range is only used for new, uninitialized NAT state.
> + * Returns either NF_ACCEPT or NF_DROP.
> + */
> +static int nf_ct_nat_execute(struct sk_buff *skb, struct nf_conn *ct,
> + enum ip_conntrack_info ctinfo, int *action,
> + const struct nf_nat_range2 *range,
> + enum nf_nat_manip_type maniptype)
> +{
> + __be16 proto = skb_protocol(skb, true);
> + int hooknum, err = NF_ACCEPT;
> +
> + /* See HOOK2MANIP(). */
> + if (maniptype == NF_NAT_MANIP_SRC)
> + hooknum = NF_INET_LOCAL_IN; /* Source NAT */
> + else
> + hooknum = NF_INET_LOCAL_OUT; /* Destination NAT */
> +
> + switch (ctinfo) {
> + case IP_CT_RELATED:
> + case IP_CT_RELATED_REPLY:
> + if (proto == htons(ETH_P_IP) &&
> + ip_hdr(skb)->protocol == IPPROTO_ICMP) {
> + if (!nf_nat_icmp_reply_translation(skb, ct, ctinfo,
> +hooknum))
> + err = NF_DROP;
> + goto out;
> + } else if (IS_ENABLED(CONFIG_IPV6) && proto ==
> htons(ETH_P_IPV6)) {
> + __be16 frag_off;
> + u8 nexthdr = ipv6_hdr(skb)->nexthdr;
> + int hdrlen = ipv6_skip_exthdr(skb,
> + sizeof(struct ipv6hdr),
> + , _off);
> +
> + if (hdrlen >= 0 && nexthdr == IPPROTO_ICMPV6) {
> + if (!nf_nat_icmpv6_reply_translation(skb, ct,
> + ctinfo,
> + hooknum,
> + hdrlen))
> + err = NF_DROP;
> + goto out;
> + }
> + }
> + /* Non-ICMP, fall thru to initialize if needed. */
> + fallthrough;
> + case IP_CT_NEW:
> + /* Seen it before? This can happen for loopback, retrans,
> + * or local packets.
> + */
> + if (!nf_nat_initialized(ct, maniptype)) {
> + /* Initialize according to the NAT action. */
> + err = (range && range->flags & NF_NAT_RANGE_MAP_IPS)
> + /* Action is set up to establish a new
> + * mapping.
> + */
> + ? nf_nat_setup_info(ct, range, maniptype)
> + : nf_nat_alloc_null_binding(ct, hooknum);
> + if (err != NF_ACCEPT)
> + goto out;
> + }
> + break;
> +
Re: [ovs-dev] [PATCH net-next 2/5] openvswitch: return NF_ACCEPT when OVS_CT_NAT is net set in info nat
Xin Long writes: > Either OVS_CT_SRC_NAT or OVS_CT_DST_NAT is set, OVS_CT_NAT must be > set in info->nat. Thus, if OVS_CT_NAT is not set in info->nat, it > will definitely not do NAT but returns NF_ACCEPT in ovs_ct_nat(). > > This patch changes nothing funcational but only makes this return > earlier in ovs_ct_nat() to keep consistent with TC's processing > in tcf_ct_act_nat(). > > Signed-off-by: Xin Long > --- Acked-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH net-next 1/5] openvswitch: delete the unncessary skb_pull_rcsum call in ovs_ct_nat_execute
Xin Long writes: > The calls to ovs_ct_nat_execute() are as below: > > ovs_ct_execute() > ovs_ct_lookup() > __ovs_ct_lookup() > ovs_ct_nat() > ovs_ct_nat_execute() > ovs_ct_commit() > __ovs_ct_lookup() > ovs_ct_nat() > ovs_ct_nat_execute() > > and since skb_pull_rcsum() and skb_push_rcsum() are already > called in ovs_ct_execute(), there's no need to do it again > in ovs_ct_nat_execute(). > > Signed-off-by: Xin Long > --- LGTM Acked-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] northd: bypass connection tracking for stateless flows when there are LB flows present
Thanks for looking at this, Numan. I haven't worked with the system tests
before, and am having some
issues getting it to run. So, let me get back to you on this.
regards,
-venu
From: Numan Siddique
Sent: Monday, November 14, 2022 9:17 AM
To: Han Zhou
Cc: Venugopal Iyer; d...@openvswitch.org
Subject: Re: [ovs-dev] [PATCH ovn] northd: bypass connection tracking for
stateless flows when there are LB flows present
External email: Use caution opening links or attachments
On Wed, Nov 9, 2022 at 4:15 PM Numan Siddique wrote:
>
> On Wed, Nov 9, 2022 at 4:11 PM Han Zhou wrote:
> >
> > On Tue, Nov 8, 2022 at 7:51 AM venu.iyer wrote:
> > >
> > > Currently, even stateless flows are subject to connection tracking when
> > there are
> > > LB rules (for DNAT). However, if a flow needs to be subjected to LB, then
> > it shouldn't
> > > be configured as stateless.
> > >
> > > A stateless flow means we should not track it, and this change exempts
> > stateless
> > > flows from being tracked regardless of whether LB rules are present or
> > not.
> > >
> > > Signed-off-by: venu.iyer
> > > Acked-by: Han Zhou
I had a quick look and it looks ok to me.
Can you please add system tests to cover some scenarios for this use
case to avoid future regressions ?
Thanks
Numan
> > > ---
> > > northd/northd.c | 24 +-
> > > northd/ovn-northd.8.xml | 56 ++---
> > > ovn-nb.xml | 3 +++
> > > tests/ovn-northd.at | 48 ---
> > > tests/ovn.at| 4 +--
> > > 5 files changed, 69 insertions(+), 66 deletions(-)
> > >
> > > diff --git a/northd/northd.c b/northd/northd.c
> > > index b7388afc5..da4beede6 100644
> > > --- a/northd/northd.c
> > > +++ b/northd/northd.c
> > > @@ -137,8 +137,8 @@ enum ovn_stage {
> > > PIPELINE_STAGE(SWITCH, IN, L2_UNKNOWN,24, "ls_in_l2_unknown")
> > \
> > >
> > \
> > > /* Logical switch egress stages. */
> > \
> > > -PIPELINE_STAGE(SWITCH, OUT, PRE_LB, 0, "ls_out_pre_lb")
> > \
> > > -PIPELINE_STAGE(SWITCH, OUT, PRE_ACL, 1, "ls_out_pre_acl")
> > \
> > > +PIPELINE_STAGE(SWITCH, OUT, PRE_ACL, 0, "ls_out_pre_acl")
> > \
> > > +PIPELINE_STAGE(SWITCH, OUT, PRE_LB, 1, "ls_out_pre_lb")
> > \
> > > PIPELINE_STAGE(SWITCH, OUT, PRE_STATEFUL, 2, "ls_out_pre_stateful")
> > \
> > > PIPELINE_STAGE(SWITCH, OUT, ACL_HINT, 3, "ls_out_acl_hint")
> > \
> > > PIPELINE_STAGE(SWITCH, OUT, ACL, 4, "ls_out_acl")
> > \
> > > @@ -210,6 +210,7 @@ enum ovn_stage {
> > > #define REGBIT_ACL_LABEL "reg0[13]"
> > > #define REGBIT_FROM_RAMP "reg0[14]"
> > > #define REGBIT_PORT_SEC_DROP "reg0[15]"
> > > +#define REGBIT_ACL_STATELESS "reg0[16]"
> > >
> > > #define REG_ORIG_DIP_IPV4 "reg1"
> > > #define REG_ORIG_DIP_IPV6 "xxreg1"
> > > @@ -271,7 +272,7 @@ enum ovn_stage {
> > > * | R0 | REGBIT_{CONNTRACK/DHCP/DNS} | |
> > |
> > > * || REGBIT_{HAIRPIN/HAIRPIN_REPLY} | |
> > |
> > > * || REGBIT_ACL_HINT_{ALLOW_NEW/ALLOW/DROP/BLOCK} | |
> > |
> > > - * || REGBIT_ACL_LABEL | X |
> > |
> > > + * || REGBIT_ACL_{LABEL/STATELESS} | X |
> > |
> > > * ++--+ X |
> > |
> > > * | R1 | ORIG_DIP_IPV4 (>= IN_PRE_STATEFUL) | R |
> > |
> > > * ++--+ E |
> > |
> > > @@ -5677,17 +5678,18 @@ build_stateless_filter(struct ovn_datapath *od,
> > > const struct nbrec_acl *acl,
> > > struct hmap *lflows)
> > > {
> > > +const char *action = REGBIT_ACL_STATELESS" = 1; next;";
> > > if (!strcmp(acl->direction, "from-lport")) {
> > > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_IN_PRE_ACL,
> > > acl->priority + OVN_ACL_PRI_OFFSET,
> > > acl->match,
> > > -"next;",
> > > +action,
> > > >header_);
> > > } else {
> > > ovn_lflow_add_with_hint(lflows, od, S_SWITCH_OUT_PRE_ACL,
> > > acl->priority + OVN_ACL_PRI_OFFSET,
> > > acl->match,
> > > -"next;",
> > > +action,
> > > >header_);
> > > }
> > > }
> > > @@ -5779,6 +5781,10 @@ build_pre_acls(struct ovn_datapath *od, const
> > struct hmap *port_groups,
> > >REGBIT_CONNTRACK_DEFRAG" = 1; next;");
> > > ovn_lflow_add(lflows, od, S_SWITCH_OUT_PRE_ACL, 100, "ip",
> > >REGBIT_CONNTRACK_DEFRAG" = 1; next;");
> >
Re: [ovs-dev] [RFC PATCH] dpdk: Update to use v22.11.
Bleep bloop. Greetings Ian Stokes, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Line is 80 characters long (recommended limit is 79) #112 FILE: Documentation/intro/install/dpdk.rst:63: .. _DPDK requirements: https://doc.dpdk.org/guides-22.11/linux_gsg/sys_reqs.html WARNING: Line is 80 characters long (recommended limit is 79) #156 FILE: Documentation/topics/dpdk/phy.rst:120: .. _dpdk-drivers: https://doc.dpdk.org/guides-22.11/linux_gsg/linux_drivers.html WARNING: Line is 94 characters long (recommended limit is 79) #165 FILE: Documentation/topics/dpdk/phy.rst:238: __ https://doc.dpdk.org/guides-22.11/prog_guide/env_abstraction_layer.html#iova-mode-detection WARNING: Line is 92 characters long (recommended limit is 79) #174 FILE: Documentation/topics/dpdk/phy.rst:270: __ https://doc.dpdk.org/guides-22.11/prog_guide/switch_representation.html#port-representors WARNING: Line is 104 characters long (recommended limit is 79) #183 FILE: Documentation/topics/dpdk/phy.rst:404: .. _bifurcated-drivers: https://doc.dpdk.org/guides-22.11/linux_gsg/linux_drivers.html#bifurcated-driver Lines checked: 557, Warnings: 5, Errors: 0 Please check this out. If you feel there has been an error, please email acon...@redhat.com Thanks, 0-day Robot ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [RFC PATCH] dpdk: Update to use v22.11.
This commit add support to for DPDK v22.11, it includes the following
changes.
1. ci: Reduce DPDK compilation time.
2. system-dpdk: Update vhost tests to be compatible with DPDK 22.07.
http://patchwork.ozlabs.org/project/openvswitch/list/?series=316528
3. system-dpdk: Update vhost tests to be compatible with DPDK 22.07.
http://patchwork.ozlabs.org/project/openvswitch/list/?series=311332
4. netdev-dpdk: Report device bus specific information.
5. netdev-dpdk: Drop reference to Rx header split.
http://patchwork.ozlabs.org/project/openvswitch/list/?series=321808
In addition documentation was also updated in this commit for use with
DPDK v22.11.
For credit all authors of the original commits to 'dpdk-latest' with the
above changes have been added as co-authors for this commit
Signed-off-by: David Marchand
Co-authored-by: David Marchand
Signed-off-by: Sunil Pai G
Co-authored-by: Sunil Pai G
Signed-off-by: Ian Stokes
---
1. Please Note: Although DPDK documentation has been updated in this patch
the resource has not been updated on the DPDK site as of yet, this will
be expected as part of DPDK RC4.
---
.ci/linux-build.sh | 9 -
Documentation/faq/releases.rst | 2 +-
Documentation/intro/install/dpdk.rst | 16
Documentation/topics/dpdk/phy.rst| 8 ++--
NEWS | 18 +
lib/netdev-dpdk.c| 24 ---
rhel/openvswitch-fedora.spec.in | 2 +-
tests/system-dpdk.at | 78 ++--
8 files changed, 69 insertions(+), 88 deletions(-)
diff --git a/.ci/linux-build.sh b/.ci/linux-build.sh
index 23c8bbb7a..4a4dc0fb0 100755
--- a/.ci/linux-build.sh
+++ b/.ci/linux-build.sh
@@ -142,7 +142,7 @@ function install_dpdk()
fi
# No cache or version mismatch.
rm -rf dpdk-dir
-wget https://fast.dpdk.org/rel/dpdk-$1.tar.xz
+wget https://git.dpdk.org/dpdk/snapshot/dpdk-$1.tar.xz
tar xvf dpdk-$1.tar.xz > /dev/null
DIR_NAME=$(tar -tf dpdk-$1.tar.xz | head -1 | cut -f1 -d"/")
mv ${DIR_NAME} dpdk-dir
@@ -160,6 +160,11 @@ function install_dpdk()
# meson verbose outputs.
DPDK_OPTS="$DPDK_OPTS -Ddeveloper_mode=disabled"
+# OVS compilation and "normal" unit tests (run in the CI) do not depend on
+# any DPDK driver being present.
+# We can disable all drivers to save compilation time.
+DPDK_OPTS="$DPDK_OPTS -Ddisable_drivers=*/*"
+
# Install DPDK using prefix.
DPDK_OPTS="$DPDK_OPTS --prefix=$(pwd)/build"
@@ -228,7 +233,7 @@ fi
if [ "$DPDK" ] || [ "$DPDK_SHARED" ]; then
if [ -z "$DPDK_VER" ]; then
-DPDK_VER="21.11.2"
+DPDK_VER="22.11-rc3"
fi
install_dpdk $DPDK_VER
fi
diff --git a/Documentation/faq/releases.rst b/Documentation/faq/releases.rst
index ac0001cd5..e19f54c8f 100644
--- a/Documentation/faq/releases.rst
+++ b/Documentation/faq/releases.rst
@@ -233,7 +233,7 @@ Q: Are all the DPDK releases that OVS versions work with
maintained?
The latest information about DPDK stable and LTS releases can be found
at `DPDK stable`_.
-.. _DPDK stable: http://doc.dpdk.org/guides-21.11/contributing/stable.html
+.. _DPDK stable: http://doc.dpdk.org/guides-22.11/contributing/stable.html
Q: I get an error like this when I configure Open vSwitch:
diff --git a/Documentation/intro/install/dpdk.rst
b/Documentation/intro/install/dpdk.rst
index a284e6851..2193efddc 100644
--- a/Documentation/intro/install/dpdk.rst
+++ b/Documentation/intro/install/dpdk.rst
@@ -42,7 +42,7 @@ Build requirements
In addition to the requirements described in :doc:`general`, building Open
vSwitch with DPDK will require the following:
-- DPDK 21.11.2
+- DPDK 22.11
- A `DPDK supported NIC`_
@@ -59,8 +59,8 @@ vSwitch with DPDK will require the following:
Detailed system requirements can be found at `DPDK requirements`_.
-.. _DPDK supported NIC: https://doc.dpdk.org/guides-21.11/nics/index.html
-.. _DPDK requirements:
https://doc.dpdk.org/guides-21.11/linux_gsg/sys_reqs.html
+.. _DPDK supported NIC: https://doc.dpdk.org/guides-22.11/nics/index.html
+.. _DPDK requirements:
https://doc.dpdk.org/guides-22.11/linux_gsg/sys_reqs.html
.. _dpdk-install:
@@ -73,9 +73,9 @@ Install DPDK
#. Download the `DPDK sources`_, extract the file and set ``DPDK_DIR``::
$ cd /usr/src/
- $ wget https://fast.dpdk.org/rel/dpdk-21.11.2.tar.xz
- $ tar xf dpdk-21.11.2.tar.xz
- $ export DPDK_DIR=/usr/src/dpdk-stable-21.11.2
+ $ wget https://fast.dpdk.org/rel/dpdk-22.11.tar.xz
+ $ tar xf dpdk-22.11.tar.xz
+ $ export DPDK_DIR=/usr/src/dpdk-22.11
$ cd $DPDK_DIR
#. Configure and install DPDK using Meson
@@ -121,7 +121,7 @@ Install DPDK
.. _DPDK sources: http://dpdk.org/rel
.. _DPDK documentation:
- https://doc.dpdk.org/guides-21.11/linux_gsg/build_dpdk.html
+
Re: [ovs-dev] [PATCH net-next 0/5] net: eliminate the duplicate code in the ct nat functions of ovs and tc
On 15 Nov 10:50, Xin Long wrote: The changes in the patchset: "net: add helper support in tc act_ct for ovs offloading" had moved some common ct code used by both OVS and TC into netfilter. There are still some big functions pretty similar defined and used in each of OVS and TC. It is not good to maintain such similar code in 2 places. This patchset is to extract the functions for NAT processing from OVS and TC to netfilter. To make this change clear and safe, this patchset gets the common code out of OVS and TC step by step: The patch 1-4 make some minor changes in OVS and TC to make the NAT code of them completely the same, then the patch 5 moves the common code to the netfilter and exports one function called by each of OVS and TC. not super expert on TC or OVS, but LGTM. Reviewed-by: Saeed Mahameed ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v6] revalidator: add a USDT probe after evaluation when flows are deleted.
On 21 Oct 2022, at 18:35, Kevin Sprague wrote: > During normal operations, it is useful to understand when a particular flow > gets removed from the system. This can be useful when debugging performance > issues tied to ofproto flow changes, trying to determine deployed traffic > patterns, or while debugging dynamic systems where ports come and go. > > Prior to this change, there was a lack of visibility around flow expiration. > The existing debugging infrastructure could tell us when a flow was added to > the datapath, but not when it was removed or why. > > This change introduces a USDT probe at the point where the revalidator > determines that the flow should be removed. Additionally, we track the > reason for the flow eviction and provide that information as well. With > this change, we can track the complete flow lifecycle for the netlink datapath > by hooking the upcall tracepoint in kernel, the flow put USDT, and the > revaldiator USDT, letting us watch as flows are added and removed from the > kernel datapath. > > This change only enables this information via USDT probe, so it won't be > possible to access this information any other way (see: > Documentation/topics/usdt-probes.rst). > > Also included is a script (utilities/usdt-scripts/flow_reval_monitor.py) that > serves as a demonstration of how the new USDT probe might be used going > forward. > > Change since v5: fixed author information. > > Signed-off-by: Kevin Sprague Hi Kevin, Most of the changes look fine to me, however, there are still a lot of crashes in the filter code. Also for now including the OVS data structures in the script will work for now. If we do not get a solution before this gets merged, I’ll fix up all the scripts that need this later. About the crash, it has to do with when we do not receive any uuid/key (I think I did not research). But if I start the script, and do the following (RHEL8): ovs-vsctl del-br br-int ovs-vsctl add-br br-int Now I get this: TIME UFID EVENT/REASON 5361884.255647616 ufid:---- Insert (put) flow to kernel. 5361884.255689699 ufid:---- Insert (put) flow to kernel. 5361884.255712148 ufid:---- Insert (put) flow to kernel. 5361884.255734158 ufid:---- Insert (put) flow to kernel. 5361884.255753341 ufid:---- Insert (put) flow to kernel. 5361884.255772079 ufid:---- Insert (put) flow to kernel. 5361884.255805591 ufid:2876428c-567e-429c-9dc3-d83503f1 Insert (put) flow to kernel. 5361884.255832007 ufid:---- Insert (put) flow to kernel. 5361884.255852449 ufid:---- Insert (put) flow to kernel. 5361884.255871090 ufid:---- Insert (put) flow to kernel. 5361884.255889960 ufid:---- Insert (put) flow to kernel. 5361884.255909455 ufid:---- Insert (put) flow to kernel. 5361884.255928863 ufid:---- Insert (put) flow to kernel. 5361884.255948291 ufid:---- Insert (put) flow to kernel. So a lot of all 0 ufid’s, did not investigate if this is true, or a script error. Now if I use the script with the -k option: $ ./flow_reval_monitor.py -k TIME UFID EVENT/REASON Traceback (most recent call last): File "_ctypes/callbacks.c", line 234, in 'calling callback function' File "/usr/lib/python3.6/site-packages/bcc/table.py", line 1068, in ringbuf_cb_ ret = callback(ctx, data, size) File "./flow_reval_monitor.py", line 502, in handle_event handle_flow_put(event) File "./flow_reval_monitor.py", line 227, in handle_flow_put key = decode_key(bytes(event.key)[:event.key_size]) File "./flow_reval_monitor.py", line 328, in decode_key result[get_ovs_key_attr_str(nla_type)] = nla_data File "./flow_reval_monitor.py", line 373, in get_ovs_key_attr_str return ovs_key_attr[attr] IndexError: list index out of range Same thing if I try to use a filter option: [wsfd-netdev64:~/...ilities/usdt-scripts]$ ./flow_reval_monitor.py -f ipv6 TIME UFID EVENT/REASON Traceback (most recent call last): File "_ctypes/callbacks.c", line 234, in 'calling callback function' File "/usr/lib/python3.6/site-packages/bcc/table.py", line 1068, in ringbuf_cb_ ret = callback(ctx, data, size) File "./flow_reval_monitor.py", line 502, in handle_event handle_flow_put(event) File "./flow_reval_monitor.py", line 227, in handle_flow_put key = decode_key(bytes(event.key)[:event.key_size]) File "./flow_reval_monitor.py", line 328, in
Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to include latest fixes.
On 11/16/22 15:43, Dumitru Ceara wrote: > On 11/16/22 15:40, Mark Michelson wrote: >> Acked-by: Mark Michelson >> > Thanks! > I pushed this to the main branch. Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to include latest fixes.
On 11/16/22 15:40, Mark Michelson wrote: > Acked-by: Mark Michelson > Thanks! > Which branches of OVN does this need to be backported to? > Only the main branch. The other stable branches use branch-3.0. So we need separate patches for them. I can prepare them if needed. > On 11/16/22 07:37, Dumitru Ceara wrote: >> Specifically we care about: >> 02be2c318 netdev-linux: Fix inability to apply QoS on ports with >> custom qdiscs. >> b5d972299 Add support for OpenSSL 3.0 functions. >> 1a9482d53 dhparams: Fix .c file generation with OpenSSL >= 3.0. >> >> That's because OVN still configures QoS directly and because on newer >> distros compilation fails due to: >> >> lib/stream-ssl.c: In function 'do_ssl_init': >> lib/stream-ssl.c:1073:5: error: 'SSL_CTX_set_tmp_dh_callback' is >> deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations] >> 1073 | SSL_CTX_set_tmp_dh_callback(ctx, tmp_dh_callback); >> | ^~~ >> >> Signed-off-by: Dumitru Ceara >> --- >> ovs | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/ovs b/ovs >> index 5a686267d3..bb9fedb79a 16 >> --- a/ovs >> +++ b/ovs >> @@ -1 +1 @@ >> -Subproject commit 5a686267d36c5c4229ec801a9616ceb60740fbe3 >> +Subproject commit bb9fedb79af8df5f14922ae588866314a0e31bf5 > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 8/8] debian, rhel: Enable Werror option in spec files
On 16/11/2022 16:20, Ilya Maximets wrote:
> On 11/16/22 11:47, Eli Britstein wrote:
>> After resolving DPDK cast align warnings as stated in [1], and
>> resolving some more warnings in OVS side, enforce -Werror for debian and
>> rhel builds too.
>>
>> [1] 0b6d2faace76 ("ci: Remove -Wno-cast-align from CI.")
>>
>> Signed-off-by: Eli Britstein
>> ---
>> debian/rules | 4 ++--
>> rhel/openvswitch.spec.in | 2 +-
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> Hi, Eli. Thanks for fixing some warnings.
> But I don't think we can enable Werror for the distribution builds
> that easily. The main issue here is that we do not test on all the
> versions of different distributions. We only cover Ubuntu 20.04,
> for example, in our CI. Not 22.04 and not any version of Debian.
>
> And it's not really about cast-align, it's more about compiler flags
> imposed by the distribution. You may notice that on Ubuntu -flto
> is enabled by default and lots of other flags. We can't control
> that and we can't protect ourselves from future failures, since these
> flags are different from one version of the distribution to another.
>
> Also, having -Werror will likely mean issues for users who may try
> building OVS packages from slightly outdated branches or for slightly
> outdated or instead newer versions of distributions. Such users
> may not know enough about build process to fix unforeseen issues
> themselves. Especially because these are mostly false-positive
> warnings triggered due to obscure link time optimizations or even
> compiler bugs as demonstrated in this patch set.
>
> So, I'm not sure if enabling -Werror is a good thing. At least until
> we test on all supported versions of Ubuntu/Debian, which is not
> really something we should do, IMO.
>
> Also, some of the fixes in this patch set are fairly questionable.
> I'd argue that we should not replace the memset() in the netlink code.
> At least not without a big comment in the code on what is happening.
> And moving around reset of the ol_flags is also questionable as
> this is not an implementation-specific field.
>
>
> CC: Frode, as he may also have some opinion on that topic.
>
> Best regards, Ilya Maximets.
>
>
> P.S. I'm traveling this week, so there could be big delay between
> my replies.
>
Hi Ilya,
I have something to add here. without getting into the questionable fixes
as you mentioned. this is the upstream repository. Aren't the spec file
here and debian files are for a working example?
Each distribution usually maintain their own repository where they take
some stable ovs version, add more recent fixes, revert some stuff maybe,
and probably have their own spec file or debian packaging files.
So what's wrong with upstream repository having it's own default
with -Werror ?
Changing the spec/debian here should not and probably is not affecting
a distribution release version.
Thanks,
Roi
>>
>> diff --git a/debian/rules b/debian/rules
>> index 971bc1775..ffc218e9d 100755
>> --- a/debian/rules
>> +++ b/debian/rules
>> @@ -24,7 +24,7 @@ override_dh_auto_configure:
>> cd _debian && ( \
>> test -e Makefile || \
>> ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
>> - --sysconfdir=/etc \
>> + --sysconfdir=/etc --enable-Werror \
>> $(DATAPATH_CONFIGURE_OPTS) \
>> $(EXTRA_CONFIGURE_OPTS) \
>> )
>> @@ -34,7 +34,7 @@ ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
>> cd _dpdk && ( \
>> test -e Makefile || \
>> ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
>> - --with-dpdk=shared --sysconfdir=/etc \
>> + --with-dpdk=shared --sysconfdir=/etc --enable-Werror \
>> $(DATAPATH_CONFIGURE_OPTS) \
>> $(EXTRA_CONFIGURE_OPTS) \
>> )
>> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
>> index 9903dd10a..35ae42356 100644
>> --- a/rhel/openvswitch.spec.in
>> +++ b/rhel/openvswitch.spec.in
>> @@ -70,7 +70,7 @@ Tailored Open vSwitch SELinux policy
>>
>> %build
>> ./configure --prefix=/usr --sysconfdir=/etc
>> --localstatedir=%{_localstatedir} \
>> ---libdir=%{_libdir} --enable-ssl --enable-shared
>> +--libdir=%{_libdir} --enable-ssl --enable-shared --enable-Werror
>> make %{_smp_mflags}
>> make selinux-policy
>>
>
> ___
> dev mailing list
> d...@openvswitch.org
>
Re: [ovs-dev] [PATCH ovn] ovs: Bump submodule to include latest fixes.
Acked-by: Mark Michelson Which branches of OVN does this need to be backported to? On 11/16/22 07:37, Dumitru Ceara wrote: Specifically we care about: 02be2c318 netdev-linux: Fix inability to apply QoS on ports with custom qdiscs. b5d972299 Add support for OpenSSL 3.0 functions. 1a9482d53 dhparams: Fix .c file generation with OpenSSL >= 3.0. That's because OVN still configures QoS directly and because on newer distros compilation fails due to: lib/stream-ssl.c: In function 'do_ssl_init': lib/stream-ssl.c:1073:5: error: 'SSL_CTX_set_tmp_dh_callback' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations] 1073 | SSL_CTX_set_tmp_dh_callback(ctx, tmp_dh_callback); | ^~~ Signed-off-by: Dumitru Ceara --- ovs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovs b/ovs index 5a686267d3..bb9fedb79a 16 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 5a686267d36c5c4229ec801a9616ceb60740fbe3 +Subproject commit bb9fedb79af8df5f14922ae588866314a0e31bf5 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 2/2] ofproto-dpif-xlate: Optimize the clone for patch ports
On 21 Oct 2022, at 8:26, Ales Musil wrote: > When the packet was traveling through patch port boundary > OvS would check if any of the actions is reversible, > if not it would clone the packet. However, the check > was only at the first level of the second bridge. > That caused some issues when the packet had gone > through more actions, some of them might have been > irreversible. > > In order to keep the semantics the same we might > need to run the actions twice in the worst case > scenario. During the clone there are 4 scenarios > that can happen. > > 1) The action is last one for that flow, > in that case we just continue without clone. > > 2) There is irreversible action in the action > set (first level). In this case we know that > there is at leas one irreversible action which > is enough to do clone. > > 3) All actions in first level are reversible, > we can try to run all actions as if we don't > need any clone and inspect the ofpbuf at the > end. In positive case there are no irreversible > actions so we will just submit the buffer and continue. > > 4) This is same as 3) with the difference that > there is irreversible action in the ofpbuf. > To keep the semantics we need to re-run the actions > and treat it as clone. This requires resotration > of the xlate_ctx. > > Add test cases for all irreversible actions > to see if they are properly cloned. > > Signed-off-by: Ales Musil > --- Thanks for following up with all the revisions ;) This version looks fine to me, and it is passing some basic testing. Thanks, Eelco Acked-by: Eelco Chaudron ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH V2 8/8] debian, rhel: Enable Werror option in spec files
On 11/16/22 11:47, Eli Britstein wrote:
> After resolving DPDK cast align warnings as stated in [1], and
> resolving some more warnings in OVS side, enforce -Werror for debian and
> rhel builds too.
>
> [1] 0b6d2faace76 ("ci: Remove -Wno-cast-align from CI.")
>
> Signed-off-by: Eli Britstein
> ---
> debian/rules | 4 ++--
> rhel/openvswitch.spec.in | 2 +-
> 2 files changed, 3 insertions(+), 3 deletions(-)
Hi, Eli. Thanks for fixing some warnings.
But I don't think we can enable Werror for the distribution builds
that easily. The main issue here is that we do not test on all the
versions of different distributions. We only cover Ubuntu 20.04,
for example, in our CI. Not 22.04 and not any version of Debian.
And it's not really about cast-align, it's more about compiler flags
imposed by the distribution. You may notice that on Ubuntu -flto
is enabled by default and lots of other flags. We can't control
that and we can't protect ourselves from future failures, since these
flags are different from one version of the distribution to another.
Also, having -Werror will likely mean issues for users who may try
building OVS packages from slightly outdated branches or for slightly
outdated or instead newer versions of distributions. Such users
may not know enough about build process to fix unforeseen issues
themselves. Especially because these are mostly false-positive
warnings triggered due to obscure link time optimizations or even
compiler bugs as demonstrated in this patch set.
So, I'm not sure if enabling -Werror is a good thing. At least until
we test on all supported versions of Ubuntu/Debian, which is not
really something we should do, IMO.
Also, some of the fixes in this patch set are fairly questionable.
I'd argue that we should not replace the memset() in the netlink code.
At least not without a big comment in the code on what is happening.
And moving around reset of the ol_flags is also questionable as
this is not an implementation-specific field.
CC: Frode, as he may also have some opinion on that topic.
Best regards, Ilya Maximets.
P.S. I'm traveling this week, so there could be big delay between
my replies.
>
> diff --git a/debian/rules b/debian/rules
> index 971bc1775..ffc218e9d 100755
> --- a/debian/rules
> +++ b/debian/rules
> @@ -24,7 +24,7 @@ override_dh_auto_configure:
> cd _debian && ( \
> test -e Makefile || \
> ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
> - --sysconfdir=/etc \
> + --sysconfdir=/etc --enable-Werror \
>$(DATAPATH_CONFIGURE_OPTS) \
>$(EXTRA_CONFIGURE_OPTS) \
>)
> @@ -34,7 +34,7 @@ ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
> cd _dpdk && ( \
> test -e Makefile || \
> ../configure --prefix=/usr --localstatedir=/var --enable-ssl \
> - --with-dpdk=shared --sysconfdir=/etc \
> + --with-dpdk=shared --sysconfdir=/etc --enable-Werror \
>$(DATAPATH_CONFIGURE_OPTS) \
>$(EXTRA_CONFIGURE_OPTS) \
>)
> diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
> index 9903dd10a..35ae42356 100644
> --- a/rhel/openvswitch.spec.in
> +++ b/rhel/openvswitch.spec.in
> @@ -70,7 +70,7 @@ Tailored Open vSwitch SELinux policy
>
> %build
> ./configure --prefix=/usr --sysconfdir=/etc
> --localstatedir=%{_localstatedir} \
> ---libdir=%{_libdir} --enable-ssl --enable-shared
> +--libdir=%{_libdir} --enable-ssl --enable-shared --enable-Werror
> make %{_smp_mflags}
> make selinux-policy
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 10/15] test: tc does not support conntrack timeout, skip the related test.
On 16 Nov 2022, at 14:32, Roi Dayan wrote: > On 15/11/2022 14:32, Eelco Chaudron wrote: >> The tc conntrack implementation does not support the timeout option. >> The current implementation is silently ignoring the timeout option >> by adding a general conntrack entry. >> >> This patch will skip the related test by overriding the support macro. >> >> Signed-off-by: Eelco Chaudron >> Acked-by: Roi Dayan >> --- >> tests/system-offloads.at |1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/tests/system-offloads.at b/tests/system-offloads.at >> index c8d1fd183..a9bfda164 100644 >> --- a/tests/system-offloads.at >> +++ b/tests/system-offloads.at >> @@ -51,7 +51,6 @@ conntrack - IPv4 fragmentation with fragments specified >> conntrack - IPv6 fragmentation + cvlan >> conntrack - Fragmentation over vxlan >> conntrack - IPv6 Fragmentation over vxlan >> -conntrack - zone-based timeout policy >> conntrack - multiple zones, local >> conntrack - multi-stage pipeline, local >> conntrack - ICMP related with NAT >> > > the commit is missing the override macro for offloads > in system-offloads.at > > > +# Conntrack timeout not supported for tc. > +m4_define([CHECK_CONNTRACK_TIMEOUT], > +[ > + AT_SKIP_IF([:]) > +]) > + Whoops, I messed up merging this to the new file :( Odd that the test where not failing, guess because I was only doing single runs :( Let me know once you are done with the series and do a long run over the weekend. Cheers, Eelco ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v3 2/2] northd: Add I-P for syncing SB address sets.
On Tue, Nov 15, 2022 at 8:21 PM Mark Michelson wrote:
> Thanks, Numan.
>
> Acked-by: Mark Michelson
>
> On 11/15/22 10:19, num...@ovn.org wrote:
> > From: Numan Siddique
> >
> > Updates to NB address sets and NB port groups are handled
> > incrementally for syncing the SB address sets. This patch
> > doesn't support syncing the SB Address sets for the router
> > load balancer vips incrementally, instead a full recompute is
> > triggered for any changes to NB load balancers, NB load balancer
> > groups and NB logical routers.
> >
> > Signed-off-by: Numan Siddique
> > ---
> > northd/en-sb-sync.c | 202 ---
> > northd/en-sb-sync.h | 6 ++
> > northd/inc-proc-northd.c | 18 +++-
> > tests/ovn-northd.at | 52 ++
> > 4 files changed, 260 insertions(+), 18 deletions(-)
> >
> > diff --git a/northd/en-sb-sync.c b/northd/en-sb-sync.c
> > index c3ba315df..e9ce3cec3 100644
> > --- a/northd/en-sb-sync.c
> > +++ b/northd/en-sb-sync.c
> > @@ -22,6 +22,7 @@
> > #include "openvswitch/util.h"
> >
> > #include "en-sb-sync.h"
> > +#include "include/ovn/expr.h"
> > #include "lib/inc-proc-eng.h"
> > #include "lib/lb.h"
> > #include "lib/ovn-nb-idl.h"
> > @@ -41,6 +42,13 @@ static void sync_address_sets(const struct
> nbrec_address_set_table *,
> > const struct sbrec_address_set_table *,
> > struct ovsdb_idl_txn *ovnsb_txn,
> > struct hmap *datapaths);
> > +static const struct sbrec_address_set *sb_address_set_lookup_by_name(
> > +struct ovsdb_idl_index *, const char *name);
> > +static void update_sb_addr_set(const char **nb_addresses, size_t
> n_addresses,
> > + const struct sbrec_address_set *);
> > +static void build_port_group_address_set(const struct nbrec_port_group
> *,
> > + struct svec *ipv4_addrs,
> > + struct svec *ipv6_addrs);
> >
> > void *
> > en_sb_sync_init(struct engine_node *node OVS_UNUSED,
> > @@ -94,6 +102,98 @@ en_address_set_sync_cleanup(void *data OVS_UNUSED)
> >
> > }
> >
> > +bool
> > +address_set_sync_nb_address_set_handler(struct engine_node *node
> OVS_UNUSED,
> > +void *data OVS_UNUSED)
> > +{
> > +const struct nbrec_address_set_table *nb_address_set_table =
> > +EN_OVSDB_GET(engine_get_input("NB_address_set", node));
> > +
> > +/* Return false if an address set is created or deleted.
> > + * Handle I-P for only updated address sets. */
> > +const struct nbrec_address_set *nb_addr_set;
> > +NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (nb_addr_set,
> > + nb_address_set_table) {
> > +if (nbrec_address_set_is_new(nb_addr_set) ||
> > +nbrec_address_set_is_deleted(nb_addr_set)) {
> > +return false;
> > +}
> > +}
> > +
> > +struct ovsdb_idl_index *sbrec_address_set_by_name =
> > +engine_ovsdb_node_get_index(
> > +engine_get_input("SB_address_set", node),
> > +"sbrec_address_set_by_name");
> > +
> > +NBREC_ADDRESS_SET_TABLE_FOR_EACH_TRACKED (nb_addr_set,
> > + nb_address_set_table) {
> > +const struct sbrec_address_set *sb_addr_set =
> > +sb_address_set_lookup_by_name(sbrec_address_set_by_name,
> > + nb_addr_set->name);
> > +if (!sb_addr_set) {
> > +return false;
> > +}
> > +update_sb_addr_set((const char **) nb_addr_set->addresses,
> > + nb_addr_set->n_addresses, sb_addr_set);
> > +}
> > +
> > +return true;
> > +}
> > +
> > +bool
> > +address_set_sync_nb_port_group_handler(struct engine_node *node
> OVS_UNUSED,
> > + void *data OVS_UNUSED)
> > +{
> > +const struct nbrec_port_group *nb_pg;
> > +const struct nbrec_port_group_table *nb_port_group_table =
> > +EN_OVSDB_GET(engine_get_input("NB_port_group", node));
> > +NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg,
> nb_port_group_table) {
> > +if (nbrec_port_group_is_new(nb_pg) ||
> > +nbrec_port_group_is_deleted(nb_pg)) {
> > +return false;
> > +}
> > +}
> > +
> > +struct ovsdb_idl_index *sbrec_address_set_by_name =
> > +engine_ovsdb_node_get_index(
> > +engine_get_input("SB_address_set", node),
> > +"sbrec_address_set_by_name");
> > +NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg,
> nb_port_group_table) {
> > +char *ipv4_addrs_name = xasprintf("%s_ip4", nb_pg->name);
> > +const struct sbrec_address_set *sb_addr_set_v4 =
> > +sb_address_set_lookup_by_name(sbrec_address_set_by_name,
> > +
Re: [ovs-dev] [PATCH v2 10/15] test: tc does not support conntrack timeout, skip the related test.
On 15/11/2022 14:32, Eelco Chaudron wrote: > The tc conntrack implementation does not support the timeout option. > The current implementation is silently ignoring the timeout option > by adding a general conntrack entry. > > This patch will skip the related test by overriding the support macro. > > Signed-off-by: Eelco Chaudron > Acked-by: Roi Dayan > --- > tests/system-offloads.at |1 - > 1 file changed, 1 deletion(-) > > diff --git a/tests/system-offloads.at b/tests/system-offloads.at > index c8d1fd183..a9bfda164 100644 > --- a/tests/system-offloads.at > +++ b/tests/system-offloads.at > @@ -51,7 +51,6 @@ conntrack - IPv4 fragmentation with fragments specified > conntrack - IPv6 fragmentation + cvlan > conntrack - Fragmentation over vxlan > conntrack - IPv6 Fragmentation over vxlan > -conntrack - zone-based timeout policy > conntrack - multiple zones, local > conntrack - multi-stage pipeline, local > conntrack - ICMP related with NAT > the commit is missing the override macro for offloads in system-offloads.at +# Conntrack timeout not supported for tc. +m4_define([CHECK_CONNTRACK_TIMEOUT], +[ + AT_SKIP_IF([:]) +]) + ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 06/15] tests: Add delay to dump-conntrack for tc test cases.
On 15/11/2022 14:30, Eelco Chaudron wrote: > This patch adds a delay before dumping the conntrack table because with > tc it takes a bit longer before it gets synced. > > Signed-off-by: Eelco Chaudron > --- > tests/system-common-macros.at |3 + > tests/system-offloads.at | 20 > tests/system-traffic.at | 198 > + > 3 files changed, 102 insertions(+), 119 deletions(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index d95d79791..32b9ca0de 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -347,3 +347,6 @@ m4_define([OVS_CHECK_CT_CLEAR], > # OVS_REVALIDATOR_PURGE() > m4_define([OVS_REVALIDATOR_PURGE], > [AT_CHECK([ovs-appctl revalidator/purge], [0])]) > + > +# DPCTL_DUMP_CONNTRACK() > +m4_define([DPCTL_DUMP_CONNTRACK], [ovs-appctl dpctl/dump-conntrack]) > diff --git a/tests/system-offloads.at b/tests/system-offloads.at > index b7e1c8ea2..2081e45b4 100644 > --- a/tests/system-offloads.at > +++ b/tests/system-offloads.at > @@ -36,18 +36,9 @@ m4_define([OVS_TEST_SKIP_LIST], > [ovs_test_skip_list=" > datapath - truncate and output to gre tunnel by simulated packets > datapath - truncate and output to gre tunnel > -conntrack - preserve registers > -conntrack - zones > -conntrack - zones from field > conntrack - zones from other field > conntrack - zones from other field, more tests > -conntrack - multiple zones > conntrack - multiple namespaces, internal ports > -conntrack - ct_mark > -conntrack - ct_mark bit-fiddling > -conntrack - ct_mark from register > -conntrack - ct_label > -conntrack - ct_label bit-fiddling > conntrack - ct metadata, multiple zones > conntrack - ICMP related > conntrack - ICMP related to original direction > @@ -57,8 +48,6 @@ conntrack - IPv6 fragmentation + cvlan > conntrack - Fragmentation over vxlan > conntrack - IPv6 Fragmentation over vxlan > conntrack - zone-based timeout policy > -conntrack - IPv4 HTTP > -conntrack - IPv6 HTTP > conntrack - multiple zones, local > conntrack - multi-stage pipeline, local > conntrack - FTP > @@ -66,14 +55,6 @@ conntrack - FTP over IPv6 > conntrack - IPv6 FTP Passive > conntrack - FTP with multiple expectations > conntrack - TFTP > -conntrack - simple SNAT > -conntrack - SNAT with port range > -conntrack - SNAT with port range with exhaustion > -conntrack - more complex SNAT > -conntrack - all-zero IP SNAT > -conntrack - simple DNAT > -conntrack - DNAT with additional SNAT > -conntrack - more complex DNAT > conntrack - ICMP related with NAT > conntrack - FTP SNAT prerecirc > conntrack - FTP SNAT prerecirc seqadj > @@ -86,7 +67,6 @@ conntrack - IPv4 FTP Passive with DNAT > conntrack - IPv4 FTP Passive with DNAT 2 > conntrack - IPv4 FTP Active with DNAT > conntrack - IPv4 FTP Active with DNAT with reverse skew > -conntrack - IPv6 HTTP with DNAT > conntrack - IPv6 FTP with SNAT > conntrack - IPv6 FTP Passive with SNAT > conntrack - IPv6 FTP with SNAT - orig tuple > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index 1d0d0dfd5..48545f57d 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -2215,7 +2215,7 @@ > udp,vlan_tci=0x,dl_src=50:54:00:00:00:09,dl_dst=50:54:00:00:00:0a,nw_src=10. > dnl > dnl Check that the directionality has been changed by force commit. > dnl > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.2,"], > [], [dnl > +AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.2,"], [], [dnl > > udp,orig=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1),reply=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2) > ]) > > @@ -2223,7 +2223,7 @@ dnl OK, now send another packet from port 1 and see > that it switches again > AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 > packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008 > actions=resubmit(,0)"]) > OVS_REVALIDATOR_PURGE() > > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], > [], [dnl > +AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl > > udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1) > ]) > > @@ -2253,25 +2253,25 @@ AT_CHECK([ovs-ofctl --bundle add-flows br0 flows.txt]) > dnl Test UDP from port 1 > AT_CHECK([ovs-ofctl -O OpenFlow13 packet-out br0 "in_port=1 > packet=5054000a505400090800451c0011a4cd0a0101010a010102000100020008 > actions=resubmit(,0)"]) > > -AT_CHECK([ovs-appctl dpctl/dump-conntrack | grep "orig=.src=10\.1\.1\.1,"], > [], [dnl > +AT_CHECK([DPCTL_DUMP_CONNTRACK | grep "orig=.src=10\.1\.1\.1,"], [], [dnl > > udp,orig=(src=10.1.1.1,dst=10.1.1.2,sport=1,dport=2),reply=(src=10.1.1.2,dst=10.1.1.1,sport=2,dport=1) > ]) > > AT_CHECK([ovs-appctl dpctl/flush-conntrack >
Re: [ovs-dev] [PATCH v2 04/15] test: Add delay on revalidator flush for offload test cases.
On 15/11/2022 14:29, Eelco Chaudron wrote: > The revalidator/purge commands in the system test cases sometimes > get called immediately after a partial test is completed. This > could cause the revalidator thread to log an error that it can > not find/delete a flow due to the slower flow installation nature > of TC. > > This patch uses a macro to call the revalidator/purge command, > which can be overwritten when the system tests are run on a tc > enabled datapath. > > Signed-off-by: Eelco Chaudron > --- > tests/system-common-macros.at |4 > tests/system-offloads.at |1 - > tests/system-traffic.at | 38 +++--- > 3 files changed, 23 insertions(+), 20 deletions(-) > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 8b9f5c752..d95d79791 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -343,3 +343,7 @@ m4_define([OVS_CHECK_IPROUTE_ENCAP], > # OVS_CHECK_CT_CLEAR() > m4_define([OVS_CHECK_CT_CLEAR], > [AT_SKIP_IF([! grep -q "Datapath supports ct_clear action" > ovs-vswitchd.log])]) > + > +# OVS_REVALIDATOR_PURGE() > +m4_define([OVS_REVALIDATOR_PURGE], > +[AT_CHECK([ovs-appctl revalidator/purge], [0])]) > diff --git a/tests/system-offloads.at b/tests/system-offloads.at > index fbe1dc99a..93e4e93d2 100644 > --- a/tests/system-offloads.at > +++ b/tests/system-offloads.at > @@ -34,7 +34,6 @@ m4_define([OVS_TRAFFIC_VSWITCHD_START], > # issue. > m4_define([OVS_TEST_SKIP_LIST], > [ovs_test_skip_list=" > -datapath - basic truncate action > datapath - truncate and output to gre tunnel by simulated packets > datapath - truncate and output to gre tunnel > conntrack - force commit > diff --git a/tests/system-traffic.at b/tests/system-traffic.at > index cd3ad0f68..1d0d0dfd5 100644 > --- a/tests/system-traffic.at > +++ b/tests/system-traffic.at > @@ -1517,12 +1517,12 @@ on_exit 'rm -f payload200.bin' > NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin]) > > dnl packet with truncated size > -AT_CHECK([ovs-appctl revalidator/purge], [0]) > +OVS_REVALIDATOR_PURGE() > AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n > 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl > n_bytes=100 > ]) > dnl packet with original size > -AT_CHECK([ovs-appctl revalidator/purge], [0]) > +OVS_REVALIDATOR_PURGE() > AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=5" | sed -n > 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl > n_bytes=242 > ]) > @@ -1539,7 +1539,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin]) > > dnl 100 + 100 + 242 + min(65535,242) = 684 > -AT_CHECK([ovs-appctl revalidator/purge], [0]) > +OVS_REVALIDATOR_PURGE() > AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n > 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl > n_bytes=684 > ]) > @@ -1569,7 +1569,7 @@ AT_CHECK([ovs-ofctl add-flows br0 flows.txt]) > NS_CHECK_EXEC([at_ns0], [nc $NC_EOF_OPT -u 10.1.1.2 1234 < payload200.bin]) > > dnl 100 + 100 + 242 + min(65535,242) = 684 > -AT_CHECK([ovs-appctl revalidator/purge], [0]) > +OVS_REVALIDATOR_PURGE() > AT_CHECK([ovs-ofctl dump-flows br0 table=0 | grep "in_port=3" | sed -n > 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl > n_bytes=684 > ]) > @@ -1653,7 +1653,7 @@ AT_CHECK([ovs-ofctl add-flows br-underlay > flows-underlay.txt]) > > dnl check tunnel push path, from at_ns1 to at_ns0 > NS_CHECK_EXEC([at_ns1], [nc $NC_EOF_OPT -u 10.1.1.1 1234 < payload200.bin]) > -AT_CHECK([ovs-appctl revalidator/purge], [0]) > +OVS_REVALIDATOR_PURGE() > > dnl Before truncation = ETH(14) + IP(20) + UDP(8) + 200 = 242B > AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=2" | sed -n > 's/.*\(n\_bytes=[[0-9]]*\).*/\1/p'], [0], [dnl > @@ -1669,7 +1669,7 @@ dnl This 200-byte packet is simulated on behalf of > ns_gre0 > ovs-ofctl -O OpenFlow13 packet-out br-underlay "in_port=1 > packet=02908ca8a149faadfa25056008004500010a9e9d4000402f4084ac1f0101ac1f01646558e666c122e666c111080045e46f8e40004011b4760a0101010a010102e026162e00d016e6a366ebf904c74132c6fed42a9e9e46240b4d9fd13c9b47d9704a388e70a5e77db16934a6188dc01d86aa20007ace2cf9cdb111f208474b88ffc851c871f0e3fb4fff138c1d288d437efff487e2b86a9c99fbf4229a6485e133bcf3e16f6e345207fda0932d9eeb602740456fd077b4847d25481337bd716155cc245be129ccc11bf82b834767b3760b52fe913c0e24f31c0e1b27f88acf7bba6b985fb64ee2cd6fc6bba1a9c1f021e253e1728b046fd4d023307e3296361a37ea2617ebcb2537e0284a81050dd0ee > actions=LOCAL" > > dnl After truncation = 100 byte at loopback device p2(4) > -AT_CHECK([ovs-appctl revalidator/purge], [0]) > +OVS_REVALIDATOR_PURGE() > AT_CHECK([ovs-ofctl dump-flows br0 | grep "in_port=4" | ofctl_strip], [0], > [dnl > n_packets=1, n_bytes=100, priority=1,ip,in_port=4 actions=drop > ]) > @@ -1690,7 +1690,7 @@ AT_CHECK([ovs-ofctl add-flows br-underlay >
Re: [ovs-dev] [OVN v13] OVN - Add Support for Remote Port Mirroring
Hi Ihar, Thanks for your inputs. I think I have found the issue in the test. In the test case I had missed one thing. Below are the details The log which you shared as the last message in ovs-vswitchd log led me to it. > 2022-11-15T16:31:05.280Z|00541|ofproto_dpif_xlate|WARN|over max > translation depth 64 on bridge br-int while processing > arp,in_port=LOCAL,vlan_tci=0x,dl_src=00:00:00:01:02:00,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.1.11,arp_tpa=192.168.1.12,arp_op=1,arp_sha=00:00:00:01:02:00,arp_tha=00:00:00:00:00:00 What I had missed is, the creation of hv2, setting ip to br-phys and ARP resolving. (See code at the end for what I have added). Once I added this now everything is passing fine. I don't see any failures. Also everything works in a single test case. So, I will revert back to the old method of having 1 bulk updates test case and verify all cases there and with below code, will submit a patch soon. Since in the bulk updates test case we were not verifying the actual packet flow (already covered in a separate test) I had thought the below code was not needed. But that wasn't the case! FYI, what I added now... net_add n2 sim_add hv2 as hv2 ovs-vsctl add-br br-phys -- set bridge br-phys other-config:hwaddr=\"00:00:00:02:02:00\" ovn_attach n2 br-phys 192.168.1.12 OVN_POPULATE_ARP Thanks & Regards, Abhiram R N On Wed, Nov 16, 2022 at 4:39 AM Ihar Hrachyshka wrote: > On 11/15/22 5:42 PM, Ihar Hrachyshka wrote: > > I think there's a problem with the bulk tests added in this patch. I > > will cover this issue in this email, and I'll send my code review > > tomorrow as promised, since it's getting late here. > > > > > > When running the whole suite locally, I get the following failures: > > > > > > 401: Mirror test bulk swap attachments -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=yes FAILED (ovn.at:16425) > > 402: Mirror test bulk swap attachments -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=no FAILED (ovn.at:16425) > > 403: Mirror test bulk attach multiple -- ovn-northd -- > > parallelization=yes -- ovn_monitor_all=yes FAILED (ovn.at:16537) > > 410: Mirror test bulk more detach and less attach -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=no ok > > 412: Mirror test bulk attach more than detach -- ovn-northd -- > > parallelization=yes -- ovn_monitor_all=no ok > > 416: Mirror test bulk detach multiple -- ovn-northd -- > > parallelization=yes -- ovn_monitor_all=no ok > > 408: Mirror test bulk more detach and less attach -- ovn-northd -- > > parallelization=yes -- ovn_monitor_all=no FAILED (ovn.at:16650) > > 417: Mirror test bulk detach multiple -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=yes ok > > 409: Mirror test bulk more detach and less attach -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=yes FAILED (ovn.at:16650) > > 411: Mirror test bulk attach more than detach -- ovn-northd -- > > parallelization=yes -- ovn_monitor_all=yes FAILED (ovn.at:16766) > > 418: Mirror test bulk detach multiple -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=no ok > > 413: Mirror test bulk attach more than detach -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=yes FAILED (ovn.at:16766) > > 415: Mirror test bulk detach multiple -- ovn-northd -- > > parallelization=yes -- ovn_monitor_all=yes FAILED (ovn.at:16879) > > 414: Mirror test bulk attach more than detach -- ovn-northd -- > > parallelization=no -- ovn_monitor_all=no FAILED (ovn.at:16766) > > > > > > Note that some of the test case variants passed, and I don't think > > there's a clear pattern as to which of variants in the test matrix do > > fail. > > > > > > The error that triggers the failure is during ovs-vswitchd cleanup: > > > > > > ./ovn.at:16425: ovs-appctl --timeout=10 -t ovs-vswitchd exit --cleanup > > --- /dev/null 2022-11-04 04:09:25.869645998 + > > +++ /home/vagrant/ovn/tests/testsuite.dir/at-groups/401/stderr > > 2022-11-15 16:31:15.557479369 + > > @@ -0,0 +1,2 @@ > > +2022-11-15T16:31:15Z|1|fatal_signal|WARN|terminating with signal > > 14 (Alarm clock) > > +/home/vagrant/ovn/tests/testsuite.dir/at-groups/401/test-source: line > > 282: 1033659 Alarm clock ovs-appctl --timeout=10 -t > > ovs-vswitchd exit --cleanup > > ./ovn.at:16425: exit code was 142, expected 0 > > > > > > The very last message in ovs-vswitchd log on hv1 is exactly 10 seconds > > before the alarm clock error: > > > > > > 2022-11-15T16:31:05.280Z|00541|ofproto_dpif_xlate|WARN|over max > > translation depth 64 on bridge br-int while processing > > > arp,in_port=LOCAL,vlan_tci=0x,dl_src=00:00:00:01:02:00,dl_dst=ff:ff:ff:ff:ff:ff,arp_spa=192.168.1.11,arp_tpa=192.168.1.12,arp_op=1,arp_sha=00:00:00:01:02:00,arp_tha=00:00:00:00:00:00 > > > > > > I don't see coredumps generated for any of test processes, so it's > > probably not the case of ovs-vswitchd crashing on exit request. > > > > > > I tried to adjust your test cases to a minimal reproducer and I
[ovs-dev] [PATCH ovn] ovs: Bump submodule to include latest fixes.
Specifically we care about: 02be2c318 netdev-linux: Fix inability to apply QoS on ports with custom qdiscs. b5d972299 Add support for OpenSSL 3.0 functions. 1a9482d53 dhparams: Fix .c file generation with OpenSSL >= 3.0. That's because OVN still configures QoS directly and because on newer distros compilation fails due to: lib/stream-ssl.c: In function 'do_ssl_init': lib/stream-ssl.c:1073:5: error: 'SSL_CTX_set_tmp_dh_callback' is deprecated: Since OpenSSL 3.0 [-Werror=deprecated-declarations] 1073 | SSL_CTX_set_tmp_dh_callback(ctx, tmp_dh_callback); | ^~~ Signed-off-by: Dumitru Ceara --- ovs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ovs b/ovs index 5a686267d3..bb9fedb79a 16 --- a/ovs +++ b/ovs @@ -1 +1 @@ -Subproject commit 5a686267d36c5c4229ec801a9616ceb60740fbe3 +Subproject commit bb9fedb79af8df5f14922ae588866314a0e31bf5 -- 2.31.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 7/8] ofproto: Fix 'reply.type' may be used uninitialized
../ofproto/ofproto.c: In function 'handle_openflow':
../lib/ofp-bundle.c:195:15: error: 'reply.type' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
195 | m->type = htons(msg->type);
| ^
../ofproto/ofproto.c:8460:36: note: 'reply.type' was declared here
8460 | struct ofputil_bundle_ctrl_msg reply;
|^
Fixes: 777af88d50b8 ("Add basic implementation for OpenFlow 1.4 bundles")
Signed-off-by: Eli Britstein
---
ofproto/ofproto.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 3a527683c..5ba1b55fb 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -8471,6 +8471,7 @@ handle_bundle_control(struct ofconn *ofconn, const struct
ofp_header *oh)
return error;
}
reply.flags = 0;
+reply.type = 0;
reply.bundle_id = bctrl.bundle_id;
switch (bctrl.type) {
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 6/8] netlink: Fix writing bytes into a region of size 0 overflows the destination
With --enable-Werror and --with-dpdk=no, with gcc (Ubuntu 11.2.0-19ubuntu1),
there are the following warnings (errors) emmitted. Those are reported
in [1] to be GCC bug. Workaround it.
In function 'memset',
inlined from 'nl_msg_put_uninit' at ../lib/netlink.c:212:9,
inlined from 'nl_msg_put_unspec_uninit' at ../lib/netlink.c:248:26,
inlined from 'nl_msg_put_unspec' at ../lib/netlink.c:276:11,
inlined from 'nl_msg_put_u8' at ../lib/netlink.c:294:5:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:59:10: error:
'__builtin_memset' writing 3 bytes into a region of size 0 overflows the
destination [-Werror=stringop-overflow=]
59 | return __builtin___memset_chk (__dest, __ch, __len,
| ^
In function 'memset',
inlined from 'nl_msg_put_uninit' at ../lib/netlink.c:212:9,
inlined from 'nl_msg_put_unspec_uninit' at ../lib/netlink.c:248:26,
inlined from 'nl_msg_put_unspec' at ../lib/netlink.c:276:11,
inlined from 'nl_msg_put_u16' at ../lib/netlink.c:302:5:
/usr/include/x86_64-linux-gnu/bits/string_fortified.h:59:10: error:
'__builtin_memset' writing 2 bytes into a region of size 0 overflows the
destination [-Werror=stringop-overflow=]
59 | return __builtin___memset_chk (__dest, __ch, __len,
| ^
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=92718
Signed-off-by: Eli Britstein
---
lib/netlink.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/netlink.c b/lib/netlink.c
index 6215282d6..c30620fdb 100644
--- a/lib/netlink.c
+++ b/lib/netlink.c
@@ -200,6 +200,14 @@ nl_msg_put(struct ofpbuf *msg, const void *data, size_t
size)
memcpy(nl_msg_put_uninit(msg, size), data, size);
}
+static void
+nl_msg_zero_pad(char *pad_ptr, size_t pad_size)
+{
+while (pad_size--) {
+*pad_ptr++ = 0;
+}
+}
+
/* Appends 'size' bytes of data, plus Netlink padding if needed, to the tail
* end of 'msg', reallocating and copying its data if necessary. Returns a
* pointer to the first byte of the new data, which is left uninitialized. */
@@ -208,9 +216,7 @@ nl_msg_put_uninit(struct ofpbuf *msg, size_t size)
{
size_t pad = PAD_SIZE(size, NLMSG_ALIGNTO);
char *p = ofpbuf_put_uninit(msg, size + pad);
-if (pad) {
-memset(p + size, 0, pad);
-}
+nl_msg_zero_pad(p + size, pad);
return p;
}
@@ -231,9 +237,7 @@ nl_msg_push_uninit(struct ofpbuf *msg, size_t size)
{
size_t pad = PAD_SIZE(size, NLMSG_ALIGNTO);
char *p = ofpbuf_push_uninit(msg, size + pad);
-if (pad) {
-memset(p + size, 0, pad);
-}
+nl_msg_zero_pad(p + size, pad);
return p;
}
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 4/8] dpctl: Fix zone/limit may be used uninitialized
With --enable-Werror and --with-dpdk=no:
../lib/dpctl.c: In function 'dpctl_ct_set_limits':
../lib/ct-dpif.c:698:22: error: 'zone' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
698 | zone_limit->zone = zone;
| ^
../lib/dpctl.c:2139:18: note: 'zone' was declared here
2139 | uint16_t zone;
| ^
../lib/ct-dpif.c:699:23: error: 'limit' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
699 | zone_limit->limit = limit;
| ^
../lib/dpctl.c:2140:18: note: 'limit' was declared here
2140 | uint32_t limit;
| ^
Fixes: 4eeec031d4c4 ("dpctl: Implement dpctl commands for conntrack per zone
limit")
Signed-off-by: Eli Britstein
---
lib/dpctl.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 29041fa3e..714bf3c4b 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -2136,8 +2136,9 @@ dpctl_ct_set_limits(int argc, const char *argv[],
/* Parse ct zone limit tuples */
while (i < argc) {
-uint16_t zone;
-uint32_t limit;
+uint32_t limit = 0;
+uint16_t zone = 0;
+
if (!ct_dpif_parse_zone_limit_tuple(argv[i++], , , )) {
error = EINVAL;
goto error;
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 5/8] ovsdb: Fix 'table' may be used uninitialized
With --enable-Werror and --with-dpdk=no:
../ovsdb/ovsdb.c: In function 'ovsdb_schema_from_json':
../ovsdb/ovsdb.c:246:9: error: 'table' may be used uninitialized in this
function [-Werror=maybe-uninitialized]
246 | shash_add(>tables, table->name, table);
| ^
../ovsdb/ovsdb.c:230:36: note: 'table' was declared here
230 | struct ovsdb_table_schema *table;
|^
Fixes: f85f8ebbfac9 ("Initial implementation of OVSDB.")
Signed-off-by: Eli Britstein
---
ovsdb/ovsdb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ovsdb/ovsdb.c b/ovsdb/ovsdb.c
index 1c011fab0..8e0bbc967 100644
--- a/ovsdb/ovsdb.c
+++ b/ovsdb/ovsdb.c
@@ -227,7 +227,7 @@ ovsdb_schema_from_json(const struct json *json, struct
ovsdb_schema **schemap)
schema = ovsdb_schema_create(json_string(name), version,
cksum ? json_string(cksum) : "");
SHASH_FOR_EACH (node, json_object(tables)) {
-struct ovsdb_table_schema *table;
+struct ovsdb_table_schema *table = NULL;
if (node->name[0] == '_') {
error = ovsdb_syntax_error(json, NULL, "names beginning with "
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 8/8] debian, rhel: Enable Werror option in spec files
After resolving DPDK cast align warnings as stated in [1], and
resolving some more warnings in OVS side, enforce -Werror for debian and
rhel builds too.
[1] 0b6d2faace76 ("ci: Remove -Wno-cast-align from CI.")
Signed-off-by: Eli Britstein
---
debian/rules | 4 ++--
rhel/openvswitch.spec.in | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/debian/rules b/debian/rules
index 971bc1775..ffc218e9d 100755
--- a/debian/rules
+++ b/debian/rules
@@ -24,7 +24,7 @@ override_dh_auto_configure:
cd _debian && ( \
test -e Makefile || \
../configure --prefix=/usr --localstatedir=/var --enable-ssl \
---sysconfdir=/etc \
+--sysconfdir=/etc --enable-Werror \
$(DATAPATH_CONFIGURE_OPTS) \
$(EXTRA_CONFIGURE_OPTS) \
)
@@ -34,7 +34,7 @@ ifeq (,$(filter nodpdk, $(DEB_BUILD_OPTIONS)))
cd _dpdk && ( \
test -e Makefile || \
../configure --prefix=/usr --localstatedir=/var --enable-ssl \
- --with-dpdk=shared --sysconfdir=/etc \
+ --with-dpdk=shared --sysconfdir=/etc --enable-Werror \
$(DATAPATH_CONFIGURE_OPTS) \
$(EXTRA_CONFIGURE_OPTS) \
)
diff --git a/rhel/openvswitch.spec.in b/rhel/openvswitch.spec.in
index 9903dd10a..35ae42356 100644
--- a/rhel/openvswitch.spec.in
+++ b/rhel/openvswitch.spec.in
@@ -70,7 +70,7 @@ Tailored Open vSwitch SELinux policy
%build
./configure --prefix=/usr --sysconfdir=/etc --localstatedir=%{_localstatedir} \
---libdir=%{_libdir} --enable-ssl --enable-shared
+--libdir=%{_libdir} --enable-ssl --enable-shared --enable-Werror
make %{_smp_mflags}
make selinux-policy
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 3/8] ovs-ofctl: Fix 'usable_protocols' may be used uninitialized
With --enable-Werror and --with-dpdk=no:
../utilities/ovs-ofctl.c: In function 'ofctl_group_mod_file':
../utilities/ovs-ofctl.c:3107:16: error: 'usable_protocols' may be used
uninitialized in this function [-Werror=maybe-uninitialized]
3107 | protocol = open_vconn_for_flow_mod(remote, ,
usable_protocols);
|^
../utilities/ovs-ofctl.c:3124:27: note: 'usable_protocols' was declared here
3124 | enum ofputil_protocol usable_protocols;
| ^
Fixes: 69185eb25acb ("ovs-ofctl: Only allow usable protocols for group
commands")
Signed-off-by: Eli Britstein
---
utilities/ovs-ofctl.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
index fe9114580..6805140d6 100644
--- a/utilities/ovs-ofctl.c
+++ b/utilities/ovs-ofctl.c
@@ -3121,7 +3121,7 @@ static void
ofctl_group_mod_file(int argc OVS_UNUSED, char *argv[], int command)
{
struct ofputil_group_mod *gms = NULL;
-enum ofputil_protocol usable_protocols;
+enum ofputil_protocol usable_protocols = 0;
size_t n_gms = 0;
char *error;
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 0/8] Fix warnings and enable Werror
Fixing various warnings and enable Werror for debian/rhel builds. Eli Britstein (8): dp-packet: Fix dp-packet may be used initialized ofp-port: Fix 'strnlen' specified bound may exceed source size ovs-ofctl: Fix 'usable_protocols' may be used uninitialized dpctl: Fix zone/limit may be used uninitialized ovsdb: Fix 'table' may be used uninitialized netlink: Fix writing bytes into a region of size 0 overflows the destination ofproto: Fix 'reply.type' may be used uninitialized debian, rhel: Enable Werror option in spec files debian/rules | 4 ++-- lib/dp-packet.c | 1 - lib/dp-packet.h | 7 --- lib/dpctl.c | 5 +++-- lib/netlink.c| 16 ++-- lib/ofp-port.c | 3 ++- ofproto/ofproto.c| 1 + ovsdb/ovsdb.c| 2 +- rhel/openvswitch.spec.in | 2 +- utilities/ovs-ofctl.c| 2 +- 10 files changed, 25 insertions(+), 18 deletions(-) -- 2.26.2.1730.g385c171 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 2/8] ofp-port: Fix 'strnlen' specified bound may exceed source size
With --enable-Werror and --with-dpdk=no:
In function 'ovs_strlcpy',
inlined from 'ovs_strlcpy' at ../lib/util.c:377:1,
inlined from 'ofputil_port_to_string' at ../lib/ofp-port.c:273:9,
inlined from 'ofputil_port_from_string.part.0' at ../lib/ofp-port.c:170:13:
../lib/util.c:380:22: error: 'strnlen' specified bound 15 may exceed source
size 11 [-Werror=stringop-overread]
380 | size_t len = strnlen(src, size - 1);
| ^
Fixes: 28b114322856 ("ofp-util: New function ofputil_port_to_string().")
Signed-off-by: Eli Britstein
---
lib/ofp-port.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/ofp-port.c b/lib/ofp-port.c
index 16d587488..6e884cd3f 100644
--- a/lib/ofp-port.c
+++ b/lib/ofp-port.c
@@ -270,7 +270,8 @@ ofputil_port_to_string(ofp_port_t port,
{
const char *reserved_name = ofputil_port_get_reserved_name(port);
if (reserved_name) {
-ovs_strlcpy(namebuf, reserved_name, bufsize);
+ovs_strlcpy(namebuf, reserved_name, MIN(strlen(reserved_name),
+bufsize));
return;
}
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH V2 1/8] dp-packet: Fix dp-packet may be used initialized
With --enable-Werror and --with-dpdk=no:
In function 'dp_packet_reset_offload',
inlined from 'dp_packet_init__' at ../lib/dp-packet.c:35:5,
inlined from 'dp_packet_use__' at ../lib/dp-packet.c:50:5,
inlined from 'dp_packet_use' at ../lib/dp-packet.c:60:5,
inlined from 'dp_packet_init' at ../lib/dp-packet.c:126:5,
inlined from 'dp_packet_new' at ../lib/dp-packet.c:154:5:
../lib/dp-packet.h:944:32: error: 'MEM[(uint32_t *)p_14 + 16B]' may be used
uninitialized [-Werror=maybe-uninitialized]
944 | *dp_packet_ol_flags_ptr(p) &= ~DP_PACKET_OL_SUPPORTED_MASK;
|^
Fixes: a47e2db209e4 ("dp-packet: Refactor offloading API.")
Signed-off-by: Eli Britstein
---
lib/dp-packet.c | 1 -
lib/dp-packet.h | 7 ---
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/dp-packet.c b/lib/dp-packet.c
index 4538d2a61..f654752a1 100644
--- a/lib/dp-packet.c
+++ b/lib/dp-packet.c
@@ -32,7 +32,6 @@ dp_packet_init__(struct dp_packet *b, size_t allocated, enum
dp_packet_source so
dp_packet_reset_offsets(b);
pkt_metadata_init(>md, 0);
dp_packet_reset_cutlen(b);
-dp_packet_reset_offload(b);
/* Initialize implementation-specific fields of dp_packet. */
dp_packet_init_specific(b);
/* By default assume the packet type to be Ethernet. */
diff --git a/lib/dp-packet.h b/lib/dp-packet.h
index 55eeaab2c..9864dfcbf 100644
--- a/lib/dp-packet.h
+++ b/lib/dp-packet.h
@@ -572,7 +572,8 @@ dp_packet_init_specific(struct dp_packet *p)
{
/* This initialization is needed for packets that do not come from DPDK
* interfaces, when vswitchd is built with --with-dpdk. */
-p->mbuf.ol_flags = p->mbuf.tx_offload = p->mbuf.packet_type = 0;
+*dp_packet_ol_flags_ptr(p) = 0;
+p->mbuf.tx_offload = p->mbuf.packet_type = 0;
p->mbuf.nb_segs = 1;
p->mbuf.next = NULL;
}
@@ -638,9 +639,9 @@ dp_packet_set_allocated(struct dp_packet *b, uint16_t s)
#else /* DPDK_NETDEV */
static inline void
-dp_packet_init_specific(struct dp_packet *p OVS_UNUSED)
+dp_packet_init_specific(struct dp_packet *p)
{
-/* There are no implementation-specific fields for initialization. */
+*dp_packet_ol_flags_ptr(p) = 0;
}
static inline void *
--
2.26.2.1730.g385c171
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v2 2/2] northd: Allow related traffic through LB
On Tue, Nov 15, 2022 at 5:31 PM Mark Michelson wrote:
> Hi Ales, be sure to address the checkpatch errors that 0-day robot
> reported.
>
> Other than that, I have just a couple of comments down below.
>
>
Done in v3.
>
> On 11/4/22 04:18, Ales Musil wrote:
> > In order to allow realted traffic use the
>
> s/realted/related/
>
Done in v3.
>
> > new action ct_commit_nat, which ensures that
> > the traffic is commited and NATted. In combination
> > with match on ct.rel it allows the related traffic
> > to go through with correct NAT being applied.
> >
> > Reported-at: https://bugzilla.redhat.com/2126083
> > Signed-off-by: Ales Musil
> > ---
> > v2: Add e2e test case.
> > ---
> > northd/northd.c | 29 --
> > northd/ovn-northd.8.xml | 19 +++-
> > tests/ovn-northd.at | 208 +---
> > tests/ovn.at| 10 +-
> > tests/system-ovn.at | 128 +
> > 5 files changed, 279 insertions(+), 115 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index b7388afc5..e188e4f6a 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -6686,7 +6686,8 @@ build_acls(struct ovn_datapath *od, const struct
> chassis_features *features,
> > /* Ingress and Egress ACL Table (Priority 65535).
> >*
> >* Allow traffic that is related to an existing conntrack
> entry that
> > - * has not been marked for deletion (ct_mark.blocked).
> > + * has not been marked for deletion (ct_mark.blocked). At the
> same
> > + * time apply NAT on this traffic.
> >*
> >* This is enforced at a higher priority than ACLs can be
> defined.
> >*
> > @@ -6699,9 +6700,9 @@ build_acls(struct ovn_datapath *od, const struct
> chassis_features *features,
> > use_ct_inv_match ? " && !ct.inv" : "",
> > ct_blocked_match);
> > ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
> > - ds_cstr(), "next;");
> > + ds_cstr(), "ct_commit_nat;");
> > ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
> > - ds_cstr(), "next;");
> > + ds_cstr(), "ct_commit_nat;");
> >
> > /* Ingress and Egress ACL Table (Priority 65532).
> >*
> > @@ -10006,16 +10007,16 @@ build_lrouter_nat_flows_for_lb(struct
> ovn_lb_vip *lb_vip,
> > int prio = 110;
> > if (lb_vip->vip_port) {
> > prio = 120;
> > -new_match = xasprintf("ct.new && %s && %s && "
> > +new_match = xasprintf("ct.new && !ct.rel && %s && %s && "
> > REG_ORIG_TP_DPORT_ROUTER" == %d",
> > ds_cstr(match), lb->proto,
> lb_vip->vip_port);
> > -est_match = xasprintf("ct.est && %s && %s && "
> > +est_match = xasprintf("ct.est && !ct.rel && %s && %s && "
> > REG_ORIG_TP_DPORT_ROUTER" == %d && %s ==
> 1",
> > ds_cstr(match), lb->proto,
> lb_vip->vip_port,
> > ct_natted);
> > } else {
> > -new_match = xasprintf("ct.new && %s", ds_cstr(match));
> > -est_match = xasprintf("ct.est && %s && %s == 1",
> > +new_match = xasprintf("ct.new && !ct.rel && %s",
> ds_cstr(match));
> > +est_match = xasprintf("ct.est && !ct.rel && %s && %s == 1",
> > ds_cstr(match), ct_natted);
> > }
> >
> > @@ -13664,6 +13665,20 @@ build_lrouter_nat_defrag_and_lb(struct
> ovn_datapath *od, struct hmap *lflows,
> > ovn_lflow_add(lflows, od, S_ROUTER_OUT_EGR_LOOP, 0, "1", "next;");
> > ovn_lflow_add(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 0, "1",
> "next;");
> >
> > +/* Ingress DNAT Table (Priority 50).
> > + *
> > + * Allow traffic that is related to an existing conntrack entry.
> > + * At the same time apply NAT for this traffic.
> > + *
> > + * NOTE: This does not support related data sessions (eg,
> > + * a dynamically negotiated FTP data channel), but will allow
> > + * related traffic such as an ICMP Port Unreachable through
> > + * that's generated from a non-listening UDP port. */
> > +if (od->has_lb_vip) {
> > +ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
> > + "ct.rel && !ct.est && !ct.new", "ct_commit_nat;");
> > +}
> > +
> > /* If the router has load balancer or DNAT rules, re-circulate
> every packet
> >* through the DNAT zone so that packets that need to be unDNATed
> in the
> >* reverse direction get unDNATed.
> > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > index a70f2e678..175db6130 100644
> > --- a/northd/ovn-northd.8.xml
> > +++ b/northd/ovn-northd.8.xml
> > @@ -766,6 +766,8 @@
> > related to a committed flow in the connection
[ovs-dev] [PATCH ovn v3 2/2] northd: Allow related traffic through LB
In order to allow related traffic use the
new action ct_commit_nat, which ensures that
the traffic is commited and NATted. In combination
with match on ct.rel it allows the related traffic
to go through with correct NAT being applied.
Reported-at: https://bugzilla.redhat.com/2126083
Signed-off-by: Ales Musil
---
v2: Add e2e test case.
v3: Rebase on current main.
Address comments from Mark.
---
northd/northd.c | 29 --
northd/ovn-northd.8.xml | 29 --
tests/ovn-northd.at | 208 +---
tests/ovn.at| 10 +-
tests/system-ovn.at | 135 ++
5 files changed, 291 insertions(+), 120 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index e1f3bace8..8b1655a25 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -6686,7 +6686,8 @@ build_acls(struct ovn_datapath *od, const struct
chassis_features *features,
/* Ingress and Egress ACL Table (Priority 65535).
*
* Allow traffic that is related to an existing conntrack entry that
- * has not been marked for deletion (ct_mark.blocked).
+ * has not been marked for deletion (ct_mark.blocked). At the same
+ * time apply NAT on this traffic.
*
* This is enforced at a higher priority than ACLs can be defined.
*
@@ -6699,9 +6700,9 @@ build_acls(struct ovn_datapath *od, const struct
chassis_features *features,
use_ct_inv_match ? " && !ct.inv" : "",
ct_blocked_match);
ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL, UINT16_MAX - 3,
- ds_cstr(), "next;");
+ ds_cstr(), "ct_commit_nat;");
ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL, UINT16_MAX - 3,
- ds_cstr(), "next;");
+ ds_cstr(), "ct_commit_nat;");
/* Ingress and Egress ACL Table (Priority 65532).
*
@@ -10007,16 +10008,16 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip
*lb_vip,
int prio = 110;
if (lb_vip->vip_port) {
prio = 120;
-new_match = xasprintf("ct.new && %s && %s && "
+new_match = xasprintf("ct.new && !ct.rel && %s && %s && "
REG_ORIG_TP_DPORT_ROUTER" == %d",
ds_cstr(match), lb->proto, lb_vip->vip_port);
-est_match = xasprintf("ct.est && %s && %s && "
+est_match = xasprintf("ct.est && !ct.rel && %s && %s && "
REG_ORIG_TP_DPORT_ROUTER" == %d && %s == 1",
ds_cstr(match), lb->proto, lb_vip->vip_port,
ct_natted);
} else {
-new_match = xasprintf("ct.new && %s", ds_cstr(match));
-est_match = xasprintf("ct.est && %s && %s == 1",
+new_match = xasprintf("ct.new && !ct.rel && %s", ds_cstr(match));
+est_match = xasprintf("ct.est && !ct.rel && %s && %s == 1",
ds_cstr(match), ct_natted);
}
@@ -13665,6 +13666,20 @@ build_lrouter_nat_defrag_and_lb(struct ovn_datapath
*od, struct hmap *lflows,
ovn_lflow_add(lflows, od, S_ROUTER_OUT_EGR_LOOP, 0, "1", "next;");
ovn_lflow_add(lflows, od, S_ROUTER_IN_ECMP_STATEFUL, 0, "1", "next;");
+/* Ingress DNAT Table (Priority 50).
+ *
+ * Allow traffic that is related to an existing conntrack entry.
+ * At the same time apply NAT for this traffic.
+ *
+ * NOTE: This does not support related data sessions (eg,
+ * a dynamically negotiated FTP data channel), but will allow
+ * related traffic such as an ICMP Port Unreachable through
+ * that's generated from a non-listening UDP port. */
+if (od->has_lb_vip) {
+ovn_lflow_add(lflows, od, S_ROUTER_IN_DNAT, 50,
+ "ct.rel && !ct.est && !ct.new", "ct_commit_nat;");
+}
+
/* If the router has load balancer or DNAT rules, re-circulate every packet
* through the DNAT zone so that packets that need to be unDNATed in the
* reverse direction get unDNATed.
diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
index 051f3dc6e..e20fef3bc 100644
--- a/northd/ovn-northd.8.xml
+++ b/northd/ovn-northd.8.xml
@@ -766,6 +766,8 @@
related to a committed flow in the connection tracker (e.g., an
ICMP Port Unreachable from a non-listening UDP port), as long
as the committed flow does not have ct_mark.blocked set.
+This flow also applies NAT to the related traffic so that ICMP headers
+and the inner packet have correct addresses.
If ACL logging and logging of related packets is enabled, then a
companion priority-65533 flow will be installed that accomplishes the
same thing but also logs the traffic.
@@ -3206,10 +3208,10 @@ icmp6 {
Router with gateway port in OVN_Northbound database that
includes a L4 port PORT of protocol P and IPv4
or
[ovs-dev] [PATCH ovn v3 1/2] actions: Add new action called ct_commit_nat
Add action called ct_commit_nat, that performs
NAT while committing the connection. This is
useful for related traffic on which we need
to perform NAT, mainly ICMP. We need to
commit due to design decision of OvS[0]:
"Connections identified as rel are separate from
the originating connection and must be committed separately."
[0] http://www.openvswitch.org/support/dist-docs/ovs-fields.7.txt
Reported-at: https://bugzilla.redhat.com/2126083
Acked-by: Mark Michelson
Signed-off-by: Ales Musil
---
v3: Rebase on current main.
---
include/ovn/actions.h | 3 +++
lib/actions.c | 42 +-
ovn-sb.xml| 12
tests/ovn.at | 5 +
utilities/ovn-trace.c | 34 ++
5 files changed, 95 insertions(+), 1 deletion(-)
diff --git a/include/ovn/actions.h b/include/ovn/actions.h
index d7ee84dac..5814a34aa 100644
--- a/include/ovn/actions.h
+++ b/include/ovn/actions.h
@@ -74,6 +74,7 @@ struct ovn_extend_table;
OVNACT(CT_LB_MARK,ovnact_ct_lb) \
OVNACT(SELECT,ovnact_select) \
OVNACT(CT_CLEAR, ovnact_null)\
+OVNACT(CT_COMMIT_NAT, ovnact_ct_nat) \
OVNACT(CLONE, ovnact_nest)\
OVNACT(ARP, ovnact_nest)\
OVNACT(ICMP4, ovnact_nest)\
@@ -275,6 +276,8 @@ struct ovnact_ct_nat {
uint16_t port_hi;
} port_range;
+bool commit;/* Explicit commit action. */
+
uint8_t ltable; /* Logical table ID of next table. */
};
diff --git a/lib/actions.c b/lib/actions.c
index adbb42db4..91bbabc0e 100644
--- a/lib/actions.c
+++ b/lib/actions.c
@@ -920,6 +920,7 @@ parse_ct_nat(struct action_context *ctx, const char *name,
return;
}
cn->ltable = ctx->pp->cur_ltable + 1;
+cn->commit = false;
if (lexer_match(ctx->lexer, LEX_T_LPAREN)) {
if (ctx->lexer->token.type != LEX_T_INTEGER
@@ -929,9 +930,11 @@ parse_ct_nat(struct action_context *ctx, const char *name,
return;
}
if (ctx->lexer->token.format == LEX_F_IPV4) {
+cn->commit = true;
cn->family = AF_INET;
cn->ipv4 = ctx->lexer->token.value.ipv4;
} else if (ctx->lexer->token.format == LEX_F_IPV6) {
+cn->commit = true;
cn->family = AF_INET6;
cn->ipv6 = ctx->lexer->token.value.ipv6;
}
@@ -1004,6 +1007,24 @@ parse_CT_SNAT_IN_CZONE(struct action_context *ctx)
ovnact_put_CT_SNAT_IN_CZONE(ctx->ovnacts));
}
+static void
+parse_CT_COMMIT_NAT(struct action_context *ctx)
+{
+add_prerequisite(ctx, "ip");
+
+if (ctx->pp->cur_ltable >= ctx->pp->n_tables) {
+lexer_error(ctx->lexer,
+"\"ct_commit_related\" action not allowed in last table.");
+return;
+}
+
+struct ovnact_ct_nat *cn = ovnact_put_CT_COMMIT_NAT(ctx->ovnacts);
+cn->commit = true;
+cn->ltable = ctx->pp->cur_ltable + 1;
+cn->family = AF_UNSPEC;
+cn->port_range.exists = false;
+}
+
static void
format_ct_nat(const struct ovnact_ct_nat *cn, const char *name, struct ds *s)
{
@@ -1053,6 +1074,12 @@ format_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn,
struct ds *s)
format_ct_nat(cn, "ct_snat_in_czone", s);
}
+static void
+format_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn OVS_UNUSED, struct ds *s)
+{
+ds_put_cstr(s, "ct_commit_nat;");
+}
+
static void
encode_ct_nat(const struct ovnact_ct_nat *cn,
const struct ovnact_encode_params *ep,
@@ -1104,7 +1131,7 @@ encode_ct_nat(const struct ovnact_ct_nat *cn,
ofpacts->header = ofpbuf_push_uninit(ofpacts, nat_offset);
ct = ofpacts->header;
-if (cn->family == AF_INET || cn->family == AF_INET6) {
+if (cn->commit) {
ct->flags |= NX_CT_F_COMMIT;
}
ofpact_finish(ofpacts, >ofpact);
@@ -1143,6 +1170,17 @@ encode_CT_SNAT_IN_CZONE(const struct ovnact_ct_nat *cn,
encode_ct_nat(cn, ep, true, ep->common_nat_ct_zone, ofpacts);
}
+static void
+encode_CT_COMMIT_NAT(const struct ovnact_ct_nat *cn,
+ const struct ovnact_encode_params *ep,
+ struct ofpbuf *ofpacts)
+{
+enum mf_field_id zone = ep->is_switch
+? MFF_LOG_CT_ZONE
+: MFF_LOG_DNAT_ZONE;
+encode_ct_nat(cn, ep, false, zone, ofpacts);
+}
+
static void
ovnact_ct_nat_free(struct ovnact_ct_nat *ct_nat OVS_UNUSED)
{
@@ -4732,6 +4770,8 @@ parse_action(struct action_context *ctx)
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, "ct_commit_nat")) {
+parse_CT_COMMIT_NAT(ctx);
} else if (lexer_match_id(ctx->lexer, "clone")) {
parse_CLONE(ctx);
} else if
[ovs-dev] [PATCH ovn v3 0/2] Allow related traffic for LB
The related traffic wasn't correctly forwarded through the LB, the main issue was that the traffic was not NATted. This series allows the NAT to be applied and the traffic should arrive with correct addresses. --- v2: Add e2e test case. v3: Rebase on top of main. Address comments from Mark. Ales Musil (2): actions: Add new action called ct_commit_nat northd: Allow related traffic through LB include/ovn/actions.h | 3 + lib/actions.c | 42 +++- northd/northd.c | 29 -- northd/ovn-northd.8.xml | 29 -- ovn-sb.xml | 12 +++ tests/ovn-northd.at | 208 +--- tests/ovn.at| 15 ++- tests/system-ovn.at | 135 ++ utilities/ovn-trace.c | 34 +++ 9 files changed, 386 insertions(+), 121 deletions(-) -- 2.37.3 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [ovs-dev v5 1/3] ofproto-dpif-upcall: fix push_dp_ops
On 6 Nov 2022, at 8:12, Peng He wrote:
> push_dp_ops only handles delete ops errors but ignores the modify
> ops results. It's better to handle all the dp operation errors in
> a consistent way.
>
> We observe in the production environment that sometimes a megaflow
> with wrong actions keep staying in datapath. The coverage command shows
> revalidators have dumped several times, however the correct
> actions are not set. This implies that the ukey's action does not
> equal to the meagaflow's, i.e. revalidators think the underlying
> megaflow's actions are correct however they are not.
>
> We also check the megaflow using the ofproto/trace command, and the
> actions are not matched with the ones in the actual magaflow. By
> performing a revalidator/purge command, the right actions are set.
>
> This patch prevents the inconsistency by considering modify failure
> in revalidators.
>
> To note, we cannot perform two state transitions and change ukey_state
> into UKEY_EVICTED directly here, because, if we do so, the
> sweep will remove the ukey alone and leave dp flow alive. Later, the
> dump will retrieve the dp flow and might even recover it. This will
> contribute the stats of this dp flow twice.
>
> Signed-off-by: Peng He
Hi Peng,
Thanks for looking at the statistics part, see some comments inline!
In addition, I already acked patch 2 out of this series, but it mentions patch
x/3, but I do not see patch 3 in this series. Is this missing? Or are there
only two patches left?
Cheers,
Eelco
> ---
> ofproto/ofproto-dpif-upcall.c | 39 ++-
> 1 file changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 7ad728adf..a7970fa9b 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -254,6 +254,7 @@ enum ukey_state {
> UKEY_CREATED = 0,
> UKEY_VISIBLE, /* Ukey is in umap, datapath flow install is queued.
> */
> UKEY_OPERATIONAL, /* Ukey is in umap, datapath flow is installed. */
> +UKEY_INCONSISTENT, /* Ukey is in umap, datapath flow is modified but
> failed */
> UKEY_EVICTING, /* Ukey is in umap, datapath flow delete is queued.
> */
> UKEY_EVICTED, /* Ukey is in umap, datapath flow is deleted. */
> UKEY_DELETED, /* Ukey removed from umap, ukey free is deferred. */
> @@ -1966,6 +1967,10 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> * UKEY_VISIBLE -> UKEY_EVICTED
> * A handler attempts to install the flow, but the datapath rejects it.
> * Consider that the datapath has already destroyed it.
> + * UKEY_OPERATIONAL -> UKEY_INCONSISTENT
> + * A revalidator modifies the flow with error returns.
> + * UKEY_INCONSISTENT -> UKEY_EVICTING
> + * A revalidator decides to evict the datapath flow.
> * UKEY_OPERATIONAL -> UKEY_EVICTING
> * A revalidator decides to evict the datapath flow.
> * UKEY_EVICTING-> UKEY_EVICTED
> @@ -1974,7 +1979,8 @@ transition_ukey_at(struct udpif_key *ukey, enum
> ukey_state dst,
> * A revalidator has removed the ukey from the umap and is deleting it.
> */
> if (ukey->state == dst - 1 || (ukey->state == UKEY_VISIBLE &&
> - dst < UKEY_DELETED)) {
> + dst < UKEY_DELETED) ||
> +(ukey->state == UKEY_OPERATIONAL && dst == UKEY_EVICTING)) {
> ukey->state = dst;
> } else {
> struct ds ds = DS_EMPTY_INITIALIZER;
> @@ -2416,26 +2422,31 @@ push_dp_ops(struct udpif *udpif, struct ukey_op *ops,
> size_t n_ops)
>
> for (i = 0; i < n_ops; i++) {
> struct ukey_op *op = [i];
> -struct dpif_flow_stats *push, *stats, push_buf;
> -
> -stats = op->dop.flow_del.stats;
> -push = _buf;
> -
> -if (op->dop.type != DPIF_OP_FLOW_DEL) {
> -/* Only deleted flows need their stats pushed. */
> -continue;
> -}
>
> if (op->dop.error) {
> -/* flow_del error, 'stats' is unusable. */
> if (op->ukey) {
> ovs_mutex_lock(>ukey->mutex);
> -transition_ukey(op->ukey, UKEY_EVICTED);
> +if (op->dop.type == DPIF_OP_FLOW_DEL)
We should {} to the if branch.
> +transition_ukey(op->ukey, UKEY_EVICTED);
> +else {
> +transition_ukey(op->ukey, UKEY_INCONSISTENT);
Is there a specific reason to have delete go through a different state on
failure? If not, it might be more consistent especially when debugging.
> +}
> ovs_mutex_unlock(>ukey->mutex);
> }
> +/* if modify or delete fails, there is no need to push stats */
Captial for “If” and ending with a dot.
> continue;
> }
>
> +if (op->dop.type != DPIF_OP_FLOW_DEL) {
> +
Re: [ovs-dev] [PATCH ovn] Implement Path MTU Discovery for multichassis ports
On Thu, Nov 3, 2022 at 4:46 AM Ihar Hrachyshka wrote:
> When multichassis ports attached to localnet switches are forced to use
> tunneling to communicate with other ports, MTU of the path between ports
> may effectively change (to accommodate tunnel headers or otherwise).
>
> This patch implements Path MTU Discovery for IPv4 and IPv6 for
> multichassis ports (both egress and ingress).
>
> This patch adds new flows as follows:
>
> - table 30: store check_pkt_len(mtu)->reg9[1]
> - table 37: if reg9[1] is set, then send the packet to controller.
>
> TODO: document behavior.
> TODO: dynamically calculate MTU of the interface.
> TODO: constrain new flows to communication involving m-chassis ports only.
>
> Signed-off-by: Ihar Hrachyshka
>
---
> controller/physical.c | 167
> tests/ovn.at | 252 ++
> 2 files changed, 419 insertions(+)
>
> diff --git a/controller/physical.c b/controller/physical.c
> index 705146316..96c0dc31a 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -1072,6 +1072,170 @@ setup_activation_strategy(const struct
> sbrec_port_binding *binding,
> }
> }
>
> +static size_t
> +encode_start_controller_op(enum action_opcode opcode, bool pause,
> + uint32_t meter_id, struct ofpbuf *ofpacts)
> +{
> +size_t ofs = ofpacts->size;
> +
> +struct ofpact_controller *oc = ofpact_put_CONTROLLER(ofpacts);
> +oc->max_len = UINT16_MAX;
> +oc->reason = OFPR_ACTION;
> +oc->pause = pause;
> +if (!ovs_feature_is_supported(OVS_DP_METER_SUPPORT)) {
> +meter_id = NX_CTLR_NO_METER;
> +}
> +oc->meter_id = meter_id;
> +
> +struct action_header ah = { .opcode = htonl(opcode) };
> +ofpbuf_put(ofpacts, , sizeof ah);
> +
> +return ofs;
> +}
> +
> +static void
> +encode_finish_controller_op(size_t ofs, struct ofpbuf *ofpacts)
> +{
> +struct ofpact_controller *oc = ofpbuf_at_assert(ofpacts, ofs, sizeof
> *oc);
> +ofpacts->header = oc;
> +oc->userdata_len = ofpacts->size - (ofs + sizeof *oc);
> +ofpact_finish_CONTROLLER(ofpacts, );
> +}
> +
>
+static void
> +handle_oversized_ip_packets(struct ovn_desired_flow_table *flow_table,
> +const struct sbrec_port_binding *binding,
> +bool is_ipv6)
> +{
> +uint32_t dp_key = binding->datapath->tunnel_key;
> +
> +struct match match;
> +match_init_catchall();
> +match_set_metadata(, htonll(dp_key));
> +
> +struct ofpbuf ofpacts;
> +ofpbuf_init(, 0);
> +
> +/* Store packet too large flag in reg9[1]. */
> +/* TODO: limit to multichassis traffic? */
> +match_init_catchall();
> +match_set_dl_type(, htons(is_ipv6 ? ETH_TYPE_IPV6 :
> ETH_TYPE_IP));
> +match_set_metadata(, htonll(dp_key));
> +
> +/* TODO: get mtu from interface of the tunnel */
> +uint16_t frag_mtu = 1500;
> +struct ofpact_check_pkt_larger *pkt_larger =
> +ofpact_put_CHECK_PKT_LARGER();
> +pkt_larger->pkt_len = frag_mtu;
> +pkt_larger->dst.field = mf_from_id(MFF_REG9);
> +pkt_larger->dst.ofs = 1;
> +
> +put_resubmit(31, );
> +/* TODO: should we tag table=30 as consumed for this job anywhere? */
> +ofctrl_add_flow(flow_table, 30, 110,
> +binding->header_.uuid.parts[0], , ,
> +>header_.uuid);
> +ofpbuf_uninit();
>
+
> +/* Generate ICMP Fragmentation needed for IP packets that are too
> large
> + * (reg9[1] == 1) */
> +/* TODO: limit to multichassis traffic? */
> +match_init_catchall();
> +match_set_dl_type(, htons(is_ipv6 ? ETH_TYPE_IPV6 :
> ETH_TYPE_IP));
> +match_set_reg_masked(, MFF_REG9 - MFF_REG0, 1 << 1, 1 << 1);
> +
> +/* Return ICMP error with a part of the original IP packet included.
> */
> +ofpbuf_init(, 0);
> +size_t oc_offset = encode_start_controller_op(
> +ACTION_OPCODE_ICMP, true, NX_CTLR_NO_METER, );
> +
> +/* Before sending the ICMP error packet back to the pipeline, set a
> number
> + * of fields. */
> +struct ofpbuf inner_ofpacts;
> +ofpbuf_init(_ofpacts, 0);
> +
> +/* The new error packet is no longer too large */
> +/* REGBIT_PKT_LARGER = 0 */
> +ovs_be32 value = htonl(0);
> +ovs_be32 mask = htonl(1 << 1);
> +ofpact_put_set_field(
> +_ofpacts, mf_from_id(MFF_REG9), , );
> +
> +/* The new error packet is delivered locally */
> +/* REGBIT_EGRESS_LOOPBACK = 1 */
> +value = htonl(1 << MLF_ALLOW_LOOPBACK_BIT);
> +mask = htonl(1 << MLF_ALLOW_LOOPBACK_BIT);
> +ofpact_put_set_field(
> +_ofpacts, mf_from_id(MFF_LOG_FLAGS), , );
> +
> +/* eth.dst */
> +put_stack(MFF_ETH_SRC, ofpact_put_STACK_PUSH(_ofpacts));
> +put_stack(MFF_ETH_DST, ofpact_put_STACK_POP(_ofpacts));
> +
> +/* ip.dst */
> +put_stack(is_ipv6 ? MFF_IPV6_SRC : MFF_IPV4_SRC,
> + ofpact_put_STACK_PUSH(_ofpacts));
> +
Re: [ovs-dev] [PATCH ovn v5] ovn-controller: Fixed missing flows after interface deletion
On Mon, Nov 14, 2022 at 1:24 AM Xavier Simonart wrote:
>
> In the following scenario:
> - interface "old" is created and external_ids:iface-id is set (to lp)
> - interface "new" is created and external_ids:iface-id is set (to same lp)
> - interface "old" is deleted
> flows related to lp were deleted.
>
> Note that after "new" interface is created, flows use "new" ofport.
> The state where old and new interfaces have both external_ids:iface-id
set at
> the same time is "invalid", and all flows are not installed for lpold.
>
> Fixes: 3ae8470edc64 ("I-P: Handle runtime data changes for pflow_output
engine.")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2129866
>
> Signed-off-by: Xavier Simonart
>
> ---
> v2: - Use bool instead of int for binding count to better reflect only
> one additional binding is supported.
> - Fix use after free.
> - Remove debug logging from test case
> v3: - Based on Dumitru's and Mark's feedback:
> - Support any number of interfaces bound to the same port
> - Use recomputes to make code simpler (this is corner case)
> - Added test case using three interfaces bound to te same port
> v4: - Updated based on Ales' feedback
> - Also support scenario for port-types different than localport
> - Added test case for VIF port
> - Rebased on latest main
> v5: - Updated test case based on Numan/Dumitru's feedback (hit ofport = 0)
> and added comments in code.
> - Rebased on latest main.
> ---
> controller/binding.c | 36 ++
> controller/binding.h | 1 +
> tests/ovn.at | 168 +++
> 3 files changed, 205 insertions(+)
>
> diff --git a/controller/binding.c b/controller/binding.c
> index c3d2b2e42..5f04b9a74 100644
> --- a/controller/binding.c
> +++ b/controller/binding.c
> @@ -1866,6 +1866,7 @@ build_local_bindings(struct binding_ctx_in
*b_ctx_in,
> lbinding = local_binding_create(iface_id, iface_rec);
> local_binding_add(local_bindings, lbinding);
> } else {
> +lbinding->multiple_bindings = true;
> static struct vlog_rate_limit rl =
> VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_WARN_RL(
> @@ -2156,6 +2157,10 @@ consider_iface_claim(const struct ovsrec_interface
*iface_rec,
> lbinding = local_binding_create(iface_id, iface_rec);
> local_binding_add(local_bindings, lbinding);
> } else {
> +if (lbinding->iface && lbinding->iface != iface_rec) {
> +lbinding->multiple_bindings = true;
> +b_ctx_out->local_lports_changed = true;
> +}
> lbinding->iface = iface_rec;
> }
>
> @@ -2174,6 +2179,13 @@ consider_iface_claim(const struct ovsrec_interface
*iface_rec,
> return true;
> }
>
> +/* If multiple bindings to the same port, remove the "old" binding.
> + * This ensures that change tracking is correct.
> + */
> +if (lbinding->multiple_bindings) {
> +remove_related_lport(pb, b_ctx_out);
> +}
> +
> enum en_lport_type lport_type = get_lport_type(pb);
> if (lport_type == LP_LOCALPORT) {
> return consider_localport(pb, b_ctx_in, b_ctx_out);
> @@ -2226,6 +2238,29 @@ consider_iface_release(const struct
ovsrec_interface *iface_rec,
> struct shash *binding_lports = _ctx_out->lbinding_data->lports;
>
> lbinding = local_binding_find(local_bindings, iface_id);
> +
> +if (lbinding) {
> +if (lbinding->multiple_bindings) {
> +VLOG_INFO("Multiple bindings for %s: force recompute to
clean up",
> + iface_id);
> +return false;
> +} else {
> +int64_t ofport = iface_rec->n_ofport ? *iface_rec->ofport :
0;
> +if (lbinding->iface != iface_rec && !ofport) {
> +/* If external_ids:iface-id is set within the same
transaction
> + * as adding an interface to a bridge, ovn-controller is
> + * usually initially notified of ovs interface changes
with
> + * ofport == 0. If the lport was bound to a different
interface
> + * we do not want to release it.
> + */
Hi Xavier, I think this logic in the "else" block, if really needed,
belongs to a separate patch.
It looks like an optimization to the scenario when moving a logical port
binding from one VIF to another on the same node. I guess this doesn't
happen very often in the real world. If it happens in some corner cases, it
shouldn't be a problem of letting it release and reclaim, right? So I
wonder if this change is really necessary. Assume it is correct, it still
adds some burden when reading/debugging the code. What do you think?
For the rest of the patch:
Acked-by: Han Zhou
> +VLOG_DBG("Not releasing lport %s as %s was claimed "
> + "and %s was never bound)",
