[Ovirt-devel] [PATCH]: Add host memory in kb to the database

[Steve Linabery]

On Mon, Aug 11, 2008 at 10:54:19AM -0400, Scott Seago wrote:
> Steve Linabery wrote:
>> On Mon, Aug 11, 2008 at 03:16:29PM +0200, Chris Lalancette wrote:
>>   
>>>     When inserting memory values into the database, ovirt-identify-node is
>>>     sending over values in kilobytes.  The database is also in kilobytes.
>>>     Don't run a non-sensical "mb_to_kb" conversion on the memory value
>>>     before sticking it into the database.
>>>         Signed-off-by: Chris Lalancette <clalance at redhat.com>
>>>
>>> diff --git a/wui/src/host-browser/host-browser.rb b/wui/src/host-browser/host-browser.rb
>>> index a1bda3d..881b2ae 100755
>>> --- a/wui/src/host-browser/host-browser.rb
>>> +++ b/wui/src/host-browser/host-browser.rb
>>> @@ -219,7 +219,7 @@ class HostBrowser
>>>                      "hostname"        => host_info['HOSTNAME'],
>>>                      "hypervisor_type" => host_info['HYPERVISOR_TYPE'],
>>>                      "arch"            => host_info['ARCH'],
>>> -                    "memory_in_mb"    => host_info['MEMSIZE'],
>>> +                    "memory"          => host_info['MEMSIZE'],
>>>                      "is_disabled"     => 0,
>>>                      "hardware_pool"   => HardwarePool.get_default_pool,
>>>                      # Let host-status mark it available when it
>>> @@ -232,7 +232,7 @@ class HostBrowser
>>>              host.uuid         = host_info['UUID']
>>>              host.hostname     = host_info['HOSTNAME']
>>>              host.arch         = host_info['ARCH']
>>> -            host.memory_in_mb = host_info['MEMSIZE']
>>> +            host.memory       = host_info['MEMSIZE']
>>>          end
>>>           # delete an existing CPUs and create new ones based on the 
>>> data
>>>
>>> _______________________________________________
>>> Ovirt-devel mailing list
>>> Ovirt-devel at redhat.com
>>> https://www.redhat.com/mailman/listinfo/ovirt-devel
>>>     
>>
>> I don't think this is necessary. The migration file for CreateHosts (currently wui/src/db/migrate/002_create_hosts.rb) doesn't define memory_in_mb, so this attribute is never being stuck into the database.
>>
>>   
> memory_in_mb is a method defined on Host which converts from MB to kb to  
> stuff in the db.  The idea is when you have a number  already in kb, you  
> just set host.memory, but if your value is in mb (as it would be if  
> you're soliciting user input via the WUI), you set host.memory_in_mb,  
> which converts to kb and stores in the db.
>
>> I'm not sure I understand what effect this:
>> "memory_in_mb"    => host_info['MEMSIZE'],
>>
>> was intended to have in host-browser.rb in the first place (creating an instance of Host using a non-existent attribute? Do I just not know ruby well enough to know what's going on here?)
>>
>>   
> Well the attribute doesn't exist, but as mentioned above, there's a  
> method with this name that sets the proper memory value (assuming what  
> you have is in megabytes).

For the record, there is only memory_in_mb= that does what you describe here (but which doesn't work for initializing a new instance).

>
> On this particular change, I seem to recall a bug a while back where we  
> had to change the host browser code to use the _in_mb methods to fix an  
> earlier problem. Could it be that the semantics of host_browser has  
> changed and that, at one point, we were passing memory in MB?
>

Seems likely enough.

> In a larger sense (i.e. beyond this particular bugfix, etc.) we've  
> talked about how we need a more robust way of dealing with  
> storage/memory sizing and conversion/display. As we see here, it's  
> currently a bit fragile.
>

Agreed.

To Chris's original post I say ACK, but not for the original reason stated.

ACK because if we don't set "memory" then the newly created object has no info on the memory at all.