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

Return error if snapshot doesn't exist #287

Closed
bertinatto opened this issue Apr 30, 2019 · 8 comments
Closed

Return error if snapshot doesn't exist #287

bertinatto opened this issue Apr 30, 2019 · 8 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.

Comments

@bertinatto
Copy link
Member

bertinatto commented Apr 30, 2019

/kind bug

What happened?

It seems like we're not returning a status.NotFound error when CreateVolume is given a non-existent snapshot ID.

https://github.com/kubernetes-csi/csi-test/blob/e1f2421c7f0a0cc7f9942fea1192392f253891db/pkg/sanity/controller.go#L738

CC @tsmetana @leakingtapan

@k8s-ci-robot k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Apr 30, 2019
@bertinatto
Copy link
Member Author

My guess is that AWS will return an InvalidSnapshot.NotFound error. In that case, we just have to return an cloud.ErrNotFound in CreateDisk; and in CreateVolume return an status.NotFound.

@tsmetana
Copy link
Contributor

/assign

@k8s-ci-robot
Copy link
Contributor

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

Note that only kubernetes-sigs members and repo collaborators can be assigned and that issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign

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.

@tsmetana
Copy link
Contributor

tsmetana commented Apr 30, 2019

The strange thing is that the test suite complains about controller returning nil not a wrong error. I'll take a look at this.

@bertinatto
Copy link
Member Author

The strange thing is that the test suite complains about controller returning nil not a wrong error. I'll take a look at this.

Yes, I fixed that in #288. However, in addition to fixing the fake cloud implementation (ie #288), we need to make sure this is fixed in cloud.go as well.

@tsmetana
Copy link
Contributor

Yes, I fixed that in #288. However, in addition to fixing the fake cloud implementation (ie #288), we need to make sure this is fixed in cloud.go as well.

OK. So we miss the error "translation". Thanks for explaining.

@bertinatto bertinatto added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 3, 2019
@leakingtapan
Copy link
Contributor

/close

@k8s-ci-robot
Copy link
Contributor

@leakingtapan: Closing this issue.

In response to this:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

No branches or pull requests

4 participants