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

Avoid clearing objects in CiliumEndpoint conversion funcs #24928

Merged
merged 1 commit into from
Apr 17, 2023

Conversation

aanm
Copy link
Member

@aanm aanm commented Apr 17, 2023

This removes the behavior of mutating the objects received from the client-go library. To begin with there isn't really any benefit from doing so, given we don't store the object afterwards, and it will be ready for gc when it leaves the scope inside client-go. client-go can possibly return the same pointer twice here, to trigger eg. both an object update delta and then a DeletedFinalStateUnknown delta with the same pointer.

For more info, see the issue 115658 in the kubernetes/kubernetes repo on github.

Follow-up of: 74307f1 ("Avoid clearing objects in conversion funcs")

This removes the behavior of mutating the objects received from the
client-go library. To begin with there isn't really any benefit from
doing so, given we don't store the object afterwards, and it will be
ready for gc when it leaves the scope inside client-go. client-go can
possibly return the same pointer twice here, to trigger eg. both an
object update delta and then a DeletedFinalStateUnknown delta with the
same pointer.

For more info, see the issue 115658 in the kubernetes/kubernetes repo on
github.

Follow-up of: 74307f1 ("Avoid clearing objects in conversion funcs")

Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm added kind/enhancement This would improve or streamline existing functionality. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/misc This PR makes changes that have no direct user impact. needs-backport/1.11 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Apr 17, 2023
@aanm aanm requested a review from a team as a code owner April 17, 2023 16:47
@aanm aanm requested a review from squeed April 17, 2023 16:47
@aanm
Copy link
Member Author

aanm commented Apr 17, 2023

/test

Copy link
Member

@odinuge odinuge left a comment

Choose a reason for hiding this comment

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

Thanks, lgtm!

@joestringer joestringer changed the title Avoid clearing objects in conversion funcs Avoid clearing objects in CiliumEndpoint conversion funcs Apr 17, 2023
@aanm aanm merged commit 0d7406a into cilium:master Apr 17, 2023
43 checks passed
@aanm aanm deleted the pr/follow-up-commit branch April 17, 2023 20:58
@aanm aanm added backport-pending/1.12 backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.11 needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Apr 17, 2023
@aanm aanm added the backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. label Apr 17, 2023
@julianwiedmann
Copy link
Member

Also landed in v1.11.

@julianwiedmann julianwiedmann added backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. and removed backport-pending/1.11 labels May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/enhancement This would improve or streamline existing functionality. release-note/misc This PR makes changes that have no direct user impact. sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers.
Projects
No open projects
Status: Released
Development

Successfully merging this pull request may close these issues.

None yet

4 participants