You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@deltacloud.apache.org by Michal Fojtik <mf...@redhat.com> on 2013/03/01 14:40:01 UTC
CIMI::Models split to CIMI::Services (rev 2) WIP
Hi,
This is an extended version of David patch. The goal here
is to migrate all CIMI 'service' methods to CIMI::Service
models and to make CIMI::Model contain just schema.
This patch adds/remove/breaks:
- ResourceMetada are now broken and need to be fixed.
(an informative warning included ;-)
- 5/9 should be merged to 8/9 (just note to myself ;-)
- The 'parse' method added to CIMI::Service (we use it in Create)
- The filter_by and select_by methods moved to .find, so no longer
ugly chains in Rabbit collections.
NOTE: This patch is 'work in progress'. Many things could be possibly
be broken and at least ResourceMetadata must be fixed.
David: Can you have look on RMD? I think we reached the problem with
lambdas :-)
-- Michal
On 03/01, mfojtik@redhat.com wrote:
> From: David Lutterkort <lu...@redhat.com>
>
> The current CIMI::Model classes address two concerns:
>
> * serialization/deserialization of CIMI objects
> * interaction with the current driver and the DB
>
> This patch splits these two concerns into two separate class hierarchies:
> CIMI::Model for (de)serialization and CIMI::Service for interacting with
> drivers/db
>
> Besides cleaning up the code, this will make it possible to reuse
> CIMI::Model classes as the basis for client code.
>
> Signed-off-by: Michal fojtik <mf...@redhat.com>
> ---
> server/lib/cimi/collections/base.rb | 4 +-
> server/lib/cimi/models/collection.rb | 27 ++----
> server/lib/cimi/models/machine.rb | 125 --------------------------
> server/lib/cimi/service.rb | 21 +++++
> server/lib/cimi/service/base.rb | 169 +++++++++++++++++++++++++++++++++++
> server/lib/cimi/service/machine.rb | 147 ++++++++++++++++++++++++++++++
> 6 files changed, 347 insertions(+), 146 deletions(-)
> create mode 100644 server/lib/cimi/service.rb
> create mode 100644 server/lib/cimi/service/base.rb
> create mode 100644 server/lib/cimi/service/machine.rb
>
> diff --git a/server/lib/cimi/collections/base.rb b/server/lib/cimi/collections/base.rb
> index eac1c70..3c319d6 100644
> --- a/server/lib/cimi/collections/base.rb
> +++ b/server/lib/cimi/collections/base.rb
> @@ -13,7 +13,7 @@
> # License for the specific language governing permissions and limitations
> # under the License.
>
> -require_relative '../models'
> +require_relative '../service'
>
> module CIMI::Collections
> class Base < Sinatra::Base
> @@ -24,7 +24,7 @@ module CIMI::Collections
>
> include Sinatra::Rabbit
> include Sinatra::Rabbit::Features
> - include CIMI::Model
> + include CIMI::Service
>
> helpers Deltacloud::Helpers::Drivers
> helpers Deltacloud::Helpers::Database
> diff --git a/server/lib/cimi/models/collection.rb b/server/lib/cimi/models/collection.rb
> index f36c081..cc95ab7 100644
> --- a/server/lib/cimi/models/collection.rb
> +++ b/server/lib/cimi/models/collection.rb
> @@ -120,26 +120,15 @@ module CIMI::Model
> end
>
> # Return a collection of entities
> - def list(context)
> - entries = find(:all, context)
> - desc = "#{self.name.split("::").last} Collection for the #{context.driver.name.capitalize} driver"
> - acts_as_root_entity unless collection_class
> - id = context.send("#{collection_class.entry_name}_url")
> - ops = []
> - cimi_entity = collection_class.entry_name.to_s.singularize
> - cimi_create = "create_#{cimi_entity}_url"
> - dcloud_create = context.deltacloud_create_method_for(cimi_entity)
> - if(context.respond_to?(cimi_create) &&
> - context.driver.respond_to?(dcloud_create)) ||
> - provides?(cimi_entity)
> - url = context.send(cimi_create)
> - ops << { :rel => "add", :href => url }
> + def list(id, entries, params = {})
> + params[:id] = id
> + params[:entries] = entries
> + params[:count] = params[:entries].size
> + if params[:add_url]
> + params[:operations] ||= []
> + params[:operations] << { :rel => "add", :href => params.delete(:add_url) }
> end
> - collection_class.new(:id => id,
> - :count => entries.size,
> - :entries => entries,
> - :operations => ops,
> - :description => desc)
> + collection_class.new(params)
> end
>
> def all(context)
> diff --git a/server/lib/cimi/models/machine.rb b/server/lib/cimi/models/machine.rb
> index 3beb2f7..6990fb9 100644
> --- a/server/lib/cimi/models/machine.rb
> +++ b/server/lib/cimi/models/machine.rb
> @@ -38,129 +38,4 @@ class CIMI::Model::Machine < CIMI::Model::Base
> scalar :rel, :href
> end
>
> - def self.find(id, context)
> - instances = []
> - if id == :all
> - instances = context.driver.instances(context.credentials)
> - instances.map { |instance| from_instance(instance, context) }.compact
> - else
> - instance = context.driver.instance(context.credentials, :id => id)
> - raise CIMI::Model::NotFound unless instance
> - from_instance(instance, context)
> - end
> - end
> -
> - def perform(action, context, &block)
> - begin
> - if context.driver.send(:"#{action.name}_instance", context.credentials, self.id.split("/").last)
> - block.callback :success
> - else
> - raise "Operation failed to execute on given Machine"
> - end
> - rescue => e
> - block.callback :failure, e.message
> - end
> - end
> -
> - def self.delete!(id, context)
> - context.driver.destroy_instance(context.credentials, id)
> - new(:id => id).destroy
> - end
> -
> - #returns the newly attach machine_volume
> - def self.attach_volume(volume, location, context)
> - context.driver.attach_storage_volume(context.credentials,
> - {:id=>volume, :instance_id=>context.params[:id], :device=>location})
> - CIMI::Model::MachineVolume.find(context.params[:id], context, volume)
> - end
> -
> - #returns the machine_volume_collection for the given machine
> - def self.detach_volume(volume, location, context)
> - context.driver.detach_storage_volume(context.credentials,
> - {:id=>volume, :instance_id=>context.params[:id], :device=>location})
> - CIMI::Model::MachineVolume.collection_for_instance(context.params[:id], context)
> - end
> -
> - 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)
> - machine_spec = {
> - :name => instance.name,
> - :created => instance.launch_time.nil? ? Time.now.xmlschema : Time.parse(instance.launch_time.to_s).xmlschema,
> - :description => "No description set for Machine #{instance.name}",
> - :id => context.machine_url(instance.id),
> - :state => convert_instance_state(instance.state),
> - :cpu => cpu || convert_instance_cpu(instance.instance_profile, context),
> - :memory => memory || convert_instance_memory(instance.instance_profile, context),
> - :disks => { :href => context.machine_url(instance.id)+"/disks"},
> - :volumes => { :href=>context.machine_url(instance.id)+"/volumes"},
> - :operations => convert_instance_actions(instance, context)
> - }
> - if context.expand? :disks
> - machine_spec[:disks] = CIMI::Model::Disk.find(instance, machine_conf, context, :all)
> - end
> - if context.expand? :volumes
> - machine_spec[:volumes] = CIMI::Model::MachineVolume.find(instance.id, context, :all)
> - end
> - machine_spec[:realm] = instance.realm_id if instance.realm_id
> - machine_spec[:machine_image] = { :href => context.machine_image_url(instance.image_id) } if instance.image_id
> - self.new(machine_spec)
> - end
> -
> - # FIXME: This will convert 'RUNNING' state to 'STARTED'
> - # which is defined in CIMI (p65)
> - #
> - def self.convert_instance_state(state)
> - case state
> - when "RUNNING" then "STARTED"
> - when "PENDING" then "CREATING" #aruba only exception... could be "STARTING" here
> - else state
> - end
> - end
> -
> - def self.convert_instance_cpu(profile, context)
> - cpu_override = profile.overrides.find { |p, v| p == :cpu }
> - if cpu_override.nil?
> - CIMI::Model::MachineConfiguration.find(profile.id, context).cpu
> - else
> - cpu_override[1]
> - end
> - end
> -
> - def self.convert_instance_memory(profile, context)
> - machine_conf = CIMI::Model::MachineConfiguration.find(profile.name, context)
> - memory_override = profile.overrides.find { |p, v| p == :memory }
> - memory_override.nil? ? machine_conf.memory.to_i : context.to_kibibyte(memory_override[1].to_i,"MB")
> - end
> -
> - def self.convert_instance_addresses(instance)
> - (instance.public_addresses + instance.private_addresses).collect do |address|
> - {
> - :hostname => address.is_hostname? ? address : nil,
> - :mac_address => address.is_mac? ? address : nil,
> - :state => 'Active',
> - :protocol => 'IPv4',
> - :address => address.is_ipv4? ? address : nil,
> - :allocation => 'Static'
> - }
> - end
> - end
> -
> - def self.convert_instance_actions(instance, context)
> - actions = instance.actions.collect do |action|
> - action = :restart if action == :reboot
> - name = action
> - name = :delete if action == :destroy # In CIMI destroy operation become delete
> - { :href => context.send(:"#{action}_machine_url", instance.id), :rel => "http://schemas.dmtf.org/cimi/1/action/#{name}" }
> - end
> - actions << { :href => context.send(:"machine_images_url"), :rel => "http://schemas.dmtf.org/cimi/1/action/capture" } if instance.can_create_image?
> - actions
> - end
> -
> - def self.convert_storage_volumes(instance, context)
> - instance.storage_volumes ||= [] #deal with nilpointers
> - instance.storage_volumes.map{|vol| {:href=>context.volume_url(vol.keys.first),
> - :initial_location=>vol.values.first} }
> - end
> -
> end
> diff --git a/server/lib/cimi/service.rb b/server/lib/cimi/service.rb
> new file mode 100644
> index 0000000..c9b662d
> --- /dev/null
> +++ b/server/lib/cimi/service.rb
> @@ -0,0 +1,21 @@
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements. See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership. The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License. You may obtain a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless require_relatived by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +#
> +
> +module CIMI::Service; end
> +
> +require_relative './models'
> +require_relative './service/base'
> +require_relative './service/machine'
> diff --git a/server/lib/cimi/service/base.rb b/server/lib/cimi/service/base.rb
> new file mode 100644
> index 0000000..9c281b7
> --- /dev/null
> +++ b/server/lib/cimi/service/base.rb
> @@ -0,0 +1,169 @@
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements. See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership. The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License. You may obtain a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +require 'xmlsimple'
> +
> +require_relative '../helpers/database_helper'
> +
> +# Service objects implement the server functionality of CIMI resources; in
> +# particular, these objects are responsible for interacting with the
> +# current driver. They use the CIMI::Model objects for (de)serialization
> +module CIMI::Service
> +
> + class Base
> +
> + # Extend the base model with database methods
> + extend Deltacloud::Helpers::Database
> +
> + attr_reader :model, :context
> +
> + class << self
> + def model_class
> + CIMI::Model.const_get(name.split('::').last)
> + end
> +
> + def model_name
> + name.split('::').last.underscore.to_sym
> + end
> +
> + def collection_name
> + name.split('::').last.underscore.pluralize.to_sym
> + end
> +
> + def inherited(subclass)
> + # Decorate all the attributes of the model class
> + schema = subclass.model_class.schema
> + schema.attribute_names.each do |name|
> + define_method(name) { self[name] }
> + define_method(:"#{name}=") { |newval| self[name] = newval }
> + end
> + end
> + end
> +
> + def initialize(context, opts)
> + if opts[:values]
> + @model = model_class.new(opts[:values])
> + elsif opts[:model]
> + @model = opts[:model]
> + else
> + @model = model_class.new({})
> + end
> + @context = context
> + retrieve_entity
> + end
> +
> + def model_class
> + self.class.model_class
> + end
> +
> + # Decorate some model methods
> + def []=(a, v)
> + v = (@model[a] = v)
> + retrieve_entity if a == :id
> + v
> + end
> +
> + def [](a)
> + @model[a]
> + end
> +
> + def to_xml
> + @model.to_xml
> + end
> +
> + def to_json
> + @model.to_json
> + end
> +
> + def select_attributes(attr_list)
> + @model.select_attributes(attr_list)
> + end
> +
> + def self.list(ctx)
> + id = ctx.send("#{collection_name}_url")
> + entries = find(:all, ctx)
> + params = {}
> + params[:desc] = "#{self.name.split("::").last} Collection for the #{ctx.driver.name.capitalize} driver"
> + params[:add_url] = create_url(ctx)
> + model_class.list(id, entries, params)
> + end
> +
> + def self.create_url(ctx)
> + cimi_create = "create_#{model_name}_url"
> + dcloud_create = ctx.deltacloud_create_method_for(model_name)
> + if(ctx.respond_to?(cimi_create) &&
> + ctx.respond_to?(dcloud_create)) || provides?(model_name)
> + ctx.send(cimi_create)
> + end
> + end
> +
> + # Save the common attributes name, description, and properties to the
> + # database
> + def save
> + if @entity
> + @entity.name = @model.name
> + @entity.description = @model.description
> + @entity.properties = @model.property
> + @entity.save
> + end
> + self
> + end
> +
> + # Destroy the database attributes for this model
> + def destroy
> + @entity.destroy
> + self
> + end
> +
> + # FIXME: Kludge around the fact that we do not have proper *Create
> + # objects that deserialize properties by themselves
> + def extract_properties!(data)
> + h = {}
> + if data['property']
> + # Data came from XML
> + h = data['property'].inject({}) do |r,v|
> + r[v['key']] = v['content']
> + r
> + end
> + elsif data['properties']
> + h = data['properties']
> + end
> + property ||= {}
> + property.merge!(h)
> + end
> +
> + def ref_id(ref_url)
> + ref_url.split('/').last if ref_url
> + end
> +
> + private
> +
> + # Load an existing database entity for this object, or create a new one
> + def retrieve_entity
> + if self.id
> + @entity = Deltacloud::Database::Entity::retrieve(self)
> + if @entity.exists?
> + @model.name = @entity.name
> + @model.description = @entity.description
> + @model.property ||= {}
> + @model.property.merge!(@entity.properties)
> + end
> + else
> + @entity = nil
> + end
> + end
> +
> + end
> +end
> diff --git a/server/lib/cimi/service/machine.rb b/server/lib/cimi/service/machine.rb
> new file mode 100644
> index 0000000..dedb0ad
> --- /dev/null
> +++ b/server/lib/cimi/service/machine.rb
> @@ -0,0 +1,147 @@
> +# Licensed to the Apache Software Foundation (ASF) under one or more
> +# contributor license agreements. See the NOTICE file distributed with
> +# this work for additional information regarding copyright ownership. The
> +# ASF licenses this file to you under the Apache License, Version 2.0 (the
> +# "License"); you may not use this file except in compliance with the
> +# License. You may obtain a copy of the License at
> +#
> +# http://www.apache.org/licenses/LICENSE-2.0
> +#
> +# Unless required by applicable law or agreed to in writing, software
> +# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
> +# License for the specific language governing permissions and limitations
> +# under the License.
> +
> +class CIMI::Service::Machine < CIMI::Service::Base
> +
> + def initialize(ctx, opts)
> + super
> + end
> +
> + def self.find(id, context)
> + instances = []
> + if id == :all
> + instances = context.driver.instances(context.credentials)
> + instances.map { |instance| from_instance(instance, context) }.compact
> + else
> + instance = context.driver.instance(context.credentials, :id => id)
> + raise CIMI::Model::NotFound unless instance
> + from_instance(instance, context)
> + end
> + end
> +
> + def perform(action, context, &block)
> + begin
> + if context.driver.send(:"#{action.name}_instance", context.credentials, self.id.split("/").last)
> + block.callback :success
> + else
> + raise "Operation failed to execute on given Machine"
> + end
> + rescue => e
> + block.callback :failure, e.message
> + end
> + end
> +
> + def self.delete!(id, context)
> + context.driver.destroy_instance(context.credentials, id)
> + new(:id => id).destroy
> + end
> +
> + #returns the newly attach machine_volume
> + def self.attach_volume(volume, location, context)
> + context.driver.attach_storage_volume(context.credentials,
> + {:id=>volume, :instance_id=>context.params[:id], :device=>location})
> + CIMI::Model::MachineVolume.find(context.params[:id], context, volume)
> + end
> +
> + #returns the machine_volume_collection for the given machine
> + def self.detach_volume(volume, location, context)
> + context.driver.detach_storage_volume(context.credentials,
> + {:id=>volume, :instance_id=>context.params[:id], :device=>location})
> + CIMI::Model::MachineVolume.collection_for_instance(context.params[:id], context)
> + end
> +
> + 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)
> + machine_spec = {
> + :name => instance.name,
> + :created => instance.launch_time.nil? ? Time.now.xmlschema : Time.parse(instance.launch_time.to_s).xmlschema,
> + :description => "No description set for Machine #{instance.name}",
> + :id => context.machine_url(instance.id),
> + :state => convert_instance_state(instance.state),
> + :cpu => cpu || convert_instance_cpu(instance.instance_profile, context),
> + :memory => memory || convert_instance_memory(instance.instance_profile, context),
> + :disks => { :href => context.machine_url(instance.id)+"/disks"},
> + :volumes => { :href=>context.machine_url(instance.id)+"/volumes"},
> + :operations => convert_instance_actions(instance, context)
> + }
> + if context.expand? :disks
> + machine_spec[:disks] = CIMI::Model::Disk.find(instance, machine_conf, context, :all)
> + end
> + if context.expand? :volumes
> + machine_spec[:volumes] = CIMI::Model::MachineVolume.find(instance.id, context, :all)
> + end
> + machine_spec[:realm] = instance.realm_id if instance.realm_id
> + machine_spec[:machine_image] = { :href => context.machine_image_url(instance.image_id) } if instance.image_id
> + self.new(context, :values => machine_spec)
> + end
> +
> + # FIXME: This will convert 'RUNNING' state to 'STARTED'
> + # which is defined in CIMI (p65)
> + #
> + def self.convert_instance_state(state)
> + case state
> + when "RUNNING" then "STARTED"
> + when "PENDING" then "CREATING" #aruba only exception... could be "STARTING" here
> + else state
> + end
> + end
> +
> + def self.convert_instance_cpu(profile, context)
> + cpu_override = profile.overrides.find { |p, v| p == :cpu }
> + if cpu_override.nil?
> + CIMI::Model::MachineConfiguration.find(profile.id, context).cpu
> + else
> + cpu_override[1]
> + end
> + end
> +
> + def self.convert_instance_memory(profile, context)
> + machine_conf = CIMI::Model::MachineConfiguration.find(profile.name, context)
> + memory_override = profile.overrides.find { |p, v| p == :memory }
> + memory_override.nil? ? machine_conf.memory.to_i : context.to_kibibyte(memory_override[1].to_i,"MB")
> + end
> +
> + def self.convert_instance_addresses(instance)
> + (instance.public_addresses + instance.private_addresses).collect do |address|
> + {
> + :hostname => address.is_hostname? ? address : nil,
> + :mac_address => address.is_mac? ? address : nil,
> + :state => 'Active',
> + :protocol => 'IPv4',
> + :address => address.is_ipv4? ? address : nil,
> + :allocation => 'Static'
> + }
> + end
> + end
> +
> + def self.convert_instance_actions(instance, context)
> + actions = instance.actions.collect do |action|
> + action = :restart if action == :reboot
> + name = action
> + name = :delete if action == :destroy # In CIMI destroy operation become delete
> + { :href => context.send(:"#{action}_machine_url", instance.id), :rel => "http://schemas.dmtf.org/cimi/1/action/#{name}" }
> + end
> + actions << { :href => context.send(:"machine_images_url"), :rel => "http://schemas.dmtf.org/cimi/1/action/capture" } if instance.can_create_image?
> + actions
> + end
> +
> + def self.convert_storage_volumes(instance, context)
> + instance.storage_volumes ||= [] #deal with nilpointers
> + instance.storage_volumes.map{|vol| {:href=>context.volume_url(vol.keys.first),
> + :initial_location=>vol.values.first} }
> + end
> +
> +end
> --
> 1.8.1.2
>
--
Michal Fojtik <mf...@redhat.com>
Deltacloud API, CloudForms
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by Michal Fojtik <mf...@redhat.com>.
On 03/06, David Lutterkort wrote:
> On Wed, 2013-03-06 at 15:41 +0100, Michal Fojtik wrote:
> > On 03/05, David Lutterkort wrote:
> > > I've addressed that issue and uploaded a new patch series
> > > ( http://tracker.deltacloud.org/set/368 )
> >
> > For some reason, all patches have empty body :-) (tracker upload?)
>
> I used 'tracker record' .. ugh; maybe we should just get rid of that ?
The 'tracker record' only record informations about the patch (metadata).
Then the 'tracker upload' send the patch bodies to the server.
The 'tracker publish' is an alias for: 'tracker record && tracker upload'.
There is also possibility to replace the old set with a new one (bump
rev.):
$ tracker record -o OLD_SET_ID
$ tracker upload
Cheers,
Michal
> > > I get a good number of test failures from the blackobox tests though.
> > > Will have to investigate further tomorrow.
> > >
> > > Also, when I put the attached test into tests/cimi/collections, it
> > > passes if I run it by itself with 'ruby
> > > tests/cimi/collections/volumes_test.rb) but it fails when I run rake
> > > test:cimi because the collection of volume configs is empty in
> > > make_volume_create ...
> >
> > Maybe this has something to do with initializers?
>
> Yes, seems like it has to do with tests/cimi/db/db_helper.rb which sets
> up an empty in-memory database.
>
> David
>
>
--
Michal Fojtik <mf...@redhat.com>
Deltacloud API, CloudForms
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by David Lutterkort <lu...@redhat.com>.
On Wed, 2013-03-06 at 15:41 +0100, Michal Fojtik wrote:
> On 03/05, David Lutterkort wrote:
> > I've addressed that issue and uploaded a new patch series
> > ( http://tracker.deltacloud.org/set/368 )
>
> For some reason, all patches have empty body :-) (tracker upload?)
I used 'tracker record' .. ugh; maybe we should just get rid of that ?
Also, what's the difference between publish and upload ?
> > I get a good number of test failures from the blackobox tests though.
> > Will have to investigate further tomorrow.
> >
> > Also, when I put the attached test into tests/cimi/collections, it
> > passes if I run it by itself with 'ruby
> > tests/cimi/collections/volumes_test.rb) but it fails when I run rake
> > test:cimi because the collection of volume configs is empty in
> > make_volume_create ...
>
> Maybe this has something to do with initializers?
Yes, seems like it has to do with tests/cimi/db/db_helper.rb which sets
up an empty in-memory database.
David
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by Michal Fojtik <mf...@redhat.com>.
On 03/05, David Lutterkort wrote:
> I've addressed that issue and uploaded a new patch series
> ( http://tracker.deltacloud.org/set/368 )
For some reason, all patches have empty body :-) (tracker upload?)
> I get a good number of test failures from the blackobox tests though.
> Will have to investigate further tomorrow.
>
> Also, when I put the attached test into tests/cimi/collections, it
> passes if I run it by itself with 'ruby
> tests/cimi/collections/volumes_test.rb) but it fails when I run rake
> test:cimi because the collection of volume configs is empty in
> make_volume_create ...
Maybe this has something to do with initializers?
-- Michal
>
> David
>
> On Mon, 2013-03-04 at 17:35 -0800, David Lutterkort wrote:
> > On Fri, 2013-03-01 at 17:25 -0800, David Lutterkort wrote:
> > > On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> > > > - ResourceMetada are now broken and need to be fixed.
> > > > (an informative warning included ;-)
> > >
> > > I added one more patch to your series and rerecorded as
> > > http://tracker.deltacloud.org/set/364
> >
> > I noticed a bunch of minor issues, and one bigger one today: if you look
> > at VolumeCreate#create now:
> >
> > def create(context)
> > if volume_template.href?
> > template = volume_template.find(context)
> > else
> > template = CIMI::Service::VolumeTemplate.from_xml(volume_template.to_xml)
> > end
> >
> > volume_image = template.volume_image.href? ?
> > template.volume_image.find(context) : template.volume_image
> >
> > volume_config = template.volume_config.href? ?
> > template.volume_config.find(context) : template.volume_config
> >
> > ...
> > end
> >
> > things blow up horribly because volume_template.find(context) winds up
> > calling CIMI::Model::VolumeTemplate::find - which doesn't exist anymore.
> >
> > To address that, I want to add some magic to CIMI::Service::Base; I just
> > haven't made up my mind what that magic should be exactly. Option (1)
> > would be to wrap each model object in a service object as it is returned
> > from Service::Base#[] .. but that seems like overkill. Option (2) is to
> > only do something special for Ref's, but still return CIMI::Model
> > objects from accessors. Ideally, that something could also help us get
> > rid of the boilerplate above, so that you could just write
> >
> > template = volume_template.unref(context)
> >
> > and have it (a) look up the volume_template if href is set and (b) set
> > attributes that were directly defined. That would have the advantage
> > that we'd do the right thing where requests reference an existing entity
> > and then override some of its attributes.
> >
> > Clearly, allowing to write volume_template.unref would require
> > monkeypatching the CIMI::Model::VolumeTemplateRef class - and I am still
> > hoping to come up with something that would avoid that.
> >
> > I'll look more into this business tomorrow.
> >
> > David
> >
> >
>
--
Michal Fojtik <mf...@redhat.com>
Deltacloud API, CloudForms
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by David Lutterkort <lu...@redhat.com>.
I've addressed that issue and uploaded a new patch series
( http://tracker.deltacloud.org/set/368 )
I get a good number of test failures from the blackobox tests though.
Will have to investigate further tomorrow.
Also, when I put the attached test into tests/cimi/collections, it
passes if I run it by itself with 'ruby
tests/cimi/collections/volumes_test.rb) but it fails when I run rake
test:cimi because the collection of volume configs is empty in
make_volume_create ...
David
On Mon, 2013-03-04 at 17:35 -0800, David Lutterkort wrote:
> On Fri, 2013-03-01 at 17:25 -0800, David Lutterkort wrote:
> > On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> > > - ResourceMetada are now broken and need to be fixed.
> > > (an informative warning included ;-)
> >
> > I added one more patch to your series and rerecorded as
> > http://tracker.deltacloud.org/set/364
>
> I noticed a bunch of minor issues, and one bigger one today: if you look
> at VolumeCreate#create now:
>
> def create(context)
> if volume_template.href?
> template = volume_template.find(context)
> else
> template = CIMI::Service::VolumeTemplate.from_xml(volume_template.to_xml)
> end
>
> volume_image = template.volume_image.href? ?
> template.volume_image.find(context) : template.volume_image
>
> volume_config = template.volume_config.href? ?
> template.volume_config.find(context) : template.volume_config
>
> ...
> end
>
> things blow up horribly because volume_template.find(context) winds up
> calling CIMI::Model::VolumeTemplate::find - which doesn't exist anymore.
>
> To address that, I want to add some magic to CIMI::Service::Base; I just
> haven't made up my mind what that magic should be exactly. Option (1)
> would be to wrap each model object in a service object as it is returned
> from Service::Base#[] .. but that seems like overkill. Option (2) is to
> only do something special for Ref's, but still return CIMI::Model
> objects from accessors. Ideally, that something could also help us get
> rid of the boilerplate above, so that you could just write
>
> template = volume_template.unref(context)
>
> and have it (a) look up the volume_template if href is set and (b) set
> attributes that were directly defined. That would have the advantage
> that we'd do the right thing where requests reference an existing entity
> and then override some of its attributes.
>
> Clearly, allowing to write volume_template.unref would require
> monkeypatching the CIMI::Model::VolumeTemplateRef class - and I am still
> hoping to come up with something that would avoid that.
>
> I'll look more into this business tomorrow.
>
> David
>
>
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by David Lutterkort <lu...@redhat.com>.
On Fri, 2013-03-01 at 17:25 -0800, David Lutterkort wrote:
> On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> > - ResourceMetada are now broken and need to be fixed.
> > (an informative warning included ;-)
>
> I added one more patch to your series and rerecorded as
> http://tracker.deltacloud.org/set/364
I noticed a bunch of minor issues, and one bigger one today: if you look
at VolumeCreate#create now:
def create(context)
if volume_template.href?
template = volume_template.find(context)
else
template = CIMI::Service::VolumeTemplate.from_xml(volume_template.to_xml)
end
volume_image = template.volume_image.href? ?
template.volume_image.find(context) : template.volume_image
volume_config = template.volume_config.href? ?
template.volume_config.find(context) : template.volume_config
...
end
things blow up horribly because volume_template.find(context) winds up
calling CIMI::Model::VolumeTemplate::find - which doesn't exist anymore.
To address that, I want to add some magic to CIMI::Service::Base; I just
haven't made up my mind what that magic should be exactly. Option (1)
would be to wrap each model object in a service object as it is returned
from Service::Base#[] .. but that seems like overkill. Option (2) is to
only do something special for Ref's, but still return CIMI::Model
objects from accessors. Ideally, that something could also help us get
rid of the boilerplate above, so that you could just write
template = volume_template.unref(context)
and have it (a) look up the volume_template if href is set and (b) set
attributes that were directly defined. That would have the advantage
that we'd do the right thing where requests reference an existing entity
and then override some of its attributes.
Clearly, allowing to write volume_template.unref would require
monkeypatching the CIMI::Model::VolumeTemplateRef class - and I am still
hoping to come up with something that would avoid that.
I'll look more into this business tomorrow.
David
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by Michal Fojtik <mf...@redhat.com>.
On 03/06, David Lutterkort wrote:
> On Mon, 2013-03-04 at 10:57 +0100, Michal Fojtik wrote:
> > On 03/01, David Lutterkort wrote:
> > > On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> > > > - ResourceMetada are now broken and need to be fixed.
> > > > (an informative warning included ;-)
> > >
> > > I added one more patch to your series and rerecorded as
> > > http://tracker.deltacloud.org/set/364
> I am going to selectively ACK and commit two of those patches since they
> are general fixes that I need to make rake test:cimi for the unit tests
> pass.
>
> They are:
> CIMI: Fix 'views' folder location
> CIMI: Send list of invalid parameters in message (change the
> commit message a little since it wasn't very clear to me)
>
> I also added one more trivial patch to enclose the backtrace in
> views/errors/common.xml.haml in CDATA
ACK :-) It will also make the set smaller.
-- Michal
--
Michal Fojtik <mf...@redhat.com>
Deltacloud API, CloudForms
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by David Lutterkort <lu...@redhat.com>.
On Mon, 2013-03-04 at 10:57 +0100, Michal Fojtik wrote:
> On 03/01, David Lutterkort wrote:
> > On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> > > - ResourceMetada are now broken and need to be fixed.
> > > (an informative warning included ;-)
> >
> > I added one more patch to your series and rerecorded as
> > http://tracker.deltacloud.org/set/364
I am going to selectively ACK and commit two of those patches since they
are general fixes that I need to make rake test:cimi for the unit tests
pass.
They are:
CIMI: Fix 'views' folder location
CIMI: Send list of invalid parameters in message (change the
commit message a little since it wasn't very clear to me)
I also added one more trivial patch to enclose the backtrace in
views/errors/common.xml.haml in CDATA
David
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by Michal Fojtik <mf...@redhat.com>.
On 03/01, David Lutterkort wrote:
> On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> > - ResourceMetada are now broken and need to be fixed.
> > (an informative warning included ;-)
>
> I added one more patch to your series and rerecorded as
> http://tracker.deltacloud.org/set/364
Great! I tried it and it works as expected. I have just one question about
RMD attrs in models. What if someone will try to use the models to
de/serialize CIMI to models in his own app. In that case the 'realm' and
'driver', which are DC specific attrs will appear in his model as
'required' attrs.
Does it have sense to inject that attrs to models dynamically? Like the
'metadata' method we have in Service models, will open the schema of
the inherited Model class and add these attributes?
But don't take this as a review blocker, I am perfectly fine with merging
this patches to master as they are now (maybe delete the FIXME: one ;-).
Also we should discuss what name we will give to the new CIMI models gem.
If we name it 'deltacloud-cimi-models', we can scare off some potential
users/contributors, because they will think this is DC specific gem.
What about just 'cimi-models' or just 'cimi' ("gem install cimi" sounds
kewl ;-).
Next question is versioning. We definitely need to have some constant in
models code (perhaps base.rb) to say for what version of CIMI these models
are supposed to work (CIMI::VERSION = '1.0.1', etc...).
Maybe crazy idea would be to also include the CIMI version in the gem
version itself, like:
cimi-1.0.1.RELEASE.gem
where we will just bumb the RELEASE number as we will need.
Cheers,
-- Michal
> > - The filter_by and select_by methods moved to .find, so no longer
> > ugly chains in Rabbit collections.
>
> Nice .. this is good, because ultimately we might need to want to pass
> some of this to drivers to avoid querying useless data.
>
> > David: Can you have look on RMD? I think we reached the problem with
> > lambdas :-)
>
> Things look fairly good now; there's still one test that fails, and the
> patches need to massaged a little. With a little more testing we are
> probably good to commit this.
>
> David
>
>
--
Michal Fojtik <mf...@redhat.com>
Deltacloud API, CloudForms
Re: CIMI::Models split to CIMI::Services (rev 2) WIP
Posted by David Lutterkort <lu...@redhat.com>.
On Fri, 2013-03-01 at 14:40 +0100, Michal Fojtik wrote:
> - ResourceMetada are now broken and need to be fixed.
> (an informative warning included ;-)
I added one more patch to your series and rerecorded as
http://tracker.deltacloud.org/set/364
> - The filter_by and select_by methods moved to .find, so no longer
> ugly chains in Rabbit collections.
Nice .. this is good, because ultimately we might need to want to pass
some of this to drivers to avoid querying useless data.
> David: Can you have look on RMD? I think we reached the problem with
> lambdas :-)
Things look fairly good now; there's still one test that fails, and the
patches need to massaged a little. With a little more testing we are
probably good to commit this.
David