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

Fixes #19496 - passenger recycler #89

Merged
merged 3 commits into from Nov 2, 2017

Conversation

swapab
Copy link
Member

@swapab swapab commented Aug 23, 2017

@lzap, I am carrying over #78 here

Following things covered in this PR:

  1. Updated bin/passenger-recycler to comply with Rubocop standards.
  2. Introduced extras directory. Which can be used to reside files such as .cron, .conf etc.
  3. New procedure to execute passenger-recycler
    ./bin/foreman-maintain advanced procedure run passenger-recycler

Next steps:

  1. Update foreman-packaging to install bin/passenger-recycler to /usr/bin/
  2. Setup extras/passenger-recycler to /etc/cron.d/passenger-recycler.

PS: I have not renamed the passenger-recycler script and thinking to keep it that way.

@lzap
Copy link
Member

lzap commented Aug 31, 2017

LGTM, note you have passenger-recycler.cron twice, perhaps a leftover?

@swapab
Copy link
Member Author

swapab commented Aug 31, 2017

Deleted config/passenger-recycler.cron. Thanks!

@lzap
Copy link
Member

lzap commented Sep 4, 2017

LGTM one nitpick - I realized that 5 minutes by default is too tight, we max with 4 SIGTERMs with 90 seconds delay each which is in total 6 minutes. In the worst case, another process would be spawned. I suggest to run the cron job every 15 minutes by default, can you change the cronjob and also passenger-recycler.yaml comments to 15 mins?

@swapab swapab force-pushed the 19496-passenger-recycler branch 3 times, most recently from 655bc0e to 44d8c3e Compare September 8, 2017 06:49
@swapab
Copy link
Member Author

swapab commented Sep 8, 2017

Done. @lzap Can we merge this branch into master? As I will be doing the relevant packaging changes and bump the version.

@lzap
Copy link
Member

lzap commented Sep 25, 2017

Sorry for late reponse, absolutely, I havent tested this version myself tho.

@swapab
Copy link
Member Author

swapab commented Oct 25, 2017

@iNecas Can you please review this PR once and merge accordingly?

@@ -0,0 +1,3 @@
# Configuration file /etc/cron.d/passenger-recycler to run passenger-recycler
# every 5 minutes.
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick - it is every 15 minutes now.

Copy link
Member Author

@swapab swapab Nov 2, 2017

Choose a reason for hiding this comment

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

ACKed

lzap and others added 2 commits November 2, 2017 10:11
@iNecas
Copy link
Member

iNecas commented Nov 2, 2017

Ok, let's get this in. I'm bumping the version to 0.1.0 with this: there might be some packaging work needed, so want to keep 0.0.x reserved for fast-track patches needed to support the upgrades.

@iNecas iNecas merged commit f303004 into theforeman:master Nov 2, 2017
@swapab
Copy link
Member Author

swapab commented Nov 3, 2017

Thanks, @iNecas I'll get the packaging changes ready.

end

def debug(msg)
puts(msg) if CONFIG[:DEBUG]
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: probably better to output to STDOUT (using warn vs puts)

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.

None yet

4 participants