<div dir="ltr"><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19)"><p class="gmail-MsoPlainText">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.</p><p class="gmail-MsoPlainText"><span></span></p>
<p class="gmail-MsoPlainText">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.</p><p class="gmail-MsoPlainText"><span></span></p>
<p class="gmail-MsoPlainText">But operator/users creating images with visibility as "private"
*explicitly*, this changes is going to break them:<span></span></p>
<p class="gmail-MsoPlainText"> - Images
with member already added in that would not works as Tempest tests does [2].<span></span></p>
<p class="gmail-MsoPlainText"> - They
cannot add member as they used to do that.<span></span></p>
<p class="gmail-MsoPlainText">First one is being something tested by Tempest and which
is valid tests as per current behaviour of API<span></span></p>
<p class="gmail-MsoPlainText">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).<span></span></p><p class="gmail-MsoPlainText">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?</p><p class="gmail-MsoPlainText">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. <br></p><p class="gmail-MsoPlainText"><span></span></p>
<p class="gmail-MsoPlainText"><span>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. </span></p><p class="gmail-MsoPlainText">.. [1] <a href="https://review.openstack.org/#/c/412731/">https://review.openstack.org/#/c/412731/</a> <br></p>
<p class="gmail-MsoPlainText">.. [2] <a href="https://github.com/openstack/tempest/blob/master/tempest/api/image/v2/test_images.py#L339">https://github.com/openstack/tempest/blob/master/tempest/api/image/v2/test_images.py#L339</a>
<span></span></p>
</div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature"><div class="gmail_default" style="font-family:monospace,monospace;color:rgb(39,78,19);display:inline">-gmann</div></div></div>
<br><div class="gmail_quote">On Fri, Dec 23, 2016 at 5:51 AM, Matthew Treinish <span dir="ltr"><<a href="mailto:mtreinish@kortar.org" target="_blank">mtreinish@kortar.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 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 breaking<br>
existing users is **never** the right answer. You'll never get full coverage of<br>
end users by just asking for feedback on the ML and IRC meetings. Heck, I hadn't<br>
heard of any of these proposed changes until that tempest review, and I'm 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 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 break<br>
our users is kinda important.<br>
<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 model<br>
in glance make much more sense it doesn't change that it will break glance's<br>
api contract, tempest is just showing that. Changing the tests so it doesn't<br>
expose this goes against a big part of what tempest is there for.<br>
<br>
What really needs to happen is you need to make the changes in the visibility<br>
workflow happen in a way that doesn't break users. 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 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/evaluating_api_changes.html#guidance" rel="noreferrer" target="_blank">http://specs.openstack.org/<wbr>openstack/api-wg/guidelines/<wbr>evaluating_api_changes.html#<wbr>guidance</a><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>
<br></blockquote></div><br></div></div>