From: Tom Clegg <tom@curoverse.com>
Date: Wed, 19 Apr 2017 14:44:21 +0000 (-0400)
Subject: 11283: Fix "available slot number" query.
X-Git-Tag: 1.1.0~190^2~2
X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/050be6a5be43ab503820955dbca2751ca368063c

11283: Fix "available slot number" query.

Fixes repetitive queries and excessive Postgres and Rails log
messages:

2017-04-10 16:39:09 UTC [4734-1] arvados@arvados_test ERROR: duplicate key value violates unique constraint "index_nodes_on_slot_number"

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_nodes_on_slot_number"

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tom@curoverse.com>
---

diff --git a/services/api/app/models/node.rb b/services/api/app/models/node.rb
index 82ea0acbd6..f04fa70c1b 100644
--- a/services/api/app/models/node.rb
+++ b/services/api/app/models/node.rb
@@ -104,17 +104,19 @@ class Node < ArvadosModel
 
     # Assign slot_number
     if self.slot_number.nil?
-      try_slot = 1
-      begin
-        self.slot_number = try_slot
+      while true
+        n = self.class.available_slot_number
+        if n.nil?
+          raise "No available node slots"
+        end
+        self.slot_number = n
         begin
           self.save!
           break
         rescue ActiveRecord::RecordNotUnique
-          try_slot += 1
+          # try again
         end
-        raise "No available node slots" if try_slot == Rails.configuration.max_compute_nodes
-      end while true
+      end
     end
 
     # Assign hostname
@@ -136,6 +138,19 @@ class Node < ArvadosModel
 
   protected
 
+  def self.available_slot_number
+    connection.exec_query('SELECT n FROM generate_series(1, $1) AS slot(n)
+                          LEFT JOIN nodes ON n=slot_number
+                          WHERE slot_number IS NULL
+                          LIMIT 1',
+                          # query label:
+                          'Node.available_slot_number',
+                          # [col_id, val] for $1 vars:
+                          [['max_compute_nodes',
+                            Rails.configuration.max_compute_nodes]],
+                         ).rows.first.andand.first
+  end
+
   def ensure_ping_secret
     self.info['ping_secret'] ||= rand(2**256).to_s(36)
   end
diff --git a/services/api/test/unit/node_test.rb b/services/api/test/unit/node_test.rb
index 2330e7c528..e3bd753c33 100644
--- a/services/api/test/unit/node_test.rb
+++ b/services/api/test/unit/node_test.rb
@@ -128,9 +128,8 @@ class NodeTest < ActiveSupport::TestCase
   test "ping two nodes one with no hostname and one with hostname and check hostnames" do
     # ping node with no hostname and expect it set with config format
     node = ping_node(:new_with_no_hostname, {})
-    slot_number = node.slot_number
     refute_nil node.slot_number
-    assert_equal "compute#{slot_number}", node.hostname
+    assert_equal "compute#{node.slot_number}", node.hostname
 
     # ping node with a hostname and expect it to be unchanged
     node2 = ping_node(:new_with_custom_hostname, {})
@@ -191,4 +190,22 @@ class NodeTest < ActiveSupport::TestCase
       assert_equal '10.5.5.5', n1.ip_address
     end
   end
+
+  test 'run out of slots' do
+    Rails.configuration.max_compute_nodes = 3
+    act_as_system_user do
+      Node.destroy_all
+      (1..4).each do |i|
+        n = Node.create!
+        args = { ip: "10.0.0.#{i}", ping_secret: n.info['ping_secret'] }
+        if i <= Rails.configuration.max_compute_nodes
+          n.ping(args)
+        else
+          assert_raises do
+            n.ping(args)
+          end
+        end
+      end
+    end
+  end
 end