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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[馃挕 FEATURE REQUEST]: New CLI command to reload/restart RR with the current config #1772

Open
FluffyDiscord opened this issue Nov 9, 2023 · 15 comments
Assignees
Labels
A-other Area: other C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. C-feature-request Category: feature requested, but need to be discussed help-heeded-medium Call for participation: Experience needed to fix: Medium / intermediate P-config Plugin: Config

Comments

@FluffyDiscord
Copy link

Plugin

None

I have an idea!

Sometimes I want to quick test config changes on production and having to rebuild whole docker container is painful.

Would it be possible to add option to reload, without stop and start, RR and use current .rr.conf with it's changed settings? It would make tweaking or quick fixing production issues easy.

@FluffyDiscord FluffyDiscord added the C-feature-request Category: feature requested, but need to be discussed label Nov 9, 2023
@rustatian
Copy link
Member

Hey @FluffyDiscord 馃憢
Do you mean smt like: ./rr reload ?

@FluffyDiscord
Copy link
Author

FluffyDiscord commented Nov 9, 2023

Hey @FluffyDiscord 馃憢 Do you mean smt like: ./rr reload ?

Yes, something like ./rr reload, but reload configuration from currently used .rr.yml, for example ./rr reload --with-config, if this makes any sense to you

@rustatian
Copy link
Member

Makes sense, I think.

@cv65kr
Copy link
Member

cv65kr commented Nov 9, 2023

Sometimes I want to quick test config changes on production and having to rebuild whole docker container is painful.

I am just thinking, but doing fixes in this way on production is not the securest way. Consider some things:

  1. When RR is entrypoint and you made any mistake with configuration file then RR will crash and your container in result will be restarted
  2. When your containers are scaled let say 5 times then you need to execute command in all containers.
  3. What about graceful shutdown during reload?

@FluffyDiscord
Copy link
Author

FluffyDiscord commented Nov 9, 2023

Sometimes I want to quick test config changes on production and having to rebuild whole docker container is painful.

I am just thinking, but doing fixes in this way on production is not the securest way. Consider some things:

1. When RR is entrypoint and you made any mistake with configuration file then RR will crash and your container in result will be restarted

2. When your containers are scaled let say 5 times then you need to execute command in all containers.

3. What about graceful shutdown during reload?

It's not about correcting an mistake. For mistakes, there are full rebuilds :) I am talking more about situations, where you for example want to add another service to test some behavior, or maybe change kv settings, because you need a quick hotfix without downtime.

Preferably the RR would not restart as a process, to keep docker container running.

@rustatian
Copy link
Member

Thank you, guys, for the feedback on this feature 馃憤

I have worryings about use-cases:

  • It would be fine (not sure atm) to have a configuration watcher and reloader. Because even in the docker, if you can enter into the container and execute ./rr reload it would be the same as just hitting CTRL+C and then ./rr server.
  • Having this option via RPC would require allocating an additional port. Which is not a problem, but users should map it outside the container. And also, would require additional configuration and potential security concerns.
  • Watch: having a watcher might help by auto-watching the configuration for the changes and reload RR. But it also can cause many issues if you pause for a second and RR reloads with a broken configuration.

@FluffyDiscord
Copy link
Author

FluffyDiscord commented Nov 9, 2023

Configuration watcher is not something that I want. That is indeed a bad, or simply, unnecessary thing to have. The config reload would be only manual. There does not even need an RPC port to control this (or I guess it's needed for communication from the CLI to the running RR?). Main point is to not restart whole RR process if possible, just reload config and reload workers/services (re-initialize).

This is indeed incredibly niche FR. You can always say no :D

@rustatian
Copy link
Member

rustatian commented Nov 9, 2023

@FluffyDiscord You simply can't reload the configuration w/o restarting every plugin internally. This would be the same as hitting CTRL+C and then starting RR again (conceptually).

This is indeed incredibly niche FR. You can always say no :D

You know I can't say no 馃槂 Not because of some rules, but because I respect all FRs created in the RR repository from our community 鉂わ笍

I need some time to think about that. It would be nice to hear more feedback. I would say I mostly agree to have this 馃槃

EDIT: Since we have a -e (--enable-experimental) flag we might at least experiment with that...

@rustatian rustatian added C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. help-heeded-medium Call for participation: Experience needed to fix: Medium / intermediate A-other Area: other P-config Plugin: Config labels Nov 9, 2023
@rustatian rustatian changed the title [馃挕 FEATURE REQUEST]: reload/restart with current config [馃挕 FEATURE REQUEST]: New CLI command to reload/restart RR with the current config Nov 9, 2023
@stevenbrookes
Copy link

Hi

I have a possibly similar scenario you might like to consider. We use deployer (https://deployer.org/) to deploy our application to production. Deployer works by creating a new directory for the application each time it's released, downloading the application files from git, installing vendors and then the final step is to change the symbolic link from the current running version to the new version. This allows for a quick switch to the new version and rollbacks etc.

I've been testing RR with this approach, by adding a rr reset step just after the link is switched, hoping that would mean RR would read the config file in the new release and restart the workers with the new code. However this doesn't seem to work reliably - I get an unresponsive RR and no workers. If I restart the RR - it's managed by systemd then it all works well.

What I'd like is a command like reset that does a restart in essence, rereading the config files and the rebooting the PHP application from scratch.

I think that's similar to what is being discussed here and would make for a nice easy solution without having to vie our web user permission to restart systemd services.

@rustatian
Copy link
Member

Hey @stevenbrookes 馃憢
Yeah, ./rr reset updates workers code (PHP), but not the configuration. If your workers depend on some configuration values - this might be a reason for unresponsiveness.
I'll add this feature to v2024.1 milestone, thanks for the comments.

@rustatian rustatian added this to the v2024.1.0 milestone Nov 24, 2023
@stevenbrookes
Copy link

@rustatian it's interesting you say that rr reset should update the latest PHP code. Out configuration rr.ryaml is stable now so when we do a deployment it's only the PHP code being updated. However I do get an unresponsive rr main process and no workers. I'm happy to investigate this further - is there any additional debugging info I can provide to help get to the bottom of that?

@rauanmayemir
Copy link

Couldn't you just make an alias like:

alias rrctl="envdir /etc/env.d /usr/local/bin/rr -c /etc/spiral/.rr.yaml -w /var/spiral"

Then after you run your daemon with rrctl serve, you call anything like rrctl reset, etc.
Anything 'stateful' would only bring complexity and confusion.

@rustatian
Copy link
Member

@stevenbrookes Could you please create a discussion (and describe your case with the workers, attach a configuration, rr version, etc) here for your case: link.
As I mentioned, I guess this is a good feature to implement.

@rustatian rustatian removed this from the v2024.1.0 milestone Apr 15, 2024
@Kaspiman
Copy link
Sponsor

Hi guys! How do you like this idea for the command: checking the relevance of the config, if the config has changed - restart the server, otherwise - reset?

@FluffyDiscord
Copy link
Author

I personally do, but its niche use case. I had to change config often at the beginnings or when something unexpected happens. Its then easier to just edit the config and reload, but if you use RR inside a Docker, restarting RR would restart docker and lose the config change, so here, if RR could restart without exiting itself...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-other Area: other C-enhancement Category: enhancement. Meaning improvements of current module, transport, etc.. C-feature-request Category: feature requested, but need to be discussed help-heeded-medium Call for participation: Experience needed to fix: Medium / intermediate P-config Plugin: Config
Projects
Status: 馃敄 Ready
Development

No branches or pull requests

6 participants