The default of the missing project_id and user_id in placement is an invalid UUID

Bug #1780107 reported by Balazs Gibizer
8
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Compute (nova)
Fix Released
Medium
Chris Dent

Bug Description

The placement config incomplete_consumer_project_id and incomplete_consumer_user_id defaulted to an invalid UUID[1]:

DEFAULT_CONSUMER_MISSING_ID = '00000000-0000-0000-0000-0000000000000'

Hint, the last block has 13 digits instead of 12. The DB schema doesn't catch this as it stores UUIDs in integer fields[2].

[1] https://github.com/openstack/nova/blob/07d3844123115b4b0e47d202a339aaab942d1a92/nova/conf/placement.py#L20
[2] https://github.com/openstack/nova/blob/07d3844123115b4b0e47d202a339aaab942d1a92/nova/db/sqlalchemy/api_models.py#L643

Tags: placement
Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

How to fix this besides that we remove the extra trailing 0 from DEFAULT_CONSUMER_MISSING_ID? Do we need to change the DB schema? What to do with the rows in the DB that already has the wrong value set as project_id user_id?

tags: added: placement
Revision history for this message
Chris Dent (cdent) wrote :

Gibi and I were being tired and misreading the table schemas.

The issue is that in the Project table the project_id is String(255), maintaining what it was from back when it was on the Consumer table.

But this means there's was no validity checking on the on the DEFAULT ID. However it's also the case that there's no requirement that it be a uuid, so we could go ahead and live with it, except that it is a bit weird as it is now, as it looks like it is supposed to be a uuid.

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

@Chris: thanks for the correction about the DB schema. Shouldn't we need a foreign key constraint between Consumer.project_id and Project.id ?

The failures in [1] tells me that somewhere along the way we try to store the value in an UUID field so I don't think we can live with the current value.

[1] http://logs.openstack.org/86/540386/8/check/nova-tox-functional/509458f/testr_results.html.gz

Revision history for this message
Chris Dent (cdent) wrote :

Those test results mean that somewhere prior to the db we are validating the fields as uuids but then at the db level keeping things as String(255). One of these things is wrong. If we say at the object level they are uuids, and change the validation warning to error, we're going to die _and_ we're disallowing non-uuid project ids, which at one point we wanted to allow.

So presumably we need to decide what our constraints are and make them the same in all three of the places where we could potentially be doing validation:

* json schema
* object field validation
* database schema

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

@Chris: thanks for the corrections regarding the DB schema. Should we still need a foreign key constraint between Consumer.project_id and Project.id and Consumer.user_id and User.id the express the relationship?

Based on the test result [1] somewhere we store the default id in a UUIDField so I think we cannot live with it but I have to dig further where we store it as UUIDField

[1] http://logs.openstack.org/86/540386/8/check/nova-tox-functional/509458f/testr_results.html.gz

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

disregard my comment #5 it was a result of a network glitch I guess.

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

The UUID warning (that is escalated to error) is not directly related. That is caused by a wrong yaml reference in the gabbit tests[1]:

  vars:
  - &default_incomplete_id 00000000-0000-0000-0000-0000000000000
  - &consumer_id fbad1a87-c341-4ac0-be49-777b21ce1b7b
  tests:

  - name: put an allocation without project/user (1.7)
    PUT: /allocations/*consumer_id

Here the *consumer_id treated as string and does not resolved to fbad1a87-c341-4ac0-be49-777b21ce1b7b.

So in summary:
* We can keep the value of the default incomplete id. It is misleading but it is not wrong per se.
* For me the foreign key still feels missing between Consumer and Project, User
* I will open a separate bug to fix the gabbit test causing the test failure in https://review.openstack.org/#/c/540386/

https://github.com/openstack/nova/blob/187f80a32c8b5cdfc115e9b90f4818f3d9d1f8fe/nova/tests/functional/api/openstack/placement/gabbits/ensure-consumer.yaml#L19

Revision history for this message
Balazs Gibizer (balazs-gibizer) wrote :

For the pure gabbi test issue I filed a separate bug: https://bugs.launchpad.net/nova/+bug/1780238

Revision history for this message
Jay Pipes (jaypipes) wrote :

Crap. Sorry about this, folks. All my fault, I'm afraid.

So, technically, there's no requirement that the project_id or user_id (being externally-set identifiers from Keystone) are UUIDs. They could just as easily be "fake_project" and "fake_user". It's not possible to specify an ID (UUID or otherwise) when creating projects and users in Keystone, so the UUID-ness of Keystone's current identifiers is an implementation detail AFAICT.

I think that what Matt is doing with the heal_allocations work will eventually cause this issue to resolve itself, though. What the heal_allocations work does is go through instances and sets the project and user ID on the allocations to the instance's project and user if they don't match (which they wouldn't match if the CONF.incomplete_consumer_project_id was used.

Jay Pipes (jaypipes)
Changed in nova:
assignee: nobody → Jay Pipes (jaypipes)
importance: Undecided → Medium
status: New → Triaged
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to nova (master)

Fix proposed to branch: master
Review: https://review.openstack.org/580358

Changed in nova:
status: Triaged → In Progress
Changed in nova:
assignee: Jay Pipes (jaypipes) → Chris Dent (cdent)
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to nova (master)

Reviewed: https://review.openstack.org/580358
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=4b62b6bfc9db4e318ae25221e9d5e606fff8e996
Submitter: Zuul
Branch: master

commit 4b62b6bfc9db4e318ae25221e9d5e606fff8e996
Author: Jay Pipes <email address hidden>
Date: Thu Jul 5 08:06:12 2018 -0400

    make incomplete_consumer_project_id a valid UUID

    Unfortunately, when I added the CONF.incomplete_consumer_project_id and
    CONF.incomplete_consumer_user_id options, the default value for those
    options was an invalid UUID (it had 13 zeroes for the last part of the
    UUID instead of 12). This is preventing some work that tries to disable
    warnings about invalid UUIDs being stored in ovo UUIDField objects.

    The work to "heal allocations" will be able to fix up any deployments
    that deployed the invalid UUID default CONF value because it looks for
    any instances in Nova where the placement allocation records don't have
    a matching user/project and replaces the mismatched user/project IDs on
    the consumer appropriately.

    Change-Id: I67b3b771f20e0b83225fce4839d378696fe1ab24
    Closes-bug: #1780107

Changed in nova:
status: In Progress → Fix Released
Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix included in openstack/nova 18.0.0.0b3

This issue was fixed in the openstack/nova 18.0.0.0b3 development milestone.

To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.