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