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 customer logging support #75

Open
wants to merge 2 commits into
base: windows_port
Choose a base branch
from

Conversation

worldhello126
Copy link

The change add customer logging support for cri container in windows platform. it extend the existing log_path property of the container definition. and make it support binary protocal (containerd have similar impementaion in linux plat).

for example now you can define in your contianerdef.json file
"log_path": "binary:d:\gosrc\bin\logger.exe"
at the time the container get create, the logger is launched automatically. today we use a fixed protocal for the logger, it will require following parameters
[stdout-pipename] [stderr-pipename] [liftime-signal-filename] [containerid] [labels]

the two named pipe is require to be host to receive the log pull out from hcsshim. and lifetime-signal] file is required to be create by the logger, and the logger then take responsible to watch the file, once the containerd shutdownt he container, it will delete the file, and logger take responsible to shutdown at that time. containerd will wait at most 10s to wait for logger shutdown, if not, it will kill the logger.

the other two parmeter is used to pass information into the customer logger.

@kevpar
Copy link

kevpar commented Sep 28, 2020

@ambarve @katiewasnothere @dcantah FYI

Overall I'm supportive of us exposing logging binary support using the CRI log path fields. I think that is a fine approach to take given the forked nature of our code base from upstream CRI.

However, I've been reading through the original pluggable logging PR in containerd (containerd/containerd#3085), and I think we will need to make some changes to align with the approach taken there.

  • The shim process should invoke the logging binary, rather than cri. This will require changes to Microsoft/hcsshim to support logging URIs and specifically logging binary support.
  • The logging binary support package (github.com/containerd/containerd/runtime/v2/logging) does not currently support Windows. It will need some changes to accommodate this, such as a different way to pass files to the logging binary process, and different signal handling.

@worldhello126
Copy link
Author

i think we need to calrify in wihch layer we want to expose such ability. from ctr layer, there is some customer logger tech implemented, but this change implement this in cri layer. if we want to make cri layer directly depend on ctr layer then we need to implement windows part for ctr. however, the problem is the protocal for linux /windows can't be same unlucky.
another problem is ctr is the parent of the logger which mean if we want to keep ctr support customer logger for windows, we will have to leave ctr be the owner of logger, but the hcsshim or runner

@worldhello126
Copy link
Author

let's have async offline to clarify this. i think we need to define a goal here. basically, with this change, it introduce the customer logging for CRI layer. so it looks like similar for the customer logging in CTR layer but it's different. we can have two different approach.

  1. Keep the cri layer customer logging be indepdent with the CTR layer. in this case, as all the change I have now is in CRI layer so it's there. I can move the shim become the parent of logger but then it mean it can't keep consistant with linux implementation
  2. Keep the cri layer depend on ctr layer customer logging. this we will need to implement the ctr customer logging in containerd first and then we can use it. in addtional, we will have to make the containerd be the parent of the logger.

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.

2 participants