Mailing-List: contact dev-help@deltacloud.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@deltacloud.apache.org
Received-SPF: pass (nike.apache.org: domain of lutter@redhat.com designates
 209.132.183.28 as permitted sender)
Subject: Re: [PATCH core 2/5] CIMI: Initial import of the Mock driver
From: David Lutterkort <lutter@redhat.com>
To: dev@deltacloud.apache.org
Date: Wed, 23 Nov 2011 17:08:57 -0800
In-Reply-To: <1322065405-54983-3-git-send-email-mfojtik@redhat.com>
References: <1322065405-54983-1-git-send-email-mfojtik@redhat.com>
	 <1322065405-54983-3-git-send-email-mfojtik@redhat.com>
Organization: Red Hat Inc
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: 7bit
Message-ID: <1322096937.17242.117.camel@avon.watzmann.net>
Mime-Version: 1.0

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