incubator-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] dmtf reference implementation initial checkin
Date Wed, 28 Sep 2011 23:34:49 GMT
Hi Tong,

On Wed, 2011-09-28 at 11:33 -0700, email4tong@gmail.com wrote:
> From: Tong Li <litong01@us.ibm.com>

first off, congrats, the patch applies now without any warnings. We are
making progress ;)

I have quite a few comments:

      * stylistically, there are still quite a few camelCased variable
        and method names. They should become under_scored.
      * I don't like the fact that server.rb hands all the actual work
        for put/post/delete off to cmwgapp_helper. I suspect the main
        reason why that works is because a lot of stuff is simply stored
        in files right now; once drivers are used, I fear we'll wind up
        with big switches like we have in get_collection_item_from_DC.
        The way this code is factored is exactly the reverse of what it
        should be: instead of using something generic in server.rb, and
        doing all the interesting work in a helper, we should do all the
        interesting work in server.rb, and put the boring, repetitive
        stuff into helpers. Yes, that means we won't have a generic
        route '/api/collection/:collType'. Instead, we'd have specific
        routes for each collType. The idea is that reading server.rb
        alone should give you a good idea of what the web code (i.e.,
        the code sitting above the drivers) does
      * In general, I would feel better if we removed all the stuff that
        stores state in local files; it's ultimately distracting from
        what we want to do with this code: we want the DMTF frontend to
        (a) give us a feeling for how hard it is to implement CIMI and
        (b) shed light on how hard it is to write CIMI adapters for
        existing cloud API's. IOW, I'd rather have less code that does a
        lot less right now, but works across all drivers, and build from
        there, than offer a wide variety of API which is essentially
        echoing data. At an absolute minimum, local file storage should
        go through the mock driver. At least that would tell us how much
        the internal driver API needs to change to support a CIMI
        frontend.
      * We can not store files under lib/ - in most installations, this
        will live somewhere in /usr, and the user that is running the DC
        server should never have permissions to write files there. The
        mock driver stores its files in /var/tmp (how well does that
        actually work under Windows ?)
      * What's the reason for not using rabbit at all ? There's a few
        things that rabbit does behind the scenes that are quite
        valuable, most notably parameter validation and generation of
        API docs. It also makes it clearer what parameters individual
        API's take as you declare them with each operation.

Some smaller comments inline:
> diff --git a/server/lib/cimi/helpers/cmwgapp_helper.rb b/server/lib/cimi/helpers/cmwgapp_helper.rb
> new file mode 100644
> index 0000000..a111b33
> --- /dev/null
> +++ b/server/lib/cimi/helpers/cmwgapp_helper.rb
> @@ -0,0 +1,358 @@

This method deserves a good comment; it's not straightforward what it
does (flatten out certain hashes-within-hashes)
> +def fixupContent(aHash, keyName="content", attrName="name")
> +  #this check is to make sure we are not handling nil values.
> +  if aHash
more idiomatic: 'return unless aHash'

> +    aHash.each_pair do |key, value|
> +      if value.kind_of? Hash
> +        #We can only handle the element without any other attribute,
> +        #if the element also has other attribute, then we can not do fixups since it
will lose information.
> +        if value[keyName] && value.size == 1
> +          aHash[key] = value[keyName]
> +        elsif value[keyName] && value[attrName] && value.size == 2
> +          aHash[key] = { "#{value[attrName]}" => value[keyName] }
easier to read: 'aHash[key] = { value[attrName].to_s = value[keyName] }'

> +        else
> +          fixupContent value, keyName, attrName
> +        end
> +      end
> +    end
> +  end
> +end


> +def is_valid? anObj
> +  return false unless anObj
> +  return false if anObj["uri"].nil? || anObj["name"].nil? || anObj["created"].nil?
> +  true
> +end
> +
> +def convert_xml_to_html xml_str
> +  if xml_str
> +    xml_str.gsub(/</, "&lt;").gsub(/>/, "&gt;").gsub(/\n/, "\r")

That last gsub seems Windows-specific to me. You also need to gsub(/&/,
"&amp;") if this is meant for an HTML <pre/> section.

> +  end
> +end
> +
> +module ApplicationHelper
> +
> +  include Deltacloud
> +
> +  def bread_crumb_ext
> +    s = "<ul class='breadcrumb'><li class='first'><a href='#{root_url}'>&#948
home</a></li>"
> +    s+="<li class='docs'>#{link_to_documentation}</li>"
> +    s+="</ul>"
> +  end
> +
> +  def machine_action(name)
> +    original_instance = driver.instance(credentials, :id => params[:id])
> +
> +    @operationParam = { "machineId" => params[:id], "machineOperation" => name}
> +
> +    # If original instance doesn't include called action
> +    # return with 405 error (Method is not Allowed)
> +    if original_instance
> +      unless driver.instance_actions_for(original_instance.state).include?(name.to_sym)
> +        return report_error(405, 'not_allowed')
> +      end
> +    end
> +
> +    @instance = driver.send(:"#{name}_instance", credentials, params["id"])
> +
> +    respond_to do |format|
> +      info "the format is " + format.to_s
> +      format.html { haml :"machine/operation" }
> +      format.xml  { haml :"machine/operation" }
> +      format.json do
> +        responseXML = haml :"machine/operation"
> +        hash_response = XmlSimple.xml_in responseXML, {'ForceArray' => true, 'KeepRoot'=>true,
'KeyAttr' => ['name']}
> +        hash_response.to_json
> +      end
> +    end
> +  end

This is the kind of thing that's really ugly anywhere but in server.rb:
this is clearly controller code (it uses params, it sets up instance
vars for views, and calls respond_to) It really needs to live in
server.rb, not in some helper.

> +  def get_collection_item_from_DC(collType, force_array = false)
> +    colItem = []
> +    case collType
> +    when "machineConfiguration"
> +      profiles = driver.hardware_profiles(credentials, nil)
> +      if profiles
> +        profiles.map do |profile|
> +          if force_array
> +            newItem = { "name" => [profile.name],"uri" => [profile.name],
> +              "href" => [HOST_API_PATH + "/" + collType + "/" + profile.name] }
> +          else
> +            newItem = { "name" => profile.name,"uri" => profile.name,
> +              "href" => HOST_API_PATH + "/" + collType + "/" + profile.name }
> +          end
> +          attr = cimi_get_profile_properties profile
> +          if attr
> +            newItem["entityMetadata"] = attr
> +          end
> +          colItem.insert 0,  newItem
> +        end
> +      end
> +    when "machineImage"
> +      #Retrieve machine images
> +      images = driver.send(:images, credentials, {})
> +      if images
> +        images.map do |image|
> +          if force_array
> +            newItem = { "name" => [image.name],"description" => [image.description],
> +             "uri" => [image.id],"href" => [HOST_API_PATH + "/" + collType + "/"
+ image.id] }
> +          else
> +            newItem = { "name" => image.name,"description" => image.description,
> +             "uri" => image.id,"href" => HOST_API_PATH + "/" + collType + "/"
+ image.id }
> +          end
> +          attr = cimi_get_image_metadata image
> +          if attr
> +            newItem["entityMetadata"] = attr
> +          end
> +          colItem.insert 0,  newItem
> +        end
> +      end
> +    when "machine"
> +      #Retrieve instances
> +      instances = driver.send(:instances, credentials, {})
> +      if instances
> +        instances.map do |instance|
> +          puts instance.inspect
> +          if force_array
> +            newItem = { "name" => [instance.name],"status" => [instance.state],"uri"
=> [instance.id],
> +              "href" => [HOST_API_PATH + "/" + collType + "/" + instance.id] }
> +          else
> +            newItem = { "name" => instance.name,"status" => instance.state, "uri"
=> instance.id,
> +              "href" => HOST_API_PATH + "/" + collType + "/" + instance.id }
> +          end
> +          attr = cimi_get_machine_metadata instance
> +          if attr
> +            newItem["entityMetadata"] = attr
> +          end
> +          colItem.insert 0,  newItem
> +        end
> +      end
> +    when "volume"
> +      instances = driver.send(:storage_volumes, credentials, {})
> +      if instances
> +        instances.map do |instance|
> +          info instance.inspect
> +          newItem = {
> +             "name" => instance.id,
> +             "href" => HOST_API_PATH + "/" + collType + "/" + instance.id
> +          }
> +          colItem.insert 0,  newItem
> +        end
> +      end
> +    end
> +    colItem
> +  end

This is really 4 methods combined into one. If server.rb had separate
routes for machine, machineImage, machineConfiguration, and volume, this
code would be a lot clearer and in the place where somebody looking at
what happens to, say machineImages, would see it easily. The current
route for get "/api/collection/:collType" is roughly a call to the above
method and then a very big respond_to block. To avoid duplication in
routes for the 4 different collection types above, a lot of that should
go into helpers. I'll explain more below.

> +  def handle_post
> +    raw = request.env["rack.input"].read
> +
> +    content_obj = is_valid_xml_input raw
> +    if content_obj
> +      @xmlRootNode = content_obj.first[0]
> +      @dmtfitem = content_obj.first[1][0]
> +      info @xmlRootNode.inspect
> +      fileId = @dmtfitem["uri"][0]
> +      info @dmtfitem["uri"].inspect
> +
> +      filePath = File.join(STOREROOT, fileId + '.' + params[:collType])
> +      if is_valid?(@dmtfitem) && File.exist?(filePath) == false
> +
> +        File.open(filePath, "w") do |file|
> +          file.write serialize_object_to_xml(content_obj)
> +        end
> +        respond_to do |format|
> +          format.xml do
> +            response.status = 201
> +            response['Location'] = HOST_API_PATH + "#{params[:collType]}/#{fileId}"
> +            haml :"collection/response"
> +          end
> +        end
> +      else
> +        report_error(409)
> +      end
> +    else
> +        #can not accept the content. the request body is not valid.
> +        #412 - Precondition Failed.
> +      report_error(412)
> +    end
> +  end

Again, it's a bad sign that helpers access params, set up instance vars
for the views and call respond_to. This is not helper code, but
controller code.

> diff --git a/server/lib/cimi/helpers/cmwgres_helper.rb b/server/lib/cimi/helpers/cmwgres_helper.rb
> new file mode 100644
> index 0000000..b00339b
> --- /dev/null
> +++ b/server/lib/cimi/helpers/cmwgres_helper.rb

> +module ApplicationHelper
> +
> +  #This method will check the resource created locally and then mix the attributes from
cloud
> +  #then transform the data into a format that UI can handle
> +  def check_DC_Resource!
> +    if DC_SUPPORTED_RESOURCES.include? params[:collType]
> +      filePath = File.join(STOREROOT, params[:id] + '.' + params[:collType])
> +      allRes = get_collection_item_from_DC params[:collType], true
> +      #getting the resource that match the passed in id.
> +      aRes = allRes.select { |item| item["uri"][0] == params[:id]}
> +      #if the array is not empty
> +      if aRes.empty? == false
> +        #using driver as part of the file name to have some namespace.
> +        filePath = File.join STOREROOT, params[:id] + '.' + params[:collType]
> +        #using default value xml
> +        defaultFilePath = File.join(STOREROOT, "defaultRes/" + params[:collType] + ".col.xml")
> +        #read the default xml file in, then mixin the retrieved value, then save it
for further process.
> +
> +        rootHash = XmlSimple.xml_in(defaultFilePath, {'ForceArray' => true, 'KeepRoot'=>true,
'KeyAttr' => ['name']})
> +        info rootHash.first[0]
> +
> +        #handling the merge
> +        rootHash = { "#{rootHash.first[0]}" => [rootHash.first[1][0].merge(aRes.first)]
}
> +
> +        if aRes.first["entityMetadata"]
> +          @metadata = XmlSimple.xml_out aRes.first["entityMetadata"],
> +            {'Indent'=>"&nbsp;&nbsp;&nbsp;",'RootName'=>'entityMetadata','KeyAttr'=>'name','KeepRoot'=>true,'ContentKey'=>'content'}
> +        end
> +
> +        #create a file to represent this resource
> +        File.open(filePath, 'w') do |file|
> +          file.write XmlSimple.xml_out(rootHash, { 'KeyAttr' => 'name', 'KeepRoot'
=> true})
> +        end
> +      end
> +    end
> +  end

I think what we are struggling with is that CIMI wants more data for
many things than what existing clouds give you, and therefore requires
that some things are stored locally, while others come from the backend
cloud.

Codewise, a cleaner approach to doing this would be to have a CIMI
'driver' that decorates the existing Deltacloud drivers. We'd do
something like the following early in server.rb

        cimi = CIMI::Decorator.new(driver)
        
CIMI::Decorator would than provide methods for various collections
similar to how DC drivers are structured today. Controller code could
then call, for example cimi.get_instance, cimi.create_machine_config
etc.

The implementation of get_instance, for example, would call into the
underlying driver to get an instance, mix in any locally managed data
and return the result to the controller code.

Adding this layer of indirection would also make it later easy to use
different storage backends (e.g., flat file storage, RDBMS, in the
backend cloud, ...) for CIMI-specific data.

> +  #this method will convert each hardware profile property into attribute string
> +  def cimi_get_profile_properties profile
> +    #check if this profile has properties
> +    if profile.properties and profile.properties.length > 0
> +      val ='<EntityMetadata xmlns="http://www.dmtf.org/cimi">'
> +      val += '<uri>' + HOST_API_PATH + '/types/MC</uri>'
> +      val += '<name>MachineConfiguration</name>'
> +      val += '<typeURI>http://www.dmtf.org/cimi/MachineConfiguration</typeURI>'
> +      profile.each_property do |p|
> +        the_value = ''
> +        if p.kind == :range
> +          the_type = "xs:integer"
> +          the_value = '<range low="' + p.first.to_s + '" high="' + p.last.to_s +
'" />'
> +        else
> +          the_type = 'xs:string'
> +          if p.kind == :fixed
> +            the_value = '<value>' + p.value.to_s + '</value>'
> +          elsif p.kind == :enum
> +            the_value = '<value>' + p.values.join(',') + '</value>'
> +          end
> +        end
> +        val += '<attribute name="' + p.name.to_s + '" namespace="http://' + ENV["API_HOST"]
+ '" type="' + the_type
> +        val += '" unit="' + p.unit.to_s + '">' + the_value + '</attribute>'
> +      end
> +      val += '</EntityMetadata>'
> +      val = XmlSimple.xml_in(val, {'ForceArray' => true, 'KeepRoot'=>false, 'KeyAttr'
=> ['name']})
> +      return val
> +    end
> +    return nil
> +  end

This really needs to go into a HAML template. (applies to
cimi_get_image_metadata and cimi_get_machine_metadata, too)

> diff --git a/server/lib/cimi/server.rb b/server/lib/cimi/server.rb
> new file mode 100644
> index 0000000..ed406bf
> --- /dev/null
> +++ b/server/lib/cimi/server.rb
> @@ -0,0 +1,252 @@
>
> +# FIXME: Can we get rid of the dependency on active_support ?
> +#require 'active_support'
> +#require 'active_support/inflector'
> +#require 'active_support/core_ext/object/blank'
> +#require 'active_support/core_ext/hash/conversions'

Since this is commented out, can it be removed now ?

> +# FIXME: This contains a mix of static and dynamically modified data. This
> +# needs to be separated out, and only drivers should touch dynamic data

Is this still the case ?

> +# FIXME: There used to be code that created STOREROOT when it doesn't
> +# exist.  But not having storeroot leads to an error when requesting the
> +# entry point

Has this FIXME been addressed ? Namely the observation that the server
won't start if lib/cimi/data/ is empty.

> +# here we setup a directory for persistence, everything will be saved as
> +# files the file format will be uuid.resourceTypeName, for example: if
> +# there is a machineTemplte, and its id is
> +# dab4fdae-1451-48f4-b5b6-2b0dcc06bb14, then the file at the storage will
> +# be dab4fdae-1451-48f4-b5b6-2b0dcc06bb14.machineTemplate, each file
> +# content should be in xml format.  the root directory of the storage is
> +# <rootDir>/store so we create one if one does not exist yet.
> +
> +STOREROOT = File.join($top_srcdir, 'lib', 'cimi', 'data')
> +#We would like to know the storage root.
> +puts "store root is " + STOREROOT

Please remove all 'puts'

One general question about routes: if we ever want to run the DC and
CIMI frontends in the same server, we'd need to segregate the URL
spaces. Should we just prefix all CIMI routes with /cimi instead
of /api ?

> +get "/api/cloudEntryPoint" do
> +
> +  @allAPIs = CIMI_RESOURCES
> +  respond_to do |format|
> +    format.xml  do
> +      content_type 'application/CIMI-CloudEntryPoint+xml', :charset => 'utf-8'
> +      haml:"cloudEntryPoint/index"
> +    end
> +    format.html { haml:"cloudEntryPoint/index" }
> +    format.json do
> +      content_type 'application/CIMI-CloudEntryPoint+json', :charset => 'utf-8'
> +      engine = Haml::Engine.new(File.read(settings.views + "/cloudEntryPoint/index.xml.haml"))
> +      responseXML = engine.render self
> +      hash_response = XmlSimple.xml_in responseXML, {'ForceArray' => false, 'KeepRoot'=>true,
'KeyAttr' => ['name']}
> +      info hash_response
> +      hash_response = hash_response.first[1]
> +      info hash_response
> +      if hash_response.has_key?("xmlns")
> +        hash_response.delete "xmlns"
> +      end
> +      res = hash_response.to_json

This whole block is the kind of thing that would do well in a helper
(say 'json_from_xml_template')

Also, there's a good amount of debug output.

> +get "/api/collection/:collType" do
> +  if RESOURCE_NAMES.include? params[:collType]
> +
> +    # here we will handle the update of changing the collection attribute.
> +    # no item in the collection will be changed, this will be only used to change properties
of a collection.
> +    #the resources should be retrieved from DeltaCloud
> +    if DC_SUPPORTED_RESOURCES.include? params[:collType]
> +      @dmtfColItems = get_collection_item_from_DC params[:collType]
> +    else
> +      @dmtfColItems = get_collection_item params[:collType]
> +    end
> +
> +    respond_to do |format|
> +      format.html do
> +        rootHash = XmlSimple.xml_in(File.join(STOREROOT, 'collections/' + params[:collType]
+ '.col.xml'),
> +                   { 'ForceArray' => false, 'KeepRoot'=>true, 'KeyAttr' =>
['name']})
> +
> +        @xmlRootNode = rootHash.first[0]
> +        @dmtfitem = rootHash.first[1]
> +        info @dmtfitem
> +        haml :"collection/index"
> +      end
> +      format.xml do
> +        rootHash = XmlSimple.xml_in(File.join(STOREROOT, 'collections/' + params[:collType]
+ '.col.xml'),
> +                   { 'ForceArray' => true, 'KeepRoot'=>true, 'KeyAttr' => ['name']})
> +        info rootHash
> +        colItemName = rootHash.first[0]
> +        content_type get_response_content_type(colItemName, 'xml'), :charset => 'utf-8'
> +        colItemName = colItemName.sub(/Collection/,'') #Remove the Collection at the
end.

Again, the last 5 lines would do well in their own helper.

> +        colItemName = colItemName[0].downcase + colItemName[1, colItemName.length]

This one line should become a method in
lib/deltacloud/core_ext/string.rb (I forget what Rails calls that
method, we should just copy it)

> +        #we need to produce the url collection
> +        urls = []
> +        @dmtfColItems.map do |item|
> +          urls << {"href" => item["href"]}
> +        end
> +
> +        rootHash.first[1][0]["#{colItemName}"] = urls
> +
> +        XmlSimple.xml_out(rootHash, { 'KeyAttr' => 'name', 'KeepRoot' => true,
'ContentKey' => 'content'})

Why doesn't this code set up some instance variables and then hand over
to a HAML template ? It seems kinda backwards to construct an XML DOM,
then modify that a little and then send it back.

I still need to look at the rest of the patch, but this should be a good
start ;)

David



Mime
View raw message