Make sure the error message regarding stub drives is clear #244
Conversation
While we don't support the use-case right now, the error message regarding the lack of free stub drives should be clear for customers. This change also cleans up drive_handler.go. Signed-off-by: Kazuyoshi Kato <[email protected]>
runtime/service_integ_test.go
Outdated
|
|
||
| // When we reuse a VM explicitly, we cannot start multiple containers unless we pre-allocate stub drives. | ||
| _, err = c2.NewTask(ctx, cio.NewCreator(cio.WithStreams(nil, &stdout, &stderr))) | ||
| assert.Equal("failed to patch stub drive: There are no remaining drives to be used: unknown", err.Error()) |
There was a problem hiding this comment.
This unknown is a bit annoying. Should we return one of containerd's pre-defined errors such as ErrUnavailable?
There was a problem hiding this comment.
Yeah I think returning ErrUnavailable is a great idea
runtime/service_integ_test.go
Outdated
|
|
||
| // When we reuse a VM explicitly, we cannot start multiple containers unless we pre-allocate stub drives. | ||
| _, err = c2.NewTask(ctx, cio.NewCreator(cio.WithStreams(nil, &stdout, &stderr))) | ||
| assert.Equal("failed to patch stub drive: There are no remaining drives to be used: unknown", err.Error()) |
There was a problem hiding this comment.
Yeah I think returning ErrUnavailable is a great idea
Signed-off-by: Kazuyoshi Kato <[email protected]>
The number of stub drives cannot be changed once a microVM is started. Modeling the limitation on the stub driver interface may prevent programmer errors. Signed-off-by: Kazuyoshi Kato <[email protected]>
|
|
||
| // When we reuse a VM explicitly, we cannot start multiple containers unless we pre-allocate stub drives. | ||
| _, err = c2.NewTask(ctx, cio.NewCreator(cio.WithStreams(nil, &stdout, &stderr))) | ||
| assert.Equal("no remaining stub drives to be used: unavailable: unknown", err.Error()) |
There was a problem hiding this comment.
Did changing to use ErrUnavailable not result in this error message being changed? I wonder if ErrUnavailable is getting lost somewhere in the call chain (as it goes client->ctrd->shimstart->fc-control->shim and back).
The updates in this PR LGTM anyways, the error message is still more clear and the code is more clear now too. I don't personally mind if dealing with unknown is a separate, not high-priority issue.
There was a problem hiding this comment.
no remaining stub drives to be used: unavailable: unknown
This part is coming from https://github.com/containerd/containerd/blob/ea13c9fe9941b0714630aa614ef5a0038c0083a3/errdefs/errors.go#L48, but maybe I should check the type rather than its string representation coming from Error() to make the intention more clear.
There was a problem hiding this comment.
Ah okay, totally missed the "unavailable"! Makes sense
Signed-off-by: Kazuyoshi Kato [email protected]
Issue #, if available:
Related, but doesn't fix #243
Description of changes:
While we don't support the use-case right now, the error message
regarding the lack of free stub drives should be clear for customers.
This change also cleans up drive_handler.go.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.