l2: exclude common virtual interfaces for announce services #1767

cyclinder · 2023-01-05T06:13:17Z

In arp mode, Some common virtual interfaces should not be used to announce services, such as kube-ipvs0, docker0, etc. This PR will dispatch these common virtual interfaces.

Signed-off-by: cyclinder qifeng.guo@daocloud.io

#1727

l2: exclude common virtual interfaces for announce services.

internal/layer2/announcer.go

fedepaol · 2023-01-17T14:12:43Z

cc @oribon @gclawes want to hear your thoughts about this

oribon · 2023-01-19T07:24:45Z

the idea seems good but maybe having it as a configmap is better
also, if someone has an interface selector that selects one of these will this "global exclude" override it? (might be unrelated, need to revisit the l2 code)

fedepaol · 2023-01-27T09:52:39Z

So, let's move this forward but let's have the list overrideable and not hardcoded. A configmap read upon start is good enough imo, no need to handle the live reload.

fedepaol · 2023-01-27T09:52:54Z

So, let's move this forward but let's have the list overrideable and not hardcoded. A configmap read upon start is good enough imo, no need to handle the live reload.

Wdyt @cyclinder ?

cyclinder · 2023-01-28T12:36:25Z

So, let's move this forward but let's have the list overrideable and not hardcoded. A configmap read upon start is good enough imo, no need to handle the live reload.

I think both are ok, but I prefer hardcoded :)

First of all, it shouldn't be related to the current interface-selector, it's more of a global-exclude list, right? If it is a configmap, I think this will increase its complexity. 🤔

fedepaol · 2023-01-30T14:21:41Z

So, let's move this forward but let's have the list overrideable and not hardcoded. A configmap read upon start is good enough imo, no need to handle the live reload.

I think both are ok, but I prefer hardcoded :)

First of all, it shouldn't be related to the current interface-selector, it's more of a global-exclude list, right? If it is a configmap, I think this will increase its complexity. thinking

right, it must be independent and always applied, more or less as your current logic is but loading that list from a configmap when the process starts instead of having it hardcoded. This will help us to avoid releases when users hit a corner case we didn't think about

cyclinder · 2023-01-31T09:30:47Z

@fedepaol got it. I try to working on this.

cyclinder · 2023-02-08T13:05:42Z

Hi @fedepaol , Could you mind having a review? Thanks. It seems this CI failing isn't related to this changes.

fedepaol · 2023-02-08T13:12:22Z

Hi @fedepaol , Could you mind having a review? Thanks. It seems this CI failing isn't related to this changes.

yup, will do soon-ish (and I have an outstanding pr to fix the ci)

config/manifests/metallb-frr-prometheus.yaml

internal/layer2/announcer.go

cyclinder · 2023-02-27T09:36:26Z

I made some testing base on these changes on my local, It works well, as speaker log shown below:

➜  metallb git:(l2ann) ✗ kubectl get po -n metallb-system         
NAME                          READY   STATUS    RESTARTS   AGE
controller-76f86696dd-xznhb   0/1     Running   0          6s
speaker-ccqtb                 0/1     Running   0          4s
speaker-pzznd                 0/1     Running   0          5s
speaker-vsd9l                 0/1     Running   0          4s
➜  metallb git:(l2ann) ✗ kubectl logs -f -n metallb-system speaker-ccqtb 
{"branch":"l2ann","caller":"main.go:97","commit":"f56103a5-dirty","goversion":"gc / go1.18.3 / amd64","level":"info","msg":"MetalLB speaker starting (commit f56103a5-dirty, branch l2ann)","ts":"2023-02-27T09:16:35Z","version":""}
{"caller":"main.go:148","interfacesToExclude":["docker.*","cbr.*","dummy.*","virbr.*","lxcbr.*","veth.*","lo","^cali.*","^tunl.*","flannel.*","kube-ipvs.*","cni.*","^nodelocaldns.*"],"level":"error","op":"show the list of interfaces that announce to exclude","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"lo","level":"info","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"tunl0","level":"info","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth313c8572","level":"info","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth24b1f795","level":"info","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth8013fa28","level":"info","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:134","event":"createARPResponder","interface":"eth0","level":"info","msg":"created ARP responder for interface","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:143","event":"createNDPResponder","interface":"eth0","level":"info","msg":"created NDP responder for interface","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:134","event":"createARPResponder","interface":"eth1","level":"info","msg":"created ARP responder for interface","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:143","event":"createNDPResponder","interface":"eth1","level":"info","msg":"created NDP responder for interface","ts":"2023-02-27T09:16:35Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth24b1f795","level":"info","ts":"2023-02-27T09:16:45Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth8013fa28","level":"info","ts":"2023-02-27T09:16:45Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"lo","level":"info","ts":"2023-02-27T09:16:55Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"tunl0","level":"info","ts":"2023-02-27T09:16:55Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth313c8572","level":"info","ts":"2023-02-27T09:16:55Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth24b1f795","level":"info","ts":"2023-02-27T09:16:55Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth8013fa28","level":"info","ts":"2023-02-27T09:16:55Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"lo","level":"info","ts":"2023-02-27T09:17:05Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"tunl0","level":"info","ts":"2023-02-27T09:17:05Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth313c8572","level":"info","ts":"2023-02-27T09:17:05Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth24b1f795","level":"info","ts":"2023-02-27T09:17:05Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth8013fa28","level":"info","ts":"2023-02-27T09:17:05Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"lo","level":"info","ts":"2023-02-27T09:17:15Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"tunl0","level":"info","ts":"2023-02-27T09:17:15Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth313c8572","level":"info","ts":"2023-02-27T09:17:15Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth24b1f795","level":"info","ts":"2023-02-27T09:17:15Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth8013fa28","level":"info","ts":"2023-02-27T09:17:15Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"lo","level":"info","ts":"2023-02-27T09:17:25Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"tunl0","level":"info","ts":"2023-02-27T09:17:25Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth313c8572","level":"info","ts":"2023-02-27T09:17:25Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth24b1f795","level":"info","ts":"2023-02-27T09:17:25Z"}
{"caller":"announcer.go:85","event":"announced interface to exclude","interface":"veth8013fa28","level":"info","ts":"2023-02-27T09:17:25Z"}

Could you mind taking a look? Thanks :) @fedepaol

fedepaol · 2023-03-01T16:46:15Z

speaker/main.go

@@ -51,6 +52,8 @@ var announcing = prometheus.NewGaugeVec(prometheus.GaugeOpts{
 	"ip",
 })

+var defaultConfigPath = "/etc/metallb/conf.yaml"


nit: this doesn't have to be a var, could be a const. Also, I'd use the presence of the file instead of the boolean to understand if we need to enable the mechanism. Maybe that's where we want to override the path? But I think it's fair to have it hardcoded.

Also also, I'd do all the parsing in one shot and have the parse function return the regexp, so we have all the logic in one place.

It would be better if we provide a flag to override the path, It's good for helm install：

when the value is empty in values.yaml: We don't create configMap, and we don't mount the file...

When the value is not empty and the file exists, we enable this mechanism， Is it ok?

In addition , I think all the configurations here(

metallb/speaker/main.go

Line 205 in 6a5ad67

type controllerConfig struct {

), should be declared in one place(in a configMap) so that it is easy to configure and update, how about this?

I still don't see the value in setting passing the name as a parameter. It can be a convention.
If we don't want the file, we don't create the volume and the configmap, the load of the file will fail and we won't override.

In addition , I think all the configurations here(

metallb/speaker/main.go

Line 205 in 6a5ad67

type controllerConfig struct {

), should be declared in one place(in a configMap) so that it is easy to configure and update, how about this?

I don't like this approach, having flags give a better understanding of what's being passed to the process and that's what people expect. Configmaps for knobs are fine if they are tweaks that are not normally expected to be used.

I don't like this approach, having flags give a better understanding of what's being passed to the process and that's what people expect. Configmaps for knobs are fine if they are tweaks that are not normally expected to be used.

OK, Thanks for the reply. I just had a whim, please ignore my thoughts

If we don't want the file, we don't create the volume and the configmap, the load of the file will fail and we won't override.

I currently read the env to determine if this mechanism is enabled and if the value is not empty and the file exists then it is enabled. The -set .speaker.excludeInterfaces.configPath flag is used when the helm install. The default configPath is '/etc/metallb/conf.yaml', if it is empty, it is not enabled.

Do you mean we don't need to expose env to enable this mechanism?

I still don't see the value in setting passing the name as a parameter.

The benefit of this is that we can decide whether to enable this mechanism when helm install

we can still decide, we set a parameter in helm that drives the creation of the volume and the related configmap (and by default I'd create them). If those are not created, the go code will land in the notexists error and will not parse the map. Or I am missing something?

Yes, That's what the PR does. The difference is that now this parameter is a string, indicating the config path to the file. Or should this be a boolean type? Then we hardcode the config file path in go code.

fedepaol · 2023-03-01T16:47:41Z

sorry for the delay, left one last comment (I hope). I think the boolean parameter is not necessary as we are using a fixed path by convention, so we can rely on the presence or the absence of the file to drive the parsing

speaker/main.go

fedepaol · 2023-03-17T16:58:10Z

Did one last minute change about naming directly on your branch.
LGTM

In arp mode, Some common virtual interfaces should not be used to announce services, such as kube-ipvs0, docker0, etc. This PR will dispatch these common virtual interfaces. Signed-off-by: cyclinder <qifeng.guo@daocloud.io>

fedepaol · 2023-03-20T08:39:08Z

There was a nit confgmap.yaml

cyclinder · 2023-03-20T10:30:15Z

There was a nit confgmap.yaml

Thanks, Does CI Failling is related to these changes?

fedepaol · 2023-03-20T10:34:44Z

There was a nit confgmap.yaml

Thanks, Does CI Failling is related to these changes?

nope, it's related to the operator not being updated to the latest frr yet (this PR: metallb/metallb-operator#328)

We can ignore the failure for now.

cyclinder requested review from fedepaol, rata, russellb, gclawes and oribon as code owners January 5, 2023 06:13

cyclinder force-pushed the l2ann branch 4 times, most recently from caad5bf to 7d8a004 Compare January 5, 2023 06:59

fedepaol reviewed Jan 17, 2023

View reviewed changes

internal/layer2/announcer.go Outdated Show resolved Hide resolved

internal/layer2/announcer.go Outdated Show resolved Hide resolved

internal/layer2/announcer.go Outdated Show resolved Hide resolved

internal/layer2/announcer.go Outdated Show resolved Hide resolved

cyclinder force-pushed the l2ann branch from 7d8a004 to bca0e63 Compare January 18, 2023 09:14

cyclinder force-pushed the l2ann branch from bca0e63 to d9440a9 Compare January 28, 2023 12:39

cyclinder force-pushed the l2ann branch 4 times, most recently from d3a0c93 to bcbdcde Compare February 7, 2023 06:34

fedepaol reviewed Feb 10, 2023

View reviewed changes

config/manifests/metallb-frr-prometheus.yaml Outdated Show resolved Hide resolved

internal/layer2/announcer.go Outdated Show resolved Hide resolved

cyclinder force-pushed the l2ann branch 4 times, most recently from 25db649 to c37394a Compare February 13, 2023 02:00

cyclinder force-pushed the l2ann branch 3 times, most recently from e069613 to caaea9a Compare February 27, 2023 13:25

fedepaol reviewed Mar 1, 2023

View reviewed changes

speaker/main.go Outdated Show resolved Hide resolved

cyclinder force-pushed the l2ann branch 6 times, most recently from 185bfea to 98eebdd Compare March 14, 2023 09:25

fedepaol force-pushed the l2ann branch from 2b60bef to bc60334 Compare March 17, 2023 16:52

cyclinder force-pushed the l2ann branch 2 times, most recently from 80df3af to fe297f5 Compare March 19, 2023 03:34

l2: exclude common virtual interfaces for announce services

b1257da

In arp mode, Some common virtual interfaces should not be used to announce services, such as kube-ipvs0, docker0, etc. This PR will dispatch these common virtual interfaces. Signed-off-by: cyclinder <qifeng.guo@daocloud.io>

fedepaol force-pushed the l2ann branch from fe297f5 to b1257da Compare March 20, 2023 08:38

fedepaol enabled auto-merge March 20, 2023 08:52

fedepaol added this pull request to the merge queue Mar 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Mar 20, 2023

fedepaol added this pull request to the merge queue Mar 20, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Mar 20, 2023

fedepaol added this pull request to the merge queue Mar 20, 2023

fedepaol merged commit ebc875a into metallb:main Mar 20, 2023
23 of 24 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

l2: exclude common virtual interfaces for announce services #1767

l2: exclude common virtual interfaces for announce services #1767

cyclinder commented Jan 5, 2023 •

edited

fedepaol commented Jan 17, 2023

oribon commented Jan 19, 2023

fedepaol commented Jan 27, 2023

fedepaol commented Jan 27, 2023

cyclinder commented Jan 28, 2023

fedepaol commented Jan 30, 2023

cyclinder commented Jan 31, 2023

cyclinder commented Feb 8, 2023

fedepaol commented Feb 8, 2023

cyclinder commented Feb 27, 2023 •

edited

fedepaol Mar 1, 2023

cyclinder Mar 9, 2023

cyclinder Mar 9, 2023 •

edited

fedepaol Mar 13, 2023

fedepaol Mar 13, 2023

cyclinder Mar 14, 2023

cyclinder Mar 14, 2023

cyclinder Mar 14, 2023

fedepaol Mar 14, 2023

cyclinder Mar 14, 2023

fedepaol commented Mar 1, 2023

fedepaol commented Mar 17, 2023

fedepaol commented Mar 20, 2023

cyclinder commented Mar 20, 2023

fedepaol commented Mar 20, 2023

l2: exclude common virtual interfaces for announce services #1767

l2: exclude common virtual interfaces for announce services #1767

Conversation

cyclinder commented Jan 5, 2023 • edited

fedepaol commented Jan 17, 2023

oribon commented Jan 19, 2023

fedepaol commented Jan 27, 2023

fedepaol commented Jan 27, 2023

cyclinder commented Jan 28, 2023

fedepaol commented Jan 30, 2023

cyclinder commented Jan 31, 2023

cyclinder commented Feb 8, 2023

fedepaol commented Feb 8, 2023

cyclinder commented Feb 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cyclinder Mar 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedepaol commented Mar 1, 2023

fedepaol commented Mar 17, 2023

fedepaol commented Mar 20, 2023

cyclinder commented Mar 20, 2023

fedepaol commented Mar 20, 2023

cyclinder commented Jan 5, 2023 •

edited

cyclinder commented Feb 27, 2023 •

edited

cyclinder Mar 9, 2023 •

edited