Support L2 mode specify interfaces for announcing LB IP #1536

qingwusunny · 2022-07-26T08:42:31Z

This PR implement the feature that MetalLB announce IP from specified interfaces for layer2. Related design is #1359.

I has add two basic e2etest about interface selector.

fedepaol · 2022-07-26T15:13:16Z

speaker/layer2_controller.go

 }

+func getInterfaces(localNode string, l2Advertisements []*config.L2Advertisement) []string {
+	ifs := sets.NewString()


This won't handle the case where you have one advertisement with no interface (and thus, collecting all of them) and one other advertisement with a selector.
The desired result is to advertise all of them, but we'll advertise to a subset only.

Yes, but the problem is, if you have two l2 advertisements, one with some interfaces and another one with no interfaces, the semantic there is : advertise to all (because you have one l2 advertisement with no interfaces) but your if won't trigger because len(interfaces) is not 0

yes, I'm ill-considered.

fedepaol · 2022-07-26T15:17:22Z

speaker/layer2_controller.go


-func (c *layer2Controller) SetConfig(log.Logger, *config.Config) error {
+func (c *layer2Controller) SetConfig(l log.Logger, cfg *config.Config) error {
+	interfacesMap := make(map[string][]string)


You are translating a set to []string from inside and then translating back when you use it.
Can't interfacesMap just be a map[string]set ?

fedepaol · 2022-07-26T15:17:54Z

speaker/layer2_controller_test.go

 	}
 }
+
+func equalStringSlice(a []string, b []string) bool {


put this on the bottom, as it's a util function

fedepaol · 2022-07-26T15:31:07Z

internal/layer2/announcer.go


+type spam struct {
+	ip       net.IP
+	poolName string


I am not sure I like the idea of poolName leaking down here.
I think a better design would be getting the list of interfaces from outside as an option of spam

If the struct is (ip, interfaces), when L2Advertisement changed，I am afraid the announced LB IP can't update its related interfaces.
By the way, I am sorry that I found I hasn't modify func (a *Announce) shouldAnnounce(ip net.IP).

If the struct is (ip, interfaces), when L2Advertisement changed，I am afraid the announced LB IP can't update its related interfaces.

When a configuration changes, MetalLB will reprocess all the services, which in turn will call SetBalancer again, so this won't be an issue.

See

metallb/speaker/main.go

Line 434 in 9fce695

level.Debug(l).Log("event", "startUpdate", "msg", "start of config update")

Regarding the namespace, MetalLB will filter objects not belonging to its namespace, so it's safe to assume namespace is not part of the key

ok, I get it.

fedepaol · 2022-07-26T15:32:20Z

internal/layer2/announcer.go

+func (a *Announce) SetBalancer(name string, ip net.IP, poolName string) {
 	// Call doSpam at the end of the function without holding the lock
-	defer a.doSpam(ip)
+	defer a.doSpam(ip, poolName)


here I would pass the list of interfaces (instead of poolName)

It has been modified.

fedepaol · 2022-07-26T15:33:50Z

speaker/layer2_controller.go

 func (c *layer2Controller) SetBalancer(l log.Logger, name string, lbIPs []net.IP, pool *config.Pool) error {
 	for _, lbIP := range lbIPs {
-		c.announcer.SetBalancer(name, lbIP)
+		c.announcer.SetBalancer(name, lbIP, pool.Name)


Assuming you maintain the mapping between pool name and interface outside, here you can just pass the list of interfaces.

fedepaol · 2022-07-26T15:42:18Z

internal/layer2/announcer.go

 func (a *Announce) spamLoop() {
 	// Map IP to spam stop time.
-	m := map[string]time.Time{}
+	type spamInfo struct {


I'd call this timedSpam or deferredSpam.

fedepaol · 2022-07-26T15:44:37Z

Overall looks good. I need to sleep a bit over the select loop and look at it carefully.
Also, I'd be more comfortable in having at least one or two basic e2e tests as part of this PR. This will give us confidence that we are merging something that works.

fedepaol · 2022-07-27T07:53:39Z

Couple of extra notes: I'd change the commit message (we are trying to adhere to https://cbea.ms/git-commit/ as guidelines)
For later, when we converge on the review:
I'd add also a commit changing the release notes (0.13.5 is not there yet, you can add it) to mention this feature.
I'd add a commit expanding the "advanced l2 configuration" section of the doc with this change

fedepaol · 2022-07-29T15:19:03Z

internal/layer2/announcer.go

 	// Map IP to spam stop time.
-	m := map[string]time.Time{}
+	type timedSpam struct {
+		t time.Time


I'd call this "until"

fedepaol · 2022-07-29T15:20:11Z

internal/layer2/announcer.go

 }

-func (a *Announce) gratuitous(ip net.IP) {
+func (a *Announce) gratuitous(ipA ipAdvertisement) {


I'd call ipA -> adv

fedepaol · 2022-07-29T15:23:15Z

internal/layer2/announcer.go

 }

+// updateIPs update announce.ips depend on service info.
+func (a *Announce) updateIPs(name string, ip net.IP, interfaces sets.String) (hasUpdate bool) {


This is used by SetBalancer, please put it below it, reasonably after the exported functions

This file's all export functions are in the last. Maybe the function's position can not change for consistency?

fedepaol · 2022-07-29T15:25:23Z

internal/layer2/announcer.go

 }

+// updateIPs update announce.ips depend on service info.
+func (a *Announce) updateIPs(name string, ip net.IP, interfaces sets.String) (hasUpdate bool) {


I'd avoid using named returns, just return the value

fedepaol · 2022-07-29T15:28:29Z

internal/layer2/announcer.go

+					return
+				}
+				ips[i].interfaces = interfaces
+				hasFound = true


we can just return here I think? If so we can remove hasFound

fedepaol · 2022-07-29T15:29:41Z

internal/layer2/announcer.go

 	return dropReasonAnnounceIP
 }

+// updateIPs update announce.ips depend on service info.


"and returns true if the announcement has changed"

fedepaol · 2022-07-29T15:30:33Z

internal/layer2/announcer.go

 }

+// updateIPs update announce.ips depend on service info.
+func (a *Announce) updateIPs(name string, ip net.IP, interfaces sets.String) (hasUpdate bool) {


nit: updateBalancerIPs

fedepaol · 2022-07-29T15:31:11Z

internal/layer2/announcer.go

 }

-func (a *Announce) shouldAnnounce(ip net.IP) dropReason {
+func isMatchIntf(intf string, ifs sets.String) bool {


nit: advertiseOnInterface

fedepaol · 2022-07-29T15:37:37Z

speaker/layer2_controller.go

+		if matchNode, ok := l2.Nodes[localNode]; !ok || !matchNode {
+			continue
+		}
+		if len(l2.Interfaces) == 0 {


Can you add a comment here explaining the semantic? I am also tempted to wrap the set under a
struct {
allInterfaces bool
interfaces Set
}

WDYT? I am not sure I like propagating the len==0 -> all interfaces down there. A flag might be more explicit and understandable.
Ideally, that flag could be even a property of the "internal" l2advertisement, so the only place where we consider len==0 -> all interfaces is the CRD.

fedepaol · 2022-07-29T15:39:15Z

speaker/layer2_controller.go

 	return "notOwner"
 }

+func getInterfaces(localNode string, l2Advertisements []*config.L2Advertisement) sets.String {


Same, put below where it's used. See https://kentcdodds.com/blog/newspaper-code-structure for reference

fedepaol · 2022-07-29T15:44:25Z

Another round, mostly nits. It's converging :-)

qingwusunny · 2022-08-15T02:04:01Z

@fedepaol I have modified the func SetBalancer in announcer.go，review again?

qingwusunny · 2022-08-16T01:34:48Z

Couple of extra notes: I'd change the commit message (we are trying to adhere to https://cbea.ms/git-commit/ as guidelines) For later, when we converge on the review: I'd add also a commit changing the release notes (0.13.5 is not there yet, you can add it) to mention this feature. I'd add a commit expanding the "advanced l2 configuration" section of the doc with this change

Add two commit for release notes and doc in this PR?

qingwusunny · 2022-08-19T01:32:48Z

@fedepaol I have modified the func SetBalancer in announcer.go，review again?

And @oribon

fedepaol · 2022-08-12T16:23:35Z

e2etest/l2tests/interface_selector.go

+			framework.ExpectNoError(err)
+
+			ginkgo.By("checking connectivity to its external VIP")
+			gomega.Eventually(func() error {


Use Consistently here, with a lower timeout (like 1 min)

tasks.py

internal/config/config.go

e2etest/e2etest_suite_test.go

internal/layer2/announcer.go

internal/layer2/ip_advertisement.go

speaker/layer2_controller.go

fedepaol · 2022-08-30T14:16:40Z

Couple of extra notes: I'd change the commit message (we are trying to adhere to https://cbea.ms/git-commit/ as guidelines) For later, when we converge on the review: I'd add also a commit changing the release notes (0.13.5 is not there yet, you can add it) to mention this feature. I'd add a commit expanding the "advanced l2 configuration" section of the doc with this change

Add two commit for release notes and doc in this PR?

Yes exactly, so when we cut a new release we won't have to add a new pr for that.

fedepaol · 2022-08-30T14:17:11Z

@fedepaol I have modified the func SetBalancer in announcer.go，review again?

Sorry for the delay, I was off. Reviewed again!

oribon · 2022-08-31T10:12:25Z

internal/layer2/announcer.go

 	if ip.To4() != nil {
 		for _, client := range a.arps {
+			if !adv.matchInterface(client.intf) {
+				level.Debug(a.logger).Log("op", "announcer", "info", "skip interfaces", client.intf)


change to:
level.Debug(a.logger).Log("op", "gratuitousAnnounce", "skipping interface", client.intf)

(also it will avoid having an odd number of elements which results in something like {"caller":"announcer.go:201","eth0":"(MISSING)","info":"skip interfaces","level":"debug","op":"announcer","ts":"2022-08-31T10:24:05Z"})

oribon · 2022-08-31T10:12:39Z

internal/layer2/announcer.go

 	} else {
 		for _, client := range a.ndps {
+			if !adv.matchInterface(client.intf) {
+				level.Debug(a.logger).Log("op", "announcer", "info", "skip interfaces", client.intf)


same: level.Debug(a.logger).Log("op", "gratuitousAnnounce", "skipping interface", client.intf)

oribon · 2022-08-31T10:34:58Z

internal/layer2/announcer.go

+func (a *Announce) updateBalancerIPs(name string, adv IPAdvertisement) (*IPAdvertisement, bool) {
+	var old *IPAdvertisement
+	if ips, ok := a.ips[name]; ok {
+		for i, _ := range ips {


nit: _ not required (also why not use _, i ?)

oribon · 2022-08-31T10:35:40Z

internal/layer2/announcer.go

+// updateBalancerIPs updates announce.ips depend on service info, and returns old config, a bool indicating if the announcement has changed or not.
+func (a *Announce) updateBalancerIPs(name string, adv IPAdvertisement) (*IPAdvertisement, bool) {
+	var old *IPAdvertisement
+	if ips, ok := a.ips[name]; ok {


re the other comment, can you change ips -> ipAdvertisements here as well?

oribon · 2022-08-31T10:41:59Z

internal/layer2/announcer.go

+	}
+
+	a.ips[name] = append(a.ips[name], adv)
+	return old, true


nit: can we return nil, true here?

this function is gonna be removed I think.

e2etest/l2tests/interface_selector.go

fedepaol · 2022-08-31T15:54:42Z

Couple of extra notes: I'd change the commit message (we are trying to adhere to https://cbea.ms/git-commit/ as guidelines) For later, when we converge on the review: I'd add also a commit changing the release notes (0.13.5 is not there yet, you can add it) to mention this feature. I'd add a commit expanding the "advanced l2 configuration" section of the doc with this change

Add two commit for release notes and doc in this PR?

Yes exactly, so when we cut a new release we won't have to add a new pr for that.

As a second thought, let's leave them out for now. We'll add documentation and declare the feature when we have enough coverage to assure us the feature is working (I am assuming we'll add new tests in another PR)

internal/layer2/announcer.go

speaker/layer2_controller.go

e2etest/l2tests/interface_selector.go

fedepaol · 2022-09-02T15:43:33Z

A couple of nits around the code, which is getting to look really good in my opinion, plus a comment about the e2e tests which I think needs to be sorted out.

oribon

apart from the few unaddressed comments, lgtm
thanks a lot!

Add a new field "Interfaces" to "L2Advertisement". By setting this field, LB IP can be announced only from these specific interfaces.

L2 DeleteBalancer: continue instead of return if one of the service's IP has been used by another service

fedepaol · 2022-09-08T14:41:03Z

Thanks for taking the time to iterate over this! The result is really neat.
LGTM

fedepaol · 2022-09-08T14:44:59Z

@qingwusunny it'd be great if you can extend the e2e tests in a separate PR as we discussed in the past.

qingwusunny requested review from fedepaol, gclawes, johananl, oribon, rata and russellb as code owners July 26, 2022 08:42

qingwusunny force-pushed the update_L2Advertisement branch 5 times, most recently from 7931e36 to ed4dfaa Compare July 26, 2022 11:43

fedepaol reviewed Jul 26, 2022

View reviewed changes

qingwusunny changed the title ~~Support L2 mode specify interfaces for announcing LB IP~~ [WIP] Support L2 mode specify interfaces for announcing LB IP Jul 27, 2022

qingwusunny force-pushed the update_L2Advertisement branch 3 times, most recently from fbd97f3 to baf5e9b Compare July 28, 2022 11:11

fedepaol reviewed Jul 29, 2022

View reviewed changes

qingwusunny force-pushed the update_L2Advertisement branch 10 times, most recently from 2da5636 to 7ae71f0 Compare August 3, 2022 09:45

qingwusunny force-pushed the update_L2Advertisement branch 2 times, most recently from 8512cb8 to 90db169 Compare August 12, 2022 09:22

fedepaol reviewed Aug 30, 2022

View reviewed changes

oribon reviewed Aug 31, 2022

View reviewed changes

qingwusunny force-pushed the update_L2Advertisement branch from 90db169 to 77511eb Compare September 2, 2022 07:38

fedepaol reviewed Sep 2, 2022

View reviewed changes

qingwusunny force-pushed the update_L2Advertisement branch 2 times, most recently from 399b413 to 0fae20a Compare September 7, 2022 03:34

oribon reviewed Sep 7, 2022

View reviewed changes

zhoujiao added 3 commits September 8, 2022 19:08

Layer2: Announce LB IPs from specific interfaces

b1e26fd

Add a new field "Interfaces" to "L2Advertisement". By setting this field, LB IP can be announced only from these specific interfaces.

E2etest: add interface selector e2etest

1f32fda

Layer2: fix bug when delete LB service

4ce7e75

L2 DeleteBalancer: continue instead of return if one of the service's IP has been used by another service

qingwusunny force-pushed the update_L2Advertisement branch from 0fae20a to 4ce7e75 Compare September 8, 2022 11:12

Merge branch 'main' into update_L2Advertisement

cfff2b9

fedepaol merged commit fb96318 into metallb:main Sep 8, 2022

fedepaol mentioned this pull request Sep 21, 2022

Add an option to pass a list of interfaces to create announcers on #277

Closed

cprivitere mentioned this pull request Oct 12, 2022

Add MetalLB 0.13 support kubernetes-sigs/cloud-provider-equinix-metal#322

Closed

eyenx mentioned this pull request Oct 19, 2022

chore(misc/metallb): update to 0.13.x adfinis/helm-charts#834

Merged

6 tasks

mnaser mentioned this pull request Feb 24, 2023

MetalLB fails to send ARPs if interface is attached to Open vSwitch #871

Closed

m-trojanowski mentioned this pull request Mar 1, 2023

Add interfaces field in metallb.io l2advertisement_v1beta1.json datreeio/CRDs-catalog#121

Merged

neoaggelos mentioned this pull request Oct 10, 2023

(1.28) MetalLB issues canonical/microk8s-core-addons#231

Open

Support L2 mode specify interfaces for announcing LB IP #1536

Support L2 mode specify interfaces for announcing LB IP #1536

Uh oh!

Conversation

qingwusunny commented Jul 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingwusunny Jul 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fedepaol commented Jul 26, 2022

Uh oh!

fedepaol commented Jul 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

qingwusunny commented Jul 26, 2022 •

edited

Loading

qingwusunny Jul 27, 2022 •

edited

Loading