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.

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

datapath: migrate off j-keck/arping #13112

Merged
merged 1 commit into from
Dec 17, 2020
Merged

datapath: migrate off j-keck/arping #13112

merged 1 commit into from
Dec 17, 2020

Conversation

vladdy
Copy link
Contributor

@vladdy vladdy commented Sep 8, 2020 •
edited

This migrates the code from j-keck/arping to an implementation in pkg/datapath/linux/arp. The main change is the use of github.com/google/gopacket which the project already depends on and utilization of os.NewFile to create a pollable resource and avoid manual dealing with timeouts.

Fixes: #10236

@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 8, 2020
@maintainer-s-little-helper

This comment has been minimized.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 8, 2020
joestringer
joestringer reviewed Sep 10, 2020
View reviewed changes
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.

👋 Thanks for the submission. Just passing by, no deep review yet, I just noticed one thing that will need to be checked as part of your sign-off of the commits.

pkg/datapath/linux/arp/ping.go Show resolved Hide resolved
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Sep 10, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 10, 2020
@stale

This comment has been minimized.

Sign in to view
@stale stale bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 11, 2020
@aanm aanm added the needs/triage This issue requires triaging to establish severity and next steps. label Oct 13, 2020
@stale stale bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 13, 2020
@vladdy
Copy link
Contributor Author

vladdy commented Oct 16, 2020

I need also to check that the new changes in https://github.com/cilium/arping are also incorporated in my work.

@brb
Copy link
Member

brb commented Oct 30, 2020

@vladdy Are you still on it?

@vladdy
Copy link
Contributor Author

vladdy commented Oct 31, 2020

Yep, should have an update soon.

@vladdy vladdy force-pushed the eliminate-arping-dep branch 5 times, most recently from e97b13f to 8611bc5 Compare November 24, 2020 01:54
@vladdy vladdy changed the title Migrate off j-keck/arping datapath: migrate off j-keck/arping Nov 24, 2020
@vladdy vladdy marked this pull request as ready for review November 24, 2020 02:26
@vladdy vladdy requested a review from a team November 24, 2020 02:26
@vladdy vladdy requested a review from a team as a code owner November 24, 2020 02:26
@vladdy vladdy requested a review from jrfastab November 24, 2020 02:26
@vladdy
datapath: migrate off j-keck/arping
b139be2 
This migrates the code from `j-keck/arping` to an implementation
in `pkg/datapath/linux/arp`.

Fixes: cilium#10236

Signed-off-by: Vlad Artamonov <742047+vladdy@users.noreply.github.com>
@vladdy vladdy requested a review from brb December 17, 2020 01:35
@brb
Copy link
Member

brb commented Dec 17, 2020

retest-runtime

@brb
Copy link
Member

brb commented Dec 17, 2020

test-me-please

@brb
Copy link
Member

brb commented Dec 17, 2020

retest-runtime

@brb
Copy link
Member

brb commented Dec 17, 2020

retest-net-next

@brb
Copy link
Member

brb commented Dec 17, 2020

retest-runtime

@brb brb merged commit 858cae1 into cilium:master Dec 17, 2020
@vladdy vladdy mentioned this pull request Dec 18, 2020
Closed
@brb brb mentioned this pull request Dec 18, 2020
Closed
@brb brb mentioned this pull request Mar 22, 2021
Merged
@brb brb mentioned this pull request Apr 28, 2021
Merged
@brb brb mentioned this pull request Apr 28, 2021
Merged
This was referenced May 13, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

@brb brb brb approved these changes

@tklauser tklauser tklauser approved these changes

@joestringer joestringer joestringer approved these changes

@jrfastab jrfastab Awaiting requested review from jrfastab

Assignees

@jrfastab jrfastab

Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cilium, daemon: remove arping lib dependency
8 participants
@vladdy @brb @aanm @joestringer @tklauser @jrfastab @rolinh @qmonnet