Ovirt devel - Mar 2009 - [PATCH server] Fixed a few bugs in o-identify-node processing.

Fixed the processing of NICs to handle properly deleting a NIC that was
not identified by the node. All unidentified NICs are now deleted from
the database after o-i-node runs.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 src/host-browser/host-browser.rb |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/src/host-browser/host-browser.rb b/src/host-browser/host-browser.rb
index 1c5cf83..b40a73f 100755
--- a/src/host-browser/host-browser.rb
+++ b/src/host-browser/host-browser.rb
@@ -271,6 +271,7 @@ class HostBrowser
 
         puts "Updating NIC records for the node"
         nics = Array.new
+        nics_to_delete = Array.new
 
         host.nics.collect do |nic|
             found = false
@@ -278,7 +279,8 @@ class HostBrowser
             nic_info.collect do |detail|
                 # if we have a match, then update the database and remove
                 # the received data to avoid creating a dupe later
-                if detail['MAC'] == nic.mac
+                if detail['MAC'].upcase == nic.mac
+                  puts "Updating details for: #{nic.interface_name}
[#{nic.mac}]}"
                     nic_info.delete(detail)
 
                     updated_nic = Nic.find_by_id(nic.id)
@@ -286,7 +288,6 @@ class HostBrowser
                     updated_nic.bandwidth = detail['BANDWIDTH'].to_i
                     updated_nic.interface_name = detail['IFACE_NAME']
 
-                    updated_nic.save!
                     found=true
                     nic_info.delete detail
                 end
@@ -294,14 +295,16 @@ class HostBrowser
 
             # if the record wasn't found, then remove it from the database
             unless found
-                host.nics.delete(nic)
-                nic.destroy
+              puts "Marking NIC for removal: #{nic.interface_name}
[#{nic.mac}]"
+                nics_to_delete << nic
             end
         end
 
+        nics_to_delete.each { |nic| puts "Removing NIC:
#{nic.interface_name} []#{nic.mac}]"; host.nics.delete(nic) }
+
         # iterate over any nics left and create new records for them.
         nic_info.collect do |nic|
-            puts "Creating a new nic..."
+            puts "Creating a new nic: #{nic.interface_name}
[#{nic.mac}]"
             detail = Nic.new(
                 'mac'          => nic['MAC'].upcase,
                 'bandwidth'    => nic['BANDWIDTH'].to_i,
-- 
1.6.0.6

Applied this patch and haven't run it yet, but noticed a few things

Darryl L. Pierce wrote:
> Fixed the processing of NICs to handle properly deleting a NIC that was
> not identified by the node. All unidentified NICs are now deleted from
> the database after o-i-node runs.
>
> Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
> ---
>  src/host-browser/host-browser.rb |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/src/host-browser/host-browser.rb
b/src/host-browser/host-browser.rb
> index 1c5cf83..b40a73f 100755
> --- a/src/host-browser/host-browser.rb
> +++ b/src/host-browser/host-browser.rb
> @@ -271,6 +271,7 @@ class HostBrowser
>  
>          puts "Updating NIC records for the node"
>          nics = Array.new
> +        nics_to_delete = Array.new
>  
>          host.nics.collect do |nic|
>   
This 'collect' should more optimally be 'each' as we're not
using the
array returned by collect.

>              found = false
> @@ -278,7 +279,8 @@ class HostBrowser
>              nic_info.collect do |detail|
>   
Same with this collect here

>                  # if we have a match, then update the database and remove
>                  # the received data to avoid creating a dupe later
> -                if detail['MAC'] == nic.mac
> +                if detail['MAC'].upcase == nic.mac
> +                  puts "Updating details for: #{nic.interface_name}
[#{nic.mac}]}"
>                      nic_info.delete(detail)
>   
The 'detail' nic is being removed from the nic_info array here, and at
the end of this if block as well (see below)

>  
>                      updated_nic = Nic.find_by_id(nic.id)
> @@ -286,7 +288,6 @@ class HostBrowser
>                      updated_nic.bandwidth =
detail['BANDWIDTH'].to_i
>                      updated_nic.interface_name =
detail['IFACE_NAME']
>  
> -                    updated_nic.save!
>   
Will this work if you remove this? If I understand what your doing by
reading this, you are relying on the host.save! a little further down to
save the updated nics, but the manipulation of the nic object (eg
updating bandwidth and interface name) occurs on a nic object you
explicitly retrieve via find_by_id here. Unless I'm mistaken you will
either need this updated_nic.save! or instead of updating the nic via
the "updated_nic" object, you can simply use the "nic"
object (eg the
iterator as set via the host.nics.collect{ |nic| above).

>                      found=true
>                      nic_info.delete detail
>   
Here is the other place the 'detail' nic is being removed from nic_info

>                  end
> @@ -294,14 +295,16 @@ class HostBrowser
>  
>              # if the record wasn't found, then remove it from the
database
>              unless found
> -                host.nics.delete(nic)
> -                nic.destroy
> +              puts "Marking NIC for removal: #{nic.interface_name}
[#{nic.mac}]"
> +                nics_to_delete << nic
>              end
>          end
>  
> +        nics_to_delete.each { |nic| puts "Removing NIC:
#{nic.interface_name} []#{nic.mac}]"; host.nics.delete(nic) }
> +
>          # iterate over any nics left and create new records for them.
>          nic_info.collect do |nic|
> -            puts "Creating a new nic..."
> +            puts "Creating a new nic: #{nic.interface_name}
[#{nic.mac}]"
>              detail = Nic.new(
>                  'mac'          => nic['MAC'].upcase,
>                  'bandwidth'    =>
nic['BANDWIDTH'].to_i,
>   
   -Mo

On Tue, Mar 31, 2009 at 06:43:30PM -0400, Mohammed Morsi
wrote:
> This 'collect' should more optimally be 'each' as we're
not using the
> array returned by collect.
Fixed those.
> > -                if detail['MAC'] == nic.mac
> > +                if detail['MAC'].upcase == nic.mac
> > +                  puts "Updating details for:
#{nic.interface_name} [#{nic.mac}]}"
> >                      nic_info.delete(detail)
> >   
> The 'detail' nic is being removed from the nic_info array here, and
at
> the end of this if block as well (see below)
D'oh! Fixed that.
> >  
> >                      updated_nic = Nic.find_by_id(nic.id)
> > @@ -286,7 +288,6 @@ class HostBrowser
> >                      updated_nic.bandwidth =
detail['BANDWIDTH'].to_i
> >                      updated_nic.interface_name =
detail['IFACE_NAME']
> >  
> > -                    updated_nic.save!
> >   
> Will this work if you remove this? If I understand what your doing by
> reading this, you are relying on the host.save! a little further down to
> save the updated nics, but the manipulation of the nic object (eg
> updating bandwidth and interface name) occurs on a nic object you
> explicitly retrieve via find_by_id here. Unless I'm mistaken you will
> either need this updated_nic.save! or instead of updating the nic via
> the "updated_nic" object, you can simply use the "nic"
object (eg the
> iterator as set via the host.nics.collect{ |nic| above).
This was some old code. I refactored that whole chunk so that it would
use the nic object it already had. Now it'll update that and save it
inline.

Updated patch is in-flight for this.

-- 
Darryl L. Pierce, Sr. Software Engineer @ Red Hat, Inc.
Virtual Machine Management - http://www.ovirt.org/
Is fearr Gaeilge bhriste n? B?arla cliste.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL:
<http://listman.redhat.com/archives/ovirt-devel/attachments/20090401/30f0a9b4/attachment.sig>

NOTE: This version includes fixes based on feedback from mmorsi and some
      refactorings to the way nics are updated.

Fixed the processing of NICs to handle properly deleting a NIC that was
not identified by the node. All unidentified NICs are now deleted from
the database after o-i-node runs.

Signed-off-by: Darryl L. Pierce <dpierce at redhat.com>
---
 src/host-browser/host-browser.rb |   36 ++++++++++++++++++------------------
 1 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/host-browser/host-browser.rb b/src/host-browser/host-browser.rb
index 1c5cf83..b927d3c 100755
--- a/src/host-browser/host-browser.rb
+++ b/src/host-browser/host-browser.rb
@@ -248,7 +248,7 @@ class HostBrowser
         Cpu.delete_all(['host_id = ?', host.id])
 
         puts "Saving new CPU records"
-        cpu_info.collect do |cpu|
+        cpu_info.each do |cpu|
             detail = Cpu.new(
                 "cpu_number"      => cpu['CPUNUM'].to_i,
                 "core_number"     => cpu['CORENUM]'].to_i,
@@ -271,37 +271,37 @@ class HostBrowser
 
         puts "Updating NIC records for the node"
         nics = Array.new
+        nics_to_delete = Array.new
 
-        host.nics.collect do |nic|
+        host.nics.each do |nic|
             found = false
 
-            nic_info.collect do |detail|
+            nic_info.each do |detail|
                 # if we have a match, then update the database and remove
                 # the received data to avoid creating a dupe later
-                if detail['MAC'] == nic.mac
-                    nic_info.delete(detail)
-
-                    updated_nic = Nic.find_by_id(nic.id)
-
-                    updated_nic.bandwidth = detail['BANDWIDTH'].to_i
-                    updated_nic.interface_name = detail['IFACE_NAME']
-
-                    updated_nic.save!
-                    found=true
-                    nic_info.delete detail
+                puts "Searching for existing record for:
#{detail['MAC'].upcase}"
+                if detail['MAC'].upcase == nic.mac
+                  puts "Updating details for:
#{detail['IFACE_NAME']} [#{nic.mac}]}"
+                  nic.bandwidth = detail['BANDWIDTH'].to_i
+                  nic.interface_name = detail['IFACE_NAME']
+                  nic.save!
+                  found = true
+                  nic_info.delete(detail)
                 end
             end
 
             # if the record wasn't found, then remove it from the database
             unless found
-                host.nics.delete(nic)
-                nic.destroy
+              puts "Marking NIC for removal: #{nic.interface_name}
[#{nic.mac}]"
+                nics_to_delete << nic
             end
         end
 
+        nics_to_delete.each { |nic| puts "Removing NIC:
#{nic.interface_name} []#{nic.mac}]"; host.nics.delete(nic) }
+
         # iterate over any nics left and create new records for them.
-        nic_info.collect do |nic|
-            puts "Creating a new nic..."
+        nic_info.each do |nic|
+            puts "Creating a new nic: #{nic.interface_name}
[#{nic.mac}]"
             detail = Nic.new(
                 'mac'          => nic['MAC'].upcase,
                 'bandwidth'    => nic['BANDWIDTH'].to_i,
-- 
1.6.0.6