Skip to content
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.

Sign up for GitHub

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

add csi timeout flag #93

Merged
merged 2 commits into from
Apr 16, 2019
Merged

add csi timeout flag #93

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6504b48
add csi timeout flag for request timeouts to CSI driver
andrewsykim Feb 14, 2019
35d8184
set default timeout to 1m
andrewsykim Apr 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions cmd/csi-snapshotter/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ const (
threads = 10

// Default timeout of short CSI calls like GetPluginInfo
csiTimeout = time.Second
defaultCSITimeout = time.Minute
)

// Command line flags
Expand All @@ -66,6 +66,7 @@ var (
snapshotNamePrefix = flag.String("snapshot-name-prefix", "snapshot", "Prefix to apply to the name of a created snapshot")
snapshotNameUUIDLength = flag.Int("snapshot-name-uuid-length", -1, "Length in characters for the generated uuid of a created snapshot. Defaults behavior is to NOT truncate.")
showVersion = flag.Bool("version", false, "Show version.")
csiTimeout = flag.Duration("timeout", defaultCSITimeout, "The timeout for any RPCs to the CSI driver. Default is 10s.")

leaderElection = flag.Bool("leader-election", false, "Enables leader election.")
leaderElectionNamespace = flag.String("leader-election-namespace", "", "The namespace where the leader election resource exists. Defaults to the pod namespace if not set.")
Expand Down Expand Up @@ -142,7 +143,7 @@ func main() {
}

// Pass a context with a timeout
ctx, cancel := context.WithTimeout(context.Background(), csiTimeout)
ctx, cancel := context.WithTimeout(context.Background(), *csiTimeout)
defer cancel()

// Find driver name
Expand All @@ -155,10 +156,9 @@ func main() {
klog.V(2).Infof("CSI driver name: %q", *snapshotterName)

// Check it's ready
if err = csirpc.ProbeForever(csiConn, csiTimeout); err != nil {
if err = csirpc.ProbeForever(csiConn, *csiTimeout); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is still just one timeout in the main.

If you take a look of https://github.com/kubernetes-csi/external-attacher/blob/master/cmd/csi-attacher/main.go#L124 and https://github.com/kubernetes-csi/external-attacher/blob/master/cmd/csi-attacher/main.go#L131, there are two different timeouts. In ProbeForever, it uses "timeout". In context.WithTimeout, it uses csiTimeout.

Also the sequence is different. In attacher, ProbeForever is called before context.WithTimeout and GetDriverName. Should we do the same here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm missing something here. I think that in the other sidecars, those two values should be the same as well. For external-attacher, it seems like csiTimeout is only used for GetDriverName which seems odd to me. I can update the other sidecars to always use --timeout for ProbeForever & GetDriverName. I think I also made the code harder to read because I assigned csiTimeout as the flag var where as in other places it's just assigned to timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having one timeout for all operations seems reasonable to me

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sgtm! This PR as-is reflects that. I will follow-up in the other sidecars to do the same

klog.Errorf("error waiting for CSI driver to be ready: %v", err)
os.Exit(1)

}

// Find out if the driver supports create/delete snapshot.
Expand Down Expand Up @@ -191,7 +191,7 @@ func main() {
*createSnapshotContentRetryCount,
*createSnapshotContentInterval,
snapShotter,
*connectionTimeout,
*csiTimeout,
*resyncPeriod,
*snapshotNamePrefix,
*snapshotNameUUIDLength,
Expand Down