From 367f812476cec3f9d9287fdb8b1f45decf358f9f Mon Sep 17 00:00:00 2001
From: Denis Egorenko <degorenko@mirantis.com>
Date: Mon, 18 Jan 2016 16:54:35 +0300
Subject: [PATCH] Hash domains by name

Currently every project and user resource generates a call to
fetch_domain, which is an unhashed query to the openstack domain show
command. This is very wasteful and does not scale well. This patch takes
the code that hashes domains by id and generates an inverse, a hash of
domains by name. This code is used in place of fetch_domain, which is
removed. Since the only thing used in this hash is the id, which should
be immutable, we should not have issues with the other fields changing
(like description and enabled).

This should provide a performance boost to users who have a lot of users
and projects, especially if everything is in the default domain.

Co-Authored-By: Denis Egorenko <degorenko@mirantis.com>

Change-Id: I99d8fe272aedc0cbc6ad48561ea50a7b7d6cdb1e
---
 lib/puppet/provider/keystone.rb     | 18 ++++++++-----
 spec/unit/provider/keystone_spec.rb | 42 ++++++++++++++++-------------
 2 files changed, 36 insertions(+), 24 deletions(-)

diff --git a/lib/puppet/provider/keystone.rb b/lib/puppet/provider/keystone.rb
index b6978dc4e..c1a747873 100644
--- a/lib/puppet/provider/keystone.rb
+++ b/lib/puppet/provider/keystone.rb
@@ -111,10 +111,17 @@ class Puppet::Provider::Keystone < Puppet::Provider::Openstack
     @domain_hash[id]
   end
 
-  def self.fetch_domain(domain)
-    request('domain', 'show', domain)
-  rescue Puppet::ExecutionFailure => e
-    raise e unless e.message =~ /No domain with a name or ID/
+  def self.domain_id_from_name(name)
+    unless @domain_hash_name
+      list = request('domain', 'list')
+      @domain_hash_name = Hash[list.collect{|domain| [domain[:name], domain[:id]]}]
+    end
+    unless @domain_hash_name.include?(name)
+      id = request('domain', 'show', name)[:id]
+      err("Could not find domain with name [#{name}]") unless id
+      @domain_hash_name[name] = id
+    end
+    @domain_hash_name[name]
   end
 
   def self.fetch_project(name, domain)
@@ -214,8 +221,7 @@ class Puppet::Provider::Keystone < Puppet::Provider::Openstack
     if domain_name.nil? || domain_name.empty?
       raise(Puppet::Error, "Missing domain name for resource #{name}")
     end
-    domain = fetch_domain(domain_name)
-    domain_id = domain ? domain[:id] : nil
+    domain_id = self.domain_id_from_name(domain_name)
     case domain_id
     when default_domain_id
       name
diff --git a/spec/unit/provider/keystone_spec.rb b/spec/unit/provider/keystone_spec.rb
index 5561a62ca..7e694acdb 100644
--- a/spec/unit/provider/keystone_spec.rb
+++ b/spec/unit/provider/keystone_spec.rb
@@ -33,26 +33,32 @@ describe Puppet::Provider::Keystone do
     another_class.reset
   end
 
-  describe '#fetch_domain' do
-    it 'should be false if the domain does not exist' do
-      # retry only once.  Not doing this make the test unnecessary
-      # long (1 minute) and retry the command ~20times
-      klass.expects(:request_timeout).returns(0)
+  describe '#domain_id_from_name' do
+    it 'should list all domains when requesting a domain name from an ID' do
       klass.expects(:openstack)
-        .with('domain', 'show', '--format', 'shell', 'no_domain')
-        .times(2)
-        .raises(Puppet::ExecutionFailure, "Execution of '/usr/bin/openstack domain show --format shell no_domain' returned 1: No domain with a name or ID of 'no_domain' exists.")
-      expect(klass.fetch_domain('no_domain')).to be_falsey
-    end
-
-    it 'should return the domain' do
-      klass.expects(:openstack)
-        .with('domain', 'show', '--format', 'shell', 'The Domain')
-        .returns('
-name="The Domain"
-id="the_domain_id"
+           .with('domain', 'list', '--quiet', '--format', 'csv', [])
+           .returns('"ID","Name","Enabled","Description"
+"someid","SomeName",True,"default domain"
 ')
-      expect(klass.fetch_domain('The Domain')).to eq({:name=>"The Domain", :id=>"the_domain_id"})
+      expect(klass.domain_id_from_name('SomeName')).to eq('someid')
+    end
+    it 'should lookup a domain when not found in the hash' do
+      klass.expects(:openstack)
+           .with('domain', 'show', '--format', 'shell', 'NewName')
+           .returns('
+name="NewName"
+id="newid"
+')
+      expect(klass.domain_id_from_name('NewName')).to eq('newid')
+    end
+    it 'should print an error when there is no such domain' do
+      klass.expects(:openstack)
+           .with('domain', 'show', '--format', 'shell', 'doesnotexist')
+           .returns('
+')
+      klass.expects(:err)
+           .with('Could not find domain with name [doesnotexist]')
+      expect(klass.domain_id_from_name('doesnotexist')).to eq(nil)
     end
   end