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

Use nodemon for reloading on all server processes #160

Merged
merged 5 commits into from
Feb 2, 2023

Conversation

agjohnson
Copy link
Contributor

This examples nodemon from the celery entrypoint script to the web and proxito service entrypoint scripts.

The Django runserver --reload option doesn't see to be configurable, and seems to watch a lot of files that are not needed for reloading. I can't tell what exactly is being watched by runserver, but I am getting frequent too many open files errors from processes in Docker. Running with --no-reload mostly solves this issue, but requires manually restarting services.

Someone else should spend some time with this, I'm not confident that this will:

  1. Fix the issue for others
  2. Not cause irreparable damage to your local installation

This examples nodemon from the celery entrypoint script to the web and
proxito service entrypoint scripts.

The Django runserver --reload option doesn't see to be configurable, and
seems to watch a lot of files that are not needed for reloading. I can't
tell what exactly is being watched by runserver, but I am getting
frequent `too many open files` errors from processes in Docker. Running with
--no-reload mostly solves this issue, but requires manually restarting
services.

Someone else should spend some time with this, I'm not confident that this will:

1. Fix the issue for others
2. Not cause irreparable damage to your local installation
@humitos
Copy link
Member

humitos commented Dec 14, 2022

I'm not facing any problem with the current Docker setup. I don't know why you are receiving this error 🤷🏼

Also, I'm not sure about the differences between using nodemon or the regular Django reload mechanism. I remember when we were using watchdog I had problems about reloading because I had a lot of "extra non-useful" files in the directory where I had cloned the repository. How many files do you have there?

Mines are:

rtfd/code/readthedocs.org
▶ find ./ | wc -l
263482

Not cause irreparable damage to your local installation

He he, irreparable damage, I'm scared about pulling down these changes now 😄

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I'm not sure to understand how this solves your problem since with these changes, you are using 2 watchers: nodemon and django default's one (since --noreload is not passed to the command).

I see

web_1       | [info     ] Watching for file changes with StatReloader [django.utils.autoreload]

and

web_1       | [nodemon] starting `python3 manage.py runserver 0.0.0.0:8000`

dockerfiles/entrypoints/proxito.sh Outdated Show resolved Hide resolved
@agjohnson
Copy link
Contributor Author

Now I'm not able to reproduce this either. I can't get the original error messages from the containers anymore, using the Django reload. These errors were crashing these services, so my development environment was not usable at all without some fix.

I'm guessing there were more open files at the kernel level, or something changed in Docker as I did some cleanup of old images/containers at some point too.

Mines are:

My count is similar, 200k files

My open file count is around 375k

% sudo lsof | wc -l
...
375255

@agjohnson
Copy link
Contributor Author

agjohnson commented Dec 14, 2022

Annnd I'm back to broken. This seems related to me working on the new templates:

% inv docker.up -w -e --no-search
...
proxito_1   |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/settings/base.py", line 22, in <module>
proxito_1   |     import readthedocsext.theme  # noqa
proxito_1   |   File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
proxito_1   |   File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
proxito_1   |   File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
proxito_1   |   File "<frozen importlib._bootstrap_external>", line 879, in exec_module
proxito_1   |   File "<frozen importlib._bootstrap_external>", line 1016, in get_code
proxito_1   |   File "<frozen importlib._bootstrap_external>", line 1073, in get_data
proxito_1   | OSError: [Errno 24] Too many open files

Though open files still seems low:

% sudo lsof | wc -l
...
401567

This branch still resolves this error for me 🤯

@agjohnson
Copy link
Contributor Author

Actually, it doesn't seem related just to the new templates either:

web_1       |   File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
web_1       |   File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
web_1       |   File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
web_1       |   File "<frozen importlib._bootstrap_external>", line 879, in exec_module
web_1       |   File "<frozen importlib._bootstrap_external>", line 1016, in get_code
web_1       |   File "<frozen importlib._bootstrap_external>", line 1073, in get_data
web_1       | OSError: [Errno 24] Too many open files: '/usr/src/app/checkouts/readthedocs.org/readthedocs/__init__.py'

agjohnson added a commit that referenced this pull request Dec 14, 2022
I'm hitting "too many open files" rather consistently (see #160 for a
first try at resolving this). I am getting some inconsistent results
still, so not sure if this solves the problem or not.

I was just able to start the development env without altering the reload
mechanism though, so perhaps this just needs to be bumped up a bit.

I'm also not opposed to changing the reload mechanism too though.
@humitos
Copy link
Member

humitos commented Dec 15, 2022

I commented on the other PR that increases the limits as well, but it seems you will need to experiment a little more here by working with these different setups for a few days/weeks and confirm that this solves your issue. Currently, I'm confused why the problem was "solved" originally when watching the files with both monitors. I'm in doubt about what is the right solution but also if either of them is a solution at all 😄

@agjohnson
Copy link
Contributor Author

agjohnson commented Dec 15, 2022

Yeah, I'm still experimenting here. It's not even clear what the problem is, but I don't yet feel this is just a host level issue. I think the solution can be in our container configuration. I can't describe why I'm hitting this, other than maybe because I'm working with extra containers too.

I would be curious if this PR solves other problems with reload, I know several others can't/couldn't use reload previously -- perhaps @ericholscher can't still? I'd because if using this branch in common/ and allows the environment to be used with reloading enabled.

I don't have any strong opinions about this change here otherwise though. But if it solves one or two problems with local dev, being on a single reload daemon (nodemon) seems nice.

@ericholscher
Copy link
Member

ericholscher commented Dec 16, 2022

Yea, I still don't use autoreloading, but that's a docker issue with open file descriptors. If we lower it, maybe that will help? 🤷

@agjohnson
Copy link
Contributor Author

Aye, that's what I'm wondering. This fix, for some reason, seemed to make Docker less grumpy with me over the number of open files. I'm not sure how to verify that the Docker containers actually see fewer open files, it didn't look different at the host system. But Docker did stop complaining about open files.

Anyways, might be worth testing this more.

Copy link
Member

@humitos humitos left a comment

Choose a reason for hiding this comment

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

I updated this PR to work properly with nodemon for web/proxito processes 👍🏼

@humitos
Copy link
Member

humitos commented Feb 2, 2023

BTW, nodemon tells you how many files it's watching:

proxito_1   | [nodemon] watching 4989 files
celery_1    | [nodemon] watching 4989 files
web_1       | [nodemon] watching 4989 files

So... right now, it's 5k per container. We have 4 containers, which gives us 20k files in total. We may be able to tweak a little more the ignore settings to reduce more if we care about this. I'm not sure where that number comes, tho:

▶ find readthedocs/ -type f -name '*.py' | wc -l
779

▶ find readthedocs -type f | wc -l
6490

▶ find ./ -type f -name '*.py' | wc -l
62408

@humitos
Copy link
Member

humitos commented Feb 2, 2023

I opened #162 to track the amount of files watched. I think we are close to figure it out.

@humitos humitos merged commit ac1215e into main Feb 2, 2023
@humitos humitos deleted the agj/container-reload branch February 2, 2023 18:16
agjohnson added a commit that referenced this pull request Feb 20, 2023
I'm hitting "too many open files" rather consistently (see #160 for a
first try at resolving this). I am getting some inconsistent results
still, so not sure if this solves the problem or not.

I was just able to start the development env without altering the reload
mechanism though, so perhaps this just needs to be bumped up a bit.

I'm also not opposed to changing the reload mechanism too though.
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.

3 participants