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

feat(docker): Add option to run via Docker Compose #40

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

julian45
Copy link

I know this isn't necessarily a needed option at this point, but I thought I'd try my hand at Dockerizing this project in the interest of making it able to run on more platforms. Open to any questions, suggestions, or concerns you might have!

The Dockerfile and Compose spec included in this draft are relatively basic at this point; for one thing, the choice of version to run currently has to be manually specified in the Dockerfile. However, if further development is desired, it may be possible to work around this while developing a more formal build workflow through GitHub Actions or the like.

Adds a basic Dockerfile and docker-compose.yml. Still needs work since
we seemingly can't pass in the results of git describe --tags at start,
but it's something.
@atj
Copy link
Collaborator

atj commented Jun 14, 2024

You should add volumes for persistent data:

  • /.env
  • /snaps.db
  • /snaps/

Otherwise the image will just lose all state when it is restarted.

@kellnerd kellnerd added the feature New feature or request label Jun 14, 2024
Copy link
Owner

@kellnerd kellnerd left a comment

Choose a reason for hiding this comment

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

I don't really see the point of using Docker here, after all Deno is a single, self-contained binary which only depends on its cache directory and, in case of this project, the contents of this repository's checked out directory. But I also don't have a lot of experience with Docker, so I may be missing some other benefits.

That said, I think you can remove the version argument, it is optional and any usage scenario which wants to use all providers requires additional environment variables being specified.
To be honest, I don't know the ideal way to pass a dynamic env variable to the container, but hardcoding it into the compose file is not maintainable.
(All other env variables are rather static and can be passed in using the .env file which atj already mentioned.)

@phw
Copy link
Collaborator

phw commented Jun 14, 2024

Something like this would work cleanly for the environment variables:

services:
  web:
    build:
      context: .
    container_name: harmony-container
    image: harmony
    ports:
      - "8000:8000"
    volumes:
      - ./snaps/:/app/snaps/:rw
      - ./snaps.db:/app/snap.db:rw
    environment:
      HARMONY_CODE_URL: ${HARMONY_CODE_URL}
      HARMONY_SUPPORT_URL: ${HARMONY_SUPPORT_URL}
      MUSICBRAINZ_URL: ${MUSICBRAINZ_URL}
      DENO_DEPLOYMENT_ID: ${DENO_DEPLOYMENT_ID}
      FORWARD_PROTO: ${FORWARD_PROTO}
      HARMONY_SPOTIFY_CLIENT_ID: ${HARMONY_SPOTIFY_CLIENT_ID}
      HARMONY_SPOTIFY_CLIENT_SECRET: ${HARMONY_SPOTIFY_CLIENT_SECRET}
      HARMONY_TIDAL_CLIENT_ID: ${HARMONY_TIDAL_CLIENT_ID}
      HARMONY_TIDAL_CLIENT_SECRET: ${HARMONY_TIDAL_CLIENT_SECRET}

This would work as docker compose also reads the .env file and makes the variables available.

Side note: While the above is a little verbose I would very much advise this over e.g. using something like env_file: .env. Explicitly listing the variables to be passed in allows you to have fine control over which variables are used, instead of just everything. Probably not so much of a problem here, but definitely something I see done wrong in many compose files involving multiple containers.

@mwiencek
Copy link
Contributor

I don't really see the point of using Docker here, after all Deno is a single, self-contained binary which only depends on its cache directory and, in case of this project, the contents of this repository's checked out directory. But I also don't have a lot of experience with Docker, so I may be missing some other benefits.

One advantage is that it can allow you to run a different (stable) version of Deno inside the container than your system has installed. (Or you don't need to install Deno at all if you don't want to.) It may only be a single binary, but I still install it through my system package manager (which may provide an older version than I want) and would rather not manage multiple binaries.


WORKDIR /app

ADD . /app
Copy link
Contributor

Choose a reason for hiding this comment

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

A .dockerignore file should probably be added (with the same or similar entries as .gitignore) so that this doesn't copy unwanted stuff.

args:
version: v2024.6.12
container_name: harmony-container
image: harmony
Copy link
Contributor

Choose a reason for hiding this comment

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

Tagging the image with $DENO_DEPLOYMENT_ID might make sense? (I don't have much Deno experience, so not sure if this variable is ideal for versioning.)

Suggested change
image: harmony
image: harmony:$DENO_DEPLOYMENT_ID

@julian45
Copy link
Author

Hi all,

Thanks for the discussion and contributions so far! Unfortunately, I'm more than a bit busy today and this weekend, but will work on responding to all of this early next week.

@julian45
Copy link
Author

Apologies for not meeting my own timeline of "early next week"... here I am a full week later. 😅

Part of my initial motivation with putting this together was to enable deployment of Harmony in environments where one does not necessarily have access to an underlying operating system/dev environment. With a built container (e.g., published through GitHub Packages), it becomes substantially easier to run things like Harmony on platforms that simply offer Docker container hosting. In addition, as @mwiencek indicated, having a Docker container obviates the need to have Deno installed wherever Harmony is being run. (e.g., personally I didn't have Deno installed on my computer before beginning to work with Harmony)
Finally, there's a bit of a security benefit by running this application in an isolated environment from its host, especially in production environments. For example, I know someone at my day job who tends not to install software directly onto his work computer whenever possible, and instead prefers to run things as VMs or containers.

In any case, I'm going to go through the discussion in this PR and address things as appropriate. Thanks again for all of your input so far!

@julian45
Copy link
Author

    volumes:
      - ./snaps/:/app/snaps/:rw
      - ./snaps.db:/app/snap.db:rw

Unfortunately, I'm having a little trouble with this portion. Regardless of whether I put it into my docker-compose.yml as is or edit the second volume to mount as /app/snaps.db (with a final S in snaps), I get an error like this upon running docker compose up while trying to test the .env-based versioning process:

[+] Running 1/0
 ✔ Container harmony  Recreated                                            0.1s
Attaching to harmony
harmony  | error: Uncaught (in promise) Error: Is a directory (os error 21): open 'snaps.db'
harmony  |       const rid = Deno.openSync(path, { read: true, write, create }).rid;
harmony  |                        ^
harmony  |     at Object.openSync (ext:deno_fs/30_fs.js:620:15)
harmony  |     at js_open (https://deno.land/x/[email protected]/build/vfs.js:29:24)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:5566)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:145428)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:141310)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:147518)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:3869)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:611986)
harmony  |     at https://deno.land/x/[email protected]/src/db.ts:208:27
harmony  |     at setStr (https://deno.land/x/[email protected]/src/wasm.ts:19:20)
harmony exited with code 1

Here are the only differences in my working directory besides adding a .env with DENO_DEPLOYMENT_ID specified:

diff --git a/Dockerfile b/Dockerfile
index ee48b9e..00de358 100644
--- a/Dockerfile
+++ b/Dockerfile
@@ -8,7 +8,5 @@ ADD . /app

 RUN deno cache server/main.ts

-ARG version
-ENV DENO_DEPLOYMENT_ID $version
 USER deno:deno
 CMD deno run -A server/main.ts
diff --git a/docker-compose.yml b/docker-compose.yml
index b800f65..91ddd3a 100644
--- a/docker-compose.yml
+++ b/docker-compose.yml
@@ -2,9 +2,12 @@ services:
   web:
     build:
       context: .
-      args:
-        version: v2024.6.12
     container_name: harmony
     image: harmony
     ports:
       - "8000:8000"
+    volumes:
+      - ./snaps/:/app/snaps/:rw
+      - ./snaps.db:/app/snaps.db:rw
+    environment:
+      DENO_DEPLOYMENT_ID: ${DENO_DEPLOYMENT_ID}

@phw @atj any suggestions for how to properly handle persisting that file and making it available to the container? I've done some looking around, and I also tried manually touching snaps.db locally before trying to bring up the container, but that didn't do the trick.

@atj
Copy link
Collaborator

atj commented Jun 25, 2024

I think you need to specify a bind mount volume type, e.g.

volumes:
 - type: bind
   source: ./snaps.db
   target: /app/snaps.db

See this Stack Overflow post:

https://stackoverflow.com/questions/42248198/how-to-mount-a-single-file-in-a-volume

@julian45
Copy link
Author

julian45 commented Jun 29, 2024

I think you need to specify a bind mount volume type, e.g.

volumes:
 - type: bind
   source: ./snaps.db
   target: /app/snaps.db

See this Stack Overflow post:

https://stackoverflow.com/questions/42248198/how-to-mount-a-single-file-in-a-volume

Tried that too, and unfortunately no luck there:

λ › docker compose up
[+] Running 0/0
 ⠋ Container harmony  Recreate                                             0.0s
Error response from daemon: invalid mount config for type "bind": bind source path does not exist: /Users/julian/harmony/snaps.db
λ › touch snaps.db
λ › docker compose up
[+] Running 1/1
 ✔ Container harmony  Recreated                                            0.1s
Attaching to harmony
harmony  | error: Uncaught (in promise) PermissionDenied: Permission denied (os error 13): open 'snaps.db-journal'
harmony  |       const rid = Deno.openSync(path, { read: true, write, create }).rid;
harmony  |                        ^
harmony  |     at Object.openSync (ext:deno_fs/30_fs.js:620:15)
harmony  |     at js_open (https://deno.land/x/[email protected]/build/vfs.js:29:24)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:5566)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:83582)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:77354)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:76776)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:75649)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:62724)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:28928)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:115655)
harmony exited with code 1

And adding a new bind mount with similar syntax for snaps.db-journal, plus pre-emptively touching that file in the working directory, gives this. I don't see anything in the "long" mount definitions in the compose file that should be pre-empting the container's ability to write the file:

[+] Running 1/0
 ✔ Container harmony  Recreated                                            0.1s
Attaching to harmony
harmony  | error: Uncaught (in promise) PermissionDenied: Permission denied (os error 13): remove 'snaps.db-journal'
harmony  |       Deno.removeSync(path);
harmony  |            ^
harmony  |     at Object.removeSync (ext:deno_fs/30_fs.js:250:3)
harmony  |     at js_delete (https://deno.land/x/[email protected]/build/vfs.js:39:12)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:5597)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:79264)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:74720)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:148564)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:147802)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:108413)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:241396)
harmony  |     at <anonymous> (wasm://wasm/0028679a:1:122092)
harmony exited with code 1

@kellnerd
Copy link
Owner

If mounting an SQLite DB into Docker is such a PITA, we could also create the DB in a dedicated data directory by passing it as an argument here:

#snaps = new SnapStorage();

Ideally that would be done using another environment variable, with the default still being the current working directory for backward compatibility.

Another option (which I haven't tested to confirm it is working) would be to mount a data/ directory from outside the container into the container's /app working directory, then the source code doesn't even have to be adapted.

@phw
Copy link
Collaborator

phw commented Jun 30, 2024

Yes, having a separate directory would be preferable and easy management, because when bind mounting a single file it should exist beforehand.

Also this allows specifying this directory as a volume directly in the Dockerfile. Then one can easily run the docker image and have the persistent data in a volume even without explicitly defining a local directory as a bind mount.

@kellnerd
Copy link
Owner

I am sorry that it took me so long to get back to this, but I have finally made the data directory configurable in 6b02fe5. You can set it with a HARMONY_DATA_DIR environment variable.
This should unblock the PR if you are still interested in setting up Docker for Harmony.

@julian45
Copy link
Author

No problem! I'm still interested in setting up Docker for Harmony. I'm currently preoccupied working on MetaBrainz-internal stuff, but this is on my radar for sure.

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

Successfully merging this pull request may close these issues.

5 participants