Hi Tong,
On Wed, 2011-09-28 at 11:33 -0700, [email protected] wrote:
> From: Tong Li <[email protected]>
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(/</, "<").gsub(/>/, ">").gsub(/\n/, "\r")
That last gsub seems Windows-specific to me. You also need to gsub(/&/,
"&") 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}'>δ 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'=>" ",'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
