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

Build docker and kubernetes features only on supported platforms #13509

Merged

Conversation

kvch
Copy link
Contributor

@kvch kvch commented Sep 5, 2019

As discussed in #13415 (comment), before updating github.com/docker/docer to a newer version, I am cleaning up building Docker related features. Thus, we can get rid of the forked upstream repo: https://github.com/exekias/moby.

From now on add_docker_metadata, add_kubernetes_metadata, kubernetes and docker autodiscovery providers are only supported on linux, darwin and windows.

I added a separate plugin registry for the processor javascript, so there is no need to include other processors in it: https://github.com/elastic/beats/compare/master...kvch:fix-libbeat-compile-docker-on-supported-platforms?expand=1#diff-7dcd061ff7d5ba1d82f976b8ecdc4d73

I also moved a few processor unit tests to system tests and added a mock processor to the JS processor instead.

Note for developers

If you would like to add your processor to the JS processor you need to register it during init when it is added to the "global" processor registry using its RegisterPlugin function.

Example from add_docker_metadata:

 import (
     "github.com/elastic/beats/libbeat/processors"
     jsprocessor "github.com/elastic/beats/libbeat/processors/script/javascript/module/processor"
 )

 func init() {
     // registering the constructor of add_docker_metadata in the global processor registry
     processors.RegisterPlugin(processorName, New)
     // registering the constructor of add_docker_metadata for JS processor
     jsprocessor.RegisterPlugin("AddDockerMetadata", New)
 }

// ... more code ...

 // New constructs a new add_docker_metadata processor.
 func New(cfg *common.Config) (processors.Processor, error) {
     return buildDockerMetadataProcessor(cfg, docker.NewWatcher)
 }

// ... more code ...

@kvch kvch requested review from a team as code owners September 5, 2019 14:14
@kvch kvch added the in progress Pull request is currently in progress. label Sep 6, 2019
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

I think import files shouldn't include the platforms as suffix. For the rest it looks good.

libbeat/cmd/instance/imports_linux_darwin_windows.go Outdated Show resolved Hide resolved
metricbeat/docs/modules/docker/container.asciidoc Outdated Show resolved Hide resolved
metricbeat/include/list_linux_darwin_windows.go Outdated Show resolved Hide resolved
metricbeat/scripts/generate_imports_helper.py Outdated Show resolved Hide resolved
libbeat/processors/actions/add_fields.go Outdated Show resolved Hide resolved
metricbeat/docs/modules/docker/container.asciidoc Outdated Show resolved Hide resolved
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM. @jsoriano has some good suggestions. After making those changes it should be good to go.

@urso urso self-assigned this Sep 6, 2019
@kvch kvch force-pushed the fix-libbeat-compile-docker-on-supported-platforms branch from b24a9b4 to 43ac7ee Compare September 9, 2019 10:01
@kvch kvch requested a review from a team as a code owner September 9, 2019 15:12
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

It LGTM, just a couple of fixes in comments and a question about the test skipped in Windows.

filebeat/tests/system/test_processors.py Outdated Show resolved Hide resolved
filebeat/tests/system/test_processors.py Outdated Show resolved Hide resolved
filebeat/tests/system/test_processors.py Outdated Show resolved Hide resolved
@urso
Copy link

urso commented Sep 10, 2019

What is the advantage of moving unit tests to system tests?

@kvch
Copy link
Contributor Author

kvch commented Sep 11, 2019

@urso In a nutshell, I think the architectural changes require changes in testing: #13509 (comment)

@kvch kvch force-pushed the fix-libbeat-compile-docker-on-supported-platforms branch from 97f980f to 88d3ace Compare September 11, 2019 10:29
@urso
Copy link

urso commented Sep 11, 2019

I think it is somewhat unfortunate we need to introduce an extra registry for making processors available to JavaScript explicitely. Looks like an easy step to forget for future developers. If we want to stick with this, we should document in the processors registry that developers need to register with JavaScript as well + add a developer changelog entry.

All in all +100 on removing docker/k8s from other platforms then linux/windows/darwin.

@kvch
Copy link
Contributor Author

kvch commented Sep 11, 2019

@urso For the record, my changes do not add an extra registration step. Previously, if someone wanted to add a processor to JS processor, he/she had to add its constructor explicitly to a list of constructors in the processor. See: https://github.com/elastic/beats/pull/13509/files#diff-7dcd061ff7d5ba1d82f976b8ecdc4d73L47

I am adding a dev changelog entry.

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

I like that you registered a mock processor for unit testing the script processor. I would like to see the "chain of chains" unit test added back to Go. I don't think its necessary to port most of test cases to Python because they don't add much value beyond testing the target processor (like add_locale) and that should already be covered by the target processors unit tests.

And maybe instead of keeping a separate registry for JS processors we could just have one processor registry and mechanically translate the names from underscore separated to upper-camel-case (with special cases for known acronyms). Like translate community_id to CommunityID or decode_json_fields to DecodeJSONFields. We do a translation like this in the beats asset package and also in ECS (https://github.com/elastic/ecs/blob/f609a30b8f8cc18709e93a23d7990dfab5a58d5d/scripts/cmd/gocodegen/gocodegen.go#L297-L315).

@kvch
Copy link
Contributor Author

kvch commented Sep 12, 2019

@andrewkroh In this case, we still need to keep a list of valid processors for the JS one. It is true that this way we only have one registry and it possibly leads to a bit smaller memory footprint. However, we still need to maintain the list for JS processor. So from the developers' point of view, I don't see any additional value of this approach.

@urso
Copy link

urso commented Sep 12, 2019

I'd say let's postpone the JavaScript processor registration for later (separate PR), and keep the focus on docker/k8s here.

Another idea for registering JavaScript processors would be the introduction of functional options:

	processors.RegisterPlugin("add_fields",
		checks.ConfigChecked(CreateAddFields,
		    checks.RequireFields(FieldsKey),
		    checks.AllowedFields(FieldsKey, "target", "when")),
        processors.JavaScript("AddFields"),
    )

In case we don't give a name, we could derive the name from the processor name and register like this:

	processors.RegisterPlugin("add_fields",
		checks.ConfigChecked(CreateAddFields,
		    checks.RequireFields(FieldsKey),
		    checks.AllowedFields(FieldsKey, "target", "when")),
        processors.JavaScript(""),
    )

All in all, all solutions have the same problem. The developer needs to remember to register a processor as JavaScript compatible. We can just keep it as it is right now.

@andrewkroh is there a reason for not including all processors automatically?

@kvch
Copy link
Contributor Author

kvch commented Sep 12, 2019

Opened issue to track the enhancements: #13652 So we can merge this PR.

@kvch kvch merged commit 24c1e59 into elastic:master Sep 12, 2019
@andrewkroh
Copy link
Member

@andrewkroh is there a reason for not including all processors automatically?

I think it should be find to include all of the processors with the exception of script processor itself. So we don't actually need to keep a list of "valid processors" for the JS processor. It's just a matter of getting the name formatted appropriately.

main package to automatically register all of the standard supported Metricbeat
modules.
*/
// +build linux darwin windows
Copy link
Member

@andrewkroh andrewkroh Sep 12, 2019

Choose a reason for hiding this comment

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

@kvch This build tag will not have any effect because it's preceeded by a block comment (rather than a line comment).

Constraints may appear in any kind of source file (not just Go), but they must appear near the top of the file, preceded only by blank lines and other line comments. https://golang.org/pkg/go/build/

Copy link
Member

Choose a reason for hiding this comment

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

I opened a fix in #13654.

andrewkroh added a commit to andrewkroh/beats that referenced this pull request Sep 12, 2019
The build tags added in elastic#13509 didn't have an effect because only line comments can precede build tags.

This also adds back cross-compilation for freebsd and netbsd that was disabled due to Docker compilation issues. Closes elastic#13400
andrewkroh added a commit that referenced this pull request Sep 12, 2019
The build tags added in #13509 didn't have an effect because only line comments can precede build tags.

This also adds back cross-compilation for freebsd and netbsd that was disabled due to Docker compilation issues. Closes #13400
@urso urso added the v7.5.0 label Oct 22, 2019
@mostlyjason mostlyjason mentioned this pull request Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Pull request is currently in progress. libbeat review v7.5.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants