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

Update github/docker/docker to a consistent version across all packages #13415

Merged
merged 6 commits into from
Sep 6, 2019

Conversation

andrewkroh
Copy link
Member

@andrewkroh andrewkroh commented Aug 29, 2019

This bumps up the version of all vendored github.com/docker/docker packages to moby/moby@8af4db6. Beats was mixing versions of packages from the Docker project.

This does impact netbsd, because this version of Docker doesn't compile for netbsd. If we wanted to address this we could fork moby/moby and apply this commit exekias/moby@83d94aa. If we fork then we should also fix the FreeBSD compilation issue referenced in #13400.

I was trying to minimize the amount of changes to the Docker libraries to avoid breaking changes so moby/moby@8af4db6 is earliest version of Docker that fixes the github.com/Sirupsen/logrus issue causing problems with go.mod in #11850. go.mod also requires that you use the same version for the entire project so this fixes that issue too.

@andrewkroh andrewkroh force-pushed the update-docker-vendor branch 5 times, most recently from 6b813ed to 84d6a24 Compare September 3, 2019 14:11
@andrewkroh andrewkroh marked this pull request as ready for review September 3, 2019 14:50
@andrewkroh andrewkroh requested a review from a team as a code owner September 3, 2019 14:50
metricbeat/Makefile Show resolved Hide resolved
@jsoriano
Copy link
Member

jsoriano commented Sep 3, 2019

@andrewkroh thanks for taking this!

@andrewkroh andrewkroh requested a review from kvch September 3, 2019 15:56
@kvch
Copy link
Contributor

kvch commented Sep 4, 2019

It might be an unpopular opinion, but I don't think we need to support docker (metrics, autodiscovery) on FreeBSD. Docker(moby) does not support it, why should we make an additional effort? I suspect as it's not supported on FreeBSD, not many people use it in production. Forking a lib just puts extra work on us, without any gains.

Instead of forking moby, I would not compile docker related code for FreeBSD (using build tags).

Regarding our PRs, I would take more risks when updating docker. I haven't seen many breaking changes in the latest releases. Also, our automated tests should catch the issues. But I like the changes in you made in our codebase more than in mine. :D So it would be nice to combine both PRs into one. WDYT?

@andrewkroh
Copy link
Member Author

andrewkroh commented Sep 4, 2019

It might be an unpopular opinion, but I don't think we need to support docker (metrics, autodiscovery) on FreeBSD... Instead of forking moby, I would not compile docker related code for FreeBSD (using build tags).

I agree 100%. Let's get those build tags put into place in a separate PR and add back the netbsd and freebsd cross-compiles. Do you want to try to make this happen?

Regarding our PRs, I would take more risks when updating docker.

I'm fine with that after seeing the changes in your PR. I didn't want to jump into the unknown 😨 . I'll do an update that pulls the latest docker/docker from docker/engine@<newest-tag>. Then compare it against your PR. I am surprised at how much the two PRs differ now w.r.t. number of lines changes in vendor/.

@andrewkroh
Copy link
Member Author

@kvch I pushed an update with the lastest release from github.com/docker/engine. Let's see how testing goes.

@andrewkroh
Copy link
Member Author

There's one test failure happening on Windows for Metricbeat:

=== RUN TestData
--- FAIL: TestData (0.00s)
modules.go:95: failed to create new MetricSet 1 error: protocol not available
FAIL
FAIL github.com/elastic/beats/metricbeat/module/docker/image 0.265s

This bumps up the version of all vendored github.com/docker/docker packages to 8af4db6f002ac907b6ef8610b237879dfcaa5b7a. This does impact netbsd, because this version of Docker doesn't compile for netbsd.

This is earliest version of Docker that fixes the github.com/Sirupsen/logrus issue causing problems with go.mod in elastic#11850. I was trying to minimize the amount of changes to the Docker libraries to avoid breaking changes.
Ran command:
    govendor fetch github.com/docker/docker/...::github.com/docker/engine@v19.03.2

github.com/docker/engine is a fork of github.com/moby/moby but with release tags. (source: moby/moby#39302 (comment))
@andrewkroh
Copy link
Member Author

I think an integration build tag was missing because that test should never have been run on Windows. The test is configured to use unix:// as the host so I'm not surprised it failed. I just wonder why it never failed in the past. I guess Docker improved input validation.

@andrewkroh
Copy link
Member Author

jenkins, test this

@jsoriano
Copy link
Member

jsoriano commented Sep 5, 2019

+1 to drop docker support on FreeBSD (or any platform not supported by docker itself)

@andrewkroh
Copy link
Member Author

jenkins, test this

There was a failure this time in linux/libbeat. Not sure if this was flaky before these changes.

command [go test -race -tags integration -cover -coverprofile /tmp/gotestcover-545208368 github.com/elastic/beats/libbeat/autodiscover/providers/docker]: exit status 1
--- FAIL: TestDockerStart (14.03s)
docker_integration_test.go:116: Timeout waiting for provider events
FAIL
coverage: 76.7% of statements
FAIL github.com/elastic/beats/libbeat/autodiscover/providers/docker 14.585s

@andrewkroh
Copy link
Member Author

jenkins, test this

That autodiscover test is passing 100% of the time and quickly on machine.

@andrewkroh
Copy link
Member Author

The autodiscover test passed in the last run. But Filebeat test_shutdown_wait_ok failed.

@andrewkroh andrewkroh merged commit 44f4b3f into elastic:master Sep 6, 2019
kvch added a commit that referenced this pull request Sep 12, 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`:
```golang

 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 ...
```
andrewkroh added a commit to andrewkroh/beats that referenced this pull request Oct 17, 2019
The libbeat Docker client wasn't negotiate the Docker API version after updating the Docker package versions. This changes the Docker client to use the `NewClientWithOpts` function rather than the deprecated `NewClient` function.

When the client is constructed we will pass in the `WithAPIVersionNegotiation` option if no version is explicitly configured in the DOCKER_API_VERSION environment variable. Upon the first request the client will negotiate the API version.

Fixes changes made in elastic#13415.
andrewkroh added a commit that referenced this pull request Oct 21, 2019
The libbeat Docker client wasn't negotiate the Docker API version after updating the Docker package versions. This changes the Docker client to use the `NewClientWithOpts` function rather than the deprecated `NewClient` function.

When the client is constructed we will pass in the `WithAPIVersionNegotiation` option if no version is explicitly configured in the DOCKER_API_VERSION environment variable. Upon the first request the client will negotiate the API version.

Fixes changes made in #13415.
andrewkroh added a commit to andrewkroh/beats that referenced this pull request Oct 21, 2019
The libbeat Docker client wasn't negotiate the Docker API version after updating the Docker package versions. This changes the Docker client to use the `NewClientWithOpts` function rather than the deprecated `NewClient` function.

When the client is constructed we will pass in the `WithAPIVersionNegotiation` option if no version is explicitly configured in the DOCKER_API_VERSION environment variable. Upon the first request the client will negotiate the API version.

Fixes changes made in elastic#13415.

(cherry picked from commit ca6dc3a)
@urso urso added the v7.5.0 label Oct 22, 2019
andrewkroh added a commit that referenced this pull request Oct 26, 2019
The libbeat Docker client wasn't negotiate the Docker API version after updating the Docker package versions. This changes the Docker client to use the `NewClientWithOpts` function rather than the deprecated `NewClient` function.

When the client is constructed we will pass in the `WithAPIVersionNegotiation` option if no version is explicitly configured in the DOCKER_API_VERSION environment variable. Upon the first request the client will negotiate the API version.

Fixes changes made in #13415.

(cherry picked from commit ca6dc3a)
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
The libbeat Docker client wasn't negotiate the Docker API version after updating the Docker package versions. This changes the Docker client to use the `NewClientWithOpts` function rather than the deprecated `NewClient` function.

When the client is constructed we will pass in the `WithAPIVersionNegotiation` option if no version is explicitly configured in the DOCKER_API_VERSION environment variable. Upon the first request the client will negotiate the API version.

Fixes changes made in elastic#13415.

(cherry picked from commit a647b0a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants