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

Added support of max_rss ala reload_on_rss of uwsgi #568

Closed
wants to merge 7 commits into from

Conversation

pomarec
Copy link
Contributor

@pomarec pomarec commented Oct 14, 2013

To be honest i'm not really sure of what i'm doing. Especially concerning logging and notify_event.
I also added a fix concerning some processes that were not recognized as zombie but were kind of stopped.

@pomarec
Copy link
Contributor Author

pomarec commented Oct 14, 2013

Actually the fix is for this kind of situation :

(circusctl) stats web
web:
10891: 10891  python deploy 0 54M 442M 0.0 5.5 0:04.25
10707: No such process (stopped?)
10845: 10845  python deploy 0 86M 473M 0.0 8.7 0:12.35

so the process 10707 is considered as not existing any more and another one is respawned

@Natim
Copy link
Contributor

Natim commented Oct 15, 2013

I though they were already a plugin for that?

https://github.com/mozilla-services/circus/blob/master/circus/plugins/resource_watcher.py

@pomarec
Copy link
Contributor Author

pomarec commented Oct 15, 2013

My bad, I didn't read enough doc. In my case, I need to give absolute value for max memory use anyway. Are you intrested in a pull request on ressource watcher ?
What about the fix for non-existing processes ?

@tarekziade
Copy link
Member

Are you intrested in a pull request on ressource watcher ?

Absolutely, thanks!

What about the fix for non-existing processes ?

That could be a specific PR yeah. We should not loop on all processes though to do this, as this can cost some CPU. I propose that we cleanup this kind of process in https://github.com/mozilla-services/circus/blob/master/circus/watcher.py#L633 when process.info() returns 'no such process'

do you think you can manage to write a test for this failure ? (I know this one can be tough)

@pomarec
Copy link
Contributor Author

pomarec commented Oct 19, 2013

I started writing some code fore the ressource watcher but strangely I can't make it work (even the non-modified version). Here is a gist of what i'm trying to do https://gist.github.com/pomarec/7056349 but the process is not restarted (my computer has 4GB of memory).
Do you have any lead ?

I will also make the modification for the non-existing process but i'm affraid I don't know how to simulate this situation.

@tarekziade
Copy link
Member

@pomarec You can run plugin manually using the circus-plugin script. that way you can add a pdb into the plugin to see what's going on

@thefab
Copy link
Contributor

thefab commented Oct 25, 2013

@pomarec some progress ? I'm interested in your new feature (reload on rss) :-)

@pomarec
Copy link
Contributor Author

pomarec commented Oct 25, 2013

The first part is not ready.
The plugin is ready and working so far https://github.com/pomarec/circus/compare/ressource-watcher-mem-abs. I would like to package it nicely with tests and probably implement per process watching.
Don't worry I don't forget you, i just have to much work those days.

@pomarec
Copy link
Contributor Author

pomarec commented Oct 26, 2013

New pull request for max and min mem_abs #609

@pomarec
Copy link
Contributor Author

pomarec commented Oct 26, 2013

@tarekziade concerning the zombie process issue, it seems strange to me that it would be managed by stat. Shouldn't it be the watcher that watches for this kind or error ?
Since I don't know how to reproduce the bug and my fix works for my usage I will stick with it for my personal usage. I don't know how to fix it otherwise. If you want me to package it in a clean PR just tell me.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling e74acf3 on pomarec:master into fb43aba on mozilla-services:master.

@tarekziade
Copy link
Member

@tarekziade concerning the zombie process issue, it seems strange to me that it would be managed by stat. Shouldn't it be the watcher that watches for this kind or error ?

Processes can die at any time, so it seems right to me to make sure stats knows how to deal with this situation

Since I don't know how to reproduce the bug and my fix works for my usage I will stick with it for my personal usage.
I don't know how to fix it otherwise. If you want me to package it in a clean PR just tell me.

I'd love that - I would not want to lose your fix. We can work from there to add a test

@tarekziade
Copy link
Member

Should I close this PR ?

@tarekziade
Copy link
Member

Closing for now - feel free to reopen in case we need to

@pomarec
Copy link
Contributor Author

pomarec commented Nov 3, 2013

Opened new PR for zombie processes #638

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

5 participants