fix: collect prior output from attached containers

[setrofim]

When attaching to a docker container, specify logs=True in order to
receive the output the container has generated prior to it being
attached.

Issue #, if available:

Related to #1551

PR #1552 switched to using Docker API for attaching to containers. However, it is invoked with logs=False so that only the output generated by the container after attaching is collected. This means that some initial output may get lost.

Description of changes:

Specify logs=True when attaching to a container to ensure the complete output is collected.

Checklist:

• Write Design Document (Do I need to write a design document?)
• Write unit tests
• Write/update functional tests
• Write/update integration tests
• make pr passes
• Write documentation

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

[jfuss]

@setrofim Can you be more specific on the output that is lost? I am not sure what you are referring to.

[setrofim]

@jfuss The output between when the container is stared by the container_manager inside invoke() and is attached to inside wait_for_logs(). Not much happens between these two points, but it seems that occasionally, the attach happens too late, and we miss output from some of our lambdas, resulting in intermittent failures (basically, the same symptoms as in #1551, but due to a different timing issue). Setting logs=True ensures that we collect output from the start of the container, regardless of the timing of the attach().

[jfuss]

@setrofim Makes sense. I don't know if we really want to do logs=True. I took this from another PR I had open for warm containers #1305. In that case, we actually do not want all the logs from the container.

I initially made the change in #1552 thinking the bug was within our attach implementation and could instead just use dockers. As you and @thomas-fossati have pointed out, this is actually not the case. Instead of just introducing another failure/case into the mix, I am going to revert the #1552 PR so it does not get released. I want to do a little deeper understanding of these things to make a better call on next steps.

Really appreciate the dive you guys have done here! It is very helpful.

[setrofim]

FWIW, I have submitted a PR to docker with a proposed fix for this on their end: docker/docker-py#2467

[jfuss]

@setrofim That is awesome! I think the part we are missing is timestamp information, so we can either tell docker-py to filter logs or do it ourselves. It looks like this should be possible, given the attach() method is a wrapper around logs(). So maybe the right thing here is to actually update to logs=True now and just fix the timestamp related concern when we finish up the 'warm container' pr.

Sorry thinking out loud/as I type.