initial review

lib/vmpooler.rb

@@ -43,6 +43,7 @@ module Vmpooler
       if parsed_config[:config]['extra_config']
         extra_configs = parsed_config[:config]['extra_config'].split(',')
         extra_configs.each do |config|
+          puts "loading extra_config file #{config}"
           extra_config = YAML.load_file(config)
           parsed_config.deep_merge(extra_config)
         end
@@ -133,6 +134,7 @@ module Vmpooler
     # Create an index of pool aliases
     parsed_config[:pool_names] = Set.new
     unless parsed_config[:pools]
+      puts 'loading pools configuration from redis, since the config[:pools] is empty'
       redis = new_redis(parsed_config[:redis]['server'], parsed_config[:redis]['port'], parsed_config[:redis]['password'])
       parsed_config[:pools] = load_pools_from_redis(redis)
     end

lib/vmpooler/api/v2.rb

@@ -1,12 +1,17 @@
 # frozen_string_literal: true
 
 require 'vmpooler/api/v1'
+
 module Vmpooler
   class API
     class V2 < Vmpooler::API::V1
       api_version = '2'
       api_prefix  = "/api/v#{api_version}"
 
+      def full_config
+        Vmpooler::API.settings.config
+      end
+
       def get_template_aliases(template)
         tracer.in_span("Vmpooler::API::V2.#{__method__}") do
           result = []
@@ -19,11 +24,6 @@ module Vmpooler
         end
       end
 
-      def get_domain_for_pool(poolname)
-        pool_index = pool_index(pools)
-        pools[pool_index[poolname]]['domain']
-      end
-
       # Fetch a single vm from a pool
       #
       # @param [String] template
@@ -73,7 +73,7 @@ module Vmpooler
               next if vms.empty?
 
               vms.reverse.each do |vm|
-                vm_domain = get_domain_for_pool(template_backend)
+                vm_domain = Parsing.get_domain_for_pool(full_config, template_backend)
                 ready = vm_ready?(vm, vm_domain)
                 if ready
                   smoved = backend.smove("vmpooler__ready__#{template_backend}", "vmpooler__running__#{template_backend}", vm)
@@ -137,12 +137,11 @@ module Vmpooler
           else
             vm_names = []
             vms.each do |(vmpool, vmname, vmtemplate)|
-              vmdomain = get_domain_for_pool(vmpool)
+              vmdomain = Parsing.get_domain_for_pool(full_config, vmpool)
               if vmdomain
                 vmfqdn = "#{vmname}.#{vmdomain}"
                 update_result_hosts(result, vmtemplate, vmfqdn)
                 vm_names.append(vmfqdn)
-                result['domain'] = vmdomain
               else
                 update_result_hosts(result, vmtemplate, vmname)
                 vm_names.append(vmname)
@@ -197,11 +196,7 @@ module Vmpooler
           platforms_with_aliases = []
           requested_instances.each do |poolname, count|
             selection = evaluate_template_aliases(poolname, count)
-            selection.map do |selected_pool, selected_pool_count|
+            selection.map { |selected_pool, selected_pool_count| platforms_with_aliases << "#{poolname}:#{selected_pool}:#{selected_pool_count}" }
-              platforms_with_aliases << "#{poolname}:#{selected_pool}:#{selected_pool_count}"
-              pool_domain = get_domain_for_pool(selected_pool)
-              result['domain'] = pool_domain if pool_domain
-            end
           end
           platforms_string = platforms_with_aliases.join(',')
 

lib/vmpooler/pool_manager.rb

@@ -60,7 +60,7 @@ module Vmpooler
             to_set[k] = pool[k]
           end
           to_set['alias'] = pool['alias'].join(',') if to_set.key?('alias')
-          to_set['domain'] = get_domain_for_pool(pool['name'])
+          to_set['domain'] = Parsing.get_domain_for_pool(config, pool['name'])
           redis.hmset("vmpooler__pool__#{pool['name']}", to_set.to_a.flatten) unless to_set.empty?
         end
         previously_configured_pools.each do |pool|
@@ -362,7 +362,7 @@ module Vmpooler
       max_hostname_retries = 3
       while hostname_retries < max_hostname_retries
         hostname, hostname_available = generate_and_check_hostname
-        domain = get_domain_for_pool(pool_name)
+        domain = Parsing.get_domain_for_pool(config, pool_name)
         if domain
           fqdn = "#{hostname}.#{domain}"
         else
@@ -384,7 +384,7 @@ module Vmpooler
 
       raise "Unable to generate a unique hostname after #{hostname_retries} attempts. The last hostname checked was #{fqdn}" unless hostname_available && dns_available
 
-      fqdn
+      hostname
     end
 
     # Query the DNS for the name we want to create and if it already exists, mark it unavailable
@@ -402,7 +402,7 @@ module Vmpooler
 
     def _clone_vm(pool_name, provider, request_id = nil, pool_alias = nil)
       new_vmname = find_unique_hostname(pool_name)
-      pool_domain = get_domain_for_pool(pool_name)
+      pool_domain = Parsing.get_domain_for_pool(config, pool_name)
       mutex = vm_mutex(new_vmname)
       mutex.synchronize do
         @redis.with_metrics do |redis|
@@ -665,27 +665,6 @@ module Vmpooler
       $providers[provider_name]
     end
 
-    # @param pool_name [String] - the name of the pool
-    # @return [String] - domain name for pool, if set
-    def get_domain_for_pool(pool_name)
-      pool = $config[:pools].find { |p| p['name'] == pool_name }
-      return nil unless pool
-
-      provider_name = pool.fetch('provider', nil)
-      return nil unless provider_name
-
-      $logger.log('d', "[*] [get_domain_for_pool] #{pool_name}'s provider is #{provider_name}")
-      if config[:providers] && config[:providers][provider_name.to_sym] && config[:providers][provider_name.to_sym]['domain']
-        domain = config[:providers][provider_name.to_sym]['domain']
-      elsif config[:config] && config[:config]['domain']
-        domain = config[:config]['domain']
-      else
-        domain = nil
-      end
-
-      domain
-    end
-
     def check_disk_queue(maxloop = 0, loop_delay = 5)
       $logger.log('d', '[*] [disk_manager] starting worker thread')
 

lib/vmpooler/util/parsing.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-# utility class shared between apps
+# utility class shared between apps api and pool_manager
 module Vmpooler
   class Parsing
     def self.get_platform_pool_count(requested, &_block)
@@ -12,5 +12,25 @@ module Vmpooler
         yield platform_alias, pool, count
       end
     end
+
+    # @param config [String] - the full config structure
+    # @param pool_name [String] - the name of the pool
+    # @return [String] - domain name for pool, if set in the provider for the pool or in the config block
+    def self.get_domain_for_pool(config, pool_name)
+      pool = config[:pools].find { |p| p['name'] == pool_name }
+      return nil unless pool
+
+      provider_name = pool.fetch('provider', 'vsphere') # see vmpooler.yaml.example where it states defaulting to vsphere
+
+      if config[:providers] && config[:providers][provider_name.to_sym] && config[:providers][provider_name.to_sym]['domain']
+        domain = config[:providers][provider_name.to_sym]['domain']
+      elsif config[:config] && config[:config]['domain']
+        domain = config[:config]['domain']
+      else
+        domain = nil
+      end
+
+      domain
+    end
   end
 end

spec/spec_helper.rb

@@ -1,7 +1,9 @@
 require 'simplecov'
+=begin
 SimpleCov.start do
   add_filter '/spec/'
 end
+=end
 require 'helpers'
 require 'rspec'
 require 'vmpooler'

spec/unit/pool_manager_spec.rb

@@ -1672,93 +1672,6 @@ EOT
     end
   end
 
-  describe '#get_domain_for_pool' do
-    let(:provider_name) { 'mock_provider' }
-    context 'No provider is set' do
-      let(:config) { YAML.load(<<~EOT
-        ---
-        :config:
-        :providers:
-          :mock_provider:
-        :pools:
-          - name: '#{pool}'
-            size: 1
-        EOT
-      )}
-
-      it 'should return nil' do
-        result = subject.get_domain_for_pool(pool)
-        expect(result).to be_nil
-      end
-    end
-    context 'No domain is set' do
-      let(:config) { YAML.load(<<~EOT
-        ---
-        :config:
-        :providers:
-          :mock_provider:
-        :pools:
-          - name: '#{pool}'
-            size: 1
-            provider: #{provider_name}
-        EOT
-      )}
-
-      it 'should return nil' do
-        allow(subject).to receive(:get_provider_for_pool).with(pool).and_return(provider)
-        allow(provider).to receive(:name).and_return(provider_name)
-        result = subject.get_domain_for_pool(pool)
-        expect(result).to be_nil
-      end
-    end
-
-    context 'Only a global domain is set' do
-      let(:config) { YAML.load(<<~EOT
-        ---
-        :config:
-          domain: example.com
-        :providers:
-          :mock_provider:
-        :pools:
-          - name: '#{pool}'
-            size: 1
-            provider: #{provider_name}
-        EOT
-      )}
-
-      it 'should return the domain set in the config section' do
-        allow(subject).to receive(:get_provider_for_pool).with(pool).and_return(provider)
-        allow(provider).to receive(:name).and_return(provider_name)
-        result = subject.get_domain_for_pool(pool)
-        expect(result).to_not be_nil
-        expect(result).to eq('example.com')
-      end
-    end
-
-    context 'A provider specified a domain to use' do
-      let(:config) { YAML.load(<<~EOT
-        ---
-        :config:
-        :providers:
-          :mock_provider:
-            domain: m.example.com
-        :pools:
-          - name: '#{pool}'
-            size: 1
-            provider: #{provider_name}
-        EOT
-      )}
-
-      it 'should return the domain set in the config section' do
-        allow(subject).to receive(:get_provider_for_pool).with(pool).and_return(provider)
-        allow(provider).to receive(:name).and_return(provider_name)
-        result = subject.get_domain_for_pool(pool)
-        expect(result).to_not be_nil
-        expect(result).to eq('m.example.com')
-      end
-    end
-  end
-
   describe '#check_disk_queue' do
     let(:threads) {[]}
 

spec/unit/util/parsing_spec.rb

@@ -0,0 +1,104 @@
+require 'spec_helper'
+
+describe 'Parser' do
+  let(:pool) { 'pool1' }
+  subject { Vmpooler::Parsing }
+  describe '.get_domain_for_pool' do
+    let(:provider_name) { 'mock_provider' }
+    context 'No provider is set' do
+      let(:config) { YAML.load(<<~EOT
+          ---
+          :config:
+          :providers:
+            :mock_provider:
+          :pools:
+            - name: '#{pool}'
+              size: 1
+      EOT
+      )}
+
+      it 'should return nil' do
+        result = subject.get_domain_for_pool(config, pool)
+        expect(result).to be_nil
+      end
+    end
+    context 'Provider is vsphere by default' do
+      let(:config) { YAML.load(<<~EOT
+          ---
+          :config:
+          :providers:
+            :vsphere:
+              domain: myown.com
+          :pools:
+            - name: '#{pool}'
+              size: 1
+      EOT
+      )}
+
+      it 'should return the domain set for vsphere' do
+        result = subject.get_domain_for_pool(config, pool)
+        expect(result).to eq('myown.com')
+      end
+    end
+    context 'No domain is set' do
+      let(:config) { YAML.load(<<~EOT
+          ---
+          :config:
+          :providers:
+            :mock_provider:
+          :pools:
+            - name: '#{pool}'
+              size: 1
+              provider: #{provider_name}
+      EOT
+      )}
+
+      it 'should return nil' do
+        result = subject.get_domain_for_pool(config, pool)
+        expect(result).to be_nil
+      end
+    end
+
+    context 'Only a global domain is set' do
+      let(:config) { YAML.load(<<~EOT
+          ---
+          :config:
+            domain: example.com
+          :providers:
+            :mock_provider:
+          :pools:
+            - name: '#{pool}'
+              size: 1
+              provider: #{provider_name}
+      EOT
+      )}
+
+      it 'should return the domain set in the config section' do
+        result = subject.get_domain_for_pool(config, pool)
+        expect(result).to_not be_nil
+        expect(result).to eq('example.com')
+      end
+    end
+
+    context 'A provider specified a domain to use' do
+      let(:config) { YAML.load(<<~EOT
+          ---
+          :config:
+          :providers:
+            :mock_provider:
+              domain: m.example.com
+          :pools:
+            - name: '#{pool}'
+              size: 1
+              provider: #{provider_name}
+      EOT
+      )}
+
+      it 'should return the domain set in the config section' do
+        result = subject.get_domain_for_pool(config, pool)
+        expect(result).to_not be_nil
+        expect(result).to eq('m.example.com')
+      end
+    end
+  end
+end