)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"16c42cec45821baea15e7cfb25211071f81beec3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"2fd715f2_0cad4ed5","updated":"2026-04-06 21:39:47.000000000","message":"No, this isn\u0027t safe to do. We have gone through the details of what the risks are with a different bug: https://bugs.launchpad.net/manila/+bug/1867030 and this \"Do Not Merge\" patch. \n\nServices can be offline for a number of reasons, including, and we shouldn\u0027t provide API behavior to target resources during service maintenance windows. We already support a \"manila-manage share delete\" if someone really needs a tool to do this.","commit_id":"eb711087ab5ef4cecd8cbd54477618945fbb6977"},{"author":{"_account_id":38940,"name":"Oluwasola Akintewe","display_name":"nathan_akin","email":"solaakintewe@gmail.com","username":"nathan_akin"},"change_message_id":"68fbcb0d508dcb3400d02a538d5a0bff5c3cdd7e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"e15473ca_538df276","in_reply_to":"2fd715f2_0cad4ed5","updated":"2026-04-06 21:43:37.000000000","message":"Hi Goutham,thank you for the context and the pointer to bug #1867030. I understand the concern,  bypassing the RPC layer during a service outage is risky and the existing manila manage share delete already covers this. I\u0027ll abandon this patch.","commit_id":"eb711087ab5ef4cecd8cbd54477618945fbb6977"}],"manila/share/api.py":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"d9b0401b363ea657f9e944aa4dcc72732cd846c0","unresolved":true,"context_lines":[{"line_number":1663,"context_line":"            )"},{"line_number":1664,"context_line":""},{"line_number":1665,"context_line":"        host \u003d share_utils.extract_host(share_instance[\u0027host\u0027])"},{"line_number":1666,"context_line":"        service_is_up \u003d True"},{"line_number":1667,"context_line":"        try:"},{"line_number":1668,"context_line":"            service \u003d self.db.service_get_by_host_and_topic("},{"line_number":1669,"context_line":"                context, host, \u0027manila-share\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"5c322e83_34d69ff4","line":1666,"updated":"2026-03-31 21:32:38.000000000","message":"Nit: the variable service_is_up is initialized to True and then\npotentially reassigned twice. Consider simplifying:\n    try:\n        service \u003d self.db.service_get_by_args(\n            context, host, \u0027manila-share\u0027)\n        host_is_up \u003d utils.service_is_up(service)\n    except exception.HostBinaryNotFound:\n        host_is_up \u003d False\nAlso, naming the variable \"service_is_up\" shadows the utils\nfunction of the same name, which could be confusing. Consider\n\"host_is_up\" or \"host_online\" instead.","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":38940,"name":"Oluwasola Akintewe","display_name":"nathan_akin","email":"solaakintewe@gmail.com","username":"nathan_akin"},"change_message_id":"1251bf2e22142c4c8a67f48c73e7d05197e3f120","unresolved":false,"context_lines":[{"line_number":1663,"context_line":"            )"},{"line_number":1664,"context_line":""},{"line_number":1665,"context_line":"        host \u003d share_utils.extract_host(share_instance[\u0027host\u0027])"},{"line_number":1666,"context_line":"        service_is_up \u003d True"},{"line_number":1667,"context_line":"        try:"},{"line_number":1668,"context_line":"            service \u003d self.db.service_get_by_host_and_topic("},{"line_number":1669,"context_line":"                context, host, \u0027manila-share\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"045c818a_c33814dc","line":1666,"in_reply_to":"5c322e83_34d69ff4","updated":"2026-03-31 23:34:06.000000000","message":"Done","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"d9b0401b363ea657f9e944aa4dcc72732cd846c0","unresolved":true,"context_lines":[{"line_number":1665,"context_line":"        host \u003d share_utils.extract_host(share_instance[\u0027host\u0027])"},{"line_number":1666,"context_line":"        service_is_up \u003d True"},{"line_number":1667,"context_line":"        try:"},{"line_number":1668,"context_line":"            service \u003d self.db.service_get_by_host_and_topic("},{"line_number":1669,"context_line":"                context, host, \u0027manila-share\u0027)"},{"line_number":1670,"context_line":"            service_is_up \u003d utils.service_is_up(service)"},{"line_number":1671,"context_line":"        except exception.NotFound:"}],"source_content_type":"text/x-python","patch_set":1,"id":"9616f0dd_68f23208","line":1668,"updated":"2026-03-31 21:32:38.000000000","message":"service_get_by_host_and_topic() filters by disabled\u003dFalse (see\nmanila/db/sqlalchemy/api.py line 653). This means that if an admin\nhas disabled the service (but it is still running), the lookup will\nraise NotFound and the share will be deleted directly from the\ndatabase, bypassing the driver entirely. This would leave orphaned\nresources on the storage backend.\nUse service_get_by_args(context, host, \u0027manila-share\u0027) instead,\nwhich is the same pattern used elsewhere in this file (see line\n2220). That function looks up by host and binary without filtering\non the disabled flag. You would then check service_is_up() on the\nresult as you already do.","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":38940,"name":"Oluwasola Akintewe","display_name":"nathan_akin","email":"solaakintewe@gmail.com","username":"nathan_akin"},"change_message_id":"1251bf2e22142c4c8a67f48c73e7d05197e3f120","unresolved":false,"context_lines":[{"line_number":1665,"context_line":"        host \u003d share_utils.extract_host(share_instance[\u0027host\u0027])"},{"line_number":1666,"context_line":"        service_is_up \u003d True"},{"line_number":1667,"context_line":"        try:"},{"line_number":1668,"context_line":"            service \u003d self.db.service_get_by_host_and_topic("},{"line_number":1669,"context_line":"                context, host, \u0027manila-share\u0027)"},{"line_number":1670,"context_line":"            service_is_up \u003d utils.service_is_up(service)"},{"line_number":1671,"context_line":"        except exception.NotFound:"}],"source_content_type":"text/x-python","patch_set":1,"id":"e9af96d7_153bbce3","line":1668,"in_reply_to":"9616f0dd_68f23208","updated":"2026-03-31 23:34:06.000000000","message":"Done","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"d9b0401b363ea657f9e944aa4dcc72732cd846c0","unresolved":true,"context_lines":[{"line_number":1671,"context_line":"        except exception.NotFound:"},{"line_number":1672,"context_line":"            service_is_up \u003d False"},{"line_number":1673,"context_line":""},{"line_number":1674,"context_line":"        if force and not service_is_up:"},{"line_number":1675,"context_line":"            LOG.warning(\"Share instance %(id)s host %(host)s is down. \""},{"line_number":1676,"context_line":"                        \"Deleting share instance from the database directly.\","},{"line_number":1677,"context_line":"                        {\u0027id\u0027: share_instance[\u0027id\u0027], \u0027host\u0027: host})"}],"source_content_type":"text/x-python","patch_set":1,"id":"00ec33db_11e65359","line":1674,"updated":"2026-03-31 21:32:38.000000000","message":"When deleting from the DB directly, access rules and export\nlocations are cleaned up by share_instance_delete (via cascade),\nwhich is good. However, the manager\u0027s delete_share_instance also\ncalls _check_delete_share_server to potentially clean up the\nshare server when its last share is removed. That logic is skipped\nhere. Consider whether the share server should also be cleaned up\n(or at least have its updated_at timestamp set, which does happen\non line 1692). This may be acceptable for a first pass since\nforce-delete with host-down is an exceptional path, but it is\nworth noting as a follow-up.","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":38940,"name":"Oluwasola Akintewe","display_name":"nathan_akin","email":"solaakintewe@gmail.com","username":"nathan_akin"},"change_message_id":"1251bf2e22142c4c8a67f48c73e7d05197e3f120","unresolved":false,"context_lines":[{"line_number":1671,"context_line":"        except exception.NotFound:"},{"line_number":1672,"context_line":"            service_is_up \u003d False"},{"line_number":1673,"context_line":""},{"line_number":1674,"context_line":"        if force and not service_is_up:"},{"line_number":1675,"context_line":"            LOG.warning(\"Share instance %(id)s host %(host)s is down. \""},{"line_number":1676,"context_line":"                        \"Deleting share instance from the database directly.\","},{"line_number":1677,"context_line":"                        {\u0027id\u0027: share_instance[\u0027id\u0027], \u0027host\u0027: host})"}],"source_content_type":"text/x-python","patch_set":1,"id":"b728693a_49db1d6c","line":1674,"in_reply_to":"00ec33db_11e65359","updated":"2026-03-31 23:34:06.000000000","message":"Done","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"}],"manila/tests/share/test_api.py":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"d9b0401b363ea657f9e944aa4dcc72732cd846c0","unresolved":true,"context_lines":[{"line_number":3297,"context_line":"            instance"},{"line_number":3298,"context_line":"        )"},{"line_number":3299,"context_line":""},{"line_number":3300,"context_line":"    def test_force_delete_share_instance_host_down(self):"},{"line_number":3301,"context_line":"        \"\"\"Force delete should delete from DB directly when host is down.\"\"\""},{"line_number":3302,"context_line":"        instance \u003d self._setup_delete_share_instance_mocks("},{"line_number":3303,"context_line":"            status\u003dconstants.STATUS_AVAILABLE,"}],"source_content_type":"text/x-python","patch_set":1,"id":"38de7b4d_83d647ab","line":3300,"updated":"2026-03-31 21:32:38.000000000","message":"Good tests for the force+host-down and force+service-not-found\npaths. Consider also adding a test that verifies the else branch:\nwhen force\u003dTrue but the service IS up, the RPC path should still\nbe used. This ensures we don\u0027t accidentally bypass the driver\nwhen the host is healthy. The existing test_delete_share_instance\nparametrized tests partially cover this since they default\nservice_is_up\u003dTrue, but an explicit test for force\u003dTrue with\nservice up would make the intent clearer.","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":38940,"name":"Oluwasola Akintewe","display_name":"nathan_akin","email":"solaakintewe@gmail.com","username":"nathan_akin"},"change_message_id":"1251bf2e22142c4c8a67f48c73e7d05197e3f120","unresolved":false,"context_lines":[{"line_number":3297,"context_line":"            instance"},{"line_number":3298,"context_line":"        )"},{"line_number":3299,"context_line":""},{"line_number":3300,"context_line":"    def test_force_delete_share_instance_host_down(self):"},{"line_number":3301,"context_line":"        \"\"\"Force delete should delete from DB directly when host is down.\"\"\""},{"line_number":3302,"context_line":"        instance \u003d self._setup_delete_share_instance_mocks("},{"line_number":3303,"context_line":"            status\u003dconstants.STATUS_AVAILABLE,"}],"source_content_type":"text/x-python","patch_set":1,"id":"95af2a3d_84b19103","line":3300,"in_reply_to":"38de7b4d_83d647ab","updated":"2026-03-31 23:34:06.000000000","message":"Done","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"d9b0401b363ea657f9e944aa4dcc72732cd846c0","unresolved":true,"context_lines":[{"line_number":3311,"context_line":"        self.api.share_rpcapi.delete_share_instance.assert_not_called()"},{"line_number":3312,"context_line":""},{"line_number":3313,"context_line":"    def test_force_delete_share_instance_host_not_found(self):"},{"line_number":3314,"context_line":"        \"\"\"Force delete should delete from DB when service record is missing.\"\"\""},{"line_number":3315,"context_line":"        instance \u003d self._setup_delete_share_instance_mocks("},{"line_number":3316,"context_line":"            status\u003dconstants.STATUS_AVAILABLE,"},{"line_number":3317,"context_line":"            share_server_id\u003d\u0027fake\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"c3d17c96_ed32d7e3","line":3314,"updated":"2026-03-31 21:32:38.000000000","message":"This line is 80 characters long, exceeding Manila\u0027s 79-character limit.\nThis is the cause of the pep8 CI failure (E501). Shorten the docstring,\ne.g.:\n    \"\"\"Force delete should work when service record is missing.\"\"\"","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"},{"author":{"_account_id":38940,"name":"Oluwasola Akintewe","display_name":"nathan_akin","email":"solaakintewe@gmail.com","username":"nathan_akin"},"change_message_id":"1251bf2e22142c4c8a67f48c73e7d05197e3f120","unresolved":false,"context_lines":[{"line_number":3311,"context_line":"        self.api.share_rpcapi.delete_share_instance.assert_not_called()"},{"line_number":3312,"context_line":""},{"line_number":3313,"context_line":"    def test_force_delete_share_instance_host_not_found(self):"},{"line_number":3314,"context_line":"        \"\"\"Force delete should delete from DB when service record is missing.\"\"\""},{"line_number":3315,"context_line":"        instance \u003d self._setup_delete_share_instance_mocks("},{"line_number":3316,"context_line":"            status\u003dconstants.STATUS_AVAILABLE,"},{"line_number":3317,"context_line":"            share_server_id\u003d\u0027fake\u0027)"}],"source_content_type":"text/x-python","patch_set":1,"id":"41e6c16f_96d80b28","line":3314,"in_reply_to":"c3d17c96_ed32d7e3","updated":"2026-03-31 23:34:06.000000000","message":"Done","commit_id":"8c8d0f2d8b242142e6cfbb52aa98727762588542"}]}