-
Notifications
You must be signed in to change notification settings - Fork 81
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
Fix test setup to run on ARM instances #3904
Conversation
This pull request does not have a backport label. Could you fix it @michel-laterman? 🙏
|
|
@@ -1,5 +1,6 @@ | |||
ARG GO_VERSION | |||
FROM docker.elastic.co/beats-dev/golang-crossbuild:${GO_VERSION}-main-debian11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking as someone seeing this for the first time, the fact that ARCH
can be main
is very surprising. Can we not use the built-in TARGETARCH
and use main
in the Dockerfile if the value is amd64
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or not use the built-in TARGETARCH
arg at all, as it seems misused in this case. Use GO_ARCH
(in line with GO_VERSION
) or another arbitrary name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main
arch seems a bit weird actually...
@michel-laterman Does this come from some quirk on golang-crossbuild docker tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it's defined in the crossbuild readme
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see elastic/golang-crossbuild#446.
I recommend using the Debian-X
version in the name of the Docker image tag. This might help with reproducibility.
That's how it's also done in Beats, see elastic/beats#34921
However in Elastic Agent -> https://github.com/elastic/elastic-agent/blob/d99b09b0769f6f34428321eedb00c0b4339c202b/dev-tools/mage/crossbuild.go#L243-L246 , I don't know the build system but it looks weird to me they use debian-8
but debian-10
in Beats.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or not use the built-in TARGETARCH arg at all, as it seems misused in this case. Use GO_ARCH (in line with GO_VERSION) or another arbitrary name?
I wanted to use TARGETARCH, because it lets us actually build a multiplatform image. But it looks like there isn't a way to conditionally use main
in the crossbuild image tag based on this. It'd be best if the crossbuild image was multiplatform as well - then this would be completely transparent, and we'd just do docker buildx build --platform=...
in the Makefile.
But as this cannot be done, I'm ok with the previous approach of using a custom variable here.
Incidentally, what are the chances of making the crossbuild image multiplatform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@v1v is there a specific reason why main
is used instead of x86/amd64 ? I see the list of the images in golang-crossbuild README and the main
arch sticks out while all the other archs follow more conventional naming (arm, mips, ppc etc.)
But at least I can see now why main
is used as default arg here, to target x86/amd64 architecture
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe main
is a specific folder:
See https://github.com/elastic/golang-crossbuild#gomain-docker-image
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed the name variable name to SUFFIX
to distance it from the TARGETARCH/ARCH
concepts as it includes more information than just the architecture.
I also set it to use the same main-debian11
suffix as our other targets use.
I'm more then happy to change to TARGETARCH
if the upstream sources support it.
@michel-laterman I just looked at a slack conversation between @v1v and @jmlrt concluding that there was a recent change on the elasticsearch side that removed those images suffixed with the arch value.
|
This pull request is now in conflicts. Could you fix it @michel-laterman? 🙏
|
buildkite test this |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fix test setup to run on ARM instances (cherry picked from commit 2df9042)
Fix test setup to run on ARM instances (cherry picked from commit 2df9042) Co-authored-by: Michel Laterman <[email protected]>
What is the problem this PR solves?
Allow tests to run off arm platforms
How does this PR solve the problem?
Remove hard-coded
amd64
values from test setupHow to test this PR locally
make test-int
ormake test-e2e
Related issues