Refactor docker watcher to fix flaky test and other small issues by jsoriano · Pull Request #21851 · elastic/beats

jsoriano · 2020-10-15T12:52:57Z

What does this PR do?

Refactor docker watcher to fix some small issues and improve testability:

Actually release resources of previous connections when reconnecting.
Watcher uses a clock that can be mocked in tests for time-sensitive functionality.
Use nanoseconds-precision from events timestamps, this is important to avoid duplicated events on reconnection.
Fix logger initialization (it was being initialized as docker.docker).
Refactor test helpers to have more control on test watcher when needed.
Some other code refactors.

Why is it important?

Fixes flaky test [Libbeat] Flaky TestWatcherDie test #7906.
Watch loop relied on deferred calls to do cleanups, but the watch loop never finished, so resources associated to each reconnection were never released. On reconnections caused by not receiving events, previous connections were not being closed, so several connections were kept alive, but all except the last one were ignored.
Fix duplication of events on reconnections. Each reconnection asks for events since last received events, using seconds granularity made each reconnection to retrieve again the events happened during the second the last event was received, including the event itself. API supports nanosecond granularity, that avoids this problem.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
~~I have made corresponding changes to the documentation~~
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Fixes [Libbeat] Flaky TestWatcherDie test #7906.

$ go test -count 10000 ./libbeat/common/docker/
ok  	github.com/elastic/beats/v7/libbeat/common/docker	5.220s

elasticmachine · 2020-10-15T12:53:53Z

Pinging @elastic/integrations-platforms (Team:Platforms)

jsoriano · 2020-10-15T12:56:36Z

libbeat/common/docker/watcher.go

+				if time.Since(lastReceivedEventTime) > dockerEventsWatchPityTimerTimeout {
+					w.log.Infof("No events received within %s, restarting watch call", dockerEventsWatchPityTimerTimeout)
+					return false
 				}


Not sure if this reconnection is needed, leaving it just in case, I guess this is to handle connections being stalled for some reason.

elasticmachine · 2020-10-15T14:00:45Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Build Cause: [Pull request #21851 updated]
Start Time: 2020-10-16T13:23:07.962+0000
Duration: 79 min 50 sec

Test stats 🧪

Test	Results
Failed	0
Passed	16349
Skipped	1343
Total	17692

…her-die

ChrsMark

Looks really good. Just left some comments/questions.

libbeat/common/docker/watcher.go

ChrsMark · 2020-10-16T10:04:59Z

libbeat/common/docker/watcher.go

+				if time.Since(lastReceivedEventTime) > dockerEventsWatchPityTimerTimeout {
+					w.log.Infof("No events received within %s, restarting watch call", dockerEventsWatchPityTimerTimeout)
+					return false
 				}


libbeat/common/docker/watcher_test.go

…her-die

libbeat/common/docker/watcher.go

ChrsMark

lgtm

just one non-blocking suggestion

…stic#21851) Refactor docker watcher to fix some small issues and improve testability: * Actually release resources of previous connections when reconnecting. * Watcher uses a clock that can be mocked in tests for time-sensitive functionality. * Use nanoseconds-precision from events timestamps, this is important to avoid duplicated events on reconnection. * Fix logger initialization (it was being initialized as docker.docker). * Refactor test helpers to have more control on test watcher when needed. * Some other code refactors. (cherry picked from commit 4427fa5)

) (#21918) Refactor docker watcher to fix some small issues and improve testability: * Actually release resources of previous connections when reconnecting. * Watcher uses a clock that can be mocked in tests for time-sensitive functionality. * Use nanoseconds-precision from events timestamps, this is important to avoid duplicated events on reconnection. * Fix logger initialization (it was being initialized as docker.docker). * Refactor test helpers to have more control on test watcher when needed. * Some other code refactors. (cherry picked from commit 4427fa5)

* upstream/master: (23 commits) [Ingest Manager] Prevent reporting ecs version twice (elastic#21616) [CI] Use google storage to keep artifacts (elastic#21910) Update docs.asciidoc (elastic#21849) Kubernetes leaderelection improvements (elastic#21896) Apply name changes to elastic agent docs (elastic#21549) Add 7.7.1 relnotes to 7.8 docs (elastic#21937) (elastic#21941) [libbeat] Fix potential deadlock in the disk queue + add more unit tests (elastic#21930) Refactor docker watcher to fix flaky test and other small issues (elastic#21851) [CI] Add stage name in the step (elastic#21887) [docs] Remove extra word in autodiscover docs (elastic#21871) [CI] lint stage doesn't produce test reports (elastic#21888) Add tests of reader of filestream input (elastic#21814) [Ingest Manager] Use local temp instead of system one (elastic#21883) chore: delegate variant pushes to the right method (elastic#21861) [CI] kind setup fails sometimes (elastic#21857) Fix panic on add_docker_metadata close (elastic#21882) Add tests for fileProspector in filestream input (elastic#21712) [Filebeat][okta] Fix okta pagination (elastic#21797) Add cloud.account.id into add_cloud_metadata for gcp (elastic#21776) Fix syslog RFC 5424 parsing in CheckPoint module (elastic#21854) ...

…laky-test-analyser * upstream/master: (22 commits) [Ingest Manager] Prevent reporting ecs version twice (elastic#21616) [CI] Use google storage to keep artifacts (elastic#21910) Update docs.asciidoc (elastic#21849) Kubernetes leaderelection improvements (elastic#21896) Apply name changes to elastic agent docs (elastic#21549) Add 7.7.1 relnotes to 7.8 docs (elastic#21937) (elastic#21941) [libbeat] Fix potential deadlock in the disk queue + add more unit tests (elastic#21930) Refactor docker watcher to fix flaky test and other small issues (elastic#21851) [CI] Add stage name in the step (elastic#21887) [docs] Remove extra word in autodiscover docs (elastic#21871) [CI] lint stage doesn't produce test reports (elastic#21888) Add tests of reader of filestream input (elastic#21814) [Ingest Manager] Use local temp instead of system one (elastic#21883) chore: delegate variant pushes to the right method (elastic#21861) [CI] kind setup fails sometimes (elastic#21857) Fix panic on add_docker_metadata close (elastic#21882) Add tests for fileProspector in filestream input (elastic#21712) [Filebeat][okta] Fix okta pagination (elastic#21797) Add cloud.account.id into add_cloud_metadata for gcp (elastic#21776) Fix syslog RFC 5424 parsing in CheckPoint module (elastic#21854) ...

…-store-in-another-location-too * upstream/master: [Ingest Manager] Prevent reporting ecs version twice (elastic#21616) [CI] Use google storage to keep artifacts (elastic#21910) Update docs.asciidoc (elastic#21849) Kubernetes leaderelection improvements (elastic#21896) Apply name changes to elastic agent docs (elastic#21549) Add 7.7.1 relnotes to 7.8 docs (elastic#21937) (elastic#21941) [libbeat] Fix potential deadlock in the disk queue + add more unit tests (elastic#21930) Refactor docker watcher to fix flaky test and other small issues (elastic#21851) [CI] Add stage name in the step (elastic#21887) [docs] Remove extra word in autodiscover docs (elastic#21871) [CI] lint stage doesn't produce test reports (elastic#21888) Add tests of reader of filestream input (elastic#21814) [Ingest Manager] Use local temp instead of system one (elastic#21883)

jsoriano added 4 commits October 15, 2020 14:17

Refactor docker watcher

1eeb7f6

Use nanosecond precission on events

1941d03

Rename variable

ca6cd8c

Fix flaky test TestWatcherDie

a2b905c

jsoriano added enhancement flaky-test Unstable or unreliable test cases. Team:Platforms Label for the Integrations - Platforms team labels Oct 15, 2020

jsoriano self-assigned this Oct 15, 2020

Add changelog

a59f944

botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Oct 15, 2020

jsoriano added needs_backport PR is waiting to be backported to other branches. v7.11.0 labels Oct 15, 2020

botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Oct 15, 2020

jsoriano added bug needs_team Indicates that the issue/PR needs a Team:* label and removed enhancement needs_team Indicates that the issue/PR needs a Team:* label labels Oct 15, 2020

jsoriano commented Oct 15, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into fix-flaky-test-watc…

6fbad01

…her-die

ChrsMark reviewed Oct 16, 2020

View reviewed changes

jsoriano added 2 commits October 16, 2020 12:51

Merge remote-tracking branch 'origin/master' into fix-flaky-test-watc…

05724e8

…her-die

Move interface closer to where it is used

6129492

ChrsMark reviewed Oct 16, 2020

View reviewed changes

libbeat/common/docker/watcher.go Outdated Show resolved Hide resolved

ChrsMark reviewed Oct 16, 2020

View reviewed changes

libbeat/common/docker/watcher.go Outdated Show resolved Hide resolved

ChrsMark approved these changes Oct 16, 2020

View reviewed changes

Move the implementation of the system clock

6c7d98f

ChrsMark approved these changes Oct 16, 2020

View reviewed changes

jsoriano merged commit 4427fa5 into elastic:master Oct 16, 2020

jsoriano deleted the fix-flaky-test-watcher-die branch October 16, 2020 15:54

jsoriano mentioned this pull request Oct 16, 2020

Cherry-pick #21851 to 7.x: Refactor docker watcher to fix flaky test and other small issues #21918

Merged

6 tasks

jsoriano removed the needs_backport PR is waiting to be backported to other branches. label Oct 16, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor docker watcher to fix flaky test and other small issues#21851

Refactor docker watcher to fix flaky test and other small issues#21851
jsoriano merged 9 commits intoelastic:masterfrom
jsoriano:fix-flaky-test-watcher-die

jsoriano commented Oct 15, 2020 •

edited

Loading

Uh oh!

elasticmachine commented Oct 15, 2020

Uh oh!

jsoriano Oct 15, 2020

Uh oh!

ChrsMark Oct 16, 2020

Uh oh!

elasticmachine commented Oct 15, 2020 •

edited by jenkins-beats-ci bot

Loading

Build stats

Test stats 🧪

Uh oh!

ChrsMark left a comment

Uh oh!

Uh oh!

ChrsMark Oct 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrsMark left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

jsoriano commented Oct 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Related issues

Uh oh!

elasticmachine commented Oct 15, 2020

Uh oh!

jsoriano Oct 15, 2020

Choose a reason for hiding this comment

Uh oh!

ChrsMark Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Oct 15, 2020 • edited by jenkins-beats-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ChrsMark Oct 16, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ChrsMark left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jsoriano commented Oct 15, 2020 •

edited

Loading

elasticmachine commented Oct 15, 2020 •

edited by jenkins-beats-ci bot

Loading