tessellator / openfaas-clojure-template

An OpenFaaS template for writing functions in Clojure
MIT License
9 stars 6 forks source link

Update build system #8

Closed XVincentX closed 2 years ago

XVincentX commented 2 years ago

Hey,

I have been using this template for a bit of time and have modernized it a bit. Let me know if these changes make sense for you!

  1. Use Clojure official images instead of downloading the CLI manually
  2. Move to OpenJDK 17, since it's LTS
  3. Use latest Watchdog version
  4. Use the new CLI tools.build system
  5. Remove the cpugroup flags since the new JDK uses container limits automatically
tessellator commented 2 years ago

Thank you for your contribution!

I have done some testing locally, and it is working for me. The only issue I see is that this change removes the ability to apply a local manifest.mf file to the uberjar. Would you mind restoring that capability? I will point out the changes necessary in code review comments.

It appears from digging around on Docker Hub that this change will support ARM64. @ccfontes would you mind double-checking this, please? (I unfortunately do not have an ARM64 machine to test on.) If it is supported, we will also need to update the README with updated (fewer!) instructions.

XVincentX commented 2 years ago

@tessellator Changes addressed. I also do not have unfortunately a M1 or any ARM64 machine.

tessellator commented 2 years ago

Thank you! I'll give Carlos a couple of days to get back to us on the ARM64 support before merging.

ccfontes commented 2 years ago

@tessellator I'll give it a try tomorrow (Monday).

tessellator commented 2 years ago

Perfect, thank you!

ccfontes commented 2 years ago

@tessellator , Works On My Machine with the changes below (I tried the commented lines):

As before, we need to add this for arm64 fwatchdog:

ARG ARCH=amd64
# ARG ARCH=arm64

For OpenJDK 17:

FROM openjdk:17-alpine as ship
# FROM bellsoft/liberica-openjdk-alpine:17-35-aarch64 as ship
XVincentX commented 2 years ago

This is weird; the base image for OpenJDK I am using for both Clojure and the final runtime support Arm64 out of the box, so the change in the base image should not be necessary.

Additionally speaking, I have also discovered that the of-watchdog has multi-arch Docker images published on ghcr — so I've replaced this file to use them instead of having to pull the data on our own and detect the architecture.

ccfontes commented 2 years ago

@XVincentX

Using your Dockerfile previous to changes in 9f59c7a, with ARG ARCH=amd64, it works after all. I'm experimenting with the hello world function from this repo. Note that more complex code could trigger some issue, as it was the case for me in the past. I don't have another app at hands reach to try.

Using your Dockerfile with changes in 9f59c7a, of-watchdog still works.

With FROM openjdk:17-alpine as ship I get:

 > [internal] load metadata for docker.io/library/openjdk:17-alpine:
------
failed to solve with frontend dockerfile.v0: failed to create LLB definition: no match for platform in manifest sha256:4b6abae565492dbe9e7a894137c966a7485154238902f2f25e9dbd9784383d81: not found
XVincentX commented 2 years ago

It seems like I am then wrong and openjdk:17 does not provide ARM64 images 🤔

ccfontes commented 2 years ago

@XVincentX

With FROM openjdk:17-alpine as ship, and export DOCKER_BUILDKIT=0 I get:

Errors received during build:
- [my-function] received non-zero exit code from build, error: no matching manifest for linux/arm64/v8 in the manifest list entries
XVincentX commented 2 years ago

From your error thought it appears that the clojure:openjdk-17-tools-deps has an arm64 version. So confusing.

I do not know what to do about it, I guess we can either leave it as it is or…if anybody has an alternative, great.

ccfontes commented 2 years ago

Replace:

FROM openjdk:17-alpine as ship

with:

FROM clojure:openjdk-17-tools-deps as ship

and remove:

RUN addgroup -S app && adduser -S app -G app
USER app

Now it works. The image is the same as in the first stage, so no need to download again!

@tessellator What is the rationale for using Alpine?

XVincentX commented 2 years ago

I guess less dependencies and smaller images

Sent from my iPhone

On Oct 4, 2021, at 5:39 PM, Carlos @.***> wrote:

 Replace:

FROM openjdk:17-alpine as ship with:

FROM clojure:openjdk-17-tools-deps as ship and remove:

RUN addgroup -S app && adduser -S app -G app USER app Now it works. The image is the same as in the first stage, so no need to download again!

@tessellator What is the rationale for using Alpine?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

tessellator commented 2 years ago

Yeah, I was using Alpine for smaller images. I'm okay using a non-Alpine image if necessary. However, we should not need a clojure:* image as the basis for our shipped image since we're building an uberjar.

tessellator commented 2 years ago

What if we were to leave comments in for swapping the images for ARM64 like we had before? Or maybe it is worth splitting out an arm version into its own template?

What do y'all think?

XVincentX commented 2 years ago

I vote to leave this as it is

ccfontes commented 2 years ago

I vote for leaving comments in for swapping the images for ARM64, but have a disclaimer at the top of the readme for ARM64/M1 users linking to https://github.com/tessellator/openfaas-clojure-template#arm64-support documentation.

The reasoning is some users may not even consider looking further after encountering their first issue. I may be wrong, and users of this project no way are going to give up!

XVincentX commented 2 years ago

I agree. And if one day (hopefully not too long in the future) they’ll provide OpenJDK for Arm64 the current code would just work out of the box.

V.

On Oct 5, 2021, at 08:36, Carlos @.***> wrote:

 I vote for leaving comments in for swapping the images for ARM64, but have a disclaimer at the top of the readme for ARM64/M1 users linking to https://github.com/tessellator/openfaas-clojure-template#arm64-support documentation.

The reasoning is some users may not even consider looking further after encountering their first issue. I may be wrong, and users of this project no way are going to give up!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

XVincentX commented 2 years ago

One more thing folks. While checking out the Docker Images tag for OpenJDK I just noticed this:

image

That is enough to me not to use it for production. It might make sense to use just openjdk:17 instead of the alpine variant. I took the same that the Clojure CLI is using (slim-bullseye)

It might be a bigger in size, but at least we'd not go in the dangerous path of testing stuff for other people.

Additionally speaking, it seems like the alpine variant is shipping with potentially vulnerable software:

image

Thoughts?


I have also modified the template to run java with the -jar option and specify the main class in the :manifest section of build.clj. Nothing should be changing though.

XVincentX commented 2 years ago

I have also fixed the cli-kondo configuration. It was previously broken reporting unused var that we do not really care about.

tessellator commented 2 years ago

That is enough to me not to use it for production. It might make sense to use just openjdk:17 instead of the alpine variant. I took the same that the Clojure CLI is using (slim-bullseye)

It might be a bigger in size, but at least we'd not go in the dangerous path of testing stuff for other people.

Yeah, this is a good point. Let's go ahead and use the image you suggested. Thank you for catching this!

ccfontes commented 2 years ago

@XVincentX nice one! 💯

XVincentX commented 2 years ago

What else is required on my side to see this merged?

tessellator commented 2 years ago

I think this looks good. I'm going to go ahead and merge this and we can address any issues that crop up in new PRs. Thank you so much @XVincentX for making these changes!