Don't add non-existent hosts to host aggregates

nova will throw an error if you attempt to add a node to a host
aggergate that does not "exist" in terms of nova. Therefore
depending on the order in which your nodes come up, you can have puppet
attempting to add a node to a host aggregate when the node does not yet
have nova compute installed and "registered". This is especially true if
you manage your host list with a hiera list or other mechanism that is
not directly tied to nova-compute being installed. Instead of blowing up
with an error, we will instead print a warning and let eventual
consistency solve it as your nodes are installed.

Since it is inefficient to call nova host-list once for every member of
a host aggregate we also cache this information and similarly cache the
output of nova aggegrate-details which previously was also called
once for every member of every aggegrate.

Change-Id: I6249cd070ad804c626d880decb6767e2ff14dcbd

lib/puppet/provider/nova.rb

@@ -158,22 +158,46 @@ class Puppet::Provider::Nova < Puppet::Provider
     return hash_list
   end
 
-  def self.nova_aggregate_resources_ids
+  def self.nova_hosts
+    return @nova_hosts if @nova_hosts
+    cmd_output = auth_nova("host-list")
+    @nova_hosts = cliout2list(cmd_output)
+    @nova_hosts
+  end
+
+  def self.nova_get_host_by_name_and_type(host_name, service_type)
+    #find the host by name and service type
+    nova_hosts.each do |entry|
+      if entry["host_name"] == host_name
+        if entry["service"] == service_type
+            return entry["host_name"]
+        end
+      end
+    end
+    #name/service combo not found
+    return nil
+  end
+
+  def self.nova_aggregate_resources_ids(force_refresh=false)
+    # return the cached list unless requested
+    if not force_refresh
+      return @nova_aggregate_resources_ids if @nova_aggregate_resources_ids
+    end
     #produce a list of hashes with Id=>Name pairs
     lines = []
     #run command
     cmd_output = auth_nova("aggregate-list")
     #parse output
-    hash_list = cliout2list(cmd_output)
+    @nova_aggregate_resources_ids = cliout2list(cmd_output)
     #only interessted in Id and Name
-    hash_list.map{ |e| e.delete("Availability Zone")}
-    hash_list.map{ |e| e['Id'] = e['Id'].to_i}
-  return hash_list
+    @nova_aggregate_resources_ids.map{ |e| e.delete("Availability Zone")}
+    @nova_aggregate_resources_ids.map{ |e| e['Id'] = e['Id'].to_i}
+    @nova_aggregate_resources_ids
   end
 
-  def self.nova_aggregate_resources_get_name_by_id(name)
+  def self.nova_aggregate_resources_get_name_by_id(name, force_refresh=false)
     #find the id by the given name
-    nova_aggregate_resources_ids.each do |entry|
+    nova_aggregate_resources_ids(force_refresh).each do |entry|
       if entry["Name"] == name
         return entry["Id"]
       end

lib/puppet/provider/nova_aggregate/nova.rb

@@ -61,7 +61,8 @@ Puppet::Type.type(:nova_aggregate).provide(
     result = auth_nova("aggregate-create", resource[:name], extras)
 
     #get Id by Name
-    id = self.class.nova_aggregate_resources_get_name_by_id(resource[:name])
+    #force a refresh of the aggregate list on creation
+    id = self.class.nova_aggregate_resources_get_name_by_id(resource[:name], true)
 
     @property_hash = {
       :ensure => :present,
@@ -81,12 +82,23 @@ Puppet::Type.type(:nova_aggregate).provide(
     #add hosts - This throws an error if the host is already attached to another aggregate!
     if not @resource[:hosts].nil? and not @resource[:hosts].empty?
       @resource[:hosts].each do |host|
+        # make sure the host exists in nova, or nova will fail the call
+        # this solves weird ordering issues with a compute node that's
+        # not 100% up being added to the host aggregate
+        if is_host_in_nova?(host)
           auth_nova("aggregate-add-host", id, "#{host}")
+        else
+          warning("Cannot add #{host} to host aggregate, it's not available yet in nova host-list")
+        end
       end
       @property_hash[:hosts] = resource[:hosts]
     end
   end
 
+  def is_host_in_nova?(host)
+    return host==self.class.nova_get_host_by_name_and_type(host, "compute")
+  end
+
   def hosts=(val)
     #get current hosts
     id = self.class.nova_aggregate_resources_get_name_by_id(name)
@@ -101,7 +113,11 @@ Puppet::Type.type(:nova_aggregate).provide(
     #add hosts from the value list
     val.each do |h|
       if not attrs['Hosts'].include? h
+        if is_host_in_nova?(h)
           auth_nova("aggregate-add-host", id, "#{h}")
+        else
+          warning("Cannot add #{h} to host aggregate, it's not available yet in nova host-list")
+        end
       end
     end
   end

spec/unit/provider/nova_spec.rb

@@ -240,7 +240,8 @@ EOT
 | 2  | haha2 | -                 |
 +----+-------+-------------------+
       EOT
-      klass.expects(:auth_nova).returns(output)
+      # used the cache copy, don't call nova again
+      klass.expects(:auth_nova).never()
       res = klass.nova_aggregate_resources_get_name_by_id("notavailable")
       expect(res).to eql(nil)
     end
@@ -269,6 +270,38 @@ EOT
         }
       })
     end
+  end
 
+  describe 'when searching for a host/type combo' do
+    it 'should find the hostname if there is a match' do
+      output = <<-EOT
++-------------------+-------------+----------+
+| host_name         | service     | zone     |
++-------------------+-------------+----------+
+| node-control-001  | consoleauth | internal |
+| node-control-001  | cert        | internal |
+| node-compute-002  | compute     | nova     |
++-------------------+-------------+----------+
+      EOT
+      klass.expects(:auth_nova).returns(output)
+      res = klass.nova_get_host_by_name_and_type("node-compute-002","compute")
+      expect(res).to eq("node-compute-002")
+    end
+
+    it 'should return nil because there is no host/type combo match' do
+      output = <<-EOT
++-------------------+-------------+----------+
+| host_name         | service     | zone     |
++-------------------+-------------+----------+
+| node-control-001  | consoleauth | internal |
+| node-control-001  | cert        | internal |
+| node-compute-002  | compute     | nova     |
++-------------------+-------------+----------+
+      EOT
+      # used the cache copy, don't call nova again
+      klass.expects(:auth_nova).never()
+      res = klass.nova_get_host_by_name_and_type("node-compute-002","internal")
+      expect(res).to eql(nil)
+    end
   end
 end