Race condition exists for default security group creation

[Maru Newby]

A race condition exists in the creation of a tenant's default security group, since the uniqueness is guaranteed by the name and the check is done in code. As a result, multiple requests have the potential to create multiple default security groups.

It probably makes sense to ensure the uniqueness of the security group name and tenant id relationship via a constraint on the model.

[yong sheng gong (gongysh)]

I have registered a BP: https://blueprints.launchpad.net/quantum/+spec/distributed-lock-resource-allocation for such kind of bugs.

[Marios Andreou (marios-b)]

Hi, I just submitted https://review.openstack.org/#/c/91805/. Mark, I don't know if this is the bug I asked you about a while back, may well be as I recall you saying something about overhauling some code to enforce this. I mean as was envisaged/described by the bluprint [1]. I then came across [2] which deals with the corresponding mac issue (though in code, no just on the model as slightly different situation) and there wasn't too much pushback there (though was ultimately abandoned...).

In any case, I made the review to also learn about the alembic migrations as I've never touched it before. Let me know if I should abandon this for whatever it is you have brewing to address it (unless it's a different bug...)

thanks, marios

[1] https://blueprints.launchpad.net/neutron/+spec/distributed-lock-resource-allocation
[2] https://review.openstack.org/#/c/34823/

[Marios Andreou (marios-b)]

This bug is specifically about creation of the security group called 'default'. Some questions:

* Do we really want to enforce that security group names are unique _per_tenant_? A lot of tests are failing with this migration because of duplicate security group names (this can be fixed obviously).
* If we do, we probably need to document it (will add !DocImpact) [1]
* How do we handle the case when someone tries to migrate who already has duplicate sg names for a given tenant_id? [2]

I just came across a relevant review @ [3]"Duplicate security group name cause fail to start instance" but this tried to enforce uniqueness in code, rather than the db schema.

[1] http://docs.openstack.org/api/openstack-network/2.0/content/security_groups.html
[2] https://review.openstack.org/#/c/91805/1/neutron/db/migration/alembic_migrations/versions/591bbc0502b2_enforce_unique_tenan.py
[3] https://review.openstack.org/#/c/79270/2

[Attila Fazekas (afazekas)]

It is very rare at the gate, but looks like it can happen:
http://logstash.openstack.org/#eyJzZWFyY2giOiJtZXNzYWdlOiBcIlNlY3VyaXR5IGdyb3VwIGRlZmF1bHQgYWxyZWFkeSBleGlzdHMgZm9yIHByb2plY3RcIiIsImZpZWxkcyI6W10sIm9mZnNldCI6MCwidGltZWZyYW1lIjoiYWxsIiwiZ3JhcGhtb2RlIjoiY291bnQiLCJ0aW1lIjp7InVzZXJfaW50ZXJ2YWwiOjB9LCJzdGFtcCI6MTQwMzgwNzgyMDAxNX0=

It also can happen with the tempest vm boot stress (rare) :
https://github.com/openstack/tempest/blob/master/tempest/stress/actions/server_create_destroy.py

[Eugene Nikanorov (enikanorov)]

Hi, are we going to move on with the fix?
The patch is on gerrit, Marios, can you resurrect it?

[OpenStack Infra (hudson-openstack)]

Change abandoned by Salvatore Orlando (<email address hidden>) on branch: master
Review: https://review.openstack.org/91805
Reason: This patch has been dead enough that I think it's safe to abandon.
The author can resurrect it if needed.

[Ann Taraday (akamyshnikova)]

Suggested solution https://review.openstack.org/135006

[OpenStack Infra (hudson-openstack)]

Change abandoned by Ann Kamyshnikova (<email address hidden>) on branch: master
Review: https://review.openstack.org/135006
Reason: I continue working on existing change as I see that people share my opinion here in thread. Seems that I'm wrong and disadvantages listed by others are more important. I will work on alternative.

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/142101
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=79c97120de9cff4d0992b5d41ff4bbf05e890f89
Submitter: Jenkins
Branch: master

commit 79c97120de9cff4d0992b5d41ff4bbf05e890f89
Author: Ann Kamyshnikova <email address hidden>
Date: Fri Dec 12 15:30:06 2014 +0300

    Default security group table

    This change prevents the race condition by enforcing a single default
    security group via new table default_security_group. It has tenant_id
    as primary key and security_group_id, which is id of default
    security group. Migration that inroduces this table has sanity check that
    verifies that there is no duplicate default security group in any
    tenant.

    This idea has come up from discussion in comments to
    https://review.openstack.org/135006

    DocImpact

    Closes-bug: #1194579

    Change-Id: Ifa8fbddd22bce4c50836cf443ebe10dff37443ef

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: stable/juno
Review: https://review.openstack.org/156596

[OpenStack Infra (hudson-openstack)]

Change abandoned by Ihar Hrachyshka (<email address hidden>) on branch: stable/juno
Review: https://review.openstack.org/156596
Reason: That's an unreasonable and incomplete approach that we cannot take for stable releases. There seems to be no solution for Juno without db migration rules that are not available for backports in upstream, so abandoning the patch.