_get_subnetpool_id does not return None when a cidr is specified and a subnetpool_id isn't.

[John Davidge]

I've just noticed this while rebasing the ongoing prefix delegation patch to align with the changes made by: https://review.openstack.org/#/c/197090/

When (https://review.openstack.org/#/c/168098/) was merged, it was decided that to get the old behaviour the user would have to explicitly specify "None" as the subnetpool_id when creating a subnet. I'm not sure this was the correct choice, as this breaks legacy applications using the neutron API.

This can be seen by simply running devstack with a value set for default_ipv4_subnet_pool and/or default_ipv6_subnet_pool.

To recreate in devstack:

1. Set default_ipv4_subnet_pool and/or default_ipv6_subnet_pool in /opt/stack/neutron/etc/neutron.conf to any value other than None, e.g default_ipv6_subnet_pool = "prefix_delegation"
2. Run the ./stack.sh script with ip_version set appropriately for the defaults set above. It will fail when trying to create the default networks.

[Pavel Bondar (pasha117)]

My undestanding of expected behavior is a bit different here.
If value for default_ipv4_subnet_pool or default_ipv6_subnet_pool are set in config,
then all subnet allocations are expected to be done using subnet_pool,
independently whether cidr is set (specific subnet) or not (auto subnet).

Need to check with Ryan Tidwell and Carl Baldween to confirm expected behavior here,
because they know subnetpool stuff better.

At first I thought [1] caused this issue, but after double checking
I didn't notice any change is handling subnetpool_id.
I.e. _get_subnetpool_id always returned default subnet_pool if it is set in config (independently from presence of cidr)

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

[John Davidge (john-davidge)]

My first thought was that this had been caused by [1], but actually it was [1] which uncovered what I believe to be an existing issue while rebasing [2] onto [1]. The approach in [2] previously bypassed this issue when default_ipv6_subnet_pool was set to "prefix_delegation" by only calling _create_pd_enabled_subnet if no cidr was given in the subnet_create call. By aligning it with the single-method approach in [1] I ran into this while testing. I hadn't run into this issue until now because the approach in [2] was effectively emulating the behaviour proposed in [3], but only when default_ipv6_subnet_pool is set to "prefix_delegation". Setting it to any other value breaks all legacy applications, including devstack.

The decision for the default behaviour to work this way was made during the implementation of [4], but I'm not sure this was the correct choice. I'd like to propose that the behaviour is updated as in [3], to be compatible with the currently published API.

[1] https://review.openstack.org/#/c/197090/
[2] https://review.openstack.org/#/c/158697/
[3] https://review.openstack.org/#/c/198437/
[4] https://review.openstack.org/#/c/168098/

[Carl Baldwin (carl-baldwin)]

The intention wass that when default_ipv6_subnet_pool is set, then that pool will be used by default regardless of the presence of a cidr in the request. When this is set, legacy behavior no longer applies.

IMO, making the default pool dependent on if a cidr is supplied adds yet another dimension to the decision-making process which could confuse API users even more.

[John Davidge (john-davidge)]

My thinking here is that the user possibly doesn't know that a default subnetpool has been set by the admin, and therefore it shouldn't be up to the user to change their API calls to get the behaviour they expect. It was my understanding that the default pools would be used only when the user has requested a subnet without expressing a cidr preference - therefore not breaking existing applications making use of the neutron api.

My concern is that all legacy applications (including devstack) are going to need to be updated to reflect the need to specify subnetpool_id=None, otherwise they will break or at least not function as expected where a default pool value has been set.

[John Davidge (john-davidge)]

Fix proposed: https://review.openstack.org/#/c/198437/

[Sam Betts (sambetts)]

Me and John had a quick discussion about this, and we came to an agreement on this truth table:

    | CIDR Given in request | Subnet Pool Given in request | Decision that should be made by Neutron
    | Yes | No | CIDR is used, subnet pool = None.
    | Yes | Yes | Neutron will validate CIDR against pool and will either error or suceed accordingly.
    | No | Yes | Subnet pool used is the one specified in the request.
    | No | No | Default subnet pool is used.

Using these cases should provide the most consistant API for the user, and should allow apps that currently call subnet-create without a subnet pool to continue operating as expected.

[John Davidge (john-davidge)]

Using the implicit (None) pool when a cidr is specified also matches with my understanding of the truth table in the REST API Impact section of [1], whereas the current implementation does not. [1] States that in the case of a cidr being specified and no subnetpool being given by the user, the implicit (None) pool should be used.

[1] http://specs.openstack.org/openstack/neutron-specs/specs/kilo/subnet-allocation.html

[Carl Baldwin (carl-baldwin)]

John, you're right about the truth table in the spec. It does say that. However, I'd still like to see this conversation come to a close and consider Amotoki's input too.

[Akihiro Motoki (amotoki)]

Sorry for late.

As long as I see comment #6 and #7, it seems we can safely return None for subnetpool_id when a cidr is specified and a subnet_pool is not specified.

IMO an important thing in the API perspective is that the defined attributes in a specific API version (or extension at the moment) should be visible in a response. Some attribute can be optional in a POST request, but we have a consistent logic to determine the default value for each case and we can return a deterministic value in a response.
Another important point is that we return a consistent response for a POST/PUT/GET request.
We can satisfy the above my points by returning None for subnetpool_id when only CIDR is specified.

[Mohammad Banikazemi (mb-s)]

@akihiro, I think the question left to answer is what to do when "cidr is specified and a subnet_pool is not specified" and a default subnet pool is specified?

[John Davidge (john-davidge)]

@Mohammad That's the situation we're discussing here. We don't want to treat the default subnet pool differently to any other subnet pool, and therefore it should be returned only when both CIDR and subnetpool are not specified.

[Baodong (Robert) Li (baoli)]

Extracted from /etc/netron.conf

# Default Subnet Pool to be used for IPv6 subnet-allocation.
# Specifies by UUID the pool to be used in case of subnet-create being
# called without a subnet-pool ID. Set to "prefix_delegation"
# to enable IPv6 Prefix Delegation in a PD-capable environment.
# See the description for default_ipv4_subnet_pool for more information.
# default_ipv6_subnet_pool =

it doesn't indicate that cidr specified in subnet-create will override the default subnet pool.

I'd like to suggest that to use prefix delegation in a cloud, the admin should create a shared pool for prefix delegation in the admin project, and openstack should reserve the name 'prefix-delegation' (or some sort) for PD. A few benefits with it:
  . when hint with PD is supported, the hind can be specified by the 'cidr'
  . a service plugin can choose to provision its PD server with the pool

So the default pool still maintains the same semantics as specified in above:
   -- if it's not None, and the user doesn't specify --subnetpool, it will be used for creating the subnet. If it's specified as prefix-delegation, then PD will be used. If the user specify --subnetpool, it will override the default

Another issue with the existing default subnetpool definition is the use of UUID in the config. This becomes a chicken and egg problem as the neutron server relies on the neutron.conf to be started so that the default pool can be created.

The default pool should be specified as name. And to make it further, the pool parameters may be specified in the config and neutron server can create the pool in the admin project as part of starting up the service.

[John Davidge (john-davidge)]

Let's put this on hold until we can discuss further at the summit. In the meantime we can fix the devstack failures when a default subnetpool is set https://bugs.launchpad.net/devstack/+bug/1472200

[OpenStack Infra (hudson-openstack)]

Change abandoned by John Davidge (<email address hidden>) on branch: master
Review: https://review.openstack.org/198437
Reason: Similar patch merged https://review.openstack.org/#/c/279378/