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

Remove sockops from eBPF datapath diagrams #24824

Merged
merged 3 commits into from May 2, 2023

Conversation

zacharysarah
Copy link
Contributor

@zacharysarah zacharysarah commented Apr 11, 2023

Fixes: #24735

docs: Remove sockops, sockmaps from eBPF datapath diagrams

@zacharysarah zacharysarah added area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/sockops labels Apr 11, 2023
@maintainer-s-little-helper
Copy link

Commit 55bae9865ad2c1bd5bc4cab2ff317138d9aac0f5 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. labels Apr 11, 2023
@maintainer-s-little-helper
Copy link

Commit 55bae9865ad2c1bd5bc4cab2ff317138d9aac0f5 does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@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 Apr 11, 2023
@zacharysarah zacharysarah added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 12, 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 12, 2023
@zacharysarah zacharysarah marked this pull request as ready for review April 12, 2023 00:15
@zacharysarah zacharysarah requested review from a team as code owners April 12, 2023 00:15
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks a lot! I have three comments:

  • Sockmaps are gone as well, along with sockops. File bpf_redir.c was entirely removed in 87c8746. Could you please remove the corresponding boxes as well?

  • The title of some diagrams refers to sockmaps and should also be updated. The version numbers as well, I suppose.

  • Could we maybe adjust the size of the frames that contained the removed boxes on the diagrams (maybe narrow down the entire diagram by also moving the keys when relevant)? The resulting diagrams currently have frames with some empty spaces, just as if something in it was missing. This might be easier with an SVG editor rather than a text editor.

@zacharysarah
Copy link
Contributor Author

@qmonnet 👋🏻

Sockmaps are gone as well, along with sockops. File bpf_redir.c was entirely removed in 87c8746. Could you please remove the corresponding boxes as well?

👍🏻

The title of some diagrams refers to sockmaps and should also be updated. The version numbers as well, I suppose.

👍🏻

Could we maybe adjust the size of the frames that contained the removed boxes on the diagrams

Exactly the point of my reviewer note in the OP. 🎯

This might be easier with an SVG editor rather than a text editor.

I've never actually edited an SVG with an SVG editor before. I welcome your recommendation about which editor you think would work best.

@zacharysarah zacharysarah marked this pull request as ready for review April 27, 2023 05:22
@zacharysarah
Copy link
Contributor Author

@qmonnet 👋🏻 PTAL

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks! The diagrams look better now.

I've got a few comments on the second one (cilium_bpf_endpoint.svg):

  • Should it be renamed v1.13 Datapath instead of v1.x? It's not clear to me why one is 1.x and the others have a fixed version number. Maybe I'm missing some context.

  • Should we remove the bottom part of the diagram? Now that sockops/sockmaps have been removed from it, I'm not sure there's much interest in keeping that part.

Other than these, looks good to me.

I'll add a request review from Joe who has a good vision of the datapath and should be able to tell if we missed something that is outdated?

@zacharysarah
Copy link
Contributor Author

zacharysarah commented Apr 29, 2023

@qmonnet 👋🏻

Should it be renamed v1.13 Datapath instead of v1.x?

I kept 1.x because it's unique in the current version (the others were 1.8). I'm happy to change it.

Should we remove the bottom part of the diagram?

That's my thought as well, but I didn't want to presume. @joestringer WDYT? 🙏🏻

UPDATE: I've made both changes. If you like them, please review accordingly. 🙇🏻‍♀️

Signed-off-by: ZSC <sarah.corleissen@isovalent.com>
@zacharysarah zacharysarah force-pushed the 24735-fix branch 2 times, most recently from a0d3f6b to 8147eff Compare April 30, 2023 22:26
Signed-off-by: ZSC <sarah.corleissen@isovalent.com>
Signed-off-by: ZSC <sarah.corleissen@isovalent.com>
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.

LGTM. I think we could follow up to add the bpf_sock logic for socket-level L4 LoadBalancing, but that can be a separate submission on top IMO.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks all good to me as well, thanks a lot for working on this!

@qmonnet
Copy link
Member

qmonnet commented May 2, 2023

Doc change only, with Documentation workflow passing. We have reviews from all code-owner teams (Anton was pulled for cilium/sig-datapath, but Joe and I are part of that team as well). I'm just changing the release-note label to misc as there's no functionality change, and marking as ready-to-merge.

@qmonnet qmonnet added 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. and removed release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels May 2, 2023
@joestringer joestringer merged commit d8b68ee into cilium:main May 2, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Impacts the documentation, including textual changes, sphinx, or other doc generation code. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

docs: Update diagram to remove mention of sockops
3 participants