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

nodeid map: provide map via hive cell #25574

Merged
merged 1 commit into from May 31, 2023

Conversation

mhofstetter
Copy link
Member

This commit introduces a hive cell for the nodeid map.

The datapath cell has been refactored to depend on the nodeid map - which introduces explicit dependencies with a proper init order.

This way, the map is properly initialized when accessing it for nodeid restoration. Currently this is only working due to the fact, that the map gets implicitly opened when iterating over its entries.

Follow up of #25497

@mhofstetter mhofstetter added release-note/misc This PR makes changes that have no direct user impact. area/modularization labels May 22, 2023
@mhofstetter
Copy link
Member Author

/test

@mhofstetter
Copy link
Member Author

mhofstetter commented May 22, 2023

/test -> 🟢

@mhofstetter mhofstetter marked this pull request as ready for review May 22, 2023 14:40
@mhofstetter mhofstetter requested review from a team as code owners May 22, 2023 14:40
Copy link
Member

@jschwinger233 jschwinger233 left a comment

Choose a reason for hiding this comment

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

Thanks

@mhofstetter mhofstetter added the kind/enhancement This would improve or streamline existing functionality. label May 23, 2023
@mhofstetter
Copy link
Member Author

rebased to main and resolved conflicts with signalmap registration

@nathanjsweet nathanjsweet added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 24, 2023
@mhofstetter mhofstetter removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 25, 2023
@mhofstetter
Copy link
Member Author

rebased to main and resolved conflicts

@jrajahalme & @nathanjsweet: ready for review

@mhofstetter
Copy link
Member Author

next rebase to main to resolve conflicts 🙏

})
return nodeMap
type nodeMap struct {
bpfMap *ebpf.Map
Copy link
Member

Choose a reason for hiding this comment

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

nit: This could have renamed unnamed as well? Or is there value in having to call bpf Map functions explicitly?

Copy link
Member Author

Choose a reason for hiding this comment

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

i kind of prefer having the inner maps functions not exposed. kind of wrapping the functionality and not extending it as it would be when having it as anonymous field.

edit: and it kind of got implemented this way for all the other BPF maps which are provided via hive cell too

@jrajahalme
Copy link
Member

/test

This commit introduces a hive cell for the nodeid map.

The datapath cell has been refactored to depend on the nodeid map -
which introduces proper init order before accessing the map during
nodeid restoration (currently only working due to the fact, that the map
gets implicitly opened when iterating over its entries).

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter
Copy link
Member Author

rebased to main and resolved conflicts.

@nathanjsweet : ready for review

@mhofstetter
Copy link
Member Author

/test

@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 May 31, 2023
@julianwiedmann julianwiedmann merged commit ef26119 into cilium:main May 31, 2023
62 checks passed
@mhofstetter mhofstetter deleted the pr/mhofstetter/nodemap-cell branch May 31, 2023 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/modularization kind/enhancement This would improve or streamline existing functionality. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants