(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.foss...@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.foss...@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 <mtrein...@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-requ...@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-requ...@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-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev