Open Stack

On 1/11/17 10:35 PM, GHANSHYAM MANN wrote:
> Sorry I could not attend meeting due to TZ.

I wound up missing the meeting, too; I thought it was at 17:00 UTC.

> 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.

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