Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Balázs Gibizer]

On Mon, May 14, 2018 at 11:49 AM, Balázs Gibizer 
 wrote:



On Thu, May 10, 2018 at 8:48 PM, Dan Smith  wrote:


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.




I proposed a patch to suppress those warnings: 
https://review.openstack.org/#/c/568263


Cheers,
gibi


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Balázs Gibizer]

On Thu, May 10, 2018 at 8:48 PM, Dan Smith  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
 '----' 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




__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[melanie witt]

On Thu, 10 May 2018 11:48:31 -0700, Dan Smith wrote:

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.


I'm in favor of this "solution" because, as you mentioned earlier, 
project_id/user_id aren't supposed to be restricted to UUID-only or 36 
characters anyway -- they come from the identity service and could be 
any string. We've been good about keeping with String(255) in the 
database schema for project_id/user_id originating from the identity 
service.


And, I noticed Instance.project_id is a StringField too [1]. Really, 
IMHO we should be consistent with this field type among the various 
objects for project_id/user_id.


Best,
-melanie

[1] 
https://github.com/openstack/nova/blob/e35e8d7/nova/objects/instance.py#L121


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Dan Smith]

> Takashi Natsume  writes:
>
>> In some compute REST APIs, it returns the 'marker' parameter
>> in their pagination.
>> Then users can specify the 'marker' parameter in the next request.

I read this as you saying there was some way that the in-band marker
mapping could be leaked to the user via the REST API. However, if you
meant to just offer up the REST API's pagination as an example that we
could follow in the nova-manage CLI, requiring users to provide the
marker each time, then ignore this part:

> How is this possible? The only way we would get the marker is if we
> either (a) listed the mappings by project_id, using
> INSTANCE_MAPPING_MARKER as the query value, or (b) listed all the
> mappings and somehow returned those to the user.
>
> I don't think (a) is a thing, and I'm not seeing how (b) could be
> either. If you know of a place, please write a functional test for it
> and we can get it resolves. In my proposed patch, I added a filter to
> ensure that this doesn't show up in the get_by_cell_id() query, but
> again, I'm not sure how this would ever be exposed to a user.
>
> https://review.openstack.org/#/c/567669/1/nova/objects/instance_mapping.py@173

As I said in my reply to gibi, I don't think making the user keep track
of the marker is a very nice UX for a management CLI, nor is it as
convenient for something like puppet to run as it has to parse the
(grossly verbose) output each time to extract that marker.

--Dan

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Dan Smith]

Takashi Natsume  writes:

> In some compute REST APIs, it returns the 'marker' parameter
> in their pagination.
> Then users can specify the 'marker' parameter in the next request.

How is this possible? The only way we would get the marker is if we
either (a) listed the mappings by project_id, using
INSTANCE_MAPPING_MARKER as the query value, or (b) listed all the
mappings and somehow returned those to the user.

I don't think (a) is a thing, and I'm not seeing how (b) could be
either. If you know of a place, please write a functional test for it
and we can get it resolves. In my proposed patch, I added a filter to
ensure that this doesn't show up in the get_by_cell_id() query, but
again, I'm not sure how this would ever be exposed to a user.

https://review.openstack.org/#/c/567669/1/nova/objects/instance_mapping.py@173

--Dan

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Dan Smith]

> 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.

> 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
> '----' 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.

> 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.

> 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.

--Dan

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Ed Leafe]

On May 10, 2018, at 12:50 AM, Takashi Natsume  
wrote:
> 
> So it is one way to change the command to stop storing a 'marker' value
> in the InstanceMapping (instance_mappings) DB table
> and return (print) a 'marker' value and be able to be specifid
> the 'marker' value as the command line argument.

Anything that gets rid of the awful hack used for the instance mapping code 
would be welcome. Storing a marker in the table is terrible, and then munging a 
UUID to be a not-UUID is even worse. My cleanup at least got rid of the second 
hack, but I would have preferred to fix the whole thing by not storing the 
marker in the first place.

-- Ed Leafe






__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Stephen Finucane]

On Tue, 2018-05-08 at 11:49 +0200, Balázs Gibizer wrote:
> Hi,
> 
> 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. 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 
> '----' might work 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.
> 
> I'm open to any suggestions.
> 
> Cheers,
> gibi

This is a somewhat complicated issue. I haven't got any ideas to solve
this (edleafe tried and failed) but I have submitted a patch to explain
why we do this, pending a real resolution.

   https://review.openstack.org/567597

Stephen

> 
> [1] 
> https://github.com/openstack/nova/blob/09af976016a83288df22ac6ed1cce1676c2294cc/nova/cmd/manage.py#L1168
> [2] 
> https://github.com/openstack/nova/blob/09af976016a83288df22ac6ed1cce1676c2294cc/nova/cmd/manage.py#L1180
> [3] 
> https://github.com/openstack/oslo.versionedobjects/blob/29e643e4a9866b33965b68fc8dfb8acf30fa/oslo_versionedobjects/fields.py#L359
> [4] https://review.openstack.org/#/c/540386
> 
> 
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [nova] nova-manage cell_v2 map_instances uses invalid UUID as marker in the db

[Takashi Natsume]

The temporary fix to pass the gate jobs is to mock
the 'warnings.warn'　method in the test method.
Then add a TODO note to fix storing non-UUID value
in the 'map_instances' command of the 'CellV2Commands' class.

The fundamental solution is to change the design of
the 'map_instances' command.
In the first place, it is not good to store non-UUID value in the UUID 
field.


In some compute REST APIs, it returns the 'marker' parameter
in their pagination.
Then users can specify the 'marker' parameter in the next request.

So it is one way to change the command to stop storing a 'marker' value
in the InstanceMapping (instance_mappings) DB table
and return (print) a 'marker' value and be able to be specifid
the 'marker' value as the command line argument.

On 2018/05/08 18:49, Balázs Gibizer wrote:

Hi,

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. 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 '----' 
might work 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.


I'm open to any suggestions.

Cheers,
gibi


[1] 
https://github.com/openstack/nova/blob/09af976016a83288df22ac6ed1cce1676c2294cc/nova/cmd/manage.py#L1168 

[2] 
https://github.com/openstack/nova/blob/09af976016a83288df22ac6ed1cce1676c2294cc/nova/cmd/manage.py#L1180 

[3] 
https://github.com/openstack/oslo.versionedobjects/blob/29e643e4a9866b33965b68fc8dfb8acf30fa/oslo_versionedobjects/fields.py#L359 


[4] https://review.openstack.org/#/c/540386


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev


Regards,
Takashi Natsume
NTT Software Innovation Center
E-mail: natsume.taka...@lab.ntt.co.jp


__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev