From: Chris Lalancette clalance@redhat.com
I modified Chris' patch to use a thread instead of fork. This pushes the communications with deltacloud into a thread which can take as long as it needs to complete the start.
In the future we may want to put a retry in here too.
You will notice in V2 we now set allow_concurrency = true in the postgres config. I'm a little worried that this means it may not work with other DB providers.
V4: Fix whitespace errors
Signed-off-by: Ian Main imain@redhat.com --- src/app/util/taskomatic.rb | 81 ++++++++++++++++++++++++++++++------------- src/config/database.pg | 3 ++ 2 files changed, 59 insertions(+), 25 deletions(-)
diff --git a/src/app/util/taskomatic.rb b/src/app/util/taskomatic.rb index 84dabd0..2f08daa 100644 --- a/src/app/util/taskomatic.rb +++ b/src/app/util/taskomatic.rb @@ -26,27 +26,15 @@ module Taskomatic task.state = Task::STATE_PENDING task.save!
- task.instance.provider_account = match.provider_account - task.instance.create_auth_key unless task.instance.instance_key - - dcloud_instance = create_dcloud_instance(task.instance, match) - - handle_dcloud_error(dcloud_instance) + create_dcloud_instance(task.instance, match)
task.state = Task::STATE_RUNNING - task.save! - - Rails.logger.info "Task instance create completed with key #{dcloud_instance.id} and state #{dcloud_instance.state}" - task.instance.external_key = dcloud_instance.id - task.instance.state = dcloud_to_instance_state(dcloud_instance.state) - task.instance.save! rescue HttpException => ex task.failure_code = Task::FAILURE_PROVIDER_CONTACT_FAILED handle_create_instance_error(task, ex) rescue Exception => ex handle_create_instance_error(task, ex) ensure - task.instance.save! task.save! end end @@ -146,18 +134,61 @@ module Taskomatic end
def self.create_dcloud_instance(instance, match) - client = match.provider_account.connect - - overrides = HardwareProfile.generate_override_property_values(instance.hardware_profile, match.hwp) - - client.create_instance(:image_id => match.provider_image.target_identifier, - :name => instance.name.tr("/", "-"), - :hwp_id => match.hwp.external_key, - :hwp_memory => overrides[:memory], - :hwp_cpu => overrides[:cpu], - :hwp_storage => overrides[:storage], - :realm_id => (match.realm.external_key rescue nil), - :keyname => (instance.instance_key.name)) + # because creating an instance can take a potentially long time (and + # creating multiple of them just prolongs this), we start a new thread + # that does this work. The new thread continues to run in the background + # and communicates the status back to the main UI via the database + + # This cleans up old DB connections. AR creates a new connection for each + # thread. This can leak FDs if you don't clean them up. Basically this + # ensures the previously used FD is cleaned up. + ActiveRecord::Base.connection_pool.clear_stale_cached_connections! + + Thread.new do + begin + # These all need to be reloaded because when you create a new thread + # AR creates a new connection and these objects become stale. + # + # Everything used here must be reloaded. + instance.reload + match.provider_account.reload + match.hwp.reload + match.realm.reload if match.realm + + instance.provider_account = match.provider_account + instance.create_auth_key unless instance.instance_key + + client = match.provider_account.connect + + overrides = HardwareProfile.generate_override_property_values(instance.hardware_profile, match.hwp) + + dcloud_instance = client.create_instance(:image_id => match.provider_image.target_identifier, + :name => instance.name.tr("/", "-"), + :hwp_id => match.hwp.external_key, + :hwp_memory => overrides[:memory], + :hwp_cpu => overrides[:cpu], + :hwp_storage => overrides[:storage], + :realm_id => (match.realm.external_key rescue nil), + :keyname => (instance.instance_key.name)) + + handle_dcloud_error(dcloud_instance) + + Rails.logger.info "Task instance create completed with key #{dcloud_instance.id} and state #{dcloud_instance.state}" + instance.external_key = dcloud_instance.id + instance.state = dcloud_to_instance_state(dcloud_instance.state) + rescue Exception => ex + # any sort of exception causes us to put the instance in CREATE_FAILED + # FIXME: if the exception is raised *after* the create_instance, this + # isn't true and will result in a rogue instance + Rails.logger.error ex.message + Rails.logger.error ex.backtrace.join("\n") + instance.state = Instance::STATE_CREATE_FAILED + raise ex + ensure + instance.save! + Thread.exit + end + end end
def self.matches(instance) diff --git a/src/config/database.pg b/src/config/database.pg index 19fe4e6..72a3895 100644 --- a/src/config/database.pg +++ b/src/config/database.pg @@ -45,6 +45,7 @@ development: username: aeolus password: v23zj59an host: localhost + allow_concurrency: true min_messages: warning
# Warning: The database defined as 'test' will be erased and @@ -56,6 +57,7 @@ test: &TEST username: aeolus password: v23zj59an host: localhost + allow_concurrency: true min_messages: warning
production: @@ -63,6 +65,7 @@ production: database: conductor username: aeolus password: v23zj59an + allow_concurrency: true host: localhost
cucumber:
On 09/22/11 - 01:01:06PM, Ian Main wrote:
From: Chris Lalancette clalance@redhat.com
I modified Chris' patch to use a thread instead of fork. This pushes the communications with deltacloud into a thread which can take as long as it needs to complete the start.
In the future we may want to put a retry in here too.
You will notice in V2 we now set allow_concurrency = true in the postgres config. I'm a little worried that this means it may not work with other DB providers.
V4: Fix whitespace errors
So I tested this out a bit, and it is *mostly* working. No more weird errors, no more deadlocks, no more hacking the activerecord source. I launched a 16 instance deployable with this in place, and it seemed to do the job.
I did hit one snag. When I launched an 8 instance deployable, all 8 jobs got launched in the backend. And 7 of the 8 jobs properly updated the database state to "pending". But one job did not, and got stuck in "new". I'm not sure what that means, but it probably bears investigating.
On Fri, Sep 23, 2011 at 11:53:22AM -0400, Chris Lalancette wrote:
On 09/22/11 - 01:01:06PM, Ian Main wrote:
From: Chris Lalancette clalance@redhat.com
I modified Chris' patch to use a thread instead of fork. This pushes the communications with deltacloud into a thread which can take as long as it needs to complete the start.
In the future we may want to put a retry in here too.
You will notice in V2 we now set allow_concurrency = true in the postgres config. I'm a little worried that this means it may not work with other DB providers.
V4: Fix whitespace errors
So I tested this out a bit, and it is *mostly* working. No more weird errors, no more deadlocks, no more hacking the activerecord source. I launched a 16 instance deployable with this in place, and it seemed to do the job.
I did hit one snag. When I launched an 8 instance deployable, all 8 jobs got launched in the backend. And 7 of the 8 jobs properly updated the database state to "pending". But one job did not, and got stuck in "new". I'm not sure what that means, but it probably bears investigating.
Nothing in the logs? Only time I saw this is when there was an error of some type. I was running into hitting my max ec2 instances and that causing errors..
Ian
On 09/22/2011 10:01 PM, Ian Main wrote:
From: Chris Lalancetteclalance@redhat.com
I modified Chris' patch to use a thread instead of fork. This pushes the communications with deltacloud into a thread which can take as long as it needs to complete the start.
In the future we may want to put a retry in here too.
You will notice in V2 we now set allow_concurrency = true in the postgres config. I'm a little worried that this means it may not work with other DB providers.
V4: Fix whitespace errors
Signed-off-by: Ian Mainimain@redhat.com
src/app/util/taskomatic.rb | 81 ++++++++++++++++++++++++++++++------------- src/config/database.pg | 3 ++ 2 files changed, 59 insertions(+), 25 deletions(-)
diff --git a/src/app/util/taskomatic.rb b/src/app/util/taskomatic.rb index 84dabd0..2f08daa 100644 --- a/src/app/util/taskomatic.rb +++ b/src/app/util/taskomatic.rb @@ -26,27 +26,15 @@ module Taskomatic task.state = Task::STATE_PENDING task.save!
task.instance.provider_account = match.provider_accounttask.instance.create_auth_key unless task.instance.instance_keydcloud_instance = create_dcloud_instance(task.instance, match)handle_dcloud_error(dcloud_instance)
create_dcloud_instance(task.instance, match) task.state = Task::STATE_RUNNING
task.save!Rails.logger.info "Task instance create completed with key #{dcloud_instance.id} and state #{dcloud_instance.state}"task.instance.external_key = dcloud_instance.idtask.instance.state = dcloud_to_instance_state(dcloud_instance.state)task.instance.save! rescue HttpException => ex task.failure_code = Task::FAILURE_PROVIDER_CONTACT_FAILED handle_create_instance_error(task, ex) rescue Exception => ex handle_create_instance_error(task, ex) ensure endtask.instance.save! task.save! end@@ -146,18 +134,61 @@ module Taskomatic end
def self.create_dcloud_instance(instance, match)
- client = match.provider_account.connect
- overrides = HardwareProfile.generate_override_property_values(instance.hardware_profile, match.hwp)
- client.create_instance(:image_id => match.provider_image.target_identifier,
:name => instance.name.tr("/", "-"),:hwp_id => match.hwp.external_key,:hwp_memory => overrides[:memory],:hwp_cpu => overrides[:cpu],:hwp_storage => overrides[:storage],:realm_id => (match.realm.external_key rescue nil),:keyname => (instance.instance_key.name))
# because creating an instance can take a potentially long time (and
# creating multiple of them just prolongs this), we start a new thread
# that does this work. The new thread continues to run in the background
# and communicates the status back to the main UI via the database
# This cleans up old DB connections. AR creates a new connection for each
# thread. This can leak FDs if you don't clean them up. Basically this
# ensures the previously used FD is cleaned up.
ActiveRecord::Base.connection_pool.clear_stale_cached_connections!
Thread.new do
begin# These all need to be reloaded because when you create a new thread# AR creates a new connection and these objects become stale.## Everything used here must be reloaded.instance.reloadmatch.provider_account.reloadmatch.hwp.reloadmatch.realm.reload if match.realminstance.provider_account = match.provider_accountinstance.create_auth_key unless instance.instance_keyclient = match.provider_account.connectoverrides = HardwareProfile.generate_override_property_values(instance.hardware_profile, match.hwp)dcloud_instance = client.create_instance(:image_id => match.provider_image.target_identifier,:name => instance.name.tr("/", "-"),:hwp_id => match.hwp.external_key,:hwp_memory => overrides[:memory],:hwp_cpu => overrides[:cpu],:hwp_storage => overrides[:storage],:realm_id => (match.realm.external_key rescue nil),:keyname => (instance.instance_key.name))handle_dcloud_error(dcloud_instance)Rails.logger.info "Task instance create completed with key #{dcloud_instance.id} and state #{dcloud_instance.state}"instance.external_key = dcloud_instance.idinstance.state = dcloud_to_instance_state(dcloud_instance.state)rescue Exception => ex# any sort of exception causes us to put the instance in CREATE_FAILED# FIXME: if the exception is raised *after* the create_instance, this# isn't true and will result in a rogue instanceRails.logger.error ex.messageRails.logger.error ex.backtrace.join("\n")instance.state = Instance::STATE_CREATE_FAILEDraise exensureinstance.save!Thread.exitendend end
def self.matches(instance)
diff --git a/src/config/database.pg b/src/config/database.pg index 19fe4e6..72a3895 100644 --- a/src/config/database.pg +++ b/src/config/database.pg @@ -45,6 +45,7 @@ development: username: aeolus password: v23zj59an host: localhost
allow_concurrency: true min_messages: warning
# Warning: The database defined as 'test' will be erased and
@@ -56,6 +57,7 @@ test:&TEST username: aeolus password: v23zj59an host: localhost
allow_concurrency: true min_messages: warning
production:
@@ -63,6 +65,7 @@ production: database: conductor username: aeolus password: v23zj59an
allow_concurrency: true host: localhost
cucumber:
Hi Ian, I was trying this patch today and here are some findings: - patch needs to be rebased, it's quite a long time since this patch was sent. I applied it manually for brief testing
- I'm still hitting the error I mentioned before with objects mismatch, this could be solved by passing instance_id instead of instance object when creating an instance key (though passing instance object is valid way too). Instance(#69892236976160) expected, got Instance(#69892238912300) /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/associations/association_proxy.rb:264:in `raise_on_type_mismatch' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/associations/belongs_to_association.rb:23:in `replace' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/associations.rb:1465:in `instance=' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/base.rb:1564:in `send' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/base.rb:1564:in `attributes=' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/base.rb:1560:in `each' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/base.rb:1560:in `attributes=' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/base.rb:1412:in `initialize' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/validations.rb:32:in `new' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/validations.rb:32:in `create!' /home/jprovazn/devel/conductor/src/app/models/instance.rb:261:in `create_auth_key' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/associations/association_proxy.rb:222:in `send' /usr/lib/ruby/gems/1.8/gems/activerecord-3.0.9/lib/active_record/associations/association_proxy.rb:222:in `method_missing' /home/jprovazn/devel/conductor/src/app/util/taskomatic.rb:158:in `create_dcloud_instance' /home/jprovazn/devel/conductor/src/app/util/taskomatic.rb:146:in `initialize' /home/jprovazn/devel/conductor/src/app/util/taskomatic.rb:146:in `new' /home/jprovazn/devel/conductor/src/app/util/taskomatic.rb:146:in `create_dcloud_instance' /home/jprovazn/devel/conductor/src/app/util/taskomatic.rb:29:in `create_instance' /home/jprovazn/devel/conductor/src/app/models/deployment.rb:252:in `launch' /home/jprovazn/devel/conductor/src/app/models/deployment.rb:239:in `each' /home/jprovazn/devel/conductor/src/app/models/deployment.rb:239:in `launch' /home/jprovazn/devel/conductor/src/app/controllers/deployments_controller.rb:95:in `create'
update: above error is probably caused by reloading rails classes in development mode. config.cache_classes = true should solve it but autoreloading is wanted feature and dev secusk w/o it.
- I hit twice this segfault: /usr/lib/ruby/gems/1.8/gems/activesupport-3.0.9/lib/active_support/dependencies.rb:484: [BUG] Segmentation fault ruby 1.8.7 (2011-06-30 patchlevel 352) [x86_64-linux] ^ this makes me very nervous as I don't remember when I hit segfault last time, then with this patch I hit it twice in ~30 minutes
I must admit that I like the simplicity of this solution (= solve whole background job thing just by running separate thread), but there are also some reasons why I'm afraid that: - I have bad feeling of leaving thread running after sending a response (when rails server is restarted, running threads are terminated too) - we will need to run other instance actions on background too (for example user selectes 20 instances deployed on different providers to stop, if not done on background he could get timeout), once we run other operations on background too, there could be problem with race conditions and accessing same objects by multiple threads - here is nice blogpost about threads and background jobs: http://bibwild.wordpress.com/2009/02/11/rails-threading-nightmare/
Better alternative could be forking processes instead of threads, though it doesn't solve all problems. Personally I would tend to pass background tasks to standalone daemon => IOW extend dbomatic or add another daemon, but it's thing to discuss (which should be done soon - I guess we need this for 1.0 release).
Jan
aeolus-devel@lists.fedorahosted.org