lxd: catch CalledProcessError in _container_run

[kalikiana]

• Have you followed the guidelines for contributing?
• Have you signed the CLA?
• If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
• If this is a new feature. Have you discussed the design on the forum?
• Have you successfully run ./runtests.sh static?
• Have you successfully run ./runtests.sh unit?

---

Show a nice, clean error if a command fails in the container.

[sergiusens]

Given that we sort of want to be transparent, the error that showed up when running in the container is (or probably should be) the actual error.

Unless it is an error related to the handling of containers specificallly (lxd), then we don't want to add another error message. This could be captured by our exit handler to have an error code (maybe a new one), but not show a new error message.

[sergiusens]

talk to @kyrofa about this to see what he thinks on how to implement this.

[kyrofa]

So here's how it works right now:

We run a command in the container via subprocess.check_call. The command exits non-zero, and thus causes the check_call to raise. However, check_call doesn't capture anything-- no stdout, no stderr. It just prints to the screen. So by the time we're handling the error, it's already sitting there on the screen and we don't know exactly what it was.

I suggest refactoring this a bit. At the very least, I say we start capturing stderr, so when we fail we can say "hey yo, this command failed: <stderr>"

To do that, I suggest moving to subprocess.run with with check=True and stderr set to subprocess.PIPE. Then when an exception is thrown, we have stderr available to us instead of already sitting on the screen.

Thoughts?

[kyrofa]

(It's worth noting that in at least the setup code, stdout isn't all that helpful either and could be reasonably replaced with a progress bar, but we use the same codepath for everything. That refactor would probably be another PR, but I think it would be ideal.)

[sergiusens]

On vie, oct 27, 2017 at 12:39 PM, Kyle Fazzari ***@***.***> wrote: So here's how it works right now: We run a command in the container via subprocess.check_call. The command exits non-zero, and thus causes the check_call to raise. However, check_call doesn't capture anything-- no stdout, no stderr. It just prints to the screen. So by the time we're handling the error, it's already sitting there on the screen and we don't know exactly what it was. I suggest refactoring this a bit. At the very least, I say we start capturing stderr, so when we fail we can say "hey yo, this command failed: " To do that, I suggest moving to subprocess.run with with check=True and stderr set to subprocess.PIPE. Then when an exception is thrown, we have stderr. Thoughts?

The error is already thrown in the snapcraft running in the container, I am trying to avoid a double error and just exit with a proper exit code indicating there was an error but no message, as from the point of view of a user the error is already displayed.

…

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[kyrofa]

Well, not during setup anyway, which is what caused this PR to come into being, right? During setup these are e.g. snap install commands.

But you're right, they're currently sitting on screen. So would you prefer simply raising an exception with no message, then? That leaves the error white, which isn't the most consistent UX.

[kyrofa]

So I see three options here:

1. What we're doing: let the error print, then say "hey we had an error" and exit
2. Let the error print, then exit non-zero with no error message
3. Capture the error rather than letting it print, then exit saying "here's what happened"

I was advocating for (3), but @sergiusens it sounds like you would prefer (2). Is that correct?

[sergiusens]

On vie, oct 27, 2017 at 1:14 PM, Kyle Fazzari ***@***.***> wrote: So I see three options here: What we're doing: let the error print, then say "hey we had an error" and exit Let the error print, then exit non-zero with no error message Capture the error rather than letting it print, then exit saying "here's what happened" I was advocating for (3), but @sergiusens it sounds like you would prefer (2). Is that correct?

Yes, +1 on 2 for when errors happen within the container run. A specific exception to track might be best so the handler can treat this correctly and we don't accidentally put all of these in the potential same future basket

…

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[kyrofa]

Okay we hammered this out in IRC, so to summarize: the disagreement stemmed from the fact that the bug this is fixing is caused by the setup code (not the nested snapcraft), but this fix will double-up errors from the nested snapcraft, which already has error handling built in. This is because the exception handling being added here is to _container_run, which is used for every command run in the container, be that setup commands or running the nested snapcraft itself.

So here's the new proposal:

Implement a logical split between the error handling from commands that are setting up the container (that don't involve snapcraft) and the commands that are essentially re-executing snapcraft in the container. Since both paths use _container_run, it cannot contain the error handling.

1. Errors encountered when running setup commands should be handled as we're doing here.
2. Errors encountered when running the nested snapcraft should have already been properly handled and messaged by the nested snapcraft, so all we need to do is exit. We can do that by raising a SnapcraftError-based exception whose fmt is an empty string (one carefully documented and used just for this purpose).

Sound about right?

[sergiusens]

@kyrofa almost, that will print an empy line always, I would change

    exit_code = 1
    is_snapcraft_error = issubclass(
        exception_type, snapcraft.internal.errors.SnapcraftError)

    if debug or not is_snapcraft_error:
        traceback.print_exception(
            exception_type, exception, exception_traceback)

    if is_snapcraft_error:
        exit_code = exception.get_exit_code()
        if not debug:
            echo.error(str(exception))

    sys.exit(exit_code)

and enhance is_snapcraft_error to also determine, is_snapcraft_printable_error and check for that instead of if not debug

[sergiusens]

replacing with #1647