(POOLER-70) Update create_vm_disk for VM Provider

Previously the Pool Manager would use vSphere objects directly.  This commit
- Modifies the pool_manager to use the VM provider methods instead

lib/vmpooler/pool_manager.rb

@@ -255,44 +255,40 @@ module Vmpooler
       $metrics.timing("destroy.#{pool}", finish)
     end
 
-    def create_vm_disk(vm, disk_size, provider)
+    def create_vm_disk(pool_name, vm, disk_size, provider)
       Thread.new do
-        _create_vm_disk(vm, disk_size, provider)
+        begin
+          _create_vm_disk(pool_name, vm, disk_size, provider)
+        rescue => err
+          $logger.log('d', "[!] [#{pool_name}] '#{vm}' failed while creating disk: #{err}")
+          raise
+        end
       end
     end
 
-    def _create_vm_disk(vm, disk_size, provider)
+    def _create_vm_disk(pool_name, vm_name, disk_size, provider)
-      host = provider.find_vm(vm)
+      raise("Invalid disk size of '#{disk_size}' passed") if (disk_size.nil?) || (disk_size.empty?) || (disk_size.to_i <= 0)
 
-      if (host) && ((! disk_size.nil?) && (! disk_size.empty?) && (disk_size.to_i > 0))
+      $logger.log('s', "[ ] [disk_manager] '#{vm_name}' is attaching a #{disk_size}gb disk")
-        $logger.log('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
 
       start = Time.now
 
-        template = $redis.hget('vmpooler__vm__' + vm, 'template')
+      result = provider.create_disk(pool_name, vm_name, disk_size.to_i)
-        datastore = nil
-
-        $config[:pools].each do |pool|
-          if pool['name'] == template
-            datastore = pool['datastore']
-          end
-        end
-
-        if ((! datastore.nil?) && (! datastore.empty?))
-          provider.add_disk(host, disk_size, datastore)
-
-          rdisks = $redis.hget('vmpooler__vm__' + vm, 'disk')
-          disks = rdisks ? rdisks.split(':') : []
-          disks.push("+#{disk_size}gb")
-          $redis.hset('vmpooler__vm__' + vm, 'disk', disks.join(':'))
 
       finish = '%.2f' % (Time.now - start)
 
-          $logger.log('s', "[+] [disk_manager] '#{vm}' attached #{disk_size}gb disk in #{finish} seconds")
+      if result
+        rdisks = $redis.hget('vmpooler__vm__' + vm_name, 'disk')
+        disks = rdisks ? rdisks.split(':') : []
+        disks.push("+#{disk_size}gb")
+        $redis.hset('vmpooler__vm__' + vm_name, 'disk', disks.join(':'))
+
+        $logger.log('s', "[+] [disk_manager] '#{vm_name}' attached #{disk_size}gb disk in #{finish} seconds")
       else
-          $logger.log('s', "[+] [disk_manager] '#{vm}' failed to attach disk")
+        $logger.log('s', "[+] [disk_manager] '#{vm_name}' failed to attach disk")
-        end
       end
+
+      result
     end
 
     def create_vm_snapshot(vm, snapshot_name, provider)

spec/unit/pool_manager_spec.rb

@@ -730,107 +730,98 @@ EOT
 
     it 'calls _create_vm_disk' do
       expect(Thread).to receive(:new).and_yield
-      expect(subject).to receive(:_create_vm_disk).with(vm, disk_size, provider)
+      expect(subject).to receive(:_create_vm_disk).with(pool, vm, disk_size, provider)
 
-      subject.create_vm_disk(vm, disk_size, provider)
+      subject.create_vm_disk(pool, vm, disk_size, provider)
     end
   end
 
   describe "#_create_vm_disk" do
-    let(:provider) { double('provider') }
     let(:disk_size) { '15' }
-    let(:datastore) { 'datastore0'}
-    let(:config) {
-      YAML.load(<<-EOT
----
-:pools:
-  - name: #{pool}
-    datastore: '#{datastore}'
-EOT
-      )
-    }
-
-    before do
-      expect(subject).not_to be_nil
-    end
 
     before(:each) do
-      allow(provider).to receive(:find_vm).with(vm).and_return(host)
+      expect(subject).not_to be_nil
+      allow(logger).to receive(:log)
+
       create_running_vm(pool,vm,token)
     end
 
-    it 'should not do anything if the VM does not exist' do
+    context 'Given a VM that does not exist' do
-      expect(provider).to receive(:find_vm).with(vm).and_return(nil)
+      before(:each) do
-      expect(logger).to receive(:log).exactly(0).times
+        # As per base_spec, create_disk will raise if the VM does not exist
-      subject._create_vm_disk(vm, disk_size, provider)
+        expect(provider).to receive(:create_disk).with(pool,vm,disk_size.to_i).and_raise("VM #{vm} does not exist")
       end
 
-    it 'should not do anything if the disk size is nil' do
+      it 'should not update redis if the VM does not exist' do
-      expect(logger).to receive(:log).exactly(0).times
+        expect(redis).to receive(:hset).exactly(0).times
-      subject._create_vm_disk(vm, nil, provider)
+        expect{ subject._create_vm_disk(pool, vm, disk_size, provider) }.to raise_error(RuntimeError)
+      end
     end
 
-    it 'should not do anything if the disk size is empty string' do
+    context 'Given an invalid disk size' do
-      expect(logger).to receive(:log).exactly(0).times
+      [{ :description => 'is nil',                            :value => nil },
-      subject._create_vm_disk(vm, '', provider)
+       { :description => 'is an empty string',                :value => '' },
+       { :description => 'is less than 1',                    :value => '0' },
+       { :description => 'cannot be converted to an integer', :value => 'abc123' },
+      ].each do |testcase|
+        it "should not attempt the create the disk if the disk size #{testcase[:description]}" do
+          expect(provider).to receive(:create_disk).exactly(0).times
+          expect{ subject._create_vm_disk(pool, vm, testcase[:value], provider) }.to raise_error(/Invalid disk size/)
         end
-
-    it 'should not do anything if the disk size is less than 1' do
-      expect(logger).to receive(:log).exactly(0).times
-      subject._create_vm_disk(vm, '0', provider)
-    end
-
-    it 'should not do anything if the disk size cannot be converted to an integer' do
-      expect(logger).to receive(:log).exactly(0).times
-      subject._create_vm_disk(vm, 'abc123', provider)
       end
 
       it 'should raise an error if the disk size is a Fixnum' do
-      expect(logger).to receive(:log).exactly(0).times
+        expect(redis).to receive(:hset).exactly(0).times
-      expect{ subject._create_vm_disk(vm, 10, provider) }.to raise_error(NoMethodError,/empty?/)
+        expect{ subject._create_vm_disk(pool, vm, 10, provider) }.to raise_error(NoMethodError,/empty?/)
+      end
     end
 
-    it 'should not do anything if the datastore for pool is nil' do
+    context 'Given a successful disk creation' do
-      expect(logger).to receive(:log).with('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
+      before(:each) do
-      expect(logger).to receive(:log).with('s', "[+] [disk_manager] '#{vm}' failed to attach disk")
+        expect(provider).to receive(:create_disk).with(pool,vm,disk_size.to_i).and_return(true)
-      config[:pools][0]['datastore'] = nil
-
-      subject._create_vm_disk(vm, disk_size, provider)
       end
 
-    it 'should not do anything if the datastore for pool is empty' do
+      it 'should log a message' do
-      expect(logger).to receive(:log).with('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
-      expect(logger).to receive(:log).with('s', "[+] [disk_manager] '#{vm}' failed to attach disk")
-      config[:pools][0]['datastore'] = ''
-
-      subject._create_vm_disk(vm, disk_size, provider)
-    end
-
-    it 'should attach the disk' do
         expect(logger).to receive(:log).with('s', "[ ] [disk_manager] '#{vm}' is attaching a #{disk_size}gb disk")
         expect(logger).to receive(:log).with('s', /\[\+\] \[disk_manager\] '#{vm}' attached #{disk_size}gb disk in 0.[\d]+ seconds/)
-      expect(provider).to receive(:add_disk).with(host,disk_size,datastore)
 
-      subject._create_vm_disk(vm, disk_size, provider)
+        subject._create_vm_disk(pool, vm, disk_size, provider)
       end
 
       it 'should update redis information when attaching the first disk' do
-      expect(provider).to receive(:add_disk).with(host,disk_size,datastore)
+        subject._create_vm_disk(pool, vm, disk_size, provider)
-
-      subject._create_vm_disk(vm, disk_size, provider)
         expect(redis.hget("vmpooler__vm__#{vm}", 'disk')).to eq("+#{disk_size}gb")
       end
 
       it 'should update redis information when attaching the additional disks' do
-      expect(provider).to receive(:add_disk).with(host,disk_size,datastore)
         initial_disks = '+10gb:+20gb'
         redis.hset("vmpooler__vm__#{vm}", 'disk', initial_disks)
 
-      subject._create_vm_disk(vm, disk_size, provider)
+        subject._create_vm_disk(pool, vm, disk_size, provider)
         expect(redis.hget("vmpooler__vm__#{vm}", 'disk')).to eq("#{initial_disks}:+#{disk_size}gb")
       end
     end
 
+    context 'Given a failed disk creation' do
+      before(:each) do
+        expect(provider).to receive(:create_disk).with(pool,vm,disk_size.to_i).and_return(false)
+      end
+
+      it 'should not update redis information' do
+        expect(redis).to receive(:hset).exactly(0).times
+
+        subject._create_vm_disk(pool, vm, disk_size, provider)
+        expect(redis.hget("vmpooler__vm__#{vm}", 'disk')).to be_nil
+      end
+
+      it 'should log a message' do
+        expect(logger).to receive(:log).with('s', "[+] [disk_manager] '#{vm}' failed to attach disk")
+
+        subject._create_vm_disk(pool, vm, disk_size, provider)
+      end
+    end
+  end
+
   describe '#create_vm_snapshot' do
     let(:provider) { double('provider') }
     let(:snapshot_name) { 'snapshot' }