Open Stack

On 09/08/2015 08:15 PM, Ken'ichi Ohmichi wrote:
> 2015-09-08 19:45 GMT+09:00 Sean Dague <sean at dague.net>:
>> On 09/06/2015 11:15 PM, GHANSHYAM MANN wrote:
>>> Hi All,
>>> 
>>> As we all knows, api-paste.ini default setting for /v2 was
>>> changed to run those on v2.1 (v2.0 on v2.1) which is really great
>>> think for easy code maintenance in future (removal of v2 code).
>>> 
>>> To keep "v2.0 on v2.1" fully compatible with "v2.0 on v2.0", some
>>> bugs were found[1] and fixed. But I think we should fix those
>>> only for v2 compatible mode not for v2.1.
>>> 
>>> For example bug#1491325, 'device' on volume attachment Request
>>> is optional param[2] (which does not mean 'null-able' is allowed)
>>> and v2.1 used to detect and error on usage of 'device' as "None".
>>> But as it was used as 'None' by many /v2 users and not to break
>>> those, we should allow 'None' on v2 compatible mode also. But we
>>> should not allow the same for v2.1.
>>> 
>>> IMO v2.1 strong input validation feature (which helps to make
>>> API usage in correct manner) should not be changed, and for v2
>>> compatible mode we should have another solution without affecting
>>> v2.1 behavior may be having different schema for v2 compatible
>>> mode and do the necessary fixes there.
>>> 
>>> Trying to know other's opinion on this or something I missed
>>> during any discussion.
>>> 
>>> [1]: https://bugs.launchpad.net/python-novaclient/+bug/1491325 
>>> https://bugs.launchpad.net/nova/+bug/1491511
>>> 
>>> [2]:
>>> http://developer.openstack.org/api-ref-compute-v2.1.html#attachVolume
>>
>>
>>> 
A lot of these issue need to be a case by case determination.
>> 
>> In this particular case, we had the Documetation, the nova code,
>> the clients, and the future.
>> 
>> The documentation: device is optional. That means it should be a
>> string or not there at all. The schema was set to enforce this on
>> v2.1
>> 
>> The nova code: device = None was accepted previously, because
>> device is a mandatory parameter all the way down the call stack. 2
>> layers in we default it to None if it wasn't specified.
>> 
>> The clients: both python-novaclient and ruby fog sent device=None
>> in the common case. While only 2 data points, this does demonstrate
>> this is more wide spread than just our buggy code.
>> 
>> The future: it turns out we really can't honor this parameter in
>> most cases anyway, and passing it just means causing bugs. This is
>> an artifact of the EC2 API that only works on specific (and
>> possibly forked) versions of Xen that Amazon runs. Most hypervisor
>> / guest relationships don't allow this to be set. The long term
>> direction is going to be removing it from our API.
>> 
>> Given that it seemed fine to relax this across all API. We screwed
>> up and didn't test this case correctly, and long term we're going
>> to dump it. So we don't want to honor 3 different versions of this
>> API, especially as no one seems written to work against the
>> documentation, but were written against the code in question. If
>> they write to the docs, they'll be fine. But the clients that are
>> out in the wild will be fine as well.
> 
> I think the case by case determination is fine, but current change 
> progress of relaxing validation seems wrong. In Kilo, we required
> nova-specs for relaxing v2.1 API validation like 
> https://review.openstack.org/#/c/126696/ and we had much enough
> discussion and we built a consensus about that. But we merged the
> above patch in just 2 working days without any nova-spec even if we
> didn't have a consensus about that v2.1 validation change requires
> microversion bump or not.
> 
> If we really need to relax validation thing for v2.0 compatible API, 
> please consider separating v2.0 API schema from v2.1 API schema. I
> have one idea about that like
> https://review.openstack.org/#/c/221129/
> 
> We worked for strict and consistent validation way on v2.1 API over
> 2 years, and I don't want to make it loose without enough thinking.

There also was no field data about what it broke. The strict schema's
were based on an assumed understanding of how people were interacting
with the API. But that wasn't tested until we merged a change to make
everyone in OpenStack use it.

And we found a couple of bugs in our assumptions. Those issues were
blocking other parts of the OpenStack ecosystem from merging any code.
Which is a pretty big deal. We did a lot of thinking about this one. It
also went to a Nova meeting and got discussed there.

I'm also in favor of the v2.0 schema patch, I just +2ed it. That doesn't
mean that we don't address real issues that will inhibit adoption. The
promise of v2.1 is that it was going to be the same surface as v2.0
except that stuff no one ever should have sent, or would send, would be
rejected on the surface. Which is a win from code complexity and
security. But in this particular case the schema made the wrong call
about how people were actually using this API. So we fixed that.

Being responsive to real users that we end up breaking by accident is
important.

	-Sean

-- 
Sean Dague
http://dague.net