Improve readability of errors#2372
Merged
kolyshkin merged 2 commits intoMay 3, 2020
Merged
Conversation
genericError.Error() was formatting the underlying error using `%q`; as a result, quotes in underlying errors were escaped multiple times, which caused the output to become hard to read, for example (wrapped for readability): ``` container_linux.go:345: starting container process caused "process_linux.go:430: container init caused \"rootfs_linux.go:58: mounting \\\"/foo.txt\\\" to rootfs \\\"/var/lib/docker/overlay2/f49a0ae0ec6646c818dcf05dbcbbdd79fc7c42561f3684fbb1fc5d2b9d3ad192/merged\\\" at \\\"/var/lib/docker/overlay2/f49a0ae0ec6646c818dcf05dbcbbdd79fc7c42561f3684fbb1fc5d2b9d3ad192/merged/usr/share/nginx/html\\\" caused \\\"not a directory\\\"\"": unknown ``` With this patch applied: ``` container_linux.go:348: starting container process caused: process_linux.go:438: container init caused: rootfs_linux.go:58: mounting "/foo.txt" to rootfs "/var/lib/docker/overlay2/de506d67da606b807009e23b548fec60d72359c77eec88785d8c7ecd54a6e4b2/merged" at "/var/lib/docker/overlay2/de506d67da606b807009e23b548fec60d72359c77eec88785d8c7ecd54a6e4b2/merged/usr/share/nginx/html" caused: not a directory: unknown ``` Signed-off-by: Sebastiaan van Stijn <[email protected]>
The error message was including both the rootfs path, and the full mount path, which also includes the path of the rootfs. This patch removes the rootfs path from the error message, as it was redundant, and made the error message overly verbose Before this patch (errors wrapped for readability): ``` container_linux.go:348: starting container process caused: process_linux.go:438: container init caused: rootfs_linux.go:58: mounting "/foo.txt" to rootfs "/var/lib/docker/overlay2/de506d67da606b807009e23b548fec60d72359c77eec88785d8c7ecd54a6e4b2/merged" at "/var/lib/docker/overlay2/de506d67da606b807009e23b548fec60d72359c77eec88785d8c7ecd54a6e4b2/merged/usr/share/nginx/html" caused: not a directory: unknown ``` With this patch applied: ``` container_linux.go:348: starting container process caused: process_linux.go:438: container init caused: rootfs_linux.go:58: mounting "/foo.txt" to rootfs at "/var/lib/docker/overlay2/de506d67da606b807009e23b548fec60d72359c77eec88785d8c7ecd54a6e4b2/merged/usr/share/nginx/html" caused: not a directory: unknown ``` Signed-off-by: Sebastiaan van Stijn <[email protected]>
Member
Author
|
@kolyshkin @AkihiroSuda PTAL I'm also wondering if we could replace these custom error-types with (e.g.) github.com/pkg/errors, and use |
Member
|
LGTM |
Contributor
|
LGTM |
Contributor
+1 to this suggestion @thaJeztah |
Member
|
Yeah, these custom error types were written back before |
Member
Author
|
I started a scratch document to see if anything relies on them specifically (original implementation of the "rich" errors was in docker-archive/libcontainer#185, but looks like it was never really used beyond the initial changes). A quick search on github also didn't reveal active use of them, so looks like it should be able to replace them |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
libcontainer: don't double-quote errors
genericError.Error() was formatting the underlying error using
%q; as aresult, quotes in underlying errors were escaped multiple times, which
caused the output to become hard to read, for example (wrapped for readability):
With this patch applied:
libcontainer: simplify error message
The error message was including both the rootfs path, and the full
mount path, which also includes the path of the rootfs.
This patch removes the rootfs path from the error message, as it
was redundant, and made the error message overly verbose
Before this patch (errors wrapped for readability):
With this patch applied: