)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"06a2e24a6b71766590be3287c6242ac9a951d106","unresolved":true,"context_lines":[{"line_number":13,"context_line":"The `dev` attribute of the `target` element in the libvirt device XML is"},{"line_number":14,"context_line":"just a hint and not guaranteed to be how the OS maps the device [1]. It"},{"line_number":15,"context_line":"is usually suggested to use `/dev/disk/by-id/` to be sure about the"},{"line_number":16,"context_line":"underlying device, but CirrOS currently does not populate `/dev/disk/`."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"[1]: https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"b838118c_701cbba8","line":16,"updated":"2025-01-20 14:50:25.000000000","message":"that is populated by udev rule normally \n\nbut yes you should use the serial or volume id rather than the name.\n\nthe device name is not part of the api conct in nova so as you noted it is just a hint and not something the test code shoudl rely on.\n\nyou can us blkid or other means to correcatlae the cinder volume with the block device in the guest as an alternitive to /dev/disk/by-id","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"8a2dd66f11c72bcc87e89c5003f499a453ef3445","unresolved":false,"context_lines":[{"line_number":13,"context_line":"The `dev` attribute of the `target` element in the libvirt device XML is"},{"line_number":14,"context_line":"just a hint and not guaranteed to be how the OS maps the device [1]. It"},{"line_number":15,"context_line":"is usually suggested to use `/dev/disk/by-id/` to be sure about the"},{"line_number":16,"context_line":"underlying device, but CirrOS currently does not populate `/dev/disk/`."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"[1]: https://libvirt.org/formatdomain.html#hard-drives-floppy-disks-cdroms"},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":2,"id":"a4355180_38c31919","line":16,"in_reply_to":"b838118c_701cbba8","updated":"2025-01-21 10:46:19.000000000","message":"`blkid` does not work with virtio devices currently, only option is the\n(truncated) serial in `/sys/block/\u003cDEV\u003e/serial`","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"1d59376eae1977f9d92e30bfe87c64303f9ebbcc","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"8f093b75_4ed94ed6","updated":"2025-01-20 18:10:27.000000000","message":"lgtm, it is moving the volume attach operation afte ssh access which is right thing. Waiting from sean response if anything else I am missing.","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8303c3daddf5c13777428c6dea5b821f79834521","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":8,"id":"4b16120d_5f4c9f51","updated":"2025-01-21 13:39:53.000000000","message":"im mostly fine with this but i defer to the tempest cores on the poitn i raised inline","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"38b1638d0668289923165094a2a86287e0cae751","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"3cc63bd4_49b7ac44","updated":"2025-01-21 14:46:05.000000000","message":"Thanks again for the review, I\u0027ve changed a few things here and there.","commit_id":"0e98f6063cf6934c900a107c447d06cdb0186817"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"c0c3b446c7cc95f6013e4fdc41780296519ea0a4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"03159f7f_3c9010cb","updated":"2025-01-24 03:23:32.000000000","message":"lgtm,thanks","commit_id":"83f386cd79aa6fc2c418da5b219f843c5443b9d6"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"45cca082a6269aaf243c3d8dd8899ad6d370e8fd","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"5187731c_83422d0c","updated":"2025-01-22 16:57:55.000000000","message":"recheck\n\ntempest-full-ubuntu-jammy","commit_id":"83f386cd79aa6fc2c418da5b219f843c5443b9d6"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"82b880d263a6414f60ec3070cbbb070688a7b413","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"32d789e5_cfdd0e98","updated":"2025-01-28 16:18:48.000000000","message":"seems worth testing","commit_id":"83f386cd79aa6fc2c418da5b219f843c5443b9d6"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5cc651661fbdaffd0b504d3894fe1db3673e70c3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"3bde65be_7387fd3e","updated":"2025-01-22 12:46:16.000000000","message":"thanks i think this looks pretty good to me lets see if gmann agrees :)","commit_id":"83f386cd79aa6fc2c418da5b219f843c5443b9d6"}],"tempest/scenario/test_instances_with_cinder_volumes.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bdddd5cc5cff91d207a31329a9a77addba0ab218","unresolved":true,"context_lines":[{"line_number":148,"context_line":"            attached_volumes \u003d []"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"            # wait for server to become active"},{"line_number":151,"context_line":"            waiters.wait_for_server_status(self.servers_client,"},{"line_number":152,"context_line":"                                           server[\u0027id\u0027], \u0027ACTIVE\u0027)"},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"            # assign floating ip"},{"line_number":155,"context_line":"            floating_ip \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"b76ee9cc_e18887b3","line":152,"range":{"start_line":151,"start_character":12,"end_line":152,"end_character":66},"updated":"2025-01-20 14:47:07.000000000","message":"Active is not a sufficent precondition to volume attachment in tempest.\n\nall thest athat attach or detach a volume at runtime should wait fo sshable.","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"19f53fbfdb920bd72bcae2712e4a9106f67fa873","unresolved":false,"context_lines":[{"line_number":148,"context_line":"            attached_volumes \u003d []"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"            # wait for server to become active"},{"line_number":151,"context_line":"            waiters.wait_for_server_status(self.servers_client,"},{"line_number":152,"context_line":"                                           server[\u0027id\u0027], \u0027ACTIVE\u0027)"},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"            # assign floating ip"},{"line_number":155,"context_line":"            floating_ip \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"8029421f_a6799f4e","line":152,"range":{"start_line":151,"start_character":12,"end_line":152,"end_character":66},"in_reply_to":"afbc7a48_7f5f5d9a","updated":"2025-01-20 18:24:11.000000000","message":"Done","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"1d59376eae1977f9d92e30bfe87c64303f9ebbcc","unresolved":true,"context_lines":[{"line_number":148,"context_line":"            attached_volumes \u003d []"},{"line_number":149,"context_line":""},{"line_number":150,"context_line":"            # wait for server to become active"},{"line_number":151,"context_line":"            waiters.wait_for_server_status(self.servers_client,"},{"line_number":152,"context_line":"                                           server[\u0027id\u0027], \u0027ACTIVE\u0027)"},{"line_number":153,"context_line":""},{"line_number":154,"context_line":"            # assign floating ip"},{"line_number":155,"context_line":"            floating_ip \u003d None"}],"source_content_type":"text/x-python","patch_set":2,"id":"afbc7a48_7f5f5d9a","line":152,"range":{"start_line":151,"start_character":12,"end_line":152,"end_character":66},"in_reply_to":"b76ee9cc_e18887b3","updated":"2025-01-20 18:10:27.000000000","message":"I think we are doing at L169 which should be enough","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"bdddd5cc5cff91d207a31329a9a77addba0ab218","unresolved":true,"context_lines":[{"line_number":171,"context_line":"                server\u003dserver"},{"line_number":172,"context_line":"            )"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"            # attach volumes to the instances"},{"line_number":175,"context_line":"            for volume in created_volumes[start:end]:"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"                # wait for volume to become available"}],"source_content_type":"text/x-python","patch_set":2,"id":"007777d4_b4668ed3","line":174,"updated":"2025-01-20 14:47:07.000000000","message":"so really if you do not modify the previous wait you shoul add\n\n```\n waiters.wait_for_server_status(\n     self.servers_client, server[\u0027id\u0027], \u0027SSHABLE\u0027)\n```\n   \nhere to ensure that the vm is booted to a point where it can service the ahci hotplug request to complete the volume attachment and detach as relevent.","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"db8da3c7e0ccf03eba4a4eca2fea801846822928","unresolved":true,"context_lines":[{"line_number":171,"context_line":"                server\u003dserver"},{"line_number":172,"context_line":"            )"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"            # attach volumes to the instances"},{"line_number":175,"context_line":"            for volume in created_volumes[start:end]:"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"                # wait for volume to become available"}],"source_content_type":"text/x-python","patch_set":2,"id":"e9a1d162_114f9723","line":174,"in_reply_to":"007777d4_b4668ed3","updated":"2025-01-20 18:17:45.000000000","message":"gmann pointed out that get_remote_client calls linux_client.validate_authentication()\n\nhttps://github.com/openstack/tempest/blob/a110fc009a832dc74f304cad0faa613a938ff829/tempest/scenario/manager.py#L802-L805\n\nwhich internally validates the auth and tries to conenct\n\nhttps://github.com/openstack/tempest/blob/a110fc009a832dc74f304cad0faa613a938ff829/tempest/lib/common/utils/linux/remote_client.py#L118-L123\n\ninternally that also has a rety mechanism\n\nhttps://github.com/openstack/tempest/blob/master/tempest/lib/common/ssh.py#L114-L164\n\nso that should be sufficnt to make sure the guest os is suffictly up to be able to respond to ahci hotplug events.\n\nso we do not need an explcit\n\n```\n waiters.wait_for_server_status(\n     self.servers_client, server[\u0027id\u0027], \u0027SSHABLE\u0027)\n```\n\nin this case.","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"1d59376eae1977f9d92e30bfe87c64303f9ebbcc","unresolved":true,"context_lines":[{"line_number":171,"context_line":"                server\u003dserver"},{"line_number":172,"context_line":"            )"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"            # attach volumes to the instances"},{"line_number":175,"context_line":"            for volume in created_volumes[start:end]:"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"                # wait for volume to become available"}],"source_content_type":"text/x-python","patch_set":2,"id":"0da3b698_b820e3b7","line":174,"in_reply_to":"007777d4_b4668ed3","updated":"2025-01-20 18:10:27.000000000","message":"we do ssh access at L169","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"19f53fbfdb920bd72bcae2712e4a9106f67fa873","unresolved":false,"context_lines":[{"line_number":171,"context_line":"                server\u003dserver"},{"line_number":172,"context_line":"            )"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"            # attach volumes to the instances"},{"line_number":175,"context_line":"            for volume in created_volumes[start:end]:"},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"                # wait for volume to become available"}],"source_content_type":"text/x-python","patch_set":2,"id":"36c11ee5_05d7aff1","line":174,"in_reply_to":"e9a1d162_114f9723","updated":"2025-01-20 18:24:11.000000000","message":"Done","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4652f276b71bd57803518443510774f7c6ee5402","unresolved":true,"context_lines":[{"line_number":186,"context_line":"                attach_ok \u003d False"},{"line_number":187,"context_line":"                while retry \u003c 4 and not attach_ok:"},{"line_number":188,"context_line":"                    try:"},{"line_number":189,"context_line":"                        self.linux_client.exec_command(f\"ls /dev/{dev_name}\")"},{"line_number":190,"context_line":"                        attach_ok \u003d True"},{"line_number":191,"context_line":"                    except exceptions.SSHExecCommandFailed:"},{"line_number":192,"context_line":"                        retry +\u003d 1"}],"source_content_type":"text/x-python","patch_set":2,"id":"050ae7e7_89dda812","line":189,"updated":"2025-01-20 18:23:33.000000000","message":"this proably was not obvious but part of my orginal -1 was because of this.\n\nthis additional verification feels like its unrelated to the ordering issue an should be done in a separate commit.\n\ni mentioned din the commit message that we should not rely on this in the tempest test as it not part of the api contract and with libvirt it is explcitly not enforced.\n\ncan use blkid or similar to deicover the device by its serial adn fallback to dev_name if that does not work?","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"19f53fbfdb920bd72bcae2712e4a9106f67fa873","unresolved":true,"context_lines":[{"line_number":186,"context_line":"                attach_ok \u003d False"},{"line_number":187,"context_line":"                while retry \u003c 4 and not attach_ok:"},{"line_number":188,"context_line":"                    try:"},{"line_number":189,"context_line":"                        self.linux_client.exec_command(f\"ls /dev/{dev_name}\")"},{"line_number":190,"context_line":"                        attach_ok \u003d True"},{"line_number":191,"context_line":"                    except exceptions.SSHExecCommandFailed:"},{"line_number":192,"context_line":"                        retry +\u003d 1"}],"source_content_type":"text/x-python","patch_set":2,"id":"faae4eac_8fe9e568","line":189,"in_reply_to":"050ae7e7_89dda812","updated":"2025-01-20 18:24:11.000000000","message":"agree, we should fix this","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"8a2dd66f11c72bcc87e89c5003f499a453ef3445","unresolved":false,"context_lines":[{"line_number":186,"context_line":"                attach_ok \u003d False"},{"line_number":187,"context_line":"                while retry \u003c 4 and not attach_ok:"},{"line_number":188,"context_line":"                    try:"},{"line_number":189,"context_line":"                        self.linux_client.exec_command(f\"ls /dev/{dev_name}\")"},{"line_number":190,"context_line":"                        attach_ok \u003d True"},{"line_number":191,"context_line":"                    except exceptions.SSHExecCommandFailed:"},{"line_number":192,"context_line":"                        retry +\u003d 1"}],"source_content_type":"text/x-python","patch_set":2,"id":"8357a323_101e80ca","line":189,"in_reply_to":"b7c15672_8d0a8ef9","updated":"2025-01-21 10:46:19.000000000","message":"Done","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8ff53cbb108c65cca01c6bdc0dc709b5dcd0125f","unresolved":true,"context_lines":[{"line_number":186,"context_line":"                attach_ok \u003d False"},{"line_number":187,"context_line":"                while retry \u003c 4 and not attach_ok:"},{"line_number":188,"context_line":"                    try:"},{"line_number":189,"context_line":"                        self.linux_client.exec_command(f\"ls /dev/{dev_name}\")"},{"line_number":190,"context_line":"                        attach_ok \u003d True"},{"line_number":191,"context_line":"                    except exceptions.SSHExecCommandFailed:"},{"line_number":192,"context_line":"                        retry +\u003d 1"}],"source_content_type":"text/x-python","patch_set":2,"id":"b7c15672_8d0a8ef9","line":189,"in_reply_to":"faae4eac_8fe9e568","updated":"2025-01-20 19:33:01.000000000","message":"if we have the baselien block device reexecuting the same command willl provide the current blks and if you subtackt the previous blks set form ti you will get the new deivce name.\n\n```\ncurrnt_blks \u003d set(linux_client.exec_command(command).strip())\nnew_blk \u003d currnet_blks - blks\n```\n\nyou can then decided to validate the serical if you want to with \n\n```\nnew_serial \u003d linux_client.exec_command(f\"cat /sys/block/{new_blk}/serial\") \nself.assertIn(new_serial, volume[\u0027id\u0027])\n```\n\nif we dont want to compare either the serial or just the device name hint then you coudl simple  `self.assertTrue(len(currnt_blks) \u003e len(blks))`","commit_id":"d67475368212bbf421219643c88ccea3ada5ac13"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8ff53cbb108c65cca01c6bdc0dc709b5dcd0125f","unresolved":true,"context_lines":[{"line_number":177,"context_line":"                # wait for volume to become available"},{"line_number":178,"context_line":"                waiters.wait_for_volume_resource_status("},{"line_number":179,"context_line":"                    self.volumes_client, volume[\u0027id\u0027], \u0027available\u0027)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"                attached_volume \u003d self.nova_volume_attach(server, volume)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"                dev_name \u003d attached_volume[\u0027attachments\u0027][0][\u0027device\u0027][5:]"}],"source_content_type":"text/x-python","patch_set":4,"id":"e1892d43_c84844fe","line":180,"updated":"2025-01-20 19:33:01.000000000","message":"would it be better to recored the existign block device by doing somethign like this\n\n```\n19:21:32]➜ ls -al /sys/block | awk \u0027{print $9}\u0027 | grep -v \u0027\\.\u0027 | xargs -l\ndm-0\nnvme0n1\nzram0\n```\n\nthat provide the baselien set of block devices.\n\n```\n  command \u003d \"\u0027ls -al /sys/block | awk \u0027{print $9}\u0027 | grep -v \u0027\\.\u0027 | xargs -l\"\n  blks \u003d set(linux_client.exec_command(command).strip())\n```","commit_id":"868e9a55808c1c2841c120b27dda7ea5451f6cbc"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"8a2dd66f11c72bcc87e89c5003f499a453ef3445","unresolved":true,"context_lines":[{"line_number":177,"context_line":"                # wait for volume to become available"},{"line_number":178,"context_line":"                waiters.wait_for_volume_resource_status("},{"line_number":179,"context_line":"                    self.volumes_client, volume[\u0027id\u0027], \u0027available\u0027)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"                attached_volume \u003d self.nova_volume_attach(server, volume)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"                dev_name \u003d attached_volume[\u0027attachments\u0027][0][\u0027device\u0027][5:]"}],"source_content_type":"text/x-python","patch_set":4,"id":"e5db1270_ff8a7fd0","line":180,"in_reply_to":"e1892d43_c84844fe","updated":"2025-01-21 10:46:19.000000000","message":"I prefer `find` instead of `ls` for this but let me know if you want `ls` specifically.","commit_id":"868e9a55808c1c2841c120b27dda7ea5451f6cbc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8303c3daddf5c13777428c6dea5b821f79834521","unresolved":false,"context_lines":[{"line_number":177,"context_line":"                # wait for volume to become available"},{"line_number":178,"context_line":"                waiters.wait_for_volume_resource_status("},{"line_number":179,"context_line":"                    self.volumes_client, volume[\u0027id\u0027], \u0027available\u0027)"},{"line_number":180,"context_line":""},{"line_number":181,"context_line":"                attached_volume \u003d self.nova_volume_attach(server, volume)"},{"line_number":182,"context_line":""},{"line_number":183,"context_line":"                dev_name \u003d attached_volume[\u0027attachments\u0027][0][\u0027device\u0027][5:]"}],"source_content_type":"text/x-python","patch_set":4,"id":"0e58e25f_e30a5a32","line":180,"in_reply_to":"e5db1270_ff8a7fd0","updated":"2025-01-21 13:39:53.000000000","message":"either works for me. i was avoiding /dev in my example to ignore the other special devices such as TTYs or kvm and only look at the block devices.\n\nper the docs for find https://man7.org/linux/man-pages/man1/find.1.html\n\n-type b will filter to only the block devcies so the comman you specified below is equivelent","commit_id":"868e9a55808c1c2841c120b27dda7ea5451f6cbc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8ff53cbb108c65cca01c6bdc0dc709b5dcd0125f","unresolved":true,"context_lines":[{"line_number":189,"context_line":"                        attach_ok \u003d True"},{"line_number":190,"context_line":"                    except exceptions.SSHExecCommandFailed:"},{"line_number":191,"context_line":"                        retry +\u003d 1"},{"line_number":192,"context_line":"                        time.sleep(2**retry)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"                self.assertTrue(attach_ok, \"Could not attach volume\" +"},{"line_number":195,"context_line":"                                f\" {volume[\u0027id\u0027]} as device /dev/{dev_name}\" +"}],"source_content_type":"text/x-python","patch_set":4,"id":"db18cb14_92930b2c","line":192,"updated":"2025-01-20 19:33:01.000000000","message":"here you would need to do \n``blk \u003d current_blk``\n\nto update it for the next iteration of the loop.","commit_id":"868e9a55808c1c2841c120b27dda7ea5451f6cbc"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"8a2dd66f11c72bcc87e89c5003f499a453ef3445","unresolved":false,"context_lines":[{"line_number":189,"context_line":"                        attach_ok \u003d True"},{"line_number":190,"context_line":"                    except exceptions.SSHExecCommandFailed:"},{"line_number":191,"context_line":"                        retry +\u003d 1"},{"line_number":192,"context_line":"                        time.sleep(2**retry)"},{"line_number":193,"context_line":""},{"line_number":194,"context_line":"                self.assertTrue(attach_ok, \"Could not attach volume\" +"},{"line_number":195,"context_line":"                                f\" {volume[\u0027id\u0027]} as device /dev/{dev_name}\" +"}],"source_content_type":"text/x-python","patch_set":4,"id":"29da7c30_48a237bf","line":192,"in_reply_to":"db18cb14_92930b2c","updated":"2025-01-21 10:46:19.000000000","message":"Done","commit_id":"868e9a55808c1c2841c120b27dda7ea5451f6cbc"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8303c3daddf5c13777428c6dea5b821f79834521","unresolved":true,"context_lines":[{"line_number":222,"context_line":"        waiters.wait_for_volume_resource_status("},{"line_number":223,"context_line":"            self.volumes_client, volume[\u0027id\u0027], \u0027available\u0027)"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"        list_blks \u003d \"find /dev -maxdepth 1 -type b -exec basename \u0027{}\u0027 \u0027;\u0027\""},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"        blks_before \u003d set(self.linux_client.exec_command("},{"line_number":228,"context_line":"            list_blks).strip().splitlines())"}],"source_content_type":"text/x-python","patch_set":8,"id":"4a4f7db6_ca3ba12c","line":225,"range":{"start_line":225,"start_character":31,"end_line":225,"end_character":51},"updated":"2025-01-21 13:39:53.000000000","message":"as noted above b is block devices and -maxdepth 1 prevent recusing into /dev/disk or /dev/mapper\n\non operating systems that have the udev rules to populate them.\n\nso this looks reasonable to me","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"11af06703867adffd35328c6daaa7fe4feecc760","unresolved":false,"context_lines":[{"line_number":222,"context_line":"        waiters.wait_for_volume_resource_status("},{"line_number":223,"context_line":"            self.volumes_client, volume[\u0027id\u0027], \u0027available\u0027)"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"        list_blks \u003d \"find /dev -maxdepth 1 -type b -exec basename \u0027{}\u0027 \u0027;\u0027\""},{"line_number":226,"context_line":""},{"line_number":227,"context_line":"        blks_before \u003d set(self.linux_client.exec_command("},{"line_number":228,"context_line":"            list_blks).strip().splitlines())"}],"source_content_type":"text/x-python","patch_set":8,"id":"1dde904c_e01937a9","line":225,"range":{"start_line":225,"start_character":31,"end_line":225,"end_character":51},"in_reply_to":"4a4f7db6_ca3ba12c","updated":"2025-01-21 15:03:07.000000000","message":"Acknowledged","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8303c3daddf5c13777428c6dea5b821f79834521","unresolved":true,"context_lines":[{"line_number":235,"context_line":"        while retry \u003c 4 and not actual_dev:"},{"line_number":236,"context_line":"            blks_now \u003d set(self.linux_client.exec_command("},{"line_number":237,"context_line":"                list_blks).strip().splitlines())"},{"line_number":238,"context_line":"            for blk_dev in (blks_now - blks_before):"},{"line_number":239,"context_line":"                serial \u003d self.linux_client.exec_command("},{"line_number":240,"context_line":"                    f\"cat /sys/block/{blk_dev}/serial\")"},{"line_number":241,"context_line":"                if serial \u003d\u003d volume[\u0027id\u0027][:len(serial)]:"}],"source_content_type":"text/x-python","patch_set":8,"id":"001c0b9c_f94bd2e0","line":238,"range":{"start_line":238,"start_character":27,"end_line":238,"end_character":51},"updated":"2025-01-21 13:39:53.000000000","message":"are you doing this to support volumes with a partition table with multiple partitions?","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"38b1638d0668289923165094a2a86287e0cae751","unresolved":true,"context_lines":[{"line_number":235,"context_line":"        while retry \u003c 4 and not actual_dev:"},{"line_number":236,"context_line":"            blks_now \u003d set(self.linux_client.exec_command("},{"line_number":237,"context_line":"                list_blks).strip().splitlines())"},{"line_number":238,"context_line":"            for blk_dev in (blks_now - blks_before):"},{"line_number":239,"context_line":"                serial \u003d self.linux_client.exec_command("},{"line_number":240,"context_line":"                    f\"cat /sys/block/{blk_dev}/serial\")"},{"line_number":241,"context_line":"                if serial \u003d\u003d volume[\u0027id\u0027][:len(serial)]:"}],"source_content_type":"text/x-python","patch_set":8,"id":"41ab6ea5_42744dbc","line":238,"range":{"start_line":238,"start_character":27,"end_line":238,"end_character":51},"in_reply_to":"001c0b9c_f94bd2e0","updated":"2025-01-21 14:46:05.000000000","message":"It is because I would like the detection to run only on devices that have appeared after the attach has been initiated.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5cc651661fbdaffd0b504d3894fe1db3673e70c3","unresolved":false,"context_lines":[{"line_number":235,"context_line":"        while retry \u003c 4 and not actual_dev:"},{"line_number":236,"context_line":"            blks_now \u003d set(self.linux_client.exec_command("},{"line_number":237,"context_line":"                list_blks).strip().splitlines())"},{"line_number":238,"context_line":"            for blk_dev in (blks_now - blks_before):"},{"line_number":239,"context_line":"                serial \u003d self.linux_client.exec_command("},{"line_number":240,"context_line":"                    f\"cat /sys/block/{blk_dev}/serial\")"},{"line_number":241,"context_line":"                if serial \u003d\u003d volume[\u0027id\u0027][:len(serial)]:"}],"source_content_type":"text/x-python","patch_set":8,"id":"332a184c_de9214e8","line":238,"range":{"start_line":238,"start_character":27,"end_line":238,"end_character":51},"in_reply_to":"13ea1bf3_7026c8f0","updated":"2025-01-22 12:46:16.000000000","message":"Acknowledged","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"f8309b03a09d42591124a20298de3a878fa26089","unresolved":true,"context_lines":[{"line_number":235,"context_line":"        while retry \u003c 4 and not actual_dev:"},{"line_number":236,"context_line":"            blks_now \u003d set(self.linux_client.exec_command("},{"line_number":237,"context_line":"                list_blks).strip().splitlines())"},{"line_number":238,"context_line":"            for blk_dev in (blks_now - blks_before):"},{"line_number":239,"context_line":"                serial \u003d self.linux_client.exec_command("},{"line_number":240,"context_line":"                    f\"cat /sys/block/{blk_dev}/serial\")"},{"line_number":241,"context_line":"                if serial \u003d\u003d volume[\u0027id\u0027][:len(serial)]:"}],"source_content_type":"text/x-python","patch_set":8,"id":"a86470b9_9bbf3486","line":238,"range":{"start_line":238,"start_character":27,"end_line":238,"end_character":51},"in_reply_to":"21d5d67b_40008ba6","updated":"2025-01-22 08:49:12.000000000","message":"Right, I understand now. I could change it to something like:\n\n    only_new \u003d blks_now - blks_before\n    if len(only_new) \u003e 0: # assert(len(only_new) \u003d\u003d 1)?\n        blk_dev \u003d only_new.pop()\n        ...\n\nThe iteration over the set difference handles the empty set diff case,\nso to me they seem the same in this case. Unless we want to be very\nprecise about what happens inside the guest and error out if we detect\nmore than one new block device, I would like to keep the for loop.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"11af06703867adffd35328c6daaa7fe4feecc760","unresolved":true,"context_lines":[{"line_number":235,"context_line":"        while retry \u003c 4 and not actual_dev:"},{"line_number":236,"context_line":"            blks_now \u003d set(self.linux_client.exec_command("},{"line_number":237,"context_line":"                list_blks).strip().splitlines())"},{"line_number":238,"context_line":"            for blk_dev in (blks_now - blks_before):"},{"line_number":239,"context_line":"                serial \u003d self.linux_client.exec_command("},{"line_number":240,"context_line":"                    f\"cat /sys/block/{blk_dev}/serial\")"},{"line_number":241,"context_line":"                if serial \u003d\u003d volume[\u0027id\u0027][:len(serial)]:"}],"source_content_type":"text/x-python","patch_set":8,"id":"21d5d67b_40008ba6","line":238,"range":{"start_line":238,"start_character":27,"end_line":238,"end_character":51},"in_reply_to":"41ab6ea5_42744dbc","updated":"2025-01-21 15:03:07.000000000","message":"thats not what i mean\n\nwe are attach at most one volume per invocation of this function.\n\nthe only way for there to be multiple block devices here is if the cinder volume has multiple partions and each partion is included in the find command which they are\n\n```\n[15:01:29]❯ find /dev -maxdepth 1 -type b -exec basename \u0027{}\u0027 \u0027;\u0027\nnvme0n1\nnvme0n1p1\nnvme0n1p2\nnvme0n1p3\ndm-0\nzram0\n```","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"e1560f23a73aa10b4237c3c0de4386a9f79b8160","unresolved":true,"context_lines":[{"line_number":235,"context_line":"        while retry \u003c 4 and not actual_dev:"},{"line_number":236,"context_line":"            blks_now \u003d set(self.linux_client.exec_command("},{"line_number":237,"context_line":"                list_blks).strip().splitlines())"},{"line_number":238,"context_line":"            for blk_dev in (blks_now - blks_before):"},{"line_number":239,"context_line":"                serial \u003d self.linux_client.exec_command("},{"line_number":240,"context_line":"                    f\"cat /sys/block/{blk_dev}/serial\")"},{"line_number":241,"context_line":"                if serial \u003d\u003d volume[\u0027id\u0027][:len(serial)]:"}],"source_content_type":"text/x-python","patch_set":8,"id":"13ea1bf3_7026c8f0","line":238,"range":{"start_line":238,"start_character":27,"end_line":238,"end_character":51},"in_reply_to":"a86470b9_9bbf3486","updated":"2025-01-22 11:13:35.000000000","message":"I changed the `find` to an `lsblk`, which will filter out partitions now. This will also fixe a bug with the retry logic.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8303c3daddf5c13777428c6dea5b821f79834521","unresolved":true,"context_lines":[{"line_number":249,"context_line":"            retry +\u003d 1"},{"line_number":250,"context_line":"            time.sleep(2**retry)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        self.assertIsNotNone("},{"line_number":253,"context_line":"            actual_dev,"},{"line_number":254,"context_line":"            f\"Could not attach volume {volume[\u0027id\u0027]} inside the guest.\" +"},{"line_number":255,"context_line":"            \" The test will not work\")"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"        if dev_name !\u003d actual_dev:"},{"line_number":258,"context_line":"            LOG.info("}],"source_content_type":"text/x-python","patch_set":8,"id":"32ee0652_320cd359","line":255,"range":{"start_line":252,"start_character":0,"end_line":255,"end_character":38},"updated":"2025-01-21 13:39:53.000000000","message":"so if disk type is lun, we cannot program the cinder volume uuid as the device serial so this is not true in all cases and im not sure if its corect for non libvirt backend i.e. vmware.\n\nso im inclidned to say that as long as   len(blks_now - blks_before) \u003e0\nwe shoudl not fail the test but should log a warning instead.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"11af06703867adffd35328c6daaa7fe4feecc760","unresolved":false,"context_lines":[{"line_number":249,"context_line":"            retry +\u003d 1"},{"line_number":250,"context_line":"            time.sleep(2**retry)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        self.assertIsNotNone("},{"line_number":253,"context_line":"            actual_dev,"},{"line_number":254,"context_line":"            f\"Could not attach volume {volume[\u0027id\u0027]} inside the guest.\" +"},{"line_number":255,"context_line":"            \" The test will not work\")"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"        if dev_name !\u003d actual_dev:"},{"line_number":258,"context_line":"            LOG.info("}],"source_content_type":"text/x-python","patch_set":8,"id":"72195da6_0cdf7810","line":255,"range":{"start_line":252,"start_character":0,"end_line":255,"end_character":38},"in_reply_to":"2c6d3a52_3c85add3","updated":"2025-01-21 15:03:07.000000000","message":"Acknowledged","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"38b1638d0668289923165094a2a86287e0cae751","unresolved":true,"context_lines":[{"line_number":249,"context_line":"            retry +\u003d 1"},{"line_number":250,"context_line":"            time.sleep(2**retry)"},{"line_number":251,"context_line":""},{"line_number":252,"context_line":"        self.assertIsNotNone("},{"line_number":253,"context_line":"            actual_dev,"},{"line_number":254,"context_line":"            f\"Could not attach volume {volume[\u0027id\u0027]} inside the guest.\" +"},{"line_number":255,"context_line":"            \" The test will not work\")"},{"line_number":256,"context_line":""},{"line_number":257,"context_line":"        if dev_name !\u003d actual_dev:"},{"line_number":258,"context_line":"            LOG.info("}],"source_content_type":"text/x-python","patch_set":8,"id":"2c6d3a52_3c85add3","line":255,"range":{"start_line":252,"start_character":0,"end_line":255,"end_character":38},"in_reply_to":"32ee0652_320cd359","updated":"2025-01-21 14:46:05.000000000","message":"Thanks, I reverted back to a try/except and a LOG.warning at the end in order to handle this case.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8303c3daddf5c13777428c6dea5b821f79834521","unresolved":true,"context_lines":[{"line_number":258,"context_line":"            LOG.info("},{"line_number":259,"context_line":"                f\"OpenStack mapping {volume[\u0027id\u0027]} to device {dev_name}\" +"},{"line_number":260,"context_line":"                f\" is actually {actual_dev} inside the guest\")"},{"line_number":261,"context_line":"            attached_volume[\u0027attachments\u0027][0][\u0027device\u0027] \u003d f\"/dev/{actual_dev}\""},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"        return attached_volume"}],"source_content_type":"text/x-python","patch_set":8,"id":"ed824b87_ae2ca5ca","line":262,"range":{"start_line":261,"start_character":12,"end_line":262,"end_character":1},"updated":"2025-01-21 13:39:53.000000000","message":"i also do not think you shoudl be modifying the attachment as that is not what is in cidner\n\nthat could be very misleadign and hard to debug.\n\nreturning the actul name or just loging the info/debug message i think is good but i woudl expect he voluem attached_volume to remain unmodifed.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":35429,"name":"Biser Milanov","email":"biser.milanov@storpool.com","username":"sp-bmilanov"},"change_message_id":"38b1638d0668289923165094a2a86287e0cae751","unresolved":false,"context_lines":[{"line_number":258,"context_line":"            LOG.info("},{"line_number":259,"context_line":"                f\"OpenStack mapping {volume[\u0027id\u0027]} to device {dev_name}\" +"},{"line_number":260,"context_line":"                f\" is actually {actual_dev} inside the guest\")"},{"line_number":261,"context_line":"            attached_volume[\u0027attachments\u0027][0][\u0027device\u0027] \u003d f\"/dev/{actual_dev}\""},{"line_number":262,"context_line":""},{"line_number":263,"context_line":"        return attached_volume"}],"source_content_type":"text/x-python","patch_set":8,"id":"fe160eaa_3bd7863b","line":262,"range":{"start_line":261,"start_character":12,"end_line":262,"end_character":1},"in_reply_to":"ed824b87_ae2ca5ca","updated":"2025-01-21 14:46:05.000000000","message":"Makes sense, I refactored this to return and make use of a `(attached_volume, dev_name)` tuple.","commit_id":"d720e5185d51c52b5bc25196492df40aa579ba47"},{"author":{"_account_id":8556,"name":"Ghanshyam Maan","display_name":"Ghanshyam Maan","email":"gmaan.os14@gmail.com","username":"ghanshyam"},"change_message_id":"c0c3b446c7cc95f6013e4fdc41780296519ea0a4","unresolved":false,"context_lines":[{"line_number":222,"context_line":"        blks_before \u003d set(self.linux_client.exec_command("},{"line_number":223,"context_line":"            list_blks).strip().splitlines())"},{"line_number":224,"context_line":""},{"line_number":225,"context_line":"        attached_volume \u003d self.nova_volume_attach(server, volume)"},{"line_number":226,"context_line":"        # dev name volume[\u0027attachments\u0027][0][\u0027device\u0027][5:] is like"},{"line_number":227,"context_line":"        # /dev/vdb, we need to remove /dev/ -\u003e first 5 chars"},{"line_number":228,"context_line":"        dev_name \u003d attached_volume[\u0027attachments\u0027][0][\u0027device\u0027][5:]"}],"source_content_type":"text/x-python","patch_set":11,"id":"bd184416_e2301bce","line":225,"range":{"start_line":225,"start_character":0,"end_line":225,"end_character":65},"updated":"2025-01-24 03:23:32.000000000","message":"I was thinking to attach the volumes first and then update the device key in parallel but after seeing the test timing it is ok as it is now. It is not adding much time to test time compare to existing code https://zuul.opendev.org/t/openstack/build/ddf94a1f59a34734a63cceb61d307337/log/job-output.txt#34752","commit_id":"83f386cd79aa6fc2c418da5b219f843c5443b9d6"}]}