diff mbox series

[ovs-dev] python: ovs: flowviz: Make datapath tree easier to read.

Message ID 20241021191609.1549014-1-i.maximets@ovn.org
State New
Delegated to: Ilya Maximets
Headers show
Series [ovs-dev] python: ovs: flowviz: Make datapath tree easier to read. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Ilya Maximets Oct. 21, 2024, 7:15 p.m. UTC
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(-)

Comments

Eelco Chaudron Oct. 23, 2024, 2:47 p.m. UTC | #1
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
Ilya Maximets Oct. 25, 2024, 10:11 a.m. UTC | #2
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
>
Adrián Moreno Oct. 25, 2024, 7:53 p.m. UTC | #3
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 mbox series

Patch

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)