)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":5520,"name":"Luis Fernández Álvarez","email":"luis.fernandez.alvarez@cern.ch","username":"luis-fernandez-alvarez"},"change_message_id":"099fcd28d1da50323891029338b46e78f3206e77","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":1,"id":"481d29e9_7edb74bb","updated":"2023-01-25 11:04:20.000000000","message":"I\u0027ve added a comment about the proposed implementation as it might not fix the bug due to the current implementation of the subvolume creation command on the Ceph side.","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"df6c00ebd57ef43f33af6938f273c014ae7b0ea3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"46633f4d_83ec76da","updated":"2023-01-24 23:16:53.000000000","message":"Thanks Carlos; ptal at the comments inline","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":5520,"name":"Luis Fernández Álvarez","email":"luis.fernandez.alvarez@cern.ch","username":"luis-fernandez-alvarez"},"change_message_id":"e1243312a1ab84cb97e7795d7d82fd665258bcb1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"b19d7ef3_639d991c","in_reply_to":"481d29e9_7edb74bb","updated":"2023-02-14 10:14:06.000000000","message":"Done","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"84b31cbd6430370a2bddabff246969fb2a36cc8c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"f3af8f97_8c2f454d","updated":"2023-02-03 14:42:47.000000000","message":"Thank you for the reviews! Please take a look at the latest PS","commit_id":"6b2803bda920f3be6aeecc6b7ae259139e481183"},{"author":{"_account_id":6413,"name":"Victoria Martinez de la Cruz","email":"victoria@redhat.com","username":"vkmc"},"change_message_id":"689e71cc8252669de5e4136875b2feb06ed41bce","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"06a03a44_70b4d581","updated":"2023-02-16 18:18:36.000000000","message":"Looks good to me, great job Carlos!","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"},{"author":{"_account_id":5520,"name":"Luis Fernández Álvarez","email":"luis.fernandez.alvarez@cern.ch","username":"luis-fernandez-alvarez"},"change_message_id":"0484773ee72862049ddb7f3ada402a2d8068f316","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"47fcb4d6_a1a890c8","updated":"2023-02-14 10:12:54.000000000","message":"Many thanks Carlos/Goutham for the updates. I think the proposed approach is sensible.","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"216833bec93358ffbd1c512c9feb16915cf09274","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"f7c943b8_3d961af6","updated":"2023-02-16 15:56:32.000000000","message":"recheck\n\nwe had some unrelated job failures here","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"cfa7ba9283a30fb0428ee9272036547c0b17b6ef","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"9f8839d9_8685386c","updated":"2023-02-06 12:26:33.000000000","message":"recheck\nFailures seem unrelated - first and second failures had different tests failing","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"7dfae25f9880e4804ec01033f224ae0574b6348e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"94867ff4_ba692090","updated":"2023-02-16 20:13:11.000000000","message":"recheck\nLVM failure - not related to this change","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"b4e6ea0ea5c4c8804fda93a841c1552a2ebc9030","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"d275ead4_e14d775d","updated":"2023-02-04 01:56:38.000000000","message":"recheck - the dummy driver failure is a race condition triggered by the tests, and unrelated to this change.\n\nLGTM; Thanks for the fix Carlos. Hoping this works for the CERN environment as well.. ","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"}],"manila/share/drivers/cephfs/driver.py":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"df6c00ebd57ef43f33af6938f273c014ae7b0ea3","unresolved":true,"context_lines":[{"line_number":132,"context_line":"                     \"will be used to construct share export locations.\"),"},{"line_number":133,"context_line":"    cfg.StrOpt(\u0027cephfs_volume_mode\u0027,"},{"line_number":134,"context_line":"               default\u003dDEFAULT_VOLUME_MODE,"},{"line_number":135,"context_line":"               help\u003d\"The read/write/execute permissions mode for CephFS \""},{"line_number":136,"context_line":"                    \"volumes, snapshots, and snapshot groups expressed in \""},{"line_number":137,"context_line":"                    \"Octal as with linux \u0027chmod\u0027 or \u0027umask\u0027 commands.\"),"},{"line_number":138,"context_line":"    cfg.StrOpt(\u0027cephfs_filesystem_name\u0027,"},{"line_number":139,"context_line":"               help\u003d\"The name of the filesystem to use, if there are \""},{"line_number":140,"context_line":"                    \"multiple filesystems in the cluster.\"),"}],"source_content_type":"text/x-python","patch_set":1,"id":"0aaaa7b5_9eb6ae5c","line":137,"range":{"start_line":135,"start_character":21,"end_line":137,"end_character":72},"updated":"2023-01-24 23:16:53.000000000","message":"I suggest adding a line here about the behavior of this option:\n\nChanges to this configuration option will only affect new resources.","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"df6c00ebd57ef43f33af6938f273c014ae7b0ea3","unresolved":true,"context_lines":[{"line_number":439,"context_line":"        return self._volname"},{"line_number":440,"context_line":""},{"line_number":441,"context_line":"    def create_share(self, context, share, share_server\u003dNone,"},{"line_number":442,"context_line":"                     ensuring_share_exists\u003dFalse):"},{"line_number":443,"context_line":"        \"\"\"Create a CephFS volume."},{"line_number":444,"context_line":""},{"line_number":445,"context_line":"        :param context: A RequestContext."}],"source_content_type":"text/x-python","patch_set":1,"id":"99e1f117_24159bc9","line":442,"range":{"start_line":442,"start_character":21,"end_line":442,"end_character":48},"updated":"2023-01-24 23:16:53.000000000","message":"i suggest calling this \"set_mode\", so it is generic, and default to True here..","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"dc8775576252145739ff15fcbd269265d9aa55aa","unresolved":false,"context_lines":[{"line_number":439,"context_line":"        return self._volname"},{"line_number":440,"context_line":""},{"line_number":441,"context_line":"    def create_share(self, context, share, share_server\u003dNone,"},{"line_number":442,"context_line":"                     ensuring_share_exists\u003dFalse):"},{"line_number":443,"context_line":"        \"\"\"Create a CephFS volume."},{"line_number":444,"context_line":""},{"line_number":445,"context_line":"        :param context: A RequestContext."}],"source_content_type":"text/x-python","patch_set":1,"id":"bd4dadd1_7b1cd8cb","line":442,"range":{"start_line":442,"start_character":21,"end_line":442,"end_character":48},"in_reply_to":"99e1f117_24159bc9","updated":"2023-02-03 14:42:35.000000000","message":"Ack","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"df6c00ebd57ef43f33af6938f273c014ae7b0ea3","unresolved":true,"context_lines":[{"line_number":470,"context_line":"            \"namespace_isolated\": True,"},{"line_number":471,"context_line":"        }"},{"line_number":472,"context_line":""},{"line_number":473,"context_line":"        # NOTE(carloss): We should enforce the share mode only when the share"},{"line_number":474,"context_line":"        # is being created. If mode is sent to the storage when we ensuring"},{"line_number":475,"context_line":"        # that a share exists and the configured value for mode changed after"},{"line_number":476,"context_line":"        # the share creation, it will end up modifying the current permissions"},{"line_number":477,"context_line":"        # of the share and can cause shares to be inaccessible."},{"line_number":478,"context_line":"        if not ensuring_share_exists:"},{"line_number":479,"context_line":"            argdict[\"mode\"] \u003d self._cephfs_volume_mode"},{"line_number":480,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"66eeac43_1001a0eb","line":477,"range":{"start_line":473,"start_character":0,"end_line":477,"end_character":63},"updated":"2023-01-24 23:16:53.000000000","message":"this comment makes more sense in the ensure_share method when you will explicitly turn off setting the mode","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"dc8775576252145739ff15fcbd269265d9aa55aa","unresolved":false,"context_lines":[{"line_number":470,"context_line":"            \"namespace_isolated\": True,"},{"line_number":471,"context_line":"        }"},{"line_number":472,"context_line":""},{"line_number":473,"context_line":"        # NOTE(carloss): We should enforce the share mode only when the share"},{"line_number":474,"context_line":"        # is being created. If mode is sent to the storage when we ensuring"},{"line_number":475,"context_line":"        # that a share exists and the configured value for mode changed after"},{"line_number":476,"context_line":"        # the share creation, it will end up modifying the current permissions"},{"line_number":477,"context_line":"        # of the share and can cause shares to be inaccessible."},{"line_number":478,"context_line":"        if not ensuring_share_exists:"},{"line_number":479,"context_line":"            argdict[\"mode\"] \u003d self._cephfs_volume_mode"},{"line_number":480,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"88b555ea_6cc7ae1a","line":477,"range":{"start_line":473,"start_character":0,"end_line":477,"end_character":63},"in_reply_to":"66eeac43_1001a0eb","updated":"2023-02-03 14:42:35.000000000","message":"Ack","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":5520,"name":"Luis Fernández Álvarez","email":"luis.fernandez.alvarez@cern.ch","username":"luis-fernandez-alvarez"},"change_message_id":"099fcd28d1da50323891029338b46e78f3206e77","unresolved":true,"context_lines":[{"line_number":476,"context_line":"        # the share creation, it will end up modifying the current permissions"},{"line_number":477,"context_line":"        # of the share and can cause shares to be inaccessible."},{"line_number":478,"context_line":"        if not ensuring_share_exists:"},{"line_number":479,"context_line":"            argdict[\"mode\"] \u003d self._cephfs_volume_mode"},{"line_number":480,"context_line":""},{"line_number":481,"context_line":"        if share[\u0027share_group_id\u0027] is not None:"},{"line_number":482,"context_line":"            argdict.update({\"group_name\": share[\"share_group_id\"]})"}],"source_content_type":"text/x-python","patch_set":1,"id":"8c12b6bd_a00c60f8","line":479,"updated":"2023-01-25 11:04:20.000000000","message":"Unfortunately, this condition is not enough to avoid this bug.\n\nEven if the driver doesn\u0027t pass the \"mode\" to the command, the current implementation of the command defines a default mode of \"755\" (even if \"--mode\" is not set!):\nhttps://github.com/ceph/ceph/blob/v16.2.9/src/pybind/mgr/volumes/module.py#L449\n\nThis behaviour has been reported here: https://tracker.ceph.com/issues/58432.\n\nSaid that, I would say only two \"safe\" options exist here:\n* Fetch current mode as proposed by Victoria in LP.\n* Modify `ensure_share` to only call creation if it doesn\u0027t exist (this is the patch I deployed in our deployment to temporary workaround the problem until his is fixed)","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"dc8775576252145739ff15fcbd269265d9aa55aa","unresolved":false,"context_lines":[{"line_number":476,"context_line":"        # the share creation, it will end up modifying the current permissions"},{"line_number":477,"context_line":"        # of the share and can cause shares to be inaccessible."},{"line_number":478,"context_line":"        if not ensuring_share_exists:"},{"line_number":479,"context_line":"            argdict[\"mode\"] \u003d self._cephfs_volume_mode"},{"line_number":480,"context_line":""},{"line_number":481,"context_line":"        if share[\u0027share_group_id\u0027] is not None:"},{"line_number":482,"context_line":"            argdict.update({\"group_name\": share[\"share_group_id\"]})"}],"source_content_type":"text/x-python","patch_set":1,"id":"f8dae849_fc23d298","line":479,"in_reply_to":"76454793_2449ed25","updated":"2023-02-03 14:42:35.000000000","message":"Thank you for the input - We thought about modifying the ensure_share to call creation only when the share was not already created, but as the end we think that wouldn\u0027t be the ideal behavior. If a subvolume no longer exists in ceph, then there is something wrong and it needs to be treated accordingly, so instead, we will do the ensure shares by only ensuring that the export path exists. Please take a look at the newest PS and let us know your thoughts.","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"0b8e867bfbbd68e60264c33161970632f15938a0","unresolved":true,"context_lines":[{"line_number":476,"context_line":"        # the share creation, it will end up modifying the current permissions"},{"line_number":477,"context_line":"        # of the share and can cause shares to be inaccessible."},{"line_number":478,"context_line":"        if not ensuring_share_exists:"},{"line_number":479,"context_line":"            argdict[\"mode\"] \u003d self._cephfs_volume_mode"},{"line_number":480,"context_line":""},{"line_number":481,"context_line":"        if share[\u0027share_group_id\u0027] is not None:"},{"line_number":482,"context_line":"            argdict.update({\"group_name\": share[\"share_group_id\"]})"}],"source_content_type":"text/x-python","patch_set":1,"id":"76454793_2449ed25","line":479,"in_reply_to":"8c12b6bd_a00c60f8","updated":"2023-01-27 00:53:18.000000000","message":"Thanks for the response here Luis; i think the latter is a better approach for now. We wanted to be able to avoid the ensure_share call altogether with this driver unless some config changes are made... I don\u0027t think the idempotent share creation logic is very helpful to us, atleast on each share service restart.","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"b4e6ea0ea5c4c8804fda93a841c1552a2ebc9030","unresolved":true,"context_lines":[{"line_number":545,"context_line":"            export_location \u003d self._get_export_locations(share)"},{"line_number":546,"context_line":"        except exception.ShareBackendException as e:"},{"line_number":547,"context_line":"            if \u0027does not exist\u0027 in str(e).lower():"},{"line_number":548,"context_line":"                raise exception.ShareResourceNotFound(share_id\u003dshare[\u0027id\u0027])"},{"line_number":549,"context_line":"            raise"},{"line_number":550,"context_line":""},{"line_number":551,"context_line":"        return export_location"}],"source_content_type":"text/x-python","patch_set":3,"id":"36ca5bfb_994d37e7","line":548,"range":{"start_line":548,"start_character":0,"end_line":548,"end_character":75},"updated":"2023-02-04 01:56:38.000000000","message":"thanks, as we discussed, it makes sense for the share manager to handle such exceptions and set the share status to \"error\"","commit_id":"c61c1977c25e151fd2839febd08a7fdac6b1f7b8"}],"releasenotes/notes/bug-2002394-fix-bad-mode-enforcement-on-ensure-shares-a2e4d8f6c07c8cf5.yaml":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"df6c00ebd57ef43f33af6938f273c014ae7b0ea3","unresolved":true,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Fixed an issue that made the CephFS driver to override the permissions in"},{"line_number":5,"context_line":"    a share. There are two moments that Manila sends requests to the Ceph"},{"line_number":6,"context_line":"    storage to create shares: when creating a new shared file system and when"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"83d207eb_dcb9f421","line":3,"updated":"2023-01-24 23:16:53.000000000","message":"This reads a bit confusing to me... \n\nthe bug arises because of a regression in ceph wrt idempotency of creation. the regression is a correction of a bug.. \n\nSo if a share\u0027s mode was modified outside of manila, or the configuration option ``cephfs_volume_mode`` was modified after a bunch of shares have been created, it is undesirable to enforce mode changes with manila during our ``ensure_shares`` routine because such changes may break applications; so we decided to ignore the mode here..\n\nPerhaps abridge along these lines? \n\nI do like the references to the ceph tracker and the LP..","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"},{"author":{"_account_id":29632,"name":"Carlos Eduardo","email":"ces.eduardo98@gmail.com","username":"silvacarlos"},"change_message_id":"dc8775576252145739ff15fcbd269265d9aa55aa","unresolved":false,"context_lines":[{"line_number":1,"context_line":"---"},{"line_number":2,"context_line":"fixes:"},{"line_number":3,"context_line":"  - |"},{"line_number":4,"context_line":"    Fixed an issue that made the CephFS driver to override the permissions in"},{"line_number":5,"context_line":"    a share. There are two moments that Manila sends requests to the Ceph"},{"line_number":6,"context_line":"    storage to create shares: when creating a new shared file system and when"}],"source_content_type":"text/x-yaml","patch_set":1,"id":"70015547_3c2d16eb","line":3,"in_reply_to":"83d207eb_dcb9f421","updated":"2023-02-03 14:42:35.000000000","message":"Thanks for the suggestions. As direction changed, I have updated to fit to the new approach, PTAL :)","commit_id":"4bee810f208e2358e12d94547c009b76288b619b"}]}