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

Allow reconfiguration without restart #742

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Allow reconfiguration without restart #742

merged 5 commits into from
Oct 14, 2024

Conversation

mbuechse
Copy link
Contributor

@mbuechse mbuechse commented Sep 6, 2024

Closes #666.

@mbuechse
Copy link
Contributor Author

mbuechse commented Sep 6, 2024

This is still in draft state. What it does so far: upon receiving SIGUSR2, it will increase the global counter, thus causing a reload of all certificate scopes next time they are needed.

I confirmed that it works by changing the file and invoking

docker compose kill -s SIGUSR2 web

However, if the file cannot be loaded (because of syntax errors, for instance), the error will only appear later on (as I said, the next time the certificate scopes are needed). I want to see the error at once. For this and other reasons, I still want to implement the following changes

  • load the scope files at once
  • either share the precomputed values across threads (because they are read only) or compute them from pre-loaded spec files
  • also reload bootstrap.yaml
  • (maybe?) no longer keep account data in the database, but use the same technique as for the scopes?

Oh, and

  • the templates must be reloaded as well

Copy link
Member

@martinmo martinmo left a comment

Choose a reason for hiding this comment

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

Is it intentional that functionality which seems unrelated to online reload is removed in this PR? (e.g., get_subjects and so on.)

The signal part seems to be alright as far as I can see, if really only a single process is ever used to serve requests. That might change when workers are scaled, because this starts separate Python processes. Also, in the future, using USR2 could be problematic if the server needs to be changed (e.g., using gunicorn to run the uvicorn workers), because libraries like gunicorn could be using signals for similar purposes of "online reconfiguration".

@mbuechse mbuechse marked this pull request as ready for review September 19, 2024 09:03
@mbuechse
Copy link
Contributor Author

@martinmo It's now based on #751 to remove the subject parts, so maybe merge that one first.

Your remarks regarding multiprocessing are correct, and I added a note. We would have to change a few things anyway, and frankly, this is a problem for the far future.

@mbuechse mbuechse merged commit b79a16b into main Oct 14, 2024
8 checks passed
@mbuechse mbuechse deleted the issue/666 branch October 14, 2024 14:31
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.

Compliance Monitor: allow reconfiguration without restart
2 participants