fix: client should return instantly when no backend #307

mainred · 2021-12-10T15:12:55Z

Two changes are included in this PR:

If no backend is registered to the server, and the grpc client request will hang for 30s instead of returning instantly with no backend error. This is caused by the issue that random id is not included in dia resp from server to client, thus the client does not recognize the dial resp and return, dropping the error.
for a normal client-server close case, close req is sent to backend twice, one as proxy from client, and the other one is as a generalized one. The latter one is not necessary, and bring misleading error message for both proxy-server and proxy-agent

k8s-ci-robot · 2021-12-10T15:13:03Z

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

pkg/server/server_test.go

pkg/server/server.go

jkh52 · 2021-12-18T01:07:35Z

/ok-to-test

andrewsykim · 2022-02-24T02:36:38Z

Sorry for the delay getting to this PR, I will spend some time this week testing this patch

andrewsykim · 2022-02-24T02:40:18Z

pkg/server/server.go

 				klog.ErrorS(err, "CLOSE_REQ to Backend failed", "serverID", s.serverID, "connectionID", connID)
+			} else {
+				klog.V(5).Infoln("CLOSE_REQ sent to backend", "serverID", s.serverID, "connectionID", connID)
+				return


@mainred I think it might be simpler to scope this PR to only include DialResponse containing random IDs and exclude the fix for sending CLOSE_REQ twice in a separate PR. @mihivagyok also has a PR out regarding this #322

k8s-ci-robot · 2022-03-04T05:33:23Z

@mainred: 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.

This allows the client to notice immediately, instead of timing out. I forked this from a slightly larger kubernetes-sigs#307 and will try to annotate PR so that mainred@'s contribution is recognized.

k8s-triage-robot · 2022-06-02T06:33:14Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-07-02T06:44:05Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-08-01T07:40:16Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2022-08-01T07:40:28Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

cheftako · 2022-09-09T22:34:06Z

/remove-lifecycle rotten

k8s-ci-robot · 2022-09-09T22:34:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mainred
Once this PR has been reviewed and has the lgtm label, please assign mcrute for approval by writing /assign @mcrute in a comment. 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:

OWNERS

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

k8s-ci-robot · 2022-09-09T22:34:23Z

@mainred: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-apiserver-network-proxy-make-lint	`c36b413`	link	true	`/test pull-apiserver-network-proxy-make-lint`
pull-apiserver-network-proxy-docker-build-arm64	`c36b413`	link	true	`/test pull-apiserver-network-proxy-docker-build-arm64`
pull-apiserver-network-proxy-docker-build-amd64	`c36b413`	link	true	`/test pull-apiserver-network-proxy-docker-build-amd64`
pull-apiserver-network-proxy-test	`c36b413`	link	true	`/test pull-apiserver-network-proxy-test`

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/test-infra repository. I understand the commands that are listed here.

k8s-triage-robot · 2022-12-08T22:53:06Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

mainred · 2022-12-09T14:21:27Z

Close this PR since the issue has been address by #356

Qingchuan Hao added 2 commits December 1, 2021 01:08

return early when close req sent

fab3bc9

fix: client should return instantly when no backend

01cdac0

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 10, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Dec 10, 2021

k8s-ci-robot requested review from dberkov and jkh52 December 10, 2021 15:13

mainred commented Dec 10, 2021

View reviewed changes

pkg/server/server_test.go Outdated Show resolved Hide resolved

add UT

808c3a7

mainred force-pushed the misc branch from 1def1a9 to 808c3a7 Compare December 10, 2021 15:16

jkh52 reviewed Dec 10, 2021

View reviewed changes

pkg/server/server.go Show resolved Hide resolved

jkh52 reviewed Dec 10, 2021

View reviewed changes

pkg/server/server.go Show resolved Hide resolved

add server UT for no backend

1590e4c

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 11, 2021

polish

212d1a5

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 Dec 18, 2021

fix lint

c36b413

mihivagyok mentioned this pull request Feb 1, 2022

fix: don't send closeReq twice if it is already sent #322

Closed

mihivagyok mentioned this pull request Feb 17, 2022

exit client with error when no agents are available #326

Closed

andrewsykim reviewed Feb 24, 2022

View reviewed changes

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

jkh52 mentioned this pull request May 13, 2022

Bugfix: send missing dial identifier when no agents are connected. #356

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 2, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jul 2, 2022

k8s-ci-robot closed this Aug 1, 2022

cheftako reopened this Sep 9, 2022

k8s-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Sep 9, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 8, 2022

mainred closed this Dec 9, 2022

fix: client should return instantly when no backend #307

fix: client should return instantly when no backend #307

Uh oh!

Conversation

mainred commented Dec 10, 2021

Uh oh!

k8s-ci-robot commented Dec 10, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jkh52 commented Dec 18, 2021

Uh oh!

andrewsykim commented Feb 24, 2022

Uh oh!

andrewsykim Feb 24, 2022

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Mar 4, 2022

Uh oh!

k8s-triage-robot commented Jun 2, 2022

Uh oh!

k8s-triage-robot commented Jul 2, 2022

Uh oh!

k8s-triage-robot commented Aug 1, 2022

Uh oh!

k8s-ci-robot commented Aug 1, 2022

Uh oh!

cheftako commented Sep 9, 2022

Uh oh!

k8s-ci-robot commented Sep 9, 2022

Uh oh!

k8s-ci-robot commented Sep 9, 2022

Uh oh!

k8s-triage-robot commented Dec 8, 2022

Uh oh!

mainred commented Dec 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants