Suggesting major changes

[vrslev]

I really don't like how this repository is structured. Big part of code is not tested or documented, a lot of stuff repeats time to time. It's hard to keep up (and maintain I guess). My understanding is, frappe/frappe_docker was growing into something with "this", and "that", and "that".

I created new repository as proof that we can do better and simpler: https://github.com/vrslev/new_frappe_docker.

My proposal is to deprecate this repository (but obviously keep maintaining it for some time) and create new one (frappe/docker). I would be happy to keep sending PRs to improve current project, but too many breaking changes are required and in the end nobody would recognize the project before changes.

@revant I suggest to carefully look in my code. Thoughts?

[revant]

this repository is used by helm charts.

why change the names for images? can we keep things backward compatible for images?

too many breaking changes are required and in the end nobody would recognize the project before changes.

this was done by me once. It doesn't matter as long as images are backward compatible.

[vrslev]

why change the names for images?

Because they don’t actually represent what they do. Also I changed the purpose of frappe-nginx.

this was done by me once. It doesn't matter as long as images are backward compatible.

In my repo I removed majority of extra functionality. Can we do such thing here?

[revant]

if the images are better optimised we need to propogate all of it to Kubernetes helm chart.

can you try the k3d locally with your images? I can help in making a helm chart for it.

check this script to test everything locally. https://github.com/frappe/helm/blob/main/tests/script.sh

[revant]

I'll not prefer traefik as a mandatory service. Community not adopting docker for learning overhead, adding traefik will distance it further. Right now anyone who tweaks frappe-bench/config/nginx.conf can do similar things in frappe-nginx it even has an option to completely override the config file. It makes migrating apps from native setups to containers easier. No need of additional translation from nginx to traefik.

[vrslev]

can you try the k3d locally with your images?

Let's test it after all.

Right now anyone who tweaks frappe-bench/config/nginx.conf can do similar things in frappe-nginx

Does this config has to be this complicated? Also I don't like runtime config generation. With Traefik everything is quite simple and explicit.

No need of additional translation from nginx to traefik.

We can remove Traefik service from main compose file, configure Nginx to expose port and add Traefik only in https override.

It makes migrating apps from native setups to containers easier.

When I was migrating, it wasn't easy exactly because of networking. I think we need to change something.

[revant]

Does this config has to be this complicated? Also I don't like runtime config generation. With Traefik everything is quite simple and explicit.

it is copy of https://github.com/frappe/bench/blob/develop/bench/config/templates/nginx.conf

keeping it as close to bench as possible

I really don't mind new improved technologies. Just that the more it differs from upstream more we'll have to support without any upstream help to support issues/repo

we get help from upstream if errors are translated to native setups very quickly.

nginx configs are part of syllabus. traefik is out of syllabus.

We can remove Traefik service from main compose file, configure Nginx to expose port and add Traefik only in https override.

this nginx can be closer to bench I guess.

I'm open to explore new thing as long as it is propagated to helm charts and seamlessly help people migrate their current k8s setups. I can bump the helm charts by Major version and add migration path in readme if needed

[vrslev]

@revant Can we get rid of the scripts? https://github.com/vrslev/new_frappe_docker#scripts

[revant]

• remove auto migrate
• check_connection/doctor we need some liveness/readiness probe for the workers and scheduler for kubernetes, gunicorn and socketio containers can check ping to port 9k,8k, other containers don't have ports to check health. also there is no such thing as depends_on in swarm or k8s, there the container keeps restarting
• new site should work for users @%, also with postgres.
• push/restore: need something for daily backups https://helm.erpnext.com/kubernetes-resources/create-push-backups-to-cloud-job, suggest alternative

[revant]

Instead of redis-cache, redis-queue and redis-socketio container it can use just 1 redis container and use the database feature of redis. e.g. redis:6379/0, redis:6379/1, redis:6379/2

[vrslev]

Can you give an example or reference of this feature?

[vrslev]

new site should work for users @%, also with postgres.

They work due to this patch.

push/restore: need something for daily backups https://helm.erpnext.com/kubernetes-resources/create-push-backups-to-cloud-job, suggest alternative

OK, I will dig into that 😃

[revant]

They work due to this patch.

does it belong to core code? we can fix upstream and then remove patch?

[vrslev]

does it belong to core code? we can fix upstream and then remove patch?

It patches the core code. But the idea to restrict access only to current host is good, don't you think?

[revant]

idea to restrict access only to current host is good

Something like new-site --mariadb-no-socket can be done for postgres? It was an attempt to override explictly.

For backup the backend can be used as environment and any python script can be mounted as file/config/configMap and executed.

Thoughts on naming. backend means webserver+worker. socketio is also backend? Why not backend, websocket, frontend?

Somewhere I found js assets and node_modules are needed in python/gunicorn/worker as well. Somewhere jinja template uses may be. I remember there was also a migration patch that the runs a nodejs script to build website theme. Just like the python2 for version-12/node-gyp superstition, you will find it after rigorous testing or on issues.

[revant]

Can you give an example or reference of this feature?

Here is my devcontainer/docker-compose.yml

version: "3.7"
services:
  mariadb:
    image: mariadb:10.6
    command:
      - --character-set-server=utf8mb4
      - --collation-server=utf8mb4_unicode_ci
      - --skip-character-set-client-handshake
      - --skip-innodb-read-only-compressed # Temporary fix for MariaDB 10.6
    environment:
      - MYSQL_ROOT_PASSWORD=123
      - MYSQL_USER=root
      # Sometimes db initialization takes longer than 10 seconds and site-creator goes away.
      # Frappe doesn't use CONVERT_TZ() function that requires time zone info, so we can just skip it.
      - MYSQL_INITDB_SKIP_TZINFO=1
    volumes:
      - mariadb-vol:/var/lib/mysql

  # Enable PostgreSQL only if you use it, see development/README.md for more information.
  # postgresql:
  #   image: postgres:11.8
  #   restart: on-failure
  #   environment:
  #     - POSTGRES_PASSWORD=123
  #   volumes:
  #     - postgresql-vol:/var/lib/postgresql/data

  redis:
    image: redis:alpine

  frappe:
    image: frappe/bench:latest
    command: sleep infinity
    environment:
      - SHELL=/bin/bash
    volumes:
      - ..:/workspace:cached
    working_dir: /workspace/development
    ports:
      - "8000-8005:8000-8005"
      - "9000-9005:9000-9005"

volumes:
  mariadb-vol:
  postgresql-vol:

common_site_config.json:

{
 "background_workers": 1,
 "file_watcher_port": 6788,
 "frappe_user": "frappe",
 "gunicorn_workers": 17,
 "live_reload": true,
 "rebase_on_pull": false,
 "redis_cache": "redis://redis:6379/0",
 "redis_queue": "redis://redis:6379/1",
 "redis_socketio": "redis://redis:6379/2",
 "db_host": "mariadb",
 "restart_supervisor_on_update": false,
 "restart_systemd_on_update": false,
 "serve_default_site": true,
 "shallow_clone": true,
 "socketio_port": 9000,
 "use_redis_auth": false,
 "webserver_port": 8000,
 "developer_mode": 1
}

[vrslev]

Something like new-site --mariadb-no-socket can be done for postgres? It was an attempt to override explictly.

Actually, patch overrides host. So no need for --mariadb-no-socket or Postgres alternative.

[vrslev]

I don't like the fact that stable images are being rebuilt on each push to main branch and on ERPNext AND Frappe releases. Do you think there's more elegant way to implement build triggers than repository dispatch on new Frappe and ERPNext releases?

[vrslev]

About pushing/restoring backups. It is quite easy to implement what push_backup.py did. But with restoring it would be so clunky! There's too many stuff (Postgres, MariaDB, Amazon RDS — all of them need custom restoration process which bench restore doesn't support) for little maintainable script that we are trying to support.

Also, you don't restore your database and files every day, but you do backup them.

[vrslev]

Thoughts on naming. backend means webserver+worker. socketio is also backend?

Yep, that's better. Will we change images names or just services in compose file? I mean, do we keep frappe/frappe-worker, frappe/frappe-nginx, frappe/frappe-socketio, frappe/erpnext-worker, frappe/frappe-nginx or rename them to frappe/backend, frappe/frontend, frappe/websocket, frappe/erpnext-backend, frappe/erpnext-frontend?

[vrslev]

I remember there was also a migration patch that the runs a nodejs script to build website theme.

Yep, the only thing that I found in *.py files when was searching for node was generating website theme. Do we have to support such nonsense? 😃

[revant]

Do we have to support such nonsense? 😃

Yes! if we don't then it'll come up on issues, check previous closed issues!

The asset mounting gymnastics was for all this! I think somewhere Jinja template also tries to render path to some js file from the py code.

[revant]

Thoughts on naming. backend means webserver+worker. socketio is also backend?

Yep, that's better. Will we change images names or just services in compose file? I mean, do we keep frappe/frappe-worker, frappe/frappe-nginx, frappe/frappe-socketio, frappe/erpnext-worker, frappe/frappe-nginx or rename them to frappe/backend, frappe/frontend, frappe/websocket, frappe/erpnext-backend, frappe/erpnext-frontend?

I'm thinking about the helm chart. As many changes we do here, those will need to change in Helm Chart and for current helm chart users there should be clear migration path from version 3 to version 4.

[revant]

About pushing/restoring backups. It is quite easy to implement what push_backup.py did. But with restoring it would be so clunky! There's too many stuff (Postgres, MariaDB, Amazon RDS — all of them need custom restoration process which bench restore doesn't support) for little maintainable script that we are trying to support.

Also, you don't restore your database and files every day, but you do backup them.

At least push backup can be there. It can also go into the frappe/bench_helper code so we don't keep it here.

Worst case I mount file and run script.

[vrslev]

It can also go into the frappe/bench_helper code so we don't keep it here.

I think to add it to /usr/local/bin.

[vrslev]

I'm thinking about the helm chart. As many changes we do here, those will need to change in Helm Chart and for current helm chart users there should be clear migration path from version 3 to version 4.

We can implement backwards compatibility (including Helm chart) if we don't change image names. My idea is to make this repository maintainable. To do this we don't have to change images itself that much.

I will send a PR after some time that will simplify CI, optimize images, update compose files and some minor changes.

[revant]

are you there on discuss.erpnext.com

periodically, we should post about our updates there.

[kevgeni]

Hello there I'll be following this issue, as I am currently trying to customize the docker build phase of this repo for my custom version and I am having a hard time.

1. The current docker-bake.hcl is very complex and very hard to use. I am a beginner with some experience with docker compose and docker in general. I am learning how this docker-bake.hcl works with "docker buildx bake" and how to configure it, and it's pretty complicated.
2. the erpnext image depends on the published image of frappe on the web. I don't understand why and it makes building images depending on other images of this repository much harder. I'm pondering weither I'll change the images dockerfile to use the previously build image on the local machine. Why would I need to have a working repository to push the frappe, and redownload it at the step next after when I need to build erpnext ? It's not like I will ever just distribute frappe only. If I build and publish, it's the whole package including frappe, erpnext and my app.

[vrslev]

@kevgeni Hi there! Check out beautify branch in my fork, it resolves both your issues. This is work in progress, I will work on this for a while.

[revant]

there's also a major change happening with Frappe/ERPNext modules being taken out into apps.

recently I had to do this to install twilio integration https://gitlab.com/360enrich/erpnext_containers

any ideas how to best handle Frappe apps in containers?

[vrslev]

Actually, I don’t know. I don’t think adding new services to compose file just to get assets and Python code is a good idea. 🤔 We could make slim image that doesn’t have integrations and default one that does.

[kevgeni]

yeah... adding an app on top of the existing erpnext is a challenge. When you want to have frappe+erpnext+app, there is no easy script available. Still trying to have a working solution, either in the beautify or develop branch.

[kevgeni]

recently I had to do this to install twilio integration https://gitlab.com/360enrich/erpnext_containers

will check thanks.

[revant]

I'm okay with the overhead of such additional container build git repos for setup environment.

• It helps to automate and organize things.
• Everything happens through git.
• I can even do a deploy on UAT/staging
• version tagging or upgrade is also a git commit

What I generally do is, write some dockerfiles and place them in repo to build images. Works in any CI including GHA.

This step can be automated through some pre built build container that can build and publish images (may be frappe/bench:latest images). I'll open separate issue for this

[zwessels]

what's the thoughts on running the erpnext processes via another mechanism, e.g. supervisord as opposed to having each one as their own docker container (e.g. worker-long, worker-short, etc.)?

[revant]

what's the thoughts on running the erpnext processes via another mechanism, e.g. supervisord as opposed to having each one as their own docker container (e.g. worker-long, worker-short, etc.)?

the frappe/bench:latest image has sudo in it, you can either install nginx and supervisor in it and try, or build your image with bench image as base.

[zwessels]

yeah, it's also how pipech's [1] images work.

i was more asking if there is a specific need for this repo to have separate docker containers (e.g. workers) and if it wouldn't be simpler if we only had 1 image that runs with supervisor instead.

[1] https://github.com/pipech/erpnext-docker-debian

[kevgeni]

https://gist.github.com/kevgeni/abaeccd34b2b65d45eb023ef71599786

Hello there, I ended up doing these changes. I do not need them merged in your official repository, but you can take a look if you wish. Some changes are pretty specific to me so I don't think it's really reusable. Changes are

• I've removing the tagging of images inside the docker buildx bake configuration file. Our images are tagged develop, exported locally and saved as artifact in our pipeline. Then the same build can be promoted to test or production. In those case, it gets reloaded, tagged in that pipeline and uploaded. The buildx bake file is not reused for tagging and promoting, but only used to build the latest version . A different pipeline process is used for promotion of builded images.

The tagging system with multiple targets was pretty complicated and long in the orginal file, and I really like the more simple version. It delete a lot of duplicated targets. @vrslev vrslev thanks for the inspiration.

• Local images does not rely on frappe dockerhub. This is not in the example file, but i've modified the dockerfile to not take from frappe/xxx. This is because i've customized frappe and erpnext and can't uses your images. It kind of also fixes a weird behaviour in your build. If you start a compilation for the whole stack, the build will start frappe and erpnext at the same time. As the erpnext dockerfile uses a tagged images as from base, and the tagged images does not exists localy yet, erpnext will end up downloading and using the frappe public images that are tagged and available on the public dockerhub. However, if you manually build frappe locally, it will be tagged as frappe/whatever and the local build will be used for erpnext instead of the version available online. It's not good to mix both local and online, because you end up with a frankenstein of some code from the latest git, and some code from the web in a version you don't know when it was build and what it conatins. By using local images in dockerfile as base / not tagging the builded images publicly, If I try to start a full compilation, erpnext will simply fails, because frappe does not exists yet locally (has not been builded). I don't know how to do dependencies on this file, so my solution here is simply to build target frappe-develop, then build target erpnext-develop. Works for my needs.

• I've modified this file and my local docker file to account for

  • our custom git repository
  • the custom branch name we uses

[revant]

i was more asking if there is a specific need for this repo to have separate docker containers (e.g. workers) and if it wouldn't be simpler if we only had 1 image that runs with supervisor instead.

Extra needed workers can be added on to new servers instead of adding them to the same machine. It helps in docker swarm, K8s or any other container orchestrator platform. We just need replicate the number of instances of the needed containers and the platform takes care of placing them on available nodes/servers/workers.

@vrslev you have anything to say about why use multiple containers?

@zwessels I'll recommend continue using https://github.com/pipech/erpnext-docker-debian for your use case. That image also has redis-server and mariadb-server installed. It will act like a single VM image as you expect.

[vrslev]

@zwessels It is good practice to run one service per container. It helps for debugging, logging, scaling, reusability, upgrades, security etc. I suggest you to look at these sources:

https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#decouple-applications

https://devops.stackexchange.com/questions/447/why-it-is-recommended-to-run-only-one-process-in-a-container

frappe/bench image is meant to be used development. It would be near to impossible to develop and debug something on production Docker setup. Frappe framework has quite a lot external dependencies and this image helps to bootstrap local development.

If you want not to worry about Docker and stuff, just use official virtual machine image. pipech’s image is a compromise.

[github-actions[bot]]

This issue has been automatically marked as stale. You have a week to explain why you believe this is an error.

[kevgeni]

I still find the current implementation too hard to understand, but it's up to you if you simplify it or not.

[vrslev]

@kevgeni Current implementation — on main branch or vrslev/beautify?

[kevgeni]

Sorry for the confusion. I was seeing the issue in frappe_docker being dropped. I reiterated that the docker implementation in official frappe/frappe_docker is hard to understand. Happy that you continue to work on your version.