Open Stack

(Top-posting, don't bother digging through for more.)

We're following the tempest HACKING.rst procedure.  The patch for step
#2 is: https://review.openstack.org/#/c/419658/

Please review at your earliest convenience.

thanks,
brian


On 1/12/17 3:34 PM, Brian Rosmaita wrote:
> 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
>>
>