Removing project access to volume type will not work after 128+ access rule

Bug #1496747 reported by Szymon Datko
10
This bug affects 2 people
Affects Status Importance Assigned to Milestone
Cinder
Fix Released
Medium
Michal Dulko

Bug Description

I was having fun with Cinder, when I have spotted a bug in a mechanism that is being used to grant access for projects (aka 'tenants') to volume types. To be more specific: the problem is related to removing project access.

How to reproduce the bug:

1. In a loop, about 150 times:
a) create non-public volume type ( is_public=False )
b) grant access for project to newly created type
2. Try to remove project access to one of recently created type
3. You can expect from python-cinderclient an error 500 (~ Server is incapable of performing operation)
4. On Cinder's log there is an error: "Out of range value for column 'deleted' at row 1"

Packages' versions on my host:

cinder-api 1:2015.1.1-0ubuntu2~cloud2
cinder-common 1:2015.1.1-0ubuntu2~cloud2
cinder-scheduler 1:2015.1.1-0ubuntu2~cloud2
cinder-volume 1:2015.1.1-0ubuntu2~cloud2
python-cinder 1:2015.1.1-0ubuntu2~cloud2
python-cinderclient 1:1.3.1-2
python-oslo-db 1.7.1-0ubuntu2~cloud0

Description:

The problem is related to database - column 'deleted' on table 'volume_type_projects'.

Here you can see that it is created as *Boolean* type:
https://github.com/openstack/cinder/blob/master/cinder/db/sqlalchemy/migrate_repo/versions/032_add_volume_type_projects.py#L36

During conversion process, the result of db-sync in database is type *tinyint(1)*:
mysql> desc volume_type_projects;
+----------------+--------------+------+-----+---------+----------------+
| Field | Type | Null | Key | Default | Extra |
+----------------+--------------+------+-----+---------+----------------+
| id | int(11) | NO | PRI | NULL | auto_increment |
| created_at | datetime | YES | | NULL | |
| updated_at | datetime | YES | | NULL | |
| deleted_at | datetime | YES | | NULL | |
| volume_type_id | varchar(36) | YES | MUL | NULL | |
| project_id | varchar(255) | YES | | NULL | |
| deleted | tinyint(1) | YES | | NULL | |
+----------------+--------------+------+-----+---------+----------------+

Type *tinyint(1)* is, in fact, a reason why the bug does not occur before 128 access rules.

Now, what in fact causes bug is usage of soft_delete() function during access removing. See it here:
https://github.com/openstack/cinder/blob/master/cinder/db/sqlalchemy/api.py#L2776

The mentioned function is definied in *oslo.db* as:
https://github.com/openstack/oslo.db/blob/master/oslo_db/sqlalchemy/orm.py#L28

So, what happens when project access is removed? To the column 'deleted' in table 'volume_type_projects' the *row id* is assigned instead of *Boolean* value. Due to conversion from *Boolean* to *tinyint(1)* on db-sync, the problem is not spotted on fresh installation.

Possible solutions:

a) replace soft_delete() function with just update() - like it is done in rest of the code, see example here:
https://github.com/openstack/cinder/blob/master/cinder/db/sqlalchemy/api.py#L2710

b) upgrade the db schema

Revision history for this message
Szymon Datko (sdatko) wrote :
description: updated
Revision history for this message
Sean McGinnis (sean-mcginnis) wrote :

I think we should probably stick with the use of oslo.db's soft delete and update the schema to allow this. Interesting that they set that to the id though. Not sure the reason for that.

It might be good to get other's opinion on this as well.

And it might be good to review all our table schema's and see if there are other instances where this could be an issue. Luckily I doubt there's anyone who currently uses anywhere near 128 volume types.

Changed in cinder:
status: New → Confirmed
Revision history for this message
Szymon Datko (sdatko) wrote :

@sean-mcginnis:

"Interesting that they set that to the id though. Not sure the reason for that."
-> I am not sure too, but I have noticed that in Nova, it is also *row's id* inserted into column *deleted*. But for Nova (in example, see table *instances*) proper table schema is being used.

"Luckily I doubt there's anyone who currently uses anywhere near 128 volume types."
-> the problem is not diectly related to volume types, but to type-access-lists - so, it means you might have only few types and a lot of users (customers?) that you will want to provide access to.

Changed in cinder:
assignee: nobody → Michal Dulko (michal-dulko-f)
status: Confirmed → In Progress
Revision history for this message
Michal Dulko (michal-dulko-f) wrote :

Now I'm starting to wonder. volume_type_access_remove is the only place where we're using soft_delete, in other places we're just updating deleted column with True. For consistency we should decide - either go on and make all deletes use oslo.db's soft_delete, or just update volume_type_access_remove to match rest of the code. Thoughts?

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix proposed to cinder (master)

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

Revision history for this message
Szymon Datko (sdatko) wrote :

I personally don't see any advantage of duplicating row's id in column 'deleted'.

For now I would rather stick with update() to match with the rest of the code and if there are some pros of using oslo.db, then it should be changed in all places, as well in db-schema, in the future releases.

Revision history for this message
OpenStack Infra (hudson-openstack) wrote : Fix merged to cinder (master)

Reviewed: https://review.openstack.org/225701
Committed: https://git.openstack.org/cgit/openstack/cinder/commit/?id=0e66d3ea1ec83be3e42726063198183097d40cde
Submitter: Jenkins
Branch: master

commit 0e66d3ea1ec83be3e42726063198183097d40cde
Author: Michal Dulko <email address hidden>
Date: Mon Sep 21 15:58:01 2015 +0200

    Replace soft_delete in volume_type_access_remove

    This commit replaces oslo.db's soft_delete used in
    volume_type_access_remove in db.api with a simple update setting
    deleted column to True.

    As deleted column is boolean it is represented in the DB by TINYINT,
    which max value is 127. This introduces problems when removing records
    with id higher than that, because soft_delete sets deleted column to the
    value of id. This commit fixes the issue and adds unit tests for such
    case.

    Change-Id: I484e66125b5a29f490c070696b336be4a2693b3e
    Closes-Bug: 1496747

Changed in cinder:
status: In Progress → Fix Committed
Changed in cinder:
importance: Undecided → Medium
Thierry Carrez (ttx)
Changed in cinder:
milestone: none → liberty-rc1
status: Fix Committed → Fix Released
Thierry Carrez (ttx)
Changed in cinder:
milestone: liberty-rc1 → 7.0.0
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.