[ovs-dev] [PATCH v2] bond: Reset stats when deleting post recirc rule.
In order to properly balance bond traffic, ofproto/bond periodically
reads usage statistics of the post-recirculation rules (which are added
to a hidden internal table).
To do that, each "struct bond_entry" (which represents a hash within a
bond) stores the last seen statistics for its rule. When a hash is moved
to another member (due to a bond rebalance or the previous member going
down), the rule is typically just modified, i.e: same match different
actions. In this case, statistics are preserved and accounting continues
to work.
However, if the rule gets completely deleted (e.g: when all bond members
go down) and then re-created, the new rule will have 0 tx_bytes but its
associated entry will still store a non-zero last-seen value.
This situation leads to an overflow of the delta calculation (computed
as [current_stats_value - last_seen_value]), which can affect traffic
as the hash will be considered to carry a lot of traffic and rebalancing
will kick in.
In order to fix this situation, reset the value of last seen statistics
on rule deletion.
Implementation notes:
Modifying pr_tx_bytes requires write-locking the global rwlock but a
lockless version of update_recirc_rules was being maintained to avoid locking
on bon_unref().
Considering the small impact of locking during bond removal, removing the
lockless version and relying on clang's thread safety analysis is preferred.
Also, folding Ilya's [1], i.e: fixing thread safety annotation in
update_recirc_rules() to require holding write-lock.
[1]
https://patchwork.ozlabs.org/project/openvswitch/patch/20240209161718.1149494-1-i.maxim...@ovn.org/
Reported-at: https://github.com/openvswitch/ovs-issues/issues/319
Co-authored-by: Ilya Maximets
Signed-off-by: Ilya Maximets
Signed-off-by: Adrian Moreno
---
ofproto/bond.c | 33 +++--
1 file changed, 15 insertions(+), 18 deletions(-)
diff --git a/ofproto/bond.c b/ofproto/bond.c
index cfdf44f85..e5b3f1a1b 100644
--- a/ofproto/bond.c
+++ b/ofproto/bond.c
@@ -186,7 +186,7 @@ static struct bond_member *choose_output_member(const
struct bond *,
struct flow_wildcards *,
uint16_t vlan)
OVS_REQ_RDLOCK(rwlock);
-static void update_recirc_rules__(struct bond *);
+static void update_recirc_rules(struct bond *) OVS_REQ_WRLOCK(rwlock);
static bool bond_may_recirc(const struct bond *);
static void bond_update_post_recirc_rules__(struct bond *, bool force)
OVS_REQ_WRLOCK(rwlock);
@@ -299,7 +299,10 @@ bond_unref(struct bond *bond)
}
free(bond->hash);
bond->hash = NULL;
-update_recirc_rules__(bond);
+
+ovs_rwlock_wrlock();
+update_recirc_rules(bond);
+ovs_rwlock_unlock();
hmap_destroy(>pr_rule_ops);
free(bond->primary);
@@ -331,17 +334,8 @@ add_pr_rule(struct bond *bond, const struct match *match,
hmap_insert(>pr_rule_ops, _op->hmap_node, hash);
}
-/* This function should almost never be called directly.
- * 'update_recirc_rules()' should be called instead. Since
- * this function modifies 'bond->pr_rule_ops', it is only
- * safe when 'rwlock' is held.
- *
- * However, when the 'bond' is the only reference in the system,
- * calling this function avoid acquiring lock only to satisfy
- * lock annotation. Currently, only 'bond_unref()' calls
- * this function directly. */
static void
-update_recirc_rules__(struct bond *bond)
+update_recirc_rules(struct bond *bond) OVS_REQ_WRLOCK(rwlock)
{
struct match match;
struct bond_pr_rule_op *pr_op;
@@ -407,6 +401,15 @@ update_recirc_rules__(struct bond *bond)
VLOG_ERR("failed to remove post recirculation flow %s", err_s);
free(err_s);
+} else if (bond->hash) {
+/* If the flow deletion failed, a subsecuent call to
+ * ofproto_dpif_add_internal_flow() would just modify the flow
+ * preserving its statistics. Therefore, only reset the entry's
+ * byte counter if it succeeds. */
+uint32_t hash = pr_op->hmap_node.hash & BOND_MASK;
+struct bond_entry *entry = >hash[hash];
+
+entry->pr_tx_bytes = 0;
}
hmap_remove(>pr_rule_ops, _op->hmap_node);
@@ -421,12 +424,6 @@ update_recirc_rules__(struct bond *bond)
ofpbuf_uninit();
}
-static void
-update_recirc_rules(struct bond *bond)
-OVS_REQ_RDLOCK(rwlock)
-{
-update_recirc_rules__(bond);
-}
/* Updates 'bond''s overall configuration to 's'.
*
--
2.43.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 12/12] documentation: Document ovs-flowviz.
On 2/19/24 23:53, Ilya Maximets wrote: On 2/19/24 09:14, Adrian Moreno wrote: Add a page for flow visualization with a few key concepts and examples. Signed-off-by: Adrian Moreno --- Documentation/automake.mk | 3 +- Documentation/topics/flow-visualization.rst | 469 Hi, Adrian. This doc looks a lot like a man page. Maybe it should it be a man page instead? i.e. be in Documentation/ref/ and structured a little more like a man page? I suppose it can also be split in two docs, the man page and a topic document with more comprehensive usage examples. What do you think? That's a good idea. Will do. I didn't fully read the doc, but see some typos and minor issues that caught my eye below. Best regards, Ilya Maximets. Documentation/topics/index.rst | 1 + 3 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 Documentation/topics/flow-visualization.rst diff --git a/Documentation/automake.mk b/Documentation/automake.mk index 47d2e336a..f1339e876 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -45,7 +45,7 @@ DOC_SOURCE = \ Documentation/topics/fuzzing/ovs-fuzzing-infrastructure.rst \ Documentation/topics/fuzzing/ovs-fuzzers.rst \ Documentation/topics/fuzzing/security-analysis-of-ovs-fuzzers.rst \ - Documentation/topics/testing.rst \ + Documentation/topics/flow-visualization.rst \ Documentation/topics/integration.rst \ Documentation/topics/language-bindings.rst \ Documentation/topics/networking-namespaces.rst \ @@ -55,6 +55,7 @@ DOC_SOURCE = \ Documentation/topics/ovsdb-replication.rst \ Documentation/topics/porting.rst \ Documentation/topics/record-replay.rst \ + Documentation/topics/testing.rst \ Documentation/topics/tracing.rst \ Documentation/topics/usdt-probes.rst \ Documentation/topics/userspace-checksum-offloading.rst \ diff --git a/Documentation/topics/flow-visualization.rst b/Documentation/topics/flow-visualization.rst new file mode 100644 index 0..366275c54 --- /dev/null +++ b/Documentation/topics/flow-visualization.rst @@ -0,0 +1,469 @@ +.. + Licensed under the Apache License, Version 2.0 (the "License"); you may + not use this file except in compliance with the License. You may obtain + a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + License for the specific language governing permissions and limitations + under the License. + + Convention for heading levels in Open vSwitch documentation: + + === Heading 0 (reserved for the title in a document) + --- Heading 1 + ~~~ Heading 2 + +++ Heading 3 + ''' Heading 4 + + Avoid deeper levels because they do not render well. + += +ovs-flowviz: Datapath and Openflow flow visualization * OpenFlow += + +When troubleshooting networking issues with OVS, we typically end up looking +at OpenFlow or datapath flow dumps. These dumps tend to be quite dense and +difficult to reason about. + +``ovs-flowviz`` is a utility script that helps visualizing OpenFlow and +datapath flows to make it easier to understand what is going on. + + +Installing ovs-flowviz +-- + +``ovs-flowviz`` is part of the openvswitch python package. To install it, run: +:: + +$ pip install openvswitch[flowviz] + +Or, if you are working with the OVS tree: +:: + +$ cd python && pip install .[flowviz] + +Running the tool + +Here is the basic usage of the tool: +:: + +$ ovs-flowviz --help +Usage: ovs-flowviz [OPTIONS] COMMAND [ARGS]... + + OpenvSwitch flow visualization utility. + + It reads openflow and datapath flows (such as the output of ovs-ofctl dump- + flows or ovs-appctl dpctl/dump-flows) and prints them in different formats. + +Options: + -c, --config PATH Use config file [default: /home/amorenoz/src/ovs/pyth We probbaly shouldn't list your home directory in the docs. :) +on/venv/lib64/python3.12/site- +packages/ovs/flowviz/ovs-flowviz.conf] Or venv for that matter. Uups! I just C-P the "--help" output where the default value is obtained from the current setup. I'll fix that. + --style TEXT Select style (defined in config file) + -i, --input PATH Read flows from specified filepath. If not provided, +flows will be read from stdin. This option can be +specified multiple
Re: [ovs-dev] [PATCH v1 12/12] documentation: Document ovs-flowviz.
On 2/19/24 09:14, Adrian Moreno wrote: > Add a page for flow visualization with a few key concepts and examples. > > Signed-off-by: Adrian Moreno > --- > Documentation/automake.mk | 3 +- > Documentation/topics/flow-visualization.rst | 469 Hi, Adrian. This doc looks a lot like a man page. Maybe it should it be a man page instead? i.e. be in Documentation/ref/ and structured a little more like a man page? I suppose it can also be split in two docs, the man page and a topic document with more comprehensive usage examples. What do you think? I didn't fully read the doc, but see some typos and minor issues that caught my eye below. Best regards, Ilya Maximets. > Documentation/topics/index.rst | 1 + > 3 files changed, 472 insertions(+), 1 deletion(-) > create mode 100644 Documentation/topics/flow-visualization.rst > > diff --git a/Documentation/automake.mk b/Documentation/automake.mk > index 47d2e336a..f1339e876 100644 > --- a/Documentation/automake.mk > +++ b/Documentation/automake.mk > @@ -45,7 +45,7 @@ DOC_SOURCE = \ > Documentation/topics/fuzzing/ovs-fuzzing-infrastructure.rst \ > Documentation/topics/fuzzing/ovs-fuzzers.rst \ > Documentation/topics/fuzzing/security-analysis-of-ovs-fuzzers.rst \ > - Documentation/topics/testing.rst \ > + Documentation/topics/flow-visualization.rst \ > Documentation/topics/integration.rst \ > Documentation/topics/language-bindings.rst \ > Documentation/topics/networking-namespaces.rst \ > @@ -55,6 +55,7 @@ DOC_SOURCE = \ > Documentation/topics/ovsdb-replication.rst \ > Documentation/topics/porting.rst \ > Documentation/topics/record-replay.rst \ > + Documentation/topics/testing.rst \ > Documentation/topics/tracing.rst \ > Documentation/topics/usdt-probes.rst \ > Documentation/topics/userspace-checksum-offloading.rst \ > diff --git a/Documentation/topics/flow-visualization.rst > b/Documentation/topics/flow-visualization.rst > new file mode 100644 > index 0..366275c54 > --- /dev/null > +++ b/Documentation/topics/flow-visualization.rst > @@ -0,0 +1,469 @@ > +.. > + Licensed under the Apache License, Version 2.0 (the "License"); you may > + not use this file except in compliance with the License. You may obtain > + a copy of the License at > + > + http://www.apache.org/licenses/LICENSE-2.0 > + > + Unless required by applicable law or agreed to in writing, software > + distributed under the License is distributed on an "AS IS" BASIS, > WITHOUT > + WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See > the > + License for the specific language governing permissions and limitations > + under the License. > + > + Convention for heading levels in Open vSwitch documentation: > + > + === Heading 0 (reserved for the title in a document) > + --- Heading 1 > + ~~~ Heading 2 > + +++ Heading 3 > + ''' Heading 4 > + > + Avoid deeper levels because they do not render well. > + > += > +ovs-flowviz: Datapath and Openflow flow visualization * OpenFlow > += > + > +When troubleshooting networking issues with OVS, we typically end up looking > +at OpenFlow or datapath flow dumps. These dumps tend to be quite dense and > +difficult to reason about. > + > +``ovs-flowviz`` is a utility script that helps visualizing OpenFlow and > +datapath flows to make it easier to understand what is going on. > + > + > +Installing ovs-flowviz > +-- > + > +``ovs-flowviz`` is part of the openvswitch python package. To install it, > run: > +:: > + > +$ pip install openvswitch[flowviz] > + > +Or, if you are working with the OVS tree: > +:: > + > +$ cd python && pip install .[flowviz] > + > +Running the tool > + > +Here is the basic usage of the tool: > +:: > + > +$ ovs-flowviz --help > +Usage: ovs-flowviz [OPTIONS] COMMAND [ARGS]... > + > + OpenvSwitch flow visualization utility. > + > + It reads openflow and datapath flows (such as the output of ovs-ofctl > dump- > + flows or ovs-appctl dpctl/dump-flows) and prints them in different > formats. > + > +Options: > + -c, --config PATH Use config file [default: > /home/amorenoz/src/ovs/pyth We probbaly shouldn't list your home directory in the docs. :) > +on/venv/lib64/python3.12/site- > +packages/ovs/flowviz/ovs-flowviz.conf] Or venv for that matter. > + --style TEXT Select style (defined in config file) > + -i, --input PATH Read flows from specified filepath. If not > provided, > +flows will be read from stdin. This option can be > +specified multiple times.
[ovs-dev] [PATCH ovn] northd: Fix logical router load-balancer nat rules when using DGP.
This commit fixes the build_distr_lrouter_nat_flows_for_lb function to
include one NAT flow entry for each DGP in use. Since we have added support
to create multiple gateway ports per logical router, it's necessary to
include in the LR nat rules pipeline a specific entry for each attached
DGP. Otherwise, the ingress traffic is only redirected when the incoming
LRP matches the chassis_resident field.
Considering that DNAT rules for DGPs were implemented with the need to
configure the DGP-related gateway-port column, the load-balancer NAT rule
configuration can use a similar idea. In this case, we don't know the LRP
responsible for the incoming traffic, and therefore we must apply the
load-balancer automatically created NAT rule in all DGPs to allow the
incoming traffic.
Reported-at: https://bugs.launchpad.net/ubuntu/+source/ovn/+bug/2054322
Fixes: 15348b7b806f ("ovn-northd: Multiple distributed gateway port support.")
Signed-off-by: Roberto Bartzen Acosta
---
northd/en-lr-stateful.c | 12 --
northd/northd.c | 14 ---
tests/ovn-northd.at | 92 +
3 files changed, 100 insertions(+), 18 deletions(-)
diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
index 6d0192487..7ffa4a690 100644
--- a/northd/en-lr-stateful.c
+++ b/northd/en-lr-stateful.c
@@ -537,18 +537,6 @@ lr_stateful_record_create(struct lr_stateful_table *table,
table->array[od->index] = lr_stateful_rec;
-/* Load balancers are not supported (yet) if a logical router has multiple
- * distributed gateway port. Log a warning. */
-if (lr_stateful_rec->has_lb_vip && lr_has_multiple_gw_ports(od)) {
-static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
-VLOG_WARN_RL(, "Load-balancers are configured on logical "
- "router %s, which has %"PRIuSIZE" distributed "
- "gateway ports. Load-balancer is not supported "
- "yet when there is more than one distributed "
- "gateway port on the router.",
- od->nbr->name, od->n_l3dgw_ports);
-}
-
return lr_stateful_rec;
}
diff --git a/northd/northd.c b/northd/northd.c
index 2c3560ce2..7eb943d2f 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -10919,10 +10919,9 @@ static void
build_distr_lrouter_nat_flows_for_lb(struct lrouter_nat_lb_flows_ctx *ctx,
enum lrouter_nat_lb_flow_type type,
struct ovn_datapath *od,
- struct lflow_ref *lflow_ref)
+ struct lflow_ref *lflow_ref,
+ struct ovn_port *dgp)
{
-struct ovn_port *dgp = od->l3dgw_ports[0];
-
const char *undnat_action;
switch (type) {
@@ -10953,7 +10952,7 @@ build_distr_lrouter_nat_flows_for_lb(struct
lrouter_nat_lb_flows_ctx *ctx,
if (ctx->lb_vip->n_backends || !ctx->lb_vip->empty_backend_rej) {
ds_put_format(ctx->new_match, " && is_chassis_resident(%s)",
- od->l3dgw_ports[0]->cr_port->json_key);
+ dgp->cr_port->json_key);
}
ovn_lflow_add_with_hint__(ctx->lflows, od, S_ROUTER_IN_DNAT, ctx->prio,
@@ -11164,8 +11163,11 @@ build_lrouter_nat_flows_for_lb(
if (!od->n_l3dgw_ports) {
bitmap_set1(gw_dp_bitmap[type], index);
} else {
-build_distr_lrouter_nat_flows_for_lb(, type, od,
- lb_dps->lflow_ref);
+for (size_t i = 0; i < od->n_l3dgw_ports; i++) {
+struct ovn_port *dgp = od->l3dgw_ports[i];
+build_distr_lrouter_nat_flows_for_lb(, type, od,
+ lb_dps->lflow_ref, dgp);
+}
}
if (lb->affinity_timeout) {
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 6fdd761da..fa24935e1 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -12313,3 +12313,95 @@ check_engine_stats northd recompute nocompute
check_engine_stats lflow recompute nocompute
AT_CLEANUP
+
+OVN_FOR_EACH_NORTHD_NO_HV([
+AT_SETUP([Load balancer with Distributed Gateway Ports (DGP)])
+ovn_start
+
+check ovn-nbctl ls-add public
+check ovn-nbctl lr-add lr1
+
+# lr1 DGP ts1
+check ovn-nbctl ls-add ts1
+check ovn-nbctl lrp-add lr1 lr1-ts1 00:00:01:02:03:04 172.16.10.1/24
+check ovn-nbctl lrp-set-gateway-chassis lr1-ts1 chassis-2
+
+# lr1 DGP ts2
+check ovn-nbctl ls-add ts2
+check ovn-nbctl lrp-add lr1 lr1-ts2 00:00:01:02:03:05 172.16.20.1/24
+check ovn-nbctl lrp-set-gateway-chassis lr1-ts2 chassis-3
+
+# lr1 DGP public
+check ovn-nbctl lrp-add lr1 lr1_public 00:de:ad:ff:00:01 173.16.0.1/16
+check ovn-nbctl lrp-add lr1 lr1_s1 00:de:ad:fe:00:02 172.16.0.1/24
+check ovn-nbctl lrp-set-gateway-chassis lr1_public chassis-1
+
+check ovn-nbctl ls-add s1
+# s1 - lr1
+check ovn-nbctl lsp-add s1
Re: [ovs-dev] [PATCH ovn 09/10] tests: Speed up "multicast group buffer split".
On 2/12/24 18:55, Xavier Simonart wrote:
> Signed-off-by: Xavier Simonart
> ---
> tests/ovn-controller.at | 14 --
> tests/ovn-macros.at | 11 +++
> tests/ovn.at| 8 +---
> 3 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at
> index 3089aea9d..c10bc155b 100644
> --- a/tests/ovn-controller.at
> +++ b/tests/ovn-controller.at
> @@ -2693,19 +2693,20 @@ ovs-vsctl add-br br-phys
> ovn_attach n1 br-phys 192.168.0.1
>
> for i in $(seq 1 320); do
> -ovs-vsctl -- add-port br-int hv1-vif$i -- \
> +OVS_VSCTL(add-port br-int hv1-vif$i -- \
> set interface hv1-vif$i external-ids:iface-id=sw0-p$i \
> options:tx_pcap=hv1/vif${i}-tx.pcap \
> options:rxq_pcap=hv1/vif${i}-rx.pcap \
> -ofport-request=$i
> +ofport-request=$i)
> done
> -
> +RUN_OVS_VSCTL
>
> check ovn-nbctl ls-add sw0
> for i in $(seq 1 320); do
> -check ovn-nbctl lsp-add sw0 sw0-p$i
> -check ovn-nbctl lsp-set-addresses sw0-p$i "unknown"
> +OVN_NBCTL(lsp-add sw0 sw0-p$i)
> +OVN_NBCTL(lsp-set-addresses sw0-p$i "unknown")
> done
> +RUN_OVN_NBCTL
>
> wait_for_ports_up
> ovn-nbctl --wait=hv sync
> @@ -2715,8 +2716,9 @@ OVS_WAIT_FOR_OUTPUT([as hv1 ovs-ofctl dump-flows br-int
> table=OFTABLE_LOCAL_OUTP
> ])
>
> for i in $(seq 1 280); do
> -check ovn-nbctl lsp-del sw0-p$i
> +OVN_NBCTL(lsp-del sw0-p$i)
> done
> +RUN_OVN_NBCTL
> ovn-nbctl --wait=hv sync
>
> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int table=OFTABLE_LOCAL_OUTPUT |
> grep -q controller], [1])
> diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> index 84e50d76f..a35c0ff80 100644
> --- a/tests/ovn-macros.at
> +++ b/tests/ovn-macros.at
> @@ -984,6 +984,17 @@ m4_define([RUN_OVN_NBCTL], [
> unset command
> ])
>
> +# OVS_VSCTL(VSCTL_COMMAND) adds VSCTL_COMMAND to list of commands to be run
> by RUN_OVS_VSCTL().
> +m4_define([OVS_VSCTL], [
> +command="${command} -- $1"
> +])
> +
> +# RUN_OVS_VSCTL() executes list of commands built by the OVS_VSCTL() macro.
> +m4_define([RUN_OVS_VSCTL], [
> +check ovs-vsctl ${command}
> +unset command
> +])
Hi, Xavier. Not an issue of this particular patch as this thing was
introduced long ago, but...
The shell command 'command' is a way to call a different command ignoring
the defined aliases. Overloading the 'command' itself in that sense doesn't
seem wise to me and may cause a major headache if we ever hit an issue
involving it. I'd suggest some other name should be used here.
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v4 3/4] utilities: Add ovn-debug binary tool.
Thanks Ales,
Acked-by: Mark Michelson
On 2/12/24 10:55, Ales Musil wrote:
Add ovn-debug binary tool that can be extended with commands that
might be useful for tests/debugging of OVN environment. Currently
the tool supports only two commands:
1) "lflow-stage-to-ltable STAGE_NAME" that converts stage name into
logical flow table id.
2) "lflow-stage-to-oftable STAGE_NAME" that converts stage name into
OpenFlow table id.
For now it will be used in tests to get rid remaining hardcoded table
numbers.
Signed-off-by: Ales Musil
---
v4: Rebase on top current main.
Address nit from Dumitru.
---
NEWS | 5 ++
README.rst | 1 +
debian/ovn-common.install | 1 +
debian/ovn-common.manpages | 1 +
rhel/ovn-fedora.spec.in| 2 +
utilities/.gitignore | 2 +
utilities/automake.mk | 10 ++-
utilities/ovn-debug.8.xml | 28 +++
utilities/ovn-debug.c | 155 +
9 files changed, 204 insertions(+), 1 deletion(-)
create mode 100644 utilities/ovn-debug.8.xml
create mode 100644 utilities/ovn-debug.c
diff --git a/NEWS b/NEWS
index 7114b96d1..b979e54d7 100644
--- a/NEWS
+++ b/NEWS
@@ -1,5 +1,10 @@
Post v24.03.0
-
+ - Add ovn-debug tool containing two commands.
+"lflow-stage-to-ltable STAGE_NAME" that converts stage name into logical
+flow table id.
+"lflow-stage-to-oftable STAGE_NAME" that converts stage name into OpenFlow
+table id.
OVN v24.03.0 - xx xxx
--
diff --git a/README.rst b/README.rst
index 6fb717742..428cd8ee8 100644
--- a/README.rst
+++ b/README.rst
@@ -56,6 +56,7 @@ The main components of this distribution are:
- ovn-sbctl, a tool for interfacing with the southbound database.
- ovn-trace, a debugging utility that allows for tracing of packets through
the logical network.
+- ovn-debug, a tool to simplify debugging of OVN setup.
- Scripts and specs for building RPMs.
What other documentation is available?
diff --git a/debian/ovn-common.install b/debian/ovn-common.install
index 050d1c63a..fc48f07e4 100644
--- a/debian/ovn-common.install
+++ b/debian/ovn-common.install
@@ -5,6 +5,7 @@ usr/bin/ovn-ic-nbctl
usr/bin/ovn-ic-sbctl
usr/bin/ovn-trace
usr/bin/ovn_detrace.py
+usr/bin/ovn-debug
usr/share/ovn/scripts/ovn-ctl
usr/share/ovn/scripts/ovndb-servers.ocf
usr/share/ovn/scripts/ovn-lib
diff --git a/debian/ovn-common.manpages b/debian/ovn-common.manpages
index 1fa3d9cb3..e864512e3 100644
--- a/debian/ovn-common.manpages
+++ b/debian/ovn-common.manpages
@@ -11,3 +11,4 @@ utilities/ovn-ic-nbctl.8
utilities/ovn-ic-sbctl.8
utilities/ovn-trace.8
utilities/ovn-detrace.1
+utilities/ovn-debug.8
diff --git a/rhel/ovn-fedora.spec.in b/rhel/ovn-fedora.spec.in
index 03c1f27c5..670f1ca9e 100644
--- a/rhel/ovn-fedora.spec.in
+++ b/rhel/ovn-fedora.spec.in
@@ -495,6 +495,7 @@ fi
%{_bindir}/ovn-appctl
%{_bindir}/ovn-ic-nbctl
%{_bindir}/ovn-ic-sbctl
+%{_bindir}/ovn-debug
%{_datadir}/ovn/scripts/ovn-ctl
%{_datadir}/ovn/scripts/ovn-lib
%{_datadir}/ovn/scripts/ovndb-servers.ocf
@@ -515,6 +516,7 @@ fi
%{_mandir}/man8/ovn-ic.8*
%{_mandir}/man5/ovn-ic-nb.5*
%{_mandir}/man5/ovn-ic-sb.5*
+%{_mandir}/man8/ovn-debug.8*
%{_prefix}/lib/ocf/resource.d/ovn/ovndb-servers
%config(noreplace) %{_sysconfdir}/logrotate.d/ovn
%{_unitdir}/ovn-db@.service
diff --git a/utilities/.gitignore b/utilities/.gitignore
index da237563b..3ae97b00f 100644
--- a/utilities/.gitignore
+++ b/utilities/.gitignore
@@ -13,6 +13,8 @@
/ovn-trace.8
/ovn_detrace.py
/ovn-detrace.1
+/ovn-debug
+/ovn-debug.8
/ovn-docker-overlay-driver
/ovn-docker-underlay-driver
/ovn-lib
diff --git a/utilities/automake.mk b/utilities/automake.mk
index c44563c26..6a2b96e66 100644
--- a/utilities/automake.mk
+++ b/utilities/automake.mk
@@ -11,7 +11,8 @@ man_MANS += \
utilities/ovn-ic-sbctl.8 \
utilities/ovn-trace.8 \
utilities/ovn-detrace.1 \
-utilities/ovn-appctl.8
+utilities/ovn-appctl.8 \
+utilities/ovn-debug.8
MAN_ROOTS += \
utilities/ovn-detrace.1.in
@@ -34,6 +35,7 @@ EXTRA_DIST += \
utilities/ovn-ic-sbctl.8.xml \
utilities/ovn-appctl.8.xml \
utilities/ovn-trace.8.xml \
+utilities/ovn-debug.8.xml \
utilities/ovn_detrace.py.in \
utilities/ovndb-servers.ocf \
utilities/checkpatch.py \
@@ -62,6 +64,7 @@ CLEANFILES += \
utilities/ovn-ic-nbctl.8 \
utilities/ovn-ic-sbctl.8 \
utilities/ovn-trace.8 \
+utilities/ovn-debug.8 \
utilities/ovn-detrace.1 \
utilities/ovn-detrace \
utilities/ovn_detrace.py \
@@ -119,4 +122,9 @@ UNINSTALL_LOCAL += ovn-detrace-uninstall
ovn-detrace-uninstall:
rm -f $(DESTDIR)$(bindir)/ovn-detrace
+# ovn-debug
+bin_PROGRAMS += utilities/ovn-debug
+utilities_ovn_debug_SOURCES = utilities/ovn-debug.c
+utilities_ovn_debug_LDADD = lib/libovn.la $(OVSDB_LIBDIR)/libovsdb.la
Re: [ovs-dev] [PATCH ovn v4 2/4] checkpatch: Add rule to check for hardcoded table numbers.
Thanks Ales,
Acked-by: Mark Michelson
On 2/12/24 10:55, Ales Musil wrote:
To avoid issues with hardcoded table numbers in future add rule
into check patch. The rule is only warning because there are still
legitimate use cases and not everything can be abstracted.
Signed-off-by: Ales Musil
---
v4: Rebase on top of main.
Address comments from Dumitru:
- Fix the regex.
- Add test for the new check.
---
tests/checkpatch.at | 39 +++
utilities/checkpatch.py | 12
2 files changed, 51 insertions(+)
diff --git a/tests/checkpatch.at b/tests/checkpatch.at
index e7322fff4..6ac0e51f3 100755
--- a/tests/checkpatch.at
+++ b/tests/checkpatch.at
@@ -605,3 +605,42 @@ try_checkpatch \
Subject: netdev: This is a way to long commit summary and therefor it should
report a WARNING!"
AT_CLEANUP
+
+AT_SETUP([checkpatch - hardcoded table numbers])
+try_checkpatch \
+ "COMMON_PATCH_HEADER([tests/something.at])
++table=12(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1),
action=(next(pipeline=ingress, table=??);)
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+table=12(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1),
action=(next(pipeline=ingress, table=??);)
+"
+
+try_checkpatch \
+ "COMMON_PATCH_HEADER([tests/something.at])
++table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1),
action=(next(pipeline=ingress, table=13);)
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+table=??(ls_in_hairpin ), priority=1000 , match=(reg0[[14]] == 1),
action=(next(pipeline=ingress, table=13);)
+"
+
+try_checkpatch \
+ "COMMON_PATCH_HEADER([tests/something.at])
++priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+priority=1100,ip,reg15=0x1,metadata=0x1,nw_src=10.0.0.1
actions=load:0x1->OXM_OF_PKT_REG4[[49]],resubmit(,47)
+"
+
+try_checkpatch \
+ "COMMON_PATCH_HEADER([tests/something.at])
++C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep
"dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]])
+" \
+"WARNING: Use of hardcoded table= or resubmit=(,) is
discouraged in tests. Consider using MACRO instead.
+#8 FILE: tests/something.at:1:
+C_H_E_C_K([as gw1 ovs-ofctl dump-flows br-int table=42 | grep
"dl_dst=00:00:02:01:02:04" | wc -l], [0], [[1]])
+"
+
+AT_CLEANUP
diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
index 52d3fa845..35204daa2 100755
--- a/utilities/checkpatch.py
+++ b/utilities/checkpatch.py
@@ -202,6 +202,7 @@ __regex_if_macros = re.compile(r'^ +(%s)
\([\S]([\s\S]+[\S])*\) { +\\' %
__parenthesized_constructs)
__regex_nonascii_characters = re.compile("[^\u-\u007f]")
__regex_efgrep = re.compile(r'.*[ef]grep.*$')
+__regex_hardcoded_table =
re.compile(r'.*(table=[0-9]+)|.*(resubmit\(,[0-9]+\))')
skip_leading_whitespace_check = False
skip_trailing_whitespace_check = False
@@ -371,6 +372,10 @@ def has_efgrep(line):
"""Returns TRUE if the current line contains 'egrep' or 'fgrep'."""
return __regex_efgrep.match(line) is not None
+def has_hardcoded_table(line):
+"""Return TRUE if the current line contains table= or
+ resubmit(,)"""
+return __regex_hardcoded_table.match(line) is not None
def filter_comments(current_line, keep=False):
"""remove all of the c-style comments in a line"""
@@ -656,6 +661,13 @@ checks = [
'check': lambda x: has_efgrep(x),
'print':
lambda: print_error("grep -E/-F should be used instead of egrep/fgrep")},
+
+{'regex': r'\.at$', 'match_name': None,
+ 'check': lambda x: has_hardcoded_table(x),
+ 'print':
+ lambda: print_warning("Use of hardcoded table= or"
+ " resubmit=(,) is discouraged in tests."
+ " Consider using MACRO instead.")},
]
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn v4 1/4] tests: Remove hardcoded numbers from comments.
Thanks Ales!
Acked-by: Mark Michelson
On 2/12/24 10:55, Ales Musil wrote:
There were some comments left with hardcoded numbers. Even if it
wouldn't break any test table shift/change it would just leave the
comment outdated.
Signed-off-by: Ales Musil
---
v4: Rebase on top of main.
Align the northd.at comment with others.
---
tests/ovn-northd.at | 2 +-
tests/ovn.at| 8
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index 591ad5aad..1547e6086 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -2150,7 +2150,7 @@ AT_CLEANUP
# This test case tests that when a logical switch has load balancers associated
# (with VIPs configured), the below logical flow is added by ovn-northd.
-# table=1 (ls_out_pre_lb ), priority=100 , match=(ip), action=(reg0[[0]]
= 1; next;)
+# table=ls_out_pre_lb, priority=100, match=(ip), action=(reg0[[0]] = 1; next;)
# This test case is added for the BZ -
# https://bugzilla.redhat.com/show_bug.cgi?id=1849162
#
diff --git a/tests/ovn.at b/tests/ovn.at
index 902dd3793..0d31d5cbb 100644
--- a/tests/ovn.at
+++ b/tests/ovn.at
@@ -18423,8 +18423,8 @@ AT_CHECK([cat 2.packets], [0], [expout])
# There should be total of 9 flows present with conjunction action and 2 flows
# with conj match. Eg.
-# table=44, priority=2001,conj_id=2,metadata=0x1 actions=resubmit(,45)
-# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
+# table=ls_out_acl_eval, priority=2001,conj_id=2,metadata=0x1
actions=resubmit(,ls_out_acl_action)
+# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop
# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.6 actions=conjunction(2,2/2)
# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,2/2)
# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.5 actions=conjunction(2,2/2)
@@ -18464,7 +18464,7 @@ AT_CHECK([cat 2.packets], [0], [])
# properly.
# There should be total of 6 flows present with conjunction action and 1 flow
# with conj match. Eg.
-# table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
+# table=ls_out_acl_eval, priority=2001,conj_id=3,metadata=0x1 actions=drop
# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7 actions=conjunction(4,2/2)
# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9 actions=conjunction(4,2/2)
# priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8 actions=conjunction(4,2/2)
@@ -34347,7 +34347,7 @@ in_port_sec=OFTABLE_CHK_IN_PORT_SEC
in_port_sec_nd=OFTABLE_CHK_IN_PORT_SEC_ND
out_port_sec=OFTABLE_CHK_OUT_PORT_SEC
-# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC, 74 and 75 in hv1 and hv2
+# There should be no flows in table OFTABLE_CHK_IN_PORT_SEC,
OFTABLE_CHK_IN_PORT_SEC_ND and OFTABLE_CHK_OUT_PORT_SEC in hv1 and hv2
> hv1_t${in_port_sec}_flows.expected
> hv1_t${in_port_sec_nd}_flows.expected
> hv1_t${out_port_sec}_flows.expected
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH ovn 00/10] Fixes to Unit tests.
On 2/13/24 03:47, Ales Musil wrote: On Mon, Feb 12, 2024 at 6:56 PM Xavier Simonart wrote: Check unit tests logs for errors. Fix multiple unit tests issues highlighted when checking logs for errors. Xavier Simonart (10): tests: Have tests fail when adding veth peer fails. tests: Fix typos in tests. tests: Fix flaky "options:requested-chassis ...". tests: Fix flaky "ovn-controller-vtep - binding 1". tests: Fix "ofctrl wait before clearing flows". tests: Fix "ovn-controller - Chassis other_config". tests: Fix "Load balancer incremental processing". tests: Fix "router port type update and then ...". tests: Speed up "multicast group buffer split". tests: Check unit tests logs for errors. tests/ovn-controller-vtep.at | 2 +- tests/ovn-controller.at | 108 +--- tests/ovn-ic.at | 4 +- tests/ovn-macros.at | 61 +-- tests/ovn-northd.at | 2 + tests/ovn.at | 298 +++--- tests/system-common-macros.at | 2 +- 7 files changed, 381 insertions(+), 96 deletions(-) -- 2.41.0 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev The whole series looks good to me, thanks! Acked-by: Ales Musil Thanks Xavier and Ales. I pushed the entire series to main and branch-24.03. I then was able to cherry-pick all patches except for patches 7, 9, and 10 to branch-23.09, branch-23.06, branch-23.03, and branch-22.12. Once I started trying to backport to branch-22.09, I encountered conflicts I didn't feel comfortable trying to resolve on my own. If you think these should be backported further, please send backport patches for 22.09, 22.06, and 22.03. 22.03 is the current LTS, but it won't be for much longer, and since these are test fixes, they aren't necessarily essential to backport any further in my opinion. We're a few weeks away from 23.03 being the oldest supported branch for normal bug fixes, so do with that what you will :) ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [RFC 0/7] selftests: openvswitch: cleanups for running as selftests
On Fri, 16 Feb 2024 10:28:39 -0500 Aaron Conole wrote: > The series is a host of cleanups to the openvswitch selftest suite > which should be ready to run under the netdev selftest runners using > vng. For now, the testing has been done with RW directories, but > additional testing will be done to try and keep it all as RO to be > more friendly. Would it be an option to make the output go into a dir in /tmp/ instead of in place, in the tree? mktemp -p /tmp/ -d ovs-test.X or such? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] dpif-netlink: Fix overriding the number of handler threads.
On 2/8/24 16:53, Eelco Chaudron wrote: > > > On 8 Feb 2024, at 15:00, Ilya Maximets wrote: > >> On 2/8/24 13:44, Eelco Chaudron wrote: >>> >>> >>> On 6 Feb 2024, at 16:01, Ilya Maximets wrote: >>> On 2/6/24 15:07, Eelco Chaudron wrote: > > > On 6 Feb 2024, at 14:46, Ilya Maximets wrote: > >> On 2/6/24 14:30, Eelco Chaudron wrote: >>> Previously, the ability to override the system default for the number >>> of handler threads was broken since to the introduction of the per-CPU >>> upcall handlers. >> >> It wasn't broken, it was intentionally disabled. I don't think we should >> introduce ability to configure the number of handlers for per-CPU >> dispatch >> mode. It only allows users to shoot themselves in a foot if they do not >> understand what they are doing. > > I think the design was not thought through enough, and resulted in a lot > of > corner case issues. From the start, we should still have allowed tuning of > this option as it was before. The tuning is much more complicated and has very different consequences with per-cpu dispatch compared to per-vport. So, the results of adjusting this option can be very different. >>> >> Many integration scripts in a wild are still blindly setting these >> options >> without even knowing that the underlying implementation changed >> completely. >> And those setups may suffer if we suddenly decide to respect the >> configuration >> that is ignored on modern kernels for all currently supported versions >> of OVS. > > I feel like this is not OVS’s problem. If people do not know what they > are doing, > they should not touch it… We have a nice Dutch saying for this "Wie zijn > billen > brandt, moet op de blaren zitten” :) It is an OVS's problem for the same reason as above. We changed the logic under the hood so much that users that expected a certain behavior in the past may have a very different behavior now. We can't just change the meaning of the knob that easily. See below. >>> >>> Ack, so I would suggest adding a new nob. >>> > > However, I do feel like we should have an option to configure this, as it > makes > no sense on a 256-core system to create 256 handler threads. It actually makes perfect sense. Since the traffic can appear on 256 cores, we should be able to handle it on each one of them in order to have balanced load. With per-vport dispatch, on a system with 10 cores and 5 handler threads, all 5 handler threads will share the load, because each one of them listens on all the ports, hence each one of them can process packets whenever it has time to do so. >>> >>> Yes, but this had other problems :) >> >> Sure, that's why we went away from that logic. :) >> >>> With per-CPU dispatch, on a same system these 5 threads are mapped to specific cores, 2 cores per thread. If all the traffic comes on 4 cores, we may have 2 threads overloaded and 3 other threads do nothing. In the same scenario with per-vport dispatch we would have all 5 threads sharing the load. >>> >>> Yes, but with 256 we might have 250 doing nothing, and looking at some data >>> from a >>> system with decent OpenFlow tables (1M+ entries, but only a couple of >>> tables), we >>> can handle about 232k upcalls on a single handler thread for 64 bytes >>> packets before >>> we run out of socket buffer. For 512bytes around 212k, and 1500 around 189k. >> >> Having idle threads in a system is generally not a problem. As long as we >> don't have hundreds of thousands of them, they will not impact scheduling >> performance. >> >> Also, actual performance of handlers is hard to quantify, since OVS can be >> running on a large variety of different hardware. Yours is likely on a >> powerful side of a spectrum. >> >>> >>> If we have more than 200k upcalls per second, we have other problems, >>> especially as >>> the default dp flow size is 200k. >>> >>> So I guess the new nob becomes tunable for resource consumption vs upcall >>> handling, >>> which I think is fine, and might be desirable in certain configurations. We >>> have other >>> nobs in the system that require a decent amount of knowledge. >> >> Having more knobs just for the sake of having something configurable >> doesn't sound good to me. We should try to not introduce ways to >> configure something that doesn't need to be configured. >> >> Sure, creating spare threads consumes memory. However there are few >> things to consider here: >> >> 1. A system with 256 cores likely has a 512 MB of RAM to spare >>(By default we limit to 2 MB of stack per thread). >>I haven't seen any complaint from users about memory consumption >>on large systems. >> >> 2. By reducing the number of threads we're working around the problem
Re: [ovs-dev] [PATCH v3] netlink-conntrack: Optimize flushing ct zone.
On 2/12/24 11:38, Felix Huettner via dev wrote: > Previously the kernel did not provide a netlink interface to flush/list > only conntrack entries matching a specific zone. With [1] and [2] it is now > possible to flush and list conntrack entries filtered by zone. Older > kernels not yet supporting this feature will ignore the filter. > For the list request that means just returning all entries (which we can > then filter in userspace as before). > For the flush request that means deleting all conntrack entries. > > These significantly improves the performance of flushing conntrack zones > when the conntrack table is large. Since flushing a conntrack zone is > normally triggered via an openflow command it blocks the main ovs thread > and thereby also blocks new flows from being applied. Using this new > feature we can reduce the flushing time for zones by around 93%. > > In combination with OVN the creation of a Logical_Router (which causes > the flushing of a ct zone) could block other operations, e.g. the > failover of Logical_Routers (as they cause new flows to be created). > This is visible from a user perspective as a ovn-controller that is idle > (as it waits for vswitchd) and vswitchd reporting: > "blocked 1000 ms waiting for main to quiesce" (potentially with ever > increasing times). > > The following performance tests where run in a qemu vm with 500.000 > conntrack entries distributed evenly over 500 ct zones using `ovstest > test-netlink-conntrack flush zone=`. > > With this patch and kernel v6.8-rc4: > > - >Min (s) Median (s) 90%ile (s) > 99%ile (s) Max (s) Mean (s)Total (s) Count > - > flush zone with 1000 entries 0.260 0.319 0.335 > 0.348 0.362 0.320 80.02 250 > flush zone with no entry 0.228 0.298 0.325 > 0.340 0.348 0.296 73.93 250 > - > > With this patch and kernel v6.7.1: > > - >Min (s) Median (s) 90%ile (s) > 99%ile (s) Max (s) Mean (s)Total (s) Count > - > flush zone with 1000 entries 3.946 4.237 4.367 > 4.495 4.543 4.236 1058.992250 > flush zone with no entry 3.462 4.460 4.662 > 4.931 5.390 4.430 1107.479250 > - > > Without this patch and kernel v6.8-rc4: > > - >Min (s) Median (s) 90%ile (s) > 99%ile (s) Max (s) Mean (s)Total (s) Count > - > flush zone with 1000 entries 3.497 4.349 4.522 > 4.773 5.054 4.331 1082.802250 > flush zone with no entry 3.212 4.010 4.572 > 6.003 6.396 4.071 1017.838250 > - > Maybe transpose these tables? They will not look good in a git log. Frankly, they barely fit into my email client window. :) Potentially, collect all 3 tables into one. A quick mock up for example: | flush zone with 1000 entries |flush zone with no entry +-+--+-+-- |with the patch | without |with the patch | without +--+--+--+-+-- | v6.8-rc4 | v6.7.1 | v6.8-rc4 | v6.8-rc4 | v6.7.1 | v6.8-rc4
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
Eelco Chaudron writes: > On 12 Feb 2024, at 15:15, Aaron Conole wrote: > >> Aaron Conole writes: >> >>> Eelco Chaudron writes: >>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote: > On 2/1/24 10:02, Eelco Chaudron wrote: >> >> >> On 31 Jan 2024, at 18:03, Aaron Conole wrote: >> >>> Eelco Chaudron writes: >>> On 25 Jan 2024, at 21:55, Aaron Conole wrote: > From: Kevin Sprague > > 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) > which serves as a demonstration of how the new USDT probe might be > used > going forward. > > Signed-off-by: Kevin Sprague > Co-authored-by: Aaron Conole > Signed-off-by: Aaron Conole Thanks for following this up Aaron! See comments on this patch below. I have no additional comments on patch 2. Cheers, Eelco > --- > Documentation/topics/usdt-probes.rst | 1 + > ofproto/ofproto-dpif-upcall.c| 42 +- > utilities/automake.mk| 3 + > utilities/usdt-scripts/flow_reval_monitor.py | 653 > +++ > 4 files changed, 693 insertions(+), 6 deletions(-) > create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py > > diff --git a/Documentation/topics/usdt-probes.rst > b/Documentation/topics/usdt-probes.rst > index e527f43bab..a8da9bb1f7 100644 > --- a/Documentation/topics/usdt-probes.rst > +++ b/Documentation/topics/usdt-probes.rst > @@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``: > - dpif_recv:recv_upcall > - main:poll_block > - main:run_start > +- revalidate:flow_result > - revalidate_ukey\_\_:entry > - revalidate_ukey\_\_:exit > - udpif_revalidator:start_dump You are missing the specific flow_result result section. This is from the previous patch: >>> >>> D'oh! Thanks for catching it. I'll re-add it. >>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above. - ``utilities/usdt-scripts/bridge_loop.bt`` +probe revalidate:flow_result +~ + +**Description**: +This probe is triggered when the revalidator decides whether or not to +revalidate a flow. ``reason`` is an enum that denotes that either the flow +is being kept, or the reason why the flow is being deleted. The +``flow_reval_monitor.py`` script uses this probe to notify users when flows +matching user-provided criteria are deleted. + +**Arguments**: + +- *arg0*: ``(enum flow_del_reason) reason`` +- *arg1*: ``(struct udpif *) udpif`` +- *arg2*: ``(struct udpif_key *) ukey`` + +**Script references**: + +- ``utilities/usdt-scripts/flow_reval_monitor.py`` + + Adding your own probes -- > diff --git a/ofproto/ofproto-dpif-upcall.c > b/ofproto/ofproto-dpif-upcall.c > index b5cbeed878..97d75833f7 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@
Re: [ovs-dev] [PATCH v3 2/2] conntrack: Handle persistent selection for IP addresses.
Paolo Valerio writes: > The patch, when 'persistent' flag is specified, makes the IP selection > in a range persistent across reboots. > > Signed-off-by: Paolo Valerio > Acked-by: Simon Horman > --- Acked-by: Aaron Conole ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 1/2] conntrack: Handle random selection for port ranges.
Paolo Valerio writes:
> The userspace conntrack only supported hash for port selection.
> With the patch, both userspace and kernel datapath support the random
> flag.
>
> The default behavior remains the same, that is, if no flags are
> specified, hash is selected.
>
> Signed-off-by: Paolo Valerio
> Acked-by: Simon Horman
> ---
Acked-by: Aaron Conole
> Documentation/ref/ovs-actions.7.rst | 3 +--
> NEWS| 3 +++
> lib/conntrack.c | 15 ---
> lib/conntrack.h | 5 +
> lib/dpif-netdev.c | 4 +++-
> 5 files changed, 20 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/ref/ovs-actions.7.rst
> b/Documentation/ref/ovs-actions.7.rst
> index 36adcc5db..80acd9070 100644
> --- a/Documentation/ref/ovs-actions.7.rst
> +++ b/Documentation/ref/ovs-actions.7.rst
> @@ -1551,8 +1551,7 @@ following arguments:
> should be selected. When a port range is specified, fallback to
> ephemeral ports does not happen, else, it will. The port number
> selection can be informed by the optional ``random`` and ``hash``
> flags
> -described below. The userspace datapath only supports the ``hash``
> -behavior.
> +described below.
>
> The optional *flags* are:
>
> diff --git a/NEWS b/NEWS
> index a6617546c..93046b963 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -1,5 +1,8 @@
> Post-v3.3.0
>
> + - Userspace datapath:
> + * Conntrack now supports 'random' flag for selecting ports in a range
> + while natting.
>
>
> v3.3.0 - xx xxx
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 013709bd6..e09ecdf33 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -,7 +,7 @@ nat_range_hash(const struct conn_key *key, uint32_t
> basis,
> /* Ports are stored in host byte order for convenience. */
> static void
> set_sport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> -uint32_t hash, uint16_t *curr, uint16_t *min,
> +uint32_t off, uint16_t *curr, uint16_t *min,
> uint16_t *max)
> {
> if (((ni->nat_action & NAT_ACTION_SNAT_ALL) == NAT_ACTION_SRC) ||
> @@ -2241,19 +2241,19 @@ set_sport_range(const struct nat_action_info_t *ni,
> const struct conn_key *k,
> } else {
> *min = ni->min_port;
> *max = ni->max_port;
> -*curr = *min + (hash % ((*max - *min) + 1));
> +*curr = *min + (off % ((*max - *min) + 1));
> }
> }
>
> static void
> set_dport_range(const struct nat_action_info_t *ni, const struct conn_key *k,
> -uint32_t hash, uint16_t *curr, uint16_t *min,
> +uint32_t off, uint16_t *curr, uint16_t *min,
> uint16_t *max)
> {
> if (ni->nat_action & NAT_ACTION_DST_PORT) {
> *min = ni->min_port;
> *max = ni->max_port;
> -*curr = *min + (hash % ((*max - *min) + 1));
> +*curr = *min + (off % ((*max - *min) + 1));
> } else {
> *curr = ntohs(k->dst.port);
> *min = *max = *curr;
> @@ -2388,18 +2388,19 @@ nat_get_unique_tuple(struct conntrack *ct, struct
> conn *conn,
> fwd_key->nw_proto == IPPROTO_SCTP;
> uint16_t min_dport, max_dport, curr_dport;
> uint16_t min_sport, max_sport, curr_sport;
> -uint32_t hash;
> +uint32_t hash, port_off;
>
> hash = nat_range_hash(fwd_key, ct->hash_basis, nat_info);
> +port_off = nat_info->nat_flags & NAT_RANGE_RANDOM ? random_uint32() :
> hash;
> min_addr = nat_info->min_addr;
> max_addr = nat_info->max_addr;
>
> find_addr(fwd_key, _addr, _addr, , hash,
>(fwd_key->dl_type == htons(ETH_TYPE_IP)), nat_info);
>
> -set_sport_range(nat_info, fwd_key, hash, _sport,
> +set_sport_range(nat_info, fwd_key, port_off, _sport,
> _sport, _sport);
> -set_dport_range(nat_info, fwd_key, hash, _dport,
> +set_dport_range(nat_info, fwd_key, port_off, _dport,
> _dport, _dport);
>
> if (pat_proto) {
> diff --git a/lib/conntrack.h b/lib/conntrack.h
> index 0a888be45..9b0c6aa88 100644
> --- a/lib/conntrack.h
> +++ b/lib/conntrack.h
> @@ -77,12 +77,17 @@ enum nat_action_e {
> NAT_ACTION_DST_PORT = 1 << 3,
> };
>
> +enum nat_flags_e {
> +NAT_RANGE_RANDOM = 1 << 0,
> +};
> +
> struct nat_action_info_t {
> union ct_addr min_addr;
> union ct_addr max_addr;
> uint16_t min_port;
> uint16_t max_port;
> uint16_t nat_action;
> +uint16_t nat_flags;
> };
>
> struct conntrack *conntrack_init(void);
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c1981137f..c3334c667 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9409,9 +9409,11 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> nl_attr_get_u16(b_nest);
>
Re: [ovs-dev] [PATCH v3] netlink-conntrack: Optimize flushing ct zone.
Felix Huettner writes:
>> > diff --git a/lib/netlink-conntrack.c b/lib/netlink-conntrack.c
>> > index 492bfcffb..1b050894d 100644
>> > --- a/lib/netlink-conntrack.c
>> > +++ b/lib/netlink-conntrack.c
>> > @@ -25,6 +25,7 @@
>> > #include
>> > #include
>> > #include
>> > +#include
>> >
>> > #include "byte-order.h"
>> > #include "compiler.h"
>> > @@ -141,6 +142,9 @@ nl_ct_dump_start(struct nl_ct_dump_state **statep,
>> > const uint16_t *zone,
>> >
>> > nl_msg_put_nfgenmsg(>buf, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK,
>> > IPCTNL_MSG_CT_GET, NLM_F_REQUEST);
>> > +if (zone) {
>> > +nl_msg_put_be16(>buf, CTA_ZONE, htons(*zone));
>> > +}
>> > nl_dump_start(>dump, NETLINK_NETFILTER, >buf);
>> > ofpbuf_clear(>buf);
>> >
>> > @@ -283,23 +287,72 @@ nl_ct_flush_zone(uint16_t flush_zone)
>> > return err;
>> > }
>> > #else
>> > +
>> > +static bool
>> > +netlink_flush_supports_zone(void)
>> > +{
>> > +static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> > +static bool supported = false;
>> > +
>> > +if (ovsthread_once_start()) {
>> > +struct utsname utsname;
>> > +int major, minor;
>> > +
>> > +if (uname() == -1) {
>> > +VLOG_WARN("uname failed (%s)", ovs_strerror(errno));
>> > +} else if (!ovs_scan(utsname.release, "%d.%d", , )) {
>> > +VLOG_WARN("uname reported bad OS release (%s)",
>> > utsname.release);
>>
>> Do these WARN calls need to be severe like this? There isn't much for
>> the user to do about this, and it won't impact the functioning of the
>> system in such a major way. Maybe they can be INFO instead? Otherwise,
>> we may need to change most of the selftests to ignore the "uname failed"
>> warnings.
>>
>> More of a nit, because it shouldn't generally fail on any systems.
>
> That was also my thought. I actually copied most of the logic from
> `getqdisc_is_safe` in `netdev-linux.c`.
>
> I am completely fine with changing it to something else if that makes
> everyones lives easier, since hitting it is so unrealistic.
I think it makes sense to just have a single function that the tc and
conntrack layers can call for this functionality, while we can just make
the logs at INFO level - then no issues with strange test environments.
And it doesn't prevent anything from running, or result in 'degraded'
performance - just prevents an optimization.
WDYT?
> Let me know what you would prefer.
>
> Thanks
> Felix
>
>>
>> > +} else if (major < 6 || (major == 6 && minor < 8)) {
>> > +VLOG_INFO("disabling conntrack flush by zone in Linux kernel
>> > %s",
>> > + utsname.release);
>> > +} else {
>> > +supported = true;
>> > +}
>> > +ovsthread_once_done();
>> > +}
>> > +return supported;
>> > +}
>> > +
>> > int
>> > nl_ct_flush_zone(uint16_t flush_zone)
>> > {
>> > -/* Apparently, there's no netlink interface to flush a specific zone.
>> > +/* In older kernels, there was no netlink interface to flush a
>> > specific
>> > + * conntrack zone.
>> > * This code dumps every connection, checks the zone and eventually
>> > * delete the entry.
>> > + * In newer kernels there is the option to specifiy a zone for
>> > filtering
>> > + * during dumps. Older kernels ignore this option. We set it here in
>> > the
>> > + * hope we only get relevant entries back, but fall back to filtering
>> > here
>> > + * to keep compatibility.
>> > *
>> > - * This is race-prone, but it is better than using shell scripts. */
>> > + * This is race-prone, but it is better than using shell scripts.
>> > + *
>> > + * Additionally newer kernels also support flushing a zone without
>> > listing
>> > + * it first. */
>> >
>> > struct nl_dump dump;
>> > struct ofpbuf buf, reply, delete;
>> > +int err;
>> > +
>> > +if (netlink_flush_supports_zone()) {
>> > +ofpbuf_init(, NL_DUMP_BUFSIZE);
>> > +
>> > +nl_msg_put_nfgenmsg(, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK,
>> > +IPCTNL_MSG_CT_DELETE, NLM_F_REQUEST);
>> > +nl_msg_put_be16(, CTA_ZONE, htons(flush_zone));
>> > +
>> > +err = nl_transact(NETLINK_NETFILTER, , NULL);
>> > +ofpbuf_uninit();
>> > +
>> > +return err;
>> > +}
>> >
>> > ofpbuf_init(, NL_DUMP_BUFSIZE);
>> > ofpbuf_init(, NL_DUMP_BUFSIZE);
>> >
>> > nl_msg_put_nfgenmsg(, 0, AF_UNSPEC, NFNL_SUBSYS_CTNETLINK,
>> > IPCTNL_MSG_CT_GET, NLM_F_REQUEST);
>> > +nl_msg_put_be16(, CTA_ZONE, htons(flush_zone));
>> > nl_dump_start(, NETLINK_NETFILTER, );
>> > ofpbuf_clear();
>> >
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] OVN technical community meeting - February 19th
On 1/9/24 13:30, Dumitru Ceara wrote: > I went ahead and scheduled a new instance of the meeting for: > > Date/Time: Monday February 19th 16:00 UTC > Meeting link: meet.google.com/zns-gqsd-jdn > Meeting notes: > https://docs.google.com/document/d/1dG4GwcYOSs4uArPGtOoaP5tH4KCto-GH_C3tIXSnZZ8 > > If you'd like me to explicitly add you to the meeting invite please let > me know. Just a reminder, for those who wish to join. The OVN technical community meeting is scheduled for today, 4PM UTC (in ~1.5 hours). Regards, Dumitru ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/2] conntrack: Handle persistent selection for IP addresses.
On Fri, Feb 16, 2024 at 06:19:14PM +0100, Paolo Valerio wrote: > The patch, when 'persistent' flag is specified, makes the IP selection > in a range persistent across reboots. > > Signed-off-by: Paolo Valerio > Acked-by: Simon Horman > --- > v3: > - rearranged branches in nat_get_unique_tuple() (Simon) Thanks Paolo, For the record I'm (still) happy with this patch. I'll plan to apply this series unless there is feedback to the contrary in the next few days. ... ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] upcall: Check flow consistant in upcall.
Ovs ko passes odp key and packet to userspace. Next packet is
extracted into flow, which is the input for xlate to generate wc.
At last, ukey(= odp_key/wc) is installed into datapath. If the
odp_key is not consistant with packet extracted flow. The ukey
will be wrong.
commit [1] was created to fix inconsistance caused by bad tcp
header. commit [2] was cretaed to fix inconsistance caused by bad
ip header. There is no guarantee of the consistance of odp_key and
packet flow. So it is necessary to make the check to prevent from
installing wrong ukey.
[1] 1f5749c790accd98dbcafdaefc40bf5e52d7c672
[2] 79349cbab0b2a755140eedb91833ad2760520a83
Signed-off-by: Cheng Li
---
Notes:
v2: Leverage avoid_caching to avoid ukey install.
ofproto/ofproto-dpif-upcall.c | 14 ++
1 file changed, 14 insertions(+)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index b5cbeed87..bd93f0981 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -66,6 +66,7 @@ COVERAGE_DEFINE(upcall_flow_limit_reduced);
COVERAGE_DEFINE(upcall_flow_limit_scaled);
COVERAGE_DEFINE(upcall_ukey_contention);
COVERAGE_DEFINE(upcall_ukey_replace);
+COVERAGE_DEFINE(upcall_packet_flow_inconsistant);
/* A thread that reads upcalls from dpif, forwards each upcall's packet,
* and possibly sets up a kernel flow as a cache. */
@@ -840,6 +841,7 @@ recv_upcalls(struct handler *handler)
struct dpif_upcall dupcalls[UPCALL_MAX_BATCH];
struct upcall upcalls[UPCALL_MAX_BATCH];
struct flow flows[UPCALL_MAX_BATCH];
+struct flow odp_key_flow;
size_t n_upcalls, i;
n_upcalls = 0;
@@ -903,6 +905,8 @@ recv_upcalls(struct handler *handler)
upcall->out_tun_key = dupcall->out_tun_key;
upcall->actions = dupcall->actions;
+/* Save odp flow before overwrite. */
+memcpy(_key_flow, flow, sizeof odp_key_flow);
pkt_metadata_from_flow(>packet.md, flow);
flow_extract(>packet, flow);
@@ -912,6 +916,12 @@ recv_upcalls(struct handler *handler)
goto cleanup;
}
+if (!flow_equal_except(_key_flow, flow, >wc)) {
+/* If odp flow is not consistant with flow extract from packet,
+ * bad ukey/mask will be installed. */
+COVERAGE_INC(upcall_packet_flow_inconsistant);
+upcall->xout.avoid_caching = true;
+}
n_upcalls++;
continue;
@@ -1376,6 +1386,10 @@ should_install_flow(struct udpif *udpif, struct upcall
*upcall)
return false;
}
+if (upcall->xout.avoid_caching) {
+return false;
+}
+
atomic_read_relaxed(>flow_limit, _limit);
if (udpif_get_n_flows(udpif) >= flow_limit) {
COVERAGE_INC(upcall_flow_limit_hit);
--
2.39.3
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v8 1/2] revalidator: Add a USDT probe during flow deletion with purge reason.
On 12 Feb 2024, at 15:15, Aaron Conole wrote:
> Aaron Conole writes:
>
>> Eelco Chaudron writes:
>>
>>> On 2 Feb 2024, at 11:31, Adrian Moreno wrote:
>>>
On 2/1/24 10:02, Eelco Chaudron wrote:
>
>
> On 31 Jan 2024, at 18:03, Aaron Conole wrote:
>
>> Eelco Chaudron writes:
>>
>>> On 25 Jan 2024, at 21:55, Aaron Conole wrote:
>>>
From: Kevin Sprague
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)
which serves as a demonstration of how the new USDT probe might be used
going forward.
Signed-off-by: Kevin Sprague
Co-authored-by: Aaron Conole
Signed-off-by: Aaron Conole
>>>
>>> Thanks for following this up Aaron! See comments on this patch
>>> below. I have no additional comments on patch 2.
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>>
---
Documentation/topics/usdt-probes.rst | 1 +
ofproto/ofproto-dpif-upcall.c| 42 +-
utilities/automake.mk| 3 +
utilities/usdt-scripts/flow_reval_monitor.py | 653
+++
4 files changed, 693 insertions(+), 6 deletions(-)
create mode 100755 utilities/usdt-scripts/flow_reval_monitor.py
diff --git a/Documentation/topics/usdt-probes.rst
b/Documentation/topics/usdt-probes.rst
index e527f43bab..a8da9bb1f7 100644
--- a/Documentation/topics/usdt-probes.rst
+++ b/Documentation/topics/usdt-probes.rst
@@ -214,6 +214,7 @@ Available probes in ``ovs_vswitchd``:
- dpif_recv:recv_upcall
- main:poll_block
- main:run_start
+- revalidate:flow_result
- revalidate_ukey\_\_:entry
- revalidate_ukey\_\_:exit
- udpif_revalidator:start_dump
>>>
>>> You are missing the specific flow_result result section. This is from
>>> the previous patch:
>>
>> D'oh! Thanks for catching it. I'll re-add it.
>>
>>> @@ -358,6 +360,27 @@ See also the ``main:run_start`` probe above.
>>> - ``utilities/usdt-scripts/bridge_loop.bt``
>>>
>>>
>>> +probe revalidate:flow_result
>>> +~
>>> +
>>> +**Description**:
>>> +This probe is triggered when the revalidator decides whether or not to
>>> +revalidate a flow. ``reason`` is an enum that denotes that either the
>>> flow
>>> +is being kept, or the reason why the flow is being deleted. The
>>> +``flow_reval_monitor.py`` script uses this probe to notify users when
>>> flows
>>> +matching user-provided criteria are deleted.
>>> +
>>> +**Arguments**:
>>> +
>>> +- *arg0*: ``(enum flow_del_reason) reason``
>>> +- *arg1*: ``(struct udpif *) udpif``
>>> +- *arg2*: ``(struct udpif_key *) ukey``
>>> +
>>> +**Script references**:
>>> +
>>> +- ``utilities/usdt-scripts/flow_reval_monitor.py``
>>> +
>>> +
>>> Adding your own probes
>>> --
>>>
diff --git a/ofproto/ofproto-dpif-upcall.c
b/ofproto/ofproto-dpif-upcall.c
index b5cbeed878..97d75833f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -269,6 +269,18 @@ enum ukey_state {
};
#define N_UKEY_STATES (UKEY_DELETED + 1)
+enum flow_del_reason {
+FDR_REVALIDATE =
Re: [ovs-dev] [PATCH v1 12/12] documentation: Document ovs-flowviz.
Bleep bloop. Greetings Adrian Moreno, 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 81 characters long (recommended limit is 79)
#101 FILE: Documentation/topics/flow-visualization.rst:59:
It reads openflow and datapath flows (such as the output of ovs-ofctl
dump-
WARNING: Line is 81 characters long (recommended limit is 79)
#102 FILE: Documentation/topics/flow-visualization.rst:60:
flows or ovs-appctl dpctl/dump-flows) and prints them in different
formats.
WARNING: Line is 82 characters long (recommended limit is 79)
#105 FILE: Documentation/topics/flow-visualization.rst:63:
-c, --config PATH Use config file [default:
/home/amorenoz/src/ovs/pyth
WARNING: Line is 80 characters long (recommended limit is 79)
#109 FILE: Documentation/topics/flow-visualization.rst:67:
-i, --input PATH Read flows from specified filepath. If not provided,
WARNING: Line is 80 characters long (recommended limit is 79)
#112 FILE: Documentation/topics/flow-visualization.rst:70:
Where alias is a name that shall be used to refer to
WARNING: Line is 82 characters long (recommended limit is 79)
#115 FILE: Documentation/topics/flow-visualization.rst:73:
'ovs-flowviz filter' for a detailed description of
the
WARNING: Line is 81 characters long (recommended limit is 79)
#117 FILE: Documentation/topics/flow-visualization.rst:75:
-l, --highlight TEXT Highlight flows that match the filter expression.
Run
WARNING: Line is 89 characters long (recommended limit is 79)
#142 FILE: Documentation/topics/flow-visualization.rst:100:
- Prints the flows in a configurable, colorful style in the console sorted
by table.
WARNING: Line is 81 characters long (recommended limit is 79)
#210 FILE: Documentation/topics/flow-visualization.rst:168:
[! | not ] {key}[[.subkey]...] [OPERATOR] {value})] [LOGICAL OPERATOR]
...
WARNING: Line is 80 characters long (recommended limit is 79)
#224 FILE: Documentation/topics/flow-visualization.rst:182:
To compare against a match or info field, use the field directly, e.g:
WARNING: Line is 80 characters long (recommended limit is 79)
#231 FILE: Documentation/topics/flow-visualization.rst:189:
Actions values might be dictionaries, use subkeys to access individual
WARNING: Line is 146 characters long (recommended limit is 79)
#290 FILE: Documentation/topics/flow-visualization.rst:248:
├── priority=180 priority,conj_id,in_port,vlan_tci --->
set_field,set_field,set_field,set_field,set_field,set_field resubmit(,8), ( x 1
)
WARNING: Line is 85 characters long (recommended limit is 79)
#291 FILE: Documentation/topics/flow-visualization.rst:249:
├── priority=100 priority,in_port ---> move,move,move resubmit(,40), (
x 6 )
WARNING: Line is 120 characters long (recommended limit is 79)
#292 FILE: Documentation/topics/flow-visualization.rst:250:
├── priority=100 priority,in_port --->
set_field,set_field,set_field,set_field,set_field resubmit(,8), ( x 15 )
WARNING: Line is 128 characters long (recommended limit is 79)
#293 FILE: Documentation/topics/flow-visualization.rst:251:
├── priority=100 priority,in_port,vlan_tci --->
set_field,set_field,set_field,set_field,set_field resubmit(,8), ( x 1 )
WARNING: Line is 136 characters long (recommended limit is 79)
#294 FILE: Documentation/topics/flow-visualization.rst:252:
├── priority=100 priority,in_port,dl_vlan --->
pop_vlan,set_field,set_field,set_field,set_field,set_field resubmit(,8), ( x 1 )
WARNING: Line is 103 characters long (recommended limit is 79)
#321 FILE: Documentation/topics/flow-visualization.rst:279:
── recirc_id(0),in_port(3),eth(...),ipv4(...),tcp(dst=8181),
actions:ct(zone=2,nat),recirc(0x19348)
WARNING: Line is 155 characters long (recommended limit is 79)
#322 FILE: Documentation/topics/flow-visualization.rst:280:
│ ├──
recirc_id(0x19348),in_port(3),ct_state(-new+est-rel-rpl-inv+trk),ct_label(0/0x3),eth(...),eth_type,ipv4(),
actions:ct(zone=27,nat),recirc(0x10)
WARNING: Line is 107 characters long (recommended limit is 79)
#323 FILE: Documentation/topics/flow-visualization.rst:281:
│ │ ├──
recirc_id(0x10),in_port(3),ct_state(-new+est-rel-rpl-inv+trk),eth(...),ipv4(...),
actions:9
WARNING: Line is 107 characters long (recommended limit is 79)
#324 FILE: Documentation/topics/flow-visualization.rst:282:
│ │ ├──
recirc_id(0x10),in_port(3),ct_state(-new+est-rel+rpl-inv+trk),eth(...),ipv4(...),
actions:9
WARNING: Line is 138 characters long (recommended limit is 79)
#325 FILE: Documentation/topics/flow-visualization.rst:283:
│ │ └──
recirc_id(0x10),in_port(3),ct_state(+new-est-rel-rpl-inv+trk),eth(...),ipv4(...),
actions:ct(commit,zone=27,label=0/0x1),9
WARNING: Line is 164 characters long
[ovs-dev] [PATCH v1 12/12] documentation: Document ovs-flowviz.
Add a page for flow visualization with a few key concepts and examples. Signed-off-by: Adrian Moreno --- Documentation/automake.mk | 3 +- Documentation/topics/flow-visualization.rst | 469 Documentation/topics/index.rst | 1 + 3 files changed, 472 insertions(+), 1 deletion(-) create mode 100644 Documentation/topics/flow-visualization.rst diff --git a/Documentation/automake.mk b/Documentation/automake.mk index 47d2e336a..f1339e876 100644 --- a/Documentation/automake.mk +++ b/Documentation/automake.mk @@ -45,7 +45,7 @@ DOC_SOURCE = \ Documentation/topics/fuzzing/ovs-fuzzing-infrastructure.rst \ Documentation/topics/fuzzing/ovs-fuzzers.rst \ Documentation/topics/fuzzing/security-analysis-of-ovs-fuzzers.rst \ - Documentation/topics/testing.rst \ + Documentation/topics/flow-visualization.rst \ Documentation/topics/integration.rst \ Documentation/topics/language-bindings.rst \ Documentation/topics/networking-namespaces.rst \ @@ -55,6 +55,7 @@ DOC_SOURCE = \ Documentation/topics/ovsdb-replication.rst \ Documentation/topics/porting.rst \ Documentation/topics/record-replay.rst \ + Documentation/topics/testing.rst \ Documentation/topics/tracing.rst \ Documentation/topics/usdt-probes.rst \ Documentation/topics/userspace-checksum-offloading.rst \ diff --git a/Documentation/topics/flow-visualization.rst b/Documentation/topics/flow-visualization.rst new file mode 100644 index 0..366275c54 --- /dev/null +++ b/Documentation/topics/flow-visualization.rst @@ -0,0 +1,469 @@ +.. + Licensed under the Apache License, Version 2.0 (the "License"); you may + not use this file except in compliance with the License. You may obtain + a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + License for the specific language governing permissions and limitations + under the License. + + Convention for heading levels in Open vSwitch documentation: + + === Heading 0 (reserved for the title in a document) + --- Heading 1 + ~~~ Heading 2 + +++ Heading 3 + ''' Heading 4 + + Avoid deeper levels because they do not render well. + += +ovs-flowviz: Datapath and Openflow flow visualization += + +When troubleshooting networking issues with OVS, we typically end up looking +at OpenFlow or datapath flow dumps. These dumps tend to be quite dense and +difficult to reason about. + +``ovs-flowviz`` is a utility script that helps visualizing OpenFlow and +datapath flows to make it easier to understand what is going on. + + +Installing ovs-flowviz +-- + +``ovs-flowviz`` is part of the openvswitch python package. To install it, run: +:: + +$ pip install openvswitch[flowviz] + +Or, if you are working with the OVS tree: +:: + +$ cd python && pip install .[flowviz] + +Running the tool + +Here is the basic usage of the tool: +:: + +$ ovs-flowviz --help +Usage: ovs-flowviz [OPTIONS] COMMAND [ARGS]... + + OpenvSwitch flow visualization utility. + + It reads openflow and datapath flows (such as the output of ovs-ofctl dump- + flows or ovs-appctl dpctl/dump-flows) and prints them in different formats. + +Options: + -c, --config PATH Use config file [default: /home/amorenoz/src/ovs/pyth +on/venv/lib64/python3.12/site- +packages/ovs/flowviz/ovs-flowviz.conf] + --style TEXT Select style (defined in config file) + -i, --input PATH Read flows from specified filepath. If not provided, +flows will be read from stdin. This option can be +specified multiple times. Format [alias,]FILENAME. +Where alias is a name that shall be used to refer to +this FILENAME + -f, --filter TEXT Filter flows that match the filter expression.Run +'ovs-flowviz filter' for a detailed description of the +filtering syntax + -l, --highlight TEXT Highlight flows that match the filter expression. Run +'ofparse filter' for a detailed description of the +filtering syntax + -h, --helpShow this message and exit. + +Commands: + datapath Process Datapath Flows. + openflow Process OpenFlow Flows. + + +Available visualization formats
[ovs-dev] [PATCH v1 10/12] python: ovs: flowviz: Add datapath graph format.
Graph view leverages the tree format (specially the tree-based
filtering) and uses graphviz library to build a visual graph of the
datapath in graphviz format.
Conntrack zones are shown in random colors to help visualize connection
tracking interdependencies.
An html flag builds an HTML page with both the html flows and the graph
(in svg) that enables navegation.
Examples:
$ ovs-appctl dpctl/dump-flows -m | ovs-flowviz datapath graph | dot
-Tpng -o graph.png
$ ovs-appctl dpctl/dump-flows -m | ovs-flowviz datapath graph --html >
flows.html
Acked-by: Eelco Chaudron
Signed-off-by: Adrian Moreno
---
python/automake.mk | 1 +
python/ovs/flowviz/odp/cli.py | 22 ++
python/ovs/flowviz/odp/graph.py | 418
python/ovs/flowviz/odp/tree.py | 18 +-
python/setup.py | 2 +-
5 files changed, 457 insertions(+), 4 deletions(-)
create mode 100644 python/ovs/flowviz/odp/graph.py
diff --git a/python/automake.mk b/python/automake.mk
index 44e9e08ab..9ef000480 100644
--- a/python/automake.mk
+++ b/python/automake.mk
@@ -71,6 +71,7 @@ ovs_flowviz = \
python/ovs/flowviz/main.py \
python/ovs/flowviz/odp/__init__.py \
python/ovs/flowviz/odp/cli.py \
+ python/ovs/flowviz/odp/graph.py \
python/ovs/flowviz/odp/html.py \
python/ovs/flowviz/odp/tree.py \
python/ovs/flowviz/ofp/__init__.py \
diff --git a/python/ovs/flowviz/odp/cli.py b/python/ovs/flowviz/odp/cli.py
index 059dd708e..3e470ff1e 100644
--- a/python/ovs/flowviz/odp/cli.py
+++ b/python/ovs/flowviz/odp/cli.py
@@ -14,6 +14,7 @@
import click
from ovs.flowviz.main import maincli
+from ovs.flowviz.odp.graph import GraphProcessor
from ovs.flowviz.odp.html import HTMLTreeProcessor
from ovs.flowviz.odp.tree import ConsoleTreeProcessor
from ovs.flowviz.process import (
@@ -92,3 +93,24 @@ def html(opts):
processor = HTMLTreeProcessor(opts)
processor.process()
processor.print()
+
+
+@datapath.command()
+@click.option(
+"-h",
+"--html",
+is_flag=True,
+default=False,
+show_default=True,
+help="Output an html file containing the graph",
+)
+@click.pass_obj
+def graph(opts, html):
+"""Print the flows in an graphviz (.dot) format showing the relationship
+of recirc_ids."""
+if len(opts.get("filename")) > 1:
+raise click.BadParameter("Graph format only supports one input file")
+
+processor = GraphProcessor(opts)
+processor.process()
+processor.print(html)
diff --git a/python/ovs/flowviz/odp/graph.py b/python/ovs/flowviz/odp/graph.py
new file mode 100644
index 0..b26551e67
--- /dev/null
+++ b/python/ovs/flowviz/odp/graph.py
@@ -0,0 +1,418 @@
+# Copyright (c) 2023 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at:
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+""" Defines a Datapath Graph using graphviz. """
+import colorsys
+import graphviz
+import random
+
+from ovs.flowviz.odp.html import HTMLTree
+from ovs.flowviz.odp.tree import FlowTree
+from ovs.flowviz.process import DatapathFactory, FileProcessor
+
+
+class GraphProcessor(DatapathFactory, FileProcessor):
+def __init__(self, opts):
+super().__init__(opts)
+
+def start_file(self, name, filename):
+self.tree = FlowTree()
+
+def process_flow(self, flow, name):
+self.tree.add(flow)
+
+def process(self):
+super().process(False)
+
+def print(self, html):
+flows = {}
+
+# Tree traverse callback
+def add_flow(elem, _):
+if elem.is_root:
+return
+rid = elem.flow.match.get("recirc_id") or 0
+if not flows.get(rid):
+flows[rid] = set()
+flows[rid].add(elem.flow)
+
+self.tree.build()
+if self.opts.get("filter"):
+self.tree.filter(self.opts.get("filter"))
+self.tree.traverse(add_flow)
+
+if len(flows) == 0:
+return
+
+dpg = DatapathGraph(flows)
+if not html:
+print(dpg.source())
+return
+
+html_obj = ""
+html_obj += " Flow Graph "
+html_obj += ""
+svg = dpg.pipe(format="svg")
+html_obj += svg.decode("utf-8")
+html_obj += ""
+html_tree = HTMLTree("graph", self.opts, flows)
+html_tree.build()
+html_obj += html_tree.render()
+
+print(html_obj)
+
+
+class DatapathGraph:
+"""A DatapathGraph is a class that renders a set of datapath flows
[ovs-dev] [PATCH v1 11/12] python: ovs: flowviz: Support html dark style.
In order to support dark style in html outputs, allow the config file to
express the background color and set it in a top style object.
Signed-off-by: Adrian Moreno
---
python/ovs/flowviz/html_format.py | 4 +++-
python/ovs/flowviz/odp/html.py | 30 -
python/ovs/flowviz/ofp/html.py | 28 ++-
python/ovs/flowviz/ovs-flowviz.conf | 20 +++
4 files changed, 71 insertions(+), 11 deletions(-)
diff --git a/python/ovs/flowviz/html_format.py
b/python/ovs/flowviz/html_format.py
index ebfa65c34..3f3550da5 100644
--- a/python/ovs/flowviz/html_format.py
+++ b/python/ovs/flowviz/html_format.py
@@ -48,7 +48,9 @@ class HTMLBuffer(FlowBuffer):
style = ' style="color:{}"'.format(color) if color else ""
self._text += "".format(style)
if href:
-self._text += "".format(href)
+self._text += " ".format(
+href, 'style="color:{}"'.format(color) if color else ""
+)
self._text += string
if href:
self._text += ""
diff --git a/python/ovs/flowviz/odp/html.py b/python/ovs/flowviz/odp/html.py
index 4aa08dc70..b2855bf40 100644
--- a/python/ovs/flowviz/odp/html.py
+++ b/python/ovs/flowviz/odp/html.py
@@ -55,10 +55,18 @@ class HTMLTree(FlowTree):
flows(dict[int, list[DPFlow]): Optional; initial flows
"""
+html_body_style = """
+
+body {{
+background-color: {bg};
+color: {fg};
+}}
+"""
+
html_header = """
.flow{
-background-color:white;
+background-color:inherit;
display: inline-block;
text-align: left;
font-family: monospace;
@@ -177,9 +185,9 @@ class HTMLTree(FlowTree):
append()
"""
-def __init__(self, parent_name, flow=None, opts=None):
+def __init__(self, parent_name, flow=None, fmt=None, opts=None):
self._parent_name = parent_name
-self._formatter = HTMLFormatter(opts)
+self._formatter = fmt
self._opts = opts
super(HTMLTree.HTMLTreeElem, self).__init__(flow)
@@ -232,13 +240,14 @@ class HTMLTree(FlowTree):
def __init__(self, name, opts, flows=None):
self.opts = opts
self.name = name
+self._formatter = HTMLFormatter(opts)
super(HTMLTree, self).__init__(
flows, self.HTMLTreeElem("", flow=None, opts=self.opts)
)
def _new_elem(self, flow, _):
"""Override _new_elem to provide HTMLTreeElems."""
-return self.HTMLTreeElem(self.name, flow, self.opts)
+return self.HTMLTreeElem(self.name, flow, self._formatter, self.opts)
def render(self):
"""Render the Tree in HTML.
@@ -247,10 +256,21 @@ class HTMLTree(FlowTree):
an html string representing the element
"""
name = self.name.replace(" ", "_")
+bg = (
+self._formatter.style.get("background").color
+if self._formatter.style.get("background")
+else "white"
+)
+fg = (
+self._formatter.style.get("default").color
+if self._formatter.style.get("default")
+else "black"
+)
html_text = """
""" # noqa: E501
-html_obj = self.html_header + html_text.format(name=name)
+html_obj = self.html_body_style.format(bg=bg, fg=fg)
+html_obj += self.html_header + html_text.format(name=name)
html_obj += "".format(name=name)
(html_elem, _) = self.root.render()
diff --git a/python/ovs/flowviz/ofp/html.py b/python/ovs/flowviz/ofp/html.py
index a66f5fe8e..b00ca58f0 100644
--- a/python/ovs/flowviz/ofp/html.py
+++ b/python/ovs/flowviz/ofp/html.py
@@ -24,6 +24,7 @@ class HTMLProcessor(OpenFlowFactory, FileProcessor):
def __init__(self, opts):
super().__init__(opts)
+self.formatter = HTMLFormatter(self.opts)
self.data = dict()
def start_file(self, name, filename):
@@ -39,21 +40,38 @@ class HTMLProcessor(OpenFlowFactory, FileProcessor):
self.tables[table].append(flow)
def html(self):
-html_obj = ""
+bg = (
+self.formatter.style.get("background").color
+if self.formatter.style.get("background")
+else "white"
+)
+fg = (
+self.formatter.style.get("default").color
+if self.formatter.style.get("default")
+else "black"
+)
+html_obj = """
+