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

Jump to bottom

bpf: Remove bpf_netdev.o from previously used devices #10087

Merged
merged 2 commits into from Feb 11, 2020
Merged

bpf: Remove bpf_netdev.o from previously used devices #10087

merged 2 commits into from Feb 11, 2020

Conversation

brb
Copy link
Member

@brb brb commented Feb 7, 2020 •
edited by tgraf

This commit makes cilium-agent to remove bpf_netdev.o from devices which no longer suppose to have the program attached. This can happen when e.g. a user has specified a different device for NodePort via --device or they switched from the direct routing mode to the tunnel mode.

This change is Reviewable

@brb brb added pending-review area/daemon Impacts operation of the Cilium daemon. labels Feb 7, 2020
@brb brb requested review from borkmann and a team February 7, 2020 11:18
@brb brb requested a review from a team as a code owner February 7, 2020 11:18
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

2 similar comments
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 7, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.7.0-rc4 Feb 7, 2020
@brb brb added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Feb 7, 2020
@brb
Copy link
Member Author

brb commented Feb 7, 2020

test-me-please

@coveralls
Copy link

coveralls commented Feb 7, 2020 •
edited

Coverage Status

Coverage decreased (-0.02%) to 44.582% when pulling f3d4866 on pr/brb/cleanup-bpf-netdev-progs into 022673f on master.

@brb brb added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 7, 2020
borkmann
borkmann reviewed Feb 7, 2020
View reviewed changes
bpf/init.sh Outdated Show resolved Hide resolved
@brb brb force-pushed the pr/brb/cleanup-bpf-netdev-progs branch from bed2b6c to 17e9f36 Compare February 10, 2020 09:24
@brb brb requested a review from borkmann February 10, 2020 09:24
@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

@brb brb force-pushed the pr/brb/cleanup-bpf-netdev-progs branch 2 times, most recently from d135600 to 6406ae2 Compare February 10, 2020 09:30
@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

@brb
Copy link
Member Author

brb commented Feb 10, 2020

CI hit the RuntimeCassandra Tests policy allowing all actions flake. Re-running.

@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

@brb brb force-pushed the pr/brb/cleanup-bpf-netdev-progs branch from 6406ae2 to 970e325 Compare February 10, 2020 14:03
@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

@brb brb mentioned this pull request Feb 10, 2020
Merged
3 tasks
@brb brb force-pushed the pr/brb/cleanup-bpf-netdev-progs branch from 970e325 to ff5499d Compare February 10, 2020 16:00
@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

@brb
Copy link
Member Author

brb commented Feb 10, 2020

CI provisioning failed. Re-running.

@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

1 similar comment
@brb
Copy link
Member Author

brb commented Feb 10, 2020

test-me-please

joestringer
joestringer approved these changes Feb 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.

Any chance we can work towards unit testing of this kind of functionality? I guess for safety it'd need to run in its own namespace so not super straightforward.. but it'd be nice to be able to validate the corner cases of this.

Maybe this is an item for moving bpf/init.sh into Golang..

@brb brb force-pushed the pr/brb/cleanup-bpf-netdev-progs branch from ff5499d to f3d4866 Compare February 11, 2020 06:52
@brb
Copy link
Member Author

brb commented Feb 11, 2020

test-me-please

@brb
bpf: Remove bpf_netdev.o from previously used devices
c1ceae2 
This commit makes cilium-agent to remove bpf_netdev.o from devices which
no longer suppose to have the program attached. This can happen when
e.g. a user has specified a different device for NodePort via `--device`
or they switched from the direct routing mode to the tunnel mode.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Do not remove tc filter from native dev
f3d4866 
The programs will be removed by cilium-agent during its bootstrap.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@borkmann borkmann merged commit 7fc73b8 into master Feb 11, 2020
1.8.0 automation moved this from In progress to Merged Feb 11, 2020
@borkmann borkmann deleted the pr/brb/cleanup-bpf-netdev-progs branch February 11, 2020 09:20
@brb
Copy link
Member Author

brb commented Feb 11, 2020

@joestringer I was thinking about restarting cilium-agent with different configuration in CI, and checking whether progs were successfully removed from a prev device (added to my follow-up TODO list).

Having bpf/init.sh in Go would help us to unit test the behavior due to easier mocking in Go.

@raybejjani raybejjani mentioned this pull request Feb 11, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.0-rc4 Feb 11, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.0-rc4 Feb 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer approved these changes

@jrfastab jrfastab Awaiting requested review from jrfastab

Assignees
No one assigned
Labels
area/daemon Impacts operation of the Cilium daemon. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.7.0-rc4
Backport done to v1.7
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@brb @coveralls @borkmann @joestringer @raybejjani @aanm