-
Notifications
You must be signed in to change notification settings - Fork 188
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixbug: check driver timeout #68
Conversation
/assign @jsafrane |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am open to increasing probe timeout, however, I don't like the other changes.
cmd/csi-attacher/main.go
Outdated
@@ -120,7 +120,9 @@ func main() { | |||
os.Exit(1) | |||
} | |||
|
|||
supportsService, err := csiConn.SupportsPluginControllerService(ctx) | |||
ctxController, cancelController := context.WithTimeout(context.Background(), csiTimeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like 3 different contexts. If you think that 1 second is too short for GetDriverName+SupportsPluginControllerService+SupportsControllerPublish together, then we could increase the context timeout to 3 seconds instead of using three different ones.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @jsafrane
Thanks for your reply.
After only increasing Probe timeout, I still meet timeout error in external attacher container. We may not call waitForDriverReady during the three calls, GetDriverName+SupportsPluginControllerService+SupportsControllerPublish.
If we don't use 3 different contexts, we can call waitForDriverReady before or after
the three calls.
When calling waitForDriverReady before the three calls, may waste some time because probe is not a short call.
On the contrary, we could call waitForDriverReady after the three calls and before launching CSI handler or trivial handler.
only increasing Probe timeout in external attacher:
# kubectl logs csi-neonsan-controller-0 csi-attacher
I0827 07:58:09.721682 1 main.go:75] Version: v0.2.0-35-g5a4cc3b-dirty
I0827 07:58:09.727437 1 connection.go:88] Connecting to /var/lib/csi/sockets/pluginproxy/csi.sock
I0827 07:58:09.727923 1 connection.go:115] Still trying, connection is CONNECTING
I0827 07:58:09.728326 1 connection.go:112] Connected
I0827 07:58:09.728351 1 connection.go:235] GRPC call: /csi.v0.Identity/GetPluginInfo
I0827 07:58:09.728363 1 connection.go:236] GRPC request:
I0827 07:58:09.731362 1 connection.go:238] GRPC response: name:"csi-neonsan" vendor_version:"0.3.0"
I0827 07:58:09.731484 1 connection.go:239] GRPC error: <nil>
I0827 07:58:09.731495 1 main.go:115] CSI driver name: "csi-neonsan"
I0827 07:58:09.731585 1 connection.go:235] GRPC call: /csi.v0.Identity/Probe
I0827 07:58:09.731629 1 connection.go:236] GRPC request:
I0827 07:58:11.783301 1 connection.go:238] GRPC response: ready:<value:true >
I0827 07:58:11.783424 1 connection.go:239] GRPC error: <nil>
I0827 07:58:11.783439 1 main.go:202] Probe succeeded
I0827 07:58:11.783453 1 connection.go:235] GRPC call: /csi.v0.Identity/GetPluginCapabilities
I0827 07:58:11.783464 1 connection.go:236] GRPC request:
I0827 07:58:11.784170 1 connection.go:238] GRPC response:
I0827 07:58:11.784301 1 connection.go:239] GRPC error: rpc error: code = DeadlineExceeded desc = context deadline exceeded
E0827 07:58:11.784501 1 main.go:125] rpc error: code = DeadlineExceeded desc = context deadline exceeded
5956470
to
1ce827f
Compare
@@ -114,18 +114,17 @@ func main() { | |||
} | |||
glog.V(2).Infof("CSI driver name: %q", attacher) | |||
|
|||
// Check it's ready |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I see what you meant. Can you move this check before ctx, cancel := context.WithTimeout(context.Background(), csiTimeout)
~10 lines above?
I think the order of waitForDriverReady()
and GetDriverName()
does not really matter, so let's do probe first and then start 1 context for GetDriverName+SupportsPluginControllerService+SupportsControllerPublish.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, @jsafrane
Currently, I move the probe check before GetDriverName+SupportsPluginControllerService+SupportsControllerPublish and increase probe timeout. Would you please review this PR again?
1ce827f
to
599ca57
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, wnxn 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:
Approvers can indicate their approval by writing |
@ jsafrane Events: Normal Scheduled 1m default-scheduler Successfully assigned default/csi-pod-nodeworker to 111.111.111.116 the log print :rpc error: code = DeadlineExceeded desc = context deadline exceeded is that within 15s |
STOR-1688: Chore: add .snyk file to ignore false positives
Please reference Issue #67.