[WIP]Migrate client-go/rest to contextual logging #122445

WillardHu · 2023-12-22T02:57:56Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate client-go/rest to contextual logging

Which issue(s) this PR fixes:

Fixes [#3077 ](kubernetes/enhancements#3077)

Special notes for your reviewer:

Check result of logcheck:

# echo "Component | Non-Structured Logging | Non-Contextual Logging | Owner " && echo "------ | ------- |  ------- | ------" && for i in $(find staging/src/k8s.io/client-go/rest  -maxdepth 0 -type d | sort); do      echo "$i | $(cd $i; ${GOPATH}/bin/logcheck -check-structured -check-deprecations=false 2>&1 ./... | wc -l ) | $(cd $i; ${GOPATH}/bin/logcheck -check-structured -check-deprecations=false -check-contextual ./... 2>&1 | wc -l ) |" | grep -v '| 0 | 0 |'; done
Component | Non-Structured Logging | Non-Contextual Logging | Owner
------ | ------- |  ------- | ------
staging/src/k8s.io/client-go/rest |        0 |        0 |

Does this PR introduce a user-facing change?

Migrate client-go/rest to contextual logging

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/tree/42bb993e369d166142b7181e90882066ad6c7651/keps/sig-instrumentation/3077-contextual-logging

linux-foundation-easycla · 2023-12-22T02:58:03Z

The committers listed above are authorized under a signed CLA.

✅ login: WillardHu / name: Willard (6834ea7)

k8s-ci-robot · 2023-12-22T02:58:05Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

k8s-ci-robot · 2023-12-22T02:58:05Z

Welcome @WillardHu!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2023-12-22T02:58:06Z

Hi @WillardHu. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

WillardHu · 2023-12-22T03:24:45Z

PTAL, thanks @pohly

aojea · 2023-12-22T10:32:45Z

client-go is a library consumed from multiple projects , sounds weird that controller foo with its own context has RESTRequest context, no?

/cc @liggitt

pohly · 2023-12-23T17:55:19Z

staging/src/k8s.io/client-go/rest/config.go

@@ -513,6 +515,9 @@ func InClusterConfig() (*Config, error) {
 		tokenFile  = "/var/run/secrets/kubernetes.io/serviceaccount/token"
 		rootCAFile = "/var/run/secrets/kubernetes.io/serviceaccount/ca.crt"
 	)
+	// TODO: the context parameter can be added to the method in the future


The goal is to convert the code now, not later... 🥵

I don't know what the right API should be. That is what we need to find out.

pohly · 2023-12-23T18:00:59Z

@WillardHu: I don't think this PR is ready for review. It's probably also not a good first issue. Are you sure that you want to continue?

WillardHu · 2023-12-23T23:30:45Z

@WillardHu: I don't think this PR is ready for review. It's probably also not a good first issue. Are you sure that you want to continue?

Yes, I'll follow up on the changes, and I'll think again about how to define logger instances. We can discuss in the issue first.

k8s-ci-robot · 2023-12-27T04:43:46Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: WillardHu
Once this PR has been reviewed and has the lgtm label, please assign yliaog for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

staging/src/k8s.io/client-go/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pohly · 2024-01-18T13:55:31Z

staging/src/k8s.io/client-go/rest/config.go

-		klog.Errorf("Expected to load root CA config from %s, but got err: %v", rootCAFile, err)
-	} else {
-		tlsClientConfig.CAFile = rootCAFile
+		return nil, err


This is a potentially breaking change: previously, the error was ignored, which might have allowed some apps to run. Now they are going to fail.

We need to research why this error was ignored and reach out to maintainers to figure out whether this can be changed.

The original logic looks like verifying the validity of the root CA. The function NewPool() returns an error if the file could not be read, a certificate could not be parsed, or if the file does not contain any certificates. Invalid client root certificate access k8s services also return error responses.
I will try to find the author to understand why it was considered to ignore this error.

I reverted to the original logic, but I found that error logging here was important to the caller, so I used a new function InClusterConfigWithContext() to support contextual logging.

pohly · 2024-01-18T13:56:22Z

staging/src/k8s.io/client-go/rest/client_test.go

@@ -335,36 +335,36 @@ func TestHTTPProxy(t *testing.T) {
 }

 func TestCreateBackoffManager(t *testing.T) {
-
+	ctx := context.Background()


In tests, use _, ctx := ktesting.NewTestContext(t) with ktesting = k8s.io/klog/v2/ktesting.

Done, thanks.

pohly · 2024-01-18T14:00:25Z

staging/src/k8s.io/client-go/rest/urlbackoff.go

-	UpdateBackoff(actualUrl *url.URL, err error, responseCode int)
-	CalculateBackoff(actualUrl *url.URL) time.Duration
+	UpdateBackoff(ctx context.Context, actualUrl *url.URL, err error, responseCode int)
+	CalculateBackoff(ctx context.Context, actualUrl *url.URL) time.Duration


Changing the interface is another API break.

We have to keep BackoffManager as it is and add a BackoffManagerContext. Let's keep the method names the same in both, to minimize code changes.

BackoffManager and all functions returning one must get the logcheck remark.

Done, thanks.

pohly · 2024-01-18T14:04:14Z

staging/src/k8s.io/client-go/rest/request.go

@@ -68,8 +69,8 @@ type HTTPClient interface {
 // ResponseWrapper is an interface for getting a response.
 // The response may be either accessed as a raw data (the whole output is put into memory) or as a stream.
 type ResponseWrapper interface {
-	DoRaw(context.Context) ([]byte, error)
-	Stream(context.Context) (io.ReadCloser, error)
+	DoRaw(...context.Context) ([]byte, error)


Let's keep the interface as it is (API break!). Passing nil for "no context" seems fine to me.

My idea is use a variable argument list for backward compatibility. In this case, the function DoRaw() can have a context paramete DoRaw(ctx) or no paramete DoRaw(). We can delete the context.Context paramete in the future.
But this is for the user to use, the modification behavior is not controlled. I'll modify it.

Done, thanks.

Changing an interface can also break code which implements it. This code used to work:

var r rest.Request = &myRequest{} type myRequest struct { ... } func (r *myRequest) DoRaw(context.Context) ([]byte, error) { ... }

With the change to DoRaw(...context.Context) ([]byte, error) it no longer does. Extending an interface has the same problem.

This code also breaks:

var doRaw func(context.Context) ([]byte, error) doRaw = r.DoRaw

It's not clear whether any of this is actually going to be a problem. But it might, so let's avoid it whenever possible.

I have reverted the interface definition. :-)

pohly · 2024-01-18T14:05:04Z

staging/src/k8s.io/client-go/rest/request.go

+
+	// gctx defined the global context, it's set by method chaining Context().
+	// We can get it using the r.ctx() member function, which ensures that returns context is always not nil
+	gctx context.Context


The context isn't global? It's specific for the request. I would simply call the field ctx.

Done, thanks.

pohly · 2024-01-18T14:09:48Z

staging/src/k8s.io/client-go/rest/request.go

+	return r
+}
+
+// ctx returns the global context, if it is not set, we use the default context.Background()


Presumably this is needed because klog.FromContext and logr.FromContext crash when passed a nil context. Probably too late to change that now.

getCtx returning r.ctx?

Yes, the function returns the member field r.ctx, but if the member field is nil, the default value context.Background() is returned.

WillardHu · 2024-01-25T02:47:40Z

Next I'll regenerate the client code in kubernetes/typed package, and complete the performance comparison.

Signed-off-by: WillardHu <wei.hu@daocloud.io>

k8s-ci-robot · 2024-04-25T05:38:41Z

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/test-infra repository.

k8s-ci-robot requested a review from dims December 22, 2023 02:58

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Dec 22, 2023

k8s-ci-robot requested a review from smarterclayton December 22, 2023 02:58

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Dec 22, 2023

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 22, 2023

k8s-ci-robot requested a review from liggitt December 22, 2023 10:32

pohly reviewed Dec 23, 2023

View reviewed changes

WillardHu changed the title ~~Migrate client-go/rest to contextual logging~~ [WIP]Migrate client-go/rest to contextual logging Dec 23, 2023

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 23, 2023

WillardHu force-pushed the client-go-rest-contextual branch from 5f57a7d to 6e1cf85 Compare December 27, 2023 04:38

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Dec 27, 2023

WillardHu force-pushed the client-go-rest-contextual branch from 6e1cf85 to 85dae87 Compare December 27, 2023 04:43

WillardHu force-pushed the client-go-rest-contextual branch 5 times, most recently from 25c8cff to 1bedf91 Compare January 18, 2024 09:52

pohly reviewed Jan 18, 2024

View reviewed changes

WillardHu force-pushed the client-go-rest-contextual branch 2 times, most recently from 0a8481b to ddb1101 Compare January 25, 2024 02:17

Migrate client-go/rest to contextual logging

6834ea7

Signed-off-by: WillardHu <wei.hu@daocloud.io>

WillardHu force-pushed the client-go-rest-contextual branch from ddb1101 to 6834ea7 Compare March 7, 2024 01:58

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP]Migrate client-go/rest to contextual logging #122445

[WIP]Migrate client-go/rest to contextual logging #122445

WillardHu commented Dec 22, 2023 •

edited

linux-foundation-easycla bot commented Dec 22, 2023 •

edited

k8s-ci-robot commented Dec 22, 2023

k8s-ci-robot commented Dec 22, 2023

k8s-ci-robot commented Dec 22, 2023

WillardHu commented Dec 22, 2023

aojea commented Dec 22, 2023

pohly Dec 23, 2023

pohly commented Dec 23, 2023

WillardHu commented Dec 23, 2023

k8s-ci-robot commented Dec 27, 2023

pohly Jan 18, 2024

WillardHu Jan 19, 2024

WillardHu Jan 25, 2024

pohly Jan 18, 2024

WillardHu Jan 19, 2024

pohly Jan 18, 2024

WillardHu Jan 19, 2024

pohly Jan 18, 2024

WillardHu Jan 19, 2024

WillardHu Jan 19, 2024

pohly Jan 19, 2024 •

edited

WillardHu Jan 25, 2024

pohly Jan 18, 2024

WillardHu Jan 19, 2024

pohly Jan 18, 2024

WillardHu Jan 19, 2024

WillardHu commented Jan 25, 2024

k8s-ci-robot commented Apr 25, 2024

[WIP]Migrate client-go/rest to contextual logging #122445

Are you sure you want to change the base?

[WIP]Migrate client-go/rest to contextual logging #122445

Conversation

WillardHu commented Dec 22, 2023 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

linux-foundation-easycla bot commented Dec 22, 2023 • edited

k8s-ci-robot commented Dec 22, 2023

k8s-ci-robot commented Dec 22, 2023

k8s-ci-robot commented Dec 22, 2023

WillardHu commented Dec 22, 2023

aojea commented Dec 22, 2023

Choose a reason for hiding this comment

pohly commented Dec 23, 2023

WillardHu commented Dec 23, 2023

k8s-ci-robot commented Dec 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly Jan 19, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WillardHu commented Jan 25, 2024

k8s-ci-robot commented Apr 25, 2024

WillardHu commented Dec 22, 2023 •

edited

linux-foundation-easycla bot commented Dec 22, 2023 •

edited

pohly Jan 19, 2024 •

edited