client-go/rest: finish context support #127709
Conversation
k8s-ci-robot
added
release-note
|
/test pull-kubernetes-apidiff Do we care enough about "controller-runtime continues to build" that we should try that in the We could do similar testing with other downstream consumers, but breaking controller-runtime is probably worse because it affects all controllers built on top of it. /cc @sbueringer |
I think it could be an interesting data point (cc @alvaroaleman) |
pohly
force-pushed
the
log-client-go-rest
branch
from
286dbad to
c7e4509
Compare
k8s-ci-robot
added
size/XL
|
/test |
|
@aojea: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
There are a lot of controllers that build on top of client-go, don't know exactly the population but is not negligible /test pull-kubernetes-apidiff |
| // WarningLogger is an implementation of WarningHandler that logs code 299 warnings | ||
| type WarningLogger struct{} | ||
|
|
||
| func (WarningLogger) HandleWarningHeader(code int, agent string, message string) { |
There was a problem hiding this comment.
Yes, the old HandleWarningHeader can be kept even though it's not used in-tree anymore. Let me change that.
There was a problem hiding this comment.
There also was no reason to add the separate types for the WithContext methods. The same type can provide the old and new API.
There was a problem hiding this comment.
Pushed, let's see...
/test pull-kubernetes-apidiff
There was a problem hiding this comment.
Looks good, only false positives about unrelated types (an issue with apidiff's handling of generics) and:
Compatible changes:
- ./rest.(*NoBackoff).CalculateBackoffWithContext: added
- ./rest.(*NoBackoff).SleepWithContext: added
- ./rest.(*NoBackoff).UpdateBackoffWithContext: added
- ./rest.(*Request).BackOffWithContext: added
- ./rest.(*Request).WarningHandlerWithContext: added
- ./rest.(*URLBackoff).CalculateBackoffWithContext: added
- ./rest.(*URLBackoff).SleepWithContext: added
- ./rest.(*URLBackoff).UpdateBackoffWithContext: added
- ./rest.BackoffManagerWithContext: added
- ./rest.Config.WarningHandlerWithContext: added
- ./rest.NoWarnings.HandleWarningHeaderWithContext: added
- ./rest.SetDefaultWarningHandlerWithContext: added
- ./rest.WarningHandlerWithContext: added
- ./rest.WarningLogger.HandleWarningHeaderWithContext: added
Definitely! Having CR as one data point could be valuable as it's widely used (https://github.com/search?q=controller-runtime++path%3Ago.mod&type=code, https://pkg.go.dev/sigs.k8s.io/controller-runtime?tab=importedby), but of course we stil have to keep folks in mind that are using client-go more directly |
pohly
force-pushed
the
log-client-go-rest
branch
from
c7e4509 to
e3507ad
Compare
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/test pull-kubernetes-apidiff Now with trial build against controller-runtime 😄 |
it is failing, isn't it? I mean, isnot working |
|
"main" != "master" ... => kubernetes/test-infra#33585 |
|
/test pull-kubernetes-apidiff |
pohly
force-pushed
the
log-client-go-rest
branch
from
69101e4 to
bce3ed3
Compare
The default handler now uses contextual logging. Instead of
warnings.go:106] warning 1
it now logs the caller of client-go and uses structured, contextual
logging
main.go:100] "Warning" message="warning 1"
Users of client-go have the choice whether the handler that they provide uses
the traditional API (no API break!) or contextual logging.
The BackoffManager interface sleeps without considering the caller's context, i.e. cancellation is not supported. This alone is reason enough to deprecate it and to replace it with an interface that supports a context parameter. The other reason is that contextual logging needs that parameter.
The remaining calls can be converted without API changes.
Storing a context and making sure that it never gets canceled also has overhead. We might as well just do the klog.FromContext when constructing the Result and store the logger for later use.
pohly
force-pushed
the
log-client-go-rest
branch
from
bce3ed3 to
b738646
Compare
|
@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. |
|
/lgtm not sure if you want to squash or not, up to you |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 209c2e1a443863d855d64d35924cd0cd542d00f3
|
|
/hold in case you want to squash |
k8s-ci-robot
added
the
do-not-merge/hold
k8s-ci-robot
added
the
approved
|
No, I prefer to keep the four distinct commits. They are logically separate. /hold cancel Thanks for the review! |
k8s-ci-robot
removed
the
do-not-merge/hold
What type of PR is this?
/kind feature
What this PR does / why we need it:
Some of the APIs in client-go/rest ignored the callers context:
Which issue(s) this PR fixes:
Related-to #126379
Special notes for your reviewer:
Current controller-runtime compiles okay with these changes.
This completes the work started in #126999.
Does this PR introduce a user-facing change?
/wg structured-logging
/sig instrumentation
/sig api-machinery
/cc @sttts