closes issue#2135 image pull returns 404 on manifest request if there is storage error #2138
Conversation
…rage error When get manifest, the handler will try to retrieve it from storage driver. When storage driver is cloud storage, it can fail due to various reasons even if the manifest exists (like 500, 503, etc. from storage server). Currently manifest handler blindly return 404 which can be confusing to user. This change will return 404 if the manifest blob doesn't exist, and return 500 UnknownError for all other errors (consistent with the behavior of other handlers). Signed-off-by: Yu Wang (UC) <[email protected]>
|
LGTM |
Current coverage is 51.11% (diff: 37.50%)@@ master #2138 diff @@
==========================================
Files 125 125
Lines 11436 11444 +8
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 7004 5850 -1154
- Misses 3545 4848 +1303
+ Partials 887 746 -141
|
|
@yuwaMSFT2 Could you add a test for this? |
|
@stevvooe Ok I will try to add a test for this. |
|
@stevvooe I added some test for the manifest handler. Also added a storage mockdriver so that the test can force storage errors easily. |
| var errUnexpected = errors.New("unexpected error") | ||
| var testServer = createServer() | ||
|
|
||
| func createServer() *httptest.Server { |
There was a problem hiding this comment.
There are already existing unit tests. Just add a case to those.
There was a problem hiding this comment.
Is it the api_test.go under handlers? thought there would be individual files:)
There was a problem hiding this comment.
@yuwaMSFT2 We use that to test the cross-handler flow, which really ensures that we implement the spec. That is really structured around docs/spec/api.md. You should be able to insert a few cases but I don't recall if we have spots to change upstream errors.
There was a problem hiding this comment.
Thanks @stevvooe I will add the functionality of introducing storage error into TestDriver, and add the new test in api_test.go...plz let me if you suggest differently...
| // TestDriver is a StorageDriver for testing purposes. The Writer returned by this driver | ||
| // simulates the case where Write operations are buffered. This causes the value returned by Size to lag | ||
| // behind until Close (or Commit, or Cancel) is called. | ||
| type TestDriver struct { | ||
| storagedriver.StorageDriver | ||
| // The below is to override the default GetContent on StorageDriver so that we can force error, etc. easily |
There was a problem hiding this comment.
You don't need all this. Just define a type for the test and implement the needed methods, like it did here.
There was a problem hiding this comment.
Do you mean to define and use a new storage test driver, as I did in previous commit?
There was a problem hiding this comment.
I reverted the change on the existing testdriver. Put the mock interface into a separate mockdriver. New test case uses the mockdriver to force storage error.
There was a problem hiding this comment.
All you need is this:
type myTestDriver struct {
storagedriver.StorageDriver
}
func (*myTestDriver) Stat(...) (...)
func (*myTestDriver) Get(...) (...)
You don't need:
- A separate package
- A stub for every possible method call
There was a problem hiding this comment.
Thanks! I thought to make the mock driver generic enough so that other future tests can use it. I will follow your suggestion to only scope the driver for this new manifest test if that is preferred.
| ) | ||
|
|
||
| // ErrMockFuncNotImpl is the error when a function of the interface is called but no implementation is provided | ||
| var ErrMockFuncNotImpl = errors.New("The mock function called is not implemented") |
There was a problem hiding this comment.
Again, you don't need to do this. Go will panic for you.
| defer env1.Shutdown() | ||
| testDriver := env1.app.driver.(*testdriver.MockDriver) | ||
| // Override the GetContent to generate errors | ||
| testDriver.MockGetContent = func(ctx context.Context, path string) ([]byte, error) { |
There was a problem hiding this comment.
Declare the type here and assign an error per path. No need to have a separate package, function pointers or any other nonsense.
Literally, just declare the type here and make it have this implementation.
I know you think "Someone will use this later", but they won't and we'll end up with yet another mock. Make mocks narrow.
type mockErrorDriver struct {
returns []struct {
partial
content []byte
err error
}
}
func (dr *mockErrorDriver) GetContent(ctx context.Context, path string) ([]byte, error) {
for _, returns := range dr.returns {
if strings.Contains(path, returns.partial) {
return returns.content, returns.err
}
}
return nil, errGenericStorage
}There was a problem hiding this comment.
Refactored the test as suggested...thx!
|
Thanks for adding test here, update looks good. Do you mind squashing the last 5 commits into 1, so you just have a commit for the fix and commit for the test. Thanks for sticking with this and being responsive to feedback! |
Signed-off-by: Yu Wang (UC) <[email protected]>
|
LGTM |
1 similar comment
|
LGTM |
closes issue#2135 image pull returns 404 on manifest request if there is storage error
When get manifest, the handler will try to retrieve it from storage driver. When storage driver is cloud storage, it can fail due to various reasons even if the manifest exists
(like 500, 503, etc. from storage server). Currently manifest handler blindly return 404 which can be confusing to user.
This change will return 404 if the manifest blob doesn't exist, and return 500 UnknownError for all other errors (consistent with the behavior of other handlers).
Signed-off-by: Yu Wang (UC) [email protected]