Add logs on docker build failure

[palfrey]

Which issue(s) does this change fix?

#7671

Why is this change necessary?

So we can actually debug Docker build issues

How does it address the issue?

By adding logs :)

What side effects does this change have?

None

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[jonife]

When writing to _stream_writer, I think checking it should be clear what type of output is shown. I think we should include context to indicate whether it is part of a standard log or an error message, such as prepending "[Error]: " when writing log["error"]. I was thinking

except docker.errors.BuildError as ex:
    LOG.error("Failed building function %s", function_name)
    for log in ex.build_log:
        if "stream" in log:
            self._stream_writer.write_str(f"Log: {log['stream']}")
        elif "error" in log:
            self._stream_writer.write_str(f"[Error]: {log['error']}")
        else:
            LOG.warning("Unexpected log entry format: %s", log)

[roger-zhangg]

Seems we have a very similar function here

aws-sam-cli/samcli/lib/build/app_builder.py

Line 443 in a18782a

self._stream_lambda_image_build_logs(build_logs, function_name)

I wonder if it's possible to reuse this function in this place

Suggested change

	for log in ex.build_log:
	if "stream" in log:
	self._stream_writer.write_str(log["stream"])
	elif "error" in log:
	self._stream_writer.write_str(log["error"])
	self._stream_lambda_image_build_logs(ex.build_log, function_name)

[palfrey]

Looked at that initially, but rejected because the _stream_lambda_image_build_logs uses LogStreamer._stream_write which will throw a LogStreamError if it sees an error rather than logging it. Provided the logs are always of the form <several 'stream' errors>,<one 'error' log> this will work, otherwise not.

This is incompatible with @jonife's suggestion from above BTW. Which one should I build?

[roger-zhangg]

Thanks for the clarification. Yes you are right. This function won't work on logs that has multiple error. Maybe we can consider adding a param in LogStreamer to suppress the error. In this way it will also address @jonife concerns

[palfrey]

Adding the param would work, I'll have a look at that. It won't address @jonife's concerns though, as LogStreamer doesn't write anything about what sort of log it is. My experiments with it so far indicate that for the usual Docker log patterns it literally does what my patch did (minus the error problems).

[roger-zhangg]

@palfrey Thanks for implementing the change! FYI here I believe this is a vaild test failure in CI, please check

FAILED tests/unit/lib/build_module/test_app_builder.py::TestApplicationBuilder_build_lambda_image_function::test_can_raise_build_error - AssertionError: [call('Some earlier log'), call(''), call('Build failed'), call('\r\n')] != [call('Some earlier log'), call(''), call('Build failed'), call('\n')]

[palfrey]

@palfrey Thanks for implementing the change! FYI here I believe this is a vaild test failure in CI, please check

FAILED tests/unit/lib/build_module/test_app_builder.py::TestApplicationBuilder_build_lambda_image_function::test_can_raise_build_error - AssertionError: [call('Some earlier log'), call(''), call('Build failed'), call('\r\n')] != [call('Some earlier log'), call(''), call('Build failed'), call('\n')]

Looks like a Windows-specific line-ending failure. Got a probable fix, but no local Windows machine so will have to rely on CI to test this one.

[mndeveci]

When we write this to the error stream, shouldn't we also throw it again? Changing this variable into opposite boolean to indicate whether an error should be written into stream or not could be better 🤔

[palfrey]

No, because you might have multiple error lines. Also the throw in this features case is with the whole Docker build exception as the content vs just an individual error line.

[roger-zhangg]

LGTM ❤️

[jonife]

Thank you for your contribution

[palfrey]

So how does this get final approval? Anyone have any sense of the timelines on this one?