)]}'
{"glance/api/v2/image_data.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        \"\"\""},{"line_number":74,"context_line":"        try:"},{"line_number":75,"context_line":"            if image.locations is not None:"},{"line_number":76,"context_line":"                for i in range(len(image.locations)):"},{"line_number":77,"context_line":"                    if image.locations[i].get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":78,"context_line":"                        image.locations.pop(i)"},{"line_number":79,"context_line":"        except Exception as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"d83beaa9_2f5e2b1c","line":76,"updated":"2021-02-22 23:25:41.000000000","message":"fixed to use enumerate() for next PS.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":75,"context_line":"            if image.locations is not None:"},{"line_number":76,"context_line":"                for i, loc in enumerate(image.locations):"},{"line_number":77,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":78,"context_line":"                        image.locations.pop(i)"},{"line_number":79,"context_line":"        except Exception as e:"},{"line_number":80,"context_line":"            LOG.debug(\"Location delete failed while unstaging %(image_id)s: \""},{"line_number":81,"context_line":"                      \"%(e)s\"), {\u0027image_id\u0027: self.image_id,"}],"source_content_type":"text/x-python","patch_set":4,"id":"8920d525_841b0301","line":78,"updated":"2021-02-23 22:39:48.000000000","message":"Where is the delete that used to be here? Is it implicit in a save of the image with one fewer locations than it had before?\n\nIf so, I\u0027d like to see a test that validates that it happens. It looks like this path is only taken if we fail the stage and need to cleanup, which means I assume we\u0027re dependent on the image being saved with the new location before we try to revert? I\u0027ll have to go looking for the magic that results in a delete here to understand better.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":75,"context_line":"            if image.locations is not None:"},{"line_number":76,"context_line":"                for i, loc in enumerate(image.locations):"},{"line_number":77,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":78,"context_line":"                        image.locations.pop(i)"},{"line_number":79,"context_line":"        except Exception as e:"},{"line_number":80,"context_line":"            LOG.debug(\"Location delete failed while unstaging %(image_id)s: \""},{"line_number":81,"context_line":"                      \"%(e)s\"), {\u0027image_id\u0027: self.image_id,"}],"source_content_type":"text/x-python","patch_set":4,"id":"27e4b961_f3e4b52a","line":78,"in_reply_to":"8920d525_841b0301","updated":"2021-03-01 16:10:11.000000000","message":"yes, the pop is what calls the delete from the store here https://github.com/openstack/glance/blob/master/glance/location.py#L238 I\u0027m sure we have formal tests somewhere for that as that has been the production code at least since 2014. location.remove() https://github.com/openstack/glance/blob/master/glance/location.py#L266 also just does the pop and the tests for that I stumbled upon recently. The whole point of these cleanups was to utilize the code that has been in production for years and already tested rather than the hacks we came up with just for this specific use case to get the tasks to the state that they could be merged, and fixed later. And no, the image does not need to be saved as we\u0027re not refreshing it from the database, but passing the object to the _unstage.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":76,"context_line":"                for i, loc in enumerate(image.locations):"},{"line_number":77,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":78,"context_line":"                        image.locations.pop(i)"},{"line_number":79,"context_line":"        except Exception as e:"},{"line_number":80,"context_line":"            LOG.debug(\"Location delete failed while unstaging %(image_id)s: \""},{"line_number":81,"context_line":"                      \"%(e)s\"), {\u0027image_id\u0027: self.image_id,"},{"line_number":82,"context_line":"                                 \u0027e\u0027: encodeutils.exception_to_unicode(e)}"}],"source_content_type":"text/x-python","patch_set":4,"id":"0d6d9475_4b795f80","line":79,"range":{"start_line":79,"start_character":15,"end_line":79,"end_character":24},"updated":"2021-02-23 22:39:48.000000000","message":"This is far too wide, IMHO. I\u0027m guessing maybe this is just to make tests pass now, but this should be specific exceptions, with a save-and-reraise or at least a LOG.exception(). You\u0027ve got a save-and-reraise in the stage() handler already, which means that you\u0027re expected to raise 500 for truly unexpected issues, and this diaper handler should do the same so we don\u0027t quietly hide failures we should be able to resolve.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":76,"context_line":"                for i, loc in enumerate(image.locations):"},{"line_number":77,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":78,"context_line":"                        image.locations.pop(i)"},{"line_number":79,"context_line":"        except Exception as e:"},{"line_number":80,"context_line":"            LOG.debug(\"Location delete failed while unstaging %(image_id)s: \""},{"line_number":81,"context_line":"                      \"%(e)s\"), {\u0027image_id\u0027: self.image_id,"},{"line_number":82,"context_line":"                                 \u0027e\u0027: encodeutils.exception_to_unicode(e)}"}],"source_content_type":"text/x-python","patch_set":4,"id":"d61f6b41_18e6706b","line":79,"range":{"start_line":79,"start_character":15,"end_line":79,"end_character":24},"in_reply_to":"0d6d9475_4b795f80","updated":"2021-03-01 16:10:11.000000000","message":"I don\u0027t think it\u0027s too wide, the one thing I have to say is that we probably should log it on much higher level than debug. At this point we\u0027re trying to handle already exception. Limiting what we catch here will likely cause us returning 500 to the user rather than meaningful response from Lines 301+","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":287,"context_line":"        # the glance.location module now so we don\u0027t need any of that here."},{"line_number":288,"context_line":"        try:"},{"line_number":289,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":290,"context_line":"            if len(image.locations) \u003d\u003d 1 and image.locations[0].get("},{"line_number":291,"context_line":"                    \u0027metadata\u0027, {}).get(\u0027staging_host\u0027):"},{"line_number":292,"context_line":"                msg \u003d _(\"The image %s has data on staging\") % image_id"},{"line_number":293,"context_line":"                raise webob.exc.HTTPConflict(explanation\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"aeeebe87_635eb47a","line":290,"updated":"2021-02-23 22:39:48.000000000","message":"If we somehow (like, someday) had two locations this check wouldn\u0027t pass and we\u0027d not raise on L293.\n\nEven still, isn\u0027t this cart-before-horse? In this refactor, all we need to check is the image state to provide the proper HTTPConflict error to the user. That happens on L295. I think this staging host awareness should be moved to the later patches where we actually know about it.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":287,"context_line":"        # the glance.location module now so we don\u0027t need any of that here."},{"line_number":288,"context_line":"        try:"},{"line_number":289,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":290,"context_line":"            if len(image.locations) \u003d\u003d 1 and image.locations[0].get("},{"line_number":291,"context_line":"                    \u0027metadata\u0027, {}).get(\u0027staging_host\u0027):"},{"line_number":292,"context_line":"                msg \u003d _(\"The image %s has data on staging\") % image_id"},{"line_number":293,"context_line":"                raise webob.exc.HTTPConflict(explanation\u003dmsg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"e12394cd_9a5521f7","line":290,"in_reply_to":"aeeebe87_635eb47a","updated":"2021-03-01 16:10:11.000000000","message":"Good point, there should be no way we are ever in this situation.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":304,"context_line":"            image_repo.save(image, from_state\u003d\u0027uploading\u0027)"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        except webob.exc.HTTPConflict:"},{"line_number":307,"context_line":"            raise"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        except exception.NotFound as e:"},{"line_number":310,"context_line":"            raise webob.exc.HTTPNotFound(explanation\u003de.msg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"2515a9d1_b3789ff8","line":307,"range":{"start_line":307,"start_character":12,"end_line":307,"end_character":17},"updated":"2021-02-23 22:39:48.000000000","message":"This is redundant.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":304,"context_line":"            image_repo.save(image, from_state\u003d\u0027uploading\u0027)"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        except webob.exc.HTTPConflict:"},{"line_number":307,"context_line":"            raise"},{"line_number":308,"context_line":""},{"line_number":309,"context_line":"        except exception.NotFound as e:"},{"line_number":310,"context_line":"            raise webob.exc.HTTPNotFound(explanation\u003de.msg)"}],"source_content_type":"text/x-python","patch_set":4,"id":"11f9bdbe_a0f567de","line":307,"range":{"start_line":307,"start_character":12,"end_line":307,"end_character":17},"in_reply_to":"2515a9d1_b3789ff8","updated":"2021-03-01 16:10:11.000000000","message":"how come, we need something in that except block, no?","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"}],"glance/api/v2/images.py":[{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":true,"context_lines":[{"line_number":623,"context_line":"        image_repo \u003d self.gateway.get_repo(req.context)"},{"line_number":624,"context_line":"        try:"},{"line_number":625,"context_line":"            image \u003d image_repo.get(image_id)"},{"line_number":626,"context_line":"            # NOTE(abhishekk): Delete the data from staging area"},{"line_number":627,"context_line":"            if CONF.enabled_backends:"},{"line_number":628,"context_line":"                separator, staging_dir \u003d store_utils.get_dir_separator()"},{"line_number":629,"context_line":"                file_path \u003d \"%s%s%s\" % (staging_dir,"},{"line_number":630,"context_line":"                                        separator,"},{"line_number":631,"context_line":"                                        image_id)"},{"line_number":632,"context_line":"                try:"},{"line_number":633,"context_line":"                    fn_call \u003d glance_store.get_store_from_store_identifier"},{"line_number":634,"context_line":"                    staging_store \u003d fn_call(\u0027os_glance_staging_store\u0027)"},{"line_number":635,"context_line":"                    loc \u003d location.get_location_from_uri_and_backend("},{"line_number":636,"context_line":"                        file_path, \u0027os_glance_staging_store\u0027)"},{"line_number":637,"context_line":"                    staging_store.delete(loc)"},{"line_number":638,"context_line":"                except (glance_store.exceptions.NotFound,"},{"line_number":639,"context_line":"                        glance_store.exceptions.UnknownScheme):"},{"line_number":640,"context_line":"                    pass"},{"line_number":641,"context_line":"            else:"},{"line_number":642,"context_line":"                file_path \u003d str("},{"line_number":643,"context_line":"                    CONF.node_staging_uri + \u0027/\u0027 + image_id)[7:]"},{"line_number":644,"context_line":"                if os.path.exists(file_path):"},{"line_number":645,"context_line":"                    try:"},{"line_number":646,"context_line":"                        LOG.debug("},{"line_number":647,"context_line":"                            \"After upload to the backend, deleting staged \""},{"line_number":648,"context_line":"                            \"image data from %(fn)s\", {\u0027fn\u0027: file_path})"},{"line_number":649,"context_line":"                        os.unlink(file_path)"},{"line_number":650,"context_line":"                    except OSError as e:"},{"line_number":651,"context_line":"                        LOG.error("},{"line_number":652,"context_line":"                            \"After upload to backend, deletion of staged \""},{"line_number":653,"context_line":"                            \"image data from %(fn)s has failed because \""},{"line_number":654,"context_line":"                            \"[Errno %(en)d]\", {\u0027fn\u0027: file_path,"},{"line_number":655,"context_line":"                                               \u0027en\u0027: e.errno})"},{"line_number":656,"context_line":"                else:"},{"line_number":657,"context_line":"                    LOG.warning(_("},{"line_number":658,"context_line":"                        \"After upload to backend, deletion of staged \""},{"line_number":659,"context_line":"                        \"image data has failed because \""},{"line_number":660,"context_line":"                        \"it cannot be found at %(fn)s\"), {\u0027fn\u0027: file_path})"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"            image.delete()"},{"line_number":663,"context_line":"            self._delete_encryption_key(req.context, image)"}],"source_content_type":"text/x-python","patch_set":1,"id":"9d8ace0f_ac414232","line":660,"range":{"start_line":626,"start_character":12,"end_line":660,"end_character":75},"updated":"2021-02-23 12:48:36.000000000","message":"I guess this code is not required now","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":true,"context_lines":[{"line_number":659,"context_line":"                        \"image data has failed because \""},{"line_number":660,"context_line":"                        \"it cannot be found at %(fn)s\"), {\u0027fn\u0027: file_path})"},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"            image.delete()"},{"line_number":663,"context_line":"            self._delete_encryption_key(req.context, image)"},{"line_number":664,"context_line":"            image_repo.remove(image)"},{"line_number":665,"context_line":"        except (glance_store.Forbidden, exception.Forbidden) as e:"}],"source_content_type":"text/x-python","patch_set":1,"id":"7914cf62_9c56d414","line":662,"range":{"start_line":662,"start_character":12,"end_line":662,"end_character":26},"updated":"2021-02-23 12:48:36.000000000","message":"Confirmed that if store.delete (os.unlink) fails to delete the staging data then image deletion fails and we have track of location data intact.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":true,"context_lines":[{"line_number":1327,"context_line":"                for loc in tmp_locations:"},{"line_number":1328,"context_line":"                    # NOTE(jokke): Lets hide the staging_host details here"},{"line_number":1329,"context_line":"                    # rather than in every place that consumes this result"},{"line_number":1330,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1331,"context_line":"                        loc[\u0027metadata\u0027][\u0027staging_host\u0027] \u003d \"***HIDDEN***\""},{"line_number":1332,"context_line":"                return tmp_locations"},{"line_number":1333,"context_line":"            except exception.Forbidden:"},{"line_number":1334,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":1,"id":"1c8add4b_fd5e2dd1","line":1331,"range":{"start_line":1330,"start_character":21,"end_line":1331,"end_character":72},"updated":"2021-02-23 12:48:36.000000000","message":"I guess we decided to not to show staging related locations to user at all.\n\nAlso this also shown in the \"stores\" property in response, using this user can easily issue command delete-from-store \n\n| id               | 2cff9379-5219-4342-8c63-f7bcd742d643                                             |\n| locations        | [{\"url\": \"file:///opt/stack/data/glance/os_glance_staging_store/2cff9379-5219-43 |\n|                  | 42-8c63-f7bcd742d643\", \"metadata\": {\"store\": \"os_glance_staging_store\",          |\n|                  | \"staging_host\": \"***HIDDEN***\"}}]                                                |                                                                         |\n| status           | uploading                                                                        |\n| stores           | os_glance_staging_store                                                          |\n|","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"f0c266eab59d84a1d59497932dba29441cddf63a","unresolved":true,"context_lines":[{"line_number":1327,"context_line":"                for loc in tmp_locations:"},{"line_number":1328,"context_line":"                    # NOTE(jokke): Lets hide the staging_host details here"},{"line_number":1329,"context_line":"                    # rather than in every place that consumes this result"},{"line_number":1330,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1331,"context_line":"                        loc[\u0027metadata\u0027][\u0027staging_host\u0027] \u003d \"***HIDDEN***\""},{"line_number":1332,"context_line":"                return tmp_locations"},{"line_number":1333,"context_line":"            except exception.Forbidden:"},{"line_number":1334,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":1,"id":"3c4873a4_1ba78827","line":1331,"range":{"start_line":1330,"start_character":21,"end_line":1331,"end_character":72},"in_reply_to":"1c8add4b_fd5e2dd1","updated":"2021-02-23 13:35:09.000000000","message":"That will break way too many things on the locations API, like all the patch operations. We need to indicate that the location exists, we can make sure we don\u0027t expose those host details. Th reason I left the \"staging_host\" key visible was to ensure user who has access to locations does know what it is and to help debugging (say there is failure to delete image due to not being able to delete the staging location)","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":1327,"context_line":"                for loc in tmp_locations:"},{"line_number":1328,"context_line":"                    # NOTE(jokke): Lets hide the staging_host details here"},{"line_number":1329,"context_line":"                    # rather than in every place that consumes this result"},{"line_number":1330,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1331,"context_line":"                        loc[\u0027metadata\u0027][\u0027staging_host\u0027] \u003d \"***HIDDEN***\""},{"line_number":1332,"context_line":"                return tmp_locations"},{"line_number":1333,"context_line":"            except exception.Forbidden:"},{"line_number":1334,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":1,"id":"26eb7053_4f057403","line":1331,"range":{"start_line":1330,"start_character":21,"end_line":1331,"end_character":72},"in_reply_to":"3c4873a4_1ba78827","updated":"2021-02-23 22:39:48.000000000","message":"I think we should absolutely delete it. This exposes something about the backend config just by the virtue of it being here. It\u0027s also not useful to a client, so there\u0027s really no point in it being here. Also, this seems like maybe residue because of the below - won\u0027t we never be showing locations with this key set anyway?","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":1328,"context_line":"                    # NOTE(jokke): Lets hide the staging_host details here"},{"line_number":1329,"context_line":"                    # rather than in every place that consumes this result"},{"line_number":1330,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1331,"context_line":"                        loc[\u0027metadata\u0027][\u0027staging_host\u0027] \u003d \"***HIDDEN***\""},{"line_number":1332,"context_line":"                return tmp_locations"},{"line_number":1333,"context_line":"            except exception.Forbidden:"},{"line_number":1334,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":4,"id":"a239043b_95ccbe03","line":1331,"range":{"start_line":1331,"start_character":58,"end_line":1331,"end_character":72},"updated":"2021-02-23 22:39:48.000000000","message":"Echoing the concern with this approach, but also the confusion about whether or not this is ever even run, if we\u0027re deleting any location that looks to be staging-related from the list.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":1328,"context_line":"                    # NOTE(jokke): Lets hide the staging_host details here"},{"line_number":1329,"context_line":"                    # rather than in every place that consumes this result"},{"line_number":1330,"context_line":"                    if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1331,"context_line":"                        loc[\u0027metadata\u0027][\u0027staging_host\u0027] \u003d \"***HIDDEN***\""},{"line_number":1332,"context_line":"                return tmp_locations"},{"line_number":1333,"context_line":"            except exception.Forbidden:"},{"line_number":1334,"context_line":"                return []"}],"source_content_type":"text/x-python","patch_set":4,"id":"134a9316_41e426f8","line":1331,"range":{"start_line":1331,"start_character":58,"end_line":1331,"end_character":72},"in_reply_to":"a239043b_95ccbe03","updated":"2021-03-01 16:10:11.000000000","message":"I think this should be addressed on the ps5","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":1369,"context_line":"                    # NOTE(jokke): Never provide staging location as direct_url"},{"line_number":1370,"context_line":"                    for i, loc in enumerate(locations):"},{"line_number":1371,"context_line":"                        if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1372,"context_line":"                            locations.pop(i)"},{"line_number":1373,"context_line":"                    # Do we still have locations left?"},{"line_number":1374,"context_line":"                if locations:"},{"line_number":1375,"context_line":"                    # Choose best location configured strategy"}],"source_content_type":"text/x-python","patch_set":4,"id":"9ea6a54a_5428ec9f","line":1372,"updated":"2021-02-23 22:39:48.000000000","message":"AFAICT, the image update API will allow an external entity to add locations directly the the image (AFAIK nova has to do this when it does an RBD-resident snapshot). If something adds a location with metadata containing staging_host, we will quietly never show that location because of this, right? That is a problem, IMHO.\n\nWhat if I stage an image, then update the locations and put something in stage_host that causes a subsequent import to copy /etc/shadow to my final image?\n\nI guess you need to implement a new reserved os_glance_* namespacing scheme for the location metadata as well?","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":1369,"context_line":"                    # NOTE(jokke): Never provide staging location as direct_url"},{"line_number":1370,"context_line":"                    for i, loc in enumerate(locations):"},{"line_number":1371,"context_line":"                        if loc.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":1372,"context_line":"                            locations.pop(i)"},{"line_number":1373,"context_line":"                    # Do we still have locations left?"},{"line_number":1374,"context_line":"                if locations:"},{"line_number":1375,"context_line":"                    # Choose best location configured strategy"}],"source_content_type":"text/x-python","patch_set":4,"id":"5d091b2d_a8faabcf","line":1372,"in_reply_to":"9ea6a54a_5428ec9f","updated":"2021-03-01 16:10:11.000000000","message":"\u003e AFAICT, the image update API will allow an external entity to add locations directly the the image (AFAIK nova has to do this when it does an RBD-resident snapshot). If something adds a location with metadata containing staging_host, we will quietly never show that location because of this, right? That is a problem, IMHO.\n\u003e \nThe metadata namespace collision is addressed in PS5, to be clear this filtering here is about what gets presented as direct_url for the image and we should never present the staging location as direct_url for anyone external to consume.\n\n\u003e What if I stage an image, then update the locations and put something in stage_host that causes a subsequent import to copy /etc/shadow to my final image?\n\u003e\nWe had CVE few years back about this part of the locations API. One cannot create locations to the image via the locations api that are `file:` (or `swift+config:`) so again, no special case this is already handled by our existing production code.\n  \n\u003e I guess you need to implement a new reserved os_glance_* namespacing scheme for the location metadata as well?\nThis is taken into account on the refactoring leading to PS5","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"}],"glance/async_/flows/_internal_plugins/copy_image.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":false,"context_lines":[{"line_number":51,"context_line":"        :param image_id: Glance Image ID"},{"line_number":52,"context_line":"        \"\"\""},{"line_number":53,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":54,"context_line":"        LOG.debug(image.locations)"},{"line_number":55,"context_line":"        # NOTE(jokke): If we already have data staged, lets use that."},{"line_number":56,"context_line":"        for location in image.locations:"},{"line_number":57,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f2307c1_a67d820c","line":54,"updated":"2021-02-22 23:25:41.000000000","message":"oops, this was left behind, removed for next PS.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":false,"context_lines":[{"line_number":95,"context_line":"        action.set_image_data(loc[\u0027url\u0027], self.task_id,"},{"line_number":96,"context_line":"                              \u0027os_glance_staging_store\u0027, False,"},{"line_number":97,"context_line":"                              staging\u003dTrue, image_data\u003dimage_data)"},{"line_number":98,"context_line":"        LOG.debug(image.locations)"},{"line_number":99,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":100,"context_line":"        LOG.debug(image.locations)"},{"line_number":101,"context_line":"        for location in image.locations:"}],"source_content_type":"text/x-python","patch_set":1,"id":"bc7cec2c_45e5dfd6","line":98,"updated":"2021-02-22 23:25:41.000000000","message":"ditto","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":false,"context_lines":[{"line_number":97,"context_line":"                              staging\u003dTrue, image_data\u003dimage_data)"},{"line_number":98,"context_line":"        LOG.debug(image.locations)"},{"line_number":99,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":100,"context_line":"        LOG.debug(image.locations)"},{"line_number":101,"context_line":"        for location in image.locations:"},{"line_number":102,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":103,"context_line":"                return location[\u0027url\u0027]"}],"source_content_type":"text/x-python","patch_set":1,"id":"72c5c880_4f38603d","line":100,"updated":"2021-02-22 23:25:41.000000000","message":"ditto","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":true,"context_lines":[{"line_number":109,"context_line":"                      {\u0027task_id\u0027: self.task_id,"},{"line_number":110,"context_line":"                       \u0027image_id\u0027: self.image_id})"},{"line_number":111,"context_line":"        with self.action_wrapper as action:"},{"line_number":112,"context_line":"            action.remove_staging_location()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"def get_flow(**kwargs):"}],"source_content_type":"text/x-python","patch_set":1,"id":"2f2c65c7_065533fb","line":112,"updated":"2021-02-22 23:25:41.000000000","message":"Not sure about this. We either left the staging data behind to speed up next run, but we just might have overlooked this in the first place. Thought consistent cleanup is likely good approach.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":true,"context_lines":[{"line_number":109,"context_line":"                      {\u0027task_id\u0027: self.task_id,"},{"line_number":110,"context_line":"                       \u0027image_id\u0027: self.image_id})"},{"line_number":111,"context_line":"        with self.action_wrapper as action:"},{"line_number":112,"context_line":"            action.remove_staging_location()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"def get_flow(**kwargs):"}],"source_content_type":"text/x-python","patch_set":1,"id":"38e9ac71_3e058786","line":112,"range":{"start_line":112,"start_character":19,"end_line":112,"end_character":42},"updated":"2021-02-23 12:48:36.000000000","message":"as per the logic/fundamental of taskflow we should revert what execute is doing in case of failure, so removing the location makes more sense here.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"f0c266eab59d84a1d59497932dba29441cddf63a","unresolved":false,"context_lines":[{"line_number":109,"context_line":"                      {\u0027task_id\u0027: self.task_id,"},{"line_number":110,"context_line":"                       \u0027image_id\u0027: self.image_id})"},{"line_number":111,"context_line":"        with self.action_wrapper as action:"},{"line_number":112,"context_line":"            action.remove_staging_location()"},{"line_number":113,"context_line":""},{"line_number":114,"context_line":""},{"line_number":115,"context_line":"def get_flow(**kwargs):"}],"source_content_type":"text/x-python","patch_set":1,"id":"0a8eac8e_ab44f42d","line":112,"range":{"start_line":112,"start_character":19,"end_line":112,"end_character":42},"in_reply_to":"38e9ac71_3e058786","updated":"2021-02-23 13:35:09.000000000","message":"Ack","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":53,"context_line":"        # NOTE (abhishekk): If ``all_stores_must_succeed`` is set to True"},{"line_number":54,"context_line":"        # and copying task fails then we keep data in staging area as it"},{"line_number":55,"context_line":"        # is so that if second call is made to copy the same image then"},{"line_number":56,"context_line":"        # no need to copy the data in staging area again."},{"line_number":57,"context_line":"        file_path \u003d \"%s/%s\" % (getattr("},{"line_number":58,"context_line":"            CONF, \u0027os_glance_staging_store\u0027).filesystem_store_datadir,"},{"line_number":59,"context_line":"            self.image_id)"}],"source_content_type":"text/x-python","patch_set":3,"id":"615f0990_1b6bbfbb","side":"PARENT","line":56,"updated":"2021-02-23 22:39:48.000000000","message":"Is this comment removed because it\u0027s no longer relevant? If so, can you explain why?","commit_id":"ac53e433e23419defc745bb42b2419daca87adc5"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"59a2b0006e2ed47fe8dbec25a5054d5aa37b60a5","unresolved":true,"context_lines":[{"line_number":54,"context_line":"        # and copying task fails then we keep data in staging area as it"},{"line_number":55,"context_line":"        # is so that if second call is made to copy the same image then"},{"line_number":56,"context_line":"        # no need to copy the data in staging area again."},{"line_number":57,"context_line":"        file_path \u003d \"%s/%s\" % (getattr("},{"line_number":58,"context_line":"            CONF, \u0027os_glance_staging_store\u0027).filesystem_store_datadir,"},{"line_number":59,"context_line":"            self.image_id)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"        if os.path.exists(file_path):"},{"line_number":62,"context_line":"            # NOTE (abhishekk): If previous copy-image operation is failed"},{"line_number":63,"context_line":"            # due to power failure, network failure or any other reason and"},{"line_number":64,"context_line":"            # the image data here is partial then clear the staging area and"},{"line_number":65,"context_line":"            # re-stage the fresh image data."},{"line_number":66,"context_line":"            # Ref: https://bugs.launchpad.net/glance/+bug/1885003"},{"line_number":67,"context_line":"            size_in_staging \u003d os.path.getsize(file_path)"},{"line_number":68,"context_line":"            if image.size \u003d\u003d size_in_staging:"},{"line_number":69,"context_line":"                return file_path, 0"},{"line_number":70,"context_line":"            else:"},{"line_number":71,"context_line":"                LOG.debug((\"Found partial image data in staging \""},{"line_number":72,"context_line":"                           \"%(fn)s, deleting it to re-stage \""},{"line_number":73,"context_line":"                           \"again\"), {\u0027fn\u0027: file_path})"},{"line_number":74,"context_line":"                try:"},{"line_number":75,"context_line":"                    os.unlink(file_path)"},{"line_number":76,"context_line":"                except OSError as e:"},{"line_number":77,"context_line":"                    LOG.error(_LE(\"Deletion of staged \""},{"line_number":78,"context_line":"                                  \"image data from %(fn)s has failed because \""},{"line_number":79,"context_line":"                                  \"[Errno %(en)d]\"), {\u0027fn\u0027: file_path,"},{"line_number":80,"context_line":"                                                      \u0027en\u0027: e.errno})"},{"line_number":81,"context_line":"                    raise"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        # At first search image in default_backend"},{"line_number":84,"context_line":"        default_store \u003d CONF.glance_store.default_backend"}],"source_content_type":"text/x-python","patch_set":4,"id":"3d8808c1_a9caf22f","side":"PARENT","line":81,"range":{"start_line":57,"start_character":8,"end_line":81,"end_character":25},"updated":"2021-02-24 08:43:09.000000000","message":"In addition to checking location entry for staging we need to have this check as well; there might be possibility that location entry exists but data is tampered or unavailable or deleted by mistake.\n\n\\","commit_id":"ac53e433e23419defc745bb42b2419daca87adc5"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"c20e890f1cdb7d2cb42d5cee589cd99b5c41b288","unresolved":true,"context_lines":[{"line_number":54,"context_line":"        # and copying task fails then we keep data in staging area as it"},{"line_number":55,"context_line":"        # is so that if second call is made to copy the same image then"},{"line_number":56,"context_line":"        # no need to copy the data in staging area again."},{"line_number":57,"context_line":"        file_path \u003d \"%s/%s\" % (getattr("},{"line_number":58,"context_line":"            CONF, \u0027os_glance_staging_store\u0027).filesystem_store_datadir,"},{"line_number":59,"context_line":"            self.image_id)"},{"line_number":60,"context_line":""},{"line_number":61,"context_line":"        if os.path.exists(file_path):"},{"line_number":62,"context_line":"            # NOTE (abhishekk): If previous copy-image operation is failed"},{"line_number":63,"context_line":"            # due to power failure, network failure or any other reason and"},{"line_number":64,"context_line":"            # the image data here is partial then clear the staging area and"},{"line_number":65,"context_line":"            # re-stage the fresh image data."},{"line_number":66,"context_line":"            # Ref: https://bugs.launchpad.net/glance/+bug/1885003"},{"line_number":67,"context_line":"            size_in_staging \u003d os.path.getsize(file_path)"},{"line_number":68,"context_line":"            if image.size \u003d\u003d size_in_staging:"},{"line_number":69,"context_line":"                return file_path, 0"},{"line_number":70,"context_line":"            else:"},{"line_number":71,"context_line":"                LOG.debug((\"Found partial image data in staging \""},{"line_number":72,"context_line":"                           \"%(fn)s, deleting it to re-stage \""},{"line_number":73,"context_line":"                           \"again\"), {\u0027fn\u0027: file_path})"},{"line_number":74,"context_line":"                try:"},{"line_number":75,"context_line":"                    os.unlink(file_path)"},{"line_number":76,"context_line":"                except OSError as e:"},{"line_number":77,"context_line":"                    LOG.error(_LE(\"Deletion of staged \""},{"line_number":78,"context_line":"                                  \"image data from %(fn)s has failed because \""},{"line_number":79,"context_line":"                                  \"[Errno %(en)d]\"), {\u0027fn\u0027: file_path,"},{"line_number":80,"context_line":"                                                      \u0027en\u0027: e.errno})"},{"line_number":81,"context_line":"                    raise"},{"line_number":82,"context_line":""},{"line_number":83,"context_line":"        # At first search image in default_backend"},{"line_number":84,"context_line":"        default_store \u003d CONF.glance_store.default_backend"}],"source_content_type":"text/x-python","patch_set":4,"id":"1752c511_465c043a","side":"PARENT","line":81,"range":{"start_line":57,"start_character":8,"end_line":81,"end_character":25},"in_reply_to":"3d8808c1_a9caf22f","updated":"2021-02-24 13:48:18.000000000","message":"Indeed, and that makes me wonder... the special case in the location code that avoids setting image.size if staging\u003dTrue... won\u0027t that leave us with no record of the image size to compare like L68 does currently? It seems like we must be setting image.size right now when we stage so we can compare before import, so why does this new code avoid setting it if staging\u003dTrue?","commit_id":"ac53e433e23419defc745bb42b2419daca87adc5"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":32,"context_line":"    default_provides \u003d \u0027file_uri\u0027"},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self, task_id, task_type, image_repo, image_id,"},{"line_number":35,"context_line":"                 action_wrapper):"},{"line_number":36,"context_line":"        self.task_id \u003d task_id"},{"line_number":37,"context_line":"        self.task_type \u003d task_type"},{"line_number":38,"context_line":"        self.image_repo \u003d image_repo"}],"source_content_type":"text/x-python","patch_set":4,"id":"92c30b01_8d456039","line":35,"updated":"2021-02-23 22:39:48.000000000","message":"This (and all the related bits) will conflict with this other large bugfix series:\n\nhttps://review.opendev.org/q/topic:%2522bug/1914826%2522\n\nThat series is ready to go, just needs +W, and has been sitting a while. So I think we should land that and then this can rebase after that\u0027s done. That series breaks this out into its own refactor anyway, which I would normally ask for in this case, as cramming all of this into a single patch makes it unnecessarily large.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":54,"context_line":"        # NOTE(jokke): If we already have data staged, lets use that."},{"line_number":55,"context_line":"        for location in image.locations:"},{"line_number":56,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":57,"context_line":"                return location[\u0027url\u0027]"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"        # At first search image in default_backend"},{"line_number":60,"context_line":"        default_store \u003d CONF.glance_store.default_backend"}],"source_content_type":"text/x-python","patch_set":4,"id":"d664c0e7_7593cad1","line":57,"updated":"2021-02-23 22:39:48.000000000","message":"I think this creates an upgrade breakage, where if I have staged my multi-TB image before the upgrade window starts, after this code is applied my should-be-valid-to-import image will not be able to find its staging file and thus fail (or maybe re-download it?).\n\nI think either you have to migrate all queued images to have a location (not good) or keep code here to honor the old \"might just be in the staging directory\" method for at least one cycle for regular upgrades, and likely a number of them to cover FFUs.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":54,"context_line":"        # NOTE(jokke): If we already have data staged, lets use that."},{"line_number":55,"context_line":"        for location in image.locations:"},{"line_number":56,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":57,"context_line":"                return location[\u0027url\u0027]"},{"line_number":58,"context_line":""},{"line_number":59,"context_line":"        # At first search image in default_backend"},{"line_number":60,"context_line":"        default_store \u003d CONF.glance_store.default_backend"}],"source_content_type":"text/x-python","patch_set":4,"id":"dfa52a01_003ed494","line":57,"in_reply_to":"d664c0e7_7593cad1","updated":"2021-03-01 16:10:11.000000000","message":"We can address this as reno/upgrade check afterwards, right ;)","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"}],"glance/async_/flows/_internal_plugins/web_download.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":false,"context_lines":[{"line_number":58,"context_line":"        for location in image.locations:"},{"line_number":59,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":60,"context_line":"                self._path \u003d location[\u0027url\u0027][7:]"},{"line_number":61,"context_line":"                return self._path"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        # NOTE(jokke): We\u0027ve decided to use staging area for this task as"},{"line_number":64,"context_line":"        # a way to expect users to configure a local store for pre-import"}],"source_content_type":"text/x-python","patch_set":1,"id":"6dd45369_14200bd8","line":61,"updated":"2021-02-22 23:25:41.000000000","message":"Must have not saved the file. I\u0027m sure I already cleaned this out already once, we basically never should have existing data when doing web_download.\n\nRemoved from next PS.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":false,"context_lines":[{"line_number":58,"context_line":"        for location in image.locations:"},{"line_number":59,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":60,"context_line":"                self._path \u003d location[\u0027url\u0027][7:]"},{"line_number":61,"context_line":"                return self._path"},{"line_number":62,"context_line":""},{"line_number":63,"context_line":"        # NOTE(jokke): We\u0027ve decided to use staging area for this task as"},{"line_number":64,"context_line":"        # a way to expect users to configure a local store for pre-import"}],"source_content_type":"text/x-python","patch_set":1,"id":"2255d237_dc01162e","line":61,"in_reply_to":"6dd45369_14200bd8","updated":"2021-02-23 12:48:36.000000000","message":"++ this will never be case","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":71,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":72,"context_line":"        for location in image.locations:"},{"line_number":73,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":74,"context_line":"                self._path \u003d location[\u0027url\u0027][7:]"},{"line_number":75,"context_line":"        return self._path"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    def revert(self, result, **kwargs):"}],"source_content_type":"text/x-python","patch_set":4,"id":"b5ebca08_b2abbec9","line":74,"updated":"2021-02-23 22:39:48.000000000","message":"Are you intentionally taking the last location with a staging_host? Shouldn\u0027t this have a break here? Or maybe some checks to make sure there is only one, and bail if unexpectedly we find two? What if we find none? You\u0027ll get a NameError on L83.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":71,"context_line":"        image \u003d self.image_repo.get(self.image_id)"},{"line_number":72,"context_line":"        for location in image.locations:"},{"line_number":73,"context_line":"            if location.get(\u0027metadata\u0027).get(\u0027staging_host\u0027):"},{"line_number":74,"context_line":"                self._path \u003d location[\u0027url\u0027][7:]"},{"line_number":75,"context_line":"        return self._path"},{"line_number":76,"context_line":""},{"line_number":77,"context_line":"    def revert(self, result, **kwargs):"}],"source_content_type":"text/x-python","patch_set":4,"id":"3447d84b_2dfd1092","line":74,"in_reply_to":"b5ebca08_b2abbec9","updated":"2021-03-01 16:10:11.000000000","message":"makes no difference as there never should be more than one. fixed the indent on PS5","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"}],"glance/async_/flows/api_image_import.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":true,"context_lines":[{"line_number":347,"context_line":""},{"line_number":348,"context_line":"    def execute(self):"},{"line_number":349,"context_line":"        with self.action_wrapper as action:"},{"line_number":350,"context_line":"            self._execute(action)"},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"    def _execute(self, action):"},{"line_number":353,"context_line":"        \"\"\"Remove file from the backend"}],"source_content_type":"text/x-python","patch_set":1,"id":"0e47a3d6_34292094","line":350,"updated":"2021-02-22 23:25:41.000000000","message":"I don\u0027t think we need this, more consistent with the rest of the module but we\u0027re really doing only one function call there anyways. Opinions?","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":347,"context_line":""},{"line_number":348,"context_line":"    def execute(self):"},{"line_number":349,"context_line":"        with self.action_wrapper as action:"},{"line_number":350,"context_line":"            self._execute(action)"},{"line_number":351,"context_line":""},{"line_number":352,"context_line":"    def _execute(self, action):"},{"line_number":353,"context_line":"        \"\"\"Remove file from the backend"}],"source_content_type":"text/x-python","patch_set":1,"id":"feed3a16_f57bb49f","line":350,"in_reply_to":"0e47a3d6_34292094","updated":"2021-02-23 22:39:48.000000000","message":"I much prefer just doing what you have here for consistency in how we get the image and do the store cleanup.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"}],"glance/location.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":true,"context_lines":[{"line_number":496,"context_line":"            self.image.os_hash_value \u003d multihash"},{"line_number":497,"context_line":"            self.image.size \u003d size"},{"line_number":498,"context_line":"            self.image.os_hash_algo \u003d hashing_algo"},{"line_number":499,"context_line":"        else:"},{"line_number":500,"context_line":"            if CONF.enabled_backends:"},{"line_number":501,"context_line":"                (location,"},{"line_number":502,"context_line":"                 size,"}],"source_content_type":"text/x-python","patch_set":1,"id":"a8c2c90d_91253544","line":499,"updated":"2021-02-22 23:25:41.000000000","message":"AKA `if staging:`","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":true,"context_lines":[{"line_number":598,"context_line":"                 set_active\u003dTrue, staging\u003dFalse):"},{"line_number":599,"context_line":"        if size is None:"},{"line_number":600,"context_line":"            size \u003d 0  # NOTE(markwash): zero -\u003e unknown size"},{"line_number":601,"context_line":"        if staging:"},{"line_number":602,"context_line":"            set_active \u003d False  # NOTE(jokke): never activate while staging"},{"line_number":603,"context_line":""},{"line_number":604,"context_line":"        # Create the verifier for signature verification (if correct properties"},{"line_number":605,"context_line":"        # are present)"}],"source_content_type":"text/x-python","patch_set":1,"id":"70d5a955_f8169bd7","line":602,"range":{"start_line":601,"start_character":8,"end_line":602,"end_character":75},"updated":"2021-02-23 12:48:36.000000000","message":"Why do we need this staging flag, we can set_active\u003dFalse while calling set_data method and also can pass this set_active at line #636\n\nAnd also if we managed to do so then we could avoid passing this new parameter from entire onion layer (which means less files to review)","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"f0c266eab59d84a1d59497932dba29441cddf63a","unresolved":true,"context_lines":[{"line_number":598,"context_line":"                 set_active\u003dTrue, staging\u003dFalse):"},{"line_number":599,"context_line":"        if size is None:"},{"line_number":600,"context_line":"            size \u003d 0  # NOTE(markwash): zero -\u003e unknown size"},{"line_number":601,"context_line":"        if staging:"},{"line_number":602,"context_line":"            set_active \u003d False  # NOTE(jokke): never activate while staging"},{"line_number":603,"context_line":""},{"line_number":604,"context_line":"        # Create the verifier for signature verification (if correct properties"},{"line_number":605,"context_line":"        # are present)"}],"source_content_type":"text/x-python","patch_set":1,"id":"71ba20ec_c6061d5c","line":602,"range":{"start_line":601,"start_character":8,"end_line":602,"end_character":75},"in_reply_to":"70d5a955_f8169bd7","updated":"2021-02-23 13:35:09.000000000","message":"Well the options were that we pass indication to the set_data/upload_image_data that we\u0027re staging or we have the rest of the logic duplicated through all the layers by making it new function call. If we were looking just set_active flag, when ever we did image import we would be \"staging\" all but the last image as we default to set_active\u003dFalse due to the all_stores_must_succeed defaulting True. As the set_active is kwarg, we can literally call image.set_data(payload, staging\u003dTrue) we do not need to pass any of the other parameters and we still get the correct result, also as long as staging is not passed or passed as staging\u003dFalse the functionality works exactly as before and we don\u0027t need to refactor any of the logic relying on that.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":598,"context_line":"                 set_active\u003dTrue, staging\u003dFalse):"},{"line_number":599,"context_line":"        if size is None:"},{"line_number":600,"context_line":"            size \u003d 0  # NOTE(markwash): zero -\u003e unknown size"},{"line_number":601,"context_line":"        if staging:"},{"line_number":602,"context_line":"            set_active \u003d False  # NOTE(jokke): never activate while staging"},{"line_number":603,"context_line":""},{"line_number":604,"context_line":"        # Create the verifier for signature verification (if correct properties"},{"line_number":605,"context_line":"        # are present)"}],"source_content_type":"text/x-python","patch_set":1,"id":"79ea4c06_df277651","line":602,"range":{"start_line":601,"start_character":8,"end_line":602,"end_character":75},"in_reply_to":"71ba20ec_c6061d5c","updated":"2021-02-23 22:39:48.000000000","message":"Agree with Abhi, this too is much too much linkage between layers that should be independent. If we are creating a location in the staging store, we should provide set_active\u003dFalse here. Why do we need a new special case flag?","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"9e75d8bd062ecec6775b71f9fd24ba70063f1ef4","unresolved":true,"context_lines":[{"line_number":635,"context_line":""},{"line_number":636,"context_line":"        self._upload_to_store(data, verifier, backend, size, staging\u003dstaging)"},{"line_number":637,"context_line":""},{"line_number":638,"context_line":"        if fmt and fmt.format_match and fmt.virtual_size:"},{"line_number":639,"context_line":"            self.image.virtual_size \u003d fmt.virtual_size"},{"line_number":640,"context_line":"            LOG.info(\u0027Image format matched and virtual size computed: %i\u0027,"},{"line_number":641,"context_line":"                     self.image.virtual_size)"},{"line_number":642,"context_line":"        elif fmt:"},{"line_number":643,"context_line":"            LOG.warning(\u0027Image format %s did not match; \u0027"},{"line_number":644,"context_line":"                        \u0027unable to calculate virtual size\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"2e50570c_48d0823f","line":641,"range":{"start_line":638,"start_character":8,"end_line":641,"end_character":45},"updated":"2021-02-23 12:48:36.000000000","message":"also we need to avoid setting virtual size during staging operation.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"f0c266eab59d84a1d59497932dba29441cddf63a","unresolved":true,"context_lines":[{"line_number":635,"context_line":""},{"line_number":636,"context_line":"        self._upload_to_store(data, verifier, backend, size, staging\u003dstaging)"},{"line_number":637,"context_line":""},{"line_number":638,"context_line":"        if fmt and fmt.format_match and fmt.virtual_size:"},{"line_number":639,"context_line":"            self.image.virtual_size \u003d fmt.virtual_size"},{"line_number":640,"context_line":"            LOG.info(\u0027Image format matched and virtual size computed: %i\u0027,"},{"line_number":641,"context_line":"                     self.image.virtual_size)"},{"line_number":642,"context_line":"        elif fmt:"},{"line_number":643,"context_line":"            LOG.warning(\u0027Image format %s did not match; \u0027"},{"line_number":644,"context_line":"                        \u0027unable to calculate virtual size\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"ff622f87_52e54b30","line":641,"range":{"start_line":638,"start_character":8,"end_line":641,"end_character":45},"in_reply_to":"2e50570c_48d0823f","updated":"2021-02-23 13:35:09.000000000","message":"Vitrual size should not be a problem. Even after the conversion the virtual size will be the same, only thing that changes is the actual image size. We can put check for it there if preferred, I just didn\u0027t see need for it as it won\u0027t change the end result.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":635,"context_line":""},{"line_number":636,"context_line":"        self._upload_to_store(data, verifier, backend, size, staging\u003dstaging)"},{"line_number":637,"context_line":""},{"line_number":638,"context_line":"        if fmt and fmt.format_match and fmt.virtual_size:"},{"line_number":639,"context_line":"            self.image.virtual_size \u003d fmt.virtual_size"},{"line_number":640,"context_line":"            LOG.info(\u0027Image format matched and virtual size computed: %i\u0027,"},{"line_number":641,"context_line":"                     self.image.virtual_size)"},{"line_number":642,"context_line":"        elif fmt:"},{"line_number":643,"context_line":"            LOG.warning(\u0027Image format %s did not match; \u0027"},{"line_number":644,"context_line":"                        \u0027unable to calculate virtual size\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"a2faedbb_e1cfc2e3","line":641,"range":{"start_line":638,"start_character":8,"end_line":641,"end_character":45},"in_reply_to":"ff622f87_52e54b30","updated":"2021-02-23 22:39:48.000000000","message":"I don\u0027t think you can say that it doesn\u0027t matter. Some image formats (I\u0027m thinking HDX) have minimum (or default) extent sizes, so that if you upload an image that is not an equal number of extents, the conversion process will have to inflate it to the next size up.\n\nHowever, this is another good example of my general concern that treating the staging area as a store brings a bunch of special cases that negate (or at least erode) the supposed gains we get from it. Right now, we barely use it as a store, but fleshing out its usage in this patch uncovers all of these places where we have to do special handling. I think that\u0027s concerning.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"39ede897fa0f2a2f6f780ceb1370043f2c97d2b1","unresolved":true,"context_lines":[{"line_number":524,"context_line":"                                  CONF.node_staging_uri[7:],"},{"line_number":525,"context_line":"                                  group\u003d\u0027glance_store\u0027)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"                staging_store \u003d backend._load_store(conf, \u0027file\u0027)"},{"line_number":528,"context_line":""},{"line_number":529,"context_line":"                try:"},{"line_number":530,"context_line":"                    staging_store.configure()"}],"source_content_type":"text/x-python","patch_set":3,"id":"2ab337dc_0ca468be","line":527,"range":{"start_line":527,"start_character":32,"end_line":527,"end_character":65},"updated":"2021-02-23 14:14:21.000000000","message":"Now we need to document it somewhere that if we are using single store configuration then we must have to define \u0027file\u0027 store compulsory in `stores` option under glance_store section even if we are not using it.\n\nOtherwise it will fail with 500 error at line #540 with error;\nglance_store.exceptions.UnknownScheme: Unknown scheme \u0027file\u0027 found in URI\n\nNote:\nThis is not a regression of this patch, this is pre-existing bug.\nTo fix this we need to catch above error and raise BadRequest to user","commit_id":"ae34cb91795a9dc92c98c3871d30f13383809208"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":229,"context_line":"            raise exception.DuplicateLocation(location\u003dlocation[\u0027url\u0027])"},{"line_number":230,"context_line":""},{"line_number":231,"context_line":"        self.value.insert(i, location)"},{"line_number":232,"context_line":"        if not location[\u0027metadata\u0027].get(\u0027staging_host\u0027):"},{"line_number":233,"context_line":"            # NOTE(jokke): Set the image size only if not staging as we don\u0027t"},{"line_number":234,"context_line":"            # know what the final image size will be after possible conversion"},{"line_number":235,"context_line":"            _set_image_size(self.image_proxy.context,"}],"source_content_type":"text/x-python","patch_set":4,"id":"4b55f673_fec469ae","line":232,"updated":"2021-02-23 22:39:48.000000000","message":"Same deal here: I think that if the API user creates the only location for this image directly, and happens to be using \"staging_host\" in metadata, you will avoid setting the image size here.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":229,"context_line":"            raise exception.DuplicateLocation(location\u003dlocation[\u0027url\u0027])"},{"line_number":230,"context_line":""},{"line_number":231,"context_line":"        self.value.insert(i, location)"},{"line_number":232,"context_line":"        if not location[\u0027metadata\u0027].get(\u0027staging_host\u0027):"},{"line_number":233,"context_line":"            # NOTE(jokke): Set the image size only if not staging as we don\u0027t"},{"line_number":234,"context_line":"            # know what the final image size will be after possible conversion"},{"line_number":235,"context_line":"            _set_image_size(self.image_proxy.context,"}],"source_content_type":"text/x-python","patch_set":4,"id":"7908d957_af4e04dd","line":232,"in_reply_to":"4b55f673_fec469ae","updated":"2021-03-01 16:10:11.000000000","message":"addressed in PS5 by avoiding the usage of \u0027metadata\u0027","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":459,"context_line":"        \"\"\""},{"line_number":460,"context_line":"        # NOTE(jokke): This whole function shrinks to just couple of dozen"},{"line_number":461,"context_line":"        # lines after we get rid of the old config logic making this super"},{"line_number":462,"context_line":"        # hacky at the moment."},{"line_number":463,"context_line":"        if not staging:"},{"line_number":464,"context_line":"            hashing_algo \u003d self.image.os_hash_algo or CONF[\u0027hashing_algorithm\u0027]"},{"line_number":465,"context_line":"            if CONF.enabled_backends:"}],"source_content_type":"text/x-python","patch_set":4,"id":"0a9f4c0c_2898516c","line":462,"updated":"2021-02-23 22:39:48.000000000","message":"But it doesn\u0027t shrink much in that you\u0027re planning to keep this \"if staging\" special case here right?\n\nAs I mentioned before, this method is now over a hundred lines long and far too complex, IMHO. It is at least four pieces, existing in two hemispheres (i.e. \"what we normally do\" and \"what we do if this is the special staging case\"). It effectively bakes a bunch of high-level logic into a very low-level routine, which I think is a serious anti-pattern. However, if we\u0027re going to design it that way, it really needs to be broken into pieces so it\u0027s more readable.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":459,"context_line":"        \"\"\""},{"line_number":460,"context_line":"        # NOTE(jokke): This whole function shrinks to just couple of dozen"},{"line_number":461,"context_line":"        # lines after we get rid of the old config logic making this super"},{"line_number":462,"context_line":"        # hacky at the moment."},{"line_number":463,"context_line":"        if not staging:"},{"line_number":464,"context_line":"            hashing_algo \u003d self.image.os_hash_algo or CONF[\u0027hashing_algorithm\u0027]"},{"line_number":465,"context_line":"            if CONF.enabled_backends:"}],"source_content_type":"text/x-python","patch_set":4,"id":"489d6e09_8912db8b","line":462,"in_reply_to":"0a9f4c0c_2898516c","updated":"2021-03-01 16:10:11.000000000","message":"it really does, I did some dedup there for PS5 and really the non-multi-store config is what causes lots of the code duplication here.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":460,"context_line":"        # NOTE(jokke): This whole function shrinks to just couple of dozen"},{"line_number":461,"context_line":"        # lines after we get rid of the old config logic making this super"},{"line_number":462,"context_line":"        # hacky at the moment."},{"line_number":463,"context_line":"        if not staging:"},{"line_number":464,"context_line":"            hashing_algo \u003d self.image.os_hash_algo or CONF[\u0027hashing_algorithm\u0027]"},{"line_number":465,"context_line":"            if CONF.enabled_backends:"},{"line_number":466,"context_line":"                (location, size, checksum,"}],"source_content_type":"text/x-python","patch_set":4,"id":"987edc6d_9beb9796","line":463,"updated":"2021-02-23 22:39:48.000000000","message":"IMHO, if using the glance store driver for the staging area is really the right way to manage it (instead of just assuming it\u0027s a filesystem directory and foregoing all of the layers on top), then we shouldn\u0027t need low-level hacks like this. The existing special-casing we already have everywhere to \"treat it like a store, but just grab the path and diddle the filesystem directly anyway\" is what makes me skeptical that it\u0027s the right approach. Flags like this passed through all the layers really cement that concern.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":460,"context_line":"        # NOTE(jokke): This whole function shrinks to just couple of dozen"},{"line_number":461,"context_line":"        # lines after we get rid of the old config logic making this super"},{"line_number":462,"context_line":"        # hacky at the moment."},{"line_number":463,"context_line":"        if not staging:"},{"line_number":464,"context_line":"            hashing_algo \u003d self.image.os_hash_algo or CONF[\u0027hashing_algorithm\u0027]"},{"line_number":465,"context_line":"            if CONF.enabled_backends:"},{"line_number":466,"context_line":"                (location, size, checksum,"}],"source_content_type":"text/x-python","patch_set":4,"id":"0d8adf41_7b73a1db","line":463,"in_reply_to":"987edc6d_9beb9796","updated":"2021-03-01 16:10:11.000000000","message":"ditto","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":522,"context_line":"                    pass"},{"line_number":523,"context_line":"                conf.set_override(\u0027filesystem_store_datadir\u0027,"},{"line_number":524,"context_line":"                                  CONF.node_staging_uri[7:],"},{"line_number":525,"context_line":"                                  group\u003d\u0027glance_store\u0027)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"                staging_store \u003d backend._load_store(conf, \u0027file\u0027)"},{"line_number":528,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"95602df0_20903f3b","line":525,"updated":"2021-02-23 22:39:48.000000000","message":"This is enormously hacky and mutating config is definitely not something we should be doing at runtime, IMHO. Definitely not with every image stage.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":522,"context_line":"                    pass"},{"line_number":523,"context_line":"                conf.set_override(\u0027filesystem_store_datadir\u0027,"},{"line_number":524,"context_line":"                                  CONF.node_staging_uri[7:],"},{"line_number":525,"context_line":"                                  group\u003d\u0027glance_store\u0027)"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":"                staging_store \u003d backend._load_store(conf, \u0027file\u0027)"},{"line_number":528,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"712cef4c_f96c509f","line":525,"in_reply_to":"95602df0_20903f3b","updated":"2021-03-01 16:10:11.000000000","message":"Exactly and this is something we have duplicated all over the place currently and stays that way with your original proposal.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":551,"context_line":"                loc_meta \u003d {}"},{"line_number":552,"context_line":"            # TODO(jokke): Replace this placeholder with actual staging"},{"line_number":553,"context_line":"            # host once the config option is introduced."},{"line_number":554,"context_line":"            loc_meta[\u0027staging_host\u0027] \u003d \"placeholder\""},{"line_number":555,"context_line":""},{"line_number":556,"context_line":"        self.image.locations.append({\u0027url\u0027: location, \u0027metadata\u0027: loc_meta,"},{"line_number":557,"context_line":"                                     \u0027status\u0027: \u0027active\u0027})"}],"source_content_type":"text/x-python","patch_set":4,"id":"9b1bfe8d_bc370d9f","line":554,"updated":"2021-02-23 22:39:48.000000000","message":"Just as a preview for when this is actually a URL, as described in the comment, let me say:\n\nAs I said before, this is, IMHO, a layering violation. Having the low-level storage layer setting things that are the business of the API directly is a bad design and links those two things with special behavior. The API will need to look at this and compare it to its copy of the value to know what to do anyway. Putting it here just means the low-level location-storing code is subtly linked to the high-level API code.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":598,"context_line":"                 set_active\u003dTrue, staging\u003dFalse):"},{"line_number":599,"context_line":"        if size is None:"},{"line_number":600,"context_line":"            size \u003d 0  # NOTE(markwash): zero -\u003e unknown size"},{"line_number":601,"context_line":"        if staging:"},{"line_number":602,"context_line":"            set_active \u003d False  # NOTE(jokke): never activate while staging"},{"line_number":603,"context_line":""},{"line_number":604,"context_line":"        # Create the verifier for signature verification (if correct properties"}],"source_content_type":"text/x-python","patch_set":4,"id":"34416413_d88659a4","line":601,"updated":"2021-02-23 22:39:48.000000000","message":"Echoing my concern in the latest patch set about this layering violation and brings a bunch of change to \"the onion\" that is both unnecessary and not desirable.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":true,"context_lines":[{"line_number":598,"context_line":"                 set_active\u003dTrue, staging\u003dFalse):"},{"line_number":599,"context_line":"        if size is None:"},{"line_number":600,"context_line":"            size \u003d 0  # NOTE(markwash): zero -\u003e unknown size"},{"line_number":601,"context_line":"        if staging:"},{"line_number":602,"context_line":"            set_active \u003d False  # NOTE(jokke): never activate while staging"},{"line_number":603,"context_line":""},{"line_number":604,"context_line":"        # Create the verifier for signature verification (if correct properties"}],"source_content_type":"text/x-python","patch_set":4,"id":"790c771d_7cfe2ccd","line":601,"in_reply_to":"34416413_d88659a4","updated":"2021-03-01 16:10:11.000000000","message":"I just express my humble disagreement here. Having the locations metadata recorded where the location is created is way less violating than storing that information with the characteristics of the image and hoping that someone remembers to pick it up from there.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"}],"glance/tests/functional/v2/test_images.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"75406e45_a0e5a232","updated":"2021-02-22 23:25:41.000000000","message":"Oops, will revert for next PS. Left behind unintentionally.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"}],"glance/tests/unit/async_/flows/test_api_image_import.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":524,"context_line":"        task.execute(\u0027foo\u0027)"},{"line_number":525,"context_line":"        mock_unlink.assert_not_called()"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":""},{"line_number":528,"context_line":"class TestImportCopyImageTask(test_utils.BaseTestCase):"},{"line_number":529,"context_line":""},{"line_number":530,"context_line":"    def setUp(self):"}],"source_content_type":"text/x-python","patch_set":1,"id":"844bbe2d_28a96195","side":"PARENT","line":527,"updated":"2021-02-23 22:39:48.000000000","message":"You\u0027ve removed these tests but haven\u0027t replaced them with equivalents for the new behavior.","commit_id":"ac53e433e23419defc745bb42b2419daca87adc5"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":523,"context_line":"        task \u003d import_flow._DeleteFromFS(TASK_ID1, TASK_TYPE)"},{"line_number":524,"context_line":"        task.execute(\u0027foo\u0027)"},{"line_number":525,"context_line":"        mock_unlink.assert_not_called()"},{"line_number":526,"context_line":""},{"line_number":527,"context_line":""},{"line_number":528,"context_line":"class TestImportCopyImageTask(test_utils.BaseTestCase):"},{"line_number":529,"context_line":""}],"source_content_type":"text/x-python","patch_set":4,"id":"7ada1903_7813de68","side":"PARENT","line":526,"updated":"2021-02-23 22:39:48.000000000","message":"These tests are still valid, IMHO, even though they probably need to change. The scenarios can still occur and we should make sure that none of them trip up the current code (and that the backend delete is actually happening).","commit_id":"ac53e433e23419defc745bb42b2419daca87adc5"}],"glance/tests/unit/async_/flows/test_copy_image.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"21787713_1be21ec9","updated":"2021-02-22 23:25:41.000000000","message":"IMO these tests are not useful and should be removed as the logic is not there anymore. This was easier to do the delete after feedback than bringing them back.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":true,"context_lines":[{"line_number":178,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":179,"context_line":"    def test_copy_image_to_staging_store_data_exists("},{"line_number":180,"context_line":"            self, mock_store_api, mock_exists, mock_getsize, mock_unlink):"},{"line_number":181,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":182,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":183,"context_line":"        mock_exists.return_value \u003d True"},{"line_number":184,"context_line":"        mock_getsize.return_value \u003d 4"}],"source_content_type":"text/x-python","patch_set":1,"id":"4717d2e6_7ecf4999","line":181,"updated":"2021-02-22 23:25:41.000000000","message":"This might be still valid test case, but needs refactoring if we want to keep it.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"39ede897fa0f2a2f6f780ceb1370043f2c97d2b1","unresolved":true,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":125,"context_line":"    def test_copy_image_to_staging_store(self, mock_store_api):"},{"line_number":126,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":127,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":128,"context_line":"        copy_image_task \u003d copy_image._CopyImage("},{"line_number":129,"context_line":"            self.task.task_id, self.task_type, self.image_repo,"}],"source_content_type":"text/x-python","patch_set":3,"id":"8c6a6f4a_261733e3","line":126,"range":{"start_line":126,"start_character":8,"end_line":126,"end_character":36},"updated":"2021-02-23 14:14:21.000000000","message":"No we need this test to ensure we have data in staging store","commit_id":"ae34cb91795a9dc92c98c3871d30f13383809208"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":125,"context_line":"    def test_copy_image_to_staging_store(self, mock_store_api):"},{"line_number":126,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":127,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":128,"context_line":"        copy_image_task \u003d copy_image._CopyImage("},{"line_number":129,"context_line":"            self.task.task_id, self.task_type, self.image_repo,"}],"source_content_type":"text/x-python","patch_set":3,"id":"83d54f8c_fb56a38e","line":126,"range":{"start_line":126,"start_character":8,"end_line":126,"end_character":36},"in_reply_to":"8c6a6f4a_261733e3","updated":"2021-02-23 22:39:48.000000000","message":"Agree here and below, of course.","commit_id":"ae34cb91795a9dc92c98c3871d30f13383809208"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"39ede897fa0f2a2f6f780ceb1370043f2c97d2b1","unresolved":true,"context_lines":[{"line_number":147,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":148,"context_line":"    def test_copy_image_to_staging_store_partial_data_exists("},{"line_number":149,"context_line":"            self, mock_store_api, mock_exists, mock_getsize, mock_unlink):"},{"line_number":150,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":151,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":152,"context_line":"        mock_exists.return_value \u003d True"},{"line_number":153,"context_line":"        mock_getsize.return_value \u003d 3"}],"source_content_type":"text/x-python","patch_set":3,"id":"4b63a19d_8ebde07f","line":150,"range":{"start_line":150,"start_character":8,"end_line":150,"end_character":36},"updated":"2021-02-23 14:14:21.000000000","message":"This scenario also could be possible;\nfor example downloading existing image in to staging location and something went wrong before entire image is downloaded here.","commit_id":"ae34cb91795a9dc92c98c3871d30f13383809208"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"39ede897fa0f2a2f6f780ceb1370043f2c97d2b1","unresolved":true,"context_lines":[{"line_number":178,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":179,"context_line":"    def test_copy_image_to_staging_store_data_exists("},{"line_number":180,"context_line":"            self, mock_store_api, mock_exists, mock_getsize, mock_unlink):"},{"line_number":181,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":182,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":183,"context_line":"        mock_exists.return_value \u003d True"},{"line_number":184,"context_line":"        mock_getsize.return_value \u003d 4"}],"source_content_type":"text/x-python","patch_set":3,"id":"a1c2abc7_e03cb4c8","line":181,"range":{"start_line":181,"start_character":8,"end_line":181,"end_character":36},"updated":"2021-02-23 14:14:21.000000000","message":"This can also be a possibility","commit_id":"ae34cb91795a9dc92c98c3871d30f13383809208"},{"author":{"_account_id":9303,"name":"Abhishek Kekane","email":"akekane@redhat.com","username":"abhishekkekane"},"change_message_id":"39ede897fa0f2a2f6f780ceb1370043f2c97d2b1","unresolved":true,"context_lines":[{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":208,"context_line":"    def test_copy_non_existing_image_to_staging_store_(self, mock_store_api):"},{"line_number":209,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":210,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":211,"context_line":"        copy_image_task \u003d copy_image._CopyImage("},{"line_number":212,"context_line":"            self.task.task_id, self.task_type, self.image_repo,"}],"source_content_type":"text/x-python","patch_set":3,"id":"2e071294_f4e31938","line":209,"range":{"start_line":209,"start_character":8,"end_line":209,"end_character":36},"updated":"2021-02-23 14:14:21.000000000","message":"this is needed as well","commit_id":"ae34cb91795a9dc92c98c3871d30f13383809208"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6b941fb86331ff141693eaf34e79f07464859635","unresolved":true,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":125,"context_line":"    def test_copy_image_to_staging_store(self, mock_store_api):"},{"line_number":126,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":127,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":128,"context_line":"        copy_image_task \u003d copy_image._CopyImage("},{"line_number":129,"context_line":"            self.task.task_id, self.task_type, self.image_repo,"}],"source_content_type":"text/x-python","patch_set":4,"id":"0c249e7a_120fbe44","line":126,"updated":"2021-02-23 22:39:48.000000000","message":"Still agree that these are *not* irrelevant and need to be fixed.","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"},{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"849a4ef7c92eeda285620ab64c325d7e4e90be36","unresolved":false,"context_lines":[{"line_number":123,"context_line":""},{"line_number":124,"context_line":"    @mock.patch.object(store_api, \u0027get_store_from_store_identifier\u0027)"},{"line_number":125,"context_line":"    def test_copy_image_to_staging_store(self, mock_store_api):"},{"line_number":126,"context_line":"        self.skipTest(\"Irrelevant?\")"},{"line_number":127,"context_line":"        mock_store_api.return_value \u003d self.staging_store"},{"line_number":128,"context_line":"        copy_image_task \u003d copy_image._CopyImage("},{"line_number":129,"context_line":"            self.task.task_id, self.task_type, self.image_repo,"}],"source_content_type":"text/x-python","patch_set":4,"id":"76b77188_e6c799bc","line":126,"in_reply_to":"0c249e7a_120fbe44","updated":"2021-03-01 16:10:11.000000000","message":"Done","commit_id":"a1ddbf303c1c4195739a90d014058d4ea7c87967"}],"glance/tests/unit/async_/flows/test_web_download.py":[{"author":{"_account_id":5202,"name":"Erno Kuvaja","email":"jokke@usr.fi","username":"jokke"},"change_message_id":"1bb1e6ab69495fb76c30d3213563829a1b1a3f66","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"9fe3da61_148cbbb9","updated":"2021-02-22 23:25:41.000000000","message":"These tests are my biggest headache. Some flagged as \"Irrelevant?\" I think we likely should clean up. The ones labelled \"BROKEN\" are ones that likely has still valid test case but due to their structure I was not able to figure out how to refactor them. Specially as the functional tests passes, it feels like these failures are mocking issues rather than functional failures.\n\nAny help would be greatly appreciated.","commit_id":"4548a5a39227b24a4ed59d318693428e735f0592"}]}