client: Added new session option to set timeout for session creation #16385

AngstyDuck · 2023-08-07T10:23:09Z

When communicating via gRPC, etcd sets the gRPC option WaitForReady = true to minimize client request error responses due to transient failures. But because of this option, the creation of new sessions hang without errors when all nodes could not be contacted by the client. This new session option allows client to receive an error if the nodes cannot be contacted after the specified amount of time.

This seeks to resolve #14631

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

ahrtr · 2023-08-09T09:05:55Z

What if the etcdserver shutdown after the connection has already established? Will the session timeout or be blocked waiting for the cluster to recover? The expected behaviour should be the latter. Can you add a test case to verify it?

AngstyDuck · 2023-08-13T17:17:24Z

What if the etcdserver shutdown after the connection has already established? Will the session timeout or be blocked waiting for the cluster to recover? The expected behaviour should be the latter. Can you add a test case to verify it?

Thanks for pointing this out, I've fixed the bug and added another test case to verify this. Do let me know what you think.

AngstyDuck · 2023-08-20T13:45:56Z

Hey @ahrtr, any updates regarding the changes I've made? I'm not quite sure how to write a test that verifies that hanging would occur (without using timeout, as the behavior has to be differentiated from the timeout that the new session option creates), so I have verified the context object instead.

client/v3/concurrency/session.go

tests/integration/clientv3/concurrency/session_test.go

client/v3/concurrency/session.go

AngstyDuck · 2023-09-03T16:40:03Z

Hey @fuweid, thanks for your review. I've addressed them accordingly, so do let me know what you think 🙏

fuweid

LGTM

client/v3/concurrency/session.go

tests/integration/clientv3/concurrency/session_test.go

ahrtr · 2023-09-04T07:49:37Z

Please also squash the commits, thx

AngstyDuck · 2023-09-05T16:20:17Z

Hey @ahrtr, I've added a new test case where we verify that the default behavior isn't affected if an operation hangs beyond the timeout specified by the new option CreationTimeout. Do let me know if this works for you 🙏

ahrtr · 2023-09-05T18:07:58Z

tests/integration/clientv3/concurrency/session_test.go

+	// if Put operation is blocked beyond the timeout specified using WithCreationTimeout,
+	// that timeout is not used by the Put operation
+	case <-time.After(2 * time.Second):
+	}


Please start the etcdserver again, and verify that eventually the cli.Put will succeed.

AngstyDuck · 2023-09-06T17:31:49Z

Hey @ahrtr, I've altered the test TestTimeoutDoesntAffectSubsequentConnections where I've restarted the test and checked that the ongoing Put succeeds. Unfortunately this has made the test flaky (53.29% failing after 152 runs) and I'm unsure how to proceed. The log is as attached here: log.txt

My first choice would be to omit the testing of this specific logic until we've resolved the flakiness issue. If we go with this, I'll be creating the issue and would like to look into this myself.
My second choice would be to resolve the flakiness before merging this into main, but the scope of this PR may be inflated.

How do you think we should proceed?

tests/integration/clientv3/concurrency/session_test.go

AngstyDuck · 2023-09-07T14:25:26Z

Hey @ahrtr, I've added the logging as suggested, and the error returned was session_test.go:198: Put failed: etcdserver: requested lease not found. This error is raised with or without the usage of the new option during session creation, so I think this might be a separate issue. I can look into this if you'd like

ahrtr · 2023-09-07T15:08:10Z

tests/integration/clientv3/concurrency/session_test.go

+	case <-donec:
+	case err := <-errorc:
+		t.Errorf("Put failed: %v", err)


There is a potential issue here. When the Put finishes, both donec and errorc might be ready to be read, then it's uncertain which case is selected.

So I suggest to remove donec, and always feed the err (returned by Put) into errroc, even it's nil.

Sorry for the confusion.

AngstyDuck · 2023-09-07T15:39:30Z

Hey @ahrtr no worries, I've removed donec as instructed. However I'm still facing the same error session_test.go:199: Put failed: etcdserver: requested lease not found from errorc, with or without the new session creation option. How should I proceed?

ahrtr · 2023-09-07T17:38:20Z

However I'm still facing the same error session_test.go:199: Put failed: etcdserver: requested lease not found from errorc, with or without the new session creation option. How should I proceed?

I will take a closer look sometime next week.
The workflow checks are all green. You only occasionally see the error?

tests/integration/clientv3/concurrency/session_test.go

AngstyDuck · 2023-09-09T11:10:48Z

The workflow checks are all green. You only occasionally see the error?

Hmm no, oddly this happens every time I've ran the test TestTimeoutDoesntAffectSubsequentConnections. I'll see if I can figure it out as well.

ahrtr · 2023-09-09T13:14:35Z

tests/integration/clientv3/concurrency/session_test.go

+
+	// create new cluster
+	clus := integration2.NewCluster(t, &integration2.ClusterConfig{Size: 1})
+	defer clus.Terminate(t)


Suggested change

defer clus.Terminate(t)

defer clus.Terminate(t)

clus.Members[0].KeepDataDirTerminate = true

ahrtr · 2023-09-09T13:14:56Z

tests/integration/clientv3/concurrency/session_test.go

+	}
+
+	// restarting and ensuring that the Put operation will eventually succeed
+	clus.Members[0].Restart(t)


Suggested change

clus.Members[0].Restart(t)

clus.Members[0].Restart(t)

clus.Members[0].WaitOK(t)

ahrtr · 2023-09-09T13:17:31Z

The reason why the workflow is green is the test was skipped.

=== SKIP: integration/clientv3/concurrency TestTimeoutDoesntAffectSubsequentConnections (0.05s)
    leak.go:102: Found leaked goroutined BEFORE test appears to have leaked :
        github.com/soheilhy/cmux.muxListener.Accept(...)
        	/go/pkg/mod/github.com/soheilhy/[email protected]/cmux.go:262
        net/http.(*Server).Serve(0xc00001c0f0, {0x1992220, 0xc0003b9820})
        	/__t/go/1.20.8/x64/src/net/http/server.go:3059 +0x5a7
        net/http/httptest.(*Server).goServe.func1()
        	/__t/go/1.20.8/x64/src/net/http/httptest/server.go:310 +0xbc
        created by net/http/httptest.(*Server).goServe
        	/__t/go/1.20.8/x64/src/net/http/httptest/server.go:308 +0xa7

When terminating the member, the data is removed automatically. You need to keep the data dir so that the lease is still valid after restarting the member. Please apply the suggested change, and try again.

jmhbnz

Hey @AngstyDuck - Can you please rebase this pr to address conflicts, thanks!

AngstyDuck · 2023-09-24T16:09:12Z

Hey @jmhbnz, I've rebased the PR 👍 thanks!

jmhbnz · 2024-01-18T19:11:26Z

Discussed during sig-etcd triage meeting, @AngstyDuck can you please rebase this pr once more? Thanks!

tests/framework/integration/testing.go

AngstyDuck · 2024-01-30T05:11:42Z

Hey @jmhbnz, the linting errors have been fixed, do let me know if you need anything else 👍

jmhbnz

LGTM - Thanks @AngstyDuck and apologies this pr has been in review for so long.

jmhbnz · 2024-05-23T10:33:25Z

/test all

jmhbnz · 2024-05-23T10:41:42Z

@AngstyDuck to be safe we should rebase one final time before we merge.

When communicating via gRPC, etcd sets the gRPC option WaitForReady = true to minimize client request error responses due to transient failures. But because of this option, the creation of new sessions hang without errors when all nodes could not be contacted by the client. This new session option allows client to receive an error if the nodes cannot be contacted after the specified amount of time. Signed-off-by: AngstyDuck <[email protected]>

k8s-ci-robot · 2024-08-04T17:47:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, AngstyDuck, fuweid, jmhbnz, lhmzhou

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:

~~OWNERS~~ [ahrtr,jmhbnz]

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

a-nych · 2024-09-17T14:53:45Z

Any updates on this?

k8s-ci-robot · 2024-10-05T03:22:51Z

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.

fuweid · 2024-10-11T21:12:42Z

@a-nych sorry for late update. Please rebase it
ping @AngstyDuck . Thanks!

AngstyDuck force-pushed the session-creation-timeout-option branch 4 times, most recently from cd6fd75 to ffe5ef5 Compare August 9, 2023 08:33

AngstyDuck force-pushed the session-creation-timeout-option branch from ffe5ef5 to 77ec301 Compare August 13, 2023 17:11

fuweid reviewed Aug 30, 2023

View reviewed changes

fuweid approved these changes Sep 4, 2023

View reviewed changes

ahrtr reviewed Sep 4, 2023

View reviewed changes

client/v3/concurrency/session.go Outdated Show resolved Hide resolved

tests/integration/clientv3/concurrency/session_test.go Outdated Show resolved Hide resolved

AngstyDuck force-pushed the session-creation-timeout-option branch from 9f1ce8d to 976cff5 Compare September 5, 2023 16:17

ahrtr reviewed Sep 5, 2023

View reviewed changes

AngstyDuck force-pushed the session-creation-timeout-option branch from 011d138 to 357f70d Compare September 6, 2023 17:25

ahrtr reviewed Sep 6, 2023

View reviewed changes

tests/integration/clientv3/concurrency/session_test.go Outdated Show resolved Hide resolved

tests/integration/clientv3/concurrency/session_test.go Outdated Show resolved Hide resolved

tests/integration/clientv3/concurrency/session_test.go Outdated Show resolved Hide resolved

AngstyDuck force-pushed the session-creation-timeout-option branch from 4869140 to d0ac5df Compare September 7, 2023 14:24

AngstyDuck force-pushed the session-creation-timeout-option branch from d0ac5df to 98fa529 Compare September 7, 2023 14:26

ahrtr reviewed Sep 7, 2023

View reviewed changes

tests/integration/clientv3/concurrency/session_test.go Outdated Show resolved Hide resolved

ahrtr reviewed Sep 9, 2023

View reviewed changes

AngstyDuck force-pushed the session-creation-timeout-option branch from 288707e to b3f356b Compare September 12, 2023 16:08

jmhbnz reviewed Sep 23, 2023

View reviewed changes

AngstyDuck force-pushed the session-creation-timeout-option branch from 2354cb9 to 895e9b8 Compare September 24, 2023 16:08

lhmzhou approved these changes Jan 4, 2024

View reviewed changes

AngstyDuck force-pushed the session-creation-timeout-option branch from 895e9b8 to ba58a49 Compare January 22, 2024 15:38

jmhbnz reviewed Jan 24, 2024

View reviewed changes

tests/framework/integration/testing.go Outdated Show resolved Hide resolved

AngstyDuck force-pushed the session-creation-timeout-option branch 2 times, most recently from 311eea4 to 6f76f6e Compare January 30, 2024 03:44

jmhbnz approved these changes May 23, 2024

View reviewed changes

jmhbnz added the ok-to-test label May 23, 2024

k8s-ci-robot added area/clientv3 area/testing labels May 26, 2024

AngstyDuck force-pushed the session-creation-timeout-option branch 4 times, most recently from 2b5f97c to c723ffd Compare May 26, 2024 16:47

AngstyDuck force-pushed the session-creation-timeout-option branch from c723ffd to b37c822 Compare August 4, 2024 17:47

k8s-ci-robot added approved size/L labels Aug 4, 2024

Merge branch 'etcd-io:main' into session-creation-timeout-option

cfd570a

k8s-ci-robot added the needs-rebase label Oct 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: Added new session option to set timeout for session creation #16385

client: Added new session option to set timeout for session creation #16385

AngstyDuck commented Aug 7, 2023

ahrtr commented Aug 9, 2023

AngstyDuck commented Aug 13, 2023

AngstyDuck commented Aug 20, 2023 •

edited

Loading

AngstyDuck commented Sep 3, 2023

fuweid left a comment

ahrtr commented Sep 4, 2023

AngstyDuck commented Sep 5, 2023

ahrtr Sep 5, 2023

AngstyDuck commented Sep 6, 2023 •

edited

Loading

AngstyDuck commented Sep 7, 2023 •

edited

Loading

ahrtr Sep 7, 2023

AngstyDuck commented Sep 7, 2023

ahrtr commented Sep 7, 2023

AngstyDuck commented Sep 9, 2023

ahrtr Sep 9, 2023

ahrtr Sep 9, 2023

ahrtr commented Sep 9, 2023

jmhbnz left a comment

AngstyDuck commented Sep 24, 2023 •

edited

Loading

jmhbnz commented Jan 18, 2024

AngstyDuck commented Jan 30, 2024

jmhbnz left a comment

jmhbnz commented May 23, 2024

jmhbnz commented May 23, 2024

k8s-ci-robot commented Aug 4, 2024

a-nych commented Sep 17, 2024

k8s-ci-robot commented Oct 5, 2024

fuweid commented Oct 11, 2024 •

edited

Loading

	defer clus.Terminate(t)
	defer clus.Terminate(t)
	clus.Members[0].KeepDataDirTerminate = true

	clus.Members[0].Restart(t)
	clus.Members[0].Restart(t)
	clus.Members[0].WaitOK(t)

client: Added new session option to set timeout for session creation #16385

Are you sure you want to change the base?

client: Added new session option to set timeout for session creation #16385

Conversation

AngstyDuck commented Aug 7, 2023

ahrtr commented Aug 9, 2023

AngstyDuck commented Aug 13, 2023

AngstyDuck commented Aug 20, 2023 • edited Loading

AngstyDuck commented Sep 3, 2023

fuweid left a comment

Choose a reason for hiding this comment

ahrtr commented Sep 4, 2023

AngstyDuck commented Sep 5, 2023

ahrtr Sep 5, 2023

Choose a reason for hiding this comment

AngstyDuck commented Sep 6, 2023 • edited Loading

AngstyDuck commented Sep 7, 2023 • edited Loading

ahrtr Sep 7, 2023

Choose a reason for hiding this comment

AngstyDuck commented Sep 7, 2023

ahrtr commented Sep 7, 2023

AngstyDuck commented Sep 9, 2023

ahrtr Sep 9, 2023

Choose a reason for hiding this comment

ahrtr Sep 9, 2023

Choose a reason for hiding this comment

ahrtr commented Sep 9, 2023

jmhbnz left a comment

Choose a reason for hiding this comment

AngstyDuck commented Sep 24, 2023 • edited Loading

jmhbnz commented Jan 18, 2024

AngstyDuck commented Jan 30, 2024

jmhbnz left a comment

Choose a reason for hiding this comment

jmhbnz commented May 23, 2024

jmhbnz commented May 23, 2024

k8s-ci-robot commented Aug 4, 2024

a-nych commented Sep 17, 2024

k8s-ci-robot commented Oct 5, 2024

fuweid commented Oct 11, 2024 • edited Loading

AngstyDuck commented Aug 20, 2023 •

edited

Loading

AngstyDuck commented Sep 6, 2023 •

edited

Loading

AngstyDuck commented Sep 7, 2023 •

edited

Loading

AngstyDuck commented Sep 24, 2023 •

edited

Loading

fuweid commented Oct 11, 2024 •

edited

Loading