Add ability to set properties with glance_image

This change updates the glance_image provider to support the ability to
specify properties for an image.  This change only adds the support for
setting properties. The conversion and update to using the v2 will
require further rework of this provider.

Change-Id: I22b92c5ccd0f77c837e9abe987cee07c7d90867e
Co-Authored-By: Alex Schultz <aschultz@mirantis.com>
(cherry picked from commit d0957fb622)

lib/puppet/provider/glance_image/openstack.rb

@@ -10,10 +10,9 @@ Puppet::Type.type(:glance_image).provide(
 
   @credentials = Puppet::Provider::Openstack::CredentialsV2_0.new
 
-  # glanceclient support `image create` (in v2 API) but not openstackclient
+  # TODO(aschultz): v2 is now supported but the options are different and
-  # openstackclient now uses image v2 API by default.
+  # it doesn't support the source being remote. We'll have to rework this
-  # in the meantime it's implemented in openstackclient, hardcode version
+  # to support v2
-  # see https://bugs.launchpad.net/python-openstackclient/+bug/1405562
   ENV['OS_IMAGE_API_VERSION'] = '1'
 
   def initialize(value={})
@@ -22,33 +21,31 @@ Puppet::Type.type(:glance_image).provide(
   end
 
   def create
-    if resource[:source]
+    if @resource[:source]
       # copy_from cannot handle file://
-      if resource[:source] =~ /^\// # local file
+      if @resource[:source] =~ /^\// # local file
-        location = "--file=#{resource[:source]}"
+        location = "--file=#{@resource[:source]}"
       else
-        location = "--copy-from=#{resource[:source]}"
+        location = "--copy-from=#{@resource[:source]}"
       end
     # location cannot handle file://
     # location does not import, so no sense in doing anything more than this
-    elsif resource[:location]
+    elsif @resource[:location]
-      location = "--location=#{resource[:location]}"
+      location = "--location=#{@resource[:location]}"
     else
       raise(Puppet::Error, "Must specify either source or location")
     end
-    properties = [resource[:name]]
+    opts = [@resource[:name]]
-    if resource[:is_public] == :true
+
-      properties << "--public"
+    opts << (@resource[:is_public] == :true ? '--public' : '--private')
-    else
+    opts << "--container-format=#{@resource[:container_format]}"
-      # This is the default, but it's nice to be verbose
+    opts << "--disk-format=#{@resource[:disk_format]}"
-      properties << "--private"
+    opts << "--min-disk=#{@resource[:min_disk]}" if @resource[:min_disk]
-    end
+    opts << "--min-ram=#{@resource[:min_ram]}" if @resource[:min_ram]
-    properties << "--container-format=#{resource[:container_format]}"
+    opts << props_to_s(@resource[:properties]) if @resource[:properties]
-    properties << "--disk-format=#{resource[:disk_format]}"
+    opts << location
-    properties << "--min-disk=#{resource[:min_disk]}" if resource[:min_disk]
+
-    properties << "--min-ram=#{resource[:min_ram]}" if resource[:min_ram]
+    @property_hash = self.class.request('image', 'create', opts)
-    properties << location
-    @property_hash = self.class.request('image', 'create', properties)
     @property_hash[:ensure] = :present
   end
 
@@ -57,10 +54,12 @@ Puppet::Type.type(:glance_image).provide(
   end
 
   def destroy
-    self.class.request('image', 'delete', resource[:name])
+    self.class.request('image', 'delete', @resource[:name])
     @property_hash.clear
   end
 
+  mk_resource_methods
+
   def is_public=(value)
     @property_flush[:is_public] = value
   end
@@ -73,18 +72,10 @@ Puppet::Type.type(:glance_image).provide(
     @property_flush[:disk_format] = value
   end
 
-  def disk_format
-    @property_hash[:disk_format]
-  end
-
   def container_format=(value)
     @property_flush[:container_format] = value
   end
 
-  def container_format
-    @property_hash[:container_format]
-  end
-
   def min_ram=(value)
     @property_flush[:min_ram] = value
   end
@@ -93,18 +84,19 @@ Puppet::Type.type(:glance_image).provide(
     @property_flush[:min_disk] = value
   end
 
+  def properties=(value)
+    @property_flush[:properties] = value
+  end
+
   def id=(id)
     fail('id is read only')
   end
 
-  def id
-    @property_hash[:id]
-  end
-
   def self.instances
-    list = request('image', 'list', '--long')
+    list = request('image', 'list')
     list.collect do |image|
       attrs = request('image', 'show', image[:id])
+      properties = Hash[attrs[:properties].scan(/(\S+)='([^']*)'/)] rescue nil
       new(
         :ensure           => :present,
         :name             => attrs[:name],
@@ -112,8 +104,9 @@ Puppet::Type.type(:glance_image).provide(
         :container_format => attrs[:container_format],
         :id               => attrs[:id],
         :disk_format      => attrs[:disk_format],
-        :min_disk         => attrs['min_disk'],
+        :min_disk         => attrs[:min_disk],
-        :min_ram          => attrs['min_ram']
+        :min_ram          => attrs[:min_ram],
+        :properties       => properties
       )
     end
   end
@@ -128,17 +121,25 @@ Puppet::Type.type(:glance_image).provide(
   end
 
   def flush
-    properties = [resource[:name]]
     if @property_flush
-      (properties << '--public') if @property_flush[:is_public] == :true
+      opts = [@resource[:name]]
-      (properties << '--private') if @property_flush[:is_public] == :false
+
-      (properties << "--container-format=#{@property_flush[:container_format]}") if @property_flush[:container_format]
+      (opts << '--public') if @property_flush[:is_public] == :true
-      (properties << "--disk-format=#{@property_flush[:disk_format]}") if @property_flush[:disk_format]
+      (opts << '--private') if @property_flush[:is_public] == :false
-      (properties << "--min-ram=#{@property_flush[:min_ram]}") if @property_flush[:min_ram]
+      (opts << "--container-format=#{@property_flush[:container_format]}") if @property_flush[:container_format]
-      (properties << "--min-disk=#{@property_flush[:min_disk]}") if @property_flush[:min_disk]
+      (opts << "--disk-format=#{@property_flush[:disk_format]}") if @property_flush[:disk_format]
-      self.class.request('image', 'set', properties)
+      (opts << "--min-ram=#{@property_flush[:min_ram]}") if @property_flush[:min_ram]
+      (opts << "--min-disk=#{@property_flush[:min_disk]}") if @property_flush[:min_disk]
+      (opts << props_to_s(@property_flush[:properties])) if @property_flush[:properties]
+
+      self.class.request('image', 'set', opts)
       @property_flush.clear
     end
   end
 
+  private
+
+  def props_to_s(props)
+    props.flat_map{ |k, v| ['--property', "#{k}=#{v}"] }
+  end
 end

lib/puppet/type/glance_image.rb

@@ -13,13 +13,14 @@ Puppet::Type.newtype(:glance_image) do
       source           => 'http://uec-images.ubuntu.com/releases/precise/release/ubuntu-12.04-server-cloudimg-amd64-disk1.img'
       min_ram          => 1234,
       min_disk         => 1234,
+      properties       => { 'img_key' => img_value }
     }
 
     Known problems / limitations:
-      * All images are managed by the glance service. 
+      * All images are managed by the glance service.
         This means that since users are unable to manage their own images via this type,
         is_public is really of no use. You can probably hide images this way but that's all.
-      * As glance image names do not have to be unique, you must ensure that your glance 
+      * As glance image names do not have to be unique, you must ensure that your glance
         repository does not have any duplicate names prior to using this.
       * Ensure this is run on the same server as the glance-api service.
 
@@ -74,21 +75,39 @@ Puppet::Type.newtype(:glance_image) do
     newvalues(/\S+/)
   end
 
-  newparam(:min_ram) do
+  newproperty(:min_ram) do
     desc "The minimal ram size"
     newvalues(/\d+/)
   end
 
-  newparam(:min_disk) do
+  newproperty(:min_disk) do
     desc "The minimal disk size"
     newvalues(/\d+/)
   end
 
+  newproperty(:properties) do
+    desc "The set of image properties"
+
+    munge do |value|
+      return value if value.is_a? Hash
+
+      # wrap property value in commas
+      value.gsub!(/=(\w+)/, '=\'\1\'')
+      Hash[value.scan(/(\S+)='([^']*)'/)]
+    end
+
+    validate do |value|
+      return true if value.is_a? Hash
+
+      value.split(',').each do |property|
+        raise ArgumentError, "Key/value pairs should be separated by an =" unless property.include?('=')
+      end
+    end
+  end
+
   # Require the Glance service to be running
   autorequire(:service) do
-    ['glance']
+    ['glance-api', 'glance-registry']
   end
 
 end
-
-

spec/acceptance/basic_glance_spec.rb

@@ -47,6 +47,9 @@ describe 'glance class' do
         disk_format      => 'qcow2',
         is_public        => 'yes',
         source           => 'http://download.cirros-cloud.net/0.3.1/cirros-0.3.1-x86_64-disk.img',
+        min_ram          => '64',
+        min_disk         => '1024',
+        properties       => { 'icanhaz' => 'cheezburger' },
       }
     EOS
 
@@ -77,11 +80,21 @@ describe 'glance class' do
     end
 
     describe 'glance images' do
-      it 'should create a glance image' do
+      it 'should create a glance image with proper attributes' do
-        shell('openstack --os-username glance --os-password a_big_secret --os-tenant-name services --os-auth-url http://127.0.0.1:5000/v2.0 image list') do |r|
+        glance_env_opts = '--os-username glance --os-password a_big_secret --os-tenant-name services --os-auth-url http://127.0.0.1:5000/v2.0'
+        shell("openstack #{glance_env_opts} image list") do |r|
           expect(r.stdout).to match(/test_image/)
           expect(r.stderr).to be_empty
         end
+        shell("openstack #{glance_env_opts} image show test_image --format shell") do |r|
+          expect(r.stdout).to match(/visibility="public"/)
+          expect(r.stdout).to match(/container_format="bare"/)
+          expect(r.stdout).to match(/disk_format="qcow2"/)
+          expect(r.stdout).to match(/properties="icanhaz='cheezburger'"/)
+          expect(r.stdout).to match(/min_ram="64"/)
+          expect(r.stdout).to match(/min_disk="1024"/)
+          expect(r.stderr).to be_empty
+        end
       end
     end
   end

spec/unit/provider/glance_image_spec.rb

@@ -39,31 +39,26 @@ describe provider_class do
     it_behaves_like 'authenticated with environment variables' do
       describe '#create' do
         it 'creates an image' do
-          provider.class.stubs(:openstack)
-                        .with('image', 'list', '--quiet', '--format', 'csv', '--long')
-                        .returns('"ID","Name","Disk Format","Container Format","Size","Status"
-"534  5b502-efe4-4852-a45d-edaba3a3acc6","image1","raw","bare",1270,"active"
-')
           provider.class.stubs(:openstack)
                         .with('image', 'create', '--format', 'shell', ['image1', '--public', '--container-format=bare', '--disk-format=qcow2', '--min-disk=1024', '--min-ram=1024', '--copy-from=http://example.com/image1.img' ])
-                        .returns('checksum="09b9c392dc1f6e914cea287cb6be34b0"
+                        .returns('checksum="ee1eca47dc88f4879d8a229cc70a07c6"
 container_format="bare"
-created_at="2015-04-08T18:28:01"
+created_at="2016-03-29T20:52:24Z"
-deleted="False"
-deleted_at="None"
 disk_format="qcow2"
-id="5345b502-efe4-4852-a45d-edaba3a3acc6"
+file="/v2/images/8801c5b0-c505-4a15-8ca3-1d2383f8c015/file"
-is_public="True"
+id="8801c5b0-c505-4a15-8ca3-1d2383f8c015"
 min_disk="1024"
 min_ram="1024"
 name="image1"
-owner="None"
+owner="5a9e521e17014804ab8b4e8b3de488a4"
-properties="{}"
 protected="False"
-size="1270"
+schema="/v2/schemas/image"
+size="13287936"
 status="active"
-updated_at="2015-04-10T18:18:18"
+tags=""
+updated_at="2016-03-29T20:52:40Z"
 virtual_size="None"
+visibility="public"
 ')
           provider.create
           expect(provider.exists?).to be_truthy
@@ -73,9 +68,6 @@ virtual_size="None"
 
     describe '#destroy' do
       it 'destroys an image' do
-        provider.class.stubs(:openstack)
-                      .with('image', 'list', '--quiet', '--format', 'csv')
-                      .returns('"ID","Name","Disk Format","Container Format","Size","Status"')
         provider.class.stubs(:openstack)
                       .with('image', 'delete', 'image1')
         provider.destroy
@@ -87,9 +79,9 @@ virtual_size="None"
     describe '.instances' do
       it 'finds every image' do
         provider.class.stubs(:openstack)
-                      .with('image', 'list', '--quiet', '--format', 'csv', '--long')
+                      .with('image', 'list', '--quiet', '--format', 'csv', [])
-                      .returns('"ID","Name","Disk Format","Container Format","Size","Status"
+                      .returns('"ID","Name","Status"
-"5345b502-efe4-4852-a45d-edaba3a3acc6","image1","raw","bare",1270,"active"
+"5345b502-efe4-4852-a45d-edaba3a3acc6","image1","active"
 ')
         provider.class.stubs(:openstack)
                       .with('image', 'show', '--format', 'shell', '5345b502-efe4-4852-a45d-edaba3a3acc6')
@@ -118,4 +110,95 @@ virtual_size="None"
     end
 
   end
+
+  describe 'when managing an image with properties' do
+
+    let(:tenant_attrs) do
+      {
+        :ensure           => 'present',
+        :name             => 'image1',
+        :is_public        => 'yes',
+        :container_format => 'bare',
+        :disk_format      => 'qcow2',
+        :source           => '/var/tmp/image1.img',
+        :min_ram          => 1024,
+        :min_disk         => 1024,
+        :properties       => { 'something' => 'what', 'vmware_disktype' => 'sparse' }
+      }
+    end
+
+    let(:resource) do
+      Puppet::Type::Glance_image.new(tenant_attrs)
+    end
+
+    let(:provider) do
+      provider_class.new(resource)
+    end
+
+    it_behaves_like 'authenticated with environment variables' do
+      describe '#create' do
+        it 'creates an image' do
+          provider.class.stubs(:openstack)
+                        .with('image', 'create', '--format', 'shell', ['image1', '--public', '--container-format=bare', '--disk-format=qcow2', '--min-disk=1024', '--min-ram=1024', ['--property', 'something=what', '--property', 'vmware_disktype=sparse'], '--file=/var/tmp/image1.img' ])
+                        .returns('checksum="ee1eca47dc88f4879d8a229cc70a07c6"
+container_format="bare"
+created_at="2016-03-29T20:52:24Z"
+disk_format="qcow2"
+file="/v2/images/8801c5b0-c505-4a15-8ca3-1d2383f8c015/file"
+id="8801c5b0-c505-4a15-8ca3-1d2383f8c015"
+min_disk="1024"
+min_ram="1024"
+name="image1"
+owner="5a9e521e17014804ab8b4e8b3de488a4"
+properties="something=\'what\', vmware_disktype=\'sparse\'"
+protected="False"
+schema="/v2/schemas/image"
+size="13287936"
+status="active"
+tags=""
+updated_at="2016-03-29T20:52:40Z"
+virtual_size="None"
+visibility="public"
+')
+          provider.create
+          expect(provider.exists?).to be_truthy
+        end
+      end
+    end
+
+    describe '.instances' do
+      it 'finds every image' do
+        provider.class.stubs(:openstack)
+                      .with('image', 'list', '--quiet', '--format', 'csv', [])
+                      .returns('"ID","Name","Status"
+"5345b502-efe4-4852-a45d-edaba3a3acc6","image1","active"
+')
+        provider.class.stubs(:openstack)
+                      .with('image', 'show', '--format', 'shell', '5345b502-efe4-4852-a45d-edaba3a3acc6')
+                      .returns('checksum="09b9c392dc1f6e914cea287cb6be34b0"
+container_format="bare"
+created_at="2015-04-08T18:28:01"
+deleted="False"
+deleted_at="None"
+disk_format="qcow2"
+id="5345b502-efe4-4852-a45d-edaba3a3acc6"
+is_public="True"
+min_disk="1024"
+min_ram="1024"
+name="image1"
+owner="None"
+properties="something=\'what\', vmware_disktype=\'sparse\'"
+protected="False"
+size="1270"
+status="active"
+updated_at="2015-04-10T18:18:18"
+virtual_size="None"
+')
+        instances = provider_class.instances
+        expect(instances.count).to eq(1)
+      end
+    end
+
+  end
+
 end