<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">Sorry I could not attend meeting due to TZ.</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">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.</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">I do not think this is the case, Tempest test is doing what API tells/behave. Below is what test does:</div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"> 1. Tenant A creates image with explicitly visibility as private. </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"> 2. Tenant A add Tenant B as member of created image to allow Tenant B to use that. </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">API [2] or Image sharing workflow [3] does not say/recommend anywhere that Image should not be created with private visibility as explicitly. </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">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. </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">Also changing tempest tests does not solve the problem, backward incompatible is still will be introduced in API. </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"><br></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">.. 1 <a href="http://git.openstack.org/cgit/openstack/tempest/tree/tempest/api/image/v2/test_images.py#n338">http://git.openstack.org/cgit/openstack/tempest/tree/tempest/api/image/v2/test_images.py#n338</a>  </div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">.. 2 <a href="http://developer.openstack.org/api-ref/image/v2/#create-an-image">http://developer.openstack.org/api-ref/image/v2/#create-an-image</a></div><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)">.. 3 <a href="http://specs.openstack.org/openstack/glance-specs/specs/api/v2/sharing-image-api-v2.html">http://specs.openstack.org/openstack/glance-specs/specs/api/v2/sharing-image-api-v2.html</a> </div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature">Regards<br>Ghanshyam Mann<br>+818011120698</div></div>
<br><div class="gmail_quote">On Mon, Jan 9, 2017 at 10:30 PM, Brian Rosmaita <span dir="ltr"><<a href="mailto:rosmaita.fossdev@gmail.com" target="_blank">rosmaita.fossdev@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 1/5/17 10:19 AM, Brian Rosmaita wrote:<br>
> To anyone interested in this discussion: I put it on the agenda for<br>
> today's API-WG meeting (16:00 UTC):<br>
><br>
> <a href="https://wiki.openstack.org/wiki/Meetings/API-WG#Agenda" rel="noreferrer" target="_blank">https://wiki.openstack.org/<wbr>wiki/Meetings/API-WG#Agenda</a><br>
<br>
As you probably noticed in the API-WG newsletter [A], this issue was<br>
discussed at last week's API-WG meeting.  The complete discussion is in<br>
the meeting log [B], but the tldr; is that the proposed change is<br>
acceptable.  I'll address specific points inline for those who are<br>
interested, but the key request from the Glance team right now is that<br>
the QA team approve this patch:<br>
<br>
<a href="https://review.openstack.org/#/c/414261/" rel="noreferrer" target="_blank">https://review.openstack.org/#<wbr>/c/414261/</a><br>
<br>
<br>
[A]<br>
<a href="http://lists.openstack.org/pipermail/openstack-dev/2017-January/109698.html" rel="noreferrer" target="_blank">http://lists.openstack.org/<wbr>pipermail/openstack-dev/2017-<wbr>January/109698.html</a><br>
[B]<br>
<a href="http://eavesdrop.openstack.org/meetings/api_wg/2017/api_wg.2017-01-05-16.00.log.html" rel="noreferrer" target="_blank">http://eavesdrop.openstack.<wbr>org/meetings/api_wg/2017/api_<wbr>wg.2017-01-05-16.00.log.html</a><br>
<br>
> On 12/25/16 12:04 PM, GHANSHYAM MANN wrote:<br>
>> Thanks Brian for bringing this up, same we been discussing last week on QA<br>
>> channel and this patch[1] but I completely agree with Matthew opinion here.<br>
>> There is no doubt that this change(4-valued) are much better and clear than<br>
>> old one. This makes much defined and clear way of defining the image<br>
>> visibility by/for operator/user.<br>
<br>
Yes, we think that the change clarifies the visibility semantics of<br>
images and, in particular, fixes the problem of there being "private"<br>
images that aren't actually private.<br>
<br>
The current situation is easily misunderstood by end users, as evidenced<br>
by bugs that have been filed, for example,<br>
<a href="https://bugs.launchpad.net/glance/+bug/1394299" rel="noreferrer" target="_blank">https://bugs.launchpad.net/<wbr>glance/+bug/1394299</a><br>
<a href="https://bugs.launchpad.net/glance/+bug/1452443" rel="noreferrer" target="_blank">https://bugs.launchpad.net/<wbr>glance/+bug/1452443</a><br>
<br>
>> Upgrade procedure defined in all referenced ML/spec looks fine for<br>
>> redefining the visibility for images with or without members to<br>
>> shared/private. Operator feedback/acceptance for this change makes it<br>
>> acceptable.<br>
<br>
Thanks, we discussed this thoroughly and solicited operator feedback.<br>
<br>
>> But operator/users creating images with visibility as "private"<br>
>> *explicitly*, this changes is going to break them:<br>
>><br>
>>                 - Images with member already added in that would not works<br>
>> as Tempest tests does [2].<br>
>><br>
>>                 - They cannot add member as they used to do that.<br>
<br>
Yes, we recognize that this change will introduce an incompatibility in<br>
the workflow for users who are setting visibility explicitly to<br>
'private' upon image creation.  The positive side to this change,<br>
however, is that when a user requests a private image, that's what<br>
they'll get.  It's important to note that the default visibility value<br>
will allow the current create-and-immediately-share workflow to continue<br>
exactly as it does now.<br>
<br>
One way to see how small this change is in context is to look at how it<br>
will appear in release notes:<br>
<br>
<a href="https://etherpad.openstack.org/p/glance-ocata-sharing-release-note-draft" rel="noreferrer" target="_blank">https://etherpad.openstack.<wbr>org/p/glance-ocata-sharing-<wbr>release-note-draft</a><br>
<br>
The incompatibility you're worried about is explained at line 8.<br>
<br>
>> First one is being something tested by Tempest and which is valid tests as<br>
>> per current behaviour of API<br>
>><br>
>> There might be lot of operators will be doing the same and going to be<br>
>> broken after this. We really need to think about this change as API<br>
>> backward incompatible pov where upgrade Cloud with new visibility<br>
>> definition is all ok but it break the way of usage(Image of Private<br>
>> visibility explicitly with added members).<br>
<br>
It's possible that some scripts will break, but it's important to note<br>
that the default visibility upon image creation will allow the current<br>
workflow to succeed.  While that's small consolation to those whose<br>
scripts may break, the plus side is that image visibility changes will<br>
now occur in a uniform way for all visibility values with no "magic"<br>
changes occurring as a side effect of a different API call.<br>
<br>
>> After looking into glance API versioning mechanism, it seems /v2 points to<br>
>> latest version irrespective of version includes backward compatible or<br>
>> incompatible changes. I mean users cannot lock API on old version (say they<br>
>> want only v2.2). How glance introduced backward incompatible changes?<br>
>><br>
>> I am not sure why it is not done like api-wg suggested microversion way<br>
>> (like nova). I know glance API versioning is much older but when there are<br>
>> some better improvement coming like this community image change, I feel it<br>
>> should be done with backward compatible way in microversion.<br>
<br>
Not everyone in the Glance community is on board with the microversion<br>
movement.  But also, I'm not convinced that microversions would be an<br>
acceptable solution to this situation.  From Ocata on, we want 'private'<br>
to mean "private"; users have to take an extra step to update the<br>
visibility to 'shared' before an image will accept members.  I don't<br>
think a user who expects that behavior will appreciate a pre-Ocata<br>
client that can bypass this safety mechanism and simply add members to<br>
private images.<br>
<br>
>> Tempest testing the old behaviour is something very valid here and we<br>
>> should not change that to introduced backward incompatible changes which<br>
>> going to break cloud.<br>
>><br>
>> .. [1] <a href="https://review.openstack.org/#/c/412731/" rel="noreferrer" target="_blank">https://review.openstack.org/#<wbr>/c/412731/</a><br>
>><br>
>> .. [2]<br>
>> <a href="https://github.com/openstack/tempest/blob/master/tempest/api/image/v2/test_images.py#L339" rel="noreferrer" target="_blank">https://github.com/openstack/<wbr>tempest/blob/master/tempest/<wbr>api/image/v2/test_images.py#<wbr>L339</a><br>
>><br>
>> ​-gmann<br>
>><br>
>> On Fri, Dec 23, 2016 at 5:51 AM, Matthew Treinish <<a href="mailto:mtreinish@kortar.org">mtreinish@kortar.org</a>><br>
>> wrote:<br>
>><br>
>>> On Thu, Dec 22, 2016 at 02:57:20PM -0500, Brian Rosmaita wrote:<br>
>>>> Something has come up with a tempest test for Glance and the community<br>
>>>> images implementation, and I think it could use some mailing list<br>
>>>> discussion, as everyone might not be aware of the patch where the<br>
>>>> discussion is happening now [1].<br>
>>>><br>
>>>> First, the Glance background, to get everyone on the same page:<br>
>>>><br>
>>>> As part of implementing community images [2], the 'visibility' field of<br>
>>>> an image is going from being 2-valued to being 4-valued.  Up to now, the<br>
>>>> only values have been 'private' and 'public', which meant that shared<br>
>>>> images were 'private', which was inaccurate and confusing (and bugs were<br>
>>>> filed with Glance about shared images not having visibility 'shared'<br>
>>>> [3a,b]).<br>
>>>><br>
>>>> With the new visibility enum, the Images API v2 will behave as follows:<br>
>>>><br>
>>>> * An image with visibility == 'private' is not shared, and is not<br>
>>>> shareable until its visibility is changed to 'shared'.<br>
>>>><br>
>>>> * An image must have visibility == 'shared' in order to do member<br>
>>>> operations or be accessible to image members.<br>
>>>><br>
>>>> * The default visibility of a freshly-created image is 'shared'.  This<br>
>>>> may seem weird, but a freshly-created image has no members, so it's<br>
>>>> effectively private, behaving exactly as a freshly-created image does,<br>
>>>> pre-Ocata.  It's also ready to immediately accept a member-create call,<br>
>>>> as freshly-created images are pre-Ocata.  So from a workflow<br>
>>>> perspective, this change is backward compatible.<br>
>>>><br>
>>>> * After much discussion [4], including discussion with operators and an<br>
>>>> operator's survey [5], we decided that the correct migration of<br>
>>>> 'visibility' values for existing images when a cloud is updated would<br>
>>>> be: public images stay 'public', private images with members become<br>
>>>> 'shared', and private images without images stay 'private'.  (Thus, if<br>
>>>> you have a 'private' image, you'll have to change it to 'shared' before<br>
>>>> you can add members.  On the other hand, now it's *really* private.)<br>
>>>><br>
>>>> * You can specify a visibility at the time of image-creation, as you can<br>
>>>> now.  But if you specify 'private', what you get is *really* private.<br>
>>>> This either introduces a minor backward incompatibility, or it fixes a<br>
>>>> bug, depending on how you look at it.  The key thing is, if you *don't*<br>
>>>> specify a visibility, an image with the default visibility will behave<br>
>>>> exactly as it does now, from the perspective of being able to make API<br>
>>>> calls on it (e.g., adding members).<br>
>>>><br>
>>>> Thanks for reading this far.  (There's a much more detailed discussion<br>
>>>> in the spec; see the "Other end user impact" section. [2])  Here's the<br>
>>>> point of this email:<br>
>>>><br>
>>>> The community images patch [6] is causing a recently added tempest test<br>
>>>> [7] to fail.  The test in question uses an image that was created by a<br>
>>>> request that explicitly specified visibility == private.  Eventually it<br>
>>>> tries to add a member to this image, and as discussed above, this<br>
>>>> operation will fail once we have merged Community Images (because the<br>
>>>> image visibility is not 'shared').  If the image had been created with<br>
>>>> the default visibility (that is, not explicitly specifying a visibility<br>
>>>> in the image-create call), this problem would not arise.  Keep in mind<br>
>>>> that prior to Ocata, there was no reason for end users to specify an<br>
>>>> image visibility explicitly upon image creation because there were only<br>
>>>> two possible values, one of which required special permissions in order<br>
>>>> to use successfully.<br>
>>><br>
>>> While you say there was no reason for a user to do it was still part of the<br>
>>> glance API and part of your contract with end users. It's something anyone<br>
>>> could be doing today, (which is obvious because tempest is doing it)<br>
>>> regardless of whether you think there is a use case for it or not. The<br>
>>> whole<br>
>>> point of a stable api is that you don't break things like this. I'd really<br>
>>> recommend reading Sean Dague's blog post here about Nova's api here:<br>
>>><br>
>>> <a href="https://dague.net/2015/06/05/the-nova-api-in-kilo-and-beyond-2/" rel="noreferrer" target="_blank">https://dague.net/2015/06/05/<wbr>the-nova-api-in-kilo-and-<wbr>beyond-2/</a><br>
>>><br>
>>> because it does a very good job explaining typical api use cases and how to<br>
>>> think about api compatibility.<br>
>>><br>
>>>> Thus, we feel that the situation occurring in the<br>
>>>> test is not one that many end users will come across.  We have discussed<br>
>>>> the situation extensively with the broader OpenStack community, and the<br>
>>>> consensus is that this change to the API is acceptable.<br>
>>><br>
>>> The guidelines for API compatiblity [1] are there for a reason, and<br>
>>> breaking<br>
>>> existing users is **never** the right answer. You'll never get full<br>
>>> coverage of<br>
>>> end users by just asking for feedback on the ML and IRC meetings. Heck, I<br>
>>> hadn't<br>
>>> heard of any of these proposed changes until that tempest review, and I'm<br>
>>> hardly<br>
>>> a disconnected user. The thing to remember is that all APIs have warts and<br>
>>> aspects which are less than ideal, our first priority when making<br>
>>> improvements<br>
>>> should be to not randomly break our users in the process. If the goal of<br>
>>> OpenStack is to provide an interoperable cloud platform, ensuring we don't<br>
>>> break<br>
>>> our users is kinda important.<br>
<br>
You make good points, but we did discuss this carefully with the API-WG<br>
back in June, and again last week, and they are not opposed to this<br>
change.  It's neither a random breakage of users or polishing off a<br>
wart.  It's non-random because the default settings will allow the<br>
current workflow to proceed successfully, and if you specifically<br>
request a 'private' image, that's what you'll get.  It's not a simple<br>
wart removal because the current situation with image visibility is that<br>
it's conceptually incoherent and this fixes it for current and future<br>
API consumers.<br>
<br>
>>>> To conclude: before and after this change, the "default" visibility of<br>
>>>> an image will allow adding members to the image. We would hope that the<br>
>>>> failing tempest test could be revised to not explicitly request<br>
>>>> "private" visibility but to allow Glance to use its default visibility<br>
>>>> instead [10]. We have wide cross-project support [8, 9] for the<br>
>>>> "Community Images" work and the only thing blocking us now is tempest.<br>
>>>> Due to the shortened cycle in Ocata, we'd really appreciate finding<br>
>>>> common ground with the QA team quickly.<br>
>>>><br>
>>><br>
>>> The crux of the issue here is that you are changing the API in a backwards<br>
>>> incompatible way. Tempest is actually doing it's job and correctly blocking<br>
>>> you from landing that change because it will break users who are using this<br>
>>> today. While I agree the new approach is useful and makes the visibility<br>
>>> model<br>
>>> in glance make much more sense it doesn't change that it will break<br>
>>> glance's<br>
>>> api contract, tempest is just showing that. Changing the tests so it<br>
>>> doesn't<br>
>>> expose this goes against a big part of what tempest is there for.<br>
<br>
I agree, Tempest is doing its part as a guardrail, and it did detect a<br>
change.  However, on balance, this is a worthwhile change and much of<br>
the community is on board with it, so we're proposing an exception.<br>
<br>
>>> What really needs to happen is you need to make the changes in the<br>
>>> visibility<br>
>>> workflow happen in a way that doesn't break users.<br>
<br>
The problem is, as I mentioned above, is that in this situation we *do*<br>
want to break users.  Adding members to an image with 'private'<br>
visibility should return a 409.  Otherwise, we're working with a very<br>
misleading notion of what "private" means, and that's not good for<br>
current or future API consumers.<br>
<br>
>>> I can see 2 ways to do this:<br>
>>><br>
>>> 1. Version the API and add a mechanism to opt in to the new behavior. (ie<br>
>>> microversions) The microversion framework was specifically designed to<br>
>>> solve<br>
>>> this exact issue and to enable evolving an API in a backwards compatible<br>
>>> manner.<br>
>>><br>
>>> 2. You could very easily make the operation of adding a member to an image<br>
>>> created with private visibility automatically change it's visibility to<br>
>>> shared. While this is a change in behavior it wouldn't break users.<br>
>>><br>
>>> Personally, I think you should probably do both to make it opt-in and also<br>
>>> happen automatically.<br>
>>><br>
>>> -Matt Treinish<br>
>>><br>
>>> [1] <a href="http://specs.openstack.org/openstack/api-wg/guidelines/" rel="noreferrer" target="_blank">http://specs.openstack.org/<wbr>openstack/api-wg/guidelines/</a><br>
>>> evaluating_api_changes.html#<wbr>guidance<br>
<br>
We appreciate all the thought and time you've put into thinking about<br>
this, but we respectfully point out that we've put in a lot of thought<br>
and time, too, and we're convinced that this is an appropriate and<br>
acceptable change.<br>
<br>
cheers,<br>
brian<br>
<br>
<br>
______________________________<wbr>______________________________<wbr>______________<br>
OpenStack Development Mailing List (not for usage questions)<br>
Unsubscribe: <a href="http://OpenStack-dev-request@lists.openstack.org?subject:unsubscribe" rel="noreferrer" target="_blank">OpenStack-dev-request@lists.<wbr>openstack.org?subject:<wbr>unsubscribe</a><br>
<a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev" rel="noreferrer" target="_blank">http://lists.openstack.org/<wbr>cgi-bin/mailman/listinfo/<wbr>openstack-dev</a><br>
</blockquote></div><br></div>