On Fri, Oct 25, 2024 at 12:11:06PM +0200, Ilya Maximets wrote:
> On 10/23/24 16:47, Eelco Chaudron wrote:
> >
> > On 21 Oct 2024, at 21:15, Ilya Maximets wrote:
> >
> >> There are few changes here:
> >>
> >> * Make a separate recirculation node to not appear in case there is
> >> only one flow with this id. This saves a nesting level.
> >
> > When reviewing a large trace, I prefer consistency. If one entry has
> > multiple
> > sub-entries, all of them should have the extra nesting to improve ease of
> > reading.
>
> I do like consistency, but 8 nesting levels in 4-time recirculation
> do not allow me to understand what I'm looking at efficiently.
> Maybe we can add an option? e.g. "datapath tree --compact" to hide
> single-node recirculations. WDYT?
I guess this is very subjective. My eyes are OK with the lack of
indentation consistency. However, sometimes they miss the recirculation
block title that is present in others, i.e: [recirc_id(0x12) in_port(eth0)]
Maybe we could add the title within the same Panel(Group())?
Regarding, "--compact" I think this command has already quite a lot of
options and would prefer not to add more but if we cannot agree on a
style it's probably a good solution.
>
> >
> >> * Put actions into the same panel as the flow matches. This saves
> >> another nesting level.
> >
> > I liked the extra separation with the boxes. However, if we decide to
> > remove the
> > boxes, using the theme color alone would work. If we stick with the boxes,
> > we could
> > simply remove the indent.
>
> I don't think rich allows to not have an indent. Since we do have
> different preferences, maybe we could use an option here too, e.g.
> make the --compact affect both the recirculation and action node?
>
Separating the actions with blocks that have many flows. Specially if
dumped with "-m" where that can make individual flows be wrapped.
Maybe adding a newline between flows and actions would help the eye
while avoiding an extra indention level?
If we cannot agree on a style that is good enough for everyone we
can add a compact option but let's at least try to find some middle
grounds.
A combination of my two proposals is this snipped:
diff --git a/python/ovs/flowviz/odp/tree.py b/python/ovs/flowviz/odp/tree.py
index cd781e330..911387cf9 100644
--- a/python/ovs/flowviz/odp/tree.py
+++ b/python/ovs/flowviz/odp/tree.py
@@ -473,18 +473,19 @@ class ConsoleTreeProcessor(FileProcessor):
visible_blocks = list(node.visible_blocks())
- # If there is only one node with this recirc_id, don't create
- # an extra nesting level, just print the node.
- if len(visible_blocks) == 1:
- self.print_block(visible_blocks[0], parent, recirc_style)
- return
-
node_text = Text(
"[recirc_id({}) in_port({})]".format(
hex(node.recirc), node.in_port
),
style=recirc_style,
)
+ # If there is only one node with this recirc_id, don't create
+ # an extra nesting level, just print the node.
+ if len(visible_blocks) == 1:
+ self.print_block(visible_blocks[0], parent, recirc_style,
+ title=node_text)
+ return
+
console_node = parent.add(
Panel(node_text, box=TreeBox, border_style=recirc_style),
guide_style=recirc_style
@@ -493,9 +494,13 @@ class ConsoleTreeProcessor(FileProcessor):
for block in visible_blocks:
self.print_block(block, console_node, recirc_style)
- def print_block(self, block, parent, style):
+ def print_block(self, block, parent, style, title=None):
# Print the flow matches and the statistics.
flow_text = []
+ if title:
+ flow_text.append(title)
+ flow_text.append("\n")
+
omit_first = {
"actions": "all",
}
@@ -520,6 +525,7 @@ class ConsoleTreeProcessor(FileProcessor):
act_buf.append_extra("actions: ", Style(bold=(self.style is not None)))
self.ofconsole.format_flow(act_buf, block.flows[0].flow, omitted=omit)
+ flow_text.append("\n")
flow_text.append(act_buf.text)
flows_node = parent.add(
> >
> >> * Replace default panel box with a custom one, which is essentially
> >> only the side border with some angles. This makes the output much
> >> less busy and much easier on eyes, IMO.
> >>
> >> The bottom hook looks a little awkward if there is a guide line
> >> right below it, but it doesn't seem too distracting, and I didn't
> >> find an ASCII top-half of a vertical line symbol. This one also
> >> looks nice closing the blocks.
> >>
> >> Discontinuous lines of '|' symbols are also a smaller distraction
> >> than a continuous line of '│' symbols.
> >
> > I don't have a strong preference either way. Both options get the job done.
> > For the new mode, I'd even suggest removing the ending |, since there's no
> > real box anymore.
>
> Yeah, I thought about that too, but then I added the ending side back,
> just because I don't like trailing spaces and rich doesn't have ability
> to not add anything. It will add a symbol, we only can choose if it is
> a space or a vertical line.
I don't have a strong opinion on the last character, and my eyes also
appreciate this lighter box.
>
> >
> >> * Make recirculation color propagate down, so panels are colored in
> >> the color of their recirculation. This also fixes the guide line
> >> color to a recirc node to not be a default white and so to not be
> >> a visual distraction.
> >
> > I do like this.
> >
+1 this is great.
> >> Signed-off-by: Ilya Maximets <[email protected]>
> >> ---
> >
> > I didn’t find any coding or documentation issues, aside from the comments
> > mentioned above. I assume Adrian has some thoughts on this as well, so let’s
> > wait for his feedback.
>
> Sure, let's see if Adrian has some opinion on these changes.
Some nits below. A general question though: the HTML format is still using
boxes and a different indentation level for actions. Should we also
change it keep it consistent?
Thanks for improving this view.
Adrián.
>
> Thanks for taking a look!
>
> Best regards, Ilya Maximets.
>
> >
> > Cheers,
> >
> > Eelco
> >
> >> There are a few checkpatch warnings, but the long lines in the doc and
> >> the trailing spaces in the box definition are on purpose.
> >>
> >> Documentation/topics/flow-visualization.rst | 74 +++++++++------------
> >> python/ovs/flowviz/odp/tree.py | 47 +++++++++----
> >> 2 files changed, 65 insertions(+), 56 deletions(-)
> >>
> >> diff --git a/Documentation/topics/flow-visualization.rst
> >> b/Documentation/topics/flow-visualization.rst
> >> index 3165f796f..518487b45 100644
> >> --- a/Documentation/topics/flow-visualization.rst
> >> +++ b/Documentation/topics/flow-visualization.rst
> >> @@ -201,49 +201,37 @@ For example:
> >> ::
> >>
> >> Datapath Flows (logical)
> >> - └── ╭────────────────────────────────╮
> >> - │ [recirc_id(0x0) in_port(eth0)] │
> >> - ╰────────────────────────────────╯
> >> - └──
> >> ╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
> >> - │
> >> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=0a:58:0a:84:00:07,dst=22:a1:5d:dc:95:50),eth_type(0x0800),ipv4(src=10.132.0.7,dst=1
> >> │
> >> - │
> >> 0.128.0.0/255.128.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
> >> packets:4924, bytes:468961,
> >> │
> >> - │
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:01),eth_type(......),ipv4(src=10.132.0.7,dst=1
> >> │
> >> - │
> >> 0.0.0.0/255.255.128.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=32768/0x8000,dst=0/0),
> >> packets:711, bytes:114236,
> >> │
> >> - │
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:14),eth_type(......),ipv4(src=10.132.0.7,dst=1
> >> │
> >> - │
> >> 0.128.0.0/255.128.0.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=4096/0xf000,dst=0/0),
> >> packets:140, bytes:114660,
> >> │
> >> - │
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:22),eth_type(......),ipv4(src=10.132.0.7,dst=1
> >> │
> >> - │
> >> 0.128.0.0/255.128.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
> >> packets:1, bytes:66,
> >> │
> >> - │
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:09),eth_type(......),ipv4(src=10.132.0.7,dst=1
> >> │
> >> - │
> >> 0.128.0.0/255.128.0.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=4096/0xf000,dst=0/0),
> >> packets:0, bytes:0,
> >> │
> >> -
> >> ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
> >> - └── ╭───────────────────────────────────────╮
> >> - │ actions: ct(zone=32,nat),recirc(0xc1) │
> >> - ╰───────────────────────────────────────╯
> >> - └── ╭─────────────────────────────────╮
> >> - │ [recirc_id(0xc1) in_port(eth0)] │
> >> - ╰─────────────────────────────────╯
> >> - ├──
> >> ╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
> >> - │ │
> >> recirc_id(0xc1),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0x2a/0x3f),ct_zone(0/0),ct_mark(0/0xf),ct_label(0/0),eth(src=0a:58:0a:84:00:07,dst=22:a1:5d:dc:95:50),eth_type(0x0800),ip
> >> │
> >> - │ │
> >> v4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
> >> packets:4924, bytes:468961,
> >> │
> >> - │
> >> ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
> >> - │ └── ╭───────────────────────────────────────╮
> >> - │ │ actions: ct(zone=14,nat),recirc(0xc2) │
> >> - │ ╰───────────────────────────────────────╯
> >> - │ └── ╭─────────────────────────────────╮
> >> - │ │ [recirc_id(0xc2) in_port(eth0)] │
> >> - │ ╰─────────────────────────────────╯
> >> - │ └──
> >> ╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
> >> - │ │
> >> recirc_id(0xc2),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0x2a/0x3f),ct_zone(0/0),ct_mark(0/0x1),ct_label(0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00
> >> │
> >> - │ │
> >> :00:00:00/01:00:00:00:00:00),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=0/0,tos=0/0,ttl=0/0,frag=no),
> >> packets:4924, bytes:468961, │
> >> - │
> >> ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
> >> - │ └── ╭──────────────────────╮
> >> - │ │ actions: ovn-k8s-mp0 │
> >> - │ ╰──────────────────────╯
> >> - ├──
> >> ╭───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
> >> - │ │
> >> recirc_id(0xc1),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0x2a/0x3f),ct_zone(0/0),ct_mark(0/0xf),ct_label(0/0),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:14),eth_type(0x0800),ip
> >> │
> >> - │ │
> >> v4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=4096/0xf000,dst=0/0),
> >> packets:140, bytes:114660
> >> │
> >> - │
> >> ╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯
> >> -
> >> + └── ╮
> >> + |
> >> recirc_id(0),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0/0),ct_zone(0/0),ct_mark(0/0),ct_label(0/0),eth(src=0a:58:0a:84:00:07,dst=22:a1:5d:dc:95:50),eth_type(0x0800),ipv4(src=10.132.0.7,dst=10.12
> >> |
> >> + |
> >> 8.0.0/255.128.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
> >> packets:4924, bytes:468961,
> >> |
> >> + |
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:01),eth_type(......),ipv4(src=10.132.0.7,dst=10.0.
> >> |
> >> + |
> >> 0.0/255.255.128.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=32768/0x8000,dst=0/0),
> >> packets:711, bytes:114236,
> >> |
> >> + |
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:14),eth_type(......),ipv4(src=10.132.0.7,dst=10.12
> >> |
> >> + |
> >> 8.0.0/255.128.0.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=4096/0xf000,dst=0/0),
> >> packets:140, bytes:114660,
> >> |
> >> + |
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:22),eth_type(......),ipv4(src=10.132.0.7,dst=10.12
> >> |
> >> + |
> >> 8.0.0/255.128.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
> >> packets:1, bytes:66,
> >> |
> >> + |
> >> recirc_id(0),dp_hash(...),skb_priority(...),in_port(eth0),skb_mark(...),ct_state(...),ct_zone(...),ct_mark(...),ct_label(...),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:09),eth_type(......),ipv4(src=10.132.0.7,dst=10.12
> >> |
> >> + |
> >> 8.0.0/255.128.0.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=4096/0xf000,dst=0/0),
> >> packets:0, bytes:0,
> >> |
> >> + | actions: ct(zone=32,nat),recirc(0xc1)
> >>
> >>
> >> |
> >> + └
> >> + └── ╮
> >> + | [recirc_id(0xc1) in_port(eth0)]
> >>
> >>
> >> |
> >> + └
> >> + ├── ╮
> >> + │ |
> >> recirc_id(0xc1),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0x2a/0x3f),ct_zone(0/0),ct_mark(0/0xf),ct_label(0/0),eth(src=0a:58:0a:84:00:07,dst=22:a1:5d:dc:95:50),eth_type(0x0800),ipv4(src=0
> >> |
> >> + │ |
> >> .0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=6,tos=0/0,ttl=0/0,frag=no),tcp(src=0/0,dst=0/0),tcp_flags(0/0),
> >> packets:4924, bytes:468961,
> >> |
> >> + │ | actions: ct(zone=14,nat),recirc(0xc2)
> >>
> >>
> >> |
> >> + │ └
> >> + │ └──
> >> + │ |
> >> recirc_id(0xc2),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0x2a/0x3f),ct_zone(0/0),ct_mark(0/0x1),ct_label(0/0),eth(src=00:00:00:00:00:00/00:00:00:00:00:00,dst=00:00:00:00:00:00/01:00:
> >> |
> >> + │ |
> >> 00:00:00:00),eth_type(0x0800),ipv4(src=0.0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=0/0,tos=0/0,ttl=0/0,frag=no),
> >> packets:4924, bytes:468961,
> >> |
> >> + │ | actions: ovn-k8s-mp0
> >>
> >>
> >> |
> >> + │ └
> >> + ├── ╮
> >> + │ |
> >> recirc_id(0xc1),dp_hash(0/0),skb_priority(0/0),in_port(eth0),skb_mark(0/0),ct_state(0x2a/0x3f),ct_zone(0/0),ct_mark(0/0xf),ct_label(0/0),eth(src=0a:58:0a:84:00:07,dst=0a:58:0a:84:00:14),eth_type(0x0800),ipv4(src=0
> >> |
> >> + │ |
> >> .0.0.0/0.0.0.0,dst=0.0.0.0/0.0.0.0,proto=17,tos=0/0,ttl=0/0,frag=no),udp(src=4096/0xf000,dst=0/0),
> >> packets:140, bytes:114660
> >> |
> >> + │ | actions: ct(zone=14,nat),drop
> >>
> >>
> >> |
> >> + │ └
> >>
> >> The above shows a part of a bigger tree with an initial block of flows
> >> at ``recirc_id(0)`` which match on different destination Ethernet
> >> diff --git a/python/ovs/flowviz/odp/tree.py
> >> b/python/ovs/flowviz/odp/tree.py
> >> index d6526504b..cd781e330 100644
> >> --- a/python/ovs/flowviz/odp/tree.py
> >> +++ b/python/ovs/flowviz/odp/tree.py
> >> @@ -14,6 +14,7 @@
> >>
> >> import sys
> >>
> >> +from rich import box
I don't think there is global consensus around this but following the
style of the file, I think it should be either:
import rich.box # module import
or
from rich.box import Box # class import
Given only one class is being used I tend to prefer the later.
> >> from rich.style import Style
> >> from rich.console import Group
> >> from rich.panel import Panel
> >> @@ -33,6 +34,21 @@ from ovs.flowviz.process import (
> >> )
> >>
> >>
> >> +TreeBox = box.Box(
> >> + """\
> >> +╮
> >> +| |
> >> +| |
> >> +| |
> >> +| |
> >> +| |
> >> +| |
> >> +└
> >> +""", # noqa: W291
> >> + ascii=True,
> >> +)
> >> +
> >> +
> >> class TreeFlow(object):
> >> """A flow within a Tree."""
> >>
> >> @@ -453,7 +469,15 @@ class ConsoleTreeProcessor(FileProcessor):
> >> if self.ofconsole.style:
> >> recirc_style = self.recirc_style_gen(hex(node.recirc))
> >> else:
> >> - recirc_style = None
> >> + recirc_style = Style(color="default")
> >> +
> >> + visible_blocks = list(node.visible_blocks())
> >> +
> >> + # If there is only one node with this recirc_id, don't create
> >> + # an extra nesting level, just print the node.
> >> + if len(visible_blocks) == 1:
> >> + self.print_block(visible_blocks[0], parent, recirc_style)
> >> + return
> >>
> >> node_text = Text(
> >> "[recirc_id({}) in_port({})]".format(
> >> @@ -462,13 +486,14 @@ class ConsoleTreeProcessor(FileProcessor):
> >> style=recirc_style,
> >> )
> >> console_node = parent.add(
> >> - Panel.fit(node_text), guide_style=recirc_style
> >> + Panel(node_text, box=TreeBox, border_style=recirc_style),
> >> + guide_style=recirc_style
super-nit: my nit-picky eyes, probably shaped by "black", ask for a
comma at the end of this line. Same for multi-line argument list below.
> >> )
> >>
> >> - for block in node.visible_blocks():
> >> - self.print_block(block, console_node)
> >> + for block in visible_blocks:
> >> + self.print_block(block, console_node, recirc_style)
> >>
> >> - def print_block(self, block, parent):
> >> + def print_block(self, block, parent, style):
> >> # Print the flow matches and the statistics.
> >> flow_text = []
> >> omit_first = {
> >> @@ -495,18 +520,14 @@ class ConsoleTreeProcessor(FileProcessor):
> >> act_buf.append_extra("actions: ", Style(bold=(self.style is not
> >> None)))
> >>
> >> self.ofconsole.format_flow(act_buf, block.flows[0].flow,
> >> omitted=omit)
> >> + flow_text.append(act_buf.text)
> >>
> >> flows_node = parent.add(
> >> - Panel(Group(*flow_text)), guide_style=Style(color="default")
> >> - )
> >> - action_node = flows_node.add(
> >> - Panel.fit(
> >> - act_buf.text, border_style="green" if self.style else
> >> "default"
> >> - ),
> >> - guide_style=Style(color="default"),
> >> + Panel(Group(*flow_text), box=TreeBox, border_style=style),
> >> + guide_style=style
> >> )
> >>
> >> # Nested to the action, print the next recirc nodes.
> >> for node in block.next_recirc_nodes:
> >> if node.visible:
> >> - self.print_recirc_node(action_node, node)
> >> + self.print_recirc_node(flows_node, node)
> >> --
> >> 2.46.0
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
