)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":16,"context_line":"On failover, each replicated volume is promoted to primary on the"},{"line_number":17,"context_line":"secondary cluster and new connection requests will receive connection"},{"line_number":18,"context_line":"information for the volume on the secondary cluster.  At the time of"},{"line_number":19,"context_line":"writing, failback is not supported and requires admin intervention to"},{"line_number":20,"context_line":"reach a per-failover state."},{"line_number":21,"context_line":""},{"line_number":22,"context_line":"There are two configuration pieces required to make this work:"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"fa7ab95a_e6f93d4f","line":19,"range":{"start_line":19,"start_character":21,"end_line":19,"end_character":34},"updated":"2016-08-26 10:46:20.000000000","message":"nit: not supported in Cinder","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":33,"context_line":"    ..."},{"line_number":34,"context_line":"    replication_device \u003d backend_id:secondary,"},{"line_number":35,"context_line":"                         conf:/etc/ceph/secondary.conf,"},{"line_number":36,"context_line":"                         user:cinder"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"DocImpact"},{"line_number":39,"context_line":"Implements: blueprint rbd-replication"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"fa7ab95a_e6a77d4d","line":36,"updated":"2016-08-26 10:46:20.000000000","message":"nit: I think you should mention that these 3 lines should be only 1 line in the config file","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bc614a69ad850617000b63e05bdf3537bf36c167","unresolved":false,"context_lines":[{"line_number":33,"context_line":"    ..."},{"line_number":34,"context_line":"    replication_device \u003d backend_id:secondary,"},{"line_number":35,"context_line":"                         conf:/etc/ceph/secondary.conf,"},{"line_number":36,"context_line":"                         user:cinder"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"DocImpact"},{"line_number":39,"context_line":"Implements: blueprint rbd-replication"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"fa7ab95a_09ca3f4e","line":36,"in_reply_to":"fa7ab95a_c97350b0","updated":"2016-08-29 14:48:00.000000000","message":"I did it with multiple lines the first time and it didn\u0027t work, so I had to put it all in one line and then it worked.  Maybe I did something else wrong...  Will check again to be sure.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"626012f137e5754c18eec66a012106afb0b736bf","unresolved":false,"context_lines":[{"line_number":33,"context_line":"    ..."},{"line_number":34,"context_line":"    replication_device \u003d backend_id:secondary,"},{"line_number":35,"context_line":"                         conf:/etc/ceph/secondary.conf,"},{"line_number":36,"context_line":"                         user:cinder"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"DocImpact"},{"line_number":39,"context_line":"Implements: blueprint rbd-replication"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":11,"id":"fa7ab95a_c97350b0","line":36,"in_reply_to":"fa7ab95a_e6a77d4d","updated":"2016-08-26 15:35:12.000000000","message":"One line or mutiple, both will be parsed correctly.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"}],"cinder/volume/drivers/rbd.py":[{"author":{"_account_id":20599,"name":"Jason Dillaman","email":"dillaman@redhat.com","username":"dillaman"},"change_message_id":"ee0306a92aa8b3ecd3794f9a4a1a4f997e2f9dd8","unresolved":false,"context_lines":[{"line_number":595,"context_line":""},{"line_number":596,"context_line":"    def _enable_replication(self, volume):"},{"line_number":597,"context_line":"        with RBDVolumeProxy(self, volume.name) as image:"},{"line_number":598,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True)"},{"line_number":599,"context_line":"            image.mirror_image_enable()"},{"line_number":600,"context_line":""},{"line_number":601,"context_line":"    def _is_volume_replicated_type(self, volume):"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_2f7968fd","line":598,"updated":"2016-08-08 22:31:30.000000000","message":"Minor: this might raise an exception if journaling is already enabled (e.g. the ceph config might override the default image features).","commit_id":"adb1546862a4b925447f7e1025fa81a80bc7605c"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"5b492d8825b9a1b8de89bc537b981216abc802f7","unresolved":false,"context_lines":[{"line_number":595,"context_line":""},{"line_number":596,"context_line":"    def _enable_replication(self, volume):"},{"line_number":597,"context_line":"        with RBDVolumeProxy(self, volume.name) as image:"},{"line_number":598,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True)"},{"line_number":599,"context_line":"            image.mirror_image_enable()"},{"line_number":600,"context_line":""},{"line_number":601,"context_line":"    def _is_volume_replicated_type(self, volume):"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_519528dc","line":598,"in_reply_to":"9ad45d7e_2f7968fd","updated":"2016-08-09 19:41:17.000000000","message":"My approach was to catch it in the caller so that I have the context necessary to undo any volume operations if needed.  But I did miss them in retype(), update coming soon, thanks for pointing this out.","commit_id":"adb1546862a4b925447f7e1025fa81a80bc7605c"},{"author":{"_account_id":20599,"name":"Jason Dillaman","email":"dillaman@redhat.com","username":"dillaman"},"change_message_id":"ee0306a92aa8b3ecd3794f9a4a1a4f997e2f9dd8","unresolved":false,"context_lines":[{"line_number":596,"context_line":"    def _enable_replication(self, volume):"},{"line_number":597,"context_line":"        with RBDVolumeProxy(self, volume.name) as image:"},{"line_number":598,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True)"},{"line_number":599,"context_line":"            image.mirror_image_enable()"},{"line_number":600,"context_line":""},{"line_number":601,"context_line":"    def _is_volume_replicated_type(self, volume):"},{"line_number":602,"context_line":"        ctxt \u003d context.get_admin_context()"}],"source_content_type":"text/x-python","patch_set":3,"id":"9ad45d7e_4f149cf4","line":599,"updated":"2016-08-08 22:31:30.000000000","message":"Minor: if mirroring is configured in \"pool\" mode (e.g. all images in the pool should be replicated if journaling is enabled), this might raise an exception. Same comment for below with mirror_image_disable.","commit_id":"adb1546862a4b925447f7e1025fa81a80bc7605c"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8d9c597d161ebc163727a7aad5dd2b50a745f9d0","unresolved":false,"context_lines":[{"line_number":601,"context_line":"    def _is_volume_replicated_type(self, volume):"},{"line_number":602,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":603,"context_line":"        replication_flag \u003d False"},{"line_number":604,"context_line":"        if volume[\"volume_type_id\"]:"},{"line_number":605,"context_line":"            volume_type \u003d volume_types.get_volume_type("},{"line_number":606,"context_line":"                ctxt, volume[\"volume_type_id\"])"},{"line_number":607,"context_line":"            specs \u003d volume_type.get(\"extra_specs\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_d0b5e9d0","line":604,"updated":"2016-08-15 05:36:29.000000000","message":"For new codes, use volume.volume_type_id instead.","commit_id":"aab82220464ea2cd2dd92362e7be0fc6db82bc55"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8d9c597d161ebc163727a7aad5dd2b50a745f9d0","unresolved":false,"context_lines":[{"line_number":602,"context_line":"        ctxt \u003d context.get_admin_context()"},{"line_number":603,"context_line":"        replication_flag \u003d False"},{"line_number":604,"context_line":"        if volume[\"volume_type_id\"]:"},{"line_number":605,"context_line":"            volume_type \u003d volume_types.get_volume_type("},{"line_number":606,"context_line":"                ctxt, volume[\"volume_type_id\"])"},{"line_number":607,"context_line":"            specs \u003d volume_type.get(\"extra_specs\")"},{"line_number":608,"context_line":"            if specs and EXTRA_SPECS_REPL_ENABLED in specs:"},{"line_number":609,"context_line":"                replication_capability \u003d specs[EXTRA_SPECS_REPL_ENABLED]"},{"line_number":610,"context_line":"                replication_flag \u003d (replication_capability \u003d\u003d \"\u003cis\u003e True\")"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_90db711b","line":607,"range":{"start_line":605,"start_character":0,"end_line":607,"end_character":50},"updated":"2016-08-15 05:36:29.000000000","message":"volume is an object here, you may just following:\n\nspecs \u003d volume.volume_type.extra_specs","commit_id":"aab82220464ea2cd2dd92362e7be0fc6db82bc55"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8d9c597d161ebc163727a7aad5dd2b50a745f9d0","unresolved":false,"context_lines":[{"line_number":900,"context_line":""},{"line_number":901,"context_line":"        new_vol_replicated \u003d False"},{"line_number":902,"context_line":"        if new_type:"},{"line_number":903,"context_line":"            specs \u003d new_type.get(\"extra_specs\")"},{"line_number":904,"context_line":"            if specs and EXTRA_SPECS_REPL_ENABLED in specs:"},{"line_number":905,"context_line":"                replication_capability \u003d specs[EXTRA_SPECS_REPL_ENABLED]"},{"line_number":906,"context_line":"                # Do not validate settings, ignore invalid."},{"line_number":907,"context_line":"                new_vol_replicated \u003d (replication_capability \u003d\u003d \"\u003cis\u003e True\")"},{"line_number":908,"context_line":""},{"line_number":909,"context_line":"        if previous_vol_replicated and not new_vol_replicated:"},{"line_number":910,"context_line":"            try:"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_f9d29133","line":907,"range":{"start_line":903,"start_character":0,"end_line":907,"end_character":76},"updated":"2016-08-15 05:36:29.000000000","message":"Can we make it a function to decrease the code duplication with Line 607 - 610?","commit_id":"aab82220464ea2cd2dd92362e7be0fc6db82bc55"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8d9c597d161ebc163727a7aad5dd2b50a745f9d0","unresolved":false,"context_lines":[{"line_number":921,"context_line":"                raise exception.ReplicationError(reason\u003derr_msg,"},{"line_number":922,"context_line":"                                                 volume_id\u003dvolume.id)"},{"line_number":923,"context_line":""},{"line_number":924,"context_line":"        return True, None"},{"line_number":925,"context_line":""},{"line_number":926,"context_line":"    def _failover_volume(self, volume, remote, updates):"},{"line_number":927,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_192b2d32","line":924,"updated":"2016-08-15 05:36:29.000000000","message":"Return True is enough.","commit_id":"aab82220464ea2cd2dd92362e7be0fc6db82bc55"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8d9c597d161ebc163727a7aad5dd2b50a745f9d0","unresolved":false,"context_lines":[{"line_number":974,"context_line":"                    reason\u003d_(\u0027RBD: Failover host not found: %s.\u0027) %"},{"line_number":975,"context_line":"                    secondary_id)"},{"line_number":976,"context_line":"        else:"},{"line_number":977,"context_line":"            remote \u003d self._replication_targets[0]"},{"line_number":978,"context_line":"            if not remote:"},{"line_number":979,"context_line":"                raise exception.UnableToFailOver("},{"line_number":980,"context_line":"                    reason\u003d_(\u0027RBD: Failover host does not exist.\u0027))"},{"line_number":981,"context_line":""},{"line_number":982,"context_line":"        for volume in volumes:"},{"line_number":983,"context_line":"            volume_updates \u003d self._failover_volume(volume, remote,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_50b0d98b","line":980,"range":{"start_line":977,"start_character":0,"end_line":980,"end_character":67},"updated":"2016-08-15 05:36:29.000000000","message":"May I have the understanding that _replication_targets contains at least one target if _is_replication_enabled is true? \n\nIf it is true, no need to check whether remote is None in else.","commit_id":"aab82220464ea2cd2dd92362e7be0fc6db82bc55"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"8d9c597d161ebc163727a7aad5dd2b50a745f9d0","unresolved":false,"context_lines":[{"line_number":980,"context_line":"                    reason\u003d_(\u0027RBD: Failover host does not exist.\u0027))"},{"line_number":981,"context_line":""},{"line_number":982,"context_line":"        for volume in volumes:"},{"line_number":983,"context_line":"            volume_updates \u003d self._failover_volume(volume, remote,"},{"line_number":984,"context_line":"                                                   volume_updates)"},{"line_number":985,"context_line":""},{"line_number":986,"context_line":"        return remote[\u0027backend_id\u0027], volume_updates"}],"source_content_type":"text/x-python","patch_set":5,"id":"3ac371cc_d9d4f548","line":983,"updated":"2016-08-15 05:36:29.000000000","message":"At first, no need to set volume_updates again. As it is updated in _failover_volume.\n\nSecondly, why don\u0027t return {\u0027volume_id\u0027: volume.id,\n                                    \u0027updates\u0027: {\n                                        \u0027replication_status\u0027: \u0027failed-over\u0027}} in _failover_volume? And add it into volume_updates in this function?\n\nYour method seems a little unusual.","commit_id":"aab82220464ea2cd2dd92362e7be0fc6db82bc55"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":295,"context_line":"        self._active_config \u003d {}"},{"line_number":296,"context_line":"        self._replication_targets \u003d []"},{"line_number":297,"context_line":"        self._is_replication_enabled \u003d False"},{"line_number":298,"context_line":"        self._active_backend_id \u003d kwargs.get(\u0027active_backend_id\u0027, None)"},{"line_number":299,"context_line":"        self._backend_name \u003d (self.configuration.volume_backend_name or"},{"line_number":300,"context_line":"                              self.__class__.__name__)"},{"line_number":301,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_c19343f9","line":298,"updated":"2016-08-26 10:46:20.000000000","message":"-1: You will always receive active_backend_id argument, so it should be added to the list of arguments being received so it\u0027s more visible what we are expecting to receive and what we are going to use.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":312,"context_line":"            for target in self._replication_targets:"},{"line_number":313,"context_line":"                if target[\u0027name\u0027] \u003d\u003d self._active_backend_id:"},{"line_number":314,"context_line":"                    failover_target \u003d target"},{"line_number":315,"context_line":"                    break"},{"line_number":316,"context_line":""},{"line_number":317,"context_line":"        self._set_active_config(failover_target)"},{"line_number":318,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_84f3060a","line":315,"updated":"2016-08-26 10:46:20.000000000","message":"-1: I don\u0027t think it is reasonable to use the primary if we couldn\u0027t find the active_backend_id that\u0027s defined in the DB.  We should raise an exception.\n\n for target in self._replication_targets:\n     if target[\u0027name\u0027] \u003d\u003d self._active_backend_id:\n         failover_target \u003d target\n         break\n else:\n     raise InvalidReplicationTarget(reason\u003d...)\n\nI don\u0027t know if that\u0027s the best exception to raise, but I think we certainly shouldn\u0027t continue with the primary.\n\nYou are already doing this on L997, and since the loop in there and this one are basically the same, shouldn\u0027t we make it a function?","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":327,"context_line":"        for replication_device in replication_devices:"},{"line_number":328,"context_line":"            try:"},{"line_number":329,"context_line":"                name \u003d replication_device[\u0027backend_id\u0027]"},{"line_number":330,"context_line":"                conf \u003d replication_device[\u0027conf\u0027]"},{"line_number":331,"context_line":"                user \u003d replication_device[\u0027user\u0027]"},{"line_number":332,"context_line":"            except KeyError as err:"},{"line_number":333,"context_line":"                raise exception.InvalidConfigurationValue("}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_01052bc1","line":330,"updated":"2016-08-26 10:46:20.000000000","message":"nit: I think we could have a sensible default that does the same as rbd-mirror does and goes to /etc/ceph/$backend_id.conf if no config file is specified:\n\n conf \u003d replication_device.get(\u0027conf\u0027,\n                               \u0027/etc/ceph/%s.conf\u0027 % name)","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":328,"context_line":"            try:"},{"line_number":329,"context_line":"                name \u003d replication_device[\u0027backend_id\u0027]"},{"line_number":330,"context_line":"                conf \u003d replication_device[\u0027conf\u0027]"},{"line_number":331,"context_line":"                user \u003d replication_device[\u0027user\u0027]"},{"line_number":332,"context_line":"            except KeyError as err:"},{"line_number":333,"context_line":"                raise exception.InvalidConfigurationValue("},{"line_number":334,"context_line":"                    option\u003d\u0027replication_device\u0027, value\u003derr)"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_e11767eb","line":331,"updated":"2016-08-26 10:46:20.000000000","message":"nit: A sensible default for when it\u0027s missing would be to use the same user as we do for the primary:\n\n user \u003d replication_devices.get(\u0027user\u0027,\n                                self.configuration.rbd_user)","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":330,"context_line":"                conf \u003d replication_device[\u0027conf\u0027]"},{"line_number":331,"context_line":"                user \u003d replication_device[\u0027user\u0027]"},{"line_number":332,"context_line":"            except KeyError as err:"},{"line_number":333,"context_line":"                raise exception.InvalidConfigurationValue("},{"line_number":334,"context_line":"                    option\u003d\u0027replication_device\u0027, value\u003derr)"},{"line_number":335,"context_line":""},{"line_number":336,"context_line":"            replication_target \u003d {\u0027name\u0027: name,"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_81413b3c","line":333,"updated":"2016-08-26 10:46:20.000000000","message":"-1: Resulting message is misleading:\n\n Value \"conf\" is not valid for configuration option replication_device.\n\nIt should reflect that the issue is that it is actually missing.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":337,"context_line":"                                  \u0027conf\u0027: utils.convert_str(conf),"},{"line_number":338,"context_line":"                                  \u0027user\u0027: utils.convert_str(user)}"},{"line_number":339,"context_line":"            LOG.info(_LI(\u0027Adding replication target: %(backend)s.\u0027),"},{"line_number":340,"context_line":"                     {\u0027backend\u0027: replication_device[\u0027backend_id\u0027]})"},{"line_number":341,"context_line":"            self._replication_targets.append(replication_target)"},{"line_number":342,"context_line":""},{"line_number":343,"context_line":"    def _set_active_config(self, target\u003dNone):"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_fc2ab0e6","line":340,"range":{"start_line":340,"start_character":33,"end_line":340,"end_character":65},"updated":"2016-08-26 10:46:20.000000000","message":"nit: Or you could just use name variable","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":353,"context_line":"                \u0027user\u0027: self.configuration.rbd_user}"},{"line_number":354,"context_line":""},{"line_number":355,"context_line":"    def _get_active_config(self):"},{"line_number":356,"context_line":"        return (self._active_config.get(\u0027name\u0027, None),"},{"line_number":357,"context_line":"                self._active_config.get(\u0027conf\u0027, None),"},{"line_number":358,"context_line":"                self._active_config.get(\u0027user\u0027, None))"},{"line_number":359,"context_line":""},{"line_number":360,"context_line":"    def check_for_setup_error(self):"},{"line_number":361,"context_line":"        \"\"\"Returns an error if prerequisites aren\u0027t met.\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_3c5a5884","line":358,"range":{"start_line":356,"start_character":0,"end_line":358,"end_character":54},"updated":"2016-08-26 10:46:20.000000000","message":"nit: Default valur for get is already None, no need to define it.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":602,"context_line":"                raise"},{"line_number":603,"context_line":""},{"line_number":604,"context_line":"            try:"},{"line_number":605,"context_line":"                self._enable_replication_if_needed(volume)"},{"line_number":606,"context_line":"            except Exception:"},{"line_number":607,"context_line":"                self.RBDProxy().remove(client.ioctx, volume.name)"},{"line_number":608,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_414d7071","line":605,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This method should be returning a volume update dictionary.  If it\u0027s replicated it should return {\u0027replication_status\u0027: True} and if it\u0027s not it should return None.\n\nAnd then that\u0027s what we should be returning in this create method","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":605,"context_line":"                self._enable_replication_if_needed(volume)"},{"line_number":606,"context_line":"            except Exception:"},{"line_number":607,"context_line":"                self.RBDProxy().remove(client.ioctx, volume.name)"},{"line_number":608,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"},{"line_number":609,"context_line":"                raise exception.ReplicationError(reason\u003derr_msg,"},{"line_number":610,"context_line":"                                                 volume_id\u003dvolume.id)"},{"line_number":611,"context_line":"            finally:"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_de516164","line":608,"updated":"2016-08-26 10:46:20.000000000","message":"nit: No need for the extra parenthesis","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":12484,"name":"jazeltq","email":"tianqing@unitedstack.com","username":"litianqing"},"change_message_id":"96d5ff07e62c986fc8d016b10a9d2eb51a0964c4","unresolved":false,"context_lines":[{"line_number":620,"context_line":""},{"line_number":621,"context_line":"        LOG.debug(\"clone created successfully\")"},{"line_number":622,"context_line":""},{"line_number":623,"context_line":"    def _enable_replication(self, volume):"},{"line_number":624,"context_line":"        with RBDVolumeProxy(self, volume.name) as image:"},{"line_number":625,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True)"},{"line_number":626,"context_line":"            image.mirror_image_enable()"}],"source_content_type":"text/x-python","patch_set":11,"id":"9a89bdaa_577707d4","line":623,"range":{"start_line":623,"start_character":8,"end_line":623,"end_character":27},"updated":"2016-09-03 11:23:26.000000000","message":"I think besides using volume type, we should add  api for users to dynamic to enable replication,","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"81108ced63392a62e3eb67ca5b5a98acea33c4c8","unresolved":false,"context_lines":[{"line_number":620,"context_line":""},{"line_number":621,"context_line":"        LOG.debug(\"clone created successfully\")"},{"line_number":622,"context_line":""},{"line_number":623,"context_line":"    def _enable_replication(self, volume):"},{"line_number":624,"context_line":"        with RBDVolumeProxy(self, volume.name) as image:"},{"line_number":625,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True)"},{"line_number":626,"context_line":"            image.mirror_image_enable()"}],"source_content_type":"text/x-python","patch_set":11,"id":"9a89bdaa_8be32305","line":623,"range":{"start_line":623,"start_character":8,"end_line":623,"end_character":27},"in_reply_to":"9a89bdaa_577707d4","updated":"2016-09-09 07:37:26.000000000","message":"To dynamically enable replication on a volume you would use retype, that\u0027s how Replication v2.1 was intended to work.\n\nIf the volume is already in a Ceph cluster (with replication enabled) the replication will get enabled in the volume and it will not get migrated.\n\nIf the volume is in another backend the volume will need to be migrated.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":625,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, True)"},{"line_number":626,"context_line":"            image.mirror_image_enable()"},{"line_number":627,"context_line":""},{"line_number":628,"context_line":"    def _is_replicated_type(self, volume_type):"},{"line_number":629,"context_line":"        if not volume_type:"},{"line_number":630,"context_line":"            return False"},{"line_number":631,"context_line":"        replication_flag \u003d False"},{"line_number":632,"context_line":"        specs \u003d volume_type.extra_specs"},{"line_number":633,"context_line":"        if specs and EXTRA_SPECS_REPL_ENABLED in specs:"},{"line_number":634,"context_line":"            replication_capability \u003d specs[EXTRA_SPECS_REPL_ENABLED]"},{"line_number":635,"context_line":"            replication_flag \u003d (replication_capability \u003d\u003d \"\u003cis\u003e True\")"},{"line_number":636,"context_line":"        return replication_flag"},{"line_number":637,"context_line":""},{"line_number":638,"context_line":"    def _is_volume_replicated_type(self, volume):"},{"line_number":639,"context_line":"        if not volume.volume_type_id:"},{"line_number":640,"context_line":"            return False"},{"line_number":641,"context_line":"        return self._is_replicated_type(volume.volume_type)"},{"line_number":642,"context_line":""},{"line_number":643,"context_line":"    def _enable_replication_if_needed(self, volume):"},{"line_number":644,"context_line":"        if self._is_volume_replicated_type(volume):"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_1ed90922","line":641,"range":{"start_line":628,"start_character":0,"end_line":641,"end_character":59},"updated":"2016-08-26 10:46:20.000000000","message":"Isn\u0027t this a little verbose?  I think this is easier to read:\n\n def _is_replicated_type(self, volume_type)\n     specs \u003d getattr(volume_type, \u0027extra_specs\u0027, {})\n     return specs.get(EXTRA_SPECS_REPL_ENABLED) \u003d\u003d \"\u003cis\u003e True\"\n\nAnd then we can call that method instead of _is_volume_replicated_type in L644:\n\n if self._is_replicated_type(volume.volume_type):\n    self._enable_replication(volume)","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":640,"context_line":"            return False"},{"line_number":641,"context_line":"        return self._is_replicated_type(volume.volume_type)"},{"line_number":642,"context_line":""},{"line_number":643,"context_line":"    def _enable_replication_if_needed(self, volume):"},{"line_number":644,"context_line":"        if self._is_volume_replicated_type(volume):"},{"line_number":645,"context_line":"            self._enable_replication(volume)"},{"line_number":646,"context_line":""}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_c141803b","line":643,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This method should be returning a volume update dictionary.  If it\u0027s replicated it should return {\u0027replication_status\u0027: True} and if it\u0027s not it should return None.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":662,"context_line":"                                   features\u003dclient.features)"},{"line_number":663,"context_line":""},{"line_number":664,"context_line":"            try:"},{"line_number":665,"context_line":"                self._enable_replication_if_needed(volume)"},{"line_number":666,"context_line":"            except Exception:"},{"line_number":667,"context_line":"                self.RBDProxy().remove(client.ioctx, volume.name)"},{"line_number":668,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_e13e84b7","line":665,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This method should be returning a volume update dictionary.  If it\u0027s replicated it should return {\u0027replication_status\u0027: True} and if it\u0027s not it should return None.\n\nAnd then that\u0027s what we should be returning in this create method","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":694,"context_line":"                                      order\u003dorder)"},{"line_number":695,"context_line":""},{"line_number":696,"context_line":"            try:"},{"line_number":697,"context_line":"                self._enable_replication_if_needed(volume)"},{"line_number":698,"context_line":"            except Exception:"},{"line_number":699,"context_line":"                self.RBDProxy().remove(dest_client.ioctx, volume.name)"},{"line_number":700,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_016368dd","line":697,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This method should be returning a volume update dictionary.  If it\u0027s replicated it should return {\u0027replication_status\u0027: True} and if it\u0027s not it should return None.\n\nAnd then that\u0027s what we should be returning in this _clone method","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":711,"context_line":""},{"line_number":712,"context_line":"    def create_volume_from_snapshot(self, volume, snapshot):"},{"line_number":713,"context_line":"        \"\"\"Creates a volume from a snapshot.\"\"\""},{"line_number":714,"context_line":"        self._clone(volume, self.configuration.rbd_pool,"},{"line_number":715,"context_line":"                    snapshot.volume_name, snapshot.name)"},{"line_number":716,"context_line":"        if self.configuration.rbd_flatten_volume_from_snapshot:"},{"line_number":717,"context_line":"            self._flatten(self.configuration.rbd_pool, volume.name)"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_e1c12482","line":714,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This method should be returning a volume update dictionary.  If it\u0027s replicated it should return {\u0027replication_status\u0027: True} and if it\u0027s not it should return None.\n\nAnd then that\u0027s what we should be returning in this create method","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":922,"context_line":"        \"\"\"Disable replication on the given volume.\"\"\""},{"line_number":923,"context_line":"        with RBDVolumeProxy(self, volume.name) as image:"},{"line_number":924,"context_line":"            image.mirror_image_disable(False)"},{"line_number":925,"context_line":"            image.update_features(self.rbd.RBD_FEATURE_JOURNALING, False)"},{"line_number":926,"context_line":""},{"line_number":927,"context_line":"    def retype(self, context, volume, new_type, diff, host):"},{"line_number":928,"context_line":"        \"\"\"Retype from one volume type to another on the same backend.\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_a45d6aeb","line":925,"updated":"2016-08-26 10:46:20.000000000","message":"?: Couldn\u0027t the Ceph cluster have journaling enabled by default and here we would be undoing it?","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":926,"context_line":""},{"line_number":927,"context_line":"    def retype(self, context, volume, new_type, diff, host):"},{"line_number":928,"context_line":"        \"\"\"Retype from one volume type to another on the same backend.\"\"\""},{"line_number":929,"context_line":"        old_vol_replicated \u003d self._is_volume_replicated_type(volume)"},{"line_number":930,"context_line":"        new_vol_replicated \u003d self._is_replicated_type(new_type)"},{"line_number":931,"context_line":""},{"line_number":932,"context_line":"        if old_vol_replicated and not new_vol_replicated:"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_84e786e3","line":929,"updated":"2016-08-26 10:46:20.000000000","message":"If we change _is_replicated_type as I suggest above we would be caling `self._is_replicated_type(volume.volume_type)` instead","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":946,"context_line":""},{"line_number":947,"context_line":"        return True"},{"line_number":948,"context_line":""},{"line_number":949,"context_line":"    def _failover_volume(self, volume, remote, updates):"},{"line_number":950,"context_line":""},{"line_number":951,"context_line":"        if volume.status in [\u0027error\u0027]:"},{"line_number":952,"context_line":"            LOG.error(_LE(\u0027Skipping failover for %(volume)s, \u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_e7da1478","line":949,"updated":"2016-08-26 10:46:20.000000000","message":"-1: I don\u0027t think this method should be worrying about the updates list.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":948,"context_line":""},{"line_number":949,"context_line":"    def _failover_volume(self, volume, remote, updates):"},{"line_number":950,"context_line":""},{"line_number":951,"context_line":"        if volume.status in [\u0027error\u0027]:"},{"line_number":952,"context_line":"            LOG.error(_LE(\u0027Skipping failover for %(volume)s, \u0027"},{"line_number":953,"context_line":"                          \u0027status: %(status)s\u0027) %"},{"line_number":954,"context_line":"                      {\u0027volume\u0027: volume.name, \u0027status\u0027: volume.status})"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_c7cef082","line":951,"updated":"2016-08-26 10:46:20.000000000","message":"-1: I don\u0027t think we should be skipping volumes just because they are in an error status.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":956,"context_line":""},{"line_number":957,"context_line":"        # NOTE(jbernard): If this backend is being failed over and this volume"},{"line_number":958,"context_line":"        # doesn\u0027t support replication, then perhaps its status should be set to"},{"line_number":959,"context_line":"        # \u0027error\u0027.  Leaving it as \u0027available\u0027 is certainly misleading."},{"line_number":960,"context_line":"        if volume.replication_status in [\u0027disabled\u0027, \u0027failed-over\u0027]:"},{"line_number":961,"context_line":"            LOG.error(_LE(\u0027Skipping failover for %(volume)s, \u0027"},{"line_number":962,"context_line":"                          \u0027replication status %(replication_status)s\u0027) %"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_6731a4a2","line":959,"updated":"2016-08-26 10:46:20.000000000","message":"I agree, it doesn\u0027t make much sense to leave them as available, and I think they should be changed to \u0027error\u0027 if they are not replicated and are in available status and we should explicitly give the reason in the fail_reason message.  But then when we implement the failback in Cinder we have to return them to available.  We could check the fail_reason to do it, but I think this requires at least that we discuss it and agree on it.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":957,"context_line":"        # NOTE(jbernard): If this backend is being failed over and this volume"},{"line_number":958,"context_line":"        # doesn\u0027t support replication, then perhaps its status should be set to"},{"line_number":959,"context_line":"        # \u0027error\u0027.  Leaving it as \u0027available\u0027 is certainly misleading."},{"line_number":960,"context_line":"        if volume.replication_status in [\u0027disabled\u0027, \u0027failed-over\u0027]:"},{"line_number":961,"context_line":"            LOG.error(_LE(\u0027Skipping failover for %(volume)s, \u0027"},{"line_number":962,"context_line":"                          \u0027replication status %(replication_status)s\u0027) %"},{"line_number":963,"context_line":"                      {\u0027volume\u0027: volume.name,"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_47bae0e2","line":960,"updated":"2016-08-26 10:46:20.000000000","message":"-1: You should be using cinder.objects.fields.ReplicationStatus attributes instead of the string values directly.\n\nAnd this won\u0027t work unless you return the \u0027replication_status\u0027 update when creating like I mention in other comments.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":972,"context_line":"                    target_volume.mirror_image_promote(True)"},{"line_number":973,"context_line":"                    updates.append({\u0027volume_id\u0027: volume.id,"},{"line_number":974,"context_line":"                                    \u0027updates\u0027: {"},{"line_number":975,"context_line":"                                        \u0027replication_status\u0027: \u0027failed-over\u0027}})"},{"line_number":976,"context_line":"        except Exception:"},{"line_number":977,"context_line":"            LOG.error(_LE(\u0027Failed to failover volume %(volume)s\u0027) %"},{"line_number":978,"context_line":"                      {\u0027volume\u0027: volume.name})"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_07f3789f","line":975,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This should also be using cinder.objects.fields.ReplicationStatus.FAILED_OVER attribute","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":978,"context_line":"                      {\u0027volume\u0027: volume.name})"},{"line_number":979,"context_line":"            updates.append({\u0027volume_id\u0027: volume.id,"},{"line_number":980,"context_line":"                            \u0027updates\u0027: {"},{"line_number":981,"context_line":"                                \u0027status\u0027: \u0027error\u0027, }})"},{"line_number":982,"context_line":"        return updates"},{"line_number":983,"context_line":""},{"line_number":984,"context_line":"    def failover_host(self, context, volumes, secondary_id\u003dNone):"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_e73cd431","line":981,"updated":"2016-08-26 10:46:20.000000000","message":"-1: cinder.objects.fields.ReplicationStatus.ERROR attribute","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":998,"context_line":"                if target[\u0027name\u0027] \u003d\u003d secondary_id:"},{"line_number":999,"context_line":"                    remote \u003d target"},{"line_number":1000,"context_line":"                    break"},{"line_number":1001,"context_line":"            if not remote:"},{"line_number":1002,"context_line":"                raise exception.InvalidReplicationTarget("},{"line_number":1003,"context_line":"                    reason\u003d_(\u0027RBD: Failover host not found: %s.\u0027) %"},{"line_number":1004,"context_line":"                    secondary_id)"}],"source_content_type":"text/x-python","patch_set":11,"id":"1ac06dbe_834e4cbf","line":1001,"range":{"start_line":1001,"start_character":12,"end_line":1001,"end_character":26},"updated":"2016-08-26 10:46:20.000000000","message":"nit: This would look better using `else` construction for the `for`:\n\n else:","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":1000,"context_line":"                    break"},{"line_number":1001,"context_line":"            if not remote:"},{"line_number":1002,"context_line":"                raise exception.InvalidReplicationTarget("},{"line_number":1003,"context_line":"                    reason\u003d_(\u0027RBD: Failover host not found: %s.\u0027) %"},{"line_number":1004,"context_line":"                    secondary_id)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"        for volume in volumes:"}],"source_content_type":"text/x-python","patch_set":11,"id":"1ac06dbe_861dba68","line":1003,"updated":"2016-08-26 10:46:20.000000000","message":"nit: I think this may be a confusing message, since the failover cinderclient command requires a host parameter and here we are not explicitly saying that it\u0027s the requested host to fail over to is the unknown one.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":1004,"context_line":"                    secondary_id)"},{"line_number":1005,"context_line":""},{"line_number":1006,"context_line":"        for volume in volumes:"},{"line_number":1007,"context_line":"            updates \u003d self._failover_volume(volume, remote, updates)"},{"line_number":1008,"context_line":""},{"line_number":1009,"context_line":"        self._set_active_config(remote)"},{"line_number":1010,"context_line":"        return remote[\u0027name\u0027], updates"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_877ce873","line":1007,"updated":"2016-08-26 10:46:20.000000000","message":"-1: I think _failover_volume worrying about a list of updates is wrong, it should just be returning its own updates if there are any.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":1009,"context_line":"        self._set_active_config(remote)"},{"line_number":1010,"context_line":"        return remote[\u0027name\u0027], updates"},{"line_number":1011,"context_line":""},{"line_number":1012,"context_line":"    def freeze_backend(self, context):"},{"line_number":1013,"context_line":"        \"\"\"Freeze backend notification.\"\"\""},{"line_number":1014,"context_line":"        pass"},{"line_number":1015,"context_line":""},{"line_number":1016,"context_line":"    def thaw_backend(self, context):"},{"line_number":1017,"context_line":"        \"\"\"Thaw backend notification.\"\"\""},{"line_number":1018,"context_line":"        pass"},{"line_number":1019,"context_line":""},{"line_number":1020,"context_line":"    def ensure_export(self, context, volume):"},{"line_number":1021,"context_line":"        \"\"\"Synchronously recreates an export for a logical volume.\"\"\""}],"source_content_type":"text/x-python","patch_set":11,"id":"1ac06dbe_a37b88f3","line":1018,"range":{"start_line":1012,"start_character":0,"end_line":1018,"end_character":12},"updated":"2016-08-26 10:46:20.000000000","message":"-1: There\u0027s no need to define these 2 methods, we can use base class methods since we aren\u0027t doing anything here.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":1121,"context_line":"                        url_location, image_meta):"},{"line_number":1122,"context_line":"                    _prefix, pool, image, snapshot \u003d \\"},{"line_number":1123,"context_line":"                        self._parse_location(url_location)"},{"line_number":1124,"context_line":"                    self._clone(volume, pool, image, snapshot)"},{"line_number":1125,"context_line":"                    self._resize(volume)"},{"line_number":1126,"context_line":"                    return {\u0027provider_location\u0027: None}, True"},{"line_number":1127,"context_line":"        return ({}, False)"}],"source_content_type":"text/x-python","patch_set":11,"id":"fa7ab95a_61943437","line":1124,"updated":"2016-08-26 10:46:20.000000000","message":"-1: This method should be returning a volume update dictionary.  If it\u0027s replicated it should return {\u0027replication_status\u0027: True} and if it\u0027s not it should return None.\n\nAnd then that\u0027s what we should be returning in this clone method instead of and empty dictionary like we are doing in L1127","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"af44cb1125374a9831d49c58248cd865c8e1c1d0","unresolved":false,"context_lines":[{"line_number":408,"context_line":"                 CONF.rados_connection_retries)"},{"line_number":409,"context_line":"    def _connect_to_rados(self, pool\u003dNone, remote\u003dNone, timeout\u003dNone):"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"        name, conf, user \u003d self._get_config(remote)"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"        if pool is not None:"},{"line_number":414,"context_line":"            pool \u003d utils.convert_str(pool)"}],"source_content_type":"text/x-python","patch_set":14,"id":"7a8ec9b2_b2493ad3","line":411,"updated":"2016-09-14 01:57:13.000000000","message":"I think we can create replicated and non-replicate volumes in a volume host with RBD driver.\n\nAfter failing over replicated volumes, self._active_config is the setting as remote cluster. \n\nBut when we do something on the non-replicated volumes, it gets self._active_config in _connect_to_rados, is it a problem?","commit_id":"41129eedc0b505f137dbaec2dea8bb23b8febf33"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"3369aef2902fc6234dcbcb931e72bab5a9ce44c5","unresolved":false,"context_lines":[{"line_number":408,"context_line":"                 CONF.rados_connection_retries)"},{"line_number":409,"context_line":"    def _connect_to_rados(self, pool\u003dNone, remote\u003dNone, timeout\u003dNone):"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"        name, conf, user \u003d self._get_config(remote)"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"        if pool is not None:"},{"line_number":414,"context_line":"            pool \u003d utils.convert_str(pool)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5a8bc5a2_cb700675","line":411,"in_reply_to":"7a8ec9b2_82cc7abc","updated":"2016-09-27 08:36:48.000000000","message":"You are right.","commit_id":"41129eedc0b505f137dbaec2dea8bb23b8febf33"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bd10e6c3022bd98aba878bc3a55078dcde217da0","unresolved":false,"context_lines":[{"line_number":408,"context_line":"                 CONF.rados_connection_retries)"},{"line_number":409,"context_line":"    def _connect_to_rados(self, pool\u003dNone, remote\u003dNone, timeout\u003dNone):"},{"line_number":410,"context_line":""},{"line_number":411,"context_line":"        name, conf, user \u003d self._get_config(remote)"},{"line_number":412,"context_line":""},{"line_number":413,"context_line":"        if pool is not None:"},{"line_number":414,"context_line":"            pool \u003d utils.convert_str(pool)"}],"source_content_type":"text/x-python","patch_set":14,"id":"7a8ec9b2_82cc7abc","line":411,"in_reply_to":"7a8ec9b2_b2493ad3","updated":"2016-09-22 19:45:16.000000000","message":"Yes, we can have replicated and non-replicated volumes, that\u0027s why on failover we set all non replicated volumes to error.\n\nIt\u0027s not a problem that we try to access those volumes on the failedover cluster as they will not be found as expected.","commit_id":"41129eedc0b505f137dbaec2dea8bb23b8febf33"},{"author":{"_account_id":15961,"name":"lisali","email":"xiaoyan.li@intel.com","username":"lisali"},"change_message_id":"bf07b9493ec4f3d181ed765d73894c8c80f0e500","unresolved":false,"context_lines":[{"line_number":688,"context_line":"            try:"},{"line_number":689,"context_line":"                volume_update \u003d self._enable_replication_if_needed(volume)"},{"line_number":690,"context_line":"            except Exception:"},{"line_number":691,"context_line":"                self.RBDProxy().remove(client.ioctx, volume.name)"},{"line_number":692,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"},{"line_number":693,"context_line":"                raise exception.ReplicationError(reason\u003derr_msg,"},{"line_number":694,"context_line":"                                                 volume_id\u003dvolume.id)"}],"source_content_type":"text/x-python","patch_set":14,"id":"7a8ec9b2_d203260e","line":691,"updated":"2016-09-14 02:02:34.000000000","message":"In this file, when using volume.name in rbd, most of time we use utils.convert_str(src_vref.name) to convert name, should we use it?","commit_id":"41129eedc0b505f137dbaec2dea8bb23b8febf33"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bd10e6c3022bd98aba878bc3a55078dcde217da0","unresolved":false,"context_lines":[{"line_number":688,"context_line":"            try:"},{"line_number":689,"context_line":"                volume_update \u003d self._enable_replication_if_needed(volume)"},{"line_number":690,"context_line":"            except Exception:"},{"line_number":691,"context_line":"                self.RBDProxy().remove(client.ioctx, volume.name)"},{"line_number":692,"context_line":"                err_msg \u003d (_(\u0027Failed to enable image replication\u0027))"},{"line_number":693,"context_line":"                raise exception.ReplicationError(reason\u003derr_msg,"},{"line_number":694,"context_line":"                                                 volume_id\u003dvolume.id)"}],"source_content_type":"text/x-python","patch_set":14,"id":"7a8ec9b2_5dc7b797","line":691,"in_reply_to":"7a8ec9b2_d203260e","updated":"2016-09-22 19:45:16.000000000","message":"Good catch!!  I totally forgot about the issues with Python 3.","commit_id":"41129eedc0b505f137dbaec2dea8bb23b8febf33"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"e4c29cccb04879eec98a39854831cd51570f17cc","unresolved":false,"context_lines":[{"line_number":1084,"context_line":"            raise exception.UnableToFailOver("},{"line_number":1085,"context_line":"                reason\u003d_(\u0027RBD: Replication is not enabled.\u0027))"},{"line_number":1086,"context_line":""},{"line_number":1087,"context_line":"        if secondary_id:"},{"line_number":1088,"context_line":"            remote \u003d self._get_failover_host(secondary_id)"},{"line_number":1089,"context_line":"        else:"},{"line_number":1090,"context_line":"            # grab the first one from the list"}],"source_content_type":"text/x-python","patch_set":18,"id":"da6895a0_96e7abe3","line":1087,"updated":"2016-10-23 15:22:34.000000000","message":"-1: We have to support secondary_id \u003d \u0027default\u0027 for the failback","commit_id":"7b9a2fb62648464ae4c656044fc047d277347703"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"c22b9aac17cb4e66a4f4a31ae6eaa8954841a318","unresolved":false,"context_lines":[{"line_number":232,"context_line":"            if \u0027backend_id\u0027 not in replication_device:"},{"line_number":233,"context_line":"                msg \u003d _(\u0027Missing backend_id in replication_device \u0027"},{"line_number":234,"context_line":"                        \u0027configuration.\u0027)"},{"line_number":235,"context_line":"                raise exception.InvalidConfigurationValue(msg)"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"            name \u003d replication_device[\u0027backend_id\u0027]"},{"line_number":238,"context_line":"            conf \u003d replication_device.get(\u0027conf\u0027,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a6eadb0_217245ae","line":235,"updated":"2016-12-19 03:20:39.000000000","message":"Please set value and option in exception.InvalidConfigurationValue.\n\nclass InvalidConfigurationValue(Invalid):\n    message \u003d _(\u0027Value \"%(value)s\" is not valid for \u0027\n                \u0027configuration option \"%(option)s\"\u0027)","commit_id":"a3ff494da444cfa22272e48f6a771c0b376bdfaa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f2004b4b4f0d2f67516d08f65f8f1df0e2b61ea9","unresolved":false,"context_lines":[{"line_number":232,"context_line":"            if \u0027backend_id\u0027 not in replication_device:"},{"line_number":233,"context_line":"                msg \u003d _(\u0027Missing backend_id in replication_device \u0027"},{"line_number":234,"context_line":"                        \u0027configuration.\u0027)"},{"line_number":235,"context_line":"                raise exception.InvalidConfigurationValue(msg)"},{"line_number":236,"context_line":""},{"line_number":237,"context_line":"            name \u003d replication_device[\u0027backend_id\u0027]"},{"line_number":238,"context_line":"            conf \u003d replication_device.get(\u0027conf\u0027,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a6eadb0_62399141","line":235,"in_reply_to":"1a6eadb0_217245ae","updated":"2016-12-19 10:26:36.000000000","message":"If I use that message format the user won\u0027t know what\u0027s wrong with the replication_device option, provided message here is more specific and clear for users.","commit_id":"a3ff494da444cfa22272e48f6a771c0b376bdfaa"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"c22b9aac17cb4e66a4f4a31ae6eaa8954841a318","unresolved":false,"context_lines":[{"line_number":301,"context_line":"        if timeout is None:"},{"line_number":302,"context_line":"            timeout \u003d self.configuration.rados_connect_timeout"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        LOG.debug(\"connecting to %s (timeout\u003d%s).\", name, timeout)"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        client \u003d self.rados.Rados(rados_id\u003duser,"},{"line_number":307,"context_line":"                                  clustername\u003dname,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a6eadb0_a1b57532","line":304,"updated":"2016-12-19 03:20:39.000000000","message":"Please use keyword parameters %(var)s.","commit_id":"a3ff494da444cfa22272e48f6a771c0b376bdfaa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f2004b4b4f0d2f67516d08f65f8f1df0e2b61ea9","unresolved":false,"context_lines":[{"line_number":301,"context_line":"        if timeout is None:"},{"line_number":302,"context_line":"            timeout \u003d self.configuration.rados_connect_timeout"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        LOG.debug(\"connecting to %s (timeout\u003d%s).\", name, timeout)"},{"line_number":305,"context_line":""},{"line_number":306,"context_line":"        client \u003d self.rados.Rados(rados_id\u003duser,"},{"line_number":307,"context_line":"                                  clustername\u003dname,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a6eadb0_a273a9e6","line":304,"in_reply_to":"1a6eadb0_a1b57532","updated":"2016-12-19 10:26:36.000000000","message":"I\u0027ll change it, but that\u0027s not really necessary here, as that\u0027s only required by messages that are being translated to take into account grammar rules.","commit_id":"a3ff494da444cfa22272e48f6a771c0b376bdfaa"},{"author":{"_account_id":6491,"name":"xing-yang","email":"xingyang105@gmail.com","username":"xing-yang"},"change_message_id":"c22b9aac17cb4e66a4f4a31ae6eaa8954841a318","unresolved":false,"context_lines":[{"line_number":974,"context_line":"                                         \u0027mirror_image_demote\u0027)"},{"line_number":975,"context_line":"                    demoted \u003d True"},{"line_number":976,"context_line":"                except Exception as e:"},{"line_number":977,"context_line":"                    LOG.debug(\u0027Failed to demote %s(volume)s with error: \u0027"},{"line_number":978,"context_line":"                              \u0027%(error)s.\u0027,"},{"line_number":979,"context_line":"                              {\u0027volume\u0027: volume.name, \u0027error\u0027: e})"},{"line_number":980,"context_line":"                    try_demoting \u003d not until_failure"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a6eadb0_0140e906","line":977,"updated":"2016-12-19 03:20:39.000000000","message":"There is an extra \"s\":  s/%s(volume)s/%(volume)s","commit_id":"a3ff494da444cfa22272e48f6a771c0b376bdfaa"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"f2004b4b4f0d2f67516d08f65f8f1df0e2b61ea9","unresolved":false,"context_lines":[{"line_number":974,"context_line":"                                         \u0027mirror_image_demote\u0027)"},{"line_number":975,"context_line":"                    demoted \u003d True"},{"line_number":976,"context_line":"                except Exception as e:"},{"line_number":977,"context_line":"                    LOG.debug(\u0027Failed to demote %s(volume)s with error: \u0027"},{"line_number":978,"context_line":"                              \u0027%(error)s.\u0027,"},{"line_number":979,"context_line":"                              {\u0027volume\u0027: volume.name, \u0027error\u0027: e})"},{"line_number":980,"context_line":"                    try_demoting \u003d not until_failure"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a6eadb0_fd99c0d1","line":977,"in_reply_to":"1a6eadb0_0140e906","updated":"2016-12-19 10:26:36.000000000","message":"Oooops, thanks for the catch  :-)","commit_id":"a3ff494da444cfa22272e48f6a771c0b376bdfaa"}],"releasenotes/notes/rbd-v2.1-replication-64a9d0bec5987faf.yaml":[{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c3e67fa2d4292d9e12025f520ff3502865528d98","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Added v2.1 replication support to RBD driver."}],"source_content_type":"text/x-yaml","patch_set":11,"id":"fa7ab95a_875a2882","line":3,"updated":"2016-08-26 10:46:20.000000000","message":"nit: You could add some additional information like you do in the commit message.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"bc614a69ad850617000b63e05bdf3537bf36c167","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Added v2.1 replication support to RBD driver."}],"source_content_type":"text/x-yaml","patch_set":11,"id":"fa7ab95a_c98697da","line":3,"in_reply_to":"fa7ab95a_29c91c79","updated":"2016-08-29 14:48:00.000000000","message":"Ok, I\u0027m usually more verbose with my release notes, but I think it makes sense to keep it sort since the documentation should be updated with the replication details.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"},{"author":{"_account_id":9236,"name":"Jon Bernard","email":"jobernar@redhat.com","username":"jbernard"},"change_message_id":"953e18b2ed09cf8fa7b19c40c972a93d0bee2af4","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Added v2.1 replication support to RBD driver."}],"source_content_type":"text/x-yaml","patch_set":11,"id":"fa7ab95a_29c91c79","line":3,"in_reply_to":"fa7ab95a_875a2882","updated":"2016-08-26 15:42:15.000000000","message":"I certainly can, but I looked at what others have done and one-line seems to be the standard - perhaps this is not the correct place for verbosity?  If not, I\u0027ll eloaborate here as well.","commit_id":"1e135e489088dd83c5818d766b3fa90b28a6757c"}]}