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

Build: Streamline and sync dockerfiles #58101

Merged
merged 4 commits into from Nov 28, 2022
Merged

Build: Streamline and sync dockerfiles #58101

merged 4 commits into from Nov 28, 2022

Conversation

DanCech
Copy link
Collaborator

@DanCech DanCech commented Nov 2, 2022

While working on #58005 I noticed many inconsistencies between our various dockerfiles as well as opportunities to streamline them.

This PR attempts to take the best parts of our various dockerfiles and align between alpine and ubuntu as well as between dev and packaging. It also syncs and streamlines the custom dockerfiles.

  • adds a header to the 2 main dockerfiles reminding devs that they need to be kept in sync
  • optimizes the build containers for cacheability (separating dependencies from our code)
  • combines RUN statements to reduce layer count
  • standardizes multi-line RUN indentation
  • combines and removes unneeded COPY statements
  • uses USER "$GF_UID" across all images for maximum compatibility
  • standardizes on /tmp/grafana for build & temp files
  • standardizes on ARG GF_GID="0"
  • removes unneeded apk/apt upgrade in custom dockerfiles
  • switches from gdebi to apt-get for chrome installation

After the dockerfiles were in sync I observed that the differences between alpine and ubuntu dockerfiles were relatively small and isolated to the RUN commands and underlying images, so by updating the RUN commands to detect alpine or ubuntu and updating the build scripts to pass the correct images as build args we can consolidate to 2 docker images.

In a follow-up PR we should be able to use buildkit to combine the main and packaging dockerfiles into a single dockerfile that detects whether a tarball is provided and either extracts that or builds the source.

@grafanabot
Copy link
Contributor

@DanCech DanCech marked this pull request as ready for review November 7, 2022 15:30
@grafanabot
Copy link
Contributor

@grafanabot
Copy link
Contributor

@DanCech DanCech changed the title streamline and sync dockerfiles Build: streamline and sync dockerfiles Nov 10, 2022
@DanCech DanCech added backport v9.2.x Mark PR for automatic backport to v9.2.x add to changelog labels Nov 10, 2022
@grafanabot
Copy link
Contributor

@DanCech DanCech changed the title Build: streamline and sync dockerfiles Build: Streamline and sync dockerfiles Nov 10, 2022
@DanCech DanCech added this to the 9.2.x milestone Nov 10, 2022
@DanCech DanCech requested a review from a team November 16, 2022 19:14
@zuchka
Copy link
Contributor

zuchka commented Nov 18, 2022

this PR potentially fixes #58921

@mpeaton-ng
Copy link

This does build cleanly for me, less some minor mods for injecting ca-certifcates. However it failes at runtime:
/run.sh: line 80: exec: grafana-server: not found

@DanCech
Copy link
Collaborator Author

DanCech commented Nov 20, 2022

This does build cleanly for me, less some minor mods for injecting ca-certifcates. However it failes at runtime: /run.sh: line 80: exec: grafana-server: not found

Hmm, might be an issue with the PATH changes, thanks for the feedback I'll check it out.

@DanCech
Copy link
Collaborator Author

DanCech commented Nov 20, 2022

This does build cleanly for me, less some minor mods for injecting ca-certifcates. However it failes at runtime: /run.sh: line 80: exec: grafana-server: not found

Hmm, might be an issue with the PATH changes, thanks for the feedback I'll check it out.

I haven't been able to duplicate the problem, test images built using all 4 of these methods work as expected:

make build-docker-full
make build-docker-full-ubuntu
GRAFANA_TGZ=grafana-9.2.3.linux-amd64.tar.gz ./build.sh --fast 9.2.3
GRAFANA_TGZ=grafana-9.2.3.linux-amd64.tar.gz ./build.sh --fast --ubuntu 9.2.3

You mentioned mods to inject ca-certificates, did you possibly do something that would change the PATH setting?

We set the path to ENV PATH="/usr/share/grafana/bin:$PATH" which is what allors run.sh to find the grfana-server binary without an explicit path being specified, so if the PATH does not include /usr/share/grafana/bin it won't work.

@mpeaton-ng
Copy link

mpeaton-ng commented Nov 21, 2022 via email

@DanCech
Copy link
Collaborator Author

DanCech commented Nov 21, 2022

I did not. I did, however, what I believe to be a working patch to main in the comments on make build-docker-full* fails · Issue #58921

That should not be required with this branch, it includes an update that fixes the problem that patch was trying to solve.

@zoltanbedi zoltanbedi removed this from the 9.2.x milestone Nov 22, 2022
@DanCech DanCech merged commit 9fec54d into main Nov 28, 2022
@DanCech DanCech deleted the streamline-docker branch November 28, 2022 19:43
@zuchka zuchka linked an issue Nov 28, 2022 that may be closed by this pull request
@grongor
Copy link

grongor commented Nov 30, 2022

Hi! I might be wrong, but shouldn't there be a semicolon before the backslash? https://github.com/grafana/grafana/pull/58101/files#diff-dfdb3f200d18eadfb7dca9193b67c2b8b5c8e59472317372b14bb9f3ad1fe04fR15

if [ $GF_INSTALL_IMAGE_RENDERER_PLUGIN = "true" ]; then \
  apk add --no-cache udev ttf-opensans chromium \
fi

->

if [ $GF_INSTALL_IMAGE_RENDERER_PLUGIN = "true" ]; then \
  apk add --no-cache udev ttf-opensans chromium ; \
fi

@DanCech
Copy link
Collaborator Author

DanCech commented Nov 30, 2022

you're absolutely correct, I'll open a PR to fix that!

@grafanabot
Copy link
Contributor

Hello @DanCech!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

@grafanabot
Copy link
Contributor

The backport to v9.3.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new branch
git switch --create backport-58101-to-v9.3.x origin/v9.3.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x 9fec54df2dd37c68b26f0d59d809924ea3342cf1
# Push it to GitHub
git push --set-upstream origin backport-58101-to-v9.3.x
git switch main
# Remove the local backport branch
git branch -D backport-58101-to-v9.3.x

Then, create a pull request where the base branch is v9.3.x and the compare/head branch is backport-58101-to-v9.3.x.

@grafanabot grafanabot added the backport-failed Failed to generate backport PR. Please resolve conflicts and create one manually. label Dec 1, 2022
DanCech added a commit that referenced this pull request Dec 1, 2022
* streamline and sync dockerfiles

* improve go dependency cacheability

* unify alpine and ubuntu Dockerfiles

* include glibc support in locally-built alpine images

(cherry picked from commit 9fec54d)
kwapik pushed a commit to fluxninja/grafana that referenced this pull request Jan 3, 2023
* streamline and sync dockerfiles

* improve go dependency cacheability

* unify alpine and ubuntu Dockerfiles

* include glibc support in locally-built alpine images
DanCech added a commit that referenced this pull request Jan 30, 2023
* streamline and sync dockerfiles

* improve go dependency cacheability

* unify alpine and ubuntu Dockerfiles

* include glibc support in locally-built alpine images

(cherry picked from commit 9fec54d)
gotjosh pushed a commit that referenced this pull request Mar 13, 2023
* streamline and sync dockerfiles

* improve go dependency cacheability

* unify alpine and ubuntu Dockerfiles

* include glibc support in locally-built alpine images

(cherry picked from commit 9fec54d)
kminehart added a commit that referenced this pull request Mar 14, 2023
* Build: Streamline and sync dockerfiles (#58101)

---------

Co-authored-by: gotjosh <josue.abreu@gmail.com>
Co-authored-by: Kevin Minehart <kmineh0151@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

make build-docker-full* fails
8 participants