Stop following logs using timers#6702
Conversation
This incorporates code from PR #6591 and #6614 but does not use event channels to detect container state and rather uses timers with a defined wait duration before calling t.StopAtEOF() to ensure the last log entry is output before a container exits. The polling interval is set to 250 milliseconds based on polling interval defined in hpcloud/tail here: https://github.com/hpcloud/tail/blob/v1.0.0/watch/polling.go#L117 Co-authored-by: Qi Wang <qiwan@redhat.com> Signed-off-by: jgallucci32 <john.gallucci.iv@gmail.com>
|
Hi @jgallucci32. Thanks for your PR. I'm waiting for a containers 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. |
openshift-ci-robot
added
the
needs-ok-to-test
|
/assign @vrothberg |
|
@mheon @TomSweeneyRedHat @QiWang19 @edsantiago PTAL This passed all checks first go around. The key difference with this PR is I removed the dependency on the external event module and it now runs independently using timers. To get the timing precise it needed to match the polling interval defined in hpcloud/tail here: func init() {
POLL_DURATION = 250 * time.Millisecond
}Additionally it needed to ensure when the container changes state, the 250 milisecond delay is applied before calling |
Signed-off-by: jgallucci32 <john.gallucci.iv@gmail.com>
|
LGTM |
| go func() { | ||
| for { | ||
| state, err := c.State() | ||
| time.Sleep(watch.POLL_DURATION) |
There was a problem hiding this comment.
is there any reason why we do a Sleep before checking for the error? Should the Sleep happen later?
There was a problem hiding this comment.
Yes. Under all conditions you must call stopAtEOF() when exiting the subroutine in order to break the infinite wait state t.Lines in the other subroutine. You must also wait up to the POLL_DURATION before calling stopAtEOF() or else you may run into the condition where the container has exited or does not exist and t.Lines is still polling for changes to the log file.
If you check commit 8926e1f you will see there originally was 3 sleep statements, 2 of them after the if condition. This was refactored as there is no logical difference between calling sleep first at the beginning versus calling it after the conditional statements and at the end of the entire loop.
Signed-off-by: jgallucci32 <john.gallucci.iv@gmail.com>
|
/ok-to-test |
openshift-ci-robot
added
ok-to-test
and removed
needs-ok-to-test
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, jgallucci32, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/lgtm |
github-actions
Bot
added
the
locked - please file new issue/PR
8 participants
This incorporates code from PR #6591 and #6614 but does not use
event channels to detect container state and rather uses timers
with a defined wait duration before calling
t.StopAtEOF()toensure the last log entry is output before a container exits.
The polling interval is set to 250 milliseconds based on polling
interval defined in hpcloud/tail here:
https://github.com/hpcloud/tail/blob/v1.0.0/watch/polling.go#L117
Close #6531
Co-authored-by: Qi Wang qiwan@redhat.com
Signed-off-by: jgallucci32 john.gallucci.iv@gmail.com