-
Notifications
You must be signed in to change notification settings - Fork 307
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
Revert many changes that break systemd #308
base: master
Are you sure you want to change the base?
Conversation
I also tested this through and this was working as expected. For me only I was also trying to setup multiprocess Sidekiq services today and and it was not working as expected. Started to read about how systemd works Thankfully I noticed your PR that resolved the multiple process problem. Thank you. Some resources that I was reading to understand how the systemd with |
@legendetm I will be honest I don't know much about the permissions thing but I know this option exists |
@jclusso it would be nice to update the documentation that before changing the And regarding permission issue, I also think that sudo should be only used when |
@legendetm are you referring to the usage of sudo for |
@jclusso I meant we should mention in documentation that you need to run
Other option would be Regarding the sudo part, this is unrelated to this PR. Just in my case the deploy user has no sudo privileges and therefore |
…o users know they need to run the uninstall and install commands.
@seuros where do we stand on getting this merged or are you integrating this into other changes? |
Hey @seuros, just checking in to see if you've had a chance to review this. |
Can you try master branch or the prerelease? |
@seuros I just reviewed the v3.0.0.alpha.1 and it does not appear to be correctly using Systemd's multiprocess support. Did you get a chance to review everything I wrote above? |
@seuros checking in on this again since it's been quite some time. |
@seuros could you please help us to solve this issue impacting more and more people please? 🙏 |
@seuros checking on this again... |
@seuros any chance you've gotten to review this? |
I will fix this gem in this weekend, do you mind if i ping you for testing ? |
No problem - my availability this weekend may be limited, but I'll try and respond! |
@seuros is there any update on this? |
@seuros is there going to be an update on this? |
Inject sidekiq:stop after deploy:reverted
I will look into it. |
@seuros any update on this? |
Did you test this with the latest release, i can release a new version then. |
@jclusso , It just because i see conflicts with the master branch. let rebase this PR and get it merged. I will release it today if this PR is ready. |
Current implementation creates separate service file for every Sidekiq configuration file and it does not contain The correct way to support Systemd's multiprocess, is to create only one service file with
Example: you have one service file
I think that the version 3 is improvement from initial implementation, as it's not using incorrect |
@legendetm you've confused me here. Are you suggesting the way things are now is better? |
@legendetm I think I understand what you're saying. The problem with eliminating the option like this is that at scale it becomes a mess. We have |
@jclusso, can you make a mock config in the https://github.com/seuros/capistrano-example-app ? just create a new env. https://github.com/seuros/capistrano-example-app/tree/main/config/deploy That will help document and discuss your cases. |
I meant, that version 3 is better than initial implementation that was before you PR. Form current implementation I like that it's enough just configuring I would prefer single service file with argument usage, but I think probably current version 3 would also be fine, if it supports same file multiple times in array (haven't tried it yet). But when looking code, then it seems current version 3 does not support same file multiple times in configuration file, as it adds the config filename without extension to service file, not using the index https://github.com/seuros/capistrano-sidekiq/blob/d73ad3c66ab60e549e0533fc3397f5e4bfb45eaf/lib/capistrano/tasks/systemd.rake#L130C7-L130C88 In our case, we have a lot of deploy stages with different Sidekiq config files combinations.
Also these files in our case are in |
@seuros this isn't just code I can rebase because you've accepted bad PRs that broke stuff and then moved so much around in 3.0. Essentially all of my work on #279 is either broken or lost in your refactorings it seems. There were key things I added that now are lost.
|
@jclusso, I apologize for merging those PRs without fully considering their impact. The implementation seemed to work at the time, but we lack testing in this gem, which led to these issues. This is precisely why I created a separate repo with a sample app , to manually test different scenarios and ensure stability. Regarding the number of services, whether it’s 1 or 10, I’ll spin up an EC2 instance to test the deployment thoroughly and address the problems. Let’s work together to fix this and restore your approach for gracefully terminating Sidekiq, including the quiet task and restart task with a 30-second wait. I’ll prioritize getting a release out as soon as possible. |
@seuros I'm going to start by trying to clone the latest version and see if I can try and find my way around where everything moved. Merging or rebasing the code at this point is too confusing. Appreciate you helping resolve this! |
@seuros I'm concerned I don't know enough about why things were changed at this point to really fix this. One example is, previously the Various configuration options were removed from systemd, but linger in the monit config. Is this an accident? Is monit even supported or used by anyone? It was removed from the documentation, but parts of it are lingering around. I'm pretty lost based on the current state of things. |
The current template includes all the settings from the official Sidekiq template(correct me if I'm wrong), minus the comments, and adds ERB to allow for dynamic configuration. If any configuration options were removed, it was because they were either deprecated in recent versions of Systemd or replaced with configurable alternatives. For example, Environment=MALLOC_ARENA_MAX=2 was removed because it’s now configurable. Regarding Monit, I agree it should likely be removed, as it seems to be unused and is no longer documented. If there are lingering parts, we should clean those up as well. To help resolve this, if you can commit a use case of your setup in the other repo, I’d be happy to assist in fixing or cleaning up the gem to better support your needs. |
Based on what you're saying, I think the first step is to clean the gem up before we add stuff back in. I can make a PR for some stuff to remove later or tomorrow. I'll also see if I can get to making a template app, but I rather try and put some time towards cleaning up. As for the Sidekiq config, I noticed it was largely different. To make it easy to keep up to date with Sidekiq's changes, I think it would be ideal to leave all the comments and stuff in. This also allows others to potentially modify the template for their own use case if they need to. Looking forward to getting this gem back on track! |
As I look at the template more, I see why some comments could be considered irrelevant. The configurability of Environment is a nice feature, but I wonder if we should default to including |
We could definitely add MALLOC_ARENA_MAX=2 as a default. I initially removed it because my setup involves dedicated machines where these environment variables are configured globally. However, I agree that for most users, having it as a default would be good to avoid potential performance issues out of the box. |
I'm the original author of the sidekiq multi process code. I think I have some useful insights into what's happened since my PR was merged.
In #296, an issue was opened about not supporting older versions of systemd that did not support multiple process service naming with the
@
symbol. The solution for this was to make it so that whensidekiq_processes
is set to 1, it uses thesidekiq_service_unit_name
without the@
.However, the changes implemented in #297 changed the
sidekiq_service_file_name
method to never have the@
sign, and they changed thesidekiq_service_unit_name
method to have some conditional logic that doesn't always work.As a result of the changes in #297, #298 was created, which led to #299. #299 modified the
sidekiq:install
task's functionality in an incorrect way, changing it to create multiple sidekiq@[number].service files, which is not how systemd multiple process support is supposed to work. The purpose of the@
sign in systemd service file names is that everything after the@
is passed in as an argument. Therefore, the correct service file name for a multiple process service is"sidekiq@.service". This allows for the full functionality of systemd service arguments. For example, ranges are supported, so you can run a command like
systemctl restart sidekiq@{1..3}
and that single command will execute for all 3 processes. Even though creating multiple files does technically work, I don't believe it is correct, and there is no benefit to having multiple identical service files when you only need one.In order to address the issues created by the changes mentioned above, #300 was created. However, it did not fix everything, which led to #302 and #304, which are further attempts to fix issues created by these changes. Ultimately, you end up with a whole series of changes attempting to patch issues going back to #296. In my opinion, things would be simpler and easier if the code went back to behaving closer to how things were around #296.
The code in this pull request does that. When
sidekiq_processes
is set to one, the systemd unit file will be named "sidekiq.service" and all commands will run on the service named "sidekiq". Whensidekiq_processes
is greater than one, the systemd unit file will be named "sidekiq@.service" and all commands will run either a systemd multi-process command, likesidekiq@{1..3}
, or, in the case of the restart command, it will run on each numbered process in turn (firstsidekiq@1
, thensidekiq@2
, etc.).Finally, in regards to issues around backward compatibility, you can simply run
sidekiq:uninstall
before upgrading andsidekiq:install
after upgrading. This makes more sense than over complicating the code.