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