Re: [deltacloud-devel] [PATCH] Augemented Task State Model + Added Observers for Instance & Task for Auditing

[Scott Seago]

Just a few comments inline (I haven't applied/tested it though)

[e-mail] wrote:
> From: martyntaylor
>
> diff --git a/src/app/models/instance_observer.rb b/src/app/models/instance_observer.rb
> new file mode 100644
> index 0000000..81e75ea
> --- /dev/null
> +++ b/src/app/models/instance_observer.rb
> @@ -0,0 +1,22 @@
> +class InstanceObserver < ActiveRecord::Observer
> def after_save(an_instance)
> if an_instance.changed?
>  change = an_instance.changes['state']
>  if change
>    update_timestamps(change[0], change[1], an_instance)
>  end
> end
> end
> def update_timestamps(state_from, state_to, an_instance)
> if state_to == Instance::STATE_RUNNING
>  an_instance.time_last_start = Time.now
> elsif state_from == Instance::STATE_RUNNING && state_to == Instance::STATE_STOPPED
>  an_instance.acc_run_time = an_instance.acc_run_time + (Time.now - an_instance.time_last_start)
> end
> end

We should make sure that there aren't any hidden edge cases here -- 
instances going from STATE_RUNNING to _some other state_ and then later 
to STATE_STOPPED, for example, wouldn't be tracked properly here, if 
that's a valid state transition -- some providers might return some 
temporary "stopping" or "stop pending" state which might then later 
change to stopped. I'm not sure how the various provider FSM diagrams 
handle stopping, but we need to handle all transitions from running 
states to stopped states here.
> +end
>
> +InstanceObserver.instance
> \ No newline at end of file
> diff --git a/src/app/models/task.rb b/src/app/models/task.rb
> index 6cb907e..7cf7347 100644
> --- a/src/app/models/task.rb
> +++ b/src/app/models/task.rb
> @@ -21,6 +21,7 @@
>
> class Task < ActiveRecord::Base
>    belongs_to :task_target,       :polymorphic => true
> # InstanceTask association
>  belongs_to :instance,          :class_name => "Instance",
>                                 :foreign_key => "task_target_id"
>
> @@ -37,8 +38,28 @@ class Task < ActiveRecord::Base
>    COMPLETED_STATES = [STATE_FINISHED, STATE_FAILED, STATE_CANCELED]
>    WORKING_STATES   = [STATE_QUEUED, STATE_RUNNING, STATE_PAUSED]
> # - Failure Codes -
> # Failures that happen in the provider
> PROVIDER_OVER_QUOTA = "PROVIDER_OVER_QUOTA"
> PROVIDER_FAILED_TASK = "PROVIDER_FAILED_TASK"
> PROVIDER_CONNECTION_FAILURE = "PROVIDER_CONNECTION_FAILURE"
> # Failures that happen in the aggregator
> AGGREGATOR_OVER_POOL_QUOTA = "AGGREGATOR_OVER_POOL_QUOTA"
> AGGREGATOR_OVER_USER_QUOTA = "AGGREGATOR_OVER_USER_QUOTA"

We don't need the last one above. There are currently two quota types 
defined in the aggregator:
  1) quota on a pool (shown above) limits amount of resource use by a 
user/users for a given pool (so this would effectively be a 'user 
quota', but it's enforced at the pool level)
  2) quota on a back end account. If an account is over quota, this will 
prevent the scheduler from sending it any more 'start instance' 
requests. If all otherwise-matching accounts are over quota, this will 
result in a 'no match found' error state as you define below
> AGGREGATOR_NO_MATCHING_PROVIDER = "AGGREGATOR_NO_MATCHING_PROVIDER"

This should ptobably be AGGREGATOR_NO_MATCHING_PROVIDER_ACCOUNT -- since 
it's the accounts within the providers that are the basic unit of 
resource matching
> PROVIDER_FAILURE_CODES = [PROVIDER_OVER_QUOTA, PROVIDER_FAILED_TASK, PROVIDER_CONNECTION_FAILURE]
> AGGREGATOR_FAILURE_CODES = [AGGREGATOR_OVER_POOL_QUOTA, AGGREGATOR_OVER_USER_QUOTA, AGGREGATOR_NO_MATCHING_PROVIDER]
> FAILURE_CODES = PROVIDER_FAILURE_CODES + AGGREGATOR_FAILURE_CODES + [nil]
> validates_inclusion_of :failure_code,
> :in => FAILURE_CODES
> validates_inclusion_of :type,
> :in => %w( InstanceTask )
>
> :in => %w( InstanceTask )
>
> validates_inclusion_of :state,
> :in => COMPLETED_STATES + WORKING_STATES
>
> @@ -82,6 +103,7 @@ class Task < ActiveRecord::Base
>    def type_label
>      self.class.name[0..-5]
>    end
> def task_obj
>    ""
>  end
>
> diff --git a/src/app/models/task_observer.rb b/src/app/models/task_observer.rb
> new file mode 100644
> index 0000000..2614519
> --- /dev/null
> +++ b/src/app/models/task_observer.rb
> @@ -0,0 +1,24 @@
> +class TaskObserver < ActiveRecord::Observer
> END_STATES = [ Task::STATE_CANCELED, Task::STATE_FAILED, Task::STATE_FINISHED ]
> def after_save(a_task)
> if a_task.changed?
>  change = a_task.changes['state']
>  if change
>    update_timestamp(change[0], change[1], a_task)
>  end
> end
> end
> def update_timestamp(state_from, state_to, a_task)
> if END_STATES.include?(state_to)
>  a_task.time_ended = Time.now
> elsif state_to == Task::STATE_RUNNING
>  a_task.time_started = Time.now
> end
> end
>
> +end
>
> +TaskObserver.instance
> \ No newline at end of file