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

CRI: Stop following container log when container exited. #44406

Merged

Conversation

Projects
None yet
8 participants
@Random-Liu
Copy link
Member

commented Apr 12, 2017

Fixes #44340.

This PR changed kubelet to periodically check whether container is running when following container logs, and stop following when container exited.

I've tried this PR in my local cluster:

Wed Apr 12 20:23:54 UTC 2017
Wed Apr 12 20:23:58 UTC 2017
Wed Apr 12 20:24:02 UTC 2017
Wed Apr 12 20:24:06 UTC 2017
Wed Apr 12 20:24:10 UTC 2017
Wed Apr 12 20:24:14 UTC 2017
Wed Apr 12 20:24:18 UTC 2017
Wed Apr 12 20:24:22 UTC 2017
Wed Apr 12 20:24:26 UTC 2017
Wed Apr 12 20:24:30 UTC 2017
Wed Apr 12 20:24:34 UTC 2017
Wed Apr 12 20:24:38 UTC 2017
Wed Apr 12 20:24:42 UTC 2017
Wed Apr 12 20:24:46 UTC 2017
failed to wait logs for log file "/var/log/pods/1d54634c7b31346fc3219f5e0b7507cc/nginx_0.log": container "b9a17a2c53550c3703ab350d85911743af8bf164a41813544fd08fb9585f7501" is not running (state="CONTAINER_EXITED")

The only difference is that ReadLogs will return error when container exits during following. I'm not sure whether we should get rid of it or not.

@yujuhong @feiskyer @JorritSalverda
/cc @kubernetes/sig-node-bugs

Release note:

CRI: `kubectl logs -f` now stops following when container stops, as it did pre-CRI.
@k8s-reviewable

This comment has been minimized.

Copy link

commented Apr 12, 2017

This change is Reviewable

if err != nil {
return err
}
// Only keep following container log when it is running. There may be a race condition

This comment has been minimized.

Copy link
@yujuhong

yujuhong Apr 12, 2017

Member

If the container is not running, we can't tail it anyway. Do we need to care about that?

This comment has been minimized.

Copy link
@yujuhong

yujuhong Apr 12, 2017

Member

I thought we'd only reach this function if the container is running...

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Apr 12, 2017

Author Member

ACK. I tried with docker. docker logs -f will also return immediately if the container is in created state.

If so, I think we could also return here, we just shouldn't throw out the error.

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Apr 13, 2017

Author Member

Done

// stateCheckPeriod here, it should be rare.
// Even when that happen, user could also retry after the container becomes running.
if s.State != runtimeapi.ContainerState_CONTAINER_RUNNING {
return fmt.Errorf("container %q is not running (state=%q)", id, s.State)

This comment has been minimized.

Copy link
@yujuhong

yujuhong Apr 12, 2017

Member

Don't think we need the error message.

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Apr 12, 2017

Author Member

Then I need to introduce another error to tell waitLogs not to continue parse log.

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Apr 13, 2017

Author Member

Done

@@ -196,6 +204,40 @@ func ReadLogs(path string, apiOpts *v1.PodLogOptions, stdout, stderr io.Writer)
}
}

// waitLogs wait for the next log write. If the container is not running

This comment has been minimized.

Copy link
@yujuhong

yujuhong Apr 12, 2017

Member

Is this sentence complete? If the container is not running

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Apr 12, 2017

Author Member

Sorry, must be distracted by something else at that time...

This comment has been minimized.

Copy link
@Random-Liu

Random-Liu Apr 13, 2017

Author Member

Done

@mrunalp

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2017

Can't we continue to tail the logs even if container exited? The log file should be just there, right?

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

@mrunalp We can still tail the log of exited container. However, we should not keep waiting for new lines after reading all the logs in following mode, because the container has exited already.

@JorritSalverda

This comment has been minimized.

Copy link

commented Apr 13, 2017

The behaviour prior to 1.6.0 was to stop following logs as soon as the container exited. With an exit code of 0, at least when a container terminated successfully. I used it to tail containers started by a job until successful completion.

@Random-Liu Random-Liu force-pushed the Random-Liu:stop-following-when-exited branch from 80a8f75 to 2fbf34f Apr 13, 2017

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

@yujuhong Updated the PR and get rid of the error.
Here is the new output:

$ cluster/kubectl.sh logs -f nginx-3
Thu Apr 13 18:23:42 UTC 2017
Thu Apr 13 18:23:46 UTC 2017
Thu Apr 13 18:23:50 UTC 2017
Thu Apr 13 18:23:54 UTC 2017
Thu Apr 13 18:23:58 UTC 2017
Thu Apr 13 18:24:02 UTC 2017
Thu Apr 13 18:24:06 UTC 2017
Thu Apr 13 18:24:10 UTC 2017
$
@yujuhong

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

/lgtm

@Random-Liu could you add a release-note so that we can patch 1.6 later.

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 13, 2017

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2017

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

@Random-Liu

This comment has been minimized.

Copy link
Member Author

commented Apr 14, 2017

@k8s-bot gce etcd3 e2e test this

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 14, 2017

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1cf6ef0 into kubernetes:master Apr 14, 2017

12 of 13 checks passed

Submit Queue Required Github CI test is not green: Jenkins GCE etcd3 e2e
Details
Jenkins Bazel Build Job succeeded.
Details
Jenkins GCE Node e2e Jenkins job succeeded.
Details
Jenkins GCE e2e Jenkins job succeeded.
Details
Jenkins GCE etcd3 e2e Jenkins job succeeded.
Details
Jenkins GCI GCE e2e Jenkins job succeeded.
Details
Jenkins Kubemark GCE e2e Jenkins job succeeded.
Details
Jenkins kops AWS e2e Jenkins job succeeded.
Details
Jenkins non-CRI GCE Node e2e Jenkins job succeeded.
Details
Jenkins non-CRI GCE e2e Jenkins job succeeded.
Details
Jenkins unit/integration Jenkins job succeeded.
Details
Jenkins verification Jenkins job succeeded.
Details
cla/linuxfoundation Random-Liu authorized
Details

@Random-Liu Random-Liu deleted the Random-Liu:stop-following-when-exited branch Apr 14, 2017

k8s-github-robot pushed a commit that referenced this pull request Apr 14, 2017

Kubernetes Submit Queue
Merge pull request #44470 from Random-Liu/automated-cherry-pick-of-#4…
…4406-upstream-release-1.6

Automatic merge from submit-queue

Automated cherry pick of #44406 upstream release 1.6

Cherry pick of #44406 on release-1.6.

#44406: CRI: Stop following container log when container exited.

@enisoc 

**Release note**:
```release-note
`kubectl logs -f` now stops following when container stops.
```
@k8s-cherrypick-bot

This comment has been minimized.

Copy link

commented Apr 14, 2017

Commit found in the "release-1.6" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.