)]}'
{"glance/api/v1/images.py":[{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":437,"context_line":"    def _get_from_store(context, where):"},{"line_number":438,"context_line":"        try:"},{"line_number":439,"context_line":"            image_data, image_size \u003d get_from_backend(context, where)"},{"line_number":440,"context_line":"        except (exception.NotFound, exception.ImageDataNotFound) as e:"},{"line_number":441,"context_line":"            msg \u003d unicode(e)"},{"line_number":442,"context_line":"            LOG.debug(msg)"},{"line_number":443,"context_line":"            raise HTTPNotFound(explanation\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2HE%3D","line":440,"updated":"2013-12-30 09:43:14.000000000","message":"I think get_from_backend() will never raise ImageDataNotFound exception out, am I right?","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":437,"context_line":"    def _get_from_store(context, where):"},{"line_number":438,"context_line":"        try:"},{"line_number":439,"context_line":"            image_data, image_size \u003d get_from_backend(context, where)"},{"line_number":440,"context_line":"        except (exception.NotFound, exception.ImageDataNotFound) as e:"},{"line_number":441,"context_line":"            msg \u003d unicode(e)"},{"line_number":442,"context_line":"            LOG.debug(msg)"},{"line_number":443,"context_line":"            raise HTTPNotFound(explanation\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fz2Y%3D","line":440,"in_reply_to":"AAAAT3%2F%2F2HE%3D","updated":"2013-12-31 06:48:17.000000000","message":"No it won\u0027t - I must\u0027ve gotten confused between v1 and v2! I\u0027ll remove it","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"}],"glance/api/v2/image_data.py":[{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        try:"},{"line_number":127,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":128,"context_line":"            if not image.locations:"},{"line_number":129,"context_line":"                raise exception.ImageDataNotFound()"},{"line_number":130,"context_line":"        except exception.ImageDataNotFound as e:"},{"line_number":131,"context_line":"            raise webob.exc.HTTPNoContent(explanation\u003dunicode(e))"},{"line_number":132,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2Es%3D","line":129,"updated":"2013-12-30 09:43:14.000000000","message":"we already handled unavailable locations case at here, so we don\u0027t need do that at #164.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        try:"},{"line_number":127,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":128,"context_line":"            if not image.locations:"},{"line_number":129,"context_line":"                raise exception.ImageDataNotFound()"},{"line_number":130,"context_line":"        except exception.ImageDataNotFound as e:"},{"line_number":131,"context_line":"            raise webob.exc.HTTPNoContent(explanation\u003dunicode(e))"},{"line_number":132,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fz18%3D","line":129,"in_reply_to":"AAAAT3%2F%2F2Es%3D","updated":"2013-12-31 06:48:17.000000000","message":"I think it\u0027s a different case. At this place, the exception is raised if the image.locations field is empty. But down in line 164, the exception is raised by the backend store - the location field is present but the file in the backend is missing.\n\nFor example I create a file and specify the location to be http://127.0.0.1:8000/not/exists. If during the time of creation of the image there is no web server listening on 127.0.0.1:8000, the image will be created successfully.\n\nNow if I bring up the server after the image has been created and try to download the file, there will be no exception here (because \u0027location\u0027 exists) but there will be an exception down in line 164 when the backend actually tries to get the image file (which does not exist on the backend server).","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"0106dce7c003e468ba0be76674a6c6d419aa4ace","unresolved":false,"context_lines":[{"line_number":126,"context_line":"        try:"},{"line_number":127,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":128,"context_line":"            if not image.locations:"},{"line_number":129,"context_line":"                raise exception.ImageDataNotFound()"},{"line_number":130,"context_line":"        except exception.ImageDataNotFound as e:"},{"line_number":131,"context_line":"            raise webob.exc.HTTPNoContent(explanation\u003dunicode(e))"},{"line_number":132,"context_line":"        except exception.NotFound as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FwlA%3D","line":129,"in_reply_to":"AAAAT3%2F%2Fz18%3D","updated":"2014-01-02 08:20:19.000000000","message":"Make sense to me. thanks for our explaining ;)","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":155,"context_line":"            # NOTE(markwash): filesystem store (and maybe others?) cause a"},{"line_number":156,"context_line":"            # problem with the caching middleware if they are not wrapped in"},{"line_number":157,"context_line":"            # an iterator very strange"},{"line_number":158,"context_line":"            image.prepare_data()"},{"line_number":159,"context_line":"            response.app_iter \u003d iter(image.get_data())"},{"line_number":160,"context_line":"        except exception.Forbidden as e:"},{"line_number":161,"context_line":"            msg \u003d unicode(e)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2E8%3D","line":158,"updated":"2013-12-30 09:43:14.000000000","message":"Actually I consider this BadStoreUir exception caching (possibly plus below common Exception caching) will resolve this defect completely. TBH I don\u0027t see the necessity which require us must change Image domain object.\n\nbtw, I prefer move below two BadStoreUri and Exception handling cases to above controller but serializer here.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":155,"context_line":"            # NOTE(markwash): filesystem store (and maybe others?) cause a"},{"line_number":156,"context_line":"            # problem with the caching middleware if they are not wrapped in"},{"line_number":157,"context_line":"            # an iterator very strange"},{"line_number":158,"context_line":"            image.prepare_data()"},{"line_number":159,"context_line":"            response.app_iter \u003d iter(image.get_data())"},{"line_number":160,"context_line":"        except exception.Forbidden as e:"},{"line_number":161,"context_line":"            msg \u003d unicode(e)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fzz8%3D","line":158,"in_reply_to":"AAAAT3%2F%2F2E8%3D","updated":"2013-12-31 06:48:17.000000000","message":"Yes, I would have preferred that too but this is due to the notifier Image proxy. I will try to explain what I was trying to do and what actually happened (so that you can see why I had to add the prepare_data() method).\n\nWhat I wanted and expected was:\n  0) the image object here is an instance of the authorization.ImageProxy class.\n  1) calling image.get_data() here would call notifier.ImageProxy.get_data()\n  2) ... and so on all the way to store.ImageProxy.get_data()\n  3) store.ImageProxy.get_data() would call the backend store\u0027s get() method\n    - if there are any problems (connection errors, file doesn\u0027t exist, etc.)\n      the backend would throw an exception which would be caught up here\n      in the serializer.\n\nBut, unfortunately, that is not what happens.\n\nThe notifier.ImageProxy.get_data() method has a \u0027yield\u0027 statement and so when the method is called in step (1) the notifier.ImageProxy.get_data() method returns *immediately* with a generator object *without* going through the remaining steps. In effect, the notifier.ImageProxy.get_data() function is *not* called and instead, a generator object is returned.\n\nThis generator object is passed on to the WSGI framework as the response to be sent back to the caller. The WSGI framework goes through the entire pipleline and finally starts sending the response back to the caller. It does this by iterating through this response object (which is now the generator object returned by notifier.ImageProxy.get_data()). It is *now* that the remaining calls to the get_data() methods are made. So if there are any backend errors, they are raised *at this stage*, where it\u0027s too late to catch them.\n\nSo we see that the root cause of this whole problem is that the data *setup* and the data *transfer* get coupled together by notifier.ImageProxy.get_data(), resulting in the setup happening too late when the WSGI framework tries to transfer data (at which stage it is too late to catch exceptions).\n\nTherefore what I wanted was to decouple the data setup and the data transfer - do the setup in the serializer and return the actual data iterator to the WSGI framework, and let the framework transfer the data at the right time. I believe/assume this is what was originally intended but the notifier.ImageProxy.get_data() method unintentionally broke the whole thing.\n\nSo that\u0027s why I added a new \"prepare_data\" method which would go all the way down to the store and set up the data and retrieve the data iterator. Once the data iterator has been retrieved, the get_data() method needs only to return this data iterator. This way we decouple the data setup and the data transfer.\n\nHope all this makes sense. Sorry for the long explanation!","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"6798d8ccba17a8fc1dc2aa11089435343f25901c","unresolved":false,"context_lines":[{"line_number":155,"context_line":"            # NOTE(markwash): filesystem store (and maybe others?) cause a"},{"line_number":156,"context_line":"            # problem with the caching middleware if they are not wrapped in"},{"line_number":157,"context_line":"            # an iterator very strange"},{"line_number":158,"context_line":"            image.prepare_data()"},{"line_number":159,"context_line":"            response.app_iter \u003d iter(image.get_data())"},{"line_number":160,"context_line":"        except exception.Forbidden as e:"},{"line_number":161,"context_line":"            msg \u003d unicode(e)"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FzYg%3D","line":158,"in_reply_to":"AAAAT3%2F%2Fzz8%3D","updated":"2013-12-31 06:50:59.000000000","message":"I have another idea which should be a little easier - shouldn\u0027t need to touch the domain.ImageProxy class, only the notifier.ImageProxy class. I\u0027ll test it out first and then upload a new patchset.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":170,"context_line":"            LOG.debug(msg)"},{"line_number":171,"context_line":"            raise webob.exc.HTTPBadGateway(msg)"},{"line_number":172,"context_line":"        except Exception as e:"},{"line_number":173,"context_line":"            log_msg \u003d (\"Caught unknown exception: \u0027%s\u0027, \""},{"line_number":174,"context_line":"                       \"probably a design bug\" % (unicode(e)))"},{"line_number":175,"context_line":"            LOG.debug(log_msg)"},{"line_number":176,"context_line":"            msg \u003d _(\"Request gives rise to error\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2DU%3D","line":173,"updated":"2013-12-30 09:43:14.000000000","message":"pls use _() instead.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":170,"context_line":"            LOG.debug(msg)"},{"line_number":171,"context_line":"            raise webob.exc.HTTPBadGateway(msg)"},{"line_number":172,"context_line":"        except Exception as e:"},{"line_number":173,"context_line":"            log_msg \u003d (\"Caught unknown exception: \u0027%s\u0027, \""},{"line_number":174,"context_line":"                       \"probably a design bug\" % (unicode(e)))"},{"line_number":175,"context_line":"            LOG.debug(log_msg)"},{"line_number":176,"context_line":"            msg \u003d _(\"Request gives rise to error\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fzwk%3D","line":173,"in_reply_to":"AAAAT3%2F%2F2DU%3D","updated":"2013-12-31 06:48:17.000000000","message":"Do debug logs need translation? I can\u0027 seem to find any policy on this ... all I can find is a discussion on the mailing list a year back but no consensus.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"0106dce7c003e468ba0be76674a6c6d419aa4ace","unresolved":false,"context_lines":[{"line_number":170,"context_line":"            LOG.debug(msg)"},{"line_number":171,"context_line":"            raise webob.exc.HTTPBadGateway(msg)"},{"line_number":172,"context_line":"        except Exception as e:"},{"line_number":173,"context_line":"            log_msg \u003d (\"Caught unknown exception: \u0027%s\u0027, \""},{"line_number":174,"context_line":"                       \"probably a design bug\" % (unicode(e)))"},{"line_number":175,"context_line":"            LOG.debug(log_msg)"},{"line_number":176,"context_line":"            msg \u003d _(\"Request gives rise to error\")"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fwlc%3D","line":173,"in_reply_to":"AAAAT3%2F%2Fzwk%3D","updated":"2014-01-02 08:20:19.000000000","message":"So tbh I personally like go _() way, not only at here but other places also.\n\nAnyway you of cause can have your choice.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"}],"glance/common/exception.py":[{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":"class BadStoreUri(GlanceException):"},{"line_number":74,"context_line":"    message \u003d _(\"Backend store operation failed, \""},{"line_number":75,"context_line":"                \"see logs for more information.\")"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"class Duplicate(GlanceException):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2KI%3D","line":75,"updated":"2013-12-30 09:43:14.000000000","message":"I really don\u0027t think this line is useful for end user. if you still we need say something for end user, I prefer tell he ask Cloud administrator to get help. (since end user can not check logs at all but admin can). And related with my comments in the http.py, I think we\u0027d better allow this exception expose an internal worth information to end user, says something like to add a \"reason\" param.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":"class BadStoreUri(GlanceException):"},{"line_number":74,"context_line":"    message \u003d _(\"Backend store operation failed, \""},{"line_number":75,"context_line":"                \"see logs for more information.\")"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"class Duplicate(GlanceException):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FzbE%3D","line":75,"in_reply_to":"AAAAT3%2F%2F2KI%3D","updated":"2013-12-31 06:48:17.000000000","message":"I changed the original description because I felt the message wasn\u0027t (always) meaningful - not all store operation failures were because of malformed store URIs.\n\nRegarding exposing some internal info - I thought you didn\u0027t want that because there was a risk of exposing security info :)?\n\nI had originally wanted to expose the following errors:\n\n  - Connection failed\n  - File not found\n  - Unauthorized\n  - Forbidden\n\nwithout mentioning the backend URL to avoid security leaks. Any other errors would be converted to a BadStoreUri error.\n\nI agree regarding the wording of the BadStoreUri message - perhaps \"... failed, contact cloud administrator for more information\" is better?\n\nSo shall I expose the 4 errors mentioned above? There are existing exceptions for most of them:\n\n  - Connection falure -\u003e ClientConnectionError\n  - File not found -\u003e ImageDataNotFound\n  - Unauthorized -\u003e NotAuthenticated\n  - Forbidden -\u003e Forbidden\n\nBTW, I just noticed, Forbidden.message is the same as ForbiddenPublicImage.message - is that deliberate or a (very minor) bug?","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"0106dce7c003e468ba0be76674a6c6d419aa4ace","unresolved":false,"context_lines":[{"line_number":72,"context_line":""},{"line_number":73,"context_line":"class BadStoreUri(GlanceException):"},{"line_number":74,"context_line":"    message \u003d _(\"Backend store operation failed, \""},{"line_number":75,"context_line":"                \"see logs for more information.\")"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":""},{"line_number":78,"context_line":"class Duplicate(GlanceException):"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FwqI%3D","line":75,"in_reply_to":"AAAAT3%2F%2FzbE%3D","updated":"2014-01-02 08:20:19.000000000","message":"Yes, the new message you supposed is good to me, thanks.\n\nAs i replied in your bug report, I don\u0027t really think we need expose detailed exceptions to end user (those 4 types what you mentioned in above), and the essential logs+BadStoreUri is a good way to go IMO, since it is necessary for end user, if i understand your point correct, for example, why we need tell end user our backend storage has rejected our internal connection request from Glance which caused a connection failure issue, or an image data entry is gone which should be there in normal case as Glance/OpenStack designed.\n\nbtw, yes I believe it\u0027s deliberate but a bug. you can refer ./glance/db/sqlalchemy/api.py Line140 and 142.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"}],"glance/store/http.py":[{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    def _query(self, location, verb, depth\u003d0):"},{"line_number":154,"context_line":"        def msgstr(err_msg):"},{"line_number":155,"context_line":"            return (\"\u0027HTTP store \u0027%(op)s\u0027 operation failed. \""},{"line_number":156,"context_line":"                    \"Error:%(error_msg)s\") % {\u0027store_type\u0027: \u0027HTTP\u0027,"},{"line_number":157,"context_line":"                                              \u0027op\u0027: verb,"},{"line_number":158,"context_line":"                                              \u0027error_msg\u0027: err_msg}"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2Lg%3D","line":155,"updated":"2013-12-30 09:43:14.000000000","message":"_( ... )","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    def _query(self, location, verb, depth\u003d0):"},{"line_number":154,"context_line":"        def msgstr(err_msg):"},{"line_number":155,"context_line":"            return (\"\u0027HTTP store \u0027%(op)s\u0027 operation failed. \""},{"line_number":156,"context_line":"                    \"Error:%(error_msg)s\") % {\u0027store_type\u0027: \u0027HTTP\u0027,"},{"line_number":157,"context_line":"                                              \u0027op\u0027: verb,"},{"line_number":158,"context_line":"                                              \u0027error_msg\u0027: err_msg}"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fzag%3D","line":155,"in_reply_to":"AAAAT3%2F%2F2Lg%3D","updated":"2013-12-31 06:48:17.000000000","message":"for debug logs too?","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"0106dce7c003e468ba0be76674a6c6d419aa4ace","unresolved":false,"context_lines":[{"line_number":152,"context_line":""},{"line_number":153,"context_line":"    def _query(self, location, verb, depth\u003d0):"},{"line_number":154,"context_line":"        def msgstr(err_msg):"},{"line_number":155,"context_line":"            return (\"\u0027HTTP store \u0027%(op)s\u0027 operation failed. \""},{"line_number":156,"context_line":"                    \"Error:%(error_msg)s\") % {\u0027store_type\u0027: \u0027HTTP\u0027,"},{"line_number":157,"context_line":"                                              \u0027op\u0027: verb,"},{"line_number":158,"context_line":"                                              \u0027error_msg\u0027: err_msg}"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FwoQ%3D","line":155,"in_reply_to":"AAAAT3%2F%2Fzag%3D","updated":"2014-01-02 08:20:19.000000000","message":"yes","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":153,"context_line":"    def _query(self, location, verb, depth\u003d0):"},{"line_number":154,"context_line":"        def msgstr(err_msg):"},{"line_number":155,"context_line":"            return (\"\u0027HTTP store \u0027%(op)s\u0027 operation failed. \""},{"line_number":156,"context_line":"                    \"Error:%(error_msg)s\") % {\u0027store_type\u0027: \u0027HTTP\u0027,"},{"line_number":157,"context_line":"                                              \u0027op\u0027: verb,"},{"line_number":158,"context_line":"                                              \u0027error_msg\u0027: err_msg}"},{"line_number":159,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2K0%3D","line":156,"updated":"2013-12-30 09:43:14.000000000","message":"nit: add a space between \":\" and \"%\".","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":153,"context_line":"    def _query(self, location, verb, depth\u003d0):"},{"line_number":154,"context_line":"        def msgstr(err_msg):"},{"line_number":155,"context_line":"            return (\"\u0027HTTP store \u0027%(op)s\u0027 operation failed. \""},{"line_number":156,"context_line":"                    \"Error:%(error_msg)s\") % {\u0027store_type\u0027: \u0027HTTP\u0027,"},{"line_number":157,"context_line":"                                              \u0027op\u0027: verb,"},{"line_number":158,"context_line":"                                              \u0027error_msg\u0027: err_msg}"},{"line_number":159,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fzac%3D","line":156,"in_reply_to":"AAAAT3%2F%2F2K0%3D","updated":"2013-12-31 06:48:17.000000000","message":"OK","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":175,"context_line":"        # Check for bad status codes"},{"line_number":176,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":177,"context_line":"            LOG.debug(msgstr(resp.status))"},{"line_number":178,"context_line":"            raise exception.BadStoreUri"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"        location_header \u003d resp.getheader(\"location\")"},{"line_number":181,"context_line":"        if location_header:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2I4%3D","line":178,"updated":"2013-12-30 09:43:14.000000000","message":"Good catch.\n\nBut it think this is a problem in BadStoreUri exception class implementation but here, I mean add a param to BadStoreUri and expose some worth/necessary details to end user is reasonable me, abd it\u0027s the purpose in original code also.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":175,"context_line":"        # Check for bad status codes"},{"line_number":176,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":177,"context_line":"            LOG.debug(msgstr(resp.status))"},{"line_number":178,"context_line":"            raise exception.BadStoreUri"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"        location_header \u003d resp.getheader(\"location\")"},{"line_number":181,"context_line":"        if location_header:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FzZY%3D","line":178,"in_reply_to":"AAAAT3%2F%2F2I4%3D","updated":"2013-12-31 06:48:17.000000000","message":"I thought of that but it seems BadStoreUri is called differently in different backends:\n\n  - cinder \u0026 sheepdog backends call it with BadStoreUri(uri, reason)\n  - store/__init__ \u0026 rbd,  call it with BadStoreUri(reason)\n  - others have mixed calls - with and without URI, with and without reason.\n\nTherefore I thought perhaps the BadStoreUri exception message was deliberately changed not to show any parameters for the sake of security. Since I was touching this file I removed the parameters from the call only in this file.\n\nSo what should we do for the BadStoreUri calls in the other backend stores - leave them as they are? Or perhaps we can modify the store so that it translates all backend exceptions into the 5 exceptions I mentioned earlier (connection failed, unauthorized, forbidden, file not found, BadStoreUri) without exposing any URL info?\n\nWhat do you think?","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"0106dce7c003e468ba0be76674a6c6d419aa4ace","unresolved":false,"context_lines":[{"line_number":175,"context_line":"        # Check for bad status codes"},{"line_number":176,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":177,"context_line":"            LOG.debug(msgstr(resp.status))"},{"line_number":178,"context_line":"            raise exception.BadStoreUri"},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"        location_header \u003d resp.getheader(\"location\")"},{"line_number":181,"context_line":"        if location_header:"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2Fwms%3D","line":178,"in_reply_to":"AAAAT3%2F%2FzZY%3D","updated":"2014-01-02 08:20:19.000000000","message":"For former, I think it is ok to me. but IMO it will be better a) if we  can make them all be consistent, and b) if needed, expose some useful details to end user.\n\nThe point a) is essential I believe, and get b) will be better since you know I don\u0027t like expose other particular exception to end user too much. of cause both should be happened in a dedicated change but this.\n\nFor latter, I have posted my thoughts in common/exception.py as a comment, pls check it.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"925b41fffe10e2b366f1c21c5aa5f30498ff98d8","unresolved":false,"context_lines":[{"line_number":180,"context_line":"        location_header \u003d resp.getheader(\"location\")"},{"line_number":181,"context_line":"        if location_header:"},{"line_number":182,"context_line":"            if resp.status not in (301, 302):"},{"line_number":183,"context_line":"                reason \u003d (_(\"The HTTP URL attempted to redirect with an \""},{"line_number":184,"context_line":"                            \"invalid %s status code.\") % resp.status)"},{"line_number":185,"context_line":"                LOG.debug(reason)"},{"line_number":186,"context_line":"                raise exception.BadStoreUri"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2F2Kg%3D","line":183,"updated":"2013-12-30 09:43:14.000000000","message":"I think we can also use \"msgstr()\" refactor here.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d9dc150235c731b98c29b8ffef9b1e0048091300","unresolved":false,"context_lines":[{"line_number":180,"context_line":"        location_header \u003d resp.getheader(\"location\")"},{"line_number":181,"context_line":"        if location_header:"},{"line_number":182,"context_line":"            if resp.status not in (301, 302):"},{"line_number":183,"context_line":"                reason \u003d (_(\"The HTTP URL attempted to redirect with an \""},{"line_number":184,"context_line":"                            \"invalid %s status code.\") % resp.status)"},{"line_number":185,"context_line":"                LOG.debug(reason)"},{"line_number":186,"context_line":"                raise exception.BadStoreUri"}],"source_content_type":"text/x-python","patch_set":1,"id":"AAAAT3%2F%2FzZM%3D","line":183,"in_reply_to":"AAAAT3%2F%2F2Kg%3D","updated":"2013-12-31 06:48:17.000000000","message":"Yes. Will do that.","commit_id":"742f8b0242cff08920d9c9a4a9c8c51893130e75"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"c23806a55e04dff62053afef3c8e6c45e422ad0b","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Fwj8%3D","line":177,"updated":"2014-01-02 08:31:24.000000000","message":"I don\u0027t think we need expose those details to end user.\n\nAnd IMO, as I mentioned in PS#1 comments, I consider essential log+badstoreuri-exception is good enough for end user and admin/operator.\n\ncan you pls tell me why we need expose those details which come from our cloud internal to end user? what\u0027s the special value from that approach? thanks.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"f314a36f4055867d39ce858cb9c4f8fefa9e1ad8","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FrzE%3D","line":177,"in_reply_to":"AAAAT3%2F%2Fr4s%3D","updated":"2014-01-03 14:53:45.000000000","message":"I don\u0027t care about the exception name (Naming is a hard problem already http://martinfowler.com/bliki/TwoHardThings.html :)), for now my biggest issue is (User eXperience) that you choose to hide the reason why the user image fail to be fetched. \n\nA faulty switch/router, that special cases, you can\u0027t build your design on special cases remember  \"Special cases aren\u0027t special enough to break the rules.\" (Python ZEN).\n\nSo start by defining the rule.\n\nFor me the rule is like i said above, in code it should look like this:\n\n  try:\n     fetch image\n  except ...:\n     bad image uri.\n\nNot \n\n   try:\n      fetch image\n   except ...:\n      maybe:\n        bad image uri.\n        or switch/router faulty\n        or no default route in the platform.\n        or Glance badly configured\n        or DNS server is down.\n        or internet is down.\n        or The word come to an end \n        or ...\n\nIf we agree on the rule, a reason in the log doesn\u0027t mean much to user, if user supply bad image URI as an administrator i can\u0027T fix it for the user (i won\u0027t to be precise), and as user if Glance told me why my image fail, i can fix it easily, if not then i will contact the administrator and the poor guy will have to check the log for the me to see what happen to to my image, jut to tell me that i have supplied a bad uri :(\n\nLike i said start by defining the rule that you want.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"1bb9b925a0408252bb0961b1b6d8fd286e8fa0d6","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2F%2BKY%3D","line":177,"in_reply_to":"AAAAT3%2F%2Frwg%3D","updated":"2014-01-06 01:51:52.000000000","message":"Well then you\u0027re pushing for what I did in this patchset (as you can see in the code) ... I tried to raise different exceptions so that the front end can decide what errors to return.\n\nBut Zhi thinks the front-end should expose/raise only the BadStoreUri exception to the user.\n\nSo you and Zhi fight it out now :).","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"8c422522c55dcb6b70cf942e5f8cacacb3916682","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Frwg%3D","line":177,"in_reply_to":"AAAAT3%2F%2FrzE%3D","updated":"2014-01-03 15:02:34.000000000","message":"\u003e This approach should cover your example (because user will\n \u003e get a 404). And that should also cover \"other cases\" (e.g.\n \u003e network problem between Glance and the backend store)\n \u003e which the user shouldn\u0027t know/care about.\n\nit doesn\u0027t 404 was just an example, it can be anything, another example.\n\n  - me: upload image http://gdfgdgf\n  - glance: NotFound(\"image url failed fetch returned: \u003cAny HTTP status that you can think about\u003e\")\n  - me: Ohh shit my url is not working because ...\n  - me: upload image http://\u003ccorrect URI\u003e\n\nLike i said if we agree that reason doesn\u0027t contain any security issue then why hide it at all, if it\u0027s not meant for the user (special case faulty switch) so be it, if it\u0027s user problem that a big win.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"de64e6839f6bc29b40903a66b7e3208a42014690","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FszY%3D","line":177,"in_reply_to":"AAAAT3%2F%2Fs38%3D","updated":"2014-01-03 10:15:58.000000000","message":"Sorry for not making it clear, but for the \"except Exception\" that i was referring to is the one here https://review.openstack.org/#/c/64409/4/glance/api/v1/images.py this break the REST api, like i explained above (a Server error become a client/user error). As for using \"except Exception\" i totally disagree with using it, and like i explained the code should make it clear and explicit what kind of error are to be raised and in which circumstance, if another error is raised that wasn\u0027t considered (not documented) it\u0027s a bug, and it must not be swallowed. IMHO if you must use  \"except Exception\" (in some special cases)  i would like to see with it a comment (with a TODO or FIXME) apologizing for it and explain why you had to do it. And don\u0027t let me get start of how many \"except Exception\" that was swallowing bugs that no one new about until costumer come knocking on you doors :)\n\nAnd about returning 500 status code, like i said if there is no bug in the code (which is what we aim for) user should not see a 500 error code, if he see it there is a bug (which we should fix) and there is no need to hide the fact that we have a bug in our code from the user.\n\nAs for other point that we disagree about:\n\n1- IMHO the HTTP store should not raise BadStoreUri instead it should raise NotFound like other stores, check here https://github.com/openstack/glance/blob/master/glance/store/base.py#L104, the type of exception to raise must be the same between get method of all stores.\n\n2- For reason or no reason IMHO i think there should be a reason,(Zhi said it\u0027s ok for both so it just me and you @David on this :)), so again thinking about user cases there is 2 scenarios here.\n\n 2.1 I am the user that own the image that i just created using a http URI which mean it will be stored in the http store, now in this case i think we all agree that user should see if Glance fail to get image that user has supplied and for what reason, if you don\u0027t include a reason then what should the user do ? Contact administrator !! That very bad IMHO. \n\n 2.2 I am using a shared/public image, well even in this case i still think that there should be a explanatory error for the user, usability first unless there is a security issue which i don\u0027t see at all.\n\nMy point is that unless there is a real vector of security attack here, we should prefer usability.\n\nIf you don\u0027t have a reason for the HTTPBadRequest, then you will have a confusing over if the \"get image\" request that fail or if it\u0027s something else (That\u0027s why i still have doubt about returning HTTPBadRequest for this).","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"8ede246ee0296f3a6be723ddcf1f59975edfc2b5","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Fs38%3D","line":177,"in_reply_to":"AAAAT3%2F%2Fs6U%3D","updated":"2014-01-03 08:03:27.000000000","message":"No problem ... will wait till tomorrow (or rather, Monday).","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"889afd9ff9a8f5e7f48a56c2c0497fa54032c954","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Fs6U%3D","line":177,"in_reply_to":"AAAAT3%2F%2Fs8U%3D","updated":"2014-01-03 07:22:12.000000000","message":"Both is OK to me, again.\n\nBut  I think it will be better if we wait a moment to listen mouad benchchaoui\u0027s feedback/thoughts if he has.\n\nThanks for your patience.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"0206b3b6e7d8a2feeeaeff393796589abdf6cf7c","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Fr4s%3D","line":177,"in_reply_to":"AAAAT3%2F%2FsEE%3D","updated":"2014-01-03 14:18:05.000000000","message":"\u003e If it just for logging purpose than yes, no harm, log and **re-raise**.\n\nYes, that\u0027s what I meant.\n\n \u003e like \"connection to backend failed\" are you talking about a bad\n \u003e configured glance/platform?\n\n \u003e If you are thinking about cases where glance fail to fetch a **correct**\n \u003e URI, than we are talking about special cases where glance (or the platform)\n \u003e is badly configured and this is not glance fault, neither the user\n\nMight not be bad configuration - might be some network error (e.g. a faulty switch/router) which might not be the operator\u0027s fault.\n\nMy point is that for errors stemming from invalid *user input* we return a suitable error to the user. \"File not found\" is one such case for which we must raise the NotFound exception (or, to be more precise, ImageDataNotFound because the image info is there in the database) - the base class defines this behaviour anyway.\n\nBut for other errors which are *not* due to user input but due to either bugs in the code or infrastructure problems - we raise the BadStoreOperation (or BadStoreUri if the cores would prefer the exception name not be changed) and leave the reason in the logs because in those cases it is a problem with the operator infrastructure, not user input.\n\nThis approach should cover your example (because user will get a 404). And that should also cover \"other cases\" (e.g. network problem between Glance and the backend store) which the user shouldn\u0027t know/care about.\n\nSo are we finally in agreement :)?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"efadf657d4001f6d2dc1c98e8f8b98cff89de960","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FsEE%3D","line":177,"in_reply_to":"AAAAT3%2F%2FsII%3D","updated":"2014-01-03 13:44:48.000000000","message":"\u003e I would still catch the exception for the sake of recording a\n  \u003e  problem in the logs, but return a 500 to the user. I think \n  \u003e you\u0027d agree, right?\n\nIf it just for logging purpose than yes, no harm, log and **re-raise**.\n\n  \u003e That was Zhi\u0027s whole point - the http store is *not* owned by\n  \u003e  you ... even if the API told you the reason (e.g. connection  \n  \u003e to backend failed) there\u0027s *nothing* you can do about it other   \n  \u003e than call the administrator and ask the admin to sort it out\n  \u003e and the admin would still have to look at the logs to figure out\n  \u003e the exact problem.\n\nHmmm  so you are saying that if an image fail to be fetched it\u0027s administrator problem ? That new to me ! Can you specify in which case where this is administrator problem ? like \"connection to backend failed\" are you talking about a bad configured glance/platform?\n\nBecause i was assuming that it\u0027s me as a user that should care about this, not the administrator, b/c the assumption here is that glance is working (b/c we are already in user space not PoC or development), and by \"working\" i mean if a user specify a **correct** URI for the image than glance will fetch it correctly, this imply that if glance fail to fetch an image, then as a user i want to know why, so that i can fix it (**user should fix it not administrator**) by changing the URI for example, that the usability that i am talking about, and that how at least i think about this.\n\nThe backend is not owned by me, yes, but the image is :) \n\nIf you are thinking about cases where glance fail to fetch a **correct** URI, than we are talking about special cases where glance (or the platform) is badly configured and this is not glance fault, neither the user (actually as a user i should probably start looking for another cloud provider hhh)\n\nPutted this way:\n\n1- No Reason:\n\n  - me: glance upload http://mispealed-uri.con\n  - glance: ok\n  - me: download image\n  - glance: BadRequest \n  - me: WAT ?\n  - me: download image (try it one more time, you never know :))\n  - glance: BadRequest\n  - me: WHY !!!! Cursing ..... :)\n\n2- With Reason\n\n  - me: glance upload http://mispealed-uri.con\n  - glance: ok\n  - me: download image\n  - glance: BadRequest(\"Image url http://mispealed-uri.con return 404\")\n  - me: hmm let\u0027s me try it locally, ohh it was my fault, i will correct it.\n  - me: upload image with correct uri\n  * Happy ending :) *\n  * after 1 month uri was removed * \n  - me: download image\n  - glance: BadRequest(\"Image url http://mispealed-uri.con return 404\")\n  - me: Ahhh my image URI is not reachable any more i should update it.\n\n  \u003e I think we can stick to the store/base.py definition - if an \n  \u003e image file is not found in the backend then we return a 404  \n  \u003e and say image data couldn\u0027t be located - this is as per\n  \u003e definition of the Store base class. \n\nThat\u0027s my point :)\n\n  \u003e Other errors are (IMHO) internal to the cloud system and so \n  \u003e can be \"hidden\" under a common BadStoreUri (I\u0027d prefer it to\n  \u003e be called BadStoreOperation but since it\u0027s for \"internal\"\n  \u003e consumption I\u0027m not to fussy about it) and we return a 500 to\n  \u003e the user. But this time it\u0027s a 500 \"by intention\" and not some\n  \u003e uncaught exception. For such cases logs are sufficient.\n\nWhat do you mean by other errors here ? are we talking about the same thing other errors -\u003e bugs in the code ? No need to answer this one i will check it in the new patch :)\n\n\nRight ?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"0340bacd9d9487718028a92d2017769168141bcf","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FsII%3D","line":177,"in_reply_to":"AAAAT3%2F%2FszY%3D","updated":"2014-01-03 13:00:43.000000000","message":"Hi Mouad,\n\n \u003e but for the \"except Exception\" that i was referring to\n \u003e is the one here\n\nYes, I got that ... and like I said earlier I tend to agree with you on that. I just wanted to see if anybody had some other reasons not to do that.\n\nTo be more precise, it\u0027s not the \"except Exception\" that I\u0027m against but the hiding of the exception - I would still catch the exception for the sake of recording a problem in the logs, but return a 500 to the user. I think you\u0027d agree, right?\n\n \u003e IMHO the HTTP store should not raise BadStoreUri instead it should raise\n \u003e NotFound like other stores\n\nNotFound is only if the image is not found in the backend. But there are many other cases where the expected behaviour is not (yet) defined.\n\nI think the issue here is basically what do we expose to the end user. We can argue and say backend image data not found is an internal error because the user doesn\u0027t know or care about that - (s)he uploaded an image to Glance and the Glance APIs show the image is there (in the DB). It\u0027s when trying to download the image that the image data is not available. So NotFound doesn\u0027t make sense to the user either.\n\n \u003e For reason or no reason IMHO i think there should be a reason\n\nI believe the BadStoreUri exception is for the operator to see and not the end user. So it\u0027s more important to focus on the log message here. In fact, as I think about it, I\u0027m strongly opposed to having a \u0027reason\u0027 parameter in BadStoreUri because I don\u0027t think a lower-layer/internal module should be throwing user-visible messages directly - that should be done by the API.\n\nSo ideally, we would have a BadStoreUri exception *and* some other error related *data* in it (not just a message string). The API layer would catch this exception, examine the data and, based on the data, decide what error to return to the end user.\n\nI\u0027ve been thinking of this for a while now but haven\u0027t yet found time to throw it out for discussion to the mailing list - should we enhance Glance exceptions to allow lower layers to pass error info to higher layers so that higher layers can analyze the error (and not just log an error string)? Perhaps it\u0027s time to put out the suggestion to the list :).\n\n \u003e I am the user that own the image that i just created using a http\n \u003e URI which mean it will be stored in the http store, now in this\n \u003e case i think we all agree that user should see if Glance fail to\n \u003e get image that user has supplied and for what reason\n\nWell, if the error was just \"Backend store operation failed\" that\u0027s good enough because the backend store is also owned by the same administrator as the Glance service.\n\nThat was Zhi\u0027s whole point - the http store is *not* owned by you ... even if the API told you the reason (e.g. connection to backend failed) there\u0027s *nothing* you can do about it other than call the administrator and ask the admin to sort it out and the admin would still have to look at the logs to figure out the exact problem.\n\n \u003e I am using a shared/public image, well even in this case i still\n \u003e think that there should be a explanatory error for the user,\n \u003e usability first unless there is a security issue which i don\u0027t see\n \u003e at all.\n\nSame as above.\n\n \u003e My point is that unless there is a real vector of security attack\n \u003e here, we should prefer usability.\n\nAgreed. But, as we see, in this case telling the user of a backend error doesn\u0027t enhance usability because the user is *not* in control of the backend.\n\n \u003e If you don\u0027t have a reason for the HTTPBadRequest, then you will have a\n \u003e confusing over if the \"get image\" request that fail or if it\u0027s something\n \u003e else (That\u0027s why i still have doubt about returning HTTPBadRequest for\n \u003e this).\n\nI think we can stick to the store/base.py definition - if an image file is not found in the backend then we return a 404 and say image data couldn\u0027t be located - this is as per definition of the Store base class. Other errors are (IMHO) internal to the cloud system and so can be \"hidden\" under a common BadStoreUri (I\u0027d prefer it to be called BadStoreOperation but since it\u0027s for \"internal\" consumption I\u0027m not to fussy about it) and we return a 500 to the user. But this time it\u0027s a 500 \"by intention\" and not some uncaught exception. For such cases logs are sufficient.\n\nWhat do you guys think?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"d773e51d52c36dcbe83b8b7d36b8a3247a7aede1","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Fs8U%3D","line":177,"in_reply_to":"AAAAT3%2F%2FtR0%3D","updated":"2014-01-03 07:05:57.000000000","message":"You\u0027ve convinced me - I agree with you :).\n\nI\u0027ll clean up the extra exceptions and keep the BadStoreUri exception. I won\u0027t add the \"reason\" parameter to the exception for the moment because then I\u0027d have to change the other backends and I don\u0027t have a setup to test all of them. Is that OK with you?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"44f5c699a4130ecbb48f97cef30ddb9f2d3b5e4a","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FtR0%3D","line":177,"in_reply_to":"AAAAT3%2F%2Ftns%3D","updated":"2014-01-03 05:03:59.000000000","message":"@koofoss, thanks for your clarification here. I have some replies as following.\n\n1. Yes that just is my thinking, and TBH, again, I really don\u0027t consider the user should be able to aware the details of the backend and has some form of control over it, just for an example on your S3 case: Glance don\u0027t allow S3 store has a dedicated configuration (like access credential) for each particular end user, there is only a global configuration [1] for all users, so a particular end user how do control his own S3 backend?\n\n[1] https://github.com/openstack/glance/blob/master/etc/glance-api.conf#L359\n\n2. Glance now support a lot of block storage backends.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"6da38f43c411b895d8316bd034b67ab5595426d1","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FtnE%3D","line":177,"in_reply_to":"AAAAT3%2F%2FukU%3D","updated":"2014-01-03 03:32:21.000000000","message":"Hi Mouad,\n\n \u003e i start by creating an image in the HTTP store so that it can be fetched from an URI,\n \u003e now if Glance fail to fetch the remote image URI, i would like to know why, right\n\nI think here is where we have a difference with Zhi. My assumption was similar to yours - that the user is aware of the backend and has some form of control over it. The control may be as simple as being the owner of an account in the backend, or as complex as being the complete owner of the full backend.\n\nBut Zhi\u0027s point is that no cloud operator will allow this kind of scenario - both the Glance service *and* the backend will be completely controlled by the operator. It is the operator who decides what kind of backend to use and he does *not* expose backend info to the end user. Therefore the user should *never* see a backend error ... (s)he isn\u0027t even aware of what backend is being used so it makes no sense to inform the user of backend errors.\n\nSo when I think about it, I kind of agree with Zhi on this one ... at least at the moment I don\u0027t think cloud operators will be willing to commit to connect to a backend chosen by the user (imagine the security risks and headaches and perhaps even business model questions). The only situation I see something like what we assumed happening (as of now) is a kind of development setup (e.g. in a lab) where the user is also the operator himself. But this is a special case of the \"operator-owns-everything\" scenario and the operator wants to see messages as a user only because it\u0027s convenient - he doesn\u0027t want to go through the logs. But he actually *has* access to the logs and *can* dig through the logs.\n\nDo you have a better scenario?\n\nSo ...\n\n \u003e the why should include what the remote url returned as status code\n \u003e and the respected message, so for me (as user)\n\nIf the backend is not hosted by you, you don\u0027t need to know this (and any of the other info you mention below) at all, right? In fact, if I tell you the URL (or anything else), I might be exposing sensitive security information so I should *not* tell you anything!\n\nSo what I\u0027m seeing is that we\u0027re all agreeing on a *principle*: We need to inform the owner of the respective subsystems when there is a failure in their subsystem.\n\nIf the API/framework and backend subsystem owners are the same (Zhi\u0027s case) then logs are sufficient and the end user need not see anything (except maybe a 500 Internal Error).\n\nIf the owners are different then we need multiple \"channels\" to convey info to each owner. But regardless of scenario, the end user should *not* see all this. (NOTE: The end user may be wearing two hats - one as an end user, one as a backend owner. But I think we should distinguish the two roles). It is quite obvious that irrespective of the technical issues, this setup is complicated for all parties. Therefore I suggest (in agreement with Zhi) we don\u0027t consider this scenario yet ... if we need to take this up in future there will be more problems to consider than just backend messages.\n\n \u003e NOTE 2: I replaced BadStoreUri with UnexpectedStatusCode.\n\nWhen I first started working on this I chose this:\n\n    class BadStoreOperation(GlanceException):\n        message \u003d _(\"%(store_type)s operation \u0027%(op_type)s\u0027 failed, \"\n                    \"error: %(error_msg)s\")\n\nI think this is even more clear ;).\n\nBut since Zhi preferred (and I agree) only to log the errors and not throw any exception to the user, I didn\u0027t define a new exception and stayed with BadStoreUri (though I changed it\u0027s message text). I added new exceptions to the http._query() function in the latest patchset because I misunderstood Zhi\u0027s review comment about adding more info (as I said in my previous reply to Zhi).\n\n \u003e why there is no validation as soon as the\n \u003e create/update image request is done\n\nBecause the backend may be reachable during the create/update but have problems later on when the user tries to download the image.\n\n \u003e please refrain from doing except Exception, if you don\u0027t know\n \u003e what kind of other exception can be raised, it mean that the\n \u003e code is badly wrote\n\nThe engineer in me agrees with you totally but from a usability point of view I would rather that the user *doesn\u0027t* see such things :). That\u0027s why if you see the log message I log it as a \"design bug\" - it\u0027s something for the operator to know (and report). What you describe is the goal we want to achieve but I\u0027m afraid we\u0027re not there yet :).\n\n \u003e a traceback in the log\n\nYes, I will add that to the log, thanks.\n\n \u003e a 500 will tell the user that we screw up and that\u0027s the truth :),\n \u003e return 400 to user mean that it\u0027s his fault which\n \u003e is **very** wrong.\n\nLike I said above, from an engineering standpoint I agree with you - fail fast makes good sense to debug problems. But from other non-technical points of view (e.g. business - do you want your customers to be seeing 500s?) the operator might rather that the customer doesn\u0027t see this.\n\nZhi (and any other reviewer) what do you think?\n\n\u003e Hope my explanations make sense.\n\nVery much. Thanks for your (and Zhi\u0027s) patient review :).","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"44f5c699a4130ecbb48f97cef30ddb9f2d3b5e4a","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FtTA%3D","line":177,"in_reply_to":"AAAAT3%2F%2FukU%3D","updated":"2014-01-03 05:03:59.000000000","message":"My thoughts on @mouad benchchaoui\t\u0027s comments:\n\n1. I think the details part in the exception message for exposing a little internal issue/status which returned to end user from Glance is make sense to me, and I think that stuff is just saying something like adding a \"reason\" field to BadStoreUri which we have discussed in above. And base on your example cases I also think a \"reason\" (or other name) is worth, and enough since from end user\u0027s perspective the main exception is a common and the details be different, btw keep single/consistent exception type can make it easy that develop upper layer application which integrated on the API @koofoss.\n\n2. Agreed, from end user\u0027s perspective, return a BadRequest from API side is make more sense, that probably means do the exception type transfer in controller layer. (I have not checked the code for this part)\n\n3. From coding style level, I total agree with you at this point. But are you replying my comments on https://review.openstack.org/#/c/63727/4/glance/store/http.py Line #135 still? I think this is not a blocker (I use \"we\u0027d better\" in original comments you know :) ), the reason why I like we catch Exception at there is that that way can 100% prevent raise exception from get_size() _and_ clear for code review/reading for other people.\n\nThanks for you explanation here. So I think we have only one different point/thought, which also for koofoss and me now, IMIO we can just using and expose BadStoreUri type exception from backend store driver only (with a \"reason\" field IMO, but it\u0027s not a blocker asking), because I think this is enough and the changing for all upper layer code will not be required.\n\nAm I miss something?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"133d4aca7f8f52d093d30710e4b94c2a876bea55","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FukU%3D","line":177,"in_reply_to":"AAAAT3%2F%2FvjI%3D","updated":"2014-01-02 20:30:22.000000000","message":"Here is how i did think about it, as a user, i start by creating an image in the HTTP store so that it can be fetched from an URI, now if Glance fail to fetch the remote image URI, i would like to know why, right ? the why should include what the remote url returned as status code and the respected message, so for me (as user) i just want to see something like:\n\n\"Image remote url failed to be fetched: server returned \u003cstatus_code\u003e \u003cbody\u003e\".\n\nAnd of course if the request can\u0027t be even send then i would like to know why Glance wasn\u0027t able to reach the URI that i gave him, error like:\n\n\"Image remote url failed to be fetched: host not found\"\n\nTwo scenario here that this method should care about.\n\n1- Request succeeded but the remote url returned an bad status code.\n\n2- Request wasn\u0027t send at all e.g. DNS failure, Infinit Redirect ...\n\nSo this method responsibility is to difference between the two scenarios and raise the appropriate exception to the caller and it\u0027s the caller (The API code: api/v1/images.py) responsibility to format a nice error for user, and log something meaningful for Administrators.\n\nNow b/c the api/v1/images.py is a shared code between all store the right thing to do is to raise the same exception across all stores in case getting an image from store failed, and that\u0027s what the API code should care about.\n\nAnd this is actually what the contract say in the docstring here https://github.com/openstack/glance/blob/stable/havana/glance/store/base.py#L96\n\nbut sadly the problem is that the http store get method doesn\u0027t respect it: https://github.com/openstack/glance/blob/stable/havana/glance/store/http.py#L115\n\nSo for me the fix will be in the get method to do:\n\n   try:\n       conn, resp, content_length \u003d self._query(location, \u0027GET\u0027)\n   except (exception.MaxRedirectsExceeded,\n               exception.ClientConnectionError,\n               exception.BadStoreUri):\n        raise exception.NotFound(\u003cexplanation for the user\u003e)\n\nNOTE 1: I don\u0027t agree with the name of NotFound.\n\nNOTE 2: I replaced BadStoreUri with UnexpectedStatusCode in this patch https://review.openstack.org/#/c/63727/ b/c it seem\nmore meaningful to me.\n\nNow talking about REST API, the image URI is entered by the user, so if this URI is not working (i.e. the two scenario above), the right status code to return is 400 i.e. BadRequest (i am a bit in doubt of this b/c why there is no validation as soon as the create/update image request is done), and it\u0027s in the BadRequest that user will see what happen with his URI why Glance fail to fetch it.\n\nLast and not least, please refrain from doing except Exception, if you don\u0027t know what kind of other exception can be raised, it mean that the code is badly wrote, you should know which error to catch, if any other error is raised it\u0027s mean that there is a bug, in this case i would prefer this to **fail hard and fast**, i.e. a traceback in the log and i really don\u0027t mind a 500 for the user (Hopefully it will mean a bug report for us to fix), a 500 will tell the user that we screw up and that\u0027s the truth :), return 400 to user mean that it\u0027s his fault which is **very** wrong.\n\nIn HTTP 400 \u003c\u003d status_code \u003c 500 implies client error.\nstatus code \u003e\u003d 500 implies server error.\n\nHope my explanations make sense.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"6da38f43c411b895d8316bd034b67ab5595426d1","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Ftns%3D","line":177,"in_reply_to":"AAAAT3%2F%2FvjI%3D","updated":"2014-01-03 03:32:21.000000000","message":"Yes, I was assuming end user can ask Glance to use his own S3 account to access his image.\n\nIf it is as you say - the cloud operator himself arranges for the backend store to be the operator\u0027s S3 account - then I fully agree with what you say ... logs are sufficient for him - the end user doesn\u0027t need to be aware of any backend store. In fact, in this case a 500 error is appropriate for the user (it *is* an internal error).\n\n \u003e Cinder also has this similar deployment mode, I never see a Cloud\n \u003e allow end user to do the configuration on its internal deployment\n\nI\u0027d say it\u0027s much, much harder for Cinder because Cinder works at a much lower block storage level. For Glance it is much easier - it just needs an object store. Of course, just because it\u0027s easier for Glance doesn\u0027t mean that that (the scenario I assumed) is what will happen ... I don\u0027t have a crystal ball to see what will happen in the future. So I\u0027ll go with your assumptions for now :).","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"92dbda1765b189ff590712cfea6dbfa8df244c4a","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2FvjI%3D","line":177,"in_reply_to":"AAAAT3%2F%2Fvwo%3D","updated":"2014-01-02 15:30:19.000000000","message":"1. For adding \"reason\" field for BadStoreUri, I think both is OK to me: add it or not. btw I believe we will do review before any code get merge, and we will try to prevent sensitive information disclosure as much as we can. Anyway I think this is not the key point for this change.\n\n2. If end user get any exception, he will need to contact Cloud operator since the problem resolving is not the responsibility for him but Cloud service provider and I don\u0027t think end user has this capability and enough information and/or skill to do this, so the logs will need to be checked by Cloud admin/operator in any case, for example: if we expose NotAuthenticated exception to end user, then when client get it, he need ask Cloud operator/admin to check (Glance) configuration to make sure whether a correct S3 access credential be used/applied (end user can not know that answer, glance-api.conf has that options you know, end user can\u0027t access that).\n\nAre you saying in your above example case, an particular end user can ask Glance using his S3 account to access his image? TBH don\u0027t think a Cloud provider can allow a particular end user using a particular storage backend for a service, in this case it\u0027s Glance, but you know Cinder also has this similar deployment mode, I never see a Cloud allow end user to do the configuration on its internal deployment, I think this is a responsibility for Cloud\u0027s admin/operator. Am I misunderstanding?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"1e9ab8747148b72d6634d5b130eb96b5d2546418","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAT3%2F%2Fvwo%3D","line":177,"in_reply_to":"AAAAT3%2F%2Fwj8%3D","updated":"2014-01-02 13:57:36.000000000","message":"Ah, I misunderstood your comments in patchset 1: \"I think we\u0027d better allow this exception expose an internal worth information to end user\".\n\nI\u0027m very hesitant to add a \"reason\" to the BadStoreUri exception because it\u0027s a \"free form\" string - you can put anything in it and so we will always have to worry about leaking sensitive information through the reason parameter. If we have to put a \"reason\" in the BadStoreUri exception, then I\u0027d rather use different exceptions instead because the amount of information we \"leak\" is under control this way ... no free form strings.\n\nBut if you think log + BadStoreUri is enough then I\u0027ll clean it up again :).\n\nI think our differences regarding one/multiple exceptions are due to our different assumptions - your assumption is that the cloud and the backend are all one unit, one operator. But I was thinking in terms of one cloud operator for the openstack/glance part and another operator for the backend part. For e.g. using Amazon S3 as a backend.\n\nIn this case if the user gets a BadStoreUri he knows there is a problem between glance and S3, but doesn\u0027t know what the problem actually is - he doesn\u0027t even have an idea. He probably has to first contact the openstack operator, convince him to get the logs, and then contact the S3 operator with the problem if it is an S3 problem. Or he has to try a lot of different tests to try to infer/guess what the problem is.\n\nBut a more specific error could give him some good hints as to where the problem is and how to solve it.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"94df86c7023aa68266a9f4652856e92a2fe92fdc","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2F9KU%3D","line":177,"in_reply_to":"AAAAUH%2F%2F%2BKY%3D","updated":"2014-01-06 09:27:24.000000000","message":"I\u0027m trying to understand our disagree points, and reply/share my thoughts to your as clear as possible. IIRC I think we are discussing related two parts: (http) store layer, and api layer, former can impact api (and domain in v2 api path) and latter will impact end user directly.\n\nSo store layer first.\n\n1. @mouad, for \"catch Exception\" topic, I partial agree with you. First I think it is an issue (dedicated but this bug fix) which we always return 0 value when get_size() implementation meet internal exception even. But I don\u0027t think we have necessary to raise various exceptions to upper layer. IMO when we meet issue we need to do only two things: a. write necessary exception message to log, this will help cloud admin/operator resolve issues. b. raise a single reorganized exception, in this case I think keep/reuse existing BadStoreUri is a good candidate to us. reorganization means, in my preferred way, make a \"reason\" message from end user\u0027s perspective. So, @koofoss, (again, sorry for repeat) I still personally preferred to add a \"reason\" field/mechanism to BadStoreUri.\n\n2. I also don\u0027t think we should return 500 to end user directly when store driver meet exception, any unexpected 500 issue should always be handled as an glance defect. This point is work for store layer and api layer also.\n\n3. The key reason to me for why I don\u0027t think we need raise various store internal exceptions to upper layer is that I really can\u0027t see a strong advantage from it, IMO single exception is enough: a. easy to handle in upper code, less code path/branch. b. it can work well for end user, it\u0027s enough and no disadvantage.  A (expose-able) reason description of store internal exception is enough for end user.\n\n4. probably we need allow store raise NotFound out also, I don\u0027t like be a blocker here, but I think if we have a \"reason\" field for BadStoreUri do you still think we need NotFound exception/branch for upper layer (and end user probably) ? and why or say any special advantage?\n\nAPI controller layer:\n\n1. IMO I agree to transfer BadStoreUri exception to BadRequest since I think BadRequest is make more sense from end user\u0027s POV.\n\n2. enhance my above #3.a point from api controller side, if we keep store exception raising easy, then we can make api layer code easy and precondition is this way not affect end user\u0027s experience, end user can get the details from \"reason\" field easily.\n\nI must lost something from your above sessions, right?","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":8759,"name":"Arnaud Legendre","email":"arnaudleg@gmail.com","username":"arnaud"},"change_message_id":"a1729e523d5781dcff7238a95aa8cc925b68e9cc","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2FaNk%3D","line":177,"in_reply_to":"AAAAUH%2F%2F25M%3D","updated":"2014-01-14 01:52:31.000000000","message":"See change https://review.openstack.org/#/c/62031/7 for get_size() not catching exception.","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"217ab94073e013c3cb577c649f55faf24d1246d7","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2F25M%3D","line":177,"in_reply_to":"AAAAUH%2F%2F5Bk%3D","updated":"2014-01-07 15:53:22.000000000","message":"Sounds good to me :)","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":9520,"name":"David Koo (koofoss)","email":"david.koo@huawei.com","username":"koofoss"},"change_message_id":"b6ad7f8eb0f1378e4e105ad4ba7fcb62c65c5573","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2F5Bk%3D","line":177,"in_reply_to":"AAAAUH%2F%2F5Mg%3D","updated":"2014-01-07 06:19:52.000000000","message":"\u003eAdd \"reason\" to BadStoreUri for end user is necessary. @koofoss agree?\n\nI don\u0027t have a problem with that but that means the BadStoreUri exception message format needs to change and each backend store seems to be following its own message format when it comes to BadStoreUri! @Zhi, I hope you\u0027re ok with that.\n\nI\u0027ll try to come up with something common for all of them (and change them all).\n\nMany thanks for everybody\u0027s comments ... it\u0027s been a long and hard discussion. The next time we meet we should go for $beverage :).","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":6549,"name":"Zhi Yan Liu","email":"lzy.dev@gmail.com","username":"lzy-dev"},"change_message_id":"af35e71d3804dd9c43ecb8304fbaf72beb4baaeb","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2F5Mg%3D","line":177,"in_reply_to":"AAAAUH%2F%2F83g%3D","updated":"2014-01-07 05:53:32.000000000","message":"OK, so can we make finally agreement on this:\n\nStore layer:\n\n1. In normal handling path, store dirver should only be able to raise two type exceptions from its public interface: NotFound and BadStoreUri. If dirver raise any other exception out, that means a defect in driver, at this time 500 will be returned to end user which just like current logic. In store internal we can raise/handle any store driver special exception but for interface outside. @mouad agree?\n\n2. Add \"reason\" to BadStoreUri for end user is necessary. @koofoss agree?\n\nAPI controller layer:\n\n1. @mouad if you ok on this point then I think we can stop do the transfer for BadStoreUri \u003d\u003e BadRequest, the reason I post this point here is that I remember you propose it in your above comments. Actually I think current logic which expose BadStoreUri to end user directly is acceptable to us.\n\nAm I miss anything?\n\nReply @mouad\u0027s asking on my last comments:\n\nPoint #2: Sorry it\u0027s a typo, we are on the same page, actually I want to say: \"I also *think* we should return 500 to end user directly when store driver meet exception, any unexpected 500 issue should always be handled as an glance defect. This point is work for store layer and api layer also.\". I mean handle all particular exception as possible as we can, if still miss catching, then 500 will be able to raise out to end user, then end user/tester can report a bug for that.\n\nthanks","commit_id":"2486521e09d902321addec49838567890ee89da7"},{"author":{"_account_id":7073,"name":"mouad benchchaoui","email":"m.benchchaoui@x-ion.de","username":"mouad"},"change_message_id":"4be3c264a246d82360c14c780c352c92ffb18c19","unresolved":false,"context_lines":[{"line_number":174,"context_line":"        # Check for bad status codes"},{"line_number":175,"context_line":"        if resp.status \u003e\u003d 400:"},{"line_number":176,"context_line":"            exc_map \u003d {"},{"line_number":177,"context_line":"                401: exception.NotAuthenticated,"},{"line_number":178,"context_line":"                403: exception.Forbidden,"},{"line_number":179,"context_line":"                404: exception.ImageDataNotFound"},{"line_number":180,"context_line":"            }"}],"source_content_type":"text/x-python","patch_set":4,"id":"AAAAUH%2F%2F83g%3D","line":177,"in_reply_to":"AAAAUH%2F%2F9KU%3D","updated":"2014-01-06 10:10:55.000000000","message":"1. We are talking about the _query method ? This is not a **big deal** for me, but I prefer to have more exception depending on the type of error, instead of hiding what went wrong in the message, and the upper layer should decide what to do with them, but like i said it\u0027s not a big deal b/c in this case we will end up dealing with them as one error i.e. \n\n    request failed -\u003e raise RequestError\n    request succeeded but return bad status code -\u003e raise UnexpectedStatus.\n\n2- We all agree about logging the traceback and such, but sorry i don\u0027t understand this point, what do you want to return as status code in case there is a glance defect (for me a Glance defect mean an unrecognized/un-planned exception raised) ? \n\n3- Totally agree with this, this was my first point, the http.Store.get should only raise NotFound for all cases like defined in the base class https://github.com/openstack/glance/blob/stable/havana/glance/store/base.py#L96.\n\n4- This is where we disagree, like i said in point 3 we should respect the Base class that define the behavior of all store so that we have, the BadStoreUri is specific to the http store, which mean this exception should not leak to upper layer (think Java \"throws\"), the upper layer is build upon the base class that define the behavior of the store implementation.\n\nAPI controller layer:\n\n1. I am a bit skeptical about this change, but i don\u0027t have an alternative :)\n\n2. Totally agree, that\u0027s what i was arguing with David about in our last exchange, for me the reason is a very important :)\n\nCheers,","commit_id":"2486521e09d902321addec49838567890ee89da7"}]}