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

ipcache: Deprecate old API #27576

Merged
merged 1 commit into from Aug 31, 2023
Merged

Conversation

joestringer
Copy link
Member

@joestringer joestringer commented Aug 17, 2023

The following commits have introduced a new API for interacting with the
ipcache, which is now preferred for all future interactions:

  • Commit e637814 ("ipcache: Introduce new asynchronous API")
  • Commit b6cef92 ("ipcache: Add new {Upsert,Remove}Prefixes() APIs")
  • Commit f9173b7 ("ipcache: Add ability to override identity via UpsertIdentity")

Mark the older APIs as deprecated in order to help to slowly phase out
future usage of those APIs. For more details, see GitHub issue #21142.

Note that there are still various paths using the older APIs to insert
ipcache entries. If an entry is inserted using the older API, then the
corresponding delete event should likewise use the same older API to
delete the entry. Future users should both insert and delete using the
newer API. Do not mix and match the upserts and deletes from deprecated
APIs and non-deprecated APIs for the same event type / resource update
into ipcache.

PR #27327 recently fixed bugs in
the case of fixed APIs and added unit tests for those cases, but still
it's better for all future users to focus on using the newer APIs for
better internal feature compatibility rather than mixing and matching.

Suggested-by: Aditi Ghag aditi@cilium.io
Signed-off-by: Joe Stringer joe@cilium.io

@joestringer joestringer requested a review from a team as a code owner August 17, 2023 17:35
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label Aug 17, 2023
@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. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Aug 17, 2023
pkg/ipcache/cidr.go Outdated Show resolved Hide resolved
pkg/ipcache/cidr.go Show resolved Hide resolved
@christarazi christarazi added kind/cleanup This includes no functional changes. area/daemon Impacts operation of the Cilium daemon. labels Aug 17, 2023
Copy link
Member

@aditighag aditighag left a comment

Choose a reason for hiding this comment

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

Thanks for following up on this.

Future users should both insert and delete using the
newer API. Do not mix and match the upserts and deletes from deprecated
APIs and non-deprecated APIs for the same event type / resource update
into ipcache.

Sorry, I lost track of the previous PR where we had a brief discussion around testing. 😓 But this mix and match can be easily unit tested such that future usages can be caught via tests? At a high level, just an example - Calling ReleaseCIDRIdentitiesByCIDR and UpsertLabels and so on... If you've already added such unit tests, please ignore my comment (although it doesn't hurt to link them in the PR description. :)).

FWIW, there seems to be a Go convention to deprecate an API - https://github.com/golang/go/wiki/Deprecated. The deprecated APIs need to have a line with "Deprecated: ...", as opposed to DEPRECATED.

pkg/ipcache/ipcache.go Outdated Show resolved Hide resolved
@joestringer joestringer marked this pull request as draft August 24, 2023 05:22
@joestringer joestringer force-pushed the pr/joe/deprecate-old-ipcache-apis branch from 37af3ed to 070efaf Compare August 29, 2023 23:09
The following commits have introduced a new API for interacting with the
ipcache, which is now preferred for all future interactions:

- Commit e637814 ("ipcache: Introduce new asynchronous API")
- Commit b6cef92 ("ipcache: Add new {Upsert,Remove}Prefixes() APIs")
- Commit f9173b7 ("ipcache: Add ability to override identity via UpsertIdentity")

Mark the older APIs as deprecated in order to help to slowly phase out
future usage of those APIs. For more details, see GitHub issue #21142
("Rework ipcache to handle metadata from multiple sources via a
dedicated worker goroutine").

Note that there are still various paths using the older APIs to insert
ipcache entries. If an entry is inserted using the older API, then the
corresponding delete event should likewise use the same older API to
delete the entry. Future users should both insert and delete using the
newer API. Do not mix and match the upserts and deletes from deprecated
APIs and non-deprecated APIs for the same event type / resource update
into ipcache.

Suggested-by: Aditi Ghag <aditi@cilium.io>
Signed-off-by: Joe Stringer <joe@cilium.io>
@joestringer joestringer force-pushed the pr/joe/deprecate-old-ipcache-apis branch from 070efaf to e218707 Compare August 29, 2023 23:14
@joestringer joestringer removed the request for review from jrajahalme August 29, 2023 23:14
@joestringer joestringer marked this pull request as ready for review August 29, 2023 23:15
@joestringer
Copy link
Member Author

/test

@aditighag aditighag merged commit d2900b9 into main Aug 31, 2023
202 checks passed
@aditighag aditighag deleted the pr/joe/deprecate-old-ipcache-apis branch August 31, 2023 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/daemon Impacts operation of the Cilium daemon. kind/cleanup This includes no functional changes. 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

3 participants