Skip to content
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

luci-mod-network: add support for netifd bonding device #7086

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

hitech95
Copy link
Contributor

This pull request addresses a gap in Luci's functionality by introducing support for device type bonding configuration.
While Netifd already incorporates this capability, Luci currently lacks the means to manage it effectively through its web interface.

immagine
immagine

…ridge

This is to prepare for tho bonding implementation,
 two configuration nodes cannot share the same name.

Probably a more unique and precise naming would be better.

Signed-off-by: hitech95 <[email protected]>
@systemcrash
Copy link
Contributor

hi @hitech95 thanks for this PR. It looks useful. It will certainly satisfy a number of users who have asked for it.

I know that e.g. @jow- has a deeper understanding of this (and opinions on why it might (not) work).

In the meantime, search through the other PRs for bonding.

o = this.replaceOption(s, 'devadvanced', form.Value, 'monitor_interval', _('Monitor Interval'));
o.description = _('Specifies the link monitoring frequency in milliseconds');
o.placeholder = '100';
// FIXME - we have to put a limit/range here somehow?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Example

		o.datatype = 'range(0,500)';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know that I can use the range method to specify the range as I already did on other controls, but the limit might depends on what you have selected as the preferred monitor system.
As the same value is used to configure MII or ARP monitoring.

See for arp_interval and miimon options in:
https://www.kernel.org/doc/Documentation/networking/bonding.txt

Copy link
Contributor

@systemcrash systemcrash Apr 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's sensible and what's absurd here? If they depend on others, then o.datatype is inflexible. Using o.validate allows access to other parameters already set via e.g.

var value = this ? this.section.formvalue('devadvanced', 'xyz'): 1000;

Then.... decide based on those other values, or what's set.

netifd support for bonding devices has been added in commit 5ba9744,
 the other way to have bonding available in luci was via the luci-proto-bonding
 a and its proto-bonding dependency.

This method forces to have an IP address on a bond interface
 due to a netifd limitation. This can causes some issues if you want
 to bridge an ethernet port and a bond.

This patch aim to have the proto device to be configurable not
 as an interface protocol but as a device.

Signed-off-by: hitech95 <[email protected]>
@hitech95
Copy link
Contributor Author

@systemcrash I've modified the limit above, the netifd takes an integer so I've used that limit in there as I wasn't able to find the real upper limit in the kernel documentation.

I've also added dependencies for another parameter. and the "primary" interface selection for the active backup policy.

@systemcrash
Copy link
Contributor

@hitech95 Is this ready as-is?

@svanheule is this something you could test in a switch environment to verify bonding functionality?

@systemcrash
Copy link
Contributor

@jow- any opinions?

@svanheule
Copy link
Member

@svanheule is this something you could test in a switch environment to verify bonding functionality?

I'm afraid I'm currently not set up for this and I won't have time soon either to do so.

@hitech95
Copy link
Contributor Author

@hitech95 Is this ready as-is?
Yep it should be, this uses netifd built in bonding support instead of the external protocol.
I'm using it on my prod router machine (x86).

Tested mainly on LACP on 2 uplinks.
During initial tests I've also tested backup mode.

@systemcrash
Copy link
Contributor

@hitech95 If you want to get this in sooner, I recommend drumming up a bit of support in the forums and see whether anyone else can verify this and throw it into their installation. Some of the switch threads would be a good start!

@systemcrash
Copy link
Contributor

@svanheule wonder if your situation has changed any?

@svanheule
Copy link
Member

@systemcrash It hasn't and is unlikely to change the coming months. Most efficient way is probably to set something up yourself :)

@systemcrash
Copy link
Contributor

@hitech95 What device do you test on? Might be faster to find some guinea pigs in the forums who have the same device to be able to test.

@hitech95
Copy link
Contributor Author

hitech95 commented Jul 6, 2024

@systemcrash
I've it currently up and tested on x89 VM and I've tested onthe mediatek target.
That said I have it running in production with LACP to a GS1900 switch (now with 3Eth uplink).
But on the other device I have only checked that the correct parameters has been set to the network configuration.
I have not tested the netifd stuff is working as I've assumed it was working as it is included by default.

I'm gonna ask some guys if they can test it out too in the forum.

@systemcrash
Copy link
Contributor

systemcrash commented Jul 8, 2024

I threw it out to a forum thread (slightly earlier than your post) for Realtek (GS1900), and the word is that the driver is currently broken (but not necessarily an older version?). But others may be inclined?

@hitech95
Copy link
Contributor Author

hitech95 commented Jul 9, 2024

I'm trying to get help from some friends to crete a virtual lab to test things out.
Unfortunatly we are a bit busy with work/univeristy.

I'll update you when some more test are completed.
We can (probably) test on the follwing hardware:

  • mediatek
  • ramips
  • ath79 (not 100% sure if I have a compatible device as all my ath79 are quite old)
  • x86 (can test also with Openwrt and Debian, probably also Opnsense not sure what is supported over there)

The main stopper here is the lack of vendor devices with OEM firmwares to test against.
(All my network devices runs openwrt with the exceptio of my GS1900)

@systemcrash
Copy link
Contributor

Any progress on this?

@hitech95
Copy link
Contributor Author

hitech95 commented Jul 23, 2024

not yet :(

@systemcrash
Copy link
Contributor

@hitech95 found any other testers in the forums?


o = this.replaceOption(s, 'devadvanced', form.Value, 'ad_actor_system', _('MAC address for LACPDUs'));
o.description = _('This specifies the mac-address for the actor in protocol packet exchanges (LACPDUs). The value cannot be NULL or multicast.');
o.datatype = 'macaddr';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a couple of error checks here to prevent NULL and multicast.

Can we add any predefined values here? e.g. 01:80:C2:XX:XX:XX? Or should this be the underlying MAC of the device(s) - which one could also probe for and provide?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added to my TO-DO list

@howels
Copy link

howels commented Sep 20, 2024

Need to ascertain if bonding is supported on Realtek yet, then I can test here also.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants