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

Introducing portprober as successor to uplinkprober #4113

Merged
merged 1 commit into from
Jul 31, 2024

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Jul 26, 2024

As we generalize the concept of NI port probing, which is currently limited to uplink & freeuplink shared labels and one port selected for all NI routes, into a multipath routing with probing-based port selection with user-defined shared labels, it is necessary to replace uplinkprober with a more advanced component (and preferably avoid using "uplink" for the name).
See: lf-edge/eve-api#53

This commit introduces portprober - a sucessor to uplinkprober. It will be used by zedrouter to probe port connectivity for multipath IP routes. These are routes that select multiple possible output ports using a shared label. This can be EVE-defined label, such as uplink matching all mgmt ports, or a user-defined label selecting a custom subset of ports. A typical example of multipath route is the default route for NI with multiple ports attached.

For now we will not support load-balancing and zedrouter will have to pick one output port at a time for each multipath route. This will be done by portprober. It will probe connectivity of each port (with possibly user-defined probing endpoint and probing method) and based on the results and some other criteria such as cost, wwan signal strength, etc., it will pick the best port. The probing algorithm is pretty much the same as implemented in uplinkprober, just extended to support user-defined shared labels, user-defined probing method, etc.

Note that in this commit we just add the portprober package and make only minimal changes in the pillar/types package (e.g. introducing SharedLabels into DPC & DNS). In the follow-up PR(s), uplinkprober will be removed and zedrouter will be updated to support multipath routes and to use portprober instead.

@milan-zededa milan-zededa force-pushed the portprober branch 4 times, most recently from 18e2624 to 7bffaa9 Compare July 29, 2024 14:27
@milan-zededa milan-zededa marked this pull request as ready for review July 29, 2024 14:27
@@ -1030,7 +1030,7 @@ func (n *nim) makeLastResortDPC() (types.DevicePortConfig, error) {
},
}
dns := n.dpcManager.GetDNS()
portStatus := dns.GetPortByIfName(ifName)
portStatus := dns.LookupPortByIfName(ifName)
Copy link
Contributor

Choose a reason for hiding this comment

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

this returned meaning should be 'portConfig', rather than status

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But this is from DNS, so there is status information additionally to the config.


// StartPortProbing tells PortProber to start periodic probing of network ports
// for this multipath route.
// It is called by zedrouter whenever a new multipath route is configured for an existing
Copy link
Contributor

Choose a reason for hiding this comment

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

Is "multipath route" the corrrect term to use here?

A linux multipath route is a route for one destination prefix which has two or more nexthops (interface,gateway). But isn't the concept here a NI with multiple exits?

Thus if I somehow configure a route for 10.1.2.3 with nexthops 11.0.0.1 on eth0 and 12.0.0.1 on eth1, would there be an new prober created?
Note that we might not be able to configure such routes today, but I concerned about terminology confusion today and tomorrow.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the api today does allow such a case for loadsharing ip static routes. so in this function, we need to detect this route, regardless of gw, has already been probed

Copy link
Contributor Author

@milan-zededa milan-zededa Jul 30, 2024

Choose a reason for hiding this comment

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

So the idea behind the API enhancements done in lf-edge/eve-api#53 is to extend/generalize the Local NI network model.

First, as you pointed out, we want to allow NI having multiple ports (exits to external networks).
Second, to solve the problem of the default route, which with multiple ports is often a multipath route but we need to pick one port at a time, I decided to generalize the probing and port selection functionality to any static IP route with multiple exits (instead of creating solution for the default route only).

Today user configures dst network + gateway IP for static route. After those API changes, they will be able to select output port(s) additionally to or instead of gateway IP using logical/shared port labels (see here). If there are multiple valid output ports for a route, user can select them using a shared label. Then we get a multipath route. Probing will be used to select one output port at a time (and user can customize method & endpoint to use for probing for a given route). I believe that users will prefer to configure dst+port without gateway IP and let EVE to find out what the gateway IP for a particular port is (based on the port config received from DHCP or applied statically). The exceptions are routes where GW is another app. In those cases user will configure dst+gateway and leave the port empty (since the port is an app VIF for which we do not have labels).

Still they can configure all 3 parameters: dst + port + gateway. But then only ports whose subnets match the gateway IP will be considered. In normal scenarios (non-overlapping port subnets) this prevents multipath routes (hence with the current API we cannot really create multipath routes). And user cannot configure different gateway IP for each port inside the route config - that would not go well with the concept of shared labels (i.e. not listing ports one-by-one but using one label assigned to each). But I think that the scenario of the default port gateway not being suitable for a given route are rather rare.

With this enhanced model, the default route will be treated as yet another multipath IP route (either user defined or automatically generated by EVE based on the NI config).
So today's NI with the "uplink" port label will turn into NI which has all mgmt ports as exits and the default route will have the output port selected by portprober (while single-path routes, such as link-local or connected routes, will always be present in the NI routing table). On top of that, user will be able to add custom multipath routes (for the default or a specific dst) where portprober will be used as well to select and failover between ports.
Hope it makes sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this we'll use the Linux kernel multipath route support? Or you will be handling the path selection using the probing in user space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or you will be handling the path selection using the probing in user space

This. We will be always installing only normal routes with single gateway + port. In zedrouter/portprober the port selection will be made (and possibly changed in runtime to fail-over when probing through the current port starts to fail).

As we generalize the concept of NI port probing, which is currently
limited to uplink & freeuplink shared labels and one port selected
for all routes, into a multipath routing with probing-based port
selection with user-defined shared labels, it is necessary to replace
uplinkprober with a more advanced component
(and preferrably avoid using "uplink" for the name).
See: lf-edge/eve-api#53

This commit introduces portprober - a sucessor to uplinkprober.
It will be used by zedrouter to probe port connectivity for multipath
IP routes. These are routes that select multiple possible output ports
using a shared label. This can be EVE-defined label, such as "uplink"
matching all mgmt ports, or a user-defined label selecting a custom
subset of ports.
For now we will not support load-balancing and zedrouter will have to
pick one output port at a time for each multipath route. This will be
done by portprober. It will probe connectivity of each port (with
possibly user-defined probing endpoint and probing method) and based
on the results and some other criteria such as cost, wwan signal
strength, etc., it will pick the best port. The probing algorithm is
pretty much the same as implemented in uplinkprober, just extended to
support user-defined shared labels, user-defined probing method, etc.

Note that in this commit we just add the portprober package and make
only minimal changes in the pillar/types package (e.g. introducing
SharedLabels into DPC & DNS).
In the follow-up commit(s), uplinkprober will be removed and zedrouter
will be updated to support multipath routes and to use portprober
instead.

Signed-off-by: Milan Lenco <milan@zededa.com>
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

I see a test failure for the networking tests:
FAIL: ../eclient/testdata/acl.txt:95: unexpected command success

I don't recall that being a spurious failiure we've seen before. Did something break (in an earlier PR or this one)?

[Re-running the tests]

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Re-run eden

@eriknordmark eriknordmark merged commit f55efa1 into lf-edge:master Jul 31, 2024
85 of 87 checks passed
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