Merge pull request #62 from puppetlabs/P4DEVOPS-9438-circuit-breaker

(P4DEVOPS-9438) Wire circuit breaker into vSphere provider with connection timeouts

.github/workflows/testing.yml

@@ -18,7 +18,7 @@ jobs:
     strategy:
       matrix:
         ruby-version:
-          - 'jruby-9.4.3.0'
+          - 'jruby-9.4.14.0'
     steps:
     - uses: actions/checkout@v4
     - name: Set up Ruby
@@ -34,7 +34,7 @@ jobs:
     strategy:
       matrix:
         ruby-version:
-          - 'jruby-9.4.3.0'
+          - 'jruby-9.4.14.0'
     steps:
     - uses: actions/checkout@v4
     - name: Set up Ruby

Gemfile.lock

@@ -9,37 +9,47 @@ GEM
   remote: https://rubygems.org/
   specs:
     ast (2.4.2)
-    bindata (2.4.15)
+    base64 (0.3.0)
+    bindata (2.5.1)
     builder (3.2.4)
     climate_control (1.2.0)
     coderay (1.1.3)
-    concurrent-ruby (1.2.2)
+    concurrent-ruby (1.3.6)
-    connection_pool (2.4.1)
+    connection_pool (2.5.5)
     deep_merge (1.2.2)
     diff-lcs (1.5.0)
     docile (1.4.0)
-    faraday (2.7.10)
+    faraday (2.14.1)
-      faraday-net_http (>= 2.0, < 3.1)
+      faraday-net_http (>= 2.0, < 3.5)
-      ruby2_keywords (>= 0.0.4)
+      json
-    faraday-net_http (3.0.2)
+      logger
+    faraday-net_http (3.4.2)
+      net-http (~> 0.5)
     ffi (1.15.5-java)
-    google-cloud-env (1.6.0)
+    google-cloud-env (2.3.1)
-      faraday (>= 0.17.3, < 3.0)
+      base64 (~> 0.2)
+      faraday (>= 1.0, < 3.a)
     json (2.6.3)
     json (2.6.3-java)
+    logger (1.7.0)
     method_source (1.0.0)
     mock_redis (0.37.0)
-    mustermann (3.0.0)
+    mustermann (3.0.4)
       ruby2_keywords (~> 0.0.1)
-    net-ldap (0.18.0)
+    net-http (0.9.1)
-    nio4r (2.5.9)
+      uri (>= 0.11.1)
-    nio4r (2.5.9-java)
+    net-ldap (0.20.0)
+      base64
+      ostruct
+    nio4r (2.7.5)
+    nio4r (2.7.5-java)
     nokogiri (1.15.4-java)
       racc (~> 1.4)
     nokogiri (1.15.4-x86_64-linux)
       racc (~> 1.4)
-    opentelemetry-api (1.2.2)
+    opentelemetry-api (1.8.0)
-    opentelemetry-common (0.20.0)
+      logger
+    opentelemetry-common (0.20.1)
       opentelemetry-api (~> 1.0)
     opentelemetry-exporter-jaeger (0.23.0)
       opentelemetry-api (~> 1.1)
@@ -47,7 +57,7 @@ GEM
       opentelemetry-sdk (~> 1.2)
       opentelemetry-semantic_conventions
       thrift
-    opentelemetry-instrumentation-base (0.22.2)
+    opentelemetry-instrumentation-base (0.22.3)
       opentelemetry-api (~> 1.0)
       opentelemetry-registry (~> 0.1)
     opentelemetry-instrumentation-concurrent_ruby (0.21.1)
@@ -70,25 +80,27 @@ GEM
       opentelemetry-common (~> 0.20.0)
       opentelemetry-instrumentation-base (~> 0.22.1)
       opentelemetry-instrumentation-rack (~> 0.21)
-    opentelemetry-registry (0.3.0)
+    opentelemetry-registry (0.4.0)
       opentelemetry-api (~> 1.1)
-    opentelemetry-resource_detectors (0.24.1)
+    opentelemetry-resource_detectors (0.24.2)
       google-cloud-env
       opentelemetry-sdk (~> 1.0)
-    opentelemetry-sdk (1.3.0)
+    opentelemetry-sdk (1.10.0)
       opentelemetry-api (~> 1.1)
       opentelemetry-common (~> 0.20)
       opentelemetry-registry (~> 0.2)
       opentelemetry-semantic_conventions
-    opentelemetry-semantic_conventions (1.10.0)
+    opentelemetry-semantic_conventions (1.36.0)
       opentelemetry-api (~> 1.0)
     optimist (3.1.0)
+    ostruct (0.6.3)
     parallel (1.23.0)
     parser (3.2.2.3)
       ast (~> 2.4.1)
       racc
     pickup (0.0.11)
-    prometheus-client (4.2.1)
+    prometheus-client (4.2.5)
+      base64
     pry (0.14.2)
       coderay (~> 1.1)
       method_source (~> 1.0)
@@ -96,27 +108,28 @@ GEM
       coderay (~> 1.1)
       method_source (~> 1.0)
       spoon (~> 0.0)
-    puma (6.3.1)
+    puma (6.6.1)
       nio4r (~> 2.0)
-    puma (6.3.1-java)
+    puma (6.6.1-java)
       nio4r (~> 2.0)
     racc (1.7.1)
     racc (1.7.1-java)
-    rack (2.2.8)
+    rack (2.2.22)
-    rack-protection (3.1.0)
+    rack-protection (3.2.0)
+      base64 (>= 0.1.0)
       rack (~> 2.2, >= 2.2.4)
     rack-test (2.1.0)
       rack (>= 1.3)
     rainbow (3.1.1)
-    rake (13.0.6)
+    rake (13.3.1)
     rbvmomi2 (3.6.1)
       builder (~> 3.2)
       json (~> 2.3)
       nokogiri (~> 1.12, >= 1.12.5)
       optimist (~> 3.0)
-    redis (5.0.7)
+    redis (5.4.1)
-      redis-client (>= 0.9.0)
+      redis-client (>= 0.22.0)
-    redis-client (0.16.0)
+    redis-client (0.27.0)
       connection_pool
     regexp_parser (2.8.1)
     rexml (3.2.6)
@@ -152,10 +165,10 @@ GEM
       simplecov_json_formatter (~> 0.1)
     simplecov-html (0.12.3)
     simplecov_json_formatter (0.1.4)
-    sinatra (3.1.0)
+    sinatra (3.2.0)
       mustermann (~> 3.0)
       rack (~> 2.2, >= 2.2.4)
-      rack-protection (= 3.1.0)
+      rack-protection (= 3.2.0)
       tilt (~> 2.0)
     spicy-proton (2.1.15)
       bindata (~> 2.3)
@@ -163,10 +176,11 @@ GEM
       ffi
     statsd-ruby (1.5.0)
     thor (1.2.2)
-    thrift (0.18.1)
+    thrift (0.22.0)
-    tilt (2.2.0)
+    tilt (2.7.0)
     unicode-display_width (2.4.2)
-    vmpooler (3.5.1)
+    uri (1.1.1)
+    vmpooler (3.9.0)
       concurrent-ruby (~> 1.1)
       connection_pool (~> 2.4)
       deep_merge (~> 1.2)
@@ -174,10 +188,11 @@ GEM
       opentelemetry-exporter-jaeger (= 0.23.0)
       opentelemetry-instrumentation-concurrent_ruby (= 0.21.1)
       opentelemetry-instrumentation-http_client (= 0.22.2)
+      opentelemetry-instrumentation-rack (= 0.23.4)
       opentelemetry-instrumentation-redis (= 0.25.3)
       opentelemetry-instrumentation-sinatra (= 0.23.2)
-      opentelemetry-resource_detectors (= 0.24.1)
+      opentelemetry-resource_detectors (= 0.24.2)
-      opentelemetry-sdk (~> 1.3, >= 1.3.0)
+      opentelemetry-sdk (~> 1.8)
       pickup (~> 0.0.11)
       prometheus-client (>= 2, < 5)
       puma (>= 5.0.4, < 7)
@@ -193,6 +208,7 @@ GEM
 
 PLATFORMS
   universal-java-11
+  universal-java-17
   x86_64-linux
 
 DEPENDENCIES
@@ -208,4 +224,4 @@ DEPENDENCIES
   yarjuf (>= 2.0)
 
 BUNDLED WITH
-   2.4.10
+   2.6.9

lib/vmpooler/providers/vsphere.rb

@@ -3,6 +3,7 @@
 require 'bigdecimal'
 require 'bigdecimal/util'
 require 'rbvmomi'
+require 'timeout'
 require 'vmpooler/providers/base'
 
 module Vmpooler
@@ -74,7 +75,7 @@ module Vmpooler
               transaction.hset("vmpooler__vm__#{vm_name}", 'destroy', Time.now.to_s)
 
               # Auto-expire metadata key
-              transaction.expire("vmpooler__vm__#{vm_name}", (data_ttl * 60 * 60))
+              transaction.expire("vmpooler__vm__#{vm_name}", data_ttl * 60 * 60)
             end
           end
 
@@ -186,16 +187,20 @@ module Vmpooler
 
         def vms_in_pool(pool_name)
           vms = []
+          with_circuit_breaker do
+            Timeout.timeout(vsphere_connection_timeout) do
               @connection_pool.with_metrics do |pool_object|
                 connection = ensured_vsphere_connection(pool_object)
                 folder_object = find_vm_folder(pool_name, connection)
 
-            return vms if folder_object.nil?
+                next if folder_object.nil?
 
                 folder_object.childEntity.each do |vm|
                   vms << { 'name' => vm.name } if vm.is_a? RbVmomi::VIM::VirtualMachine
                 end
               end
+            end
+          end
           vms
         end
 
@@ -305,13 +310,17 @@ module Vmpooler
 
         def get_vm(pool_name, vm_name)
           vm_hash = nil
+          with_circuit_breaker do
+            Timeout.timeout(vsphere_connection_timeout) do
               @connection_pool.with_metrics do |pool_object|
                 connection = ensured_vsphere_connection(pool_object)
                 vm_object = find_vm(pool_name, vm_name, connection)
-            return vm_hash if vm_object.nil?
+                next if vm_object.nil?
 
                 vm_hash = generate_vm_hash(vm_object, pool_name)
               end
+            end
+          end
           vm_hash
         end
 
@@ -320,6 +329,7 @@ module Vmpooler
           raise("Pool #{pool_name} does not exist for the provider #{name}") if pool.nil?
 
           vm_hash = nil
+          with_circuit_breaker do
             @connection_pool.with_metrics do |pool_object|
               connection = ensured_vsphere_connection(pool_object)
               # Assume all pool config is valid i.e. not missing
@@ -389,18 +399,25 @@ module Vmpooler
 
               vm_hash = generate_vm_hash(new_vm_object, pool_name)
             end
+          end
           vm_hash
         end
 
         # The inner method requires vmware tools running in the guest os
         def get_vm_ip_address(vm_name, pool_name)
+          ip = nil
+          with_circuit_breaker do
+            Timeout.timeout(vsphere_connection_timeout) do
               @connection_pool.with_metrics do |pool_object|
                 connection = ensured_vsphere_connection(pool_object)
                 vm_object = find_vm(pool_name, vm_name, connection)
                 vm_hash = generate_vm_hash(vm_object, pool_name)
-            return vm_hash['ip']
+                ip = vm_hash['ip']
               end
             end
+          end
+          ip
+        end
 
         def create_config_spec(vm_name, template_name, extra_config)
           RbVmomi::VIM.VirtualMachineConfigSpec(
@@ -540,11 +557,12 @@ module Vmpooler
         end
 
         def destroy_vm(pool_name, vm_name)
+          with_circuit_breaker do
             @connection_pool.with_metrics do |pool_object|
               connection = ensured_vsphere_connection(pool_object)
               vm_object = find_vm(pool_name, vm_name, connection)
               # If a VM doesn't exist then it is effectively deleted
-            return true if vm_object.nil?
+              next if vm_object.nil?
 
               # Poweroff the VM if it's running
               vm_object.PowerOffVM_Task.wait_for_completion if vm_object.runtime&.powerState && vm_object.runtime.powerState == 'poweredOn'
@@ -552,6 +570,7 @@ module Vmpooler
               # Kill it with fire
               vm_object.Destroy_Task.wait_for_completion
             end
+          end
           true
         end
 
@@ -595,7 +614,7 @@ module Vmpooler
           pool_configuration = pool_config(pool_name)
           return nil if pool_configuration.nil?
 
-          hostname = vm_object.summary.guest.hostName if vm_object.summary&.guest && vm_object.summary.guest.hostName
+          hostname = vm_object.summary.guest.hostName if vm_object.summary&.guest&.hostName
           boottime = vm_object.runtime.bootTime if vm_object.runtime&.bootTime
           powerstate = vm_object.runtime.powerState if vm_object.runtime&.powerState
 
@@ -631,6 +650,14 @@ module Vmpooler
         DISK_TYPE = 'thin'
         DISK_MODE = 'persistent'
 
+        def with_circuit_breaker(&block)
+          if circuit_breaker
+            circuit_breaker.call(&block)
+          else
+            yield
+          end
+        end
+
         def ensured_vsphere_connection(connection_pool_object)
           connection_pool_object[:connection] = connect_to_vsphere unless vsphere_connection_ok?(connection_pool_object[:connection])
           connection_pool_object[:connection]
@@ -651,7 +678,9 @@ module Vmpooler
             connection = RbVmomi::VIM.connect host: provider_config['server'],
                                               user: provider_config['username'],
                                               password: provider_config['password'],
-                                              insecure: provider_config['insecure'] || false
+                                              insecure: provider_config['insecure'] || false,
+                                              read_timeout: vsphere_connection_timeout,
+                                              open_timeout: vsphere_connection_timeout
             metrics.increment('connect.open')
             connection
           rescue StandardError => e
@@ -664,6 +693,12 @@ module Vmpooler
           end
         end
 
+        def vsphere_connection_timeout
+          timeout = provider_config['vsphere_timeout'] ||
+                    global_config&.dig(:config, 'vsphere_timeout') || 60
+          timeout.to_i
+        end
+
         # This should supercede the open_socket method in the Pool Manager
         def open_socket(host, domain = nil, timeout = 5, port = 22, &_block)
           target_host = host
@@ -697,7 +732,7 @@ module Vmpooler
 
           # Reverse the array back to normal and
           # then convert the array of paths into a '/' seperated string
-          (full_path.reverse.map { |p| p[1] }).join('/')
+          full_path.reverse.map { |p| p[1] }.join('/')
         end
 
         def add_disk(vm, size, datastore, connection, datacentername)
@@ -1078,7 +1113,7 @@ module Vmpooler
           vm_object = find_vm(pool_name, vm_name, connection)
           return nil if vm_object.nil?
 
-          parent_host_object = vm_object.summary.runtime.host if vm_object.summary&.runtime && vm_object.summary.runtime.host
+          parent_host_object = vm_object.summary.runtime.host if vm_object.summary&.runtime&.host
           raise('Unable to determine which host the VM is running on') if parent_host_object.nil?
 
           parent_host = parent_host_object.name
@@ -1223,7 +1258,8 @@ module Vmpooler
         def linked_clone?(pool)
           return if pool['create_linked_clone'] == false
           return true if pool['create_linked_clone']
-          return true if @config[:config]['create_linked_clones']
+
+          true if @config[:config]['create_linked_clones']
         end
       end
     end

spec/unit/providers/vsphere_spec.rb

@@ -1337,7 +1337,9 @@ EOT
           :host         => credentials['server'],
           :user         => credentials['username'],
           :password     => credentials['password'],
-          :insecure => credentials['insecure']
+          :insecure     => credentials['insecure'],
+          :read_timeout => 60,
+          :open_timeout => 60
         }).and_return(connection)
         subject.connect_to_vsphere
       end
@@ -1350,6 +1352,8 @@ EOT
           :user         => credentials['username'],
           :password     => credentials['password'],
           :insecure     => true,
+          :read_timeout => 60,
+          :open_timeout => 60
         }).and_return(connection)
         subject.connect_to_vsphere
       end
@@ -1361,7 +1365,9 @@ EOT
           :host         => credentials['server'],
           :user         => credentials['username'],
           :password     => credentials['password'],
-          :insecure => true
+          :insecure     => true,
+          :read_timeout => 60,
+          :open_timeout => 60
         }).and_return(connection)
 
         subject.connect_to_vsphere