Skip to content
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.

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

api: fix description about logs #38819

Merged
merged 1 commit into from Apr 3, 2019

Conversation

Projects
None yet
4 participants
@AkihiroSuda
Copy link
Member

AkihiroSuda commented Mar 2, 2019

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

The documentation says http hijack is used for follow=1 but it doesn't seem actually used:

$ docker run -d --name foo busybox sh -c "while sleep 1; do date; done"
$ sudo curl --include -sN --unix-socket /var/run/docker.sock 'http://docker/containers/foo/logs?stdout=1&stderr=1&follow=1' | hexdump -C
00000000  48 54 54 50 2f 31 2e 31  20 32 30 30 20 4f 4b 0d  |HTTP/1.1 200 OK.|
00000010  0a 41 70 69 2d 56 65 72  73 69 6f 6e 3a 20 31 2e  |.Api-Version: 1.|
00000020  34 30 0d 0a 44 6f 63 6b  65 72 2d 45 78 70 65 72  |40..Docker-Exper|
00000030  69 6d 65 6e 74 61 6c 3a  20 74 72 75 65 0d 0a 4f  |imental: true..O|
00000040  73 74 79 70 65 3a 20 6c  69 6e 75 78 0d 0a 53 65  |stype: linux..Se|
00000050  72 76 65 72 3a 20 44 6f  63 6b 65 72 2f 6d 61 73  |rver: Docker/mas|
00000060  74 65 72 2d 64 6f 63 6b  65 72 70 72 6f 6a 65 63  |ter-dockerprojec|
00000070  74 2d 32 30 31 39 2d 30  32 2d 32 36 20 28 6c 69  |t-2019-02-26 (li|
00000080  6e 75 78 29 0d 0a 44 61  74 65 3a 20 53 61 74 2c  |nux)..Date: Sat,|
00000090  20 30 32 20 4d 61 72 20  32 30 31 39 20 30 38 3a  | 02 Mar 2019 08:|
000000a0  31 30 3a 31 32 20 47 4d  54 0d 0a 54 72 61 6e 73  |10:12 GMT..Trans|
000000b0  66 65 72 2d 45 6e 63 6f  64 69 6e 67 3a 20 63 68  |fer-Encoding: ch|
000000c0  75 6e 6b 65 64 0d 0a 0d  0a 01 00 00 00 00 00 00  |unked...........|
000000d0  1d 53 61 74 20 4d 61 72  20 20 32 20 30 38 3a 30  |.Sat Mar  2 08:0|
000000e0  38 3a 34 32 20 55 54 43  20 32 30 31 39 0a 01 00  |8:42 UTC 2019...|
000000f0  00 00 00 00 00 1d 53 61  74 20 4d 61 72 20 20 32  |......Sat Mar  2|
00000100  20 30 38 3a 30 38 3a 34  33 20 55 54 43 20 32 30  | 08:08:43 UTC 20|
00000110  31 39 0a 01 00 00 00 00  00 00 1d 53 61 74 20 4d  |19.........Sat M|

Also, follow=0 produces the same binary stream except that the connection is closed after returning the latest buffer.

@cpuguy83 @thaJeztah PTAL.
Not sure the docs were wrong from the birth or the implementation was changed later.

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 2, 2019

is it used if there's a tty attached to the container?

@AkihiroSuda

This comment has been minimized.

Copy link
Member Author

AkihiroSuda commented Mar 2, 2019

Doesn't seem so

@thaJeztah

This comment has been minimized.

Copy link
Member

thaJeztah commented Mar 2, 2019

I'll defer this one to @cpuguy83 as I think he's more familiar with this

@AkihiroSuda AkihiroSuda force-pushed the AkihiroSuda:fix-logs-docs branch from b8e56f1 to 6f12041 Mar 2, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Mar 2, 2019

Codecov Report

❗️ No coverage uploaded for pull request base (master@32157f9). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #38819   +/-   ##
=========================================
  Coverage          ?    36.9%           
=========================================
  Files             ?      614           
  Lines             ?    45409           
  Branches          ?        0           
=========================================
  Hits              ?    16758           
  Misses            ?    26363           
  Partials          ?     2288
@AkihiroSuda

This comment has been minimized.

Copy link
Member Author

AkihiroSuda commented Mar 14, 2019

@cpuguy83 could you take a look?

@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Mar 14, 2019

So tty is not multiplexed (because there is only one stream).
All other cases is multiplexed.

Correct, there is no upgrade or hijack, this is only needed for attach with stdin.

Show resolved Hide resolved client/container_logs.go Outdated
@cpuguy83

This comment has been minimized.

Copy link
Contributor

cpuguy83 commented Mar 29, 2019

Loos good except that the streams are not multiplexed with tty.

api: fix description about logs
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

@AkihiroSuda AkihiroSuda force-pushed the AkihiroSuda:fix-logs-docs branch from 6f12041 to d2281bb Apr 2, 2019

@AkihiroSuda

This comment has been minimized.

Copy link
Member Author

AkihiroSuda commented Apr 2, 2019

thanks, updated

@cpuguy83
Copy link
Contributor

cpuguy83 left a comment

LGTM

@AkihiroSuda

This comment has been minimized.

Copy link
Member Author

AkihiroSuda commented Apr 3, 2019

Z failure is unrelated

@thaJeztah
Copy link
Member

thaJeztah left a comment

I'll rely on @cpuguy83 here; LGTM (thanks!)

@thaJeztah thaJeztah merged commit a2fdfaa into moby:master Apr 3, 2019

9 checks passed

codecov/patch Coverage not affected.
Details
codecov/project No report found to compare against
Details
dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 44711 has succeeded
Details
janky Jenkins build Docker-PRs 53515 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 13876 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 24641 has succeeded
Details
windowsRS5-process Jenkins build Docker-PRs-WoW-RS5-Process 1963 has succeeded
Details
z Jenkins build Docker-PRs-s390x 13807 has succeeded
Details

@thaJeztah thaJeztah added the area/api label Apr 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.