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 2/5] CIMI: Initial import of the Mock driver
Date Thu, 24 Nov 2011 01:08:57 GMT
On Wed, 2011-11-23 at 17:23 +0100, mfojtik@redhat.com wrote:
> From: Michal Fojtik <mfojtik@redhat.com>
> 
> 
> Signed-off-by: Michal fojtik <mfojtik@redhat.com>
> ---
>  server/lib/cimi/drivers/drivers.rb          |   55 +++++++++++++++++++++++++++
>  server/lib/cimi/drivers/mock/mock_driver.rb |   53 ++++++++++++++++++++++++++
>  server/lib/cimi/server.rb                   |   18 ++------
>  3 files changed, 113 insertions(+), 13 deletions(-)
>  create mode 100644 server/lib/cimi/drivers/drivers.rb
>  create mode 100644 server/lib/cimi/drivers/mock/mock_driver.rb

ACK; some comments:

> diff --git a/server/lib/cimi/drivers/drivers.rb b/server/lib/cimi/drivers/drivers.rb
> new file mode 100644
> index 0000000..4d1dd2e
> --- /dev/null
> +++ b/server/lib/cimi/drivers/drivers.rb

Short of copying this file wholesale, is their something more clever we
can do ? How about we start a new lib/common directory, which also has a
drivers.rb containing everything from the current
lib/deltacloud/drivers.rb except for driver_class and
driver_source_name, and make lib/cimi/drivers.rb

        module CIMI::Drivers
          include Common::Drivers
        
          def driver_class ... same as in current drivers.rb ... end
        
          def driver_source_name ... same as in current drivers.rb ...
        end
        end

and similar for Deltacloud::Drivers. Looking at how these two methods
differ between CIMI and Deltacloud, this could be refactored further, so
that CIMI::Drives and Deltacloud::Drivers only add two methods, one
returning "cimi"/"deltacloud", the other returning
CIMI::Drivers/Deltacloud::Drivers (could that be replaced by self ?)

> diff --git a/server/lib/cimi/drivers/mock/mock_driver.rb b/server/lib/cimi/drivers/mock/mock_driver.rb
> new file mode 100644
> index 0000000..bcb1e2c
> --- /dev/null
> +++ b/server/lib/cimi/drivers/mock/mock_driver.rb
> @@ -0,0 +1,53 @@

> +  private
> +
> +  def convert_machine_description(hwp)
> +    MachineConfiguration.new(
> +      :name => hwp.name,
> +      :description => machine_conf_desc(hwp), 
> +      :cpu => hwp.cpu.value,
> +      :created => Time.now.to_s,
> +      :memory => { :quantity => hwp.memory.value, :units => hwp.memory.unit
},
> +      :disks => [ { :capacity => { :quantity => hwp.storage.value, :units =>
hwp.storage.unit } } ]
> +    )
> +  end

This would be nicer as a class method on MachineConfiguration, say
MachineConfiguration.from_hwp or MachineConfiguration.convert_hwp
(and I wonder if we should alias *Configuration classes to *Config
classes)

> diff --git a/server/lib/cimi/server.rb b/server/lib/cimi/server.rb
> index ba925fa..39f72f5 100644
> --- a/server/lib/cimi/server.rb
> +++ b/server/lib/cimi/server.rb
> @@ -112,19 +112,11 @@ global_collection :machine_configurations do
>      with_capability :hardware_profile
>      param :id,          :string,    :required
>      control do
> -      @profile =  driver.hardware_profile(credentials, params[:id])
> -      if @profile
> -        #setup the default values for a machine configuration
> -        resource_default = get_resource_default "machine_configuration"
> -        #get the actual values from profile
> -        resource_value = { "name" => @profile.name,"uri" => @profile.name,
> -              "href" => machine_configuration_url(@profile.name) }
> -        #mixin actual values get from profile
> -        @dmtfitem = resource_default["dmtfitem"].merge resource_value
> -        show_resource "machine_configurations/show", "MachineConfiguration",
> -          {"property" => "properties", "disk" => "disks", "operation" => "operations"}
> -      else
> -        report_error(404)
> +      machine_conf = driver.machine_configuration(credentials, :id => params[:id])
> +      machine_conf.uri = machine_configuration_url(params[:id])

Just to sound this out in my head: if we made this a method on MC, the
code would look like

                machine_conf = MachineConfiguration.find(params[:id], self)

with

                class MachineConfiguration
                    ...
                    def self.find(id, ctx)
                      hwps = nil
                      if id == :all
                        hwps = ctx.driver.hardware_profiles(ctx.credentials)
                      else
                        hwps = ctx.driver.hardware_profile(ctx.credentials, id)
                      end
                      confs = hwps.map { |hwp| self.convert_hwp(hwp, ctx) }
                      id == :all ? confs : confs.first
                    end
                
                    private
                    def self.convert_hwp(hwp, ctx)
                      new(:name => hwp.name , :uri => ctx.machine_configuration_url(hwp.id),
...)
                    end
                    ...
                end
                
With that, pretty much all the methods in the current
CIMI::Drivers::MockDriver would just become methods in
MachineConfiguration (it would need a hardware_profiles method that
delegates to Deltacloud::Drivers::MockDriver).

I am not against having a CIMI::MockDriver, but I would like to try to
keep logic that is not driver specific in the model classes.
Essentially, the drivers are solely our internal cloud abstraction
layer.

David



Mime
View raw message