[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

[Dan Smith]

> Takashi Natsume <***@lab.ntt.co.jp> 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/***@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-***@lists.openstack.org?subject:unsubscribe
http://lists

[Ed Leafe]

On May 10, 2018, at 12:50 AM, Takashi Natsume <***@lab.ntt.co.jp> 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-***@lists.openstack.org?subject:unsubscribe
http://lists

[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
> '00000000-0000-0000-0000-00000000' 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/29e643e4a93333866b33965b68fc8dfb8acf30fa/oslo_versionedobjects/fields.py#L359
> [4] https://review.openstack.org/#/c/540386
>
>
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-***@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

[Balázs Gibizer]

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




__________________________________________________________________________
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: OpenStack-dev-***@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-b

[Balázs Gibizer]

On Mon, May 14, 2018 at 11:49 AM, Balázs Gibizer
<***@ericsson.com> wrote:
>
>
> On Thu, May 10, 2018 at 8:48 PM, Dan Smith <***@danplanet.com> 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-***@lists.openstack.org?subject:unsubscribe
http://lists.openst