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 #17175 - Added watcher that can terminate a world #211

Merged
merged 1 commit into from Mar 19, 2017

Conversation

ShimShtein
Copy link
Contributor

This watcher polls the memory size of the process it runs in and terminates the world if memory limit is exceeded.
This commit also adds terminated event to World class, so anyone can add a listener that will perform tasks after a world is terminated.

@iNecas
Copy link
Member

iNecas commented Nov 22, 2016

We are watching if we mark all futures/events as finished at the end, this was missing ShimShtein#1

@iNecas
Copy link
Member

iNecas commented Nov 22, 2016

Nitpick: there should be an empty line between subject and body in commit message :)

@@ -29,3 +30,7 @@ end
group :lint do
gem 'rubocop', '0.39.0'
end

group :memory_watcher do
gem 'get_process_mem'
Copy link
Member

Choose a reason for hiding this comment

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

I guess this should belong to the gemspec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't want to add it to gemspec, since it's optional.

Copy link
Member

Choose a reason for hiding this comment

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

I see

def initialize(world, memory_limit, options)
@memory_limit = memory_limit
@world = world
@polling_interval = options[:polling_interval] || 10
Copy link
Member

Choose a reason for hiding this comment

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

What about defauling a bit longer: 60s might be enough

@memory_limit = memory_limit
@world = world
@polling_interval = options[:polling_interval] || 10
@memory_watcher = options[:memory_watcher] || GetProcessMem.new
Copy link
Member

Choose a reason for hiding this comment

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

s/memory_watcher/memory_info_loader or somethng similar, the watcher is this class


attr_reader :memory_limit, :world

def initialize(world, memory_limit, options)
Copy link
Member

Choose a reason for hiding this comment

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

I would suggest to have an option to have a minimal limit for the world to live (we would need to set @started_at variable in the world, to be able to check this)

The reason would be to prevent immediate termination after the executor starts, without giving it a change to actually do something - this would happen if the limit would be set too low.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is why I have added options[:initial_wait] you can pass a longer value in order to give the executor the time it needs to start.

@ShimShtein
Copy link
Contributor Author

Fixed everything except moving to gemspec. Now the tests can run.

@iNecas
Copy link
Member

iNecas commented Nov 22, 2016

Technically I approve this, but let's wait for the foreman-tasks portion of the work to test it together, in case we hit some issue here

This watcher polls the memory size of the process it runs in
and terminates the world if memory limit is exceeded.
This commit also adds `terminated` event to `World` class, so
anyone can add a listener that will perform tasks after a world
is terminated.
@ShimShtein
Copy link
Contributor Author

Added an example and fixed a small bug that was not covered by the tests.

ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Nov 23, 2016
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Nov 23, 2016
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Jan 3, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Jan 8, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Jan 17, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Mar 19, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
@iNecas
Copy link
Member

iNecas commented Mar 19, 2017

Thanks @ShimShtein , I still need to do final testing in foreman-tasks, but this part should be good to go.

@iNecas iNecas merged commit d65d076 into Dynflow:master Mar 19, 2017
@iNecas
Copy link
Member

iNecas commented Mar 19, 2017

dynflow-0.8.22 with this code pushed to rubygems

ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Mar 26, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Mar 29, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Mar 29, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Mar 29, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request Apr 3, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
ShimShtein added a commit to ShimShtein/foreman-tasks that referenced this pull request May 22, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
iNecas pushed a commit to theforeman/foreman-tasks that referenced this pull request May 22, 2017
A world will be terminated if memory limit is exceeded.
This will cause the running process to be killed.
Since we are monitoring our processes, a new process will
be spawned.

Relies on Dynflow/dynflow#211
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

2 participants