Skip to content

pkg/maps: migrate to slog #38373

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
Merged
merged 1 commit into from
May 6, 2025
Merged

pkg/maps: migrate to slog #38373

merged 1 commit into from
May 6, 2025

Conversation

aanm
Copy link
Member

@aanm aanm commented Mar 20, 2025

Migrate this package to use slog instead of logrus.

@aanm aanm added area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. release-note/misc This PR makes changes that have no direct user impact. feature/slog labels Mar 20, 2025
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Mar 20, 2025
@aanm aanm force-pushed the pr/move-maps-to-slog branch from e43a847 to 70795ea Compare March 20, 2025 15:53
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@aanm aanm force-pushed the pr/move-maps-to-slog branch from 70795ea to 4505cb7 Compare March 20, 2025 15:54
@maintainer-s-little-helper

This comment was marked as resolved.

Sign in to view
@aanm aanm force-pushed the pr/move-maps-to-slog branch from 4505cb7 to 7ef7878 Compare March 20, 2025 15:55
@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 Mar 20, 2025
@aanm
Copy link
Member Author

aanm commented Mar 20, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 7ef7878 to cc582de Compare March 20, 2025 18:58
@aanm
Copy link
Member Author

aanm commented Mar 20, 2025

/test

@aanm aanm marked this pull request as ready for review March 20, 2025 20:28
@aanm aanm requested review from a team as code owners March 20, 2025 20:28
@aanm aanm enabled auto-merge March 20, 2025 20:29
asauber
asauber reviewed Mar 21, 2025
View reviewed changes
@asauber asauber self-requested a review March 21, 2025 16:06
ti-mo
ti-mo requested changes Mar 26, 2025
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.

Oof, I'm not a fan of adding a Logger to bpf.Map and ebpf.Map. As far as I can see, they're only used to log the map removal warnings in OpenOrCreate, which is behaviour we want/need to get rid of anyway.

Can we keep using the default logger for those? Passing a logger to all functions returning a map really causes a ton of churn.

@aanm
Copy link
Member Author

aanm commented Mar 26, 2025

Oof, I'm not a fan of adding a Logger to bpf.Map and ebpf.Map. As far as I can see, they're only used to log the map removal warnings in OpenOrCreate, which is behaviour we want/need to get rid of anyway.

Can we keep using the default logger for those? Passing a logger to all functions returning a map really causes a ton of churn.

I understand but we can't keep the default logrus if we want to migrate to slog and keep consistency across all pkgs. Unfortunately, the way that slog is designed it can't be configured on init() functions for our use cases and it needs to be passed down to all functions. In our case, it is configured in hive cells. I would suggest to move forward as it is and once we get rid of the warnings we remove the slog from the function arguments otherwise we risk that these packages will still have the warnings + logrus at the end of this development cycle.

@aanm aanm requested a review from ti-mo March 26, 2025 15:19
@aanm aanm force-pushed the pr/move-maps-to-slog branch from febb39c to 6f136b0 Compare April 4, 2025 05:52
@aanm
Copy link
Member Author

aanm commented Apr 4, 2025

/test

@aanm aanm requested a review from Copilot April 4, 2025 07:08
Copilot
Copilot AI reviewed Apr 4, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 58 out of 58 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (4)

pkg/loadbalancer/experimental/bpf_reconciler_test.go:972

  • Verify that the 'log' variable being assigned to BPFLBMaps.Log is of type *slog.Logger to ensure the migration is consistent across the code.
Log:       log,

pkg/ebpf/map.go:60

  • Ensure that all call sites are updated to pass a valid slog.Logger when invoking NewMap, LoadRegisterMap, LoadPinnedMap, and MapFromID to prevent runtime logging issues.
func NewMap(logger *slog.Logger, spec *MapSpec) *Map {

pkg/ebpf/ebpf.go:1

  • Confirm that the removal of pkg/ebpf/ebpf.go is intentional and that any dependent imports or references in the codebase have been updated accordingly.
// file removed

pkg/bpf/map_linux.go:194

  • [nitpick] Consider refactoring the logger injection approach to eliminate the TODO comment and implement a more consistent dependency injection pattern for the logger.
// TODO inject the logger in a better way than this.

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 6f136b0 to 565cbc9 Compare April 22, 2025 14:39
@aanm
Copy link
Member Author

aanm commented Apr 22, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 565cbc9 to 63c1e8a Compare April 22, 2025 14:55
@aanm
Copy link
Member Author

aanm commented Apr 22, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 63c1e8a to 73a46cc Compare April 23, 2025 07:42
@aanm
Copy link
Member Author

aanm commented Apr 23, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 73a46cc to bb6c98b Compare April 23, 2025 07:59
@aanm
Copy link
Member Author

aanm commented Apr 23, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from bb6c98b to 51d2de5 Compare April 25, 2025 07:33
@aanm
Copy link
Member Author

aanm commented Apr 25, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 51d2de5 to 8a75f02 Compare April 29, 2025 15:19
@aanm
Copy link
Member Author

aanm commented Apr 29, 2025

/test

@aanm
Copy link
Member Author

aanm commented Apr 30, 2025

@pchaigno @aditighag can you PTAL?

@aanm aanm force-pushed the pr/move-maps-to-slog branch from 8a75f02 to e61fbd0 Compare May 5, 2025 07:24
@aanm
Copy link
Member Author

aanm commented May 5, 2025

/test

@aanm aanm force-pushed the pr/move-maps-to-slog branch from e61fbd0 to 5cc5a83 Compare May 5, 2025 07:26
@aanm
Copy link
Member Author

aanm commented May 5, 2025

/test

@aanm
pkg/maps: migrate to slog
c83bf2a 
Migrate this package to use slog instead of logrus.

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/move-maps-to-slog branch from 5cc5a83 to c83bf2a Compare May 6, 2025 07:39
@aanm
Copy link
Member Author

aanm commented May 6, 2025

/test

@aanm aanm added this pull request to the merge queue May 6, 2025
Merged via the queue into main with commit 4ce357a May 6, 2025
348 of 351 checks passed
@aanm aanm deleted the pr/move-maps-to-slog branch May 6, 2025 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot code review Copilot Copilot left review comments

@borkmann borkmann borkmann approved these changes

@pchaigno pchaigno pchaigno approved these changes

@asauber asauber asauber approved these changes

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

@aditighag aditighag Awaiting requested review from aditighag aditighag is a code owner automatically assigned from cilium/sig-lb

Assignees
No one assigned
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. feature/slog release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@aanm @borkmann @pchaigno @asauber @ti-mo