Skip to content

fix: Remove attach_api and replace with docker library calls #1552

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jfuss merged 5 commits into from
Nov 19, 2019
Merged

fix: Remove attach_api and replace with docker library calls #1552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
546a41b
fix: Remove attach_api and replace with docker library calls
jfuss Nov 19, 2019
6b606e2
Fix tests on stdout/stderr being null
jfuss Nov 19, 2019
615bbf7
compare json data instead of json strings
jfuss Nov 19, 2019
309d5c7
More test adjusting
jfuss Nov 19, 2019
13f3b73
format with black
jfuss Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 0 additions & 178 deletions samcli/local/docker/attach_api.py
View file
Open in desktop

This file was deleted.

29 changes: 8 additions & 21 deletions samcli/local/docker/container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import docker

from samcli.local.docker.attach_api import attach
from .utils import to_posix_path

LOG = logging.getLogger(__name__)
Expand Down Expand Up @@ -200,7 +199,7 @@ def wait_for_logs(self, stdout=None, stderr=None):
real_container = self.docker_client.containers.get(self.id)

# 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)
Copy link
Contributor

@sriram-mv sriram-mv Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this with the latest version of docker? do we need any version bumps?

Copy link
Contributor Author

@jfuss jfuss Nov 19, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@thomas-fossati thomas-fossati Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting logs=False changes the previous behaviour. Is this intentional?

Copy link
Contributor Author

@jfuss jfuss Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

@thomas-fossati thomas-fossati Nov 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jfuss jfuss Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

@thomas-fossati thomas-fossati Nov 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good plan to me, thanks!


self._write_container_output(logs_itr, stdout=stdout, stderr=stderr)

Expand Down Expand Up @@ -238,25 +237,13 @@ def _write_container_output(output_itr, stdout=None, stderr=None):
Stream writer to write stderr data from the Container into
"""

# Iterator returns a tuple of (frame_type, data) where the frame type determines which stream we write output
# to
for frame_type, data in output_itr:

if frame_type == Container._STDOUT_FRAME_TYPE and stdout:
# Frame type 1 is stdout data.
stdout.write(data)

elif frame_type == Container._STDERR_FRAME_TYPE and stderr:
# Frame type 2 is stderr data.
stderr.write(data)

else:
# Either an unsupported frame type or stream for this frame type is not configured
LOG.debug(
"Dropping Docker container output because of unconfigured frame type. " "Frame Type: %s. Data: %s",
frame_type,
data,
)
# Iterator returns a tuple of (stdout, stderr)
for stdout_data, stderr_data in output_itr:
if stdout_data and stdout:
stdout.write(stdout_data)

if stderr_data and stderr:
stderr.write(stderr_data)

@property
def network_id(self):
Expand Down
17 changes: 12 additions & 5 deletions tests/integration/local/start_lambda/test_start_lambda.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
from concurrent.futures import ThreadPoolExecutor, as_completed
from time import time
import json

import pytest

Expand Down Expand Up @@ -159,13 +160,19 @@ def test_invoke_with_invocation_type_RequestResponse(self):
@pytest.mark.timeout(timeout=300, method="thread")
def test_lambda_function_raised_error(self):
response = self.lambda_client.invoke(FunctionName="RaiseExceptionFunction", InvocationType="RequestResponse")
response_data = json.loads(response.get("Payload").read().decode("utf-8"))

print(response_data)

self.assertEqual(
response.get("Payload").read().decode("utf-8"),
'{"errorMessage": "Lambda is raising an exception", '
'"errorType": "Exception", '
'"stackTrace": [["/var/task/main.py", 48, "raise_exception", '
'"raise Exception(\\"Lambda is raising an exception\\")"]]}',
response_data,
{
"errorMessage": "Lambda is raising an exception",
"errorType": "Exception",
"stackTrace": [
' File "/var/task/main.py", line 48, in raise_exception\n raise Exception("Lambda is raising an exception")\n'
],
},
)
self.assertEqual(response.get("FunctionError"), "Unhandled")
self.assertEqual(response.get("StatusCode"), 200)
Expand Down
29 changes: 8 additions & 21 deletions tests/unit/local/docker/test_container.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,26 +435,23 @@ def setUp(self):

self.container.is_created = Mock()

@patch("samcli.local.docker.container.attach")
def test_must_fetch_stdout_and_stderr_data(self, attach_mock):
def test_must_fetch_stdout_and_stderr_data(self):

self.container.is_created.return_value = True

real_container_mock = Mock()
self.mock_docker_client.containers.get.return_value = real_container_mock

output_itr = Mock()
attach_mock.return_value = output_itr
real_container_mock.attach.return_value = output_itr
self.container._write_container_output = Mock()

stdout_mock = Mock()
stderr_mock = Mock()

self.container.wait_for_logs(stdout=stdout_mock, stderr=stderr_mock)

attach_mock.assert_called_with(
self.mock_docker_client, container=real_container_mock, stdout=True, stderr=True, logs=True
)
real_container_mock.attach.assert_called_with(stream=True, logs=False, demux=True)
self.container._write_container_output.assert_called_with(output_itr, stdout=stdout_mock, stderr=stderr_mock)

def test_must_skip_if_no_stdout_and_stderr(self):
Expand All @@ -472,17 +469,7 @@ def test_must_raise_if_container_is_not_created(self):

class TestContainer_write_container_output(TestCase):
def setUp(self):
self.output_itr = [
(Container._STDOUT_FRAME_TYPE, b"stdout1"),
(Container._STDERR_FRAME_TYPE, b"stderr1"),
(30, b"invalid1"),
(Container._STDOUT_FRAME_TYPE, b"stdout2"),
(Container._STDERR_FRAME_TYPE, b"stderr2"),
(30, b"invalid2"),
(Container._STDOUT_FRAME_TYPE, b"stdout3"),
(Container._STDERR_FRAME_TYPE, b"stderr3"),
(30, b"invalid3"),
]
self.output_itr = [(b"stdout1", None), (None, b"stderr1"), (b"stdout2", b"stderr2"), (None, None)]

self.stdout_mock = Mock()
self.stderr_mock = Mock()
Expand All @@ -492,15 +479,15 @@ def test_must_write_stdout_and_stderr_data(self):

Container._write_container_output(self.output_itr, stdout=self.stdout_mock, stderr=self.stderr_mock)

self.stdout_mock.write.assert_has_calls([call(b"stdout1"), call(b"stdout2"), call(b"stdout3")])
self.stdout_mock.write.assert_has_calls([call(b"stdout1"), call(b"stdout2")])

self.stderr_mock.write.assert_has_calls([call(b"stderr1"), call(b"stderr2"), call(b"stderr3")])
self.stderr_mock.write.assert_has_calls([call(b"stderr1"), call(b"stderr2")])

def test_must_write_only_stdout(self):

Container._write_container_output(self.output_itr, stdout=self.stdout_mock, stderr=None)

self.stdout_mock.write.assert_has_calls([call(b"stdout1"), call(b"stdout2"), call(b"stdout3")])
self.stdout_mock.write.assert_has_calls([call(b"stdout1"), call(b"stdout2")])

self.stderr_mock.write.assert_not_called() # stderr must never be called

Expand All @@ -511,7 +498,7 @@ def test_must_write_only_stderr(self):

self.stdout_mock.write.assert_not_called()

self.stderr_mock.write.assert_has_calls([call(b"stderr1"), call(b"stderr2"), call(b"stderr3")])
self.stderr_mock.write.assert_has_calls([call(b"stderr1"), call(b"stderr2")])


class TestContainer_image(TestCase):
Expand Down