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? > >> * 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? > >> * 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. > >> * 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. > >> 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. 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 >> 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 >> ) >> >> - 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
