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

Development: Unify docker compose settings #5809

Merged

Conversation

4ludwig4
Copy link
Contributor

@4ludwig4 4ludwig4 commented Nov 4, 2022

⚠️ This is a PR to the develop-deployment-wg branch which is WIP, there might be still open TODOs which will be fixed in different PRs, but reviews and feedbacks are welcome!

Checklist

General

Motivation and Context

We would like to keep the Artemis container to a minimum and not integrate things needed in Docker, but maybe not in Kubernetes.
Also, the Docker Compose setup should be unified and easier to use.

Description

This PR unifies the Docker Compose settings and setups by doing the following:

This PR also improves the current Docker setup with these changes:

  • first iteration of multi-stage Dockerfile for Artemis
  • refactored the bootstrap.sh for Artemis to make it rootless according to docker best practices
  • introduced wait-for.sh shell script for docker-compose setups to control service startup order(this is something docker compose doesn't support out of the box)
  • basic docker yaml config setup
  • added some in-code TODOs which are either still thoughts or real tasks for which I didn't have time yet (limited to /src/main/docker and /docs/dev)
  • added saml-test service for basic SAML manual testing/debugging
  • added mailhog service for basic Mail testing/debugging
  • added Java Remote Debugger settings/profiles for easy in Container Debugging
  • gradle cleanup

Steps for Testing

Play a bit with the different docker-compose setups:

Build Artemis locally:
docker-compose -f src/main/docker/artemis-dev-mysql.yml build artemis-app

Run Artemis (dev profile) and MySQL:
docker-compose -f src/main/docker/artemis-dev-mysql.yml up

Stop and remove the volumes for this instance:
docker-compose -f src/main/docker/artemis-dev-mysql.yml down -v


For gitlab and jenkins setups, you can keep for instance the jenkins and gitlab volumes by stopping the containers:
docker-compose -f src/main/docker/artemis-dev-mysql-gitlab-jenkins.yml down

Then just remove the artemis-app and artemis-mysql volume:
docker volume rm artemis-data artemis-mysql-data

Also, maybe test the following:

  • adding mailhog or saml (probalby not fully working yet) as an extra service with and additional -f mailhog.yaml
  • other addtional services or docker-compose setups
  • read the documentation about docker-compose and docker

Code Review

  • Review 1
  • Review 2

Manual Tests

  • Test 1
  • Test 2

@4ludwig4 4ludwig4 requested a review from a team as a code owner November 4, 2022 14:17
@4ludwig4 4ludwig4 self-assigned this Nov 4, 2022
@4ludwig4 4ludwig4 changed the base branch from develop to develop-deployment-wg November 4, 2022 14:18
@4ludwig4 4ludwig4 changed the title [develop-deployment-wg] Development: Unify docker compose settings [develop-deployment-wg] Development: Unify docker compose settings Nov 4, 2022
Copy link
Contributor

@dfuchss dfuchss left a comment

Choose a reason for hiding this comment

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

Some comments

docs/dev/setup.rst Outdated Show resolved Hide resolved
docs/dev/setup.rst Outdated Show resolved Hide resolved
docs/dev/setup.rst Outdated Show resolved Hide resolved
docs/dev/setup.rst Outdated Show resolved Hide resolved
docs/dev/setup.rst Outdated Show resolved Hide resolved
docs/dev/setup.rst Outdated Show resolved Hide resolved
src/main/docker/README.md Outdated Show resolved Hide resolved
src/main/docker/README.md Outdated Show resolved Hide resolved
service: artemis-app
ports:
- 5005:5005 # Java Remote Debugging port declared in the java cmd options
command: >
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of wait-for, you can use something like this:

  gitlab:
    build:
      context: ./docker/gitlab
      args:
          GITLAB_VERSION: ${GITLAB_VERSION}
    restart: unless-stopped
    volumes:
      - ./data/gitlab/config:/etc/gitlab
      - ./data/gitlab/logs:/var/log/gitlab
      - $BACKUP_DIR/gitlab:/var/opt/gitlab/backups
      - ./data/gitlab/data:/var/opt/gitlab
    ports:
      - "22:22"
    networks:
      - artemis-net
    healthcheck:
      test: curl --fail -s -o /dev/null http://gitlab:80 || exit 1
      interval: 60s
      retries: 10
      start_period: 120s
      timeout: 10s

Copy link
Contributor Author

@4ludwig4 4ludwig4 Nov 7, 2022

Choose a reason for hiding this comment

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

I just remember these two stackoverflows threads [1] [2] when I looked into health checks for the Dockerfile.

Both say that the healthchecks of docker are not triggering anything unless you hack something together with killing certain things in your healthcheck CMD etc. ...

It seems like Kubernetes and Podman have better integrations for this feature.

They also sadly don't provide other probes like Kubernetes does. That's why I thought the mounting of the wait-for script is the easiest way without a lot of own nc and curl scripting?! But I'm open for better solutions 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

mmh ok, the command is waiting until gitlab is ready ; I did't mentioned any problems on our systems 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After writing you a PM on Slack about your prod setup I now have more of an idea of an alternative to the wait-for script but just added it for now as a todo.

Co-authored-by: Dominik Fuchß <dominik.fuchss@kit.edu>
Co-authored-by: Dominik Fuchß <dominik.fuchss@kit.edu>
@4ludwig4
Copy link
Contributor Author

@b-fein @dfuchss added your suggestions either as To-dos or directly. If the PR suits you this way, I would be happy to get an approval to merge it to our develop-deployment-wg branch and hopefully soon deploy it on the dedicated test server!

Copy link
Contributor

@b-fein b-fein left a comment

Choose a reason for hiding this comment

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

Looks good for the initial iteration of the Docker cleanup. The remaining ToDos can be solved incrementally in the future.

Added just one more non-blocking question about the Artemis container build.

src/main/docker/artemis/Dockerfile Show resolved Hide resolved
docs/Dockerfile Show resolved Hide resolved
@krusche krusche changed the title [develop-deployment-wg] Development: Unify docker compose settings Development: Unify docker compose settings Nov 18, 2022
@krusche krusche merged commit e5b9b3e into develop-deployment-wg Nov 18, 2022
@krusche krusche deleted the feature/deployment/docker-compose-unification branch November 18, 2022 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants