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 Dockerfile #62

Merged
merged 7 commits into from Sep 7, 2022
Merged

Update Dockerfile #62

merged 7 commits into from Sep 7, 2022

Conversation

modem7
Copy link
Contributor

@modem7 modem7 commented Sep 5, 2022

Heya,

Thanks for the project! Looks great so far, I thought I'd do a bit of legwork given that amount of feature requests I put in!

  • Optimised Dockerfile to utilise buildkit.
  • Removed base stage as this effectively did nothing bar add time to build.
  • Moved labels to the runner stage as this applies them natively (the metadata exporter should still be able to grab from the new label location).
  • Disabled telemetry for Next.js.
  • Removed apk del instruction as this stage gets discarded anyway, so there's no need for it.
  • Optimised build time from 3.5 minutes down to around 2.8 minutes (this could potentially come down in speed with cache mounts).
  • Added Docker healthcheck

Notes:
In the github deploy action, it may be worthwhile using the gha cache rather than local (as per https://github.com/docker/build-push-action/blob/master/docs/advanced/cache.md#github-cache (albeit, this is still considered experimental)).

mode=max would be required as this is a multistage image. mode=min (default) only caches the last stage typically.

https://github.com/benphelps/homepage/blob/cf3939592490d266e87b9ec27d5a692ba2b3d210/.github/workflows/docker-publish.yml#L99

Command to test

This is the command I'm using to build the images locally:
DOCKER_BUILDKIT=1 docker build --no-cache -t homepage:test .

@modem7
Copy link
Contributor Author

modem7 commented Sep 5, 2022

Regarding caching the apk and pnpm downloads (as it's around 310mb of packages), it may be worthwhile doing something like:

RUN --mount=type=cache,id=apk,sharing=locked,target=/var/cache/apk \
    <<EOF
    set -xe
    apk add libc6-compat
    apk add --virtual .gyp python3 make g++
    yarn global add pnpm
EOF

RUN --mount=type=cache,id=pnpm-store,target=/root/.local/share/pnpm/store pnpm fetch | grep -v "cross-device link not permitted\|Falling back to copying packages from store"

RUN --mount=type=cache,id=pnpm-store,target=/root/.local/share/pnpm/store pnpm install -r --offline

That way the last two RUN commands are cached between builds.

Example Dockerfile proposal
# syntax = docker/dockerfile:latest

# Install dependencies only when needed
FROM node:16-alpine AS deps

WORKDIR /app

COPY --link package.json pnpm-lock.yaml* ./

RUN --mount=type=cache,id=apk,sharing=locked,target=/var/cache/apk \
    <<EOF
    set -xe
    apk add libc6-compat
    apk add --virtual .gyp python3 make g++
    yarn global add pnpm
EOF

RUN --mount=type=cache,id=pnpm-store,target=/root/.local/share/pnpm/store pnpm fetch | grep -v "cross-device link not permitted\|Falling back to copying packages from store"

RUN --mount=type=cache,id=pnpm-store,target=/root/.local/share/pnpm/store pnpm install -r --offline

# Rebuild the source code only when needed
FROM node:16-alpine AS builder
WORKDIR /app

COPY --link --from=deps /app/node_modules ./node_modules/
COPY . .

RUN <<EOF
    set -xe
    yarn next telemetry disable
    npm run build
EOF

# Production image, copy all the files and run next
FROM node:16-alpine AS runner
LABEL org.opencontainers.image.title "Homepage"
LABEL org.opencontainers.image.description "A self-hosted services landing page, with docker and service integrations."
LABEL org.opencontainers.image.url="https://github.com/benphelps/homepage"
LABEL org.opencontainers.image.documentation='https://github.com/benphelps/homepage/wiki'
LABEL org.opencontainers.image.source='https://github.com/benphelps/homepage'
LABEL org.opencontainers.image.licenses='Apache-2.0'

ENV NODE_ENV production

WORKDIR /app

# Copy files from context (this allows the files to copy before the builder stage is done).
COPY --link package.json next.config.js ./
COPY --link --chmod=755 healthcheck.js ./
COPY --link /public ./public

# Copy files from builder
COPY --link --from=builder /app/.next/standalone ./
COPY --link --from=builder /app/.next/static/ ./.next/static/

HEALTHCHECK --interval=12s --timeout=12s --start-period=5s \  
    CMD node ./healthcheck.js

EXPOSE 3000
ENV PORT 3000
CMD ["node", "server.js"]

@benphelps
Copy link
Member

Would a Next API endpoint be better for the healthcheck? That way it's tied directly to the actual API server / app server process.

@modem7
Copy link
Contributor Author

modem7 commented Sep 6, 2022

Would a Next API endpoint be better for the healthcheck? That way it's tied directly to the actual API server / app server process.

Absolutely! If that can be created/exposed, we can probably just drop the script altogether and curl/wget (both already in the image) the endpoint, saving a bit of hassle and config later down the line.

@benphelps
Copy link
Member

As for the GHA cache, I'm running the build on a self-hosted runner (VM on a Ryzen 9 5950X box with 24GB RAM allocated to it, as the builds where taking 30+ minutes before).

I've tried both gha and local as options for the caching mechanism, and nether of them seemed to make much of a difference between the two, using local, it still downloads a lot of cache data from github (so it doesn't seem very local to me). I really just gave up as it was "fast enough".

@modem7
Copy link
Contributor Author

modem7 commented Sep 6, 2022

As for the GHA cache, I'm running the build on a self-hosted runner (VM on a Ryzen 9 5950X box with 24GB RAM allocated to it, as the builds where taking 30+ minutes before).

I've tried both gha and local as options for the caching mechanism, and nether of them seemed to make much of a difference between the two, using local, it still downloads a lot of cache data from github (so it doesn't seem very local to me). I really just gave up as it was "fast enough".

If it's a local cache, try the mode=max syntax + add the cache mounts on the run commands, that should speed things up.

As an example, I've done one here: https://hub.docker.com/r/modem7/conda/tags one is the image, one is the multi-stage cache export (although in this case it should work far better than my example as there's fewer (different) moving parts).

Here's the drone file for the above example https://github.com/modem7/conda/blob/master/.drone.yml

@benphelps
Copy link
Member

If you want, you can apply the apk and pnpm cache solution you gave above and I'll rerun the action.

And the healthcheck could be changed to (I just pushed an endpoint for it):

HEALTHCHECK --interval=10s --timeout=3s \
  CMD curl -f http://localhost:3000/api/healthcheck || exit 1

@benphelps
Copy link
Member

The current cache settings are:

cache-from: type=local,src=/tmp/.buildx-cache
cache-to: type=local,dest=/tmp/.buildx-cache-new,mode=max

I've followed the examples here for local cache: https://github.com/docker/build-push-action/blob/master/docs/advanced/cache.md

But it's still grabbing almost 600mb of cache from GitHub when the action starts, so something for sure seems off.

@modem7
Copy link
Contributor Author

modem7 commented Sep 6, 2022

cache-from: type=local,src=/tmp/.buildx-cache
cache-to: type=local,dest=/tmp/.buildx-cache-new,mode=max

cache-from should surely be /tmp/.buildx-cache-new?

It should be caching to and from the same location

@modem7
Copy link
Contributor Author

modem7 commented Sep 6, 2022

That should be all done.

The cache seems to be working nicely (SS was taken during build after merge)
image

Seems I was wrong about curl, but wget sufficed nicely.

@benphelps
Copy link
Member

cache-from: type=local,src=/tmp/.buildx-cache cache-to: type=local,dest=/tmp/.buildx-cache-new,mode=max

cache-from should surely be /tmp/.buildx-cache-new?

It should be caching to and from the same location

This is because of a bug, the final build step removes the old folder and replaces it with the new folder.

@benphelps benphelps merged commit 6a23894 into gethomepage:main Sep 7, 2022
@benphelps
Copy link
Member

The apk cache was causing some issues so I've disabled it. Works locally when building for a single arch, so it must have to do with buildx.

@modem7
Copy link
Contributor Author

modem7 commented Sep 7, 2022

Tbf, not actually sure how much value-add the apk cache added given we're not using apk elsewhere, so probably a good thing to keep it simple(r).

@modem7 modem7 deleted the dockerfile-optimise branch September 7, 2022 10:34
Copy link
Contributor

github-actions bot commented Feb 6, 2024

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new discussion for related concerns.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants