From 30bf2074fe410d18baaf2927f17e6fd886d0ba4d Mon Sep 17 00:00:00 2001
From: "kirby@puppetlabs.com" <mk.kirby@gmail.com>
Date: Mon, 21 Oct 2019 14:24:34 -0700
Subject: [PATCH] (POOLER-150) Synchronize checkout operations for API

This commit adds a shared mutex to vmpooler API so that checkout requests can be synchronized across threads. Without this change it is possible in some scenarios for vmpooler to allocate the same SUT to different API requests for a VM.
---
 CHANGELOG.md                                |  1 +
 bin/vmpooler                                |  3 ++-
 lib/vmpooler/api.rb                         |  3 ++-
 lib/vmpooler/api/v1.rb                      | 30 ++++++++++++---------
 spec/integration/api/v1/vm_spec.rb          |  2 ++
 spec/integration/api/v1/vm_template_spec.rb |  2 ++
 6 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index ef80643..3f7f5da 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -14,6 +14,7 @@ git logs & PR history.
 
 ### Fixed
 - Correctly detect create\_linked\_clone on a pool level (POOLER-147)
+- Ensure that checkout operations are synchronized
 
 # [0.7.0](https://github.com/puppetlabs/vmpooler/compare/0.6.3...0.7.0)
 
diff --git a/bin/vmpooler b/bin/vmpooler
index b84a139..29a4e4e 100755
--- a/bin/vmpooler
+++ b/bin/vmpooler
@@ -24,7 +24,8 @@ if torun.include? 'api'
   api = Thread.new do
     thr = Vmpooler::API.new
     redis = Vmpooler.new_redis(redis_host, redis_port, redis_password)
-    thr.helpers.configure(config, redis, metrics)
+    checkoutlock = Mutex.new
+    thr.helpers.configure(config, redis, metrics, checkoutlock)
     thr.helpers.execute!
   end
   torun_threads << api
diff --git a/lib/vmpooler/api.rb b/lib/vmpooler/api.rb
index 239155b..0b1fbe6 100644
--- a/lib/vmpooler/api.rb
+++ b/lib/vmpooler/api.rb
@@ -36,10 +36,11 @@ module Vmpooler
     use Vmpooler::API::Reroute
     use Vmpooler::API::V1
 
-    def configure(config, redis, metrics)
+    def configure(config, redis, metrics, checkoutlock)
       self.settings.set :config, config
       self.settings.set :redis, redis
       self.settings.set :metrics, metrics
+      self.settings.set :checkoutlock, checkoutlock
     end
 
     def execute!
diff --git a/lib/vmpooler/api/v1.rb b/lib/vmpooler/api/v1.rb
index 4b84e3e..1f63fc7 100644
--- a/lib/vmpooler/api/v1.rb
+++ b/lib/vmpooler/api/v1.rb
@@ -36,6 +36,10 @@ module Vmpooler
       validate_token(backend)
     end
 
+    def checkoutlock
+      Vmpooler::API::settings.checkoutlock
+    end
+
     def fetch_single_vm(template)
       template_backends = [template]
       aliases = Vmpooler::API.settings.config[:alias]
@@ -67,21 +71,23 @@ module Vmpooler
         end
       end
 
-      template_backends.each do |template_backend|
-        vms = backend.smembers("vmpooler__ready__#{template_backend}")
-        next if vms.empty?
-        vms.reverse.each do |vm|
-          ready = vm_ready?(vm, config['domain'])
-          if ready
-            backend.smove("vmpooler__ready__#{template_backend}", "vmpooler__running__#{template_backend}", vm)
-            return [vm, template_backend, template]
-          else
-            backend.smove("vmpooler__ready__#{template_backend}", "vmpooler__completed__#{template_backend}", vm)
-            metrics.increment("checkout.nonresponsive.#{template_backend}")
+      checkoutlock.synchronize do
+        template_backends.each do |template_backend|
+          vms = backend.smembers("vmpooler__ready__#{template_backend}")
+          next if vms.empty?
+          vms.reverse.each do |vm|
+            ready = vm_ready?(vm, config['domain'])
+            if ready
+              backend.smove("vmpooler__ready__#{template_backend}", "vmpooler__running__#{template_backend}", vm)
+              return [vm, template_backend, template]
+            else
+              backend.smove("vmpooler__ready__#{template_backend}", "vmpooler__completed__#{template_backend}", vm)
+              metrics.increment("checkout.nonresponsive.#{template_backend}")
+            end
           end
         end
+        [nil, nil, nil]
       end
-      [nil, nil, nil]
     end
 
     def return_vm_to_ready_state(template, vm)
diff --git a/spec/integration/api/v1/vm_spec.rb b/spec/integration/api/v1/vm_spec.rb
index 9bbe08a..78be432 100644
--- a/spec/integration/api/v1/vm_spec.rb
+++ b/spec/integration/api/v1/vm_spec.rb
@@ -29,12 +29,14 @@ describe Vmpooler::API::V1 do
     }
     let(:current_time) { Time.now }
     let(:vmname) { 'abcdefghijkl' }
+    let(:checkoutlock) { Mutex.new }
 
     before(:each) do
       app.settings.set :config, config
       app.settings.set :redis, redis
       app.settings.set :metrics, metrics
       app.settings.set :config, auth: false
+      app.settings.set :checkoutlock, checkoutlock
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end
 
diff --git a/spec/integration/api/v1/vm_template_spec.rb b/spec/integration/api/v1/vm_template_spec.rb
index d94b537..9620f1f 100644
--- a/spec/integration/api/v1/vm_template_spec.rb
+++ b/spec/integration/api/v1/vm_template_spec.rb
@@ -29,12 +29,14 @@ describe Vmpooler::API::V1 do
 
     let(:current_time) { Time.now }
     let(:socket) { double('socket') }
+    let(:checkoutlock) { Mutex.new }
 
     before(:each) do
       app.settings.set :config, config
       app.settings.set :redis, redis
       app.settings.set :metrics, metrics
       app.settings.set :config, auth: false
+      app.settings.set :checkoutlock, checkoutlock
       create_token('abcdefghijklmnopqrstuvwxyz012345', 'jdoe', current_time)
     end