-
Notifications
You must be signed in to change notification settings - Fork 325
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
gluon-statistics-mcast: adding initial package #2367
base: main
Are you sure you want to change the base?
Conversation
5fba865
to
3e07c5f
Compare
Changelog v2:
Note: The linter still complains about the following:
Which is bogus, that's exactly what this line there is supposed to check: https://github.com/freifunk-gluon/gluon/pull/2367/files#diff-808af07a6378d71a5b65b5614fe138b14d17d21f8d4c068ecc31e97813b97592R3. And the linter seems to get confused about these two distinct variables, the global "IFNAME" vs. the function local "ifname". |
@T-X You can suppress the warning using |
3e07c5f
to
8e22b67
Compare
Changelog v3:
|
8e22b67
to
06d95f2
Compare
Changelog v4:
Tested in BABEL VMs that babel/l3roamd/mmfd multicast packets are tracked fine (note that for instance mmfd seems to forward via individual unicast transmissions? - so for overall protocol overhead evaluation the multicast packet counts might not be complete for BABEL networks). |
The bpfcountd package could be added to https://github.com/freifunk-gluon/packages instead. It might also make sense to submit it for https://github.com/openwrt/packages, it looks quite useful. Please also add a link to the tool's README to the Makefile's description field. |
set bpfcountd.${SECTION_OUT}.filterfile='/lib/gluon/bpfcountd/mesh.filters' | ||
set bpfcountd.${SECTION_OUT}.disabled='0' | ||
|
||
commit bpfcountd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Writing files to flash from setup.d
scripts should be avoided. Better options would be to either run bpfcountd directly without procd, or to create a separate initscript that uses configuration stored in /var
(with or without UCI).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is actually stored in /var, more precisely in /var/run/bpfcountd/gluon-config :-). See UCI_CONFIG_DIR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only a very superficial review - I'd like to have a closer look soon.
[CAP_OUT] = "out", | ||
}; | ||
|
||
static int check_socket_suffix(char *socketname) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should use const char *
. There are lots more arguments that should be const
in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Urgh, I just realized how evil strchr() is. Why the hell can it return a "char *" from a "const char *"??
For socketname it seems okay to change it to const (as long as I'm not missing something). But with parse_line() I probably shouldn't change "char *line" to "const char *line", as there is a *start = '\0';
in there, which modifies *line
.
Is respondd really the right tool to provide this data? UDP is lossy, even more so when many nodes send long, fragmented packets more or less at the same time... How long does the data generation take? I'm wondering if the data might be provided through uhttpd instead... |
Yes, had been wondering about this, too, relying on fragments is not really nice... At least we have support for delayed responses since freifunk-gluon/packages@e7fbcf7. For an HTTP / uhttpd provider I was wondering whether we should put more thought on a unified format before adding that. As it might be interesting to add other detailed statistics later for all kinds of data. Or maybe we might want to move the existing statistics provided in respondd to HTTP at some point, too? An alternative I was thinking about: Adding TCP support to respondd. For all the respondd providers we have so far a respondd querier should of course keep using UDP. But then for more verbose providers, like statistics-extended, a respondd querier could then just cherry-pick it via a unicast+TCP query from the few, specific nodes which offer this provider. We could add a list of available respondd providers to the "nodeinfo" provider, so that a respondd statistics-extended querier knows in advance which few nodes it can query for it via unicast/TCP. This should basically be a similar packet overhead as HTTP in the end? |
Or another, easy way would be to add a small, generic wrapper to http://.../cgi-bin/respondd. Which could receive a respondd query encoded as a HTTP POST. And cgi-bin-respondd could proxy it as UDP to "[::1]:1001", which could take advantage of the higher, 65536 bytes MTU on the "lo" interface. And the UDP reply from [::1]:1001 would then be sent back via HTTP in one or more TCP segments. |
06d95f2
to
e06c926
Compare
Changelog v5:
Still ToDo:
With xz instead of deflate the following results would in theory be possible:
(results will vary with different multicast traffic patterns in the network) |
Changelog v6:
This allows to query bulky respondd providers like the "statistics-extended" introduced in this PR over HTTP instead of UDP, to avoid retrieving the UDP data in multiple IP fragments. Together with freifunk-gluon/packages#256 this would allow a respondd querier to retrieve the multicast statistics as follows:
|
d01840d
to
0559868
Compare
Changelog v7:
|
0559868
to
0b61595
Compare
Changelog v8:
|
0b61595
to
0f3ee91
Compare
Changelog v9:
|
@@ -0,0 +1,34 @@ | |||
include $(TOPDIR)/rules.mk |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move this package to OpenWrt packages (I See that LICENCE identifies is missing).
bpfcountd was created to obtain packet statistics in larger networks without stressing the cpu resources. bpfcountd will count the amount of packages and bytes over time (for each defined rule). The rules are defined using the tcpdump filter syntax (bpf). The collected data is provided on a unix socket in plaintext. Signed-off-by: Linus Lüssing <[email protected]>
0f3ee91
to
003b8d9
Compare
Changelog v10:
|
003b8d9
to
04b13c6
Compare
Changelog v11:
|
This ignores the following warning: warning: In POSIX sh, string replacement is undefined. [SC3060] As string replacements are supported by ash and we are only going to use ash. This allows us to use something short like ${QUERY_STRING//+/ } to replace all '+' to a whitespace in an URL or to make an ${IFNAME//-/_} UCI compatible in the future, for example. Instead of needing to use something long like "echo ${IFNAME} | tr '-' '_'. Signed-off-by: Linus Lüssing <[email protected]>
This package collects disected statistics on multicast and management traffic and provides them via respondd in "statistics-extended". Such information can be helpful for further development and comparison of mesh routing protocols and troubleshooting network issues. The statistics gathered disect into batman-adv packets as well. Statistics are collected both on client bridge ports and br-client as well as mesh interfaces. This introduces a performance penalty but tries to keep the impact on unicast traffic small. Note that respondd replies are rather large (several UDP datagrams, even when LZMA compressed). Therefore you might want to use slower intervals for querying respondd "statistics-extended". Signed-off-by: Linus Lüssing <[email protected]>
Adding a cgi-bin script which allows to query a node's respondd over HTTP. This is especially useful when the response is expected to be large and a UDP response would lead to an unreliable transfer of multiple IP fragments, like for the upcoming "statistics-extended" respondd provider. HTTP/TCP is then the more reliable approach. Query examples with HTTP GET: $ curl --compressed "http://<IP6ADDR>/cgi-bin/respondd" -> defaults to nodeinfo $ curl --compressed "http://<IP6ADDR>/cgi-bin/respondd?nodeinfo+statistics" Query examples with HTTP POST: $ curl --data "" --compressed "http://<IP6ADDR>/cgi-bin/respondd" -> defaults to nodeinfo $ curl --data "nodeinfo statistics" --compressed "http://<IP6ADDR>/cgi-bin/respondd" Signed-off-by: Linus Lüssing <[email protected]>
04b13c6
to
c6693b7
Compare
Changelog v12:
|
This comment was marked as duplicate.
This comment was marked as duplicate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a complete review, but only the results of me checking for affected PRs in #2765.
"client"|\ | ||
"mmfd") | ||
;; | ||
*) | ||
exit 0 | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"client"|\ | |
"mmfd") | |
;; | |
*) | |
exit 0 | |
;; | |
"client"|\ | |
"mmfd") | |
;; | |
*) | |
exit 0 | |
;; |
"ifup") | ||
bpfcountd_start | ||
;; | ||
"ifdown") | ||
bpfcountd_stop | ||
;; | ||
*) | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"ifup") | |
bpfcountd_start | |
;; | |
"ifdown") | |
bpfcountd_stop | |
;; | |
*) | |
;; | |
"ifup") | |
bpfcountd_start | |
;; | |
"ifdown") | |
bpfcountd_stop | |
;; | |
*) | |
;; |
GET) | ||
;; | ||
POST) | ||
QUERY_STRING="$(cat)" | ||
;; | ||
*) | ||
badrequest | ||
;; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GET) | |
;; | |
POST) | |
QUERY_STRING="$(cat)" | |
;; | |
*) | |
badrequest | |
;; | |
GET) | |
;; | |
POST) | |
QUERY_STRING="$(cat)" | |
;; | |
*) | |
badrequest | |
;; |
/* | ||
Copyright (c) 2021, Linus Lüssing <[email protected]> | ||
All rights reserved. | ||
|
||
Redistribution and use in source and binary forms, with or without | ||
modification, are permitted provided that the following conditions are met: | ||
|
||
1. Redistributions of source code must retain the above copyright notice, | ||
this list of conditions and the following disclaimer. | ||
2. Redistributions in binary form must reproduce the above copyright notice, | ||
this list of conditions and the following disclaimer in the documentation | ||
and/or other materials provided with the distribution. | ||
|
||
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" | ||
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | ||
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE | ||
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE | ||
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | ||
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR | ||
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER | ||
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, | ||
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | ||
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/* | |
Copyright (c) 2021, Linus Lüssing <linus.luessing@c0d3.blue> | |
All rights reserved. | |
Redistribution and use in source and binary forms, with or without | |
modification, are permitted provided that the following conditions are met: | |
1. Redistributions of source code must retain the above copyright notice, | |
this list of conditions and the following disclaimer. | |
2. Redistributions in binary form must reproduce the above copyright notice, | |
this list of conditions and the following disclaimer in the documentation | |
and/or other materials provided with the distribution. | |
THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" | |
AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE | |
IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE | |
DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE | |
FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL | |
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR | |
SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER | |
CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, | |
OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE | |
OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | |
*/ | |
/* SPDX-FileCopyrightText: 2021, Linus Lüssing <[email protected]> */ | |
/* SPDX-License-Identifier: BSD-2-Clause */ |
} | ||
|
||
static int parse_socket_iface(const char *socketname, enum capdir dir, | ||
char *iface) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
char *iface) | |
char *iface) |
} | ||
|
||
static int add_num_to_json(struct json_object *dir_obj, const char *key, | ||
int64_t num) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int64_t num) | |
int64_t num) |
} | ||
|
||
static int add_line_to_json(struct json_object *iface_obj, enum capdir dir, | ||
const char *rule, int64_t bytes, int64_t packets) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const char *rule, int64_t bytes, int64_t packets) | |
const char *rule, int64_t bytes, int64_t packets) |
* And adds it to the provided json object. | ||
*/ | ||
static int parse_line(struct json_object *obj, enum capdir dir, | ||
char *line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
char *line) | |
char *line) |
strncpy(addr.sun_path, socketname, sizeof(addr.sun_path) - 1); | ||
|
||
ret = connect(sd, (const struct sockaddr *)&addr, | ||
sizeof(addr)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sizeof(addr)); | |
sizeof(addr)); |
|
||
/* skip files not ending in ".sock" */ | ||
if (check_socket_suffix(dp->d_name) < 0) | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
continue; | |
continue; |
This package collects disected statistics on multicast
and management traffic and provides them via respondd in
"statistics-extended". Such information can be helpful
for further development and comparison of mesh routing
protocols and troubleshooting network issues.
The statistics gathered disect into batman-adv
packets as well. Statistics are collected both on client
bridge ports and br-client as well as mesh interfaces.
This introduces a performance penalty but tries to
keep the impact on unicast traffic small.
Note that respondd replies are rather large
(several UDP datagrams, even when LZMA compressed).
Therefore you might want to use slower intervals for
querying respondd "statistics-extended".
Signed-off-by: Linus Lüssing <[email protected]>
This PR requires / depends on the libpcap batman-adv support patch provided in a separate PR here: #2366