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

Small docker improvements and fixes #12335

Merged
merged 23 commits into from May 10, 2024

Conversation

oplik0
Copy link
Contributor

@oplik0 oplik0 commented Feb 10, 2024

This PR addresses some smaller issues with the current docker setup:

  • npm audit is now disabled
    • this unnecessarily wasted a few seconds on each startup. I don't know who'd read it there, and even if someone did it's not very useful
    • npm fund and update checks for npm were also disabled for similar reason, though their performance impact is much smaller
  • nodebb upgrade is now ran when the container is updated
    • works with a simple checksum of install/package.json
    • I think it's a better and more universal solution than just changing the build to upgrade and leaving users to force build manually.
  • for those who prefer to upgrade even when the container doesn't change there is also an option to change the build verb via environmental variables now, in a similar manner to the init verb that was already there
  • change setup.json to actually only set defaults and not override user choice
    • I genuinely thought this was already the case, but it turns out it had priority over values from the web UI
    • this required adding a new defaults key that's read into the default for questions in all database driver, so it's the only "nodebb code" change here
  • change from bind mounts to volumes... with bind
    • It seems sometimes docker can create bind mounts with root permissions for some reason - I was unable to replicate it when using volumes though
    • requires the folder structure to already exist on Linux (and probably MacOS) - hence the .gitkeeps in .docker
    • Docker documentation claims this shouldn't work on Windows but it totally does. It even auto-creates the folder structure there, so it works better than on Linux.
    • note: this will still not work on Linux if there isn't a user with uid of 1000 on host, but I'm not sure if there is any fix there
    • ideally database mounts would be removed in favor of just volumes (no reason to mount + they mess up permissions a bit), for compatibility reasons I'm leaving them for now though

@NavyStack
Copy link
Contributor

NavyStack commented Feb 13, 2024

May I join in @oplik0 ?

Initially, the Host UID and Container UID are slightly different.
It is not necessary for the user to exist on the host.

As I show you before, I would recommend to use User nodebb it is lovely isn't it?

And let's make respect User's favourite package manager :)

@barisusakli
Copy link
Member

@oplik0 is this good to merge?

@oplik0
Copy link
Contributor Author

oplik0 commented Feb 27, 2024

I think it should be good now.

@NavyStack for now I left the user as-is and the package manager support is runtime-only, as adding it to the build would add more complexity. I'll look into it in the future though :)

Also I changed the rebuild stage slightly and while it still needs to copy files it seems RUN --mount with cp is somehow faster? Don't ask me how, I don't know.

Also, it seems that the speedup from using the max cache mode is around 10 seconds at best, while the cost of just pushing this cache is around a minute. I elected to use the minimal strategy - but I'll check how it affects results in real releases with time (that is, if it increases cache misses in releases).

The action also no longer fetches all commits and all branches - as far as I can tell this isn't used anywhere? Maybe the metadata action once needed it, but I can't find any reference to (or reason for) this requirement - it only really needs what's on the current branch and its name.

@NavyStack
Copy link
Contributor

@oplik0 I was thinking of simply configuring a package manager-specific lock file and clearing it at the entry point.

#!/bin/bash

set -e

# Function to set default values for environment variables
set_defaults() {
  export CONFIG_DIR="${CONFIG_DIR:-/opt/config}"
  export CONFIG="$CONFIG_DIR/config.json"
  export NODEBB_INIT_VERB="${NODEBB_INIT_VERB:-install}"
  export START_BUILD="${START_BUILD:-false}"
  export SETUP="${SETUP:-}"
  export PACKAGE_MANAGER="${PACKAGE_MANAGER:-pnpm}"
}

# Function to check if a directory exists and is writable
check_directory() {
  local dir="$1"
  if [ ! -d "$dir" ]; then
    echo "Error: Directory $dir does not exist. Creating..."
    mkdir -p "$dir" || {
      echo "Error: Failed to create directory $dir"
      exit 1
    }
  fi
  if [ ! -w "$dir" ]; then
    echo "Error: No write permission for directory $dir"
    exit 1
  fi
}

# Function to copy or link package.json and lock files based on package manager
copy_or_link_files() {
  local src_dir="$1"
  local dest_dir="$2"
  local package_manager="$3"
  local lock_file

  case "$package_manager" in
    yarn) lock_file="yarn.lock" ;;
    npm) lock_file="package-lock.json" ;;
    pnpm) lock_file="pnpm-lock.yaml" ;;
    *)
      echo "Unknown package manager: $package_manager"
      exit 1
      ;;
  esac

  # Copy package.json and the appropriate lock file only to dest_dir if they don't exist there
  if [ ! -f "$dest_dir/package.json" ]; then
    cp "$src_dir/package.json" "$dest_dir/package.json"
  fi

  if [ ! -f "$dest_dir/$lock_file" ]; then
    cp "$src_dir/$lock_file" "$dest_dir/$lock_file"
  fi

  # Remove unnecessary lock files in src_dir
  rm -f "$src_dir/"{yarn.lock,package-lock.json,pnpm-lock.yaml}

  # Symbolically link the copied files in src_dir to dest_dir
  ln -fs "$dest_dir/package.json" "$src_dir/package.json"
  ln -fs "$dest_dir/$lock_file" "$src_dir/$lock_file"
}

# Function to install dependencies using pnpm
install_dependencies() {
  case "$PACKAGE_MANAGER" in
    yarn) yarn install || {
      echo "Failed to install dependencies with yarn"
      exit 1
    } ;;
    npm) npm install || {
      echo "Failed to install dependencies with npm"
      exit 1
    } ;;
    pnpm) pnpm install || {
      echo "Failed to install dependencies with pnpm"
      exit 1
    } ;;
    *)
      echo "Unknown package manager: $PACKAGE_MANAGER"
      exit 1
      ;;
  esac
}

# Function to start setup session
start_setup_session() {
  local config="$1"
  echo "Starting setup session"
  exec /usr/src/app/nodebb setup --config="$config"
}

# Function to start forum
start_forum() {
  local config="$1"
  local start_build="$2"

  echo "Starting forum"
  if [ "$start_build" = true ]; then
    echo "Build before start is enabled. Building..."
    /usr/src/app/nodebb build --config="$config" || {
      echo "Failed to build NodeBB. Exiting..."
      exit 1
    }
  fi
  exec "$PACKAGE_MANAGER" start --config="$config" --no-silent --no-daemon
}

# Function to start installation session
start_installation_session() {
  local nodebb_init_verb="$1"
  local config="$2"

  echo "Config file not found at $config"
  echo "Starting installation session"
  exec /usr/src/app/nodebb "$nodebb_init_verb" --config="$config"
}

# Function for debugging and logging
debug_log() {
  local message="$1"
  echo "DEBUG: $message"
}

# Main function
main() {
  set_defaults
  check_directory "$CONFIG_DIR"
  copy_or_link_files /usr/src/app "$CONFIG_DIR" "$PACKAGE_MANAGER"
  install_dependencies

  debug_log "PACKAGE_MANAGER: $PACKAGE_MANAGER"
  debug_log "CONFIG location: $CONFIG"
  debug_log "START_BUILD: $START_BUILD"

  if [ -n "$SETUP" ]; then
    start_setup_session "$CONFIG"
  fi

  if [ -f "$CONFIG" ]; then
    start_forum "$CONFIG" "$START_BUILD"
  else
    start_installation_session "$NODEBB_INIT_VERB" "$CONFIG"
  fi
}

# Execute main function
main "$@"

Because NodeBB already includes logic to check for lock files

@NavyStack
Copy link
Contributor

This link is a docker image that reflects my preference. https://github.com/NavyStack/nodebb-docker.git

@NavyStack
Copy link
Contributor

I don't have permission to co-work on your commit.
Also, if you use RUN --mount=type=cache, you need to inject cache from github actions.

@NavyStack
Copy link
Contributor

Fixed an update issue that could occur when updating from DockerImage v3.6.6 to v3.6.7.
https://github.com/NavyStack/nodebb-docker/blob/d17fc77ac226e08681179002320d929832dd0a61/docker-entrypoint.sh#L13

Overall, I'm concerned that this doesn't seem to be the right way to go for docker images.

I guess it's basically because I'm trying to keep it legacy, but it's a shame I can't get too deep into it.

oplik0 and others added 3 commits March 14, 2024 22:46
@NavyStack
Copy link
Contributor

NavyStack commented Mar 14, 2024

FROM node:lts AS git

ENV PNPM_HOME="/pnpm" \
  PATH="$PNPM_HOME:$PATH" \
  USER=nodebb \
  UID=1001 \
  GID=1001 \
  TZ="Asia/Seoul"

WORKDIR /usr/src/app/

RUN groupadd --gid ${GID} ${USER} \
  && useradd --uid ${UID} --gid ${GID} --home-dir /usr/src/app/ --shell /bin/bash ${USER} \
  && chown -R ${USER}:${USER} /usr/src/app/

RUN apt-get update \
  && apt-get -y --no-install-recommends install tini

USER ${USER}

RUN git clone --recurse-submodules -j8 --depth 1 https://github.com/NodeBB/NodeBB.git .

RUN find . -mindepth 1 -maxdepth 1 -name '.*' ! -name '.' ! -name '..' -exec bash -c 'echo "Deleting {}"; rm -rf {}' \; \
  && rm -rf install/docker/entrypoint.sh \
  && rm -rf docker-compose.yml \
  && rm -rf Dockerfile
  ## && sed -i 's|"\*/jquery":|"jquery":|g' install/package.json

FROM node:lts AS node_modules_touch

ENV NODE_ENV=production \
  PNPM_HOME="/pnpm" \
  PATH="$PNPM_HOME:$PATH" \
  USER=nodebb \
  UID=1001 \
  GID=1001 \
  TZ="Asia/Seoul"

WORKDIR /usr/src/app/

RUN corepack enable \
  && groupadd --gid ${GID} ${USER} \
  && useradd --uid ${UID} --gid ${GID} --home-dir /usr/src/app/ --shell /bin/bash ${USER} \
  && chown -R ${USER}:${USER} /usr/src/app/

COPY --from=git --chown=${USER}:${USER} /usr/src/app/install/package.json /usr/src/app/

USER ${USER}

RUN --mount=type=cache,id=pnpm,target=/pnpm/store \
  npm install \
    @nodebb/nodebb-plugin-reactions \
    nodebb-plugin-adsense \
    nodebb-plugin-extended-markdown \
    nodebb-plugin-meilisearch \
    nodebb-plugin-question-and-answer \
    nodebb-plugin-sso-github \
  && npm install --package-lock-only --omit=dev
  ## pnpm import \
  ## && pnpm install --prod --frozen-lockfile
  
FROM node:lts-slim AS final

ENV NODE_ENV=production \
  DAEMON=false \
  SILENT=false \
  PNPM_HOME="/pnpm" \
  PATH="$PNPM_HOME:$PATH" \
  USER=nodebb \
  UID=1001 \
  GID=1001 \
  TZ="Asia/Seoul"

WORKDIR /usr/src/app/

RUN corepack enable \
  && groupadd --gid ${GID} ${USER} \
  && useradd --uid ${UID} --gid ${GID} --home-dir /usr/src/app/ --shell /bin/bash ${USER} \
  && mkdir -p /usr/src/app/logs/ /opt/config/ \
  && chown -R ${USER}:${USER} /usr/src/app/ /opt/config/

COPY --from=node_modules_touch --chown=${USER}:${USER} /usr/src/app/ /usr/src/app/
COPY --from=git --chown=${USER}:${USER} /usr/src/app/ /usr/src/app/
COPY --from=git --chown=${USER}:${USER} /usr/src/app/install/docker/setup.json /usr/src/app/setup.json
COPY --from=git --chown=${USER}:${USER} /usr/bin/tini /usr/bin/tini
COPY --chown=${USER}:${USER} docker-entrypoint.sh /usr/local/bin/

USER ${USER}

EXPOSE 4567

VOLUME ["/usr/src/app/node_modules", "/usr/src/app/build", "/usr/src/app/public/uploads", "/opt/config/"]

ENTRYPOINT ["tini", "--", "docker-entrypoint.sh"]
## ENTRYPOINT ["tini", "--"]
## CMD ["start.sh"]
  1. This is my last Dockerfile.
  • I'm currently downloading NodeBB via git and prepping the plugins I need.
  • I'm running it in production, and so far it looks good to me.
  1. If you give me write access with your fork, I'll try to make change directly

  2. The build took 5 minutes. (Qemu) (amd64, arm64)
    https://github.com/NavyStack/nodebb-docker/actions/runs/8284128866

  3. Use tini

  • I also encountered a problem with PID=1 in the middle, so I use tini to handle it.
  1. Delete unnecessary folders and files starting with . (including .git)
  1. Since we are using a Debian-based image, the user will be able to set the timezone via the TZ variable in the docker-compose.yml file.

@oplik0
Copy link
Contributor Author

oplik0 commented Mar 14, 2024

@NavyStack Sent an invite :)

@NavyStack
Copy link
Contributor

@oplik0 It's an honour to work with you.

@CLAassistant
Copy link

CLAassistant commented Mar 15, 2024

CLA assistant check
All committers have signed the CLA.

@NavyStack
Copy link
Contributor

NavyStack commented Mar 15, 2024

Biggest change that matters.

changed USER node(1000:1000) to nodebb:nodebb (1001:1001)

  1. NodeBB hasn't specified the images tag in docker-compose.yml until now,
  2. To better handle permissions related to /usr/src/app/*
  3. Separate the three database backends into their own files (In each file, redis remains for the session)

  1. Make dev.Dockerfile
  2. Ready to Use image: ghcr.io/nodebb/nodebb:latest (Of course, I recommend that you specify a version.)
  • In my production? I use my own image with tag latest (and because of install plugins)

Considering the above, I made a drastic change.


TODO: It would be lovely docker init install plugins via docker-compose.yml ENV.


Running commands in a Docker image?

That's not recommended! If you need to make modifications to the container, you should rebuild the image instead.

@xSkeletor
Copy link

@NavyStack @oplik0 Any news on this PR? it's been a couple of months and I would really like to see it merged so that it can resolve a couple of issues with my version of NodeBB on Docker.

@oplik0
Copy link
Contributor Author

oplik0 commented May 9, 2024

@NavyStack made small changes and I think it looks fine now, anything you'd still want to add/change for this PR?

I agree that modifying a running container is far from perfect, but changing that would require more fundamental changes to the docker setup and maybe some improvements to NodeBB. So it'll probably have to wait for at least 4.0 (or even later major release since AP is the priority for 4.0)

@NavyStack
Copy link
Contributor

@oplik0
I thought about using something like Gosu to modify the permissions, but I think I'll wait a bit longer.
Maybe in v4 or so we'll have a chance to work together.

It looks good to me.

@xSkeletor
I also have a docker image that I'm already using for production, if you need it..
See https://github.com/NavyStack/nodebb-docker

Thanks.

@barisusakli barisusakli merged commit f4f0eb3 into NodeBB:master May 10, 2024
15 checks passed
@barisusakli barisusakli added this to the 3.7.6 milestone May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants