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