On 09/22/2011 10:01 PM, Ian Main wrote:
From: Chris Lalancette<clalance(a)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(a)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:
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