deltacloud-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From David Lutterkort <lut...@redhat.com>
Subject Re: [PATCH core 1/6] CIMI: Initial support for persisting attributes in database
Date Thu, 29 Nov 2012 23:10:23 GMT
Hi Michal,

ACK. Some questions:

On Thu, 2012-11-29 at 14:05 +0100, mfojtik@redhat.com wrote:
> diff --git a/server/config.ru b/server/config.ru
> index efed62f..406c769 100644
> --- a/server/config.ru
> +++ b/server/config.ru
> @@ -54,6 +54,7 @@ end
>  #       different root_url's
>  #
>  frontends.each do |frontend|
> +  frontend = frontend.strip

This should go into a separate patch - presumably it's to fix user
typos ?

> diff --git a/server/lib/cimi/models/base.rb b/server/lib/cimi/models/base.rb
> index 6a73cf6..a5d8f34 100644
> --- a/server/lib/cimi/models/base.rb
> +++ b/server/lib/cimi/models/base.rb
> @@ -285,4 +285,14 @@ class CIMI::Model::Base < CIMI::Model::Resource
>      end
>      self.class.new(attrs)
>    end
> +
> +  class << self
> +    def store_attributes_for(context, entity, attrs={})
> +      stored_attributes = {}
> +      stored_attributes[:description] = attrs['description'] if attrs['description']
> +      stored_attributes[:name] = attrs['name'] if attrs['name']
> +      stored_attributes[:ent_properties] = attrs['properties'].to_json if attrs['properties']
> +      context.store_attributes_for(entity, stored_attributes)

Why do we need the context here at all ? If we store in the database on
the level of CIMI model objects, rather than backend (driver) objects
there's no need for that. I don't like the idea that we need to pass the
context/driver this deep down, and that the responsibility for
storing/retrieving to/from the DB is distributed this widely (e.g., in
Machine.delete!).

Please don't address this comment just yet, we'll do that after the DMTF
meeting.

> @@ -120,10 +129,16 @@ class CIMI::Model::Machine < CIMI::Model::Base
>    def self.from_instance(instance, context)
>      cpu =  memory = (instance.instance_profile.id == "opaque")? "n/a" : nil
>      machine_conf = CIMI::Model::MachineConfiguration.find(instance.instance_profile.name,
context)
> +    stored_attributes = context.load_attributes_for(instance)
> +    if stored_attributes[:property]
> +      stored_attributes[:property].merge!(convert_instance_properties(instance, context))
> +    else
> +      stored_attributes[:property] = convert_instance_properties(instance, context)
> +    end

How about

        stored_attributes = context.load_attributes_for(instance) || {}
        stored_attributes[:property].merge!(convert_instance_properties(instance, context))

> @@ -131,8 +146,8 @@ class CIMI::Model::Machine < CIMI::Model::Base
>        :disks => { :href => context.machine_url(instance.id)+"/disks"},
>        :volumes => { :href=>context.machine_url(instance.id)+"/volumes"},
>        :operations => convert_instance_actions(instance, context),
> -      :property => convert_instance_properties(instance, context)
> -    }
> +      :property => stored_attributes
> +    }.merge(stored_attributes)

The ':property => ... ' line doesn't seem to belong there. The :merge
should already do the right thing.

> diff --git a/server/lib/db.rb b/server/lib/db.rb
> new file mode 100644
> index 0000000..e0f28d4
> --- /dev/null
> +++ b/server/lib/db.rb
> @@ -0,0 +1,55 @@
> +module Deltacloud
> +
> +  require 'data_mapper'
> +
> +  require_relative './db/provider'
> +  require_relative './db/entity'
> +
> +  DATABASE_LOCATION = ENV['DATABASE_LOCATION'] || "/var/tmp/deltacloud-mock-#{ENV['USER']}/db.sqlite"
> +
> +  def self.initialize_database
> +    DataMapper::Logger.new($stdout, :debug)
> +    DataMapper::setup(:default, "sqlite://#{DATABASE_LOCATION}")
> +    DataMapper::finalize
> +    DataMapper::auto_upgrade!
> +  end
> +
> +  module Helpers
> +    module Database

Why doesn't this live in lib/deltacloud/helpers ?

> +      include Deltacloud::Database

How do I find that when I first look at the code ? (It's not in
lib/deltacloud/database.rb)

> +      def current_provider
> +        Thread.current[:provider] || ENV['API_PROVIDER'] || 'default'
> +      end

Ugh .. this introduces nasty dependencies on how we are running things.
It might be time to write the driver/provider into every
Deltacloud::Model (or CIMI::Model) object.

Again, not something to fix right now, but ultimately, I'd like to get
to a point where one set of our model objects cleanly encapsulate
persistence.

David



Mime
View raw message