tcp_cc_surveil (3)

[cherusk]

Repeat> Ok, works fairly enough. A minor one, did not find where the generic label is altered, so for every flow that one is depicted. ^^

[tohojo]

Okay, the review comments got a bit mangled (I really do not like the github interface for this :/). But the top comment was supposed to be for the "introduce globbing to raw_keys" commit, and the comments on specific code lines were meant to go together...

[cherusk]

On 05/10/2017 04:10 PM, Toke Høiland-Jørgensen wrote:

@tohojo commented on this pull request.

NAK on this whole commit. Introducing another level of globbing is not the right way to do this;

Well, it's quite flexible, especially for the surveillance case at hand:

Could filter on flow plots by extending the suffix, that was part of the rational.

E.g.: Instead of ... [...] 'series': [ { 'data': glob("sock_stats::**"), 'label': 'TCP window size',

     'raw_key' : glob("**::tcp_cwnd")

},

[...] 'series': [ { 'data': glob("sock_stats::**"), 'label': 'TCP window size',

     'raw_key' : glob("*#433*::tcp_cwnd")

},

Selecting all those dest-pt flows for plotting and so forth.

instead, create separate series that each have one set of raw_values.

Hmmm, but that is the end what the globbing mechanism is doing, as I understood it's working. What's cleaner about it by doing it seperately?

---

In flent/runners.py https://github.com/tohojo/flent/pull/110#discussion_r115749917:

@@ -1006,6 +1006,13 @@ class SsRunner(ProcessRunner): time_re = re.compile(r"^Time: (?P\d+.\d+)", re.MULTILINE) cwnd_re = re.compile(r"cwnd:(?P\d+)", re.MULTILINE) rtt_re = re.compile(r"rtt:(?P\d+.\d+/\d+.\d+)", re.MULTILINE) +

• bbr_re_str = "bbr:(bw:(?P\d+.\d+)(?P.bps),"\
• "mrtt:(?P\d+.\d+),"\
• "pacing_gain:(?P\d+.\d+),"\
• "cwnd_gain:(?P\d+))"

You are repeating these keys in three different places in the code. Use match.groupdict() instead to just pull them out of the regex match...

ACK, that's opening a maintenace hell. Will rectifiy!

-- Besten Gruß

Matthias Tafelmeier

[cherusk]

On 05/10/2017 04:27 PM, Toke Høiland-Jørgensen wrote:

@tohojo requested changes on this pull request.

Some more detailed comments (apart from the other one on raw_key globbing)...

---

In flent/scripts/ss_iterate.sh https://github.com/tohojo/flent/pull/110#discussion_r115751124:

usage() {

• echo "$0 -c count -I interval -H host -t target"
• echo "$0 -c count -I interval -H host -t target -f filter"

Hmm, instead of adding yet another arg here, maybe just get rid of -t and move the "dst $TARGET" into the Python code as part of the filter?

---

In flent/runners.py https://github.com/tohojo/flent/pull/110#discussion_r115752324:

• timestamp = self.time_re.search(part)
• def post_filter_pred(self, sub_parts):
• if self.ext_run:
• pass
• else:
• if 1 != len(sub_parts):
• raise ParseException()
• def form_sub_raw_vals(self, cwnd, rtt, rtt_var, bbr, flow_spec):
• bbr_keys = ['bbr_bw', 'bbr_mrtt', 'bbr_pacing_gain', 'bbr_cwnd_gain']
• if self.ext_run:
• sub_raw_val = {}
• for k, val in zip(['cwnd', 'rtt', 'rtt_var'], [cwnd, rtt, rtt_var]):

You can probably get rid of the repetition here in a similar way (named groups in regexes + groupdict())

Fully agree.

---

In flent/tests/tcp_cc_surveil.inc https://github.com/tohojo/flent/pull/110#discussion_r115752978:

+if PIDS:

• for pid in PIDS.split(","):
• DATA_SETS['sock_stats::%s' % pid] = {'command': find_ss_iterate(IP_VERSION, HOST, STEP_SIZE, TOTAL_LENGTH),
• 'units': 'misc',
• 'runner': 'ss',}

• +PLOTS['tcp_cwnd'] = {'description': 'TCP window statistics',

• 'type': 'timeseries',
• 'axis_labels': ['Packets'],
• 'series': [
• {
• 'data': glob("sock_stats::**"),
• 'label': 'TCP window size',
• 'raw_key' : glob("**::tcp_cwnd")
• },
• ]}

It would be good if we could re-use the plot definitions in tcp_stats.inc - may need a bit of restructuring, though...

See what you mean, but for me it's clearly a separate test, so I could live with growing something divorced.

-- Besten Gruß

Matthias Tafelmeier

[cherusk]

This has become stale. Is there any way forward or has it went unfathomable?

[tohojo]

You should be able to rebase. There's still an issue with how to store and display the data per pid. I think the solution is to create a multi-level dictionary (i.e. raw_values[pid]['tcp_rtt'] will have the RTT val). This is how the wifi stats data is stored. Still haven't gotten around to implementing support for plotting that in raw_values, but I'll have to do that anyway for the wifi stats...

[cherusk]

On 06/20/2017 12:01 PM, Toke Høiland-Jørgensen wrote:

You should be able to rebase.

Theoretically ... this has diverged too wild by now.

-- BR

Matthias Tafelmeier