)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"050777980fffc4536560cc2cb180b474c9eef019","unresolved":false,"context_lines":[{"line_number":6,"context_line":""},{"line_number":7,"context_line":"rbd: Add breadcrumb for when udev rules are missing"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbdN\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbd/{pool}/{device}\u0027 symlink. If these rules aren\u0027t correctly"},{"line_number":11,"context_line":"configured (or entirely missing, say, when running nova-compute in a"},{"line_number":12,"context_line":"container where udev won\u0027t be running), this symlink won\u0027t be created."},{"line_number":13,"context_line":"This causes things to crash and burn when locally attaching an encrypted"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"bf51134e_edc5f88f","line":10,"range":{"start_line":9,"start_character":37,"end_line":10,"end_character":34},"updated":"2020-06-18 16:49:12.000000000","message":"Have I this symbol backwards? \u0027/dev/rbdN\u0027 is the real file. \u0027/dev/rbd/{pool}/{device}\u0027 is the symlink.","commit_id":"203a9744e60925516b6b01495acedc9cdbc6971c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5143ac10b99b9b3b32cf9eae7a4ae32bc1c3a43c","unresolved":false,"context_lines":[{"line_number":7,"context_line":"rbd: Create symlink when udev is not present or configured"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. In container-based deployments where the udev"},{"line_number":11,"context_line":"daemon is not present, this symlink will never be created. This causes"},{"line_number":12,"context_line":"things to crash and burn when locally attaching an encrypted volume:"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"  oslo_concurrency.processutils.ProcessExecutionError: Unexpected error while running command."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bf51134e_1c005901","line":11,"range":{"start_line":10,"start_character":21,"end_line":11,"end_character":58},"updated":"2020-06-18 18:13:11.000000000","message":"that is not what is happening.\n\nudev is present on the system (provided by systemd) but ceph-common pacakge is installed in the container so the udev rules are not viable on the host and are not executed.\n\nthe end result is the same but udev is still there.","commit_id":"c946fc397fbd80a41b526b4cf315e97d20c51234"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"23808d76f8e13b0b8a0f71d3cdb015768a46386e","unresolved":false,"context_lines":[{"line_number":7,"context_line":"rbd: Create symlink when udev is not present or configured"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. In container-based deployments where the udev"},{"line_number":11,"context_line":"daemon is not present, this symlink will never be created. This causes"},{"line_number":12,"context_line":"things to crash and burn when locally attaching an encrypted volume:"},{"line_number":13,"context_line":""},{"line_number":14,"context_line":"  oslo_concurrency.processutils.ProcessExecutionError: Unexpected error while running command."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"bf51134e_6328bee1","line":11,"range":{"start_line":10,"start_character":21,"end_line":11,"end_character":58},"in_reply_to":"bf51134e_1c005901","updated":"2020-07-22 10:16:59.000000000","message":"I was imprecise in my wording. I meant when libvirt is deployed/running inside a container, as opposed to running outside of the container. Will update","commit_id":"c946fc397fbd80a41b526b4cf315e97d20c51234"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"565795879df1b1019173093fe546b87526ed67cc","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_3539d93e","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"updated":"2020-07-08 11:28:46.000000000","message":"or if the host is missing the udev rule because it doesn\u0027t have ceph-common installed","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"b4490012b1b7c85a1915ed15753dbffb744d812e","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_e0a028f7","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"in_reply_to":"bf51134e_149c810c","updated":"2020-07-09 13:25:04.000000000","message":"Yeah apologies I\u0027ve been talking to Sean about this in the background and agree that longer term we need to burn down some of this debt and reliance on udev. I\u0027m only suggesting the installation of ceph-common as a short term workaround in TripleO when users want to use local attach.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcc0f31ff2de612390c6b55915d8cf45c9ea8451","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_5a92a358","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"in_reply_to":"bf51134e_149c810c","updated":"2020-07-09 12:19:32.000000000","message":"i dont think this patch exclues either adding the package to the host or altering os-brick to retrun the new path\n\ni think we should remove all depency on udev created symplink form os bricks or nova over time as in a containerise world this is not viable and it even has multi distro implication as the udev rules might not always be the same.\n\ni dont think we should install ceph-common on ooo hosts or kolla hosts for that matter.\n\ni like the idea of os-brick returing the path and allso the idea of making this a singel call but i think it might make sense to do that as a followup.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"15fcc0753cdf9f76d5e8da531c649d3a43787bd3","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_d875349a","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"in_reply_to":"bf51134e_3539d93e","updated":"2020-07-08 11:51:47.000000000","message":"Right, I\u0027d rather resolve this by ensuring these rules are present on the host itself.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"de7b6cf42517f13d26a22d888fecd342117eb987","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_290b2b7d","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"in_reply_to":"bf51134e_aef0e192","updated":"2020-07-09 15:54:33.000000000","message":"i am not ok with ooo deployting ceph-common.\n\ni consider the instalation of addtion packages on the host that are not requried at runtime to be a potential vector for security vulnerablites on the host as it increase the attack surface.\n\nthis package may be safe but out side of documenting the need to install it i dont think we should regress the removal of it form the host by adding it again.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9963c53c5b8d2e7d0a344d015da15ddf2e8eb677","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_149c810c","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"in_reply_to":"bf51134e_d875349a","updated":"2020-07-09 11:35:38.000000000","message":"That means installing packages on the host, and we may just be running os-brick inside a container. In that case telling administrators that they need to install packages on the host kind of defeats the purpose of using containers.\n\nAs an example, Ember-CSI (uses cinderlib which uses os-brick)  is deployed on OpenShift/Kubernetes, and telling administrators that they have to install ceph-common in the host would not be a good thing.\n\nThis patch is my second preferred solution, my first choice is  if we fix the Nova-Brick interaction for encrypted volumes.\n\nWe can make os-brick return a new path in the decryptor\u0027s \u0027attach_volume\u0027 and get Nova to use that path instead of the one from the connector\u0027s call.\n\nAlternatively we can make os-brick connectors\u0027 \u0027connect_volume\u0027 accept a \u0027do_decrypt\u003dTrue\u0027 parameter.  Then os-brick takes care of everything in one go, returning the decrypted DM (if it\u0027s an encrypted volume) and Nova would no longer need to make a second call to decrypt the volume.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"4c77684a6ace4b28a82c199aa617e97cb125ccd3","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Ceph uses udev rules [1] to create a \u0027/dev/rbd/{pool}/{device}\u0027 -\u003e"},{"line_number":10,"context_line":"\u0027/dev/rbdN\u0027 symlink. When using Ceph in a container environment where"},{"line_number":11,"context_line":"the udev daemon is not present, this symlink will never be created. This"},{"line_number":12,"context_line":"causes things to crash and burn when locally attaching an encrypted"},{"line_number":13,"context_line":"volume:"},{"line_number":14,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_aef0e192","line":11,"range":{"start_line":11,"start_character":0,"end_line":11,"end_character":30},"in_reply_to":"bf51134e_e0a028f7","updated":"2020-07-09 15:40:16.000000000","message":"Oh, OK, since I didn\u0027t see any TripleO reference I though we were talking in general.\nI\u0027m OK with the fix being TripleO deploying ceph-common as wel as this patch being completed, as long as we (Nova-Cinder) figure out a nice and clean solution afterwards.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"}],"os_brick/initiator/connectors/rbd.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"e895719d3e91d6f01eb46feeb8f83d9265fb710e","unresolved":false,"context_lines":[{"line_number":202,"context_line":"                        \u0027%(dev)s; are the ceph-renamer udev rules installed?\u0027,"},{"line_number":203,"context_line":"                        {"},{"line_number":204,"context_line":"                            \u0027vol\u0027: volume,"},{"line_number":205,"context_line":"                            \u0027dev\u0027: os.path.realpath(rbd_dev_path),"},{"line_number":206,"context_line":"                        },"},{"line_number":207,"context_line":"                    )"},{"line_number":208,"context_line":"            else:"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_a35caf7e","line":205,"updated":"2020-06-18 17:12:17.000000000","message":"This is wrong. If the file doesn\u0027t exist, realpath won\u0027t point to anything.","commit_id":"203a9744e60925516b6b01495acedc9cdbc6971c"},{"author":{"_account_id":5314,"name":"Brian Rosmaita","email":"rosmaita.fossdev@gmail.com","username":"brian-rosmaita"},"change_message_id":"f34c67396922a12793c76a076e20208379d47e65","unresolved":false,"context_lines":[{"line_number":204,"context_line":"                            \u0027vol\u0027: volume,"},{"line_number":205,"context_line":"                            \u0027dev\u0027: os.path.realpath(rbd_dev_path),"},{"line_number":206,"context_line":"                        },"},{"line_number":207,"context_line":"                    )"},{"line_number":208,"context_line":"            else:"},{"line_number":209,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":210,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_4dbfc4cf","line":207,"updated":"2020-06-18 15:57:30.000000000","message":"This is kind of a sanity check ... I wonder whether we should raise a BrickException as we do at lines 176-183.  You\u0027d need a more neutral message for the exception, though (don\u0027t want to expose the pool name).","commit_id":"203a9744e60925516b6b01495acedc9cdbc6971c"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"050777980fffc4536560cc2cb180b474c9eef019","unresolved":false,"context_lines":[{"line_number":204,"context_line":"                            \u0027vol\u0027: volume,"},{"line_number":205,"context_line":"                            \u0027dev\u0027: os.path.realpath(rbd_dev_path),"},{"line_number":206,"context_line":"                        },"},{"line_number":207,"context_line":"                    )"},{"line_number":208,"context_line":"            else:"},{"line_number":209,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":210,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":1,"id":"bf51134e_e33b47ce","line":207,"in_reply_to":"bf51134e_4dbfc4cf","updated":"2020-06-18 16:49:12.000000000","message":"sean-k-mooney suggested an alternative approach: we do the work that udev should be doing for us here. This would simply be a case of mapping \u0027/dev/rbdN\u0027 to \u0027/dev/rbd/{pool}/{volume}\u0027, or:\n\n  self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027,\n                root_device, rbd_dev_path,\n                root_helper\u003dself._root_helper,\n                run_as_root\u003dTrue, check_exit_code\u003dTrue)\n\nWe should still log to indicate that we\u0027re doing something out of the ordinary, but this is advantageous for the container-based deployments (which is where I\u0027ve hit this issue). With this approach, we remove the need to either run udev in the \u0027nova_compute\u0027 container or the install the ceph udev rules on the host.","commit_id":"203a9744e60925516b6b01495acedc9cdbc6971c"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"565795879df1b1019173093fe546b87526ed67cc","unresolved":false,"context_lines":[{"line_number":191,"context_line":"                LOG.error(msg)"},{"line_number":192,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"            # NOTE(stephenfin): sanity check if device is mounted"},{"line_number":195,"context_line":"            root_device \u003d self._find_root_device(connection_properties)"},{"line_number":196,"context_line":"            if not root_device:"},{"line_number":197,"context_line":"                msg \u003d _(\"device does not appear to be mounted\")"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_b51689a6","line":194,"range":{"start_line":194,"start_character":19,"end_line":194,"end_character":29},"updated":"2020-07-08 11:28:46.000000000","message":"We no longer include who writes the notes.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"4c77684a6ace4b28a82c199aa617e97cb125ccd3","unresolved":false,"context_lines":[{"line_number":191,"context_line":"                LOG.error(msg)"},{"line_number":192,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"            # NOTE(stephenfin): sanity check if device is mounted"},{"line_number":195,"context_line":"            root_device \u003d self._find_root_device(connection_properties)"},{"line_number":196,"context_line":"            if not root_device:"},{"line_number":197,"context_line":"                msg \u003d _(\"device does not appear to be mounted\")"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_c01ec4b9","line":194,"range":{"start_line":194,"start_character":19,"end_line":194,"end_character":29},"in_reply_to":"bf51134e_7a7e8719","updated":"2020-07-09 15:40:16.000000000","message":"The cinder team determined from experience that it has been useless for us, so we no longer want it.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcc0f31ff2de612390c6b55915d8cf45c9ea8451","unresolved":false,"context_lines":[{"line_number":191,"context_line":"                LOG.error(msg)"},{"line_number":192,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"            # NOTE(stephenfin): sanity check if device is mounted"},{"line_number":195,"context_line":"            root_device \u003d self._find_root_device(connection_properties)"},{"line_number":196,"context_line":"            if not root_device:"},{"line_number":197,"context_line":"                msg \u003d _(\"device does not appear to be mounted\")"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_7a7e8719","line":194,"range":{"start_line":194,"start_character":19,"end_line":194,"end_character":29},"in_reply_to":"bf51134e_b51689a6","updated":"2020-07-09 12:19:32.000000000","message":"just in os-brick because we still do this in nova and other projects. it can be useful to know who wrote the node so you can ask them about it and while git blame can sometimes help figure it out if code is moved in other patches it can be a pain.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"565795879df1b1019173093fe546b87526ed67cc","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # via the rbd kernel module."},{"line_number":203,"context_line":"            pool, volume \u003d connection_properties[\u0027name\u0027].split(\u0027/\u0027)"},{"line_number":204,"context_line":"            rbd_dev_path \u003d RBDConnector.get_rbd_device_name(pool, volume)"},{"line_number":205,"context_line":"            if not symlink_exists(rbd_dev_path):"},{"line_number":206,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027map\u0027, volume, \u0027--pool\u0027, pool]"},{"line_number":207,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_55272d13","line":205,"range":{"start_line":205,"start_character":19,"end_line":205,"end_character":33},"updated":"2020-07-08 11:28:46.000000000","message":"Since Nova is replacing the symlink we cannot tell if the symlink is actually pointing to the right device or if this is a leftover symlink...  :-(\n\nWe should at least check that when it\u0027s not an encrypted volume the real path points to root_device.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"15fcc0753cdf9f76d5e8da531c649d3a43787bd3","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # via the rbd kernel module."},{"line_number":203,"context_line":"            pool, volume \u003d connection_properties[\u0027name\u0027].split(\u0027/\u0027)"},{"line_number":204,"context_line":"            rbd_dev_path \u003d RBDConnector.get_rbd_device_name(pool, volume)"},{"line_number":205,"context_line":"            if not symlink_exists(rbd_dev_path):"},{"line_number":206,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027map\u0027, volume, \u0027--pool\u0027, pool]"},{"line_number":207,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_d827f4b5","line":205,"range":{"start_line":205,"start_character":19,"end_line":205,"end_character":33},"in_reply_to":"bf51134e_55272d13","updated":"2020-07-08 11:51:47.000000000","message":"s/Nova/os-brick/g - Nova doesn\u0027t touch the symlink, all of the logic for this is now in os-brick and has been for a while.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"9963c53c5b8d2e7d0a344d015da15ddf2e8eb677","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # via the rbd kernel module."},{"line_number":203,"context_line":"            pool, volume \u003d connection_properties[\u0027name\u0027].split(\u0027/\u0027)"},{"line_number":204,"context_line":"            rbd_dev_path \u003d RBDConnector.get_rbd_device_name(pool, volume)"},{"line_number":205,"context_line":"            if not symlink_exists(rbd_dev_path):"},{"line_number":206,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027map\u0027, volume, \u0027--pool\u0027, pool]"},{"line_number":207,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_143801ec","line":205,"range":{"start_line":205,"start_character":19,"end_line":205,"end_character":33},"in_reply_to":"bf51134e_d827f4b5","updated":"2020-07-09 11:35:38.000000000","message":"True, but as far as I remember the reason why we don\u0027t just return the device mapper\u0027s path on the decryptor\u0027s \u0027attach_volume\u0027 method and we have to replace the symlink is because of Nova. Because it uses the path it gets from the call to the connector\u0027s \u0027connect_volume\u0027.\n\nIf we fix that we can not only forget about this patch, but we can also remove some additional code that exists in the iSCSI connector code.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"565795879df1b1019173093fe546b87526ed67cc","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_959265f5","line":223,"updated":"2020-07-08 11:28:46.000000000","message":"-1: You need to ensure that the directory actually exists, because when running on a container and the host doesn\u0027t have the ceph-common common package installed, path /dev/rbd doesn\u0027t even exist, so the linking will fail.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcc0f31ff2de612390c6b55915d8cf45c9ea8451","unresolved":false,"context_lines":[{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_9ad43beb","line":223,"in_reply_to":"bf51134e_959265f5","updated":"2020-07-09 12:19:32.000000000","message":"/dev/rbdX will exist but not the /dev/rbd/ directory yes.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"565795879df1b1019173093fe546b87526ed67cc","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":209,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"                # the \u0027/dev/rbd/{pool}/{volume}\u0027 path should exist; if not, we"},{"line_number":212,"context_line":"                # probably haven\u0027t configured udev correctly or udev isn\u0027t"},{"line_number":213,"context_line":"                # running (a container, perhaps?)"},{"line_number":214,"context_line":"                if not symlink_exists(rbd_dev_path):"},{"line_number":215,"context_line":"                    LOG.info("},{"line_number":216,"context_line":"                        \u0027volume %(vol)s has not been mapped to local device \u0027"},{"line_number":217,"context_line":"                        \u0027%(dev)s; is the udev daemon running and are the \u0027"},{"line_number":218,"context_line":"                        \u0027ceph-renamer udev rules configured? Creating mapping \u0027"},{"line_number":219,"context_line":"                        \u0027manually\u0027,"},{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"},{"line_number":227,"context_line":"            else:"},{"line_number":228,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":229,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_754a31ac","line":226,"range":{"start_line":211,"start_character":1,"end_line":226,"end_character":73},"updated":"2020-07-08 11:28:46.000000000","message":"-1: Please move this to a privsep method instead of calling the command line.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"b22a2c4d92e2ba16bc91a26b1ea5f9c20d596af4","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":209,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"                # the \u0027/dev/rbd/{pool}/{volume}\u0027 path should exist; if not, we"},{"line_number":212,"context_line":"                # probably haven\u0027t configured udev correctly or udev isn\u0027t"},{"line_number":213,"context_line":"                # running (a container, perhaps?)"},{"line_number":214,"context_line":"                if not symlink_exists(rbd_dev_path):"},{"line_number":215,"context_line":"                    LOG.info("},{"line_number":216,"context_line":"                        \u0027volume %(vol)s has not been mapped to local device \u0027"},{"line_number":217,"context_line":"                        \u0027%(dev)s; is the udev daemon running and are the \u0027"},{"line_number":218,"context_line":"                        \u0027ceph-renamer udev rules configured? Creating mapping \u0027"},{"line_number":219,"context_line":"                        \u0027manually\u0027,"},{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"},{"line_number":227,"context_line":"            else:"},{"line_number":228,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":229,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_279a2813","line":226,"range":{"start_line":211,"start_character":1,"end_line":226,"end_character":73},"in_reply_to":"bf51134e_09d3c7d6","updated":"2020-07-10 10:29:14.000000000","message":"According to the Oslo Privsep documentation that\u0027s not the right way to do it [1]:\n\n  Privsep functions live in a specific privsep submodule (for example, nova.privsep for nova).\n\nand:\n\n  Contexts are defined in the privsep/__init__.py file.\n\nSome projects may call the directory \"privileged\" instead of \"privsep\", but they are all following that recommendation: Nova, Cinder, Neutron, Zun, Ceilometer, Cyborg, etc.\n\n[1]: https://docs.openstack.org/oslo.privsep/latest/user/index.html","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dfaf6388a29a7d62bea4f9355b70f0c86ef4ec5d","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":209,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"                # the \u0027/dev/rbd/{pool}/{volume}\u0027 path should exist; if not, we"},{"line_number":212,"context_line":"                # probably haven\u0027t configured udev correctly or udev isn\u0027t"},{"line_number":213,"context_line":"                # running (a container, perhaps?)"},{"line_number":214,"context_line":"                if not symlink_exists(rbd_dev_path):"},{"line_number":215,"context_line":"                    LOG.info("},{"line_number":216,"context_line":"                        \u0027volume %(vol)s has not been mapped to local device \u0027"},{"line_number":217,"context_line":"                        \u0027%(dev)s; is the udev daemon running and are the \u0027"},{"line_number":218,"context_line":"                        \u0027ceph-renamer udev rules configured? Creating mapping \u0027"},{"line_number":219,"context_line":"                        \u0027manually\u0027,"},{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"},{"line_number":227,"context_line":"            else:"},{"line_number":228,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":229,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_b80f752c","line":226,"range":{"start_line":211,"start_character":1,"end_line":226,"end_character":73},"in_reply_to":"bf51134e_279a2813","updated":"2020-07-10 12:35:09.000000000","message":"yes the oslo docs are worng and nova cinder an neuton all are bad examples to follow for different reasons. this has come up at the PTG and on the mailing list several times\n\nif you have not already read\nhttp://lists.openstack.org/pipermail/openstack-discuss/2020-March/013433.html\nand http://lists.openstack.org/pipermail/openstack-discuss/2019-March/004358.html\n\nas to why the approch taken in Nova and other project shoudl not be followed they are a good read.\n\nif you look at the cyborg review in particalr you will notice i was ask to review and share some of the best practices\nwe have developed since the mistaks that were maind in nova,cinder and neutron ectra have come to light. \nhttps://review.opendev.org/#/c/673957/16\n\ncyborg more or less implemented \n\nit still has a singel context wich is bad but they jsut have not had peole to work on decomposing it into multiple contexts yet\n\nhttps://github.com/openstack/cyborg/blob/e64bcd598ff924dab58c4c9e296bd10c4bde283f/cyborg/privsep/__init__.py#L20\n\nbut you will note that its scoped to the cyborg module and all of its sub modules.\n\nit is used to create functions in the module where they are used, that are suffixed with privaldaged \n\n@cyborg.privsep.sys_admin_pctxt.entrypoint\ndef lspci_privileged():\n    cmd \u003d [\u0027lspci\u0027, \u0027-nnn\u0027, \u0027-D\u0027]\n    return processutils.execute(*cmd)\n\nhttps://github.com/openstack/cyborg/blob/973716aa23d033df3a23aa2b34ce533f3f320c8f/cyborg/accelerator/drivers/aichip/huawei/ascend.py#L35-L38\n\nanyway this is a spereate discussion but centrailised privldage fucntion encurages more generalised function that take arbratry args that can cause problems if they take user provided input. that is the main reason for haveing lots of small module local specific functions that are defined where they are used","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcc0f31ff2de612390c6b55915d8cf45c9ea8451","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":209,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"                # the \u0027/dev/rbd/{pool}/{volume}\u0027 path should exist; if not, we"},{"line_number":212,"context_line":"                # probably haven\u0027t configured udev correctly or udev isn\u0027t"},{"line_number":213,"context_line":"                # running (a container, perhaps?)"},{"line_number":214,"context_line":"                if not symlink_exists(rbd_dev_path):"},{"line_number":215,"context_line":"                    LOG.info("},{"line_number":216,"context_line":"                        \u0027volume %(vol)s has not been mapped to local device \u0027"},{"line_number":217,"context_line":"                        \u0027%(dev)s; is the udev daemon running and are the \u0027"},{"line_number":218,"context_line":"                        \u0027ceph-renamer udev rules configured? Creating mapping \u0027"},{"line_number":219,"context_line":"                        \u0027manually\u0027,"},{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"},{"line_number":227,"context_line":"            else:"},{"line_number":228,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":229,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_dae233d5","line":226,"range":{"start_line":211,"start_character":1,"end_line":226,"end_character":73},"in_reply_to":"bf51134e_754a31ac","updated":"2020-07-09 12:19:32.000000000","message":"we proably should actully use python to create the symlink instead fo calling ln too.\n\nthe privsep method shoudl ideally be defiend in this file also and takke the pool volume and root dev and constuct the rbd_dev_path internally passing in paths to privesep fucntions is an antipattern as is grouping them in a centralised module.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"4c77684a6ace4b28a82c199aa617e97cb125ccd3","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":209,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"                # the \u0027/dev/rbd/{pool}/{volume}\u0027 path should exist; if not, we"},{"line_number":212,"context_line":"                # probably haven\u0027t configured udev correctly or udev isn\u0027t"},{"line_number":213,"context_line":"                # running (a container, perhaps?)"},{"line_number":214,"context_line":"                if not symlink_exists(rbd_dev_path):"},{"line_number":215,"context_line":"                    LOG.info("},{"line_number":216,"context_line":"                        \u0027volume %(vol)s has not been mapped to local device \u0027"},{"line_number":217,"context_line":"                        \u0027%(dev)s; is the udev daemon running and are the \u0027"},{"line_number":218,"context_line":"                        \u0027ceph-renamer udev rules configured? Creating mapping \u0027"},{"line_number":219,"context_line":"                        \u0027manually\u0027,"},{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"},{"line_number":227,"context_line":"            else:"},{"line_number":228,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":229,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_e0bdc8b7","line":226,"range":{"start_line":211,"start_character":1,"end_line":226,"end_character":73},"in_reply_to":"bf51134e_dae233d5","updated":"2020-07-09 15:40:16.000000000","message":"Yeah, we are in agreement, using Python code is precisely what I was suggesting in my comment.\n\nAs for including the privsep method here, with our current privsep contexts we can\u0027t write the method here, it must be under the cinder/privsep directory or the privsep server side will fail to execute.\n\nWe can either create the method as cinder/privsep/rbd.py (like we have for LVM) or create a new context for this specific connector (which iirc will create a new privsep service).\n\nOf course we can also just create a class/static method in here that has the code and call it from cinder/privsep, but being a simple and reusable method like \"create_symlink\" it makes more sense to have it there.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e6968ff4ef8d54ab6120d6da124e6cc6f307c0e4","unresolved":false,"context_lines":[{"line_number":208,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":209,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":210,"context_line":""},{"line_number":211,"context_line":"                # the \u0027/dev/rbd/{pool}/{volume}\u0027 path should exist; if not, we"},{"line_number":212,"context_line":"                # probably haven\u0027t configured udev correctly or udev isn\u0027t"},{"line_number":213,"context_line":"                # running (a container, perhaps?)"},{"line_number":214,"context_line":"                if not symlink_exists(rbd_dev_path):"},{"line_number":215,"context_line":"                    LOG.info("},{"line_number":216,"context_line":"                        \u0027volume %(vol)s has not been mapped to local device \u0027"},{"line_number":217,"context_line":"                        \u0027%(dev)s; is the udev daemon running and are the \u0027"},{"line_number":218,"context_line":"                        \u0027ceph-renamer udev rules configured? Creating mapping \u0027"},{"line_number":219,"context_line":"                        \u0027manually\u0027,"},{"line_number":220,"context_line":"                        {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":221,"context_line":"                    )"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"                    self._execute(\u0027ln\u0027, \u0027--symbolic\u0027, \u0027--force\u0027, root_device,"},{"line_number":224,"context_line":"                                  rbd_dev_path,"},{"line_number":225,"context_line":"                                  root_helper\u003dself._root_helper,"},{"line_number":226,"context_line":"                                  run_as_root\u003dTrue, check_exit_code\u003dTrue)"},{"line_number":227,"context_line":"            else:"},{"line_number":228,"context_line":"                LOG.debug(\u0027volume %(vol)s is already mapped to local\u0027"},{"line_number":229,"context_line":"                          \u0027 device %(dev)s\u0027,"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_09d3c7d6","line":226,"range":{"start_line":211,"start_character":1,"end_line":226,"end_character":73},"in_reply_to":"bf51134e_e0bdc8b7","updated":"2020-07-09 16:00:33.000000000","message":"actully i was suggestign that there should be a os-brick context at the os_brick modoule level that was used to allow privldaged function to be created in any submodule.\n\nusing centralised pirivsep fucntiosn leads to more generic privsep function that are less secure and tend to have more privladges then are requried.\n\nyou also dont want to have a large number of context in differeen modules with the same permissions as that wastes meory on the host for each deamon and in general resuced performance\n\nso you want as set of privsep context with limited permissions at the root of the lib and use the approicate context when needed.\n\nanyway i have not looked how os-bricks uses privesp but that is how it should be using it in my view.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"565795879df1b1019173093fe546b87526ed67cc","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027unmap\u0027, root_device]"},{"line_number":313,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":314,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":315,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":316,"context_line":"        else:"},{"line_number":317,"context_line":"            if device_info:"},{"line_number":318,"context_line":"                rbd_handle \u003d device_info.get(\u0027path\u0027, None)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_95fd453c","line":315,"updated":"2020-07-08 11:28:46.000000000","message":"-1: We have to remove the symlink, otherwise it may be pointing to the wrong device the next time we try to attach the same volume.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"b22a2c4d92e2ba16bc91a26b1ea5f9c20d596af4","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027unmap\u0027, root_device]"},{"line_number":313,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":314,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":315,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":316,"context_line":"        else:"},{"line_number":317,"context_line":"            if device_info:"},{"line_number":318,"context_line":"                rbd_handle \u003d device_info.get(\u0027path\u0027, None)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_c7888cc7","line":315,"in_reply_to":"bf51134e_29804bbf","updated":"2020-07-10 10:29:14.000000000","message":"We would have to ignore that error anyway, since the file could be removed between the exists check and the remove, so the extra step would just add additional code.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"4c77684a6ace4b28a82c199aa617e97cb125ccd3","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027unmap\u0027, root_device]"},{"line_number":313,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":314,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":315,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":316,"context_line":"        else:"},{"line_number":317,"context_line":"            if device_info:"},{"line_number":318,"context_line":"                rbd_handle \u003d device_info.get(\u0027path\u0027, None)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_a0bf50a8","line":315,"in_reply_to":"bf51134e_5a3c632e","updated":"2020-07-09 15:40:16.000000000","message":"I\u0027m missing something about your comment... Are you agreeing or disagreeing with what I said?\n\nIf you are agreeing, then we don\u0027t need to do it conditionally. We can just remove it and ignore if it fails because the file doesn\u0027t exist.\n\nThis will also need to be a privsep method.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcc0f31ff2de612390c6b55915d8cf45c9ea8451","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027unmap\u0027, root_device]"},{"line_number":313,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":314,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":315,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":316,"context_line":"        else:"},{"line_number":317,"context_line":"            if device_info:"},{"line_number":318,"context_line":"                rbd_handle \u003d device_info.get(\u0027path\u0027, None)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_5a3c632e","line":315,"in_reply_to":"bf51134e_95fd453c","updated":"2020-07-09 12:19:32.000000000","message":"well we have to conditionally remove it after the unmap if it still exists as the udev rule will do that if it exists.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"e6968ff4ef8d54ab6120d6da124e6cc6f307c0e4","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027unmap\u0027, root_device]"},{"line_number":313,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":314,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":315,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":316,"context_line":"        else:"},{"line_number":317,"context_line":"            if device_info:"},{"line_number":318,"context_line":"                rbd_handle \u003d device_info.get(\u0027path\u0027, None)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_29804bbf","line":315,"in_reply_to":"bf51134e_a0bf50a8","updated":"2020-07-09 16:00:33.000000000","message":"ya i guess we could ignore the error code if the symlink does not exist. that works too.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dfaf6388a29a7d62bea4f9355b70f0c86ef4ec5d","unresolved":false,"context_lines":[{"line_number":312,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027unmap\u0027, root_device]"},{"line_number":313,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":314,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":315,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":316,"context_line":"        else:"},{"line_number":317,"context_line":"            if device_info:"},{"line_number":318,"context_line":"                rbd_handle \u003d device_info.get(\u0027path\u0027, None)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_18194179","line":315,"in_reply_to":"bf51134e_c7888cc7","updated":"2020-07-10 12:35:09.000000000","message":"ya that is fair we coudl race with udev.","commit_id":"c4cd8794aa340f917cb3e6854ab40ee660d881a1"},{"author":{"_account_id":10135,"name":"Lee Yarwood","display_name":"Lee Yarwood","email":"lyarwood@redhat.com","username":"lyarwood"},"change_message_id":"9012efbf317f595d72f99e532f2d1f0f1330ab40","unresolved":false,"context_lines":[{"line_number":183,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"            # Sanity check if device is mounted"},{"line_number":186,"context_line":"            root_device \u003d self._find_root_device(connection_properties)"},{"line_number":187,"context_line":"            if not root_device:"},{"line_number":188,"context_line":"                msg \u003d _(\"Device does not appear to be mounted\")"},{"line_number":189,"context_line":"                LOG.error(msg)"},{"line_number":190,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"            # NOTE(e0ne): map volume to a block device"},{"line_number":193,"context_line":"            # via the rbd kernel module."}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_76f2659c","line":190,"range":{"start_line":186,"start_character":0,"end_line":190,"end_character":59},"updated":"2020-07-22 13:29:34.000000000","message":"You need to mock this out in the tests now that it\u0027s being called during connect_volume.","commit_id":"f170e29ed00ac32121cab97bf103830b6dd43122"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"aba9dbd8f344a23491f7763580fbf858821aed53","unresolved":false,"context_lines":[{"line_number":183,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":184,"context_line":""},{"line_number":185,"context_line":"            # Sanity check if device is mounted"},{"line_number":186,"context_line":"            root_device \u003d self._find_root_device(connection_properties)"},{"line_number":187,"context_line":"            if not root_device:"},{"line_number":188,"context_line":"                msg \u003d _(\"Device does not appear to be mounted\")"},{"line_number":189,"context_line":"                LOG.error(msg)"},{"line_number":190,"context_line":"                raise exception.BrickException(message\u003dmsg)"},{"line_number":191,"context_line":""},{"line_number":192,"context_line":"            # NOTE(e0ne): map volume to a block device"},{"line_number":193,"context_line":"            # via the rbd kernel module."}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_9b391df4","line":190,"range":{"start_line":186,"start_character":0,"end_line":190,"end_character":59},"in_reply_to":"bf51134e_76f2659c","updated":"2020-07-22 15:54:38.000000000","message":"Done","commit_id":"f170e29ed00ac32121cab97bf103830b6dd43122"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c4a61e528c5f1fb038c51ad3fddf3ac85f4a53be","unresolved":false,"context_lines":[{"line_number":186,"context_line":"            # via the rbd kernel module."},{"line_number":187,"context_line":"            pool, volume \u003d connection_properties[\u0027name\u0027].split(\u0027/\u0027)"},{"line_number":188,"context_line":"            rbd_dev_path \u003d RBDConnector.get_rbd_device_name(pool, volume)"},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"            if ("},{"line_number":191,"context_line":"                not os.path.islink(rbd_dev_path) or"},{"line_number":192,"context_line":"                not os.path.exists(os.path.realpath(rbd_dev_path))"},{"line_number":193,"context_line":"            ):"},{"line_number":194,"context_line":"                cmd \u003d [\u0027rbd\u0027, \u0027map\u0027, volume, \u0027--pool\u0027, pool]"},{"line_number":195,"context_line":"                cmd +\u003d self._get_rbd_args(connection_properties)"},{"line_number":196,"context_line":"                self._execute(*cmd, root_helper\u003dself._root_helper,"},{"line_number":197,"context_line":"                              run_as_root\u003dTrue)"},{"line_number":198,"context_line":"            else:"},{"line_number":199,"context_line":"                LOG.debug("},{"line_number":200,"context_line":"                    \u0027Volume %(vol)s is already mapped to local device %(dev)s\u0027,"},{"line_number":201,"context_line":"                    {\u0027vol\u0027: volume, \u0027dev\u0027: os.path.realpath(rbd_dev_path)}"},{"line_number":202,"context_line":"                )"},{"line_number":203,"context_line":""},{"line_number":204,"context_line":"            if ("},{"line_number":205,"context_line":"                not os.path.islink(rbd_dev_path) or"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_abf48f88","line":202,"range":{"start_line":189,"start_character":0,"end_line":202,"end_character":17},"updated":"2020-07-30 12:07:21.000000000","message":"nit: Please next time don\u0027t make unrelated cosmetic changes to the code when fixing something (even if they make the code more readable like in this case). They introduce unnecessary merge conflicts on other patches and make them harder to resolve.\n\nThe patch should only have been lines 204 to L215","commit_id":"ee34d925ff8a8a83345941b7876b09f2c0396864"},{"author":{"_account_id":9535,"name":"Gorka Eguileor","email":"geguileo@redhat.com","username":"Gorka"},"change_message_id":"c4a61e528c5f1fb038c51ad3fddf3ac85f4a53be","unresolved":false,"context_lines":[{"line_number":213,"context_line":"                    {\u0027vol\u0027: volume, \u0027dev\u0027: rbd_dev_path},"},{"line_number":214,"context_line":"                )"},{"line_number":215,"context_line":""},{"line_number":216,"context_line":"            return {\u0027path\u0027: rbd_dev_path, \u0027type\u0027: \u0027block\u0027}"},{"line_number":217,"context_line":""},{"line_number":218,"context_line":"        rbd_handle \u003d self._get_rbd_handle(connection_properties)"},{"line_number":219,"context_line":"        return {\u0027path\u0027: rbd_handle}"}],"source_content_type":"text/x-python","patch_set":7,"id":"9f560f44_0bf49b85","line":216,"range":{"start_line":216,"start_character":0,"end_line":216,"end_character":58},"updated":"2020-07-30 12:07:21.000000000","message":"ditto","commit_id":"ee34d925ff8a8a83345941b7876b09f2c0396864"}],"os_brick/tests/initiator/connectors/test_rbd.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"30bf397d3c6285fc4f6d9a5fd147d750c2c07e0d","unresolved":false,"context_lines":[{"line_number":198,"context_line":"        self.assertEqual(expected_info, device_info)"},{"line_number":199,"context_line":""},{"line_number":200,"context_line":"    @mock.patch.object(priv_rootwrap, \u0027execute\u0027, return_value\u003dNone)"},{"line_number":201,"context_line":"    @mock.patch.object(rbd, \u0027symlink_exists\u0027, side_effect\u003d(False, False))"},{"line_number":202,"context_line":"    @mock.patch.object(rbd.RBDConnector, \u0027_find_root_device\u0027,"},{"line_number":203,"context_line":"                       return_value\u003d\u0027/dev/rbd0\u0027)"},{"line_number":204,"context_line":"    def test_connect_local_volume_no_udev("}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_bc5fad0c","line":201,"range":{"start_line":201,"start_character":59,"end_line":201,"end_character":71},"updated":"2020-06-18 18:17:48.000000000","message":"first false is to emulate the volume is not mounted\nthe second false if for udev not creating the symlink\n\nthis could just be retrun_value\u003dFalse too\nbut this is fine","commit_id":"c946fc397fbd80a41b526b4cf315e97d20c51234"}]}