Open Stack

To anyone interested in this discussion: I put it on the agenda for
today's API-WG meeting (16:00 UTC):

https://wiki.openstack.org/wiki/Meetings/API-WG#Agenda


On 12/25/16 12:04 PM, GHANSHYAM MANN wrote:
> Thanks Brian for bringing this up, same we been discussing last week on QA
> channel and this patch[1] but I completely agree with Matthew opinion here.
> There is no doubt that this change(4-valued) are much better and clear than
> old one. This makes much defined and clear way of defining the image
> visibility by/for operator/user.
> 
> Upgrade procedure defined in all referenced ML/spec looks fine for
> redefining the visibility for images with or without members to
> shared/private. Operator feedback/acceptance for this change makes it
> acceptable.
> 
> But operator/users creating images with visibility as "private"
> *explicitly*, this changes is going to break them:
> 
>                 - Images with member already added in that would not works
> as Tempest tests does [2].
> 
>                 - They cannot add member as they used to do that.
> 
> First one is being something tested by Tempest and which is valid tests as
> per current behaviour of API
> 
> There might be lot of operators will be doing the same and going to be
> broken after this. We really need to think about this change as API
> backward incompatible pov where upgrade Cloud with new visibility
> definition is all ok but it break the way of usage(Image of Private
> visibility explicitly with added members).
> 
> After looking into glance API versioning mechanism, it seems /v2 points to
> latest version irrespective of version includes backward compatible or
> incompatible changes. I mean users cannot lock API on old version (say they
> want only v2.2). How glance introduced backward incompatible changes?
> 
> I am not sure why it is not done like api-wg suggested microversion way
> (like nova). I know glance API versioning is much older but when there are
> some better improvement coming like this community image change, I feel it
> should be done with backward compatible way in microversion.
> 
> Tempest testing the old behaviour is something very valid here and we
> should not change that to introduced backward incompatible changes which
> going to break cloud.
> 
> .. [1] https://review.openstack.org/#/c/412731/
> 
> .. [2]
> https://github.com/openstack/tempest/blob/master/tempest/api/image/v2/test_images.py#L339
> 
> ​-gmann
> 
> On Fri, Dec 23, 2016 at 5:51 AM, Matthew Treinish <mtreinish at kortar.org>
> wrote:
> 
>> On Thu, Dec 22, 2016 at 02:57:20PM -0500, Brian Rosmaita wrote:
>>> Something has come up with a tempest test for Glance and the community
>>> images implementation, and I think it could use some mailing list
>>> discussion, as everyone might not be aware of the patch where the
>>> discussion is happening now [1].
>>>
>>> First, the Glance background, to get everyone on the same page:
>>>
>>> As part of implementing community images [2], the 'visibility' field of
>>> an image is going from being 2-valued to being 4-valued.  Up to now, the
>>> only values have been 'private' and 'public', which meant that shared
>>> images were 'private', which was inaccurate and confusing (and bugs were
>>> filed with Glance about shared images not having visibility 'shared'
>>> [3a,b]).
>>>
>>> With the new visibility enum, the Images API v2 will behave as follows:
>>>
>>> * An image with visibility == 'private' is not shared, and is not
>>> shareable until its visibility is changed to 'shared'.
>>>
>>> * An image must have visibility == 'shared' in order to do member
>>> operations or be accessible to image members.
>>>
>>> * The default visibility of a freshly-created image is 'shared'.  This
>>> may seem weird, but a freshly-created image has no members, so it's
>>> effectively private, behaving exactly as a freshly-created image does,
>>> pre-Ocata.  It's also ready to immediately accept a member-create call,
>>> as freshly-created images are pre-Ocata.  So from a workflow
>>> perspective, this change is backward compatible.
>>>
>>> * After much discussion [4], including discussion with operators and an
>>> operator's survey [5], we decided that the correct migration of
>>> 'visibility' values for existing images when a cloud is updated would
>>> be: public images stay 'public', private images with members become
>>> 'shared', and private images without images stay 'private'.  (Thus, if
>>> you have a 'private' image, you'll have to change it to 'shared' before
>>> you can add members.  On the other hand, now it's *really* private.)
>>>
>>> * You can specify a visibility at the time of image-creation, as you can
>>> now.  But if you specify 'private', what you get is *really* private.
>>> This either introduces a minor backward incompatibility, or it fixes a
>>> bug, depending on how you look at it.  The key thing is, if you *don't*
>>> specify a visibility, an image with the default visibility will behave
>>> exactly as it does now, from the perspective of being able to make API
>>> calls on it (e.g., adding members).
>>>
>>> Thanks for reading this far.  (There's a much more detailed discussion
>>> in the spec; see the "Other end user impact" section. [2])  Here's the
>>> point of this email:
>>>
>>> The community images patch [6] is causing a recently added tempest test
>>> [7] to fail.  The test in question uses an image that was created by a
>>> request that explicitly specified visibility == private.  Eventually it
>>> tries to add a member to this image, and as discussed above, this
>>> operation will fail once we have merged Community Images (because the
>>> image visibility is not 'shared').  If the image had been created with
>>> the default visibility (that is, not explicitly specifying a visibility
>>> in the image-create call), this problem would not arise.  Keep in mind
>>> that prior to Ocata, there was no reason for end users to specify an
>>> image visibility explicitly upon image creation because there were only
>>> two possible values, one of which required special permissions in order
>>> to use successfully.
>>
>> While you say there was no reason for a user to do it was still part of the
>> glance API and part of your contract with end users. It's something anyone
>> could be doing today, (which is obvious because tempest is doing it)
>> regardless of whether you think there is a use case for it or not. The
>> whole
>> point of a stable api is that you don't break things like this. I'd really
>> recommend reading Sean Dague's blog post here about Nova's api here:
>>
>> https://dague.net/2015/06/05/the-nova-api-in-kilo-and-beyond-2/
>>
>> because it does a very good job explaining typical api use cases and how to
>> think about api compatibility.
>>
>>> Thus, we feel that the situation occurring in the
>>> test is not one that many end users will come across.  We have discussed
>>> the situation extensively with the broader OpenStack community, and the
>>> consensus is that this change to the API is acceptable.
>>
>> The guidelines for API compatiblity [1] are there for a reason, and
>> breaking
>> existing users is **never** the right answer. You'll never get full
>> coverage of
>> end users by just asking for feedback on the ML and IRC meetings. Heck, I
>> hadn't
>> heard of any of these proposed changes until that tempest review, and I'm
>> hardly
>> a disconnected user. The thing to remember is that all APIs have warts and
>> aspects which are less than ideal, our first priority when making
>> improvements
>> should be to not randomly break our users in the process. If the goal of
>> OpenStack is to provide an interoperable cloud platform, ensuring we don't
>> break
>> our users is kinda important.
>>
>>>
>>> To conclude: before and after this change, the "default" visibility of
>>> an image will allow adding members to the image. We would hope that the
>>> failing tempest test could be revised to not explicitly request
>>> "private" visibility but to allow Glance to use its default visibility
>>> instead [10]. We have wide cross-project support [8, 9] for the
>>> "Community Images" work and the only thing blocking us now is tempest.
>>> Due to the shortened cycle in Ocata, we'd really appreciate finding
>>> common ground with the QA team quickly.
>>>
>>
>> The crux of the issue here is that you are changing the API in a backwards
>> incompatible way. Tempest is actually doing it's job and correctly blocking
>> you from landing that change because it will break users who are using this
>> today. While I agree the new approach is useful and makes the visibility
>> model
>> in glance make much more sense it doesn't change that it will break
>> glance's
>> api contract, tempest is just showing that. Changing the tests so it
>> doesn't
>> expose this goes against a big part of what tempest is there for.
>>
>> What really needs to happen is you need to make the changes in the
>> visibility
>> workflow happen in a way that doesn't break users. I can see 2 ways to do
>> this:
>>
>> 1. Version the API and add a mechanism to opt in to the new behavior. (ie
>> microversions) The microversion framework was specifically designed to
>> solve
>> this exact issue and to enable evolving an API in a backwards compatible
>> manner.
>>
>> 2. You could very easily make the operation of adding a member to an image
>> created with private visibility automatically change it's visibility to
>> shared. While this is a change in behavior it wouldn't break users.
>>
>> Personally, I think you should probably do both to make it opt-in and also
>> happen automatically.
>>
>> -Matt Treinish
>>
>> [1] http://specs.openstack.org/openstack/api-wg/guidelines/
>> evaluating_api_changes.html#guidance
>>
>> __________________________________________________________________________
>> OpenStack Development Mailing List (not for usage questions)
>> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>
>>
> 
> 
> 
> __________________________________________________________________________
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: OpenStack-dev-request at lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>