Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLI show interfaces redesign #104

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Conversation

rical
Copy link
Contributor

@rical rical commented Aug 16, 2023

New operational data in sysrepo

Add IPv4 address info from "ip addr" json to sysrepo operational datastore. This patch adds: MTU and ip with prefix-length.

New CLI implementation

Replace the old "ip" output in show interfaces with data from the sysrepo operational datastore. This data is piped through the new json-cfg-pretty script, which formats the data in a nice human readable way.

Output Examples

Sysrepo operational data

New operational data in sysrepo

{
  "ietf-interfaces:interfaces": {
    "interface": [
      {
        "name": "e0",
        "type": "iana-if-type:ethernetCsmacd",
        "oper-status": "up",
        "if-index": 2,
        "phys-address": "02:00:00:00:00:00",
        "statistics": {
          "in-octets": "0",
          "out-octets": "16290"
        },
        "ietf-ip:ipv4": {
          "mtu": 1500,
          "address": [
            {
              "ip": "192.168.1.1",
              "prefix-length": 24
            },
            {
              "ip": "10.0.1.1",
              "prefix-length": 24
            }
          ]
        }
      }
    ]
  }
}

New output design in CLI

show interfaces

NAME            STATE           MAC                IPv4
lo              up              00:00:00:00:00:00  127.0.0.1/8
e0              up              02:00:00:00:00:00  192.168.1.1/24
                                                   10.0.1.1/24

show interfaces name e0

name                 : e0
interface-index      : 2
operational-status   : up
physical-address     : 02:00:00:00:00:00
mtu                  : 1500

ipv4:
      192.168.1.1/24
      10.0.1.1/24

in:   octets
      0

out:  octets
      19397

Copy link
Contributor

@troglobit troglobit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work on this, only one single comment that really doesn't block merging this.

@troglobit troglobit requested a review from wkz August 16, 2023 11:56
@wkz
Copy link
Contributor

wkz commented Aug 17, 2023

I think this is a great start! Some thoughts:

  • It would be great if show interfaces gave me an idea of the whole network setup, within reason.
  • Horizontal real estate is very precious. I think we should try to collapse multiple columns.
    • The state column can show the most relevant state (e.g. bridge state if the interface is connected to a bridge)
    • An address column could show all types of addresses, and possibly also some protocol info.

I made a mockup based on these ideas here:
show

@troglobit
Copy link
Contributor

I took the liberty of editing your comment, @wkz, uploading the image works fine on GitHub these days :-)

Other than that I agree it looks really good! And since we're longer restricted by test systems parsing the output of the console, we can now take liberties like Tobias propose, and I like that A LOT. It sets the goal, visualizing it perfectly. So question is what we should aim for, or limit ourselves to in this PR, or do we have everything in place already to extract it?

@wkz
Copy link
Contributor

wkz commented Aug 18, 2023

Two more ideas that came to mind:

  • When listing the interfaces, we could hash the output and check if it matches the previous interface, in that case we could group them. I think this would be great on bigger devices, where lots of ports will have the same config. It also makes the odd ducks stand out.
  • This layout allows each protocol to have a state, so we might as well show the state for all of them.

New mock-up:
show

Add IPv4 address info from "ip addr" json to sysrepo operational
datastore.

This patch adds: MTU and ip with prefix-length.

Signed-off-by: Richard Alpe <richard@bit42.se>
Replace the old "ip" output with data from the sysrepo operational
datastore. This data is piped through the new json-cfg-pretty script,
which formats the data in a nice human readable way.

Signed-off-by: Richard Alpe <richard@bit42.se>
And quote the "$name" variable.

Signed-off-by: Richard Alpe <richard@bit42.se>
Signed-off-by: Richard Alpe <richard@bit42.se>
Signed-off-by: Richard Alpe <richard@bit42.se>
Print both ip and mac in the PROTOCOL/ADDRESS column.

Signed-off-by: Richard Alpe <richard@bit42.se>
Add source, such as "ethernet" to the show ietf interfaces command.
This is translated from the somewhat obscure ietf type, such as
iana-if-type:ethernetCsmacd => ethernet

Signed-off-by: Richard Alpe <richard@bit42.se>
@rical
Copy link
Contributor Author

rical commented Aug 21, 2023

Great suggestions, thanks! I agree with the goal that the bare command show interfaces should give the clearest possible picture of the network.

I updated the PR a bit, here's an example of how it looks now in my terminal.
Screenshot from 2023-08-21 10-39-34

Regarding the rest of your input. Most if not all the data required to produce it is still missing from the operational datastore. So I suggest we take that in a later patchset. Possibly along with a rewrite of the pretty print code in a more suitable language, such as python3.

@wkz
Copy link
Contributor

wkz commented Aug 21, 2023

New draft after AFK discussions:

  • Protocol gives a hint about how the following data, so should come first
  • Try to clearly show which information applies to all interfaces in a grouped section

show

@rical
Copy link
Contributor Author

rical commented Aug 21, 2023

Screenshot from 2023-08-21 17-11-36
Here's a revised example of logically grouping interfaces under its bridge master.

@wkz
Copy link
Contributor

wkz commented Aug 21, 2023

Expanding on @rical's ideas:
show

@rical
Copy link
Contributor Author

rical commented Aug 22, 2023

Here's a updated example:
Screenshot from 2023-08-22 09-50-35

The idea here is to connect the text in STATE and DATA to the "PROTOCOL".
show-interfaces
So, for example (pseudo). If PROTOCOL is ipv4 you can expect an empty STATE column and an ipv4 address in the DATA column. If the PROTOCOL is bridge-port you can expect STATE to be FORWARDNING|BLOCKING|DOWN and DATA to contain vlan info and such.

@wkz
Copy link
Contributor

wkz commented Aug 22, 2023

Yeah I agree that it's better to have all interface names appear in the same colum. I wanted to find a way to avoid having the elongated tree lines from parent to children when there are many IP addresses, and also to avoid repeating "bridge-port" over and over again.

@wkz wkz merged commit c6513a3 into kernelkit:main Aug 23, 2023
1 check passed
@troglobit troglobit added this to the Infix v23.08 milestone Aug 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants