| Message ID | 20241021191609.1549014-1-i.maximets@ovn.org |
|---|---|
| State | New |
| Headers | show |
| Series | [ovs-dev] python: ovs: flowviz: Make datapath tree easier to read. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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 <i.maximets@ovn.org> > --- 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
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 <i.maximets@ovn.org> >> --- > > 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 >
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 <i.maximets@ovn.org> > >> --- > > > > 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 > > >
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)
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. * Put actions into the same panel as the flow matches. This saves another nesting level. * 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. * 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. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- 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(-)