Open Stack

Ken,

Thanks for the quick response ... we'll follow the procedure outlined in
the tempest HACKING.rst that you pointed out.

cheers,
brian

On 1/12/17 2:32 PM, Ken'ichi Ohmichi wrote:
> 2017-01-12 5:47 GMT-08:00 Brian Rosmaita <rosmaita.fossdev at gmail.com>:
>> On 1/11/17 10:35 PM, GHANSHYAM MANN wrote:
>>
>>> But from meeting logs, it looks like impression was(if am not wrong) that
>>> Tempest test[1] is not doing the right thing and should be ok to change.
>>> I do not think this is the case, Tempest test is doing what API
>>> tells/behave. Below is what test does:
>>>  1. Tenant A creates image with explicitly visibility as private.
>>>  2. Tenant A add Tenant B as member of created image to allow Tenant B to
>>> use that.
>>>
>>> API [2] or Image sharing workflow [3] does not say/recommend anywhere that
>>> Image should not be created with private visibility as explicitly.
>>>
>>> For me this change breaks people "Creating Image with private visibility as
>>> *explicitly* and adding member to that" which will be 409 after glance
>>> proposal.
>>>
>>>
>>> Also changing tempest tests does not solve the problem, backward
>>> incompatible is still will be introduced in API.
>>
>> The issue is that we are proposing to introduce an incompatible change
>> in order to address an incoherence with image visibility semantics.  We
>> have acknowledged that this is a breaking change, but the impact of the
>> change is mitigated by the way that the default visibility value is
>> assigned in Ocata.
>>
>> As I've documented earlier in the thread, we have discussed this change
>> with the wider community and the API Working Group, and they are OK with it.
>>
>> The current tempest test has done its duty and identified an
>> incompatibility in the Ocata code.  We acknowledge that and salute you.
>> On balance, however, this change will be better for users (and as I've
>> documented earlier in the thread, we've minimized the impact of the
>> change), and we want to make it anyway.
> 
> It is difficult to expect the impact of API change is small by us as developers.
> For example, if there could be some APPs which list images of both
> private and shared by depending on
> https://bugs.launchpad.net/glance/+bug/1394299 , the APPs will be
> broken after fixing it.
> Nova team faced such situation we expected the impact of the change
> was small but horizon was broken, then we reverted the change in the
> same dev cycle.
> Then Nova has now API microversions mechanism to avoid impacting to
> the existing APPs.
> 
> This is on very difficult balance, and we don't want to block the
> glance team development.
> We have some procedure for this situation like
> https://github.com/openstack/tempest/blob/master/HACKING.rst#2-bug-fix-on-core-project-needing-tempest-changes
> I did put some comments on https://review.openstack.org/#/c/414261 .
> Could you check that?
> 
> Thanks
> Ken Ohmichi
> 
> ---
> 
>> So the situation isn't that we are claiming that your current code is
>> flawed.  Rather, it is that we are asking the QA team to approve a
>> change to that test in order to address a change we are making in Ocata,
>> a change that has the support of the OpenStack community.
>>
>> thanks,
>> brian
>>
>>>
>>> .. 1
>>> http://git.openstack.org/cgit/openstack/tempest/tree/tempest/api/image/v2/test_images.py#n338
>>>
>>> .. 2 http://developer.openstack.org/api-ref/image/v2/#create-an-image
>>> .. 3
>>> http://specs.openstack.org/openstack/glance-specs/specs/api/v2/sharing-image-api-v2.html
>>>
>>>
>>> Regards
>>> Ghanshyam Mann
>>> +818011120698
>>>
>>>
>>> On Mon, Jan 9, 2017 at 10:30 PM, Brian Rosmaita <rosmaita.fossdev at gmail.com>
>>> wrote:
>>>
>>>> On 1/5/17 10:19 AM, Brian Rosmaita wrote:
>>>>> 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
>>>>
>>>> As you probably noticed in the API-WG newsletter [A], this issue was
>>>> discussed at last week's API-WG meeting.  The complete discussion is in
>>>> the meeting log [B], but the tldr; is that the proposed change is
>>>> acceptable.  I'll address specific points inline for those who are
>>>> interested, but the key request from the Glance team right now is that
>>>> the QA team approve this patch:
>>>>
>>>> https://review.openstack.org/#/c/414261/
>>>>
>>>>
>>>> [A]
>>>> http://lists.openstack.org/pipermail/openstack-dev/2017-
>>>> January/109698.html
>>>> [B]
>>>> http://eavesdrop.openstack.org/meetings/api_wg/2017/api_
>>>> wg.2017-01-05-16.00.log.html
>>>>
>>>>> 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.
>>>>
>>>> Yes, we think that the change clarifies the visibility semantics of
>>>> images and, in particular, fixes the problem of there being "private"
>>>> images that aren't actually private.
>>>>
>>>> The current situation is easily misunderstood by end users, as evidenced
>>>> by bugs that have been filed, for example,
>>>> https://bugs.launchpad.net/glance/+bug/1394299
>>>> https://bugs.launchpad.net/glance/+bug/1452443
>>>>
>>>>>> 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.
>>>>
>>>> Thanks, we discussed this thoroughly and solicited operator feedback.
>>>>
>>>>>> 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.
>>>>
>>>> Yes, we recognize that this change will introduce an incompatibility in
>>>> the workflow for users who are setting visibility explicitly to
>>>> 'private' upon image creation.  The positive side to this change,
>>>> however, is that when a user requests a private image, that's what
>>>> they'll get.  It's important to note that the default visibility value
>>>> will allow the current create-and-immediately-share workflow to continue
>>>> exactly as it does now.
>>>>
>>>> One way to see how small this change is in context is to look at how it
>>>> will appear in release notes:
>>>>
>>>> https://etherpad.openstack.org/p/glance-ocata-sharing-release-note-draft
>>>>
>>>> The incompatibility you're worried about is explained at line 8.
>>>>
>>>>>> 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).
>>>>
>>>> It's possible that some scripts will break, but it's important to note
>>>> that the default visibility upon image creation will allow the current
>>>> workflow to succeed.  While that's small consolation to those whose
>>>> scripts may break, the plus side is that image visibility changes will
>>>> now occur in a uniform way for all visibility values with no "magic"
>>>> changes occurring as a side effect of a different API call.
>>>>
>>>>>> 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.
>>>>
>>>> Not everyone in the Glance community is on board with the microversion
>>>> movement.  But also, I'm not convinced that microversions would be an
>>>> acceptable solution to this situation.  From Ocata on, we want 'private'
>>>> to mean "private"; users have to take an extra step to update the
>>>> visibility to 'shared' before an image will accept members.  I don't
>>>> think a user who expects that behavior will appreciate a pre-Ocata
>>>> client that can bypass this safety mechanism and simply add members to
>>>> private images.
>>>>
>>>>>> 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.
>>>>
>>>> You make good points, but we did discuss this carefully with the API-WG
>>>> back in June, and again last week, and they are not opposed to this
>>>> change.  It's neither a random breakage of users or polishing off a
>>>> wart.  It's non-random because the default settings will allow the
>>>> current workflow to proceed successfully, and if you specifically
>>>> request a 'private' image, that's what you'll get.  It's not a simple
>>>> wart removal because the current situation with image visibility is that
>>>> it's conceptually incoherent and this fixes it for current and future
>>>> API consumers.
>>>>
>>>>>>>> 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.
>>>>
>>>> I agree, Tempest is doing its part as a guardrail, and it did detect a
>>>> change.  However, on balance, this is a worthwhile change and much of
>>>> the community is on board with it, so we're proposing an exception.
>>>>
>>>>>>> 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.
>>>>
>>>> The problem is, as I mentioned above, is that in this situation we *do*
>>>> want to break users.  Adding members to an image with 'private'
>>>> visibility should return a 409.  Otherwise, we're working with a very
>>>> misleading notion of what "private" means, and that's not good for
>>>> current or future API consumers.
>>>>
>>>>>>> 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
>>>>
>>>> We appreciate all the thought and time you've put into thinking about
>>>> this, but we respectfully point out that we've put in a lot of thought
>>>> and time, too, and we're convinced that this is an appropriate and
>>>> acceptable change.
>>>>
>>>> cheers,
>>>> brian
>>
>>
>> __________________________________________________________________________
>> 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
>