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.

Sign up for GitHub

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

Jump to bottom

Use docker logs directly if the docker logging driver is not json-file #42195

Merged
merged 1 commit into from
Mar 2, 2017
Merged

Use docker logs directly if the docker logging driver is not json-file #42195

merged 1 commit into from
Mar 2, 2017

Conversation

Random-Liu
Copy link
Member

Fixes #41996.

Post the PR first, I still need to manually test this, because we don't have test coverage for journald logging pluggin.

@yujuhong @dchen1107
/cc @kubernetes/sig-node-pr-reviews

@Random-Liu Random-Liu added area/docker area/kubelet area/logging release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. labels Feb 27, 2017
@Random-Liu Random-Liu added this to the v1.6 milestone Feb 27, 2017
@Random-Liu Random-Liu requested a review from yujuhong February 27, 2017 22:38
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 27, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2017
@Random-Liu Random-Liu force-pushed the cri-support-non-json-logging branch from bee73c0 to 5ade58b Compare February 27, 2017 22:40
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 27, 2017
@Random-Liu Random-Liu force-pushed the cri-support-non-json-logging branch from 5ade58b to b948562 Compare February 27, 2017 22:42
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/old-docs size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 27, 2017
@Random-Liu Random-Liu changed the title Use docker logs directory if the docker logging driver is not Use docker logs directly if the docker logging driver is not json-file Feb 27, 2017
@Random-Liu Random-Liu force-pushed the cri-support-non-json-logging branch 3 times, most recently from 95d801d to 27ba4f2 Compare February 27, 2017 23:39
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 28, 2017
@Random-Liu Random-Liu force-pushed the cri-support-non-json-logging branch from 3066671 to 31d0f66 Compare February 28, 2017 01:26
@Random-Liu
Copy link
Member Author

Random-Liu commented Feb 28, 2017 •
edited
Loading

I manually verified this PR by running node e2e ConfigMap test https://github.com/kubernetes/kubernetes/blob/master/test/e2e/common/configmap.go. The test relies on container log output.

  • When I run the test directly, the test passes, and the node ready condition message is kubelet is posting ready status. AppArmor enabled.
  • When manually change docker client to always return info.LoggingDriver = journald, the test will still pass, which means "legacy container log" works. And the node ready condition message becomes kubelet is posting ready status. AppArmor enabled. Docker Logging Driver: journald.

@Random-Liu Random-Liu force-pushed the cri-support-non-json-logging branch from 31d0f66 to 7c01bf1 Compare February 28, 2017 02:11
@Random-Liu
Copy link
Member Author

@yujuhong Removed the node status part.

@yujuhong
Copy link
Contributor

/lgtm

Thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2017
feiskyer
feiskyer reviewed Feb 28, 2017
View reviewed changes
pkg/kubelet/dockershim/docker_service.go

// IsCRISupportedLogDriver checks whether the logging driver used by docker is
// suppoted by native CRI integration.
func IsCRISupportedLogDriver(client dockertools.DockerInterface) (bool, error) {
Copy link
Member

@feiskyer feiskyer Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nit: do we need to log the driver name? It could help for debugging if there is something wrong when getting logs.

@Random-Liu
Copy link
Member Author

@k8s-bot node e2e test this.

@Random-Liu
Copy link
Member Author

@k8s-bot non-cri node e2e test this.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2017
@Random-Liu Random-Liu force-pushed the cri-support-non-json-logging branch from 7c01bf1 to 7c261bf Compare March 1, 2017 18:50
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: Random-Liu, yujuhong

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 1, 2017
@Random-Liu
Copy link
Member Author

Rebased, assign lgtm again.

@Random-Liu Random-Liu added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Mar 1, 2017 •
edited
Loading

@Random-Liu: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins Cross Build 5ade58b8758ed6d735ae139002196c15879d1783 link @k8s-bot cross build this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions 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. I understand the commands that are listed here.

@Random-Liu
Copy link
Member Author

@k8s-bot gce etcd3 e2e test this

@k8s-reviewable
Copy link

This change is Reviewable

@Random-Liu Random-Liu added the kind/bug Categorizes issue or PR as related to a bug. label Mar 2, 2017
@Random-Liu
Copy link
Member Author

This is a bug fix to make CRI integration work with docker journald logging driver.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 41931, 39821, 41841, 42197, 42195)

@k8s-github-robot k8s-github-robot merged commit fa0387c into kubernetes:master Mar 2, 2017
@Random-Liu Random-Liu deleted the cri-support-non-json-logging branch March 7, 2017 18:08
@ncdc ncdc mentioned this pull request May 26, 2017
Merged
@joelsmith joelsmith mentioned this pull request Sep 14, 2017
Closed
@enkov enkov mentioned this pull request Oct 31, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@dchen1107 dchen1107 dchen1107 left review comments

@yujuhong yujuhong Awaiting requested review from yujuhong

Assignees

@yujuhong yujuhong

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/docker area/kubelet area/logging cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.6
Development

Successfully merging this pull request may close these issues.
7 participants
@Random-Liu @yujuhong @k8s-github-robot @k8s-ci-robot @k8s-reviewable @feiskyer @dchen1107