Add ethtool monitoring support to Flent #302
Conversation
freysteinn
force-pushed
the
ingress_egress
branch
3 times, most recently
from
cf62c50
to
2aef3c8
Compare
This commit adds ethtool monitoring to Flent. The "common.inc" file includes this capability in most Flent tests. The ethtool support adds three new test parameters: "ethtool_hosts", "ethtool_devices", and "ethtool_fields". Behind the scenes, Flent runs the "ethtool -S <nic> command on all devices that support it via SSH. A breakdown of these new parameters is as follows: - ethtool_hosts: Specifies the hosts to monitor ethtool fields. The hosts are specified using a comma-separated list. - ethtool_devices: This parameter specifies which network devices to monitor instead of all of them. This parameter is also a comma-separated list. - ethtool_fields: This parameter defines a comma-separated list of fields to monitor. If this field is not set, it will default to monitor the "rx_packets" and "tx_packets" fields. The fields can also be prefixed with the network card if you only want to see that field for that particular device. Example: ethtool_devices=eth0,eth1 ethtool_fields=tx_packets_phy,rx_packets_phy,eth2:rx_bytes In this example, the fields "tx_packets_phy" and "rx_packets_phy" will be monitored on eth0 and eth1; however, only field rx_bytes will be monitored on eth2. One limitation of this version of the ethtool capability is that all hosts will share the same values of devices and fields. Signed-off-by: Frey Alfredsson <[email protected]>
freysteinn
force-pushed
the
ingress_egress
branch
from
2aef3c8
to
1ec9b9a
Compare
|
I added that it shows the value per second by default. However, there are cases where you would want to see incremental changes instead of a delta calculated into values per second. Originally, I had the option of setting a delta or incremental as a parameter using the values 'i' and 'd': "ethtool_fields=tx_bytes:i,eth0:rx_packets:d,eth1:tx_packets:d". |
There was a problem hiding this comment.
Thank you for the patch! I like the idea of supporting ethtool stats in Flent, but have some comments on the implementation, see below.
Also, please regenerate the man page after updating the documentation, and include the changes to the man page in the same patch.
| syntax or hosts defined in ``~/.ssh/config``. Flent will attempt to run | ||
| remote commands on these hosts to gather the required statistics. For this to | ||
| work, the hosts must have passwordless login enabled. You can specify | ||
| multiple hostnames by separating them by commas. |
There was a problem hiding this comment.
Let's not change the style of the text; keep the passive voice.
| ``wifistats_iterate`` programmes available in the ``misc/`` directory of the | ||
| source code tarball can be installed. On low-powered systems this can be | ||
| critical to get correct statistics. The helper programmes are packaged for | ||
| LEDE/OpenWrt in the ``flent-tools`` package. |
|
|
||
| While CPU stats, ethtool, and netstat output are global to the machine being | ||
| connected to, the qdisc and WiFi stats are more specific and require extra | ||
| parameters to work effectively. These parameters, namely |
| connected to, the qdisc and WiFi stats are more specific and require extra | ||
| parameters to work effectively. These parameters, namely | ||
| ``qdisc_stats_interfaces``, ``wifi_stats_interfaces``, and | ||
| ``wifi_stats_stations``, play a crucial role in specifying which interfaces |
There was a problem hiding this comment.
Drop the value-laden language "play a crucial role in"
| ``qdisc_stats_interfaces``, ``wifi_stats_interfaces``, and | ||
| ``wifi_stats_stations``, play a crucial role in specifying which interfaces | ||
| to gather statistics from and which MAC addresses of stations to gather | ||
| statistics for. Remember, these parameters are paired with the hostnames, so |
There was a problem hiding this comment.
Overly pedagogical "remember...". Just describe what the options do
| However, the ``ethtool_devices`` parameter lets you filter which devices to | ||
| monitor. If no fields are specified, Flent will monitor the ``rx_packets`` | ||
| and ``tx_packets`` fields unless you specify other fields in the | ||
| ``ethtool_fields`` parameter. You can create a comma-separated list of fields |
There was a problem hiding this comment.
Drop the last half of this sentence (from "unless you...")
| and ``tx_packets`` fields unless you specify other fields in the | ||
| ``ethtool_fields`` parameter. You can create a comma-separated list of fields | ||
| to monitor; however, if you prefix the field with a network device name | ||
| separated by a colon, Flent will only monitor that field for that particular |
There was a problem hiding this comment.
This bit seems over-engineered. Let's just keep this to capturing the same fields for all interfaces.
| -v INTERVAL="$interval" \ | ||
| -v DEVICES="$devices" \ | ||
| -v FIELDS="$fields" \ | ||
| -v EXACT_VALUES="$exact_values" ' \ |
There was a problem hiding this comment.
This script is doing way too much work on the captured host. The data gathering scripts are deliberately simple, so they can work on constrained devices, and so the data parting and smarts are implemented in Python in Flent. Also, looping through all the interfaces every iteration can take way too long on a constrained device, making the data points "late" relative to the polling interval. This is the reason the other runners spawn a separate runner instance per interface, for instance; we should do the same here.
So basically, I would change this script to be something like:
patterns=""
for f in fields; do
patterns="-e '$f' $patterns"
done
command_string=$(cat <<EOF
for i in \$(seq $count); do
date '+Time: %s.%N';
ethtool -S $interface | grep -w $patterns
echo "---";
sleep $interval || exit 1;
done
EOF
)and then do all the field parsing in the Flent code; and simply spawn a helper for each interface, like the qdisc and wifi stats do.
| raise RunnerCheckError("Ethtool stats requires a Bash shell.") | ||
|
|
||
| devices = f"-d {self.devices}" if self.devices else "" | ||
| fields = f"-f {self.fields}" if self.fields else "" |
There was a problem hiding this comment.
This should set the default field list (that should not be in the script)
| default, Flent will monitor all network devices from which it can get values. | ||
| However, the ``ethtool_devices`` parameter lets you filter which devices to | ||
| monitor. If no fields are specified, Flent will monitor the ``rx_packets`` | ||
| and ``tx_packets`` fields unless you specify other fields in the |
There was a problem hiding this comment.
I think we should default to capturing both packet and byte counts
This commit adds ethtool monitoring to Flent. The "common.inc" file includes this capability in most Flent tests. The ethtool support adds three new test parameters: "ethtool_hosts", "ethtool_devices", and "ethtool_fields". Behind the scenes, Flent runs the "ethtool -S command on all devices that support it via SSH.
A breakdown of these new parameters is as follows:
Example:
ethtool_devices=eth0,eth1
ethtool_fields=tx_packets_phy,rx_packets_phy,eth2:rx_bytes
In this example, the fields "tx_packets_phy" and "rx_packets_phy" will be monitored on eth0 and eth1; however, only field rx_bytes will be monitored on eth2.
One limitation of this version of the ethtool capability is that all
hosts will share the same values of devices and fields. We could handle this by adding a character different from a comma to separate the values. However, that would not fit how the rest of the global *_hosts parameters work, and it could be more challenging to deal with in the batch file.