Skip to content

SWUTILS-949: Tabulate ethtool commands for easier comparison#31

Open
dchadwic-xilinx wants to merge 1 commit into
Xilinx-CNS:mainfrom
dchadwic-xilinx:SWUTILS-949
Open

SWUTILS-949: Tabulate ethtool commands for easier comparison#31
dchadwic-xilinx wants to merge 1 commit into
Xilinx-CNS:mainfrom
dchadwic-xilinx:SWUTILS-949

Conversation

@dchadwic-xilinx

Copy link
Copy Markdown
Collaborator

Added the ethtool output to a table rather than printing each command one by one. I have kept the 'print preformatted' in the table cell as many support members Ctrl+F for stats in ethtool outputs.
image

Comment thread sfreport.pl Outdated
@andresa-xilinx

Copy link
Copy Markdown
Contributor

How do we know whether this change in format will significantly impact customer's scripts or not? For example, Bloomberg would use w3m command to convert HTML into formatted text to extract useful state info from devices, etc.
This is how the output from legacy Ethernet settings section output after converting HTML to text using awk:

andresa@sup-cap2:~/git/sfreport$ cat test-without | grep "link modes"
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  Not reported
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  Not reported
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  25000baseCR/Full
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  25000baseCR/Full
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  25000baseCR/Full
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  25000baseCR/Full
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  Not reported
        Supported link modes:   1000baseT/Full
        Advertised link modes:  1000baseT/Full
        Link partner advertised link modes:  Not reported

And this is the output using the same grep command after applying the proposed patch:

andresa@sup-cap2:~/git/sfreport$ cat test | grep "link modes"
                   Supported link modes:   1000baseT/Full                    Supported link modes:   1000baseT/Full                    Supported link modes:   1000baseT/Full                        Supported link modes:   1000baseT/Full                        Supported link modes:   1000baseT/Full                        Supported link modes:   1000baseT/Full                        Supported link modes:   1000baseT/Full                    Supported link modes:   1000baseT/Full
                   Advertised link modes:  1000baseT/Full                    Advertised link modes:  1000baseT/Full                    Advertised link modes:  1000baseT/Full                        Advertised link modes:  1000baseT/Full                        Advertised link modes:  1000baseT/Full                        Advertised link modes:  1000baseT/Full                        Advertised link modes:  1000baseT/Full                    Advertised link modes:  1000baseT/Full
                   Link partner advertised link modes:  Not reported         Link partner advertised link modes:  Not reported         Link partner advertised link modes:  25000baseCR/Full         Link partner advertised link modes:  25000baseCR/Full         Link partner advertised link modes:  25000baseCR/Full         Link partner advertised link modes:  25000baseCR/Full         Link partner advertised link modes:  Not reported         Link partner advertised link modes:  Not reported

Although I agree that the proposed change is more aesthetic and perhaps functional from HTML PoV, I'd prefer the former, as it is simpler to query, to process in scripts, and to format in txt.

@dchadwic-xilinx

dchadwic-xilinx commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator Author

@andresa-xilinx

How do we know whether this change in format will significantly impact customer's scripts or not? For example, Bloomberg would use w3m command to convert HTML into formatted text to extract useful state info from devices, etc.

While customers are welcome to use the sfreport output in their scripts - they should understand that the primary purpose is as a diagnostics tool for the AMD Solarflare support team. So we would always prioritse changes that improve the support team's workflow even if that results in a change of format of the output.

That being said, I've double-checked - and this patch does mess up the plaintext output e.g. perl sfreport.pl -. So I will fix that.

@andresa-xilinx

Copy link
Copy Markdown
Contributor

While customers are welcome to use the sfreport output in their scripts - they should understand that the primary purpose is as a diagnostics tool for the AMD Solarflare support team. So we would always prioritse changes that improve the support team's workflow even if that results in a change of format of the output.

Processing ethtool output as it is now, and in the way indicated above, is much more convenient, as you can see. We regularly ask customers for sfreports, which need to be processed with simple commands to respond promptly to their questions (critically, in the middle of the night). Simple commands that produce clear, readable outputs can make a significant difference when triaging a case under pressure. Imagine a customer sending dozens of sfreports to identify a pattern through ethtool output. The suggested format will include a varying number of cards per sfreport, which will now need to be processed by more complex logic within tools like awk.

I wonder what the motivation is behind this change. In what way is this new format benefiting us?

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.

4 participants