beats icon indicating copy to clipboard operation
beats copied to clipboard

Update to go 1.23.2.

Open blakerouse opened this issue 1 year ago • 13 comments

Proposed commit message

Update to the latest go 1.23.2.

Checklist

  • [x] My code follows the style guidelines of this project
  • [x] I have commented my code, particularly in hard-to-understand areas
  • [x] I have made corresponding changes to the documentation
  • [x] I have made corresponding change to the default configuration files
  • [x] I have added tests that prove my fix is effective or that my feature works
  • [x] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

blakerouse avatar Oct 02 '24 21:10 blakerouse

Pinging @elastic/elastic-agent-control-plane (Team:Elastic-Agent-Control-Plane)

elasticmachine avatar Oct 02 '24 21:10 elasticmachine

This pull request does not have a backport label. If this is a bug or security fix, could you label this PR @blakerouse? 🙏. For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit

mergify[bot] avatar Oct 02 '24 21:10 mergify[bot]

backport-8.x has been added to help with the transition to the new branch 8.x. If you don't need it please use backport-skip label and remove the backport-8.x label.

mergify[bot] avatar Oct 02 '24 21:10 mergify[bot]

buildkite test this

blakerouse avatar Oct 03 '24 12:10 blakerouse

@rowlandgeoff Any ideas on why this is only to fail to build on ARM with the golang-crossbuild?

blakerouse avatar Oct 04 '24 14:10 blakerouse

@rowlandgeoff Any ideas on why this is only to fail to build on ARM with the golang-crossbuild?

Not a clue, that's outside my knowledge. @v1v, any ideas here?

rowlandgeoff avatar Oct 04 '24 14:10 rowlandgeoff

Not a clue, that's outside my knowledge. @v1v, any ideas here?

I know very little about the Beats build system and the CI Buildkite pipelines, however

exec /crossbuild: exec format error

That's somehow related to the wrong architecture in the runner versus the architecture that was built for, to list one of them, see this as it contains some details about some other possible errors.

If you use 1.23.1, does it happen? Just curious if the issue is related to the docker golang-crossbuild or something else.

v1v avatar Oct 06 '24 16:10 v1v

This pull request is now in conflicts. Could you fix it? 🙏 To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b go-1.23.2 upstream/go-1.23.2
git merge upstream/main
git push upstream go-1.23.2

mergify[bot] avatar Oct 07 '24 18:10 mergify[bot]

I think I know what's happening here. I've been casually playing around with BuildKite (BK) and crossbuilding for some time.

Here cross compiling is failing on a linux/arm64 host.

Related issues for Go 1.23.{0,1,2}, supposed to be fixed in Go 1.23.3+:

  • https://github.com/golang/go/issues/69255
  • https://github.com/golang/go/issues/68976

To check, I tried with Go 1.22.8, it worked fine. See: https://github.com/elastic/beats/pull/41140/commits/4d8ca5c17f8115580693c1b8503701bbd24dd8ff (all checks passed) as part of this https://github.com/elastic/beats/pull/41140

But cross compile works where target is linux/arm64 and the host is linux/amd64.

$ PLATFORMS=linux/arm64 PACKAGES=tar.gz mage -v package

...
...

docker run \
  --env EXEC_UID=1000 \
  --env EXEC_GID=1000 \
  -v /home/ubuntu/.gvm/pkgsets/go1.23.2/global/pkg/mod:/go/pkg/mod:ro \
  --env DEV=false \
  --rm \
  --env GOFLAGS="-mod=readonly -buildvcs=false" \
  --env MAGEFILE_VERBOSE=true \
  --env MAGEFILE_TIMEOUT= \
  --env SNAPSHOT=false \
  -v /home/ubuntu/beats:/go/src/github.com/elastic/beats \
  -w /go/src/github.com/elastic/beats/metricbeat \
  docker.elastic.co/beats-dev/golang-crossbuild:1.23.2-arm \
  --build-cmd "build/mage-linux-amd64 golangCrossBuild" \
  --platforms linux/arm64

For example, this will work, local as well as CI when the host is linux/amd64. When the host is linux/arm64 the buildMage function makes a binary mage-linux-arm64 here which when exec'ed is creating the issue.

But I noticed one thing in our BK pipeline:

So, if you see the BK pipeline for metricbeat and others (except agentbeat):

  • Target: https://github.com/elastic/beats/blob/2ff38df906203ba041d1f5492a6007809ad0b066/.buildkite/metricbeat/pipeline.yml#L329

Do note that we are cross-compiling for linux/arm64 here as well.

  • Host: https://github.com/elastic/beats/blob/2ff38df906203ba041d1f5492a6007809ad0b066/.buildkite/metricbeat/pipeline.yml#L339

But still we have a step for Package for arm64. See:

  • Target: https://github.com/elastic/beats/blob/2ff38df906203ba041d1f5492a6007809ad0b066/.buildkite/metricbeat/pipeline.yml#L350
  • Host: https://github.com/elastic/beats/blob/2ff38df906203ba041d1f5492a6007809ad0b066/.buildkite/metricbeat/pipeline.yml#L361

Here we face the failure when using Go 1.23.2 most probably because of the aforementioned issue with Go 1.23.x i.e., trying to cross compile on a linux/arm64 targeting linux/arm64.

But see how it is done for agentbeat:

  • On host https://github.com/elastic/beats/blob/2ff38df906203ba041d1f5492a6007809ad0b066/.buildkite/x-pack/pipeline.xpack.agentbeat.yml#L53 (linux/amd64) we cross compile for all platforms https://github.com/elastic/beats/blob/2ff38df906203ba041d1f5492a6007809ad0b066/.buildkite/x-pack/pipeline.xpack.agentbeat.yml#L37

Single step for cross-building for the required platforms.

So, my question shouldn't we improve the pipelines to do the cross-compiling in one step like we are doing for agentbeat.

In this PR as well I made some improvements in BK pipeline for agentbeat by removing the commands that seemed necessary.

So, could you also check if we even need the additional step for metricbeat, packetbeat, etc. Is it intentional to have the "Packaging Linux arm64" step because of some reason?

shmsr avatar Oct 08 '24 12:10 shmsr

Thank you so much for the detailed reasoning behind why this was failing. I was shocked that a simple update of golang would cause the error, but it being an issue with upstream golang makes a lot of sense.

So, could you also check if we even need the additional step for metricbeat, packetbeat, etc. Is it intentional to have the "Packaging Linux arm64" step because of some reason?

@rowlandgeoff Would be best to answer that information, or some others from that team. I do not know, I was just trying to update to go 1.23.

blakerouse avatar Oct 08 '24 14:10 blakerouse

Do you think that this will also solve the issue raised in https://github.com/elastic/beats/pull/40755#issuecomment-2394113768 ? (Also tested in https://github.com/elastic/beats/pull/41129) ?

gizas avatar Oct 08 '24 14:10 gizas

@blakerouse Given the issues with upstream Golang and that we're probably ~3 weeks away from a 1.23.3 release, should we close this PR unmerged and wait for the 1.23.3 release?

ycombinator avatar Oct 14 '24 22:10 ycombinator

Support for QEMU as a platform in Go has always been limited and generally operates on a best-effort basis. I encountered numerous issues when implementing runtime and atomic operations for various architectures due to QEMU's limitations. If this is indeed a QEMU issue, it would be helpful to clearly identify which specific issue it is, so we can ensure it gets properly addressed.

Additionally, I wonder if it makes sense to set up daily or weekly gotip builds of golang-crossbuild. This would allow us to detect breaking changes and test cutting-edge features without waiting for a new release.

mauri870 avatar Oct 15 '24 19:10 mauri870