-
Notifications
You must be signed in to change notification settings - Fork 41.1k
WIP: client-go: finish context support #129125
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
base: master
Are you sure you want to change the base?
Conversation
Please note that we're already in Test Freeze for the Fast forwards are scheduled to happen every 6 hours, whereas the most recent run was: Mon Dec 9 12:10:29 UTC 2024. |
This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
096f284
to
49e218c
Compare
I wasn't entirely sure whether this should return a value or a pointer to satisfy the interface. Both works, so I benchmarked it elsewhere (REST mapper). Mem allocs are the same (one alloc/call), but returning a value is 10% slower when calling one method. What I then benchmarked is whether pointer vs value receiver in the wrapper makes a difference. Converting from value receiver (what I had before) to pointer receiver reduced call overhead by 6%. That's because with a value receiver, Go has to auto-generate a variant with pointer receiver and calls the value receiver through that. That can be seen in a debugger (call stack) and when setting breakpoints: (dlv) b restMapperWrapper.KindForWithContext Command failed: Location "restMapperWrapper.KindForWithContext" ambiguous: k8s.io/apimachinery/pkg/api/meta.restMapperWrapper.KindForWithContext, k8s.io/apimachinery/pkg/api/meta.(*restMapperWrapper).KindForWithContext… Conventional wisdom is to define types with value receiver because those can be called also on unmutable instances, making them more flexible. But for types which will only ever be used via a pointer, I think pointer receiver is better for the reasons above (small performance difference, easier to debug).
The main purpose is to replace context.TODO with a context provided by the caller. A secondary purpose is to enable contextual logging. Modifying the existing interfaces and APIs would have a big impact on the ecosystem. This is a no-go. Instead, the following approach was taken: - All interfaces get duplicated in a *WithContext variant where the methods also have a *WithContext suffix and the ctx parameter. All methods are treated this way except for obvious local get methods (like RESTClient) because it cannot be ruled out entirely that some implementation may need a context. - Implementations of these interfaces implement both method variants which is possible because the method names are different. The old methods are implemented as thin wrappers around the updated code which is now the body of the new methods or shared helpers. In some cases there is additional overhead (type checks, potentially additional allocations) when using the old methods. - To*WithContext helpers bridge from the old to the new interfaces. They try a type cast first. Because the in-tree implementations implement both, they can be used directly. For other implementations wrappers are used. - All old APIs and interfaces are marked as deprecated. There is no intent to ever remove them, but consumers should be made aware that there are now better alternatives. Implementations also get marked this way even if nothing ever calls them directly because it shows which code, at least theoretically, could get removed. - Existing unit tests do not get updated to the new APIs. This gives us unit test coverage of the old and new API because the old APIs call the new ones. - In-tree consumers will be updated in follow-up PRs. This is likely to be a longer process. Because of the deprecation comment, `hack/golangci-lint.sh -n` can be used to find code which needs to be updated.
The only log output is for error messages which should normally not occur. It's also likely that users expect to see exactly those messages, so it's better to not touch them.
When debugging, it helps to keep output from different connections separate. This can be done with contextual logging and using different loggers for each connection. Cancellation is handled separately for requests. Therefore the new APIs only add support for passing a logger instance.
The API for the package already had a context, so all that was missing was to extract and use the logger from that.
The remaining non-structured, non-contextual log calls hopefully never get reached, so it's not worth converting them.
This changes the log output of kubectl, which affects some test cases which check for GET results in that log output.
9f7af59
to
4f211d5
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: pohly The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@pohly: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The main purpose is to replace context.TODO with a context provided by the caller. A secondary purpose is to enable contextual logging.
Special notes for your reviewer:
See #129109 for an introduction of the approach taken most of the time when adding alternative context-aware APIs. See commit messages for additional information.
Most likely this will be merged by creating smaller PRs.
Does this PR introduce a user-facing change?