fix: Remove attach_api and replace with docker library calls #1552
Conversation
|
|
||
| # Fetch both stdout and stderr streams from Docker as a single iterator. | ||
| logs_itr = attach(self.docker_client, container=real_container, stdout=True, stderr=True, logs=True) | ||
| logs_itr = real_container.attach(stream=True, logs=False, demux=True) |
There was a problem hiding this comment.
Is this with the latest version of docker? do we need any version bumps?
There was a problem hiding this comment.
This was released in 3.7 (looking at docker/docker-py#1952). We are on 4.0 or greater, so we should be covered and not need any bump.
|
|
||
| # Fetch both stdout and stderr streams from Docker as a single iterator. | ||
| logs_itr = attach(self.docker_client, container=real_container, stdout=True, stderr=True, logs=True) | ||
| logs_itr = real_container.attach(stream=True, logs=False, demux=True) |
There was a problem hiding this comment.
Setting logs=False changes the previous behaviour. Is this intentional?
There was a problem hiding this comment.
@thomas-fossati What behavior. I have asked a similar question on #1566. It is not clear to me what you are @setrofim are referring to. Please be more specific.
There was a problem hiding this comment.
Apologies for the extreme terseness of my previous comment.
If the lambda business logics completes before SAM attaches to the container, the lambda's output will only be available through the logs. So if you don't ask for the logs to be forwarded on attach, you might end up seeing no output, including the bits that SAM cares about (ie., { "status": ... }) and get stuck.
There was a problem hiding this comment.
@setrofim had a small back and forth here: #1566 (comment) I think I understand the reasoning more.
It was intentionally False because I took this from the 'warm container' PR in which we do not want all the logs from the container. Since we are not in that mode yet, making this True makes sense to me. We will need to filter logs by invocation for the warm container case but we can tackle all that at once.
There was a problem hiding this comment.
Sounds like a good plan to me, thanks!
3 participants
Issue #, if available:
Description of changes:
Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.