[Elastic Logging Plugin] Refactor of build tooling for docker plugin #15441

fearful-symmetry · 2020-01-09T20:19:53Z

This is an extensive refactor of the build scripts for the docker plugin. The infra folks were running into a lot of issues, and I'm hoping this addresses most of them.

Most notably, we're no longer shelling out docker commands, and are now using the docker client API. This gives us more control over what docker is doing, and will make it easier to improve this process later on. This also cleans up the build process, and removes some code smell issues.

There's no doubt a bunch of improvements we can still make here, but considering how close we are to 7.6, I want to make sure this just works and barring any low-hanging fruit, we can do optimizations later.

@kvch I requested you since we added a new docker dep, and I want to make sure I did it right.

fearful-symmetry · 2020-01-09T21:45:20Z

Still fighting with dependencies.

fearful-symmetry · 2020-01-09T22:55:33Z

jenkins, test this

vendor/vendor.json

urso · 2020-01-10T11:03:22Z

We already have docker + k8s dependencies. What do the new dependencies add we are missing so far?

We made an effort allowing Beats to be build and run on environments that do not have docker support altogether (e.g. AIX or BSD). Problem with docker as dependency is that it doesn't even build on some alternative platforms. Given the urgency of this change I'm fine with fixing this later on if it is indeed an issue. I think it's more an issue when calling top-level make release, no new issue introduced here.

x-pack/dockerlogbeat/magefile.go

urso · 2020-01-10T11:22:39Z

x-pack/dockerlogbeat/magefile.go

+		return errors.Wrap(err, "Error locating temp dir")
+	}
+	tarPath := filepath.Join(tmpDir, "tarRoot.tar")
+	err = sh.RunV("tar", "cf", tarPath, "./")


We switch to the Beats root directory for this tar command?

Btw. stdlib also has the archive/tar package.

Yah, we send the entire beats directory to the docker context to actually build the plugin, since we depend on libbeat and so on. Eventually I really want to find a faster way to do this. Maybe just copy over libbeat to vendor during the build or something? We honestly might not even need to do this step inside docker at all, and just copy a finished binary to our "2nd stage" container. I don't know enough about our release process to tell.

Could we reuse devtools.CrossBuild to build the beat binary as we do in other beats?

If tarring files here is really needed, I would try to avoid using Chdir, this can have undesired effects when running targets in parallel (Chdir changes the dir for the whole process), or if this code is moved somewhere at some moment.

You can try to use the -C flag of tar, or use archive/tar as suggested.

x-pack/dockerlogbeat/magefile.go

fearful-symmetry · 2020-01-10T14:43:53Z

@urso

We already have docker + k8s dependencies. What do the new dependencies add we are missing so far?

We were missing the archive library needed to directly run plugin create

We made an effort allowing Beats to be build and run on environments that do not have docker support altogether (e.g. AIX or BSD).

Really? I was under the impression that the release process required docker. I mean, technically you can run go build in dockerlogbeat and you'll get a binary, but packaging this without docker would require some serious hacks.

jmlrt · 2020-01-10T16:17:16Z

@fearful-symmetry our first release is failing because of the structure of the tarball (https://github.com/elastic/infra/issues/16748#issuecomment-573046527).
Could you rename build/package directory to elastic-logging-plugin when creating the tarball?

urso · 2020-01-10T16:41:03Z

Really? I was under the impression that the release process required docker. I mean, technically you can run go build in dockerlogbeat and you'll get a binary, but packaging this without docker would require some serious hacks.

Right. Forget what I said. We use the docker images to crossbuild for the target platform. The docker client libs do not always correctly crossbuild. But the plugin implements mage package itself.

fearful-symmetry · 2020-01-10T16:46:28Z

The docker client libs do not always correctly crossbuild.

Yah, that doesn't surprise me. This plugin is a bit weird in the sense that we have no concept of a cross-compile, since a docker plugin at a platform-independent container, although at a low-level it's a linux-only artifact.

fearful-symmetry · 2020-01-10T18:45:54Z

@jmlrt Alright, latest commit should fix your issue. Do you have a way of testing?

mikemadden42

LGTM since it fixes the RM issues.

jsoriano

If this is blocking other things I am ok with merging it, but I think that some things would need to be polished.

Most notably, we're no longer shelling out docker commands, and are now using the docker client API. This gives us more control over what docker is doing, and will make it easier to improve this process later on. This also cleans up the build process, and removes some code smell issues.

In dockerbuild.go we are using the docker commands and it hasn't been so problematic so far, but we could consider using the API also there if it works well here.
I am not sure though if for example docker build is cleaner to do with the API.

jsoriano · 2020-01-13T18:37:00Z

x-pack/dockerlogbeat/magefile.go

+		return errors.Errorf("not in dockerlogbeat directory: %s", dockerLogBeatDir)
+	}
+
+	// change back to the root beats dir so we can send the proper build context to docker
 	err = os.Chdir("../..")


Could we use devtools.XPackBeatDir here? and then we wouldn't need to check that we are in the expected directory.

Suggested change

err = os.Chdir("../..")

err = os.Chdir(devtools.XPackBeatDir())

I tried this, for for whatever reason, it kept on trying to navigate to x-pack/x-pack so...I gave up.

x-pack/dockerlogbeat/magefile.go

jsoriano · 2020-01-13T18:49:59Z

x-pack/dockerlogbeat/magefile.go

+		return errors.Wrap(err, "Error locating temp dir")
+	}
+	tarPath := filepath.Join(tmpDir, "tarRoot.tar")
+	err = sh.RunV("tar", "cf", tarPath, "./")


Could we reuse devtools.CrossBuild to build the beat binary as we do in other beats?

If tarring files here is really needed, I would try to avoid using Chdir, this can have undesired effects when running targets in parallel (Chdir changes the dir for the whole process), or if this code is moved somewhere at some moment.

You can try to use the -C flag of tar, or use archive/tar as suggested.

x-pack/dockerlogbeat/magefile.go

jsoriano · 2020-01-13T19:06:42Z

x-pack/dockerlogbeat/magefile.go

+	}
+
+	//check to see if we have a plugin we need to remove
+	plugins, err := cli.PluginList(context.Background(), filters.Args{})


Could we directly use PluginInspect?

x-pack/dockerlogbeat/magefile.go

jsoriano

Thanks for the last hour changes. Let's get this merged as is blocking other things, and please follow up with the other issues in other PRs.

x-pack/dockerlogbeat/magefile.go

…lastic#15441) * refactor of build tooling for docker plugin (cherry picked from commit b19bdf4)

…15441) (#15520) * refactor of build tooling for docker plugin (cherry picked from commit b19bdf4)

refactor of build tooling for docker plugin

6b14e3b

fearful-symmetry added needs_backport PR is waiting to be backported to other branches. v7.6.0 labels Jan 9, 2020

fearful-symmetry requested review from urso, mgreau, mikemadden42 and a team January 9, 2020 20:19

fearful-symmetry self-assigned this Jan 9, 2020

fearful-symmetry added 2 commits January 9, 2020 13:07

add archive dep

5002e0d

update notice

4cd4f33

fearful-symmetry requested a review from kvch January 9, 2020 21:10

update libcontainer dep

06ae64d

kvch reviewed Jan 10, 2020

View reviewed changes

vendor/vendor.json Show resolved Hide resolved

kvch reviewed Jan 10, 2020

View reviewed changes

vendor/vendor.json Show resolved Hide resolved

kvch reviewed Jan 10, 2020

View reviewed changes

vendor/vendor.json Outdated Show resolved Hide resolved

urso reviewed Jan 10, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Show resolved Hide resolved

urso reviewed Jan 10, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Outdated Show resolved Hide resolved

urso reviewed Jan 10, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Outdated Show resolved Hide resolved

urso reviewed Jan 10, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Outdated Show resolved Hide resolved

urso reviewed Jan 10, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Outdated Show resolved Hide resolved

urso reviewed Jan 10, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Outdated Show resolved Hide resolved

cleanup of mage file, change tar directory structure, vendor cleanup

f8bdd11

fearful-symmetry added 3 commits January 10, 2020 11:48

notice

cca3c04

change target names to make the main makefile happy

b6934ba

add snapshot tag to magefile

c04c7e5

fearful-symmetry requested a review from urso January 13, 2020 18:09

mikemadden42 approved these changes Jan 13, 2020

View reviewed changes

jsoriano reviewed Jan 13, 2020

View reviewed changes

minor fixes

9a64512

jsoriano approved these changes Jan 13, 2020

View reviewed changes

x-pack/dockerlogbeat/magefile.go Outdated Show resolved Hide resolved

fix tempdir cleanup

b008cd7

fearful-symmetry merged commit b19bdf4 into elastic:master Jan 13, 2020

fearful-symmetry added a commit to fearful-symmetry/beats that referenced this pull request Jan 13, 2020

[Elastic Logging Plugin] Refactor of build tooling for docker plugin (e…

e648126

…lastic#15441) * refactor of build tooling for docker plugin (cherry picked from commit b19bdf4)

fearful-symmetry mentioned this pull request Jan 13, 2020

backport of 15441 to 7.x: [Elastic Logging Plugin] Refactor of build tooling for docker plugin #15520

Merged

fearful-symmetry added a commit that referenced this pull request Jan 14, 2020

[Elastic Logging Plugin] Refactor of build tooling for docker plugin (#…

01b9375

…15441) (#15520) * refactor of build tooling for docker plugin (cherry picked from commit b19bdf4)

fearful-symmetry removed the needs_backport PR is waiting to be backported to other branches. label Jan 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Elastic Logging Plugin] Refactor of build tooling for docker plugin #15441

[Elastic Logging Plugin] Refactor of build tooling for docker plugin #15441

fearful-symmetry commented Jan 9, 2020 •

edited

Loading

fearful-symmetry commented Jan 9, 2020

fearful-symmetry commented Jan 9, 2020

urso commented Jan 10, 2020 •

edited

Loading

urso Jan 10, 2020

fearful-symmetry Jan 10, 2020 •

edited

Loading

jsoriano Jan 13, 2020

fearful-symmetry commented Jan 10, 2020 •

edited

Loading

jmlrt commented Jan 10, 2020

urso commented Jan 10, 2020

fearful-symmetry commented Jan 10, 2020

fearful-symmetry commented Jan 10, 2020

mikemadden42 left a comment

jsoriano left a comment

jsoriano Jan 13, 2020 •

edited

Loading

fearful-symmetry Jan 13, 2020

jsoriano Jan 13, 2020

jsoriano Jan 13, 2020

jsoriano left a comment

	err = os.Chdir("../..")
	err = os.Chdir(devtools.XPackBeatDir())

[Elastic Logging Plugin] Refactor of build tooling for docker plugin #15441

[Elastic Logging Plugin] Refactor of build tooling for docker plugin #15441

Conversation

fearful-symmetry commented Jan 9, 2020 • edited Loading

fearful-symmetry commented Jan 9, 2020

fearful-symmetry commented Jan 9, 2020

urso commented Jan 10, 2020 • edited Loading

urso Jan 10, 2020

Choose a reason for hiding this comment

fearful-symmetry Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

jsoriano Jan 13, 2020

Choose a reason for hiding this comment

fearful-symmetry commented Jan 10, 2020 • edited Loading

jmlrt commented Jan 10, 2020

urso commented Jan 10, 2020

fearful-symmetry commented Jan 10, 2020

fearful-symmetry commented Jan 10, 2020

mikemadden42 left a comment

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

jsoriano Jan 13, 2020 • edited Loading

Choose a reason for hiding this comment

fearful-symmetry Jan 13, 2020

Choose a reason for hiding this comment

jsoriano Jan 13, 2020

Choose a reason for hiding this comment

jsoriano Jan 13, 2020

Choose a reason for hiding this comment

jsoriano left a comment

Choose a reason for hiding this comment

fearful-symmetry commented Jan 9, 2020 •

edited

Loading

urso commented Jan 10, 2020 •

edited

Loading

fearful-symmetry Jan 10, 2020 •

edited

Loading

fearful-symmetry commented Jan 10, 2020 •

edited

Loading

jsoriano Jan 13, 2020 •

edited

Loading