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

Backport of env2yaml arm64 Docker build context fixes #16063

Open
wants to merge 4 commits into
base: 7.17
Choose a base branch
from

Conversation

robbavey
Copy link
Member

@robbavey robbavey commented Apr 5, 2024

This commit contains fixes made to the main 8.x branch to generate arm64 and amd64 env2yaml binaries, copy them into the build context and correctly identify the correct binary in the Dockerfile to include in the Docker image.

A clean backport was not available due to the change in format of Dockerfile template from j2 to erb in #15142

This contains fixes from #15980 and #16053.

Release notes

What does this PR do?

Why is it important/What is the impact to the user?

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 (and/or docker env variables)
  • I have added tests that prove my fix is effective or that my feature works

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

Logs

@robbavey
Copy link
Member Author

robbavey commented Apr 9, 2024

COPY env2yaml/env2yaml-amd64 env2yaml/
COPY env2yaml/env2yaml-arm64 env2yaml/

RUN env2yamlarch="$(arch)"; \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should address the comment made here docker-library/official-images#16520 (comment), to use the pkg manager to get the architecture.

docker/templates/Dockerfile.j2 Show resolved Hide resolved
docker/templates/Dockerfile.j2 Show resolved Hide resolved
docker/templates/Dockerfile.j2 Outdated Show resolved Hide resolved
@robbavey
Copy link
Member Author

Ready for another round - buildkite exhaustive tests are failing before they reach the Docker tests, unfortunately

@jsvd jsvd self-requested a review April 16, 2024 10:08
@jsvd
Copy link
Member

jsvd commented Apr 16, 2024

I kicked off exhaustive tests here: https://buildkite.com/elastic/logstash-exhaustive-tests-pipeline/builds/396

@jsvd
Copy link
Member

jsvd commented Apr 17, 2024

@robbavey looks like we're hitting issues with j2 templating:

jinja2.exceptions.TemplateSyntaxError: Encountered unknown tag 'arch_command'. Jinja was looking for the following tags: 'elif' or 'else' or 'endif'. The innermost block that needs to be closed is 'if'.

more at https://buildkite.com/elastic/logstash-exhaustive-tests-pipeline/builds/396#018ee662-c331-4c4f-b3f5-ca8ef8c2cc22/112-6001

@@ -16,13 +16,15 @@
{% if image_flavor == 'ubi8' -%}
{% set base_image = 'docker.elastic.co/ubi8/ubi-minimal' -%}
{% set package_manager = 'microdnf' -%}
{% arch_command = 'uname -m' -%}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're missing a set here?

Suggested change
{% arch_command = 'uname -m' -%}
{% set arch_command = 'uname -m' -%}

# Minimal distributions do not ship with en language packs.
{% set locale = 'C.UTF-8' -%}
{% elif image_flavor == 'ironbank' -%}
{% set package_manager = 'yum' -%}
{% else -%}
{% set base_image = 'ubuntu:20.04' -%}
{% set package_manager = 'apt-get' -%}
{% arch_command = 'dpkg --print-architecture' -%}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same:

Suggested change
{% arch_command = 'dpkg --print-architecture' -%}
{% set arch_command = 'dpkg --print-architecture' -%}

This commit contains fixes made to the main 8.x branch to generate
arm64 and amd64 env2yaml binaries, copy them into the build context
and correctly identify the correct binary in the Dockerfile to
include in the Docker image.

A clean backport was not available due to the change in format of
Dockerfile template from j2 to erb in elastic#15142

This contains fixes from elastic#15980 and elastic#16053.
Use `dpkg --print-architecture`
Combine COPY commands to reduce layers
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

# Copy over the appropriate env2yaml artifact
COPY env2yaml/env2yaml-amd64 env2yaml/env2yaml-arm64 env2yaml/

RUN env2yamlarch="$({{ arch_command }})"; \
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you've got the hood open, I'd also suggest set -eux here (at the very least set -e so failures propagate correctly):

Suggested change
RUN env2yamlarch="$({{ arch_command }})"; \
RUN set -eux; \
env2yamlarch="$({{ arch_command }})"; \

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants