Skip to content
This repository has been archived by the owner on Oct 15, 2023. It is now read-only.

Add arm support #122

Merged
merged 1 commit into from
Jan 4, 2023
Merged

Add arm support #122

merged 1 commit into from
Jan 4, 2023

Conversation

li-boxuan
Copy link
Member

Closes #92

0.2/Dockerfile Outdated
@@ -39,7 +39,7 @@ RUN apt update -y && apt install -y gpg unzip curl && \
COPY conf/janusgraph-berkeleyje-lucene-server.properties conf/log4j-server.properties ${JANUS_HOME}/conf/gremlin-server/
COPY scripts/remote-connect.groovy ${JANUS_HOME}/scripts/

FROM openjdk:8-jre-slim-buster
FROM eclipse-temurin:8-jdk
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason for this change: openjdk docker image is deprecated.
See https://hub.docker.com/_/openjdk

@li-boxuan
Copy link
Member Author

I tested https://hub.docker.com/r/liboxuanhk/janusgraph/tags on my ARM machine locally, but it would be great if someone could test it on an AMD machine.

@FlorianHockmann
Copy link
Member

I just skimmed over the changes and have some early comments:

  1. I see that you only modified push-images.sh. Isn't it possible to also build the images locally via build-images.sh? It would be good if we could build all images first locally to inspect any changes we make.
  2. I guess also testing the images won't work, via test-image.sh? Or could we maybe at least get the tests to work somehow via GH Actions?
  3. You've changed the base image, so please also update the documented base image here: https://github.com/JanusGraph/janusgraph-docker#docker-tagging-policy
  4. We're only maintaining versions 0.5 and higher so I wouldn't modify older images any more.

But thanks a lot for tackling this issue, @li-boxuan!

@li-boxuan
Copy link
Member Author

Isn't it possible to also build the images locally via build-images.sh

We can, but then we cannot test it (due to docker/buildx#166), so it does not make too much sense to build without testing. With this PR, we build the AMD image and test it in build-images.sh. Then, we build a multi-arch image without testing and then push it in push-images.sh.

But thanks a lot for tackling this issue

I was debugging an integration test (JanusGraphSerializerBaseIT) but couldn't get it working. Then I realized it was because of the incompatibility between ARM (my machine) and AMD (officially built image). Now I can run the test with the ARM image.

Copy link
Member

@FlorianHockmann FlorianHockmann left a comment

Choose a reason for hiding this comment

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

I still don't really like that push-images.sh now builds images itself and then pushes those directly as these images can differ from those that we've built and tested before, e.g., if we use different build-args or versions.
But I guess we have to live with that for now if we want ARM support.

I added two minor comments. Apart from that, this looks good to me.

README.md Outdated Show resolved Hide resolved
push-images.sh Outdated Show resolved Hide resolved
@li-boxuan li-boxuan force-pushed the add-arm-support branch 2 times, most recently from 1dbd4e2 to e98c2e2 Compare January 4, 2023 13:50
Closes JanusGraph#92

Signed-off-by: Boxuan Li <liboxuan@connect.hku.hk>
@li-boxuan
Copy link
Member Author

I still don't really like that push-images.sh now builds images itself and then pushes those directly as these images can differ from those that we've built and tested before, e.g., if we use different build-args or versions.

One way is to tag arm/amd differently. For example, we would have 0.6.2-arm and 0.6.2-amd instead of a single 0.6.2. I consider this even worse than what this PR does.

@FlorianHockmann
Copy link
Member

One way is to tag arm/amd differently. For example, we would have 0.6.2-arm and 0.6.2-amd instead of a single 0.6.2. I consider this even worse than what this PR does.

Agreed, I also just wanted to mention this as a possible problem that we should keep in mind for the future, but still accepted the PR as I also came to the conclusion that it's the best option for now.

@li-boxuan li-boxuan merged commit 29c443d into JanusGraph:master Jan 4, 2023
@li-boxuan li-boxuan deleted the add-arm-support branch January 13, 2023 13:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARM64 Image
3 participants