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 to go 1.23.2. #41087

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Update to go 1.23.2. #41087

wants to merge 2 commits into from

Conversation

blakerouse
Copy link
Contributor

Proposed commit message

Update to the latest go 1.23.2.

Checklist

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

@blakerouse blakerouse added the Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team label Oct 2, 2024
@blakerouse blakerouse self-assigned this Oct 2, 2024
@blakerouse blakerouse requested review from a team as code owners October 2, 2024 21:31
@elasticmachine
Copy link
Collaborator

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

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Oct 2, 2024
Copy link
Contributor

mergify bot commented Oct 2, 2024

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

Copy link
Contributor

mergify bot commented Oct 2, 2024

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 mergify bot added the backport-8.x Automated backport to the 8.x branch with mergify label Oct 2, 2024
@pierrehilbert pierrehilbert requested review from mauri870 and removed request for khushijain21 October 3, 2024 07:35
@blakerouse
Copy link
Contributor Author

buildkite test this

@blakerouse
Copy link
Contributor Author

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

@rowlandgeoff
Copy link
Contributor

@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?

@v1v
Copy link
Member

v1v commented Oct 6, 2024

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.

Copy link
Contributor

mergify bot commented Oct 7, 2024

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

@shmsr
Copy link
Member

shmsr commented Oct 8, 2024

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+:

To check, I tried with Go 1.22.8, it worked fine. See: 4d8ca5c (all checks passed) as part of this #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):

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

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

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:

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?

@blakerouse
Copy link
Contributor Author

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.

@gizas
Copy link
Contributor

gizas commented Oct 8, 2024

Do you think that this will also solve the issue raised in #40755 (comment) ? (Also tested in #41129) ?

@ycombinator
Copy link
Contributor

@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?

@mauri870
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.x Automated backport to the 8.x branch with mergify Team:Elastic-Agent-Control-Plane Label for the Agent Control Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants