Return the behaviour of a new default GRPC client instantiation for every request #3130

ciarams87 · 2024-06-04T10:55:57Z

What type of PR is this?

/kind bug
/area conformance

What this PR does / why we need it:

Returns the GRPCRoute conformance tests default client to instantiating a new client for every request, instead of creating one shared client for all requests, as this is leading to conflicts and closed client connections. More details in the issue: #3122

Which issue(s) this PR fixes:

Fixes #3122

Does this PR introduce a user-facing change?:

NONE

linux-foundation-easycla · 2024-06-04T10:56:01Z

The committers listed above are authorized under a signed CLA.

✅ login: ciarams87 / name: Ciara Stacke (049a77d)

k8s-ci-robot · 2024-06-04T10:56:06Z

Welcome @ciarams87!

It looks like this is your first PR to kubernetes-sigs/gateway-api 🎉. 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-sigs/gateway-api 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 · 2024-06-04T10:56:06Z

Hi @ciarams87. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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-sigs/prow repository.

snehachhabria · 2024-06-07T21:36:53Z

thanks for the fix @ciarams87, the changes lgtm!

/ok-to-test
/approve

gauravkghildiyal

Thanks for catching this, @ciarams87!

I have shared some thoughts, though perhaps we may want to address those independently of this PR (and not necessarily by you)

/ok-to-test

gauravkghildiyal · 2024-06-12T06:18:04Z

conformance/utils/grpc/grpc.go

@@ -270,6 +270,9 @@ func validateExpectedResponse(t *testing.T, expected ExpectedResponse) {
 func MakeRequestAndExpectEventuallyConsistentResponse(t *testing.T, c Client, timeoutConfig config.TimeoutConfig, gwAddr string, expected ExpectedResponse) {
 	t.Helper()
 	validateExpectedResponse(t, expected)
+	if c == nil {
+		c = &DefaultClient{Conn: nil}
+	}
 	defer c.Close()


While the change does help address the immediate concern, it does seem to reveal a flaw here -- I would expect the client to only be closed by MakeRequestAndExpectEventuallyConsistentResponse() when it was instantiated within MakeRequestAndExpectEventuallyConsistentResponse() itself.

When a non-nil client is passed to the MakeRequestAndExpectEventuallyConsistentResponse(), this function should not own the lifecycle the passed-clients lifecycle.

An implication of this would be that the Client interface should not have the Close function.

Thanks for the review @gauravkghildiyal!

When a non-nil client is passed to the MakeRequestAndExpectEventuallyConsistentResponse(), this function should not own the lifecycle the passed-clients lifecycle.
An implication of this would be that the Client interface should not have the Close function.

That makes a lot of sense to me, but I'm not entirely sure that should be addressed in this PR - the scope here is only to return the DefaultClient behaviour to how it was before. I'd like to defer to @snehachhabria as the original author of #3095 as to the decisions made in this respect.

@gauravkghildiyal what you suggested should be done and can be handled in a separate PR. Once this PR is merged, I can handle the suggested changes.

Thanks @ciarams87 and @snehachhabria!

gauravkghildiyal

This LGTM (has follow-up items which will be tackled separately).
I see @arkodg on the reviewers list who can help with conformance approval.

/lgtm

gauravkghildiyal · 2024-06-12T18:12:37Z

conformance/utils/grpc/grpc.go

@@ -270,6 +270,9 @@ func validateExpectedResponse(t *testing.T, expected ExpectedResponse) {
 func MakeRequestAndExpectEventuallyConsistentResponse(t *testing.T, c Client, timeoutConfig config.TimeoutConfig, gwAddr string, expected ExpectedResponse) {
 	t.Helper()
 	validateExpectedResponse(t, expected)
+	if c == nil {
+		c = &DefaultClient{Conn: nil}
+	}
 	defer c.Close()


Thanks @ciarams87 and @snehachhabria!

arkodg

/approve
/lgtm

thanks !

k8s-ci-robot · 2024-06-14T03:17:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arkodg, ciarams87, gnossen, snehachhabria

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~conformance/OWNERS~~ [arkodg]

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

…etes-sigs#3130)

Return new default GRPC client instantiation to every request

049a77d

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/bug Categorizes issue or PR as related to a bug. labels Jun 4, 2024

k8s-ci-robot added area/conformance cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 4, 2024

k8s-ci-robot requested review from arkodg and Xunzhuo June 4, 2024 10:56

gnossen approved these changes Jun 11, 2024

View reviewed changes

gauravkghildiyal reviewed Jun 12, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 12, 2024

gauravkghildiyal reviewed Jun 12, 2024

View reviewed changes

k8s-ci-robot assigned gauravkghildiyal Jun 12, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 12, 2024

gauravkghildiyal mentioned this pull request Jun 13, 2024

[conformance] Properly handle lifecycle of an externally passed gRPC client #3156

Open

arkodg approved these changes Jun 14, 2024

View reviewed changes

k8s-ci-robot assigned arkodg Jun 14, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 14, 2024

k8s-ci-robot merged commit 5f400a8 into kubernetes-sigs:main Jun 14, 2024
8 checks passed

ciarams87 deleted the bug/return-grpc-client-instantiation branch June 14, 2024 09:56

BobyMCbobs pushed a commit to BobyMCbobs/kubernetes-sigs-gateway-api that referenced this pull request Jul 10, 2024

Return new default GRPC client instantiation to every request (kubern…

1aff171

…etes-sigs#3130)

BobyMCbobs pushed a commit to BobyMCbobs/kubernetes-sigs-gateway-api that referenced this pull request Jul 22, 2024

Return new default GRPC client instantiation to every request (kubern…

ddd231f

…etes-sigs#3130)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return the behaviour of a new default GRPC client instantiation for every request #3130

Return the behaviour of a new default GRPC client instantiation for every request #3130

ciarams87 commented Jun 4, 2024

linux-foundation-easycla bot commented Jun 4, 2024 •

edited

Loading

k8s-ci-robot commented Jun 4, 2024

k8s-ci-robot commented Jun 4, 2024

snehachhabria commented Jun 7, 2024 •

edited

Loading

gauravkghildiyal left a comment

gauravkghildiyal Jun 12, 2024

ciarams87 Jun 12, 2024 •

edited

Loading

snehachhabria Jun 12, 2024

gauravkghildiyal Jun 12, 2024

gauravkghildiyal left a comment

gauravkghildiyal Jun 12, 2024

arkodg left a comment

k8s-ci-robot commented Jun 14, 2024

Return the behaviour of a new default GRPC client instantiation for every request #3130

Return the behaviour of a new default GRPC client instantiation for every request #3130

Conversation

ciarams87 commented Jun 4, 2024

linux-foundation-easycla bot commented Jun 4, 2024 • edited Loading

k8s-ci-robot commented Jun 4, 2024

k8s-ci-robot commented Jun 4, 2024

snehachhabria commented Jun 7, 2024 • edited Loading

gauravkghildiyal left a comment

Choose a reason for hiding this comment

gauravkghildiyal Jun 12, 2024

Choose a reason for hiding this comment

ciarams87 Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

snehachhabria Jun 12, 2024

Choose a reason for hiding this comment

gauravkghildiyal Jun 12, 2024

Choose a reason for hiding this comment

gauravkghildiyal left a comment

Choose a reason for hiding this comment

gauravkghildiyal Jun 12, 2024

Choose a reason for hiding this comment

arkodg left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 14, 2024

linux-foundation-easycla bot commented Jun 4, 2024 •

edited

Loading

snehachhabria commented Jun 7, 2024 •

edited

Loading

ciarams87 Jun 12, 2024 •

edited

Loading