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

bpf: datapath: Rewite base devices setup in Go #13915

Merged
merged 2 commits into from
Nov 16, 2020
Merged

bpf: datapath: Rewite base devices setup in Go #13915

merged 2 commits into from
Nov 16, 2020

Conversation

vadorovsky
Copy link
Member

This change removes the part of init.sh responsible for base devices
setup and remplements it in Go.

Ref: #920

Signed-off-by: Michal Rostecki mrostecki@opensuse.org

@vadorovsky vadorovsky requested review from a team and borkmann November 5, 2020 19:21
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 5, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Nov 5, 2020
@vadorovsky
Copy link
Member Author

test-me-please

@vadorovsky vadorovsky mentioned this pull request Nov 5, 2020
Closed
@vadorovsky vadorovsky added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. labels Nov 5, 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 Nov 5, 2020
@pchaigno
Copy link
Member

pchaigno commented Nov 5, 2020

test-me-please

@vadorovsky
Copy link
Member Author

This failure is legit and I'll try to fix it tomorrow:

23:24:37  RuntimePolicies
23:24:37  /home/jenkins/workspace/Cilium-PR-Runtime-4.9/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:478
23:24:37    Init Policy Test
23:24:37    /home/jenkins/workspace/Cilium-PR-Runtime-4.9/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:478
23:24:37      Init Egress Policy Test [AfterEach]
23:24:37      /home/jenkins/workspace/Cilium-PR-Runtime-4.9/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:514
23:24:37  
23:24:37      Found 1 Cilium logs matching list of errors that must be investigated:
[2020-11-05T22:24:37.386Z]     empty argument passed to bpf/init.sh
23:24:37  
23:24:37      /home/jenkins/workspace/Cilium-PR-Runtime-4.9/runtime-gopath/src/github.com/cilium/cilium/test/ginkgo-ext/scopes.go:421

But some others are failing with:

22:38:08  Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

@vadorovsky
Copy link
Member Author

test-me-please

@vadorovsky
Copy link
Member Author

retest-4.19

@vadorovsky
Copy link
Member Author

test-gke

aanm
aanm approved these changes Nov 9, 2020
View reviewed changes
Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Only small nits, overall LGTM

pkg/datapath/loader/netlink.go Show resolved Hide resolved
pkg/datapath/loader/netlink.go Show resolved Hide resolved
pkg/datapath/loader/netlink.go Outdated Show resolved Hide resolved
@aanm aanm added dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs and removed dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs labels Nov 9, 2020
@vadorovsky
sysctl: Add ApplySettings function
a272c10 
The new function writes sysctl settings provided as a slice.

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky
Copy link
Member Author

test-me-please

pchaigno
pchaigno approved these changes Nov 11, 2020
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Looks good to me. One minor nit below.

Thanks for tackling this @mrostecki! 🎉

pkg/datapath/loader/netlink.go Outdated Show resolved Hide resolved
pkg/datapath/loader/netlink.go Outdated Show resolved Hide resolved
@vadorovsky
bpf: datapath: Rewite base devices setup in Go
f5bbdad 
This change removes the part of init.sh responsible for base devices
setup and remplements it in Go.

Ref: #920

Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
@vadorovsky
Copy link
Member Author

test-me-please

@nathanjsweet nathanjsweet merged commit 9e48678 into master Nov 16, 2020
@nathanjsweet nathanjsweet deleted the pr/mrostecki/init-golang-1 branch November 16, 2020 17:19
@brb
Copy link
Member

brb commented Nov 20, 2020

@mrostecki Just echoing #14043 (comment). Have you validated ipvlan setup?

@vadorovsky
Copy link
Member Author

vadorovsky commented Nov 20, 2020 •
edited

@brb I didn't, I also was wrongly assuming that ipvlan is covered by currently existing tests. :/ I will do that later today or on Monday.

@vadorovsky
Copy link
Member Author

ipvlan doesn't work, I'm working on the fix. Sorry for that!

@vadorovsky vadorovsky mentioned this pull request Nov 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@aanm aanm aanm approved these changes

@borkmann borkmann Awaiting requested review from borkmann

Assignees

@borkmann borkmann

Labels
kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@vadorovsky @pchaigno @brb @aanm @nathanjsweet @borkmann