Improve exec api

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now

[TomasTomecek]

I'm sorry for creating new PR but git/GitHub went absolutely bananas: gh closed old PR itself (new UI glitch?), messed up commits and refused to do valid rebase.

Old PR: #853

Needed for: d11wtq/dockerpty#30
Transitively for: docker/compose#2023

I have addressed all comments:

• correctly configured garbage collection during integration test
• created new interface for stream/socket response (chose decorator instead of Client method) -- can rewrite if you don't like this (the reason I did it this way is that I didn't like the solution with partial b/c you need to supply more arguments to _get_result_tty than to _get_raw_response_socket)

[shin-]

This line is longer than 80 characters :(

[TomasTomecek]

Regarding the integration test, it works fine for me locally:

platform linux2 -- Python 2.7.10, pytest-2.8.2, py-1.4.30, pluggy-0.3.1
plugins: capturelog-0.7
collected 8 items tests/integration/exec_test.py .

docker 1.9.0

Is this a regression between 1.8 and 1.9?

[shin-]

The integration tests on Jenkins use Docker 1.9.0 as well (see the integration-dind task in Makefile). I also get successes testing outside of dind. I'll take a closer look at the test.

[TomasTomecek]

I think that the issue could be with terminal allocation, since test runner probably doesn't allocate tty in kernel.

Edit: ^ that doesn't make any sense, right? tty is allocated in container and exec api call just attaches to container so client doesn't need to allocate tty in test runner afaik

[TomasTomecek]

After doing some brief testing the issue with integration test seems like race:

First run:

tests/integration/exec_test.py:141: in test_exec_start_socket
self.assertEqual(next_size, len(line))
E AssertionError: 0 != 22

connectionpool.py 383 DEBUG "POST /v1.20/containers/create HTTP/1.1" 201 90
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/start HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/exec HTTP/1.1" 201 74
connectionpool.py 383 DEBUG "POST /v1.20/exec/63e7bcc6574a53c01e493079c51103ca6a8697eea14b821a098384b956224c8d/start HTTP/1.1" 200 None
connectionpool.py 383 DEBUG "POST /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471/stop?t=1 HTTP/1.1" 204 0
connectionpool.py 383 DEBUG "DELETE /v1.20/containers/0d8a59766345c5724aabd31a4aacb259b85cb2be2f9d21bf88a8277ff7369471?link=False&force=False&v=False HTTP/1.1" 204 0

Then I reran the exec suite commenting out self.assertEqual(next_size, len(line)) just to see where it gets:

platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 7 items tests/integration/exec_test.py .....F.
tests/integration/exec_test.py:149: in test_exec_start_socket
self.assertEqual(data.decode('utf-8'), "{0}".format(line))
E AssertionError: u'yay, interactive exec!\r\n' != 'yay, interactive exec!'

And again just a single failing test:

$ py.test tests/integration/exec_test.py::ExecTest::test_exec_start_socket
platform linux2 -- Python 2.7.10 -- py-1.4.30 -- pytest-2.6.4
plugins: capturelog
collected 8 items tests/integration/exec_test.py .

And now it works for me locally.

EDIT: passes now even locally in dind.
EDIT2: when I run the whole suite I get the error from CI, so it looks like that tests affect each other: does requests cache the connection?

[TomasTomecek]

Okay, so this is a python 3 only issue.

[TomasTomecek]

Done!

It was a race. Command in container didn't finish before tests tried to fetch results. Simple select([socket], [], []) solves it.

@shin- PTAL

[TomasTomecek]

• got rid of the decorator
• moved import select to top

[aanand]

This is a bit harder to read than it needs to be. How about just:

ifsocket:
stream=True

Also, this is just a personal preference, but I prefer to modify arguments at the very top of a function, so it's all in one place, so ideally this would go above the bit where we initialise data.

[TomasTomecek]

@aanand thanks for the review. As I said previously, this code is copy&pasted from attach container integration test. So I find it pretty funny that you are commenting on a code which is already in.

I'm assuming that I should make one helper function and use it in both codebases, right?

[TomasTomecek]

@aanand very good suggestions! code for tests looks a lot cleaner now, please take a look.

[TomasTomecek]

fixed the flake; rebased

[aanand]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied. Your suggestion was a good one and the tests indeed look much better now, so thanks!

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

[TomasTomecek]

Apologies, I hadn't looked at the codebase in a while so I didn't realise it was copied.

No worries.

I'd like to make one more suggestion, assuming it's possible - can the select call be moved to the top of the read_socket function?

Done.

[aanand]

LGTM

[dnephin]

LGTM

[TomasTomecek]

\o/
dockerpty now