[release/1.3 backport] Add logging binary support when terminal is true#4616
[release/1.3 backport] Add logging binary support when terminal is true#4616akshat-kmr wants to merge 4 commits intocontainerd:release/1.3from
Conversation
|
Hi @akshat-kmr. Thanks for your PR. I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Build succeeded.
|
akshat-kmr
force-pushed
the
release/1.3
branch
from
3108674 to
5fabecc
Compare
|
Build succeeded.
|
|
Build succeeded.
|
akshat-kmr
force-pushed
the
release/1.3
branch
from
f3b6616 to
5fabecc
Compare
|
Build succeeded.
|
|
Build succeeded.
|
|
@akshat-kmr could you mind to use |
Currently the shims only support starting the logging binary process if the io.Creator Config does not specify Terminal: true. This means that the program using containerd will only be able to specify FIFO io when Terminal: true, rather than allowing the shim to fork the logging binary process. Hence, containerd consumers face an inconsistent behavior regarding logging binary management depending on the Terminal option. Allowing the shim to fork the logging binary process will introduce consistency between the running container and the logging process. Otherwise, the logging process may die if its parent process dies whereas the container will keep running, resulting in the loss of container logs. Signed-off-by: Akshat Kumar <[email protected]> (cherry picked from commit 7a9fbec)
Signed-off-by: Akshat Kumar <[email protected]> (cherry picked from commit 4cc99e5)
Signed-off-by: Akshat Kumar <[email protected]> (cherry picked from commit 61da698)
Signed-off-by: Akshat Kumar <[email protected]>
akshat-kmr
force-pushed
the
release/1.3
branch
from
5fabecc to
28f4128
Compare
Thanks, I've done so. The first 3 commits are marked with |
|
Build succeeded.
|
|
I think we should only backport security fixes |
AkihiroSuda
left a comment
AkihiroSuda
left a comment
There was a problem hiding this comment.
Please see my comment above
|
Agreed, I don't think we should backport this change to 1.3 |
|
Sounds good, in that case I will close this PR. The backport to 1.4 should be fine though, correct? If so, I would appreciate if you can take a look when you have a chance @AkihiroSuda #4615 |
|
No, this shouldn't be back ported to 1.4 either, unless there is very specific reason |
5 participants
Backport of #4502. I've cherry-picked the individual commits from that pull request, and added a separate commit for a minor modification required for compatibility with containerd 1.3
Currently the shims only support starting the logging binary process if the
io.Creator Config does not specify Terminal: true. This means that the program
using containerd will only be able to specify FIFO io when Terminal: true,
rather than allowing the shim to fork the logging binary process. Hence,
containerd consumers face an inconsistent behavior regarding logging binary
management depending on the Terminal option.
Allowing the shim to fork the logging binary process will introduce consistency
between the running container and the logging process. Otherwise, the logging
process may die if its parent process dies whereas the container will keep
running, resulting in the loss of container logs.
Signed-off-by: Akshat Kumar [email protected]