Wait for mount command to finish when mounting volume
#4305
Conversation
This file contains 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
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon 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 |
openshift-ci-robot
added
approved
|
Still needs a test to verify we're correctly handling things. |
rhatdan
reviewed
Oct 20, 2019
| logrus.Debugf("Running mount command: %s %s", mountPath, strings.Join(mountArgs, " ")) | ||
| if output, err := mountCmd.CombinedOutput(); err != nil { | ||
| logrus.Debugf("Mount failed with %v", err) | ||
| return errors.Wrapf(errors.Errorf(string(output)), "error mounting volume %s", v.Name()) |
There was a problem hiding this comment.
I think you should wrap the err, and then put the string(output) into the extra information.
return errors.Wrapf(err, "error mounting volume %s: %v", v.Name(),errors.Errorf(string(output)))
|
The error is just the exit code - the actual text of the error is
CombinedOutput(), which gets us STDERR for mount. It's a little verbose but
a lot better than "exit status 32" for a printed error.
…On Sun, Oct 20, 2019, 07:14 Daniel J Walsh ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In libpod/volume_internal_linux.go
<#4305 (comment)>:
> @@ -72,16 +72,10 @@ func (v *Volume) mount() error {
mountArgs = append(mountArgs, volDevice, v.config.MountPoint)
mountCmd := exec.Command(mountPath, mountArgs...)
- errPipe, err := mountCmd.StderrPipe()
- if err != nil {
- return errors.Wrapf(err, "error getting stderr pipe for mount")
- }
- if err := mountCmd.Start(); err != nil {
- out, err2 := ioutil.ReadAll(errPipe)
- if err2 != nil {
- return errors.Wrapf(err2, "error reading mount STDERR")
- }
- return errors.Wrapf(errors.New(string(out)), "error mounting volume %s", v.Name())
+ logrus.Debugf("Running mount command: %s %s", mountPath, strings.Join(mountArgs, " "))
+ if output, err := mountCmd.CombinedOutput(); err != nil {
+ logrus.Debugf("Mount failed with %v", err)
+ return errors.Wrapf(errors.Errorf(string(output)), "error mounting volume %s", v.Name())
I think you should wrap the err, and then put the string(output) into the
extra information.
return errors.Wrapf(err, , "error mounting volume %s: %v", v.Name(),errors.Errorf(string(output)))
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#4305>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCALYYZHV35ATHX2XQDQPQ4PNANCNFSM4JCSAEGQ>
.
|
|
Well 32 is That way the caller could at least check for errors.Cause() on the error. |
|
did you add the test? is this ready for merge? |
|
Not yet. Will try and get to it today. |
mheon
force-pushed
the
fix_volume_mount
branch
from
5e1981e to
7434fa7
Compare
|
Test added |
openshift-ci-robot
added
the
needs-rebase
mheon
force-pushed
the
fix_volume_mount
branch
from
7434fa7 to
49c939f
Compare
openshift-ci-robot
removed
the
needs-rebase
|
☔ The latest upstream changes (presumably #4298) made this pull request unmergeable. Please resolve the merge conflicts. |
|
rebase and lets merge! |
mheon
force-pushed
the
fix_volume_mount
branch
from
49c939f to
7799aba
Compare
|
Rebased |
command.Start() just starts the command. That catches some errors, but the nasty ones - bad options and similar - happen when the command runs. Use CombinedOutput() instead - it waits for the command to exit, and thus catches non-0 exit of the `mount` command (invalid options, for example). STDERR from the `mount` command is directly used, which isn't necessarily the best, but we can't really get much more info on what went wrong. Fixes containers#4303 Signed-off-by: Matthew Heon <[email protected]>
mheon
force-pushed
the
fix_volume_mount
branch
from
7799aba to
3e891c1
Compare
|
CI should go green |
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/lgtm |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
command.Start() just starts the command. That catches some errors, but the nasty ones - bad options and similar - happen when the command runs. Use CombinedOutput() instead - it waits for the command to exit, and thus catches non-0 exit of the
mountcommand (invalid options, for example).STDERR from the
mountcommand is directly used, which isn't necessarily the best, but we can't really get much more info on what went wrong.Fixes #4303