Emit error if fileset with multiple pipelines is being used with ES < 6.5 #10001

ycombinator · 2019-01-10T17:20:41Z

Follow up to #8914.

In #8914, we introduced the ability for Filebeat filesets to have multiple Ingest pipelines, the first one being the entry point. This feature relies on the Elasticsearch Ingest node having a pipeline processor and if conditions for processors, both of which were introduced in Elasticsearch 6.5.0.

This PR implements a check for whether a fileset has multiple Ingest pipelines AND is talking to an Elasticsearch cluster < 6.5.0. If that's the case, we emit an error.

ycombinator · 2019-01-10T17:35:59Z

A couple of questions about this PR for the reviewers:

When the check implemented by this PR fails, we emit an error message in the Filebeat logs like so:

2019-01-10T09:23:42.716-0800    ERROR   fileset/factory.go:142  Error loading pipeline: the elasticsearch/slowlog fileset has multiple pipelines, which are only supported with Elasticsearch >= 6.5.0. Currently running with Elasticsearch version 7.0.0-SNAPSHOT

However, Filebeat continues to run. Just want to check with you that this is okay?

Testing this PR requires running against a version of ES < 6.5.0. Is there some way we can automate this in our tests? Is there somewhere we do this sort of testing already?

ycombinator · 2019-01-10T18:54:26Z

jenkins, test this

ycombinator · 2019-01-11T01:41:49Z

Ignore my question 2. I think I can just write a unit test for this. 🤦‍♂️

ruflin · 2019-01-11T10:07:51Z

filebeat/fileset/pipelines.go

+			// Filesets with multiple pipelines can only be supported by Elasticsearch >= 6.5.0
+			esVersion := esClient.GetVersion()
+			minESVersionRequired := common.MustNewVersion("6.5.0")
+			if len(pipelines) > 1 && esVersion.LessThan(minESVersionRequired) {


Does the current LS module also fall into this statement? Probably not because it only loads 1?

The Logstash module actually loads both/all pipelines but it only uses one of them at runtime. So it doesn't fall into this statement. I just tested this to confirm as well.

But wouldn't we want to fail installing the LS module?

I'm not sure. The logstash/log fileset's manifest.yml looks like this:

beats/filebeat/module/logstash/log/manifest.yml

Line 19 in f5a9028

ingest_pipeline: ingest/pipeline-{{.format}}.json

Whereas a fileset that is using the multiple pipelines feature will look like this:

ingest_pipeline: - ingest/entry_point_pipeline.json - ingest/some_sub_pipeline.json - ingest/another_sub_pipeline.json

The latter one uses the new multiple-pipeline feature, which is what the version check is for.

ruflin

Will need a changelog entry and would be great to have some basic tests around it.

ycombinator · 2019-01-11T11:48:35Z

Added a CHANGELOG entry and unit test. Ready for review again. Thanks!

filebeat/fileset/pipelines_test.go

ycombinator · 2019-01-11T12:48:55Z

jenkins, test this

urso · 2019-01-11T15:53:00Z

filebeat/fileset/pipelines_test.go

+		},
+	}
+
+	for _, test := range tests {


This loop creates a many concurrent HTTP servers not being cleaned up between the different tests. This is due to defer operating at function-scope, not block scope.

Use t.Run(name, func(t *testing.T) { ... }) so to create separate isolated tests (defer in the body will properly cleanup resources). This improves output/readability of test results, plus allows developers to run single tests.

Common pattern:

cases := map[string]struct{ ... }{ ... } for name, test := range cases { test := test // copy test into local block scope (in case you want to enable t.Parallel()) t.Run(name, func(t *testing.T) { // common test body }) }

This is due to defer operating at function-scope, not block scope.

Thanks, TIL!

@urso Would you recommend using t.Parallel() in general, or would you say it depends on how much work each test case is doing and how many test cases there are to run in all?

Running with t.Parallel() is a great way to see if things are really isolated :)
Tests are run package by package. Using t.Parallel() is a nice way to speed up tests in a package that already a few seconds. Checking libbeat unit tests, most are finished in a few ms. Not really worth it to optimize already fast tests.

When using t.Parallel() the parent test blocks until all it's child tests have returned. The number of active concurrent tests to be run depends on the -parallel CLI flag. by default it's the number of cores (GOMAXPROCS env variable) you have in your system. We don't use t.Parallel() much, but for packages with a large number of unit tests, that also take a little longer to execute it can really make a difference. The t.Run() method always spawns a go-routine. That is, there is no real additional cost by adding t.Parallel() even for very small tests.

Just tried to enable t.Parallel() on beats queues test on my machine. Duration went from 20s to ~5s. I'm always happy if I don't have to wait for long unit tests to finish.

We should also make more use of testing/quick (if applicable). These will definitely profit from t.Parallel().

Still most time is spend in system tests.

… 6.5

…nes is being used with ES < 6.5 (#10038) Cherry-pick of PR #10001 to 6.x branch. Original message: Follow up to #8914. In #8914, we introduced the ability for Filebeat filesets to have multiple Ingest pipelines, the first one being the entry point. This feature relies on the Elasticsearch Ingest node having a `pipeline` processor and `if` conditions for processors, both of which were introduced in Elasticsearch 6.5.0. This PR implements a check for whether a fileset has multiple Ingest pipelines AND is talking to an Elasticsearch cluster < 6.5.0. If that's the case, we emit an error.

elasticmachine · 2019-01-15T21:49:55Z

Pinging @elastic/stack-monitoring

ycombinator added review Filebeat Filebeat needs_backport PR is waiting to be backported to other branches. v7.0.0 v6.7.0 labels Jan 10, 2019

ycombinator requested review from urso and ruflin January 10, 2019 17:20

ycombinator requested a review from a team as a code owner January 10, 2019 17:20

ruflin reviewed Jan 11, 2019

View reviewed changes

ycombinator force-pushed the fb-multi-pipelines-version-check branch 2 times, most recently from aa0fe4b to 58c0a87 Compare January 11, 2019 11:48

ruflin approved these changes Jan 11, 2019

View reviewed changes

filebeat/fileset/pipelines_test.go Outdated Show resolved Hide resolved

urso reviewed Jan 11, 2019

View reviewed changes

urso approved these changes Jan 12, 2019

View reviewed changes

ycombinator added 6 commits January 12, 2019 16:00

Emit error if fileset with multiple pipelines is being used with ES <…

15ff8ce

… 6.5

Better error message

c2b7caf

Adding CHANGELOG entry

3a0f456

Adding unit test for various version checks

28edeaa

Using t.Run() to shutdown HTTP server at the end of each test case

e837254

Use custom error

27c3e97

ycombinator force-pushed the fb-multi-pipelines-version-check branch from a2820bd to 27c3e97 Compare January 13, 2019 00:00

ycombinator merged commit c55226e into elastic:master Jan 13, 2019

ycombinator deleted the fb-multi-pipelines-version-check branch January 13, 2019 04:58

ycombinator mentioned this pull request Jan 13, 2019

Cherry-pick #10001 to 6.x: Emit error if fileset with multiple pipelines is being used with ES < 6.5 #10038

Merged

ycombinator removed the needs_backport PR is waiting to be backported to other branches. label Jan 13, 2019

monicasarbu added the Feature:Stack Monitoring label Jan 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit error if fileset with multiple pipelines is being used with ES < 6.5 #10001

Emit error if fileset with multiple pipelines is being used with ES < 6.5 #10001

ycombinator commented Jan 10, 2019

ycombinator commented Jan 10, 2019 •

edited

Loading

ycombinator commented Jan 10, 2019

ycombinator commented Jan 11, 2019

ruflin Jan 11, 2019

ycombinator Jan 11, 2019

urso Jan 11, 2019

ycombinator Jan 11, 2019 •

edited

Loading

ruflin left a comment

ycombinator commented Jan 11, 2019

ycombinator commented Jan 11, 2019

urso Jan 11, 2019

ycombinator Jan 11, 2019 •

edited

Loading

ycombinator Jan 11, 2019

urso Jan 12, 2019 •

edited

Loading

elasticmachine commented Jan 15, 2019

Emit error if fileset with multiple pipelines is being used with ES < 6.5 #10001

Emit error if fileset with multiple pipelines is being used with ES < 6.5 #10001

Conversation

ycombinator commented Jan 10, 2019

ycombinator commented Jan 10, 2019 • edited Loading

ycombinator commented Jan 10, 2019

ycombinator commented Jan 11, 2019

ruflin Jan 11, 2019

Choose a reason for hiding this comment

ycombinator Jan 11, 2019

Choose a reason for hiding this comment

urso Jan 11, 2019

Choose a reason for hiding this comment

ycombinator Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

ruflin left a comment

Choose a reason for hiding this comment

ycombinator commented Jan 11, 2019

ycombinator commented Jan 11, 2019

urso Jan 11, 2019

Choose a reason for hiding this comment

ycombinator Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

ycombinator Jan 11, 2019

Choose a reason for hiding this comment

urso Jan 12, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Jan 15, 2019

ycombinator commented Jan 10, 2019 •

edited

Loading

ycombinator Jan 11, 2019 •

edited

Loading

ycombinator Jan 11, 2019 •

edited

Loading

urso Jan 12, 2019 •

edited

Loading