replace grpc.FailFast and grpc.WithDialer which is deprecated #83003

danielqsj · 2019-09-23T06:21:12Z

What type of PR is this?

/kind bug
/area apiserver

What this PR does / why we need it:

Now the grpc.FailFast and grpc.WithDialer has deprecated, so use grpc.WaitForReady and grpc.WithContextDialer instead.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

danielqsj · 2019-09-23T06:49:18Z

/cc @liggitt

mattjmcnaughton

Overall, this makes sense to me (although I defer to anyone with more grpc experience than me :) )
Confirming this shouldn't cause a functionality change, correct?

One small note around the typecheck failure... I'm wondering if this needs to be coupled with winio update?

pkg/kubelet/util/util_windows.go

liggitt · 2019-09-23T13:24:41Z

pkg/kubelet/cm/devicemanager/endpoint.go

-		grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
-			return net.DialTimeout("unix", addr, timeout)
+		grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
+			return (&net.Dialer{}).DialContext(ctx, "unix", addr)


do we still need to call WithContextDialer (I expected this to be the default implementation... xref https://github.com/grpc/grpc-go/blob/master/clientconn.go#L198-L203)

Looks to me that switching to WithTimeout and using the default dialer impl like @liggitt suggested should preserve the previous behavior.

@liggitt @jpbetz
Maybe we can simple write like this :
c, err := grpc.DialContext(ctx, unixSocketPath, grpc.WithInsecure(), grpc.WithBlock())
Because

grpc.WithDialer is actually set timeout via context.deadline , xref https://github.com/grpc/grpc-go/blob/788ffe62759ade61e0d6273e480c9e8f3d09d30d/dialoptions.go#L358-L360

grpc.DialContext finally call dial.DialContext which will set deadline from context. xref https://github.com/golang/go/blob/cf06b9aa81004a017e5c98422fce2fafd1f24921/src/net/dial.go#L368-L370
These two approach is the same, right ?
If so, we can safely skip this option.

liggitt · 2019-09-23T13:24:51Z

pkg/kubelet/pluginmanager/operationexecutor/operation_generator.go

-		grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
-			return net.DialTimeout("unix", addr, timeout)
+		grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
+			return (&net.Dialer{}).DialContext(ctx, "unix", addr)


same question as https://github.com/kubernetes/kubernetes/pull/83003/files#r327111524

liggitt · 2019-09-23T13:24:55Z

pkg/kubelet/pluginmanager/pluginwatcher/example_handler.go

-		grpc.WithDialer(func(addr string, timeout time.Duration) (net.Conn, error) {
-			return net.DialTimeout("unix", addr, timeout)
+		grpc.WithContextDialer(func(ctx context.Context, addr string) (net.Conn, error) {
+			return (&net.Dialer{}).DialContext(ctx, "unix", addr)


same question as https://github.com/kubernetes/kubernetes/pull/83003/files#r327111524

liggitt · 2019-09-23T13:28:33Z

I don't have context on the failfast / WaitForReady change... who would be a good reviewer for that?

roycaihw · 2019-09-23T21:18:28Z

/assign @jingyih

k8s-ci-robot · 2019-09-23T21:18:30Z

@roycaihw: GitHub didn't allow me to assign the following users: jingyih.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @jingyih

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 · 2019-09-24T06:06:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: danielqsj
To complete the pull request process, please assign thockin
You can assign the PR to them by writing /assign @thockin in a comment when ready.

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

danielqsj · 2019-09-24T08:32:30Z

staging/src/k8s.io/apiserver/go.mod

@@ -60,6 +61,7 @@ require (
 )

 replace (
+	github.com/pkg/errors => github.com/pkg/errors v0.8.0


@liggitt after execute hack/update-vendor.sh, this go.mod changed to this. Should I keep it or change it to v0.8.1 ?

change it to v0.8.1 (https://github.com/microsoft/go-winio/blob/v0.4.14/go.mod#L6)

k8s-ci-robot · 2019-09-24T08:34:43Z

@danielqsj: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-dependencies	`df63cee`	link	`/test pull-kubernetes-dependencies`

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.

liggitt · 2019-10-23T12:31:45Z

the vendor bump is unrelated, can you move that to a separate PR?

the FailFast(x) to WaitForReady(!x) change is straightforward. the WithContextDialer change needs much closer review to make sure we have equivalent behavior. can you split those changes so we can go ahead and merge the first?

danielqsj · 2019-10-25T09:10:54Z

@liggitt I find the FailFast(x) was fixed by #83595.
I reopen a PR to fix the remain grpc.WithDialer in #84345.
the vendor (go-winio) bump is needed for this change to support GetAddressAndDialer refactor.

/close

k8s-ci-robot · 2019-10-25T09:10:55Z

@danielqsj: Closed this PR.

In response to this:

@liggitt I find the FailFast(x) was fixed by #83595.
I reopen a PR to fix the remain grpc.WithDialer in #84345.
the vendor (go-winio) bump is needed for this change to support GetAddressAndDialer refactor.

/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.

k8s-ci-robot requested review from enj and sakshamsharma September 23, 2019 06:23

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 23, 2019

danielqsj changed the title ~~replace grpc.FailFast which is deprecated~~ replace grpc.FailFast and grpc.WithDialer which is deprecated Sep 23, 2019

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Sep 23, 2019

k8s-ci-robot requested a review from liggitt September 23, 2019 06:49

danielqsj force-pushed the grpc branch from d9d7f87 to 8d20fde Compare September 23, 2019 08:28

mattjmcnaughton reviewed Sep 23, 2019

View reviewed changes

pkg/kubelet/util/util_windows.go Show resolved Hide resolved

liggitt reviewed Sep 23, 2019

View reviewed changes

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. area/cloudprovider area/dependency Issues or PRs related to dependency changes labels Sep 24, 2019

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Sep 24, 2019

danielqsj added 4 commits September 24, 2019 16:27

replace grpc.FailFast which is deprecated

5b303ea

replace grpc.WithDialer which is deprecated

f3e6d5d

clean .staticcheck_failures

487c731

bump github.com/Microsoft/go-winio to v0.4.14

df63cee

danielqsj force-pushed the grpc branch from 43f4190 to df63cee Compare September 24, 2019 08:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2019

danielqsj commented Sep 24, 2019

View reviewed changes

k8s-ci-robot closed this Oct 25, 2019

danielqsj mentioned this pull request Oct 25, 2019

replace grpc.WithDialer which is deprecated #84345

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace grpc.FailFast and grpc.WithDialer which is deprecated #83003

replace grpc.FailFast and grpc.WithDialer which is deprecated #83003

danielqsj commented Sep 23, 2019 •

edited

danielqsj commented Sep 23, 2019

mattjmcnaughton left a comment

liggitt Sep 23, 2019

jpbetz Sep 23, 2019

danielqsj Sep 24, 2019

liggitt Sep 23, 2019

liggitt Sep 23, 2019

liggitt commented Sep 23, 2019

roycaihw commented Sep 23, 2019

k8s-ci-robot commented Sep 23, 2019

k8s-ci-robot commented Sep 24, 2019

danielqsj Sep 24, 2019

liggitt Sep 24, 2019

k8s-ci-robot commented Sep 24, 2019 •

edited

liggitt commented Oct 23, 2019

danielqsj commented Oct 25, 2019

k8s-ci-robot commented Oct 25, 2019

replace grpc.FailFast and grpc.WithDialer which is deprecated #83003

replace grpc.FailFast and grpc.WithDialer which is deprecated #83003

Conversation

danielqsj commented Sep 23, 2019 • edited

danielqsj commented Sep 23, 2019

mattjmcnaughton left a comment

Choose a reason for hiding this comment

liggitt Sep 23, 2019

Choose a reason for hiding this comment

jpbetz Sep 23, 2019

Choose a reason for hiding this comment

danielqsj Sep 24, 2019

Choose a reason for hiding this comment

liggitt Sep 23, 2019

Choose a reason for hiding this comment

liggitt Sep 23, 2019

Choose a reason for hiding this comment

liggitt commented Sep 23, 2019

roycaihw commented Sep 23, 2019

k8s-ci-robot commented Sep 23, 2019

k8s-ci-robot commented Sep 24, 2019

danielqsj Sep 24, 2019

Choose a reason for hiding this comment

liggitt Sep 24, 2019

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 24, 2019 • edited

liggitt commented Oct 23, 2019

danielqsj commented Oct 25, 2019

k8s-ci-robot commented Oct 25, 2019

danielqsj commented Sep 23, 2019 •

edited

k8s-ci-robot commented Sep 24, 2019 •

edited