Code review comment for lp:~hopem/charms/precise/cinder/trunk

Revision history for this message
Adam Gandelman (gandelman-a) wrote :

Hey Edward, Thanks for the merge. Couple of things:

- Shouldn't to add the os_version_is_gte() to openstack-common, we can take advantage of dpkg's version checking to accomplish the same thing, eg:

if dpkg --compare-versions "$(get_os_version_package cinder-volume)" ge 2013.1 ; then
  # do some grizzly stuff
fi

Also, you should check for cinder-common's version and not cinder-volume. ATM cinder-common is installed for all cinder services. cinder-volume will only be installed if enabled-services includes volume.

- Not sure the best way to set the glance API is through config. Its potentially error prone eg, settings API v2 but current related glance server only supports v1. Some other options:

  * The API setting goes into config glance charms config and api_version='$(config-get api-version)' is sent to related services via the image-service interface. glance charm can take responsibility of ensuring its configured to support that API version.

  * During image-service-relation-changed the cinder charm can inspect the glance endpoint in keystone to set it in cinder.conf accordingly. This may be a simple curl request to the endpoint to list supported API versions (if the api server supports such a request)

review: Needs Fixing

« Back to merge proposal