Add support for logging binary by anmaxvl · Pull Request #896 · microsoft/hcsshim

anmaxvl · 2020-11-18T06:34:21Z

Summary

This PR adds logging binary support (like in containerd/containerd#3085).

Just like in the above PR:

add URI parsing logic to hcsshim
start a process for logging driver and redirect all container IO through named pipes

Flow

The flow (assuming that jterry75/cri#82 is also merged) as follows:

start pod
create container inside that pod and make sure to include the log_path config:
```
...
"log_path": "binary:///path/to/binary.exe?foo&bar=baz"
...
```
NOTE that log_path needs to be a valid url (i.e. "binary:\\\\\\path\\to\\binary.exe" won't work, since url.Parse will fail)
start container

You'll notice that ctr-x-stdout and ctr-x-stderr pipes won't be created, binary-x-stdout and binary-x-stderr will replace them.
This will break the previous "feature" of being able to set log_path and do crictl attach at the same time.

TODO

make sure if wait channel is necessary, since earlier testing was failing to run the binary
tests

Signed-off-by: Maksim An [email protected]

cmd/containerd-shim-runhcs-v1/log.go

cmd/logging/logging.go

internal/cmd/io_binary.go

cmd/logging/logging.go

cmd/containerd-shim-runhcs-v1/log.go

cmd/containerd-shim-runhcs-v1/task_hcs.go

dcantah · 2020-11-18T07:26:21Z

This is still a draft but a quick summary of the entire flow for this work/anything extra that you think would be helpful to know for review of this would be good to have 😃

dcantah · 2020-11-18T23:44:43Z

@anmaxvl Thanks for the summary :) By the way, does this need to be a draft? Is there more coming and this is just to get feedback early or?

cmd/logging/logging.go

internal/cmd/io_binary.go

anmaxvl · 2020-11-19T01:12:04Z

@anmaxvl Thanks for the summary :) By the way, does this need to be a draft? Is there more coming and this is just to get feedback early or?

for early feedback and thanks for giving one! still need to validate the wait channel and add tests

logging/logging.go

internal/cmd/io_binary.go

test/cri-containerd/helpers/log.go

internal/cmd/io.go

internal/cmd/io_binary.go

test/cri-containerd/runpodsandbox_test.go

test/cri-containerd/helpers/log.go

test/cri-containerd/logging_binary_test.go

katiewasnothere · 2020-12-18T01:00:45Z

LOVE the tests. Just a few really minor questions/comments :)

appveyor.yml

internal/cmd/io_binary.go

katiewasnothere

Couple of comments but overall LGTM :)

Add integrations tests Signed-off-by: Maksim An <[email protected]>

Related work items: microsoft#173, microsoft#839, microsoft#856, microsoft#877, microsoft#881, microsoft#886, microsoft#887, microsoft#888, microsoft#889, microsoft#890, microsoft#893, microsoft#894, microsoft#896, microsoft#899, microsoft#900, microsoft#902, microsoft#904, microsoft#905, microsoft#906, microsoft#907, microsoft#908, microsoft#910, microsoft#912, microsoft#913, microsoft#914, microsoft#916, microsoft#918, microsoft#923, microsoft#925, microsoft#926, microsoft#928, microsoft#929, microsoft#932, microsoft#933, microsoft#934, microsoft#938, microsoft#939, microsoft#942, microsoft#943, microsoft#945, microsoft#946, microsoft#947, microsoft#949, microsoft#951, microsoft#952, microsoft#954

Logging binary support and integration tests Signed-off-by: Maksim An <[email protected]>

anmaxvl requested a review from a team November 18, 2020 06:34

anmaxvl mentioned this pull request Nov 18, 2020

Add support for binary logger jterry75/cri#82

Merged