Open Stack

On 03/16/2015 03:23 AM, Sylvain Bauza wrote:
>
> Le 14/03/2015 01:13, Chris Friesen a écrit :
>> 1) Do I need to do anything special in obj_make_compatible()?
>
> Yes, you need to make sure that you won't provide the new field to a previous
> version of Service object.
> See other examples on other objects to see how to do this.

Okay, that makes sense.


>> 2) Is what I've done in _from_db_object() correct?  If I don't do it like
>> this, how is the "reported_at" field handled when a node sends a v1.11 version
>> of the object (or the corresponding dict) to another node that wants a v1.12
>> version object?
>
> _from_db_object is called for transforming a DB services table SQLA object (ie.
> a tuple) into a NovaObject Service object.
> By saying that you won't check it, it means that you won't have it persisted on
> the DB table.

Would it be possible to have a case where a v1.12 Service object calls 
_from_db_object() where the argument is a DB object that hasn't been upgraded 
yet?  (Like maybe if we upgraded nova-compute before nova-conductor?)

If so, then it seems like either we implicitly set it as "None" in 
_from_db_object(), or else allow it to be lazy-loaded as "None" in obj_load_attr().


>> 3) Is it okay to lazy-load a "None" value in obj_load_attr()?  The nice thing
>> about doing it this way is that a large number of unit/functional tests can
>> stay as-is.
>>
>
> No, that's not acceptable. The goal of obj_load_attr() is to lazy-load fields by
> setting their values on the object. You should not return anything but instead
> make sure that self.reported_at field is set.

Whoops, the return is a clear bug.  Would it be okay to set "self.reported_at = 
None" in obj_load_attr()?

> Honestly, the patch is really big for being reviewed. I also have some concerns
> about how you plan to fix the bug by adding a new field which is not persisted,
> but I prefer to leave my comments on Gerrit, that's what it's used for :-)

I'm not sure what you mean here.  Are you suggesting that it should be broken 
into multiple patches?

Thanks for taking a look at this.

Chris