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

backport v1.7: node-neigh: Bump arping vsn to accept netlink.Link #15430

Merged
merged 1 commit into from
Mar 25, 2021

Conversation

brb
Copy link
Member

@brb brb commented Mar 23, 2021

See commit msg.

Fix possible deadlock when querying network interfaces for arping

This commit bumps github.com/cilium/arping version to accept
netlink.Link instead of net.Interface.

The change allows us to use netlink to query netdevs which avoids a
possible deadlock described in the previous commit.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb added kind/backports This PR provides functionality previously merged into master. backport/1.7 labels Mar 23, 2021
@brb brb requested a review from a team as a code owner March 23, 2021 08:03
@brb brb changed the title node-neigh: Bump arping vsn to accept netlink.Link backport v1.7: node-neigh: Bump arping vsn to accept netlink.Link Mar 23, 2021
@brb
Copy link
Member Author

brb commented Mar 23, 2021

test-backport-v1.7

@brb
Copy link
Member Author

brb commented Mar 23, 2021

test-me-please

@brb
Copy link
Member Author

brb commented Mar 23, 2021

test-backport-1.7

@joestringer
Copy link
Member

Hit known flake #10678 on travis run, can be ignored.

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Is this a backport of another existing PR in the main branches? How do we feel those fixes have gone so far?

I understand that the core change here is switching from the net package interface dumping over to the netlink package, which is known to address issues with the execution blocking. I'm fine with that.

The changes directly in Cilium LGTM here. I don't have context on the arping vendor changes so if we want closer review there then I suggest we figure out who's going to take that closer look.

@brb
Copy link
Member Author

brb commented Mar 24, 2021

Is this a backport of another existing PR in the main branches? How do we feel those fixes have gone so far?

Noup, there was no fix for the main branch, as we switched the arping library and the new library consumes netlink.Link instead of net.Interface. However, we had to backport almost the same fix to the 1.8 (#15225) and to the 1.9 (#15227).

The changes directly in Cilium LGTM here. I don't have context on the arping vendor changes so if we want closer review there then I suggest we figure out who's going to take that closer look.

We could pull @jrfastab to do the review. He should know the code very well.

@tgraf
Copy link
Member

tgraf commented Mar 25, 2021

test-backport-1.7

@tgraf
Copy link
Member

tgraf commented Mar 25, 2021

test-me-please

@joestringer
Copy link
Member

test-me-please

^ This is why all the failed builds show up in the checks section at the bottom, we need to use test-backport-1.7 to run CI here

@tgraf tgraf merged commit ded07c2 into v1.7 Mar 25, 2021
@tgraf tgraf deleted the pr/brb/1.7-arping-netlink branch March 25, 2021 22:11
@joestringer joestringer added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/backports This PR provides functionality previously merged into master. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants