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

init.sh,loader: load overlay programs in Go #24876

Merged
merged 2 commits into from Apr 19, 2023
Merged

init.sh,loader: load overlay programs in Go #24876

merged 2 commits into from Apr 19, 2023

Conversation

rgo3
Copy link
Contributor

@rgo3 rgo3 commented Apr 13, 2023 

With this commit we remove code from init.sh that compiles and loads 
programs from bpf_overlay.c. Without it init.sh isn't responsible for 
loading bpf programs anymore, therefore none of the remaining commands 
should depend on ciliums iproute2 fork.

Fixes: #20699

@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 Apr 13, 2023
@rgo3 rgo3 added the release-note/misc This PR makes changes that have no direct user impact. label Apr 13, 2023
@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 Apr 13, 2023
rgo3
rgo3 commented Apr 13, 2023
View reviewed changes
Copy link
Contributor Author

@rgo3 rgo3 left a comment

Choose a reason for hiding this comment

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

Currently I haven't added any tests as this mostly reuses existing infrastructure for loading datapath programs.

pkg/datapath/loader/base.go Outdated Show resolved Hide resolved
@rgo3
Copy link
Contributor Author

rgo3 commented Apr 13, 2023

/test

@pchaigno pchaigno added sig/loader Impacts the loading of BPF programs into the kernel. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Apr 17, 2023
@rgo3
Copy link
Contributor Author

rgo3 commented Apr 17, 2023

/test

ti-mo
ti-mo requested changes Apr 18, 2023
View reviewed changes
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

Thank you! Left a few comments, should be good to go after that.

pkg/datapath/loader/compile.go Outdated Show resolved Hide resolved
bpf/init.sh Show resolved Hide resolved
@rgo3
init.sh,loader: load overlay programs in Go
520555d 
With this commit we remove code from init.sh that compiles and loads
programs from bpf_overlay.c. Without it init.sh isn't responsible for
loading bpf programs anymore, therefore none of the remaining commands
should depend on ciliums iproute2 fork.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@rgo3
init.sh: remove unused arguments
9d0eef4 
Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@rgo3
Copy link
Contributor Author

rgo3 commented Apr 18, 2023

/test

@rgo3 rgo3 marked this pull request as ready for review April 18, 2023 15:29
@rgo3 rgo3 requested a review from a team as a code owner April 18, 2023 15:29
@rgo3 rgo3 requested a review from ti-mo April 18, 2023 15:29
pchaigno
pchaigno approved these changes Apr 18, 2023
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.

LGTM.

bpf/init.sh
MCPU=${20}
NR_CPUS=${21}
# MCPU=${20}
# NR_CPUS=${21}
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we just remove all of these and their corresponding Go code?

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been doing it this way since we still need to backport some (but not all) init.sh changes like the socketlb downgrade code, and we don't want to cause unnecessary churn. For example, removing LIB would cause all of these to be re-numbered, which will cause pain during backports.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 19, 2023
@ti-mo ti-mo merged commit e68592f into cilium:main Apr 19, 2023
57 checks passed
@ti-mo ti-mo deleted the move-overlay-loader branch April 19, 2023 06:46
@julianwiedmann julianwiedmann mentioned this pull request Apr 19, 2023
Merged
@ti-mo ti-mo mentioned this pull request Apr 20, 2023
Open
@joestringer joestringer mentioned this pull request Jun 13, 2023
Merged
@gandro gandro mentioned this pull request Sep 26, 2023
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

@ti-mo ti-mo ti-mo approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. 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. sig/loader Impacts the loading of BPF programs into the kernel.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

init.sh: move tunnel device setup to Go package
3 participants
@rgo3 @pchaigno @ti-mo