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.

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

Allow different operation names #75213

Conversation

@gnufied
Copy link
Member

commented Mar 8, 2019

Allow different operation names for different storage operations. This still prevents two operations on same volume from happening concurrently but if operation changes, it resets the exponential backoff.

Fixes #75209

Reset exponential backoff when storage operation changes
@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

cc @msau42
/sig storage
/kind bug
/priority important-soon

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

Putting on hold for more eyes.

/hold

@msau42

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

/assign @jingxu97 @verult

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

/retest

@jsafrane

This comment has been minimized.

Copy link
Member

commented Mar 11, 2019

I generally like the direction where this is going, however, I'd like to have more people looking at it. Does it have any impact on attach/detach? Should all operations have its own name?

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 11, 2019

@jsafrane agreed about getting more eyes on the PR. In general, I think this PR works and isn't breaking.

Obviously this PR currently does not affect attach/detach yet because I haven't put a name to those operations yet. We could this incrementally I guess since mount/unmount was the one that @smarterclayton reported as immediate problem.

@gnufied gnufied force-pushed the gnufied:allow-different-operation-names-storage branch from ff5e552 to 1240b18 Mar 12, 2019

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 12, 2019

@jingxu97 @bertinatto addressed review comments. PTAL.

@bertinatto
Copy link
Member

left a comment

@gnufied, it seems like it does affect attach/detach operations. Even though volumetypes.GeneratedOperations.OperationName isn't specified, it'll get an empty string as default, so this condition would eval to true. Perhaps am I missing something?

I guess if we only want to cover mount/unmount operations we should make OperationName an optional field (*string) and make sure it's not nil.

Also, it might be worth adding a test to simulate attach/detach operations (if we want them to have a different treatment, of course).

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

@bertinatto what I meant was for attach/detach operations this PR does not changes old behaviour. Since both attach and detach do not specify operationName, the if condition you linked will eval to true and exponential backoff error will be thrown - but that is how it used to work before this PR. So this PR preserves old behaviour for attach/detach operation.

@bertinatto

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@gnufied, thanks for clarifying.

Other than that, I checked this patch thoroughly and it LGTM (but I'm not very familiar with this code, though).

@gnufied gnufied force-pushed the gnufied:allow-different-operation-names-storage branch from 1240b18 to fdabc69 Mar 13, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Mar 13, 2019

@gnufied gnufied force-pushed the gnufied:allow-different-operation-names-storage branch from fdabc69 to 8070c31 Mar 13, 2019

@gnufied gnufied force-pushed the gnufied:allow-different-operation-names-storage branch from 8070c31 to 97ec615 Mar 13, 2019

@gnufied

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I am cancelling hold. I think it has been reviewed by people I wanted to review. :-)

/hold cancel

@jingxu97

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Mar 13, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jingxu97

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

// If an operation with the same volumeName and same or empty podName
// exists, an AlreadyExists or ExponentialBackoff error is returned.
// If an operation with the same volumeName, same or empty podName
// and same operationName exits, an AlreadyExists or ExponentialBackoff

This comment has been minimized.

Copy link
@verult

verult Mar 13, 2019

Contributor

An AlreadyExists error is returned for the same volumeName and same podName, as long as the previous operation is pending, right?

It was unclear to me from the comments whether we allow an operation to proceed when another operation with the same (volumeName, podName, operationName) key is in flight

This comment has been minimized.

Copy link
@gnufied

gnufied Mar 14, 2019

Author Member

yeah, operations with same volumename and podName aren't allowed to proceed concurrently. If there is a pending operation with same volumenme+podName there AlreadyExists error is thrown.

This comment has been minimized.

Copy link
@verult

verult Mar 16, 2019

Contributor

could you update the comments with that explanation?

@fejta-bot

This comment has been minimized.

Copy link

commented Mar 14, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

commented Mar 20, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 8da0c4a into kubernetes:master Mar 20, 2019

17 checks passed

cla/linuxfoundation gnufied authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.