-
Notifications
You must be signed in to change notification settings - Fork 326
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
Add package gluon-radv-filterd #838
Conversation
2aca457
to
be91c62
Compare
c6dcbe2
to
7053f2d
Compare
To clarify here: Is it picking the "default router" (as in, the "selected gateway" which B.A.T.M.A.N. advanced will also do its DHCP magic with) or the one with the best metric? The two are not necessarily the same; if the best metric is only slightly better than the selected GW then the selected GW does not change to avoid flapping between two GWs all the time. |
I should probably improve the readme on that. Yes, after @NeoRaider mentioned it on IRC I implemented a hysteresis, currently switching the selected gateway if ΔTQ≥20. This is comparable to the B.A.T.M.A.N. advanced algorithm with a selection class of 20, which is the default in Gluon AFAIK. But it doesn't use the B.A.T.M.A.N. advanced gateway selection, because that would require all gateways to handle both IPv4 and IPv6. |
I see -- so this way, IPv4 gateways announce themselves as B.A.T.M.A.N. advanced gateways, while IPv6 gateways just send RAs to announce themselves? That makes sense, I guess, if properly documented. :) Would it be possible to use the B.A.T.M.A.N. advanced selection class? Alternatively, at least, the 20 should be configurable. |
Currently the ΔTQ threshold is a compile time constant. Would you rather have it as a site.conf variable? Also, the other selection classes never struck me as very useful, as they stay with their initial choice of gateway forever/until that gateway vanishes. Thus, I'd rather not implement additional code to make them usable. |
Oh, I wasn't even aware there are entirely different "classes" (as in, algorithms) -- but selecting a different threshold certainly sounds like something people may want to do. That threshold could be read from BATMAN, or I guess a site.conf variable also works. A compile-time constant sounds rather inflexible. |
I now implemented the configuration of the threshold via site.conf (passed to UCI, passed to the command line). I also improved both the readme and the usage help. In turn, I removed the possibility to specify multiple interfaces, as that doesn't really make sense: We expect to find TQs from bat0 for every gateway, but that means we can only handle bat0 or a bridge on top of it as incoming interface. |
I didn't get to have a closer look at this yet, but I usually try to avoid UCI for Gluon packages as much as possible, as each added option will be leftover configuration if the package is ever removed on a firmware upgrade. |
686e69d
to
4e79529
Compare
The alternative would be to read stuff directly from the site.conf, right? That's not quite as easy using either bash/initscripts or C. Also, node owners cannot alter the behavior of the program that way. So how much are you opposed to using UCI? |
4e79529
to
7f76474
Compare
This is to be added upstream [1], but while that still takes time we want to test and use it here already. [1]: freifunk-gluon/gluon#838
According to my tests, batman-adv tell which gateway is selected (/sys/kernel/debug/batman_adv/bat0/gateways). while working with the datas returned we need only to read 2+2xgateway lines and can drop all ra's issued from gateways which are not marked as default. |
@jjsarton Part of the design goal was to not rely on batman-adv's internal gateway selection, as that implies being an IPv4 router, which may not always be true for IPv6 routers. Furthermore, there already is a canonical way to announce IPv6 routing capability: Router Advertisements. |
@jplitza I have read tje thread again and apparently the assumption ist that there are IPv4 only GW and IPv6 Only gateways. Is this true ? |
@jjsarton Yes, the assumption is that IPv4 gateways are not necessarily IPv6 gateways and vice versa. And yes, batman-adv operates on layer 2 and thus doesn't know anything about either IPv4 or IPv6. Its internal gateway selection feature depends on the (IPv4) gateways actively announcing in their batman-adv packets that they are (IPv4) gateways. But the semantics are for IPv4, not IPv6, and piggy-backing on this feature would make it impossible to have an IPv6 gateway without IPv4 connectivity or vice versa. |
Am 08.09.2016 17:34 schrieb "Matthias Schiffer" notifications@github.com:
I can't aggree to this philosophy. yes, you are right. there maybe some maybe we could add a naming convention for uci usage something like package2.parameter1 ... package10.parameter1 and a section than you could set up to 10 custom packages in site.conf wich can be would all be possible, but i ask me why we want to avoid this let it be 1 why do you think it is important to save this small amount of config data? |
This is to be added upstream [1], but while that still takes time we want to test and use it here already. [1]: freifunk-gluon/gluon#838
The version in the PR is not compatible with 2016.2 (and all other COMPAT 15 releases i guess). |
I would suggest to add something like "CONFLICTS=gluon-ebtables-filter-ra-dhcp" to the Makefile. I'm not sure, whether there is such an option in OpenWRT Makefiles? |
You should mention in your README, that only hosts which are actively sending router advertisements (with default routes! in) are considered as routers. So there is also no need in being a batman "gateway" (which refers to the ipv4 features of batman). Maybe you should also avoid calling the ipv6 routers as "gateways" in your README. |
@lemoer CONFLICTS does exist in OpenWrt Makefiles, but I don't see why these packages should conflict (if there is a file or logical conflict at the moment, we should rather try to fix the conflict). |
@NeoRaider You are right. I misinterpreted the README. Only in case you want to have local routers, you become in troubles. |
This enables the ebtables internal locking mechanism which will avoid race conditions between multiple, concurrent ebtables calls. Signed-off-by: Sven Eckelmann <sven@narfation.org>
@jplitza only after having documentation now i noticed the default of 20 for the TQ-difference that is necessary for a switch. |
@rotanid Remember that switching the gateway possibly incurs an IP address change in connected clients and thus shouldn't be done lightheartedly. Favoring a gateway with TQ 255 over one with TQ 254 (possibly only temporarily) isn't really an advantage worth the change. Furthermore, I simply copied the default behavior for IPv4: Gluon sets the B.A.T.M.A.N. adv gateway selection class to 20 by default, which has the exact same semantics. |
docs/package/gluon-radv-filterd.rst
Outdated
|
||
gluon_radv = { | ||
threshold = 20, | ||
} |
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.
gluon_radv
-> radv_filter
?
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.
Should be radv_filterd
according to the check_site.lua
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.
Where was my mind…
Fixed.
docs/package/gluon-radv-filterd.rst
Outdated
site.conf | ||
--------- | ||
|
||
radv_filter.threshold : optional |
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.
Should be radv_filterd.threshold
Before I get angry mails: I don't want this to be integrated in this PR. I just wanted to post it where I (and other people) can find it again in the future. This message contains the result of my eBPF tests which I did together with gluon-radv-filterd It might be required to to switch to eBPF at some point as shown in http://man7.org/linux/man-pages/man2/bpf.2.html. The kernel itself uses at the moment the eBPF JIT compiler/interpreter also for cBPF bytecode (https://cilium.readthedocs.io/en/latest/bpf/#bpf-and-xdp-reference-guide). But of course, cBPF bytecode doesn't use the extra (performance relevant and otherwise helpful) features which are provided by eBPF. Luckily, the program only requires a single register at the moment for the and thus doesn't gain much from the change at the moment (this might change in the future). It is therefore unlikely that we gain anything right now from switching to eBPF The eBPF bytecode can be generated using
The program can then use the bytecode by switching to SO_ATTACH_BPF and using the bpf syscall to send the bytecode to the kernel.:
|
@mweinelt and me brainstormed a little on the documentation. Here is the result as a patch: https://paste.irrelefant.net/aicai0Of.txt |
Build fails for me when merged against the current master. cp -fpR /scratch/hexa/build/gluon/lede/build_dir/target-mips_24kc_musl-1.1.16/root-ar71xx /scratch/hexa/build/gluon/lede/build_dir/target-mips_24kc_musl-1.1.16/root.orig-ar71xx
/scratch/hexa/build/gluon/lede/staging_dir/hostpkg/bin/lua: ...atch/hexa/build/gluon/lede/../scripts/check_site.lua:62: bad argument #1 to 'unpack' (table expected, got string)
stack traceback:
[C]: in function 'unpack'
...atch/hexa/build/gluon/lede/../scripts/check_site.lua:62: in function 'loadvar'
...atch/hexa/build/gluon/lede/../scripts/check_site.lua:83: in function <...atch/hexa/build/gluon/lede/../scripts/check_site.lua:82>
(tail call): ?
...atch/hexa/build/gluon/lede/../scripts/check_site.lua:150: in function 'need_table'
stdin:1: in main chunk
[C]: in function 'dofile'
...atch/hexa/build/gluon/lede/../scripts/check_site.lua:187: in main chunk
[C]: ?
postinst script ./usr/lib/opkg/info/gluon-radv-filterd.postinst has failed with exit code 1 |
Besides this build issue (most likely introduced by the multi domain preparations on the master branch) what's blocking the merge? |
Fixup for @mweinelt 's build issue: https://paste.irrelefant.net/Aetiesi0.txt |
Merged with the following changes:
Thanks! |
Thanks to everybody who made this merge possible! 😄 |
This package drops all incoming router advertisements except for the default router with the best metric according to B.A.T.M.A.N. advanced.
Note that advertisements originating from the node itself (for example via gluon-radvd) are not affected.
This PR supersedes #718.
Caveat: Router solicitations sent by clients are still broadcast, and all but one response are dropped. Making them unicast would require fiddling with the packet, which isn't easily possible from userspace as we've seen in #718.