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

Add support for logging binary #896

Merged

Conversation

anmaxvl
Copy link
Contributor

@anmaxvl anmaxvl commented Nov 18, 2020

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]

@anmaxvl anmaxvl requested a review from a team November 18, 2020 06:34
cmd/logging/logging.go Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
cmd/logging/logging.go Outdated Show resolved Hide resolved
@dcantah
Copy link
Contributor

dcantah commented Nov 18, 2020

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 😃

@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch 5 times, most recently from ee53852 to eeff4aa Compare November 18, 2020 17:18
@dcantah
Copy link
Contributor

dcantah commented Nov 18, 2020

@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 Outdated Show resolved Hide resolved
cmd/logging/logging.go Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch from eeff4aa to 0a3af6f Compare November 19, 2020 01:07
@anmaxvl
Copy link
Contributor Author

anmaxvl commented Nov 19, 2020

@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 Outdated Show resolved Hide resolved
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch 3 times, most recently from d7ddf5f to 9e2a1e2 Compare November 23, 2020 07:55
internal/cmd/io_binary.go Outdated Show resolved Hide resolved
@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch 4 times, most recently from ab2654a to 198776f Compare December 10, 2020 21:26
@katiewasnothere
Copy link
Contributor

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

@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch 2 times, most recently from a26974b to 5f5dc90 Compare December 22, 2020 05:07
appveyor.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@katiewasnothere katiewasnothere left a comment

Choose a reason for hiding this comment

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

Couple of comments but overall LGTM :)

@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch from 5f5dc90 to ae06a62 Compare December 22, 2020 21:04
Add integrations tests

Signed-off-by: Maksim An <[email protected]>
@anmaxvl anmaxvl force-pushed the user/maksiman/add-binary-logging-support branch from ae06a62 to 966a054 Compare January 7, 2021 09:54
@anmaxvl anmaxvl merged commit fd21b8d into microsoft:master Jan 7, 2021
@anmaxvl anmaxvl deleted the user/maksiman/add-binary-logging-support branch February 2, 2021 23:18
princepereira pushed a commit to princepereira/hcsshim that referenced this pull request Aug 29, 2024
Logging binary support and integration tests

Signed-off-by: Maksim An <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants