)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"5876c37600c42e1d26e5d33e3f70691ceb12cf79","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"0158c6a3_af648170","updated":"2026-08-22 04:40:33.000000000","message":"recheck\n\nfunctional test failure is unrelated; and nova team is investigating this issue post FF","commit_id":"f02878e08d020f805e1a7f03b4c40be563818bdb"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d76f5c0d3e8be8dc4d81a3d7feb8ea71d397256c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"b65a9fbd_ed613465","updated":"2026-09-02 16:10:03.000000000","message":"Actually should +1 for now until we are go on the rest of the patches, in case this one needs changes due to anything uncovered in the following patches.","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b1f8c261d7d6db3e764472b33678222e83f91d24","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"028393cb_f6eadde1","updated":"2026-09-02 16:05:35.000000000","message":"This looks OK to me","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"ad25a5e89ac131a9eb35e38262d676c7132f5577","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":6,"id":"8bcd9c9c_c46c613e","updated":"2026-09-03 22:20:44.000000000","message":"Top level comment because this patch will be updated:\n\n\n@dms@danplanet.com @jay@jvf.cc and I chatted on #openstack-nova [1] and we focussed on a concern Dan brought up.. \n\nin my words, the concern is:\n\nSince change doesn\u0027t drain the old/hardcoded `nova` rule on manila, the admin has to do the cleanup on Manila manually. After applying the fix and restarting each VM, they\u0027ll have to go deny the `nova` grant provided on each share on Manila. This is exceptionally hard to do if there are lots of instances mounting lots of shares. The restarts can take forever - order of weeks, months, years or never.\nOperators need to see where they are in the migration and know when it\u0027s safe to drop the old shared `nova` grant.\n\nOn top of that, the new identity is `nova-sha256(uuid:host)[:16]`. If you\u0027re looking at this only from manila, or from Ceph, it is a non-reversible hashed string that can\u0027t be tied back to instances.\n\nSo we came up with a proposal to stamp the instance UUID into the access rule\u0027s \"lock_reason\" field. It\u0027s tenant-safe (their own instance, their own share) and makes each per-instance rule traceable back to an instance (the \"Host\" identity isn\u0027t conveyed to Manila - that\u0027s infra topology and lock_reason is tenant-visible, so it can\u0027t carry CONF.host). \n\nDoing this will answer \"who has migrated.\" on a real production cloud. Without this, like JayF put it, we\u0027ll be leaving operators in a bad state where they don\u0027t know when they\u0027ll stop being vulnerable. We\u0027ll lay a durable trail through manila. \n\nSo an operator can verify if \"share X\u0027s nova grant is now safe to drop\". They\u0027ll do so by matching the lock-reason breadcrumbs against Nova\u0027s record of which instances still have the share attached. \n\n[1] https://meetings.opendev.org/irclogs/%23openstack-nova/%23openstack-nova.2026-09-03.log.html#openstack-nova.2026-09-03.log.html#t2026-09-03T19:13:52","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"568b03e96b38d4e8b8b1c6c7ab29096f243615d1","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"8c0a9e92_086cc561","updated":"2026-09-01 23:25:26.000000000","message":"recheck \n\ntests passed elsewhere in the series","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"}],"nova/compute/manager.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"e1a989b735d59cf154d8fcc7242c32065e2966e5","unresolved":true,"context_lines":[{"line_number":4897,"context_line":"            # CephFS uses a per-instance-per-host cephx identity, so a"},{"line_number":4898,"context_line":"            # share\u0027s access rule is never shared between instances."},{"line_number":4899,"context_line":"            # Only this instance\u0027s own mappings decide whether the rule"},{"line_number":4900,"context_line":"            # is still needed."},{"line_number":4901,"context_line":"            share_mappings_used_by_share \u003d ["},{"line_number":4902,"context_line":"                sm for sm in share_mappings_used_by_share"},{"line_number":4903,"context_line":"                if sm.instance_uuid \u003d\u003d instance_uuid"}],"source_content_type":"text/x-python","patch_set":6,"id":"d75c5653_9253dded","line":4900,"updated":"2026-09-03 19:21:27.000000000","message":"It does now (as of this patch), but didn\u0027t before. If I have an existing system with existing shares in place, isn\u0027t this going to include the system-wide identity? Perhaps it won\u0027t actually revoke it because of the status checks below?\n\nAnd if I\u0027m right about the above, I guess this won\u0027t leak any of the old mappings because on the last instance to use the old system-wide one, it _will_ be included in the list and _will_ pass the status checks below to get cleaned up?\n\nRegardless of the answer(s), I\u0027m not sure I see any tests here related to ensuring the upgrade/brownfield behavior. Am I missing them?","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"11b8f922ded6a62d7ce1f263426b8ca24a2f16f5","unresolved":false,"context_lines":[{"line_number":4897,"context_line":"            # CephFS uses a per-instance-per-host cephx identity, so a"},{"line_number":4898,"context_line":"            # share\u0027s access rule is never shared between instances."},{"line_number":4899,"context_line":"            # Only this instance\u0027s own mappings decide whether the rule"},{"line_number":4900,"context_line":"            # is still needed."},{"line_number":4901,"context_line":"            share_mappings_used_by_share \u003d ["},{"line_number":4902,"context_line":"                sm for sm in share_mappings_used_by_share"},{"line_number":4903,"context_line":"                if sm.instance_uuid \u003d\u003d instance_uuid"}],"source_content_type":"text/x-python","patch_set":6,"id":"4e0994e0_d31fa6c2","line":4900,"in_reply_to":"9333b653_23dcb136","updated":"2026-09-04 05:38:58.000000000","message":"Thanks for the async discussion as noted in the top level comment; the design to not store any breadcrumbs in nova that would tell us what instance-share combinations map to a specific access made this harder. I\u0027ve proposed a compromise that can be backported safely to older stable branches, and allow operators to secure their Ceph clusters against the shared access credential issue. We\u0027ll stash the instance info in Manila. \n\nSo, an operator that\u0027d like to fix their cloud can enumerate instances and their share attachments, find all the \"CEPHFS\" ones that are vulnerable to the shared/hardcoded access from Nova. \n\nAfter they patch their systems and reboot (or detach/migrate/attach) their servers, Nova will request and use the new ceph credential. The credential is still a one-way hash. To find out how many servers have begun using their own non-shared credentials, administrators can inspect the access rules on Manila - all the migrated ones will have the instance ID stored on Manila\u0027s access rule resource locks.\n\nSo the diff should be straight forward then to identify the \"stragglers\" that continue to use the old/shared \"nova\" credential.\n\nWill resolve this comment; please feel free to reopen if you think this approach needs more changes :)","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"e11eaba99c0bc5d8ea6e7ee3bd91ca8b78017b50","unresolved":true,"context_lines":[{"line_number":4897,"context_line":"            # CephFS uses a per-instance-per-host cephx identity, so a"},{"line_number":4898,"context_line":"            # share\u0027s access rule is never shared between instances."},{"line_number":4899,"context_line":"            # Only this instance\u0027s own mappings decide whether the rule"},{"line_number":4900,"context_line":"            # is still needed."},{"line_number":4901,"context_line":"            share_mappings_used_by_share \u003d ["},{"line_number":4902,"context_line":"                sm for sm in share_mappings_used_by_share"},{"line_number":4903,"context_line":"                if sm.instance_uuid \u003d\u003d instance_uuid"}],"source_content_type":"text/x-python","patch_set":6,"id":"9333b653_23dcb136","line":4900,"in_reply_to":"d75c5653_9253dded","updated":"2026-09-03 20:10:41.000000000","message":"No, the last instance to use nova does not clean it up. We don\u0027t touch the \"nova\" cephx user because we\u0027re recomputing the access rule on every grant/deny, so on any mapping (brownfield or not) deny() computes and targets nova-\u003chash\u003e. Nova never sees or references the old \"nova\" rule, so it never denies it (this will be like how any other manila side rules will be ignored by nova too)\n\nThat\u0027s intentional. Draining nova automatically would break not-yet-upgraded hosts during a rolling upgrade... they\u0027re still mounting via the shared nova key, and a single upgraded host can\u0027t know the whole cloud has moved off it. So the drain must be a manual, post-upgrade operator step, which I\u0027ll document in the OSSN (basically, apply fix/upgrade all compute nodes, restart each instance that uses a CephFS share via virtiofs, confirm that the new per-instance rules appear, then delete the nova access rule via Manila). The reconcile change at the end of the series [1] drains orphaned per-instance nova-\u003chex\u003e identities but deliberately leaves nova (tested via \"test_reconcile_stale_share_access_cephfs_revokes_orphan\"[2]).\n\nOn tests, `test_deny_share_cephfs_revokes_despite_other_instance` [3] asserts deny is called with nova-\u003chash\u003e (and exactly once)... and since access_to isn\u0027t persisted there\u0027s no separate brownfield DB state to set up. Do you think that\u0027s okay?\n\n[1] https://review.opendev.org/c/openstack/nova/+/1001916/\n[2] https://review.opendev.org/c/openstack/nova/+/1001916/18/nova/tests/unit/compute/test_compute_mgr.py#4254\n[3] https://review.opendev.org/c/openstack/nova/+/1001900/6/nova/tests/unit/compute/test_compute_mgr.py#3578","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8684551efd2bb45147311f30ebfbe24ebc493715","unresolved":true,"context_lines":[{"line_number":4727,"context_line":"        @utils.synchronized(share_mapping.share_id)"},{"line_number":4728,"context_line":"        def _allow_share(context, instance, share_mapping):"},{"line_number":4729,"context_line":"            def _apply_policy():"},{"line_number":4730,"context_line":"                # self.manila_api.lock(share_mapping.share_id)"},{"line_number":4731,"context_line":"                # Explicitly locking the share is not needed as"},{"line_number":4732,"context_line":"                # create_access_rule() from the sdk will do it if the"},{"line_number":4733,"context_line":"                # lock_visibility and lock_deletion flags are passed"}],"source_content_type":"text/x-python","patch_set":7,"id":"4a7c2a5a_16caebfc","line":4730,"updated":"2026-09-04 14:06:24.000000000","message":"I won\u0027t ask you to do it here, but it would be good to remove this (and the one in deny) in some patch later where we\u0027re doing cleanupy things. It was drilled into me early on to never leave commented-out code in production assets as it can be confusing. In the days of syntax highlighting and \"we don\u0027t even read the code anymore because $agents\" it\u0027s maybe less of a problem. However, this file is so large that we don\u0027t get syntax highlighting in gerrit and I read the comment below as \"we don\u0027t *need* to create a lock but we do anyway\" because it looked like we were doing so above.","commit_id":"56825a650895008ba23e9af7713c9c2e7e3e052f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8684551efd2bb45147311f30ebfbe24ebc493715","unresolved":false,"context_lines":[{"line_number":4742,"context_line":"                    share_mapping.access_type,"},{"line_number":4743,"context_line":"                    share_mapping.access_to,"},{"line_number":4744,"context_line":"                    \"rw\","},{"line_number":4745,"context_line":"                    lock_reason\u003dlock_reason,"},{"line_number":4746,"context_line":"                )"},{"line_number":4747,"context_line":""},{"line_number":4748,"context_line":"            def _wait_policy_to_be_applied():"}],"source_content_type":"text/x-python","patch_set":7,"id":"42e8a61a_c15e589c","line":4745,"updated":"2026-09-04 14:06:24.000000000","message":"Okay, so apologies that this is not all burned into my head but... I was thinking the \"reason\" was on the access rule and not the lock. But this, combined with the existing comment above tells me that the creation of the access rule will end up locking the share for each instance and providing a reason...on the lock. The access rule is _actually_ the residue that is dangerous to leave around and _could_ become disassociated from the lock and thus the reason? However, IIRC, there\u0027s one access rule and thus one lock per host now, and there will be an access rule _and_ lock per instance going forward so we\u0027re really looking for the presence of a new lock for every instance to determine if things have migrated.\n\nNow that I say that out loud, I think you said as much yesterday so I\u0027ll just leave this here as commentary for later reviewers.","commit_id":"56825a650895008ba23e9af7713c9c2e7e3e052f"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8684551efd2bb45147311f30ebfbe24ebc493715","unresolved":false,"context_lines":[{"line_number":4975,"context_line":"                        context,"},{"line_number":4976,"context_line":"                        share_mapping.share_id,"},{"line_number":4977,"context_line":"                        share_mapping.access_type,"},{"line_number":4978,"context_line":"                        share_mapping.access_to,"},{"line_number":4979,"context_line":"                    )"},{"line_number":4980,"context_line":""},{"line_number":4981,"context_line":"                share_mapping.delete()"}],"source_content_type":"text/x-python","patch_set":7,"id":"30c4a17c_03dfabbb","line":4978,"updated":"2026-09-04 14:06:24.000000000","message":"So just to be clear, this will never delete the whole-nova rule ever again after this patch, right? `deny()` calls `get_access()` which lists existing rules, looks for one that matches `access_to` and returns it for deletion. We\u0027ll never calculate the plain nova rule ever again and thus never remove it. Again, I think this is obvious but just leaving it here.","commit_id":"56825a650895008ba23e9af7713c9c2e7e3e052f"}],"nova/objects/share_mapping.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"28de238b9a7f076e17df1d9e76874f3b20151451","unresolved":true,"context_lines":[{"line_number":181,"context_line":"            digest \u003d hashlib.sha256("},{"line_number":182,"context_line":"                f\u0027{self.instance_uuid}:{CONF.host}\u0027.encode()"},{"line_number":183,"context_line":"            ).hexdigest()"},{"line_number":184,"context_line":"            self.access_to \u003d \u0027nova-\u0027 + digest[:16]"},{"line_number":185,"context_line":"        else:"},{"line_number":186,"context_line":"            raise exception.ShareProtocolNotSupported("},{"line_number":187,"context_line":"                share_proto\u003dself.share_proto"}],"source_content_type":"text/x-python","patch_set":6,"id":"801dadc7_d5810626","line":184,"updated":"2026-09-03 19:14:37.000000000","message":"Is this secret information? If it is, then okay I guess, but it seems like obfuscation that might make debugging harder as well as cleanup from places other than this. Also note that we have a hard block on renaming hosts, but this adds another cut (in the \"death by a thousand...\" realm) that would break subtly in the future if we needed to generate this value.\n\nIs this just for scoping to prevent a clash of multiple instances on the same host and a single instance (temporarily) on multiple hosts? Can we not just make this `nova-$instance-$host` or are you doing `[:16]` to fit in a length-restricted field?","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8684551efd2bb45147311f30ebfbe24ebc493715","unresolved":false,"context_lines":[{"line_number":181,"context_line":"            digest \u003d hashlib.sha256("},{"line_number":182,"context_line":"                f\u0027{self.instance_uuid}:{CONF.host}\u0027.encode()"},{"line_number":183,"context_line":"            ).hexdigest()"},{"line_number":184,"context_line":"            self.access_to \u003d \u0027nova-\u0027 + digest[:16]"},{"line_number":185,"context_line":"        else:"},{"line_number":186,"context_line":"            raise exception.ShareProtocolNotSupported("},{"line_number":187,"context_line":"                share_proto\u003dself.share_proto"}],"source_content_type":"text/x-python","patch_set":6,"id":"b0f6bf2e_de45ba02","line":184,"in_reply_to":"5b566413_04357047","updated":"2026-09-04 14:06:24.000000000","message":"Acknowledged","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"},{"author":{"_account_id":16643,"name":"Goutham Pacha Ravi","email":"gouthampravi@gmail.com","username":"gouthamr"},"change_message_id":"624e532d63b49fd80831d39a6059aa3a8bfff5cb","unresolved":true,"context_lines":[{"line_number":181,"context_line":"            digest \u003d hashlib.sha256("},{"line_number":182,"context_line":"                f\u0027{self.instance_uuid}:{CONF.host}\u0027.encode()"},{"line_number":183,"context_line":"            ).hexdigest()"},{"line_number":184,"context_line":"            self.access_to \u003d \u0027nova-\u0027 + digest[:16]"},{"line_number":185,"context_line":"        else:"},{"line_number":186,"context_line":"            raise exception.ShareProtocolNotSupported("},{"line_number":187,"context_line":"                share_proto\u003dself.share_proto"}],"source_content_type":"text/x-python","patch_set":6,"id":"5b566413_04357047","line":184,"in_reply_to":"801dadc7_d5810626","updated":"2026-09-03 19:50:52.000000000","message":"no, it\u0027s not secret — the value is fully deterministic and recomputable from instance_uuid and CONF.host. The sha256/[:16] only bounds the length to a fixed size; nothing is hidden.\n\nMy motivations: the point is scoping... reducing the blast radius if a single credential is ever stolen. instance_uuid gives per-instance isolation; CONF.host keeps grants independent when an instance is temporarily on two hosts during migration (so a source-side revoke doesn\u0027t break the destination)\n\nOn host rename: yes, this depends on CONF.host, which Nova already hard-blocks renaming... so it rides on that existing invariant. And for our virtiofs attach / instance recovery flows it\u0027s actually self-healing: this identity is recomputed on every grant/revoke, so a restart re-derives it (worst case is orphaned rules left on Manila, never broken instances).\n\n\nOn using nova-$instance-$host vs. hashing: there\u0027s no \"hard\" length limit (Manila allows 256 chars for access_to, ceph has no hardcoded limit in its C++ code). Two reasons I lean toward the hash: it matches how Manila and ceph\u0027s volume manager name on-the-fly cephx users, and it normalizes CONF.host.. from my understanding this can be an FQDN or contain characters awkward for a cephx entity name.. With the encoding, it becomes a clean, fixed-length identifier. Yes, you lose reverse lookup from `ceph auth ls`. In practice that\u0027s narrow... the OpenStack admin usually isn\u0027t the Ceph admin, and they have Manila between them. \n\nIf the OpenStack admin needs to drain rules they can recompute the identity from the prior CONF.host and the affected instances. I can document the construction here: https://review.opendev.org/c/openstack/nova/+/1000166\n\nwdyt?","commit_id":"0def1bea8f359c9f0aa29c073a865509af6bdb32"}]}