Skip to content

Fixed #1639 Apply filter to NodeReconciler similarly as PR #1607 #1666

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.

Sign up for GitHub

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

Jump to bottom
Merged
fedepaol merged 6 commits into from
Nov 23, 2022
Merged

Fixed #1639 Apply filter to NodeReconciler similarly as PR #1607 #1666

fedepaol merged 6 commits into from
Nov 23, 2022

Conversation

@giulianopz
Copy link
Contributor

@giulianopz giulianopz commented Oct 26, 2022

Fix as suggested by @mac-chaffee in issue #1639.

@fedepaol
Copy link
Member

fedepaol commented Oct 28, 2022

Hi @giulianopz , thanks for the PR!

I have a couple of asks:

@giulianopz
Copy link
Contributor Author

giulianopz commented Nov 3, 2022

Hi @fedepaol,
I added a unit test just as the one in config_controller_test.go, but the test fails when I run invoke test:

W1104 00:20:23.634880   69068 warnings.go:70] metallb.io v1beta1 AddressPool is deprecated, consider using IPAddressPool
W1104 00:20:23.635792   69068 warnings.go:70] metallb.io v1beta1 AddressPool is deprecated, consider using IPAddressPool
--- FAIL: TestNodeReconciler_SetupWithManager (6.90s)
    node_controller_test.go:138: 
        Expected
            <*fmt.wrapError | 0xc0002937a0>: {
                msg: "error listening on :8080: listen tcp :8080: bind: address already in use",
                err: <*net.OpError | 0xc000533ae0>{
                    Op: "listen",
                    Net: "tcp",
                    Source: nil,
                    Addr: <*net.TCPAddr | 0xc000c3e630>{IP: nil, Port: 8080, Zone: ""},
                    Err: <*os.SyscallError | 0xc000293780>{
                        Syscall: "bind",
                        Err: <syscall.Errno>0x62,
                    },
                },
            }
        to be nil
FAIL
FAIL    go.universe.tf/metallb/internal/k8s/controllers 19.415s

It seems a conflict between the two unit tests, but I cannot understand where in the code the port 8080 is selected.
Can you help me? Did I miss some configuration to run the unit tests locally?

@fedepaol
Copy link
Member

fedepaol commented Nov 11, 2022

Hi, sorry about the delay. I found the problem, which is about the controller metrics listener (don't know why, since every test stops the testEnv.

In any case, the fix is to not listen for the metrics (in both the old and the new test, I guess):

m, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"})

fedepaol
fedepaol reviewed Nov 11, 2022
View reviewed changes
@giulianopz giulianopz requested review from fedepaol and removed request for gclawes, johananl, oribon, rata and russellb November 12, 2022 23:43
@giulianopz
Skip unnecessary node events in node reconciler
fb06919 
This change applies a filter to the node reconciler
(similarly as the one used for the config reconciler)
to skip processing unnecessary node events
other than node updates involving a change in its labels,
since these labels are checked by the bgp controller.

It will help reducing noise in the logs and
avoid frequent reloads causing CPU usage spikes
which seem to occasionally trigger failing probes and
pod restarts.

Signed-off-by: giuliano <panzironi.giuliano@gmail.com>
fedepaol
fedepaol reviewed Nov 14, 2022
View reviewed changes
@giulianopz
Check only new node name and implement missing predicate funcs
91822d3 
Signed-off-by: giuliano <panzironi.giuliano@gmail.com>
@giulianopz giulianopz requested a review from fedepaol November 16, 2022 11:32
fedepaol
fedepaol reviewed Nov 21, 2022
View reviewed changes
internal/k8s/controllers/node_controller.go Outdated
return node.Name == r.NodeName
})

return newNodeObj.Name == newNodeObj.Name
Copy link
Member

@fedepaol fedepaol Nov 21, 2022

Choose a reason for hiding this comment

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

this will always be true? Can you use the function you added here?

Copy link
Contributor Author

@giulianopz giulianopz Nov 22, 2022

Choose a reason for hiding this comment

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

Sorry, totally missed it. Ok, I will reuse the new function.

internal/k8s/controllers/node_controller.go Outdated
Complete(r)
}

func (r *NodeReconciler) filter(obj client.Object) bool {
Copy link
Member

@fedepaol fedepaol Nov 21, 2022

Choose a reason for hiding this comment

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

nit (just becauwe you need to change the code anyway), let's call this "filterOtherNodes"

@giulianopz giulianopz requested a review from fedepaol November 22, 2022 22:20
@fedepaol
Copy link
Member

fedepaol commented Nov 23, 2022

LGTM, thanks! Let's wait for CI

@fedepaol fedepaol merged commit 27d59d3 into metallb:main Nov 23, 2022
@fedepaol fedepaol mentioned this pull request Nov 23, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fedepaol fedepaol Awaiting requested review from fedepaol fedepaol is a code owner

Assignees

No one assigned

Labels

hacktoberfest-accepted

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@giulianopz @fedepaol