subnetpool allocation not ensuring non-overlapping cidrs

Bug #1451576 reported by Cedric Brandily on 2015-05-04
12
This bug affects 2 people
Affects Status Importance Assigned to Milestone
neutron
High
Cedric Brandily
Kilo
Undecided
Unassigned

Bug Description

_get_allocated_cidrs[1] locks only allocated subnets in a subnetpool (with mysql/postgresql at least) which ensures we won't allocate a cidr overlapping with existent cidrs but nothing disallows a concurrent subnet allocation to create a subnet in the same subnetpool.

[1]: https://github.com/openstack/neutron/blob/5962d825a6c98225c51bc6dd304b5c1ac89035etef/neutron/ipam/subnet_alloc.py#L40-L44

Changed in neutron:
assignee: nobody → Cedric Brandily (cbrandily)
Changed in neutron:
status: New → In Progress

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

Changed in neutron:
importance: Undecided → High

Reviewed: https://review.openstack.org/179955
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=3682e3391f188845d0c7f382f0ccd4b38db3904e
Submitter: Jenkins
Branch: master

commit 3682e3391f188845d0c7f382f0ccd4b38db3904e
Author: Cedric Brandily <email address hidden>
Date: Mon May 4 23:36:19 2015 +0200

    Ensure non-overlapping cidrs in subnetpools without galera

    _get_allocated_cidrs[1] locks only allocated subnets in a subnetpool
    (with mysql/postgresql at least). It ensures we don't allocate a cidr
    overlapping with existent cidrs but nothing disallows a concurrent
    subnet allocation to create a subnet in the same subnetpool.

    This change replaces the lock on subnetpool subnets by a lock on the
    subnetpool itself. It disallows to allocate concurrently 2 subnets in
    the same subnetpool and ensure non-overlapping cidrs in the same
    subnetpool.

    Moreover this change solves a trouble with postgresql which disallows
    to lock an empty select with an outer join: it happens on first subnet
    allocation in a subnetpool when no specific cidr is provided. Moving
    the lock ensures the lock is done on a non-empty select.

    But this change does not ensure non-overlapping cidrs in subnetpools
    with galera because galera doesn't support SELECT FOR UPDATE locks. A
    follow-up change will (try to?) remove locks from subnet allocation[1]
    in order to ensure non-overlapping cidrs in subnetpools also with galera.

    [1] in neutron.ipam.subnet_alloc.SubnetAllocator

    Closes-Bug: #1451558
    Partial-Bug: #1451576
    Change-Id: I73854f9863f44621ae0d89c5dc4893ccc16d07e4

Download full text (93.9 KiB)

Reviewed: https://review.openstack.org/196097
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=1cfed745d54a6ce9cb3dd4e6f454666d9e6676c2
Submitter: Jenkins
Branch: feature/qos

commit ba7d673d1ddd5bfa5aa1be5b26a59e9a8cd78a9f
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:31:38 2015 -0700

    Remove duplicated call to setup_coreplugin

    The test case for vlan_transparent was calling setup_coreplugin
    before calling the super setUp method which already calls
    setup_coreplugin. This was causing duplicate core plugin fixtures
    which resulted in patching the dhcp periodic check twice.

    Change-Id: Ide4efad42748e799d8e9c815480c8ffa94b27b38
    Partial-Bug: #1468998

commit e64062efa3b793f7c4ce4ab9e62918af4f1bfcc9
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:29:37 2015 -0700

    Remove double mock of dhcp agent periodic check

    The test case for the periodic check was patching a target
    that the core plugin fixture already patched out. This removes
    that and exposes the mock from the fixture so the test case
    can reference it.

    Change-Id: I3adee6a875c497e070db4198567b52aa16b81ce8
    Partial-Bug: #1468998

commit 25ae0429a713143d42f626dd59ed4514ba25820c
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:24:10 2015 -0700

    Remove double fanout mock

    The test_mech_driver was duplicating a fanout mock already setup
    in the setUp routine.

    Change-Id: I5b88dff13113d55c72241d3d5025791a76672ac2
    Partial-Bug: #1468998

commit 993771556332d9b6bbf7eb3f0300cf9d8a2cb464
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 17:55:16 2015 -0700

    Remove double callback manager mocks

    setup_test_registry_instance() in the base test case class gives
    each test its own registry by mocking out the get_callback_manager.
    The L3 agent test cases were duplicating this.

    Partial-Bug: #1468998
    Change-Id: I7356daa846524611e9f92365939e8ad15d1e1cd8

commit 0be1efad93734f11cd63fb3b7bd2983442ce1268
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 16:57:30 2015 -0700

    Remove ensure_dirs double-patch

    test_spawn_radvd called mock.patch on ensure_dirs after the
    setup method already patched it out. This causes issues when
    mock.patch.stopall() is called because the mocks are stored
    as a set and are unwound in a non-deterministic fashion.[1]
    So some of the time they will be undone correctly, but others
    will leave a monkey-patched in mock, causing the ensure_dir
    test to fail.

    1. http://bugs.python.org/issue21239

    Closes-Bug: #1467908
    Change-Id: I321b5fed71dc73bd19b5099311c6f43640726cd4

commit 0a2238e34e72c17ca8a75e36b1f56e41a3ece74e
Author: Sukhdev Kapur <email address hidden>
Date: Thu Jun 25 15:11:28 2015 -0700

    Fix tenant-id in Arista ML2 driver to support HA router

    When HA router is created, the framework creates a network and does
    not specify the tenant-id. This casuse Arista ML2 driver to fail.
    This patch sets the tenant-id when it is not passed explicitly by
    by the network_create() call from the HA r...

tags: added: in-feature-qos

Change abandoned by Kyle Mestery (<email address hidden>) on branch: feature/pecan
Review: https://review.openstack.org/196701
Reason: This is lacking the functional fix [1], so I'll propose a new merge commit which includes that one.

[1] https://review.openstack.org/#/c/196711/

Download full text (171.5 KiB)

Reviewed: https://review.openstack.org/196920
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=7f759c077f8f860c13db92d2ea6b353ef6b70900
Submitter: Jenkins
Branch: feature/pecan

commit 8123144fadd7c5d5e6e56a76ea860512619a2cf6
Author: Moshe Levi <email address hidden>
Date: Sun Jun 28 14:37:14 2015 +0300

    Fix Consolidate sriov agent and driver code

    This patch add mising __init to mech_sriov/mech_driver/
    and update the setup.cfg to the new agent entrypoint

    Trivial Fix

    Change-Id: I53a527081feb78472f496675bbb3c5121d38a14a

commit 8942fccf02e6e179d47582fdb2792a1ca972da21
Author: Assaf Muller <email address hidden>
Date: Mon Jun 29 11:38:51 2015 -0400

    Remove failing SafeFixture tests

    The fixtures 1.3 release attempted to fix the fixtures resource
    leak issue, but failed to do so completely. Our own SafeFixture
    is still needed: The 1.3 release broke our SafeFixture tests,
    but not the usage of SafeFixture itself. This patch removes
    those failing tests for now to unbreak the gate. Jakub reported
    a bug on fixtures 1.3:
    https://bugs.launchpad.net/python-fixtures/+bug/1469759

    We will continue to use SafeFixture until that bug is fixed
    in fixtures, at which point we will be able to require
    fixtures > 1.3.

    Change-Id: I59457c3bb198ff86d5ad55a1e623d008f0034b8f
    Closes-Bug: #1469734

commit 71dffb0a2c1720cd8233a329d32958a0160dd6f5
Author: Kevin Benton <email address hidden>
Date: Mon Jun 29 08:27:41 2015 +0000

    Revert "Removed test_lib module"

    This reverts commit 9a6536de6e1a7fe9b2552adc142e254426b82b6f.

    We pulled all of the plugins out of the tree, many of which still inherit
    from neutron test classes. This change then stated that we no longer
    support testing other plugins. I think this is a bit premature and should
    have been discussed under the subject
    "Neutron plugins can't use neutron plugin unit tests" or something
    similar.

    Change-Id: I68318589f010b731574ea3bfa8df98492bab31fc

commit b20fd81dbd497e058384a0af065dd0f1fdc4c728
Author: Jakub Libosvar <email address hidden>
Date: Fri Jun 5 14:32:51 2015 +0000

    Refactor NetcatTester class

    Following capabilities were added:
       - used transport protocol is passed as a constant instead of bool
       - src port for testing was added
       - connection can be established explicitly
       - change constructor parameters of NetcatTester

    As a part of removing bool for protocol definition
    get_free_namespace_port() was also modified to match the behavior.

    Change-Id: Id2ec322e7f731c05a3754a65411c9a5d8b258126

commit 83e37980dcd0b2bad6d64dd2cb23bcd2891cafca
Author: jingliuqing <email address hidden>
Date: Sat Jun 27 13:41:54 2015 +0800

    Use REST rather than ReST

    Change-Id: I06c9deaab58c5ec13bfeec39fb8fd4b1fe21f42d

commit 1b60df85ba3ad442c2e4e7e52538e1b9a1bf9378
Author: Kevin Benton <email address hidden>
Date: Thu Jun 25 18:34:38 2015 -0700

    Add a double-mock guard to the base test case

    Use mock to patch mock with a check to prevent multiple active
    patches to the...

tags: added: in-feature-pecan
Miguel Lavalle (minsel) wrote :

Fix https://review.openstack.org/#/c/187985/ is aimed at closing this bug

Reviewed: https://review.openstack.org/191045
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=cca48ad44d43142ab40971a96870417996db0f26
Submitter: Jenkins
Branch: stable/kilo

commit cca48ad44d43142ab40971a96870417996db0f26
Author: Cedric Brandily <email address hidden>
Date: Mon May 4 23:36:19 2015 +0200

    Ensure non-overlapping cidrs in subnetpools without galera

    _get_allocated_cidrs[1] locks only allocated subnets in a subnetpool
    (with mysql/postgresql at least). It ensures we don't allocate a cidr
    overlapping with existent cidrs but nothing disallows a concurrent
    subnet allocation to create a subnet in the same subnetpool.

    This change replaces the lock on subnetpool subnets by a lock on the
    subnetpool itself. It disallows to allocate concurrently 2 subnets in
    the same subnetpool and ensure non-overlapping cidrs in the same
    subnetpool.

    Moreover this change solves a trouble with postgresql which disallows
    to lock an empty select with an outer join: it happens on first subnet
    allocation in a subnetpool when no specific cidr is provided. Moving
    the lock ensures the lock is done on a non-empty select.

    But this change does not ensure non-overlapping cidrs in subnetpools
    with galera because galera doesn't support SELECT FOR UPDATE locks. A
    follow-up change will (try to?) remove locks from subnet allocation[1]
    in order to ensure non-overlapping cidrs in subnetpools also with galera.

    [1] in neutron.ipam.subnet_alloc.SubnetAllocator

    Closes-Bug: #1451558
    Partial-Bug: #1451576
    Change-Id: I73854f9863f44621ae0d89c5dc4893ccc16d07e4
    (cherry picked from commit 3682e3391f188845d0c7f382f0ccd4b38db3904e)
    Conflicts:
     neutron/ipam/subnet_alloc.py

tags: added: in-stable-kilo
Miguel Lavalle (minsel) wrote :

Fix to this bugged is merged: https://review.openstack.org/#/c/187985/

Changed in neutron:
status: In Progress → Fix Released

Reviewed: https://review.openstack.org/187985
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=03b70b109449f3b9329834c7aa88fd26ed71cf26
Submitter: Jenkins
Branch: master

commit 03b70b109449f3b9329834c7aa88fd26ed71cf26
Author: Cedric Brandily <email address hidden>
Date: Thu May 28 18:35:17 2015 +0200

    Ensure non-overlapping cidrs in subnetpools with galera

    This change enables galera support in _lock_subnetpool[1]. It uses an
    update to disallow 2 transactions performing concurrent subnet
    allocation in the same subnetpool to succeed: the 2 transactions will
    conflict because they update the same row so the db (including Galera
    multi-writer cluster) will discard the last transaction and
    Controller.create[2] will catch and retry the "discarded" allocation.

    This change adds the "hash" attribute in "subnetpools" table to enable
    previous update.

    [1] neutron.ipam.subnet_alloc.SubnetAllocator
    [2] neutron.api.v2.base

    Change-Id: I74f7100a6fd9b7787be693adffec15ec468d0018
    Closes-Bug: #1451576

Download full text (37.3 KiB)

Reviewed: https://review.openstack.org/211492
Committed: https://git.openstack.org/cgit/openstack/neutron/commit/?id=a7b91632fc65ab9d2687298c68b1d715866d0356
Submitter: Jenkins
Branch: feature/pecan

commit 966203f89dee8fe61fb2dce654e36e510e80380f
Author: Sukhdev Kapur <email address hidden>
Date: Wed Jul 1 16:30:44 2015 -0700

    Neutron-Ironic integration patch

    This patch is in preparation for the integration
    of Ironic and Neutron. A new vnic_type is being
    added so that ML2 drivers can filter for all
    Ironic ports based upon match for 'baremetal'.
    Nova/Ironic will set this vnic_type when issuing
    port-create request to neutron.
    (e.g. binding:vnic_type = 'baremetal' )

    Change-Id: I25dc9472b31db052719db503a10c1fb1a55572ef
    Partial-Implements: blueprint neutron-ironic-integration

commit 236e408272bcb9b8e957524864e571b5afdc4623
Author: Oleg Bondarev <email address hidden>
Date: Tue Jul 7 12:02:58 2015 +0300

    DVR: fix router scheduling

    Fix scheduling of DVR routers to not stop scheduling once
    csnat portion was scheduled. See bug report for failing
    scenario.

    This partially reverts
    commit 3794b4a83e68041e24b715135f0ccf09a5631178
    and fixes bug 1374473 by moving csnat scheduling
    after general dvr router scheduling, so double binding does
    not happen.

    Closes-Bug: #1472163
    Related-Bug: #1374473
    Change-Id: I57c06e2be732e47b6cce7c724f6b255ea2d8fa32

commit e152f93878b9bb6af7cfedc9e045892fcf7d0615
Author: Assaf Muller <email address hidden>
Date: Sat Aug 8 21:15:03 2015 +0300

    TESTING.rst love

    Change-Id: I64b569048f8f87ea2fe63d861302b4020d36493d

commit 633c52cca1b383af2c900e1663c8682114acd177
Author: sridhargaddam <email address hidden>
Date: Wed Aug 5 10:49:33 2015 +0000

    Avoid dhcp_release for ipv6 addresses

    dhcp_release is only supported for IPv4 addresses [1] and not for
    IPv6 addresses [2]. There will be no effect when it is called with
    IPv6 address. This patch adds a corresponding note and avoids calling
    dhcp_release for IPv6 addresses.

    [1] http://manpages.ubuntu.com/manpages/trusty/man1/dhcp_release.1.html
    [2] http://lists.thekelleys.org.uk/pipermail/dnsmasq-discuss/2013q2/007084.html

    Change-Id: I8b8316c9d3d011c2a687a3a1e2a4da5cf1b5d604

commit 2de8fad17402f38bbc30204ee2f4f99cf21cb69d
Author: OpenStack Proposal Bot <email address hidden>
Date: Mon Aug 10 06:11:06 2015 +0000

    Imported Translations from Transifex

    For more information about this automatic import see:
    https://wiki.openstack.org/wiki/Translations/Infrastructure

    Change-Id: I2b423e83a7d0ac8b23239f81fe33dd8382c6fff6

commit fef79dc7b9162e03c8891645494c115b52d4d014
Author: Henry Gessau <email address hidden>
Date: Mon Aug 3 23:30:34 2015 -0400

    Consistent layout and headings for devref

    The lack of convention for heading levels among the independently
    written devref documents was starting to make the Table of Contents
    look rather messy when rendered in HTML.

    This patch does not cover the "Neutron Internals" section since its
    layo...

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

Other bug subscribers

Remote bug watches

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