X-Pack binaries #7783

exekias · 2018-07-27T16:15:21Z

This change brings with-xpack binaries building by doing this:

Introduce x-pack/<beatname>/main.go and its cmd package. These take oss beats RootCmd and inject X-Pack features on it.

The existing magefile.go gains awareness of this and uses it to build and package the x-pack version.

I believe this is the minimal change we would need to just have different binaries. If modules are introduced they will require more packaging changes to take them into account.

ruflin

Idea LGTM.

One thing we probably will need later is to also be able to build a custom config file.

ruflin · 2018-07-30T05:50:33Z

dev-tools/mage/crossbuild.go

-			if err := builder.Build(); err != nil {
-				return errors.Wrapf(err, "failed cross-building target=%v for platform=%v",
-					params.Target, buildPlatform.Name)
+		for _, xpack := range xpack {


I first stumbled over this line of code and was thinking "why is there a list of x-pack options"? I now get that it either has 1 or 2 entries for with and without x-pack. I wonder if a better name for the slice would be buildOptions or similar??

ruflin · 2018-07-30T05:51:51Z

dev-tools/mage/crossbuild.go

+			workDir = filepath.ToSlash(filepath.Join(workDir, "x-pack", repoInfo.SubDir))
+			buildCmd = filepath.Join("..", "..", repoInfo.SubDir, buildCmd)
+		} else {
+			workDir = filepath.ToSlash(filepath.Join(workDir, repoInfo.SubDir))


What about seeting this line above b.Xpack check and then only have an if that overwrites it? Would be more similar to what you do with buildCmd. Outcome is the same.

andrewkroh · 2018-07-30T15:19:56Z

dev-tools/mage/clean.go

@@ -35,6 +35,9 @@ var DefaultCleanPaths = []string{
 	"_meta/kibana.generated",
 	"_meta/kibana/5/index-pattern/{{.BeatName}}.json",
 	"_meta/kibana/6/index-pattern/{{.BeatName}}.json",
+
+	"../x-pack/{{.BeatName}}/build",
+	"../x-pack/{{.BeatName}}/{{.BeatName}}",


What about .exe for Windows.

andrewkroh · 2018-07-30T17:55:47Z

dev-tools/mage/crossbuild.go

@@ -75,6 +82,7 @@ type crossBuildParams struct {
 	Platforms BuildPlatformList
 	Target    string
 	Serial    bool
+	WithXPack bool


In order to omit any XPack knowledge from the CrossBuild function and the crossBuildParams struct, I suggest creating a separate build target like crossBuildXPack in the filebeat/magefile.go that invokes a new function mage.CrossBuildXPack() that is doing CrossBuild(WithTarget("buildXPack"), InDir("x-pack", BeatName)).

And then add crossBuildXPack as a dependency of package.

Thank you for your input @andrewkroh, I pushed a new change, let me know if this is looking better now. Once we are settled with how this should work I will extend it to all beats + tackle testing

andrewkroh · 2018-07-30T19:07:54Z

dev-tools/mage/crossbuild.go

 	if repoInfo.SubDir != "" {
 		workDir = filepath.ToSlash(filepath.Join(workDir, repoInfo.SubDir))
+		if b.XPack {
+			workDir = filepath.ToSlash(filepath.Join(workDir, "x-pack", repoInfo.SubDir))
+			buildCmd = filepath.Join("..", "..", repoInfo.SubDir, buildCmd)


If you do add some kind of target/work dir parameter to GolangCrossBuilder then I think you can use filepath.Rel here to get a relative path from the current working directory to the target dir.

vjsamuel · 2018-07-30T23:36:54Z

@exekias we were discussing offline on this one. it would be good if this approach allows community beats to be able to bundle their own custom processors/outputs into their custom beat. this is to overcome the short comings of lack of go-plugins in Windows.

houndci-bot · 2018-07-31T13:55:31Z

filebeat/magefile.go

@@ -56,6 +56,11 @@ func CrossBuild() error {
 	return mage.CrossBuild()
 }

+// CrossBuild cross-builds the beat with XPack for all target platforms.


comment on exported function CrossBuildXPack should be of the form "CrossBuildXPack ..."

andrewkroh

I happy with the current approach. It looks sufficiently simple.

andrewkroh · 2018-07-31T14:46:00Z

x-pack/filebeat/cmd/root.go

@@ -0,0 +1,10 @@
+package cmd


You are the lucky person who adds the first Elastic licensed Go code to the project. This code needs to have the Elastic license header and we need to figure out a way to enforce these checks.

My preferred way would be to make https://github.com/elastic/go-licenser handle more license types.

And I assume that the top-level make check fails because these files do not have Apache headers (I didn't check). That can be addressed by merging elastic/go-licenser#14 and using the new -exclude flag.

I opened elastic/go-licenser#15 to handle this, will update this PR once that's merged

Thanks for adding this @exekias I just created #7837 which leverage your work :)

Personally I use beatsfmt. This one searches for a .go_license_header file relative to the source file to be formatted.

andrewkroh · 2018-07-31T14:53:23Z

dev-tools/mage/crossbuild.go

@@ -120,6 +127,11 @@ func CrossBuild(options ...CrossBuildOption) error {
 	return nil
 }

+// CrossBuildXPack executes a given build target once for each target platform, including xpack.


How about:

// CrossBuildXPack executes the 'golangCrossBuild' target in the Beat's // associated x-pack directory to produce a version of the Beat that contains // Elastic licensed content.

andrewkroh · 2018-07-31T15:00:43Z

dev-tools/mage/crossbuild.go

@@ -63,6 +63,13 @@ func WithTarget(target string) func(params *crossBuildParams) {
 	}
 }

+// InDir specifies the base directory to use when cross-building.


I think it's important to document that the directory is automatically suffixed with the current sub-directory path relative to the repo root. I wasn't expecting this.

I was expecting to the see it used like InDir(filepath.Join("x-pack", BeatName)). Or make the signature be InDir(path ...string) and automatically invoke filepath.Join(path...) in the implementation.

I can do that, but I don't have the BeatName in CrossBuild, it's just using the current subidr. In filebeat/magefile.go I could pass InDir("filebeat") for oss build and InDir("x-pack", "filebeat") in the xpack. WDYT?

I think there is a variable named BeatName that is defined in settings.go that you can use. Does that work?

Got it, I updated this again, I think I finally understood what you meant :)

exekias · 2018-08-02T11:07:47Z

@andrewkroh I just saw APM is not copying our packages.yml (elastic/apm-server#1231) so I believe we can leave it as it is in this PR. WDYT?

andrewkroh · 2018-08-03T00:42:05Z

Then it seems fine just make sure APM is aware.

exekias · 2018-08-03T13:34:07Z

@simitt is working on introducing our default packages.yml back to apm-server (elastic/apm-server#1256) so this will need to move current packages.yml changes to a new beats-only file

ph · 2018-08-03T14:30:54Z

Makefile


 .PHONY: add-headers
 add-headers:
 	@go get github.com/elastic/go-licenser
-	@go-licenser
+	@go-licenser -exclude vendor -exclude .git -exclude x-pack
+	@go-licenser -exclude vendor -license Elastic x-pack


No need to exclude vendor or .git its in the default from go licenser.

https://github.com/elastic/go-licenser/blob/02324788cd84244b695b57aec2a4c388ea2d8fc8/main.go#L92

ph · 2018-08-03T15:23:16Z

filebeat/magefile.go

@@ -78,7 +83,7 @@ func Package() {
 	customizePackaging()

 	mg.Deps(Update, prepareModulePackaging)
-	mg.Deps(CrossBuild, CrossBuildGoDaemon)
+	mg.Deps(CrossBuild, CrossBuildXPack, CrossBuildGoDaemon)
 	mg.SerialDeps(mage.Package, TestPackages)
 }



This magefile change is missing for the other beats.

ph · 2018-08-03T18:46:02Z

I have updated this PR with what we were missing from our discussion.
I am running a make snapshot to make sure nothing is broken.

ph · 2018-08-06T18:36:13Z

I have fixed all the targets and paths and running make snapshot now produce the following artifacts.

ph · 2018-08-06T19:01:25Z

Failures will be fixed in #7882

ph · 2018-08-06T19:22:43Z

Rebased from master.

ph · 2018-08-07T12:52:44Z

The failure is not related to this change.

simitt · 2018-08-08T08:20:54Z

We discussed in the @elastic/apm-server team that it would be great to stick with copying the packages.yml as is, whenever we update the libbeat framework. APM Server has its own magefile.go where APM specific behaviour is defined. Copying the packages.yml allows us to stay in sync with changes in libbeat, that we might overlook otherwise.

Thus, keeping beats only changes in a separate definition, like suggested by @exekias would be great.

ph · 2018-08-08T13:48:28Z

@simitt I had a quick chat with @andrewkroh about this, we came to the conclusion that the current requirements is temporary and someday you have an x-pack folder for apm and you will build a xpack binary. But since the packing is done on our side we agree that we need to provide a migration path and allow futures changes to go back to you.

After looking at it, merging yaml or filtering was not an option for us. We have decided to add a new method UseElasticBeatWithoutXPackPackaging which will use the same packaging target that you currently use. So you will need to replace the UseElasticBeatPackaging with UseElasticBeatWithoutXPackPackaging in your magefile.

simitt · 2018-08-08T13:51:31Z

Sounds like a straight forward change on our side, thank you!.

andrewkroh

LGTM.

andrewkroh · 2018-08-09T05:21:21Z

dev-tools/mage/pkgspecs.go

+// that is purely OSS under Apache 2.0 and one that is licensed under the
+// Elastic License and may contain additional X-Pack features.
+//
+// NOTE: This method doesn't uses binaries produced in the x-pack folder, this is


s/uses/use/

andrewkroh · 2018-08-09T05:25:46Z

dev-tools/mage/build.go

@@ -91,7 +91,7 @@ func GolangCrossBuild(params BuildArgs) error {
 			"only be executed within the golang-crossbuild docker environment.")
 	}

-	defer DockerChown(filepath.Join(params.OutputDir, params.Name+binaryExtension(GOOS)))
+	defer DockerChown(params.OutputDir)


How come this was changed? Won't this be chowning files unnecessarily?

I've changed the logic of DockerChown to not recursively walk up the path, instead it walk down the path, using only params.outputDir will change the permission bellow that directory instead. ~~The other implementation had issues with using absolute paths.~~

I will revert it back, it will work with both logic.

ph · 2018-08-09T14:25:54Z

@andrewkroh I just updated the PR with your reviews comment.

ph · 2018-08-10T16:44:08Z

I've restarted the build, the make check failed with an SSL error when bootstraping python code.

This commit implements the necessary logic to build licensed beats, before that commit, the OSS and the License binaries were exactly the same. Changes: - Added `mage.CrossBuildXPack()` to build the elastic licensed beat. x-pack/beatname - Changed the packages.yml to include the artifact from the x-pack folder. - Added `mage.UseElasticBeatPackaging()` to build the packages with the new binaries. - Added `mage.UseElasticBeatWithoutXPackPackaging()` allow to keep the previous behavior. - `make check` and `make fmt` will use the right license for the x-pack folder. Co-authored-by: Pier-Hugues Pellerin <[email protected]>

ph · 2018-08-13T15:51:13Z

@andrewkroh I've looked at the two failures I don't think they are related to this change since other PR like #7954 have the same issue, maybe hiccups with the release manager? I haven't investigated a lot.

simitt · 2018-08-15T13:28:37Z

Updating APM Server to these changes worked smoothly, thanks to everyone involved!

This commit implements the necessary logic to build licensed beats, before that commit, the OSS and the License binaries were exactly the same. Changes: - Added `mage.CrossBuildXPack()` to build the elastic licensed beat. x-pack/beatname - Changed the packages.yml to include the artifact from the x-pack folder. - Added `mage.UseElasticBeatPackaging()` to build the packages with the new binaries. - Added `mage.UseElasticBeatWithoutXPackPackaging()` allow to keep the previous behavior. - `make check` and `make fmt` will use the right license for the x-pack folder. Co-authored-by: Carlos Pérez-Aradros Herce <[email protected]> Co-authored-by: Pier-Hugues Pellerin <[email protected]> (cherry picked from commit bcb0531)

exekias added :Packaging discuss Issue needs further discussion. labels Jul 27, 2018

urso requested a review from andrewkroh July 27, 2018 16:19

ruflin reviewed Jul 30, 2018

View reviewed changes

andrewkroh reviewed Jul 30, 2018

View reviewed changes

houndci-bot reviewed Jul 31, 2018

View reviewed changes

exekias force-pushed the xpack-binaries branch from 4e4f638 to a0eb3a3 Compare July 31, 2018 14:06

exekias added the in progress Pull request is currently in progress. label Jul 31, 2018

andrewkroh reviewed Jul 31, 2018

View reviewed changes

exekias force-pushed the xpack-binaries branch 3 times, most recently from 1c206cd to fbb5709 Compare July 31, 2018 15:47

ph mentioned this pull request Aug 1, 2018

Beatless initial PR #7837

Merged

exekias force-pushed the xpack-binaries branch from 4968e0a to a742eab Compare August 2, 2018 10:11

exekias added review needs_backport PR is waiting to be backported to other branches. v7.0.0-alpha1 v6.5.0 and removed discuss Issue needs further discussion. labels Aug 3, 2018

exekias changed the title ~~Xpack binaries~~ X-Pack binaries Aug 3, 2018

ph reviewed Aug 3, 2018

View reviewed changes

ph mentioned this pull request Aug 6, 2018

[Meta] Add FunctionBeat to run beat on serverless infrastructure #7880

Closed

42 tasks

ph force-pushed the xpack-binaries branch from 85402d7 to 12402fb Compare August 6, 2018 19:22

andrewkroh reviewed Aug 9, 2018

View reviewed changes

ph removed the in progress Pull request is currently in progress. label Aug 10, 2018

ph force-pushed the xpack-binaries branch from 35c6979 to 20e7089 Compare August 10, 2018 14:42

ph force-pushed the xpack-binaries branch from 20e7089 to 328d5e8 Compare August 13, 2018 12:34

andrewkroh merged commit bcb0531 into elastic:master Aug 13, 2018

simitt mentioned this pull request Aug 14, 2018

Update beats framework to 03f7e87 elastic/apm-server#1281

Merged

graphaelli removed the needs_backport PR is waiting to be backported to other branches. label Sep 25, 2018

graphaelli mentioned this pull request Sep 25, 2018

Cherry-pick #7783 to 6.x: X-Pack binaries #8442

Merged

graphaelli added the v6.5.0 label Sep 25, 2018

X-Pack binaries #7783

X-Pack binaries #7783

Conversation

exekias commented Jul 27, 2018 • edited Loading

ruflin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vjsamuel commented Jul 30, 2018

Choose a reason for hiding this comment

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exekias commented Aug 2, 2018

andrewkroh commented Aug 3, 2018

exekias commented Aug 3, 2018

Choose a reason for hiding this comment

ph Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

ph commented Aug 3, 2018

ph commented Aug 6, 2018

ph commented Aug 6, 2018

ph commented Aug 6, 2018

ph commented Aug 7, 2018

simitt commented Aug 8, 2018

ph commented Aug 8, 2018

simitt commented Aug 8, 2018

andrewkroh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph Aug 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ph commented Aug 9, 2018

ph commented Aug 10, 2018

ph commented Aug 13, 2018

simitt commented Aug 15, 2018

exekias commented Jul 27, 2018 •

edited

Loading

ph Aug 3, 2018 •

edited

Loading

ph Aug 9, 2018 •

edited

Loading