Skip to content

use atomic.Pointer instead of bare LoadPointer #23971

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
Mar 7, 2023
Merged

use atomic.Pointer instead of bare LoadPointer #23971

merged 1 commit into from
Mar 7, 2023

Conversation

lmb
Copy link
Contributor

@lmb lmb commented Feb 23, 2023

There are a couple of uses of LoadPointer in the code base, which lead to a proliferation of unsafe.Pointer. Since Go 1.19 we can use atomic.Pointer instead to get a type safe wrapper.

@lmb lmb added the release-note/misc This PR makes changes that have no direct user impact. label Feb 23, 2023
@lmb lmb force-pushed the use-atomic-pointer branch 2 times, most recently from cc33f04 to abe783b Compare February 23, 2023 13:46
@lmb
Copy link
Contributor Author

lmb commented Feb 23, 2023

/test

@lmb lmb force-pushed the use-atomic-pointer branch from abe783b to 2802fee Compare February 27, 2023 09:48
@lmb
Copy link
Contributor Author

lmb commented Feb 27, 2023

gateway-api-conformance-test fails due to #23999.

@lmb
Copy link
Contributor Author

lmb commented Feb 27, 2023

/test

@lmb
Copy link
Contributor Author

lmb commented Feb 27, 2023

Jenkins failures are due to #24045

@lmb lmb marked this pull request as ready for review February 27, 2023 13:13
@lmb lmb requested review from a team as code owners February 27, 2023 13:13
@lmb lmb requested review from jrajahalme and pippolo84 February 27, 2023 13:13
@lmb
Copy link
Contributor Author

lmb commented Feb 27, 2023

/test

pippolo84
pippolo84 approved these changes Feb 28, 2023
View reviewed changes
Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM 💯

@lmb
Copy link
Contributor Author

lmb commented Feb 28, 2023

ci-multicluster failed due to #23371

@lmb
Copy link
Contributor Author

lmb commented Feb 28, 2023

/test-ci-verifier

@lmb
Copy link
Contributor Author

lmb commented Feb 28, 2023

ci-multicluster hit cilium/cilium-cli#1260 from what I can tell.

@lmb
Copy link
Contributor Author

lmb commented Mar 1, 2023

@jrajahalme can you take a look please?

jrajahalme
jrajahalme approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@jrajahalme jrajahalme 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 the clean-up :-)

@lmb
use atomic.Pointer instead of bare LoadPointer
9397655 
There are a couple of uses of LoadPointer in the code base, which
lead to a proliferation of unsafe.Pointer. Since Go 1.19 we can
use atomic.Pointer instead to get a type safe wrapper.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>
@lmb lmb force-pushed the use-atomic-pointer branch from 2802fee to 9397655 Compare March 7, 2023 14:15
@lmb
Copy link
Contributor Author

lmb commented Mar 7, 2023 •
edited by maintainer-s-little-helper bot
Loading

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.19' failed:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Unable to download helm chart v1.12 from GitHub

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.19 so I can create one.

@lmb
Copy link
Contributor Author

lmb commented Mar 7, 2023

Seems like test-1.16-4.19 had wget killed due to a signal: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/365/testReport/junit/Suite-k8s-1/16/K8sUpdates_Tests_upgrade_and_downgrade_from_a_Cilium_stable_image_to_master/

@lmb
Copy link
Contributor Author

lmb commented Mar 7, 2023

/test-1.16-4.19

@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 Mar 7, 2023
@borkmann borkmann merged commit 80e8a9b into cilium:master Mar 7, 2023
@lmb lmb deleted the use-atomic-pointer branch March 8, 2023 09:02
@nbusseneau nbusseneau mentioned this pull request Dec 5, 2023
Merged
8 tasks
antonipp pushed a commit to DataDog/cilium that referenced this pull request Dec 7, 2023
@learnitall @antonipp
log: Do not export InitializeDefaultLogger function
ffb16ef 
[ upstream commit 00ab252 ]

[ Backporter's notes: ignoring changes from cilium#26327 and cilium#23971 ]

The DefaultLogger variable in the logging module serves as a parent logger
which all other loggers can be derived from. It is initialized using the
InitializeDefaultLogger function and then adjusted on startup based on
user configuration. Users should not call InitializeDefaultLogger to
create a parent logger for their logger, since the logger returned by
InitializeDefaultLogger will always use the hardcoded defaults. For
example, the logger returned will always be of level INFO, even if a user
has enabled debug logging. To make this clear, this commit renames
InitializeDefaultLogger to initializeDefaultLogger to signal that it should
not be used outside of the logging module.

Fixes: cilium#29215

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
antonipp pushed a commit to DataDog/cilium that referenced this pull request Dec 7, 2023
@learnitall @antonipp
log: Do not export InitializeDefaultLogger function
12b482e 
[ upstream commit 00ab252 ]

[ Backporter's notes: ignoring changes from cilium#26327 and cilium#23971 ]

The DefaultLogger variable in the logging module serves as a parent logger
which all other loggers can be derived from. It is initialized using the
InitializeDefaultLogger function and then adjusted on startup based on
user configuration. Users should not call InitializeDefaultLogger to
create a parent logger for their logger, since the logger returned by
InitializeDefaultLogger will always use the hardcoded defaults. For
example, the logger returned will always be of level INFO, even if a user
has enabled debug logging. To make this clear, this commit renames
InitializeDefaultLogger to initializeDefaultLogger to signal that it should
not be used outside of the logging module.

Fixes: cilium#29215

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
@antonipp antonipp mentioned this pull request Dec 7, 2023
Merged
antonipp pushed a commit to DataDog/cilium that referenced this pull request Dec 7, 2023
@learnitall @antonipp
log: Do not export InitializeDefaultLogger function
d05348f 
[ upstream commit 00ab252 ]

[ Backporter's notes: ignoring changes from cilium#26327 and cilium#23971 ]

The DefaultLogger variable in the logging module serves as a parent logger
which all other loggers can be derived from. It is initialized using the
InitializeDefaultLogger function and then adjusted on startup based on
user configuration. Users should not call InitializeDefaultLogger to
create a parent logger for their logger, since the logger returned by
InitializeDefaultLogger will always use the hardcoded defaults. For
example, the logger returned will always be of level INFO, even if a user
has enabled debug logging. To make this clear, this commit renames
InitializeDefaultLogger to initializeDefaultLogger to signal that it should
not be used outside of the logging module.

Fixes: cilium#29215

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
tklauser pushed a commit that referenced this pull request Dec 8, 2023
@learnitall @tklauser
log: Do not export InitializeDefaultLogger function
739e9de 
[ upstream commit 00ab252 ]

[ Backporter's notes: ignoring changes from #26327 and #23971 ]

The DefaultLogger variable in the logging module serves as a parent logger
which all other loggers can be derived from. It is initialized using the
InitializeDefaultLogger function and then adjusted on startup based on
user configuration. Users should not call InitializeDefaultLogger to
create a parent logger for their logger, since the logger returned by
InitializeDefaultLogger will always use the hardcoded defaults. For
example, the logger returned will always be of level INFO, even if a user
has enabled debug logging. To make this clear, this commit renames
InitializeDefaultLogger to initializeDefaultLogger to signal that it should
not be used outside of the logging module.

Fixes: #29215

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
antonipp pushed a commit to DataDog/cilium that referenced this pull request Dec 8, 2023
@learnitall @antonipp
log: Do not export InitializeDefaultLogger function
a65bfca 
[ upstream commit 00ab252 ]

[ Backporter's notes: ignoring changes from cilium#26327 and cilium#23971 ]

The DefaultLogger variable in the logging module serves as a parent logger
which all other loggers can be derived from. It is initialized using the
InitializeDefaultLogger function and then adjusted on startup based on
user configuration. Users should not call InitializeDefaultLogger to
create a parent logger for their logger, since the logger returned by
InitializeDefaultLogger will always use the hardcoded defaults. For
example, the logger returned will always be of level INFO, even if a user
has enabled debug logging. To make this clear, this commit renames
InitializeDefaultLogger to initializeDefaultLogger to signal that it should
not be used outside of the logging module.

Fixes: cilium#29215

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
pchaigno pushed a commit that referenced this pull request Jan 8, 2024
@learnitall @giorio94
log: Do not export InitializeDefaultLogger function
1995709 
[ upstream commit 00ab252 ]

[ Backporter's notes: ignoring changes from #26327 and #23971 ]

The DefaultLogger variable in the logging module serves as a parent logger
which all other loggers can be derived from. It is initialized using the
InitializeDefaultLogger function and then adjusted on startup based on
user configuration. Users should not call InitializeDefaultLogger to
create a parent logger for their logger, since the logger returned by
InitializeDefaultLogger will always use the hardcoded defaults. For
example, the logger returned will always be of level INFO, even if a user
has enabled debug logging. To make this clear, this commit renames
InitializeDefaultLogger to initializeDefaultLogger to signal that it should
not be used outside of the logging module.

Fixes: #29215

Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
Signed-off-by: Marco Iorio <marco.iorio@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pippolo84 pippolo84 pippolo84 approved these changes

@jrajahalme jrajahalme jrajahalme approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lmb @pippolo84 @jrajahalme @borkmann