Open Stack

On Thu, May 10, 2018 at 8:48 PM, Dan Smith <dms at danplanet.com> wrote:
>>  The oslo UUIDField emits a warning if the string used as a field 
>> value
>>  does not pass the validation of the uuid.UUID(str(value)) call
>>  [3]. All the offending places are fixed in nova except the 
>> nova-manage
>>  cell_v2 map_instances call [1][2]. That call uses markers in the DB
>>  that are not valid UUIDs.
> 
> No, that call uses markers in the DB that don't fit the canonical 
> string
> representation of a UUID that the oslo library is looking for. There 
> are
> many ways to serialize a UUID:
> 
> https://en.wikipedia.org/wiki/Universally_unique_identifier#Format
> 
> The 8-4-4-4-12 format is one of them (and the most popular). Changing
> the dashes to spaces does not make it not a UUID, it makes it not the
> same _string_ and it's done (for better or worse) in the 
> aforementioned
> code to skirt the database's UUID-ignorant _string_ uniqueness
> constraint.

You are right, this is oslo specific. I think this weakens the severity 
of the warning in this particular case.

> 
>>  If we could fix this last offender then we could merge the patch [4]
>>  that changes the this warning to an exception in the nova tests to
>>  avoid such future rule violations.
>> 
>>  However I'm not sure it is easy to fix. Replacing
>>  'INSTANCE_MIGRATION_MARKER' at [1] to
>>  '00000000-0000-0000-0000-00000000' might work
> 
> The project_id field on the object is not a UUIDField, nor is it 36
> characters in the database schema. It can't be because project ids are
> not guaranteed to be UUIDs.

Correct. My bad. Then this does not cause any UUID warning.

> 
>>  but I don't know what to do with instance_uuid.replace(' ', '-') [2]
>>  to make it a valid uuid. Also I think that if there is an unfinished
>>  mapping in the deployment and then the marker is changed in the code
>>  that leads to inconsistencies.
> 
> IMHO, it would be bad to do anything that breaks people in the middle 
> of
> a mapping procedure. While I understand the desire to have fewer
> spurious warnings in the test runs, I feel like doing anything to 
> impact
> the UX or performance of runtime code to make the unit test output
> cleaner is a bad idea.

Thanks for confirming my original bad feelings about these kind of 
solutions.

> 
>>  I'm open to any suggestions.
> 
> We already store values in this field that are not 8-4-4-4-12, and the
> oslo field warning is just a warning. If people feel like we need to 
> do
> something, I propose we just do this:
> 
> https://review.openstack.org/#/c/567669/
> 
> It is one of those "we normally wouldn't do this with object schemas,
> but we know this is okay" sort of situations.
> 
> 
> Personally, I'd just make the offending tests shut up about the 
> warning
> and move on, but I'm also okay with the above solution if people 
> prefer.

I think that was Takashi's first suggestion as well. As in this 
particular case the value stored in the field is still a UUID just not 
in the canonical format I think it is reasonable to silence the warning 
for these 3 tests.

Thanks,
gibi