-
Notifications
You must be signed in to change notification settings - Fork 110
odhcpd lease sharing via shared-state #1199
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
base: master
Are you sure you want to change the base?
Conversation
… received via shared state
|
Very good ! ! in fact amazing ! Just a few comments. Shared-state has been trough a review and reimplementation. This is still a work in progress. The implementation you made seems to be based on the old format. Please update it to the new way. I think that the Readme from shared-state-async may have some improvements. So feel free to propose them. Also the info from this pull request can be a good readme for the package. |
javierbrk
left a comment
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.
@G10h4ck can yo take a look for me it seems to be ok
packages/shared-state-odhcpd_leases/files/etc/uci-defaults/90_odhcpd-lease-share
Outdated
Show resolved
Hide resolved
...shared-state-odhcpd_leases/files/etc/shared-state/hooks/odhcpd-leases/generate_odhcpd_leases
Outdated
Show resolved
Hide resolved
...red-state-odhcpd_leases/files/etc/shared-state/publishers/shared-state-publish_odhcpd_leases
Outdated
Show resolved
Hide resolved
packages/shared-state-odhcpd_leases/files/usr/bin/shared-state-publish_odhcpd_leases
Show resolved
Hide resolved
packages/shared-state-odhcpd_leases/files/etc/uci-defaults/90_odhcpd-lease-share
Outdated
Show resolved
Hide resolved
|
Hi! I installed odhcpd and then this package on a router that was already flashed, so it seems that the /etc/ethers file was already there and the link was not created by this line: The TRIGGERFILE variable seems missing a I disabled and stopped dnsmasq, but now I fear it was the wrong thing to do...? So do we still need dnsmasq? (Actually it makes sense, as the package "dnsmasq" is selected by default in the buildroot. What we were adding, and hopefully will not be nneded anymore after this PR is the "dnsmasq-dhcpv6") |
-L verifies if file exists and if it is already a symlink and now routers that already have a regular file get it force-replaced by the mesh lease symlink.
f239950 to
81b6842
Compare
|
I've corrected those two errors you mentioned. (I've got a problem with some last commit when I updated this branch with the master that's why I forced pushed to reverse the last commit, please check if it's all right) About dnsmasq, I think we need that package, because we only replace the DHCP from it (with
|
|
I tried to compile an image using the OpenWrt BuildRoot and using the lime-packages feed including the new package from this pull request. By default, the BuildRoot selects So looks like we should depend on odhcpd-ipv6only also, in order to be as compatible as possible with pristine OpenWrt, no? |
|
Interesting, as you can see here, full If you think this is right, we should mention that also in development section where there is an advice of deselecting this package because it can be problematic. |
I fear I did a big mistake: I assumed that OpenWrt was using odhcpd both for IPv4 and for IPv6, but as they include by default If this is the case, this package is still very good for sharing the odhcpd leases, but we will still need also shared-state-dnsmasq_leases even when going with OpenWrt default packages selection (which would be amazing, at least in my opinion), right? So (still assuming that this is the case) the question is: why does
Can't we just stop recommending people to deselect Regarding the dependencies to use in the Makefile of shared-state-odhcpd_leases: do you think would be ok to not include neiter odhcpd nor odhcpd-ipv6only, so that people can select what they prefer? Are there going to be problems and errors if no odhcpd at all is present? Additional info: with dnsmasq we are not managing the sync of the IPv6 leases. Also, this PR refers to #1189 and #294.
My plan was to just delete these lines:
|
|
This package won't work without full I founded something useful while digging into dnsmasq-dhcpv6. This variant adds DHCPv6 support to the normal dnsmasq binary, one single daemon can hand out both IPv4 and IPv6 leases on each interface. This relates with the next task about removing VLAN from Babel interfaces, this explanation will be improved in an issue, but I think it was worth the mention |
|
Just a few comments of dnsmasq-dhcpv6 LibreMesh’s current base image installs Inside Because odhcpd is absent, the first branch hits, and dnsmasq starts with both In the man page of dnsmasq you can see that with the directive enable-ra, dnsmasq begins answering Router-Solicit packets and periodically multicasts Router Advertisements. The RA flags you set in the dhcp-range decide how clients build their IPv6, it seems that by default it uses the SLAAC system, that is an IPv6 feature where every host combines the advertised /64 with a locally generated Interface ID to make a globally unique address. What I want to say with all of this info, is that its possibly an option to use full dnsmasq instead of sharing with odhcpd, because with this we are just using one daemon for DNS and DHCP together More info: I hope that all this info helps us to understand better this situation! |
Then it has to depend on
Thanks for doing all this research! What you says makes a lot of sense, and it is what is currently happening in LibreMesh. Yet, OpenWrt people are using And maybe we also need those features of dnsmasq, for example this one looks like using it:
Going 100% on Opinions? |
This would be ideal, imho. But to avoid this kind of errors #1199 (comment) I guess we should stick with the default packages from openwrt: dnsmasq (dns + dhcpv4) and odhcpd_ipv6only (dhcpv6/ra).
Yes this is already doable/done, then in the package selection we are adjusting packages prefixing an hyphen to remove |
|
My opinion is that we can merge this PR only at the condition that the default LibreMesh setup based on In LibreMesh we opted for I don't get why there is such urge to not use |
This implementation replaces the legacy dnsmasq-based system and is built to integrate cleanly with odhcpd and the shared-state-async daemon.
When odhcpd assigns or renews a lease, it executes the script defined in the leasetrigger option.
The trigger script (
shared-state-publish_odhcpd_leases) is called. This script:The shared-state hook (
shared-state-generate_odhcpd_leases) is executed on the remote node. This script:This is an initial implementation, there are areas of improvement like IPv6 support. Feedback, suggestions for improvement, and collaborative testing are highly encouraged and appreciated as we work towards finalizing this feature.