)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"875e0c44894ae12a61279fb3642a85d6bc720ed7","unresolved":true,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"So, we simply take the first disk in the boot-order. The snapshot method"},{"line_number":15,"context_line":"takes that disk and checks, if it is an iso and raises a new error"},{"line_number":16,"context_line":"rejecting the operation."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #2055411"},{"line_number":19,"context_line":"Change-Id: Ib3088cfce4f7a0b24f05d45e7830b011c4a39f42"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"61bcbbc5_c9ea5ae0","line":16,"updated":"2024-08-27 16:45:35.000000000","message":"I was going to ask about what happens when the first disk is not the root disk.\n\nSo this is all about snapshot and snapshot during rescue (i.e. potentially booted from an iso) is not an issue I guess. Does booting from an iso to install not leave the iso as the first disk for longer term such that this might be the wrong assumption when we *do* want to snapshot?","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"60b312049f214501346985b8a22df7152f2c85e3","unresolved":true,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"So, we simply take the first disk in the boot-order. The snapshot method"},{"line_number":15,"context_line":"takes that disk and checks, if it is an iso and raises a new error"},{"line_number":16,"context_line":"rejecting the operation."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #2055411"},{"line_number":19,"context_line":"Change-Id: Ib3088cfce4f7a0b24f05d45e7830b011c4a39f42"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"8a780d2f_33223904","line":16,"in_reply_to":"61bcbbc5_c9ea5ae0","updated":"2024-09-02 07:51:32.000000000","message":"Yes, that commit message is a bit confusing I have changed it. The part about the snapshots and ISOs is even outdated.\n\nA more unambiguous formulation of that sentence would be:\n\u003e So, we simply take the first *hard* disk in the *default* boot-order. \n\nThe ISO gets attached as a `VirtualCdrom` (https://github.com/openstack/nova/blob/master/nova/virt/vmwareapi/vm_util.py#L854) not a `VirtualDisk`.\n\nIn that sense, while the `VirtualCdrom` still comes before the `VirtualDisk` in the default boot order, and the hypervisor will boot from it, it is not the first disk in the terminology of the VMware API. Saying first hard disk would be less ambiguous.\n\nSo, in summary, if we boot from a CD-Rom and have a root-disk and take a snapshot, the snapshot will be taken from the root-disk. That behavior won\u0027t change with this PR, only the way that root-disk is going to be found has.\n\nIf I read it correctly, the rescue path currently doesn\u0027t support ISOs for a rescue operation, but if we\u0027d implement it, it would behave the same way as the boot from ISO, and snapshotting the root-disk would still work.\n\nThe additional rescue hard-disk will be attached after the actual root-disk, so it would not be booted from by default. Another config option will be set to override the default boot order to boot from the rescue disk.\n\nI\u0027ve added a shorter explanation to the commit message as well.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc983e646132c6d338ae737f7108afaf819e5ca6","unresolved":true,"context_lines":[{"line_number":13,"context_line":""},{"line_number":14,"context_line":"So, we simply take the first disk in the boot-order. The snapshot method"},{"line_number":15,"context_line":"takes that disk and checks, if it is an iso and raises a new error"},{"line_number":16,"context_line":"rejecting the operation."},{"line_number":17,"context_line":""},{"line_number":18,"context_line":"Closes-Bug: #2055411"},{"line_number":19,"context_line":"Change-Id: Ib3088cfce4f7a0b24f05d45e7830b011c4a39f42"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"5ef46c86_ba8904af","line":16,"in_reply_to":"8a780d2f_33223904","updated":"2024-09-03 12:25:14.000000000","message":"resuce form an api stand point shoudl support all images supproted to create a vm so it should not depend on if its iso or not\nso that would be an exisitng bug in the vmware driver that can be corrected later.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"}],"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"84caa4f3d6390daeee965635e97aac3298d6dfd8","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f8fa002b_0b1580d3","updated":"2024-02-29 14:03:43.000000000","message":"i tired to skim this but i woudl need to speend more time then i currently ahve to get familar with the code to review properly.\n\ngeneral question, i belive the changes you are makeign shoduld not break anything in an upgrade context alsoth it might imply that specific versions of the vmware hypervior ro libs are requried.\n\nhave you consider the upgrade impacts when writing this and the other fix changes.","commit_id":"2665d854a4de5ade94e7dfd1c8d880882b6ef818"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"f54c3b11b3d15709ed3e557894ed8c18f472480e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":1,"id":"f05b8c18_3136688d","in_reply_to":"f8fa002b_0b1580d3","updated":"2024-02-29 14:11:01.000000000","message":"Sure, the change are independent of the vmmware (or openstack) versions.","commit_id":"2665d854a4de5ade94e7dfd1c8d880882b6ef818"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"f54c3b11b3d15709ed3e557894ed8c18f472480e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"711522c3_5b382073","updated":"2024-02-29 14:11:01.000000000","message":"I hope, that covers it.","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"6cb244cee3f63c97804ec2ba446233e76aa2f13b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":2,"id":"f2aa7eb0_2db5a4cc","updated":"2024-03-01 15:43:37.000000000","message":"I\u0027ve uploaded a new patch with a more verbose exception.","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"91db42212fe4a5de473dedf6d27a983380451d12","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":3,"id":"cd0e5824_f2fb169b","updated":"2024-03-01 16:16:24.000000000","message":"the ci longs seam to not be visable. is there anything we and point to form the ci third party ci job to show that this bug fix works.\n\nit woudl be good to see a passing reize test to confirm the bug is actully fixed before we merged this.","commit_id":"f2f2b9633fbad9c30eaa464e1848a68711016cc1"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"69f6e30779842b64e45fbaa4c0e388c9261b5d4e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"88217eb1_4d73a91c","updated":"2024-03-04 11:05:56.000000000","message":"Here is the output of a test run with the old version of the patch:\n  http://openstack-ci-logs.global.cloud.sap/sapcc-nova-ib3088cfce4f7a0b24f05d45e7830b011c4a39f42-jf2kd/index.html\n\nYou will see that the tempest run still fails, but now for another reason:\n  http://openstack-ci-logs.global.cloud.sap/sapcc-nova-ib3088cfce4f7a0b24f05d45e7830b011c4a39f42-jf2kd/tempest.html\n  \nI\u0027ve raised for that the bug: https://bugs.launchpad.net/nova/+bug/2055700 for it.\n  \n\nThe run depends on the patch in \n  https://review.opendev.org/c/openstack/nova/+/909474\nas otherwise we cannot even start an instance.","commit_id":"4ce0c19a3d1f9a993da5450a20b20fe60c3d02dc"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"5a1614c7c0d4ff3926c57c3357cb2b68fa9cf0ee","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":4,"id":"92ba590f_ed468077","updated":"2024-03-12 15:24:51.000000000","message":"sap-openstack-ci recheck - to get the automated comment in","commit_id":"4ce0c19a3d1f9a993da5450a20b20fe60c3d02dc"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"7a221f0d59e435d040c6a4f3a4f3dde6cbe7f903","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"bebb607f_30ac5d32","updated":"2024-07-02 08:38:25.000000000","message":"I\u0027ve changed the test from `not first_device` to `first_device is None`.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"8d7a57339774d05bd113604420df270f9df4af46","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"c7ddb78d_7d46d437","updated":"2024-06-25 15:49:26.000000000","message":"Short reminder about this patch.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"83eb5a615611a6a4e4036b0a6f30d6b1ff72d770","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"2da2bca6_32e7d920","updated":"2024-06-11 12:51:23.000000000","message":"Sorry for the late reply. I hope, I have addressed all concerns.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"741845eb2d9a3a851664a4d2263dc23a2bda91c7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"8e0c510d_58e06dfe","updated":"2024-06-25 17:07:22.000000000","message":"Sorry, I missed that. I hope, I have addressed your concern.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8152d5a9daba25f68ee74b4ae589b26d69b328b4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"b7386dd9_1e4c02f6","updated":"2024-06-25 18:24:21.000000000","message":"i dont think this is any more broken then the existing code.\nit does appear to work in ci so im inclined to say we shoudl proceed with this as is however there are a few style nits from a pure python perspective i wanted to highlight.\n\nmy main concern is what happens if we exit the loop without selecting a first_device\n\ni belive that would only happen if \n\nthere was no disk of class ```device.__class__.__name__ \u003d\u003d \"VirtualDisk\"```\nthat shoudl nto be supported so that should not happen.\n\nor if ther eis now backing object of class VirtualDiskFlatVer2BackingInfo\n\nthis is not handeled before so im inclined to say that a protenial preexiting bug that is not change by this patch.\n\n\nthat highlight another nit however.\nwe shoudl not really be acessing __class__.__name__  \n\ni think this is the same as doing \"isinstance(device, VirtualDisk)\"\n\nusing https://docs.python.org/3/library/functions.html#isinstance is more correct in general.\n\nthe exisitng code doe suste the private speical method but we shoudl avoid ading any addtion usage of them and remove the ones that are currently present over time.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1455c83f2ddf01a95c4a798519e5e61e72b921b2","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"6c0a6e6a_6614953a","updated":"2024-05-14 18:02:20.000000000","message":"im not sure this is really correct\n\nthis feel like it may result in the incorrect disk being selected in some cases?","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"20338c5695b6ac194897aa6e783a96800025cc9c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"6d9e7b2f_14fd6779","updated":"2024-05-14 15:53:54.000000000","message":"sap-openstack-ci recheck - lost the change request in the pipeline","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"741845eb2d9a3a851664a4d2263dc23a2bda91c7","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":5,"id":"97812cb2_cf69d1db","in_reply_to":"016e4bce_0b6120bc","updated":"2024-06-25 17:07:22.000000000","message":"The short summary: The enumeration over the API matches the one used by the BIOS to boot things and is deterministic. The image is the first disk attached and gets the lowest controller and disk number. As the root disk in nova is fixed, it will always be the case.\n\nLonger version: The BIOS will attempt to boot the disks  in the very same order as I get them from the API. Just using the vmwareapi directly, you could create situations, where you boot from the second volume (for whatever reasons). \nThat may be the case with the boot-from-volume feature and cinder, but since the implementation prior that patch doesn\u0027t work with boot-from-volume, the behavior from Nova side could be called undefined. So, no regression here if we take the first disk.\n\nNow for booting from an image in nova, that\u0027s exactly how the vmwareapi driver tells the the VM boot from the image. It is simply the first first disk it attaches. Any later attachments are simply getting a new device number, which is bigger. In nova, we have no means to detach the root-disk, so that\u0027s an invariant. Later attachments can re-use IDs and fill up slots, but that is of no concern for \"local\" disk boot.\n\nThe original authors also decided to give it a special name, but that carries no meaning for VMWare to the point of that it will change it.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"311661415ea6411f8b57aaf9465f02b0e0413032","unresolved":true,"context_lines":[],"source_content_type":"","patch_set":5,"id":"016e4bce_0b6120bc","in_reply_to":"6c0a6e6a_6614953a","updated":"2024-06-25 16:08:31.000000000","message":"this was my main concern\n\nhow confident are we that if we imply take the first disk in the boot-order it will not regress any exisitng instnaces on upgrade to a nova with this patch?\n\nis that deterministic?","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"60b312049f214501346985b8a22df7152f2c85e3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":5,"id":"e3d91d9e_e5115aab","in_reply_to":"97812cb2_cf69d1db","updated":"2024-09-02 07:51:32.000000000","message":"I assume, that can be closed. If not, please correct me.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"ede6929e98dfffe336f21fcf24e907a90a0f3944","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"94812941_42ee010d","updated":"2024-07-02 14:52:50.000000000","message":"recheck - Test failure likely due to: https://bugs.launchpad.net/tempest/+bug/1888224\n(at least it is the same error message)","commit_id":"0d3d725e52bca6b9a620fc5f130a0e94399ed98e"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"e0a94fbc631ba00444b633231becf5b602835115","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"12cc71e9_24137575","updated":"2024-07-18 07:21:07.000000000","message":"recheck - tempest-integrated-compute-rbac-old-defaults TIMED_OUT, all tests green though","commit_id":"0d3d725e52bca6b9a620fc5f130a0e94399ed98e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5c8a92808b848f94cc7433bdf626c7e179a67877","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"8fa3ebe0_d574f54d","updated":"2024-07-30 16:44:17.000000000","message":"sap-openstack-ci recheck","commit_id":"0d3d725e52bca6b9a620fc5f130a0e94399ed98e"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4a848b1c44ccde8cc98d29257e7d71a8360bc90d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":6,"id":"e86d0fbc_27b4fb73","in_reply_to":"8fa3ebe0_d574f54d","updated":"2024-07-30 16:44:46.000000000","message":"recheckign to get new results","commit_id":"0d3d725e52bca6b9a620fc5f130a0e94399ed98e"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"aab77292556a5944370b0f6f7c4fc94ad8881f54","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"f39624ab_83e95fbb","updated":"2024-08-30 13:22:31.000000000","message":"I\u0027ve added comments to the code. Hopefully it makes it clearer.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"d4c72565f46d84f6afe5a775ff5d14a7500e5638","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"dd25a5a5_b81734af","updated":"2024-08-27 15:12:56.000000000","message":"I\u0027ve fixed a regression introduced by a neutron update. We are back to two failed tests now.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"634e84bf0fb193116310d415806d2704236ca51d","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"63b3ae4e_77836548","updated":"2024-08-27 17:01:59.000000000","message":"holding my vote due to good dan\u0027s concerns","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"786ed697cd3bf834fe338461c48d57928387dbfb","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"83fa20b7_c5db5d42","updated":"2024-08-08 11:22:55.000000000","message":"looking at the tests result booting form a volume fails with this chagne\n\nhttp://openstack-ci-logs.global.cloud.sap/openstack-nova-910627-28zrc/tempest.html\n\nft9.2: setUpClass (tempest.api.compute.servers.test_create_server.ServersTestBootFromVolume)testtools.testresult.real._StringException: Traceback (most recent call last):\n  File \"/opt/stack/tempest/tempest/test.py\", line 185, in setUpClass\n    raise value.with_traceback(trace)\n  File \"/opt/stack/tempest/tempest/test.py\", line 178, in setUpClass\n    cls.resource_setup()\n  File \"/opt/stack/tempest/tempest/api/compute/servers/test_create_server.py\", line 63, in resource_setup\n    server_initial \u003d cls.create_test_server(\n  File \"/opt/stack/tempest/tempest/api/compute/base.py\", line 243, in create_test_server\n    body, servers \u003d compute.create_test_server(\n  File \"/opt/stack/tempest/tempest/common/compute.py\", line 340, in create_test_server\n    with excutils.save_and_reraise_exception():\n  File \"/opt/stack/tempest/.tox/venv/lib/python3.10/site-packages/oslo_utils/excutils.py\", line 227, in __exit__\n    self.force_reraise()\n  File \"/opt/stack/tempest/.tox/venv/lib/python3.10/site-packages/oslo_utils/excutils.py\", line 200, in force_reraise\n    raise self.value\n  File \"/opt/stack/tempest/tempest/common/compute.py\", line 323, in create_test_server\n    server \u003d waiters.wait_for_server_status(\n  File \"/opt/stack/tempest/tempest/common/waiters.py\", line 80, in wait_for_server_status\n    raise exceptions.BuildErrorException(details, server_id\u003dserver_id)\ntempest.exceptions.BuildErrorException: Server 67893efb-3acb-4bc9-85be-a2c830edb281 failed to build and is in ERROR status\nDetails: Fault: {\u0027code\u0027: 500, \u0027created\u0027: \u00272024-07-31T08:02:29Z\u0027, \u0027message\u0027: \u0027Exceeded maximum number of retries. Exhausted all hosts available for retrying build failures for instance 67893efb-3acb-4bc9-85be-a2c830edb281.\u0027}. Request ID of server operation performed before checking the server status req-6e8ade6d-658c-4765-87e4-e32fac162eb9.\n\n\nit failed because of a neutron portbinding failure\n\n[  563.922138] env[63076]: ERROR nova.compute.manager [instance: 67893efb-3acb-4bc9-85be-a2c830edb281] nova.exception.PortBindingFailed: Binding failed for port 3b16552b-0f3f-4005-a8ef-ae18ae61bafd, please check neutron logs for more information.\n\nhttp://openstack-ci-logs.global.cloud.sap/openstack-nova-910627-28zrc/index.html#file\u003dn-cpu-1.service.log\u0026highlight\u003d4715\n\nwhile that looks unrelated ideaaly we woudl want to confirm that boot form volume still work and more inporantly for this bug that resize of a boot form volume instance works.\n\nlooking at the enabeld test i dont think that is currenlty one of them","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a9ac36041a73168deb2be590d1b188532ca5ca4b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"7059d999_67dd2192","updated":"2024-08-27 16:06:04.000000000","message":"so test_rebuild_volume_backed_server is still faining in thei case are we not expecting that to pass?\n\ntempest.api.compute.servers.test_create_server.ServersTestBootFromVolume * do pass \n\nhttp://openstack-ci-logs.global.cloud.sap/openstack-nova-910627-fnfck/tempest.html\n\nso this is proably a rebuild sepcific edge case\n\ndo you have a seperate patch for that?","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"82ae0d4b0710559a53797e5908e84f2b0b5a5e0e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"aafd6150_7b4597bd","updated":"2024-08-27 16:51:50.000000000","message":"thanks for the hard work.\nI also see that that the vmware job run is not impacted by the code change (despite still failing on other cases)","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a9ac36041a73168deb2be590d1b188532ca5ca4b","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":7,"id":"3109a6ec_ae5e5fa1","in_reply_to":"83fa20b7_c5db5d42","updated":"2024-08-27 16:06:04.000000000","message":"looking at why that failed\n\nthe nsxv3 plugin is being rejected as there is no agent registered for host cpu-1\n\nso its trying to bind to ovs.\n\nhttp://openstack-ci-logs.global.cloud.sap/openstack-nova-910627-28zrc/index.html#file\u003dq-svc.service.log\u0026highlight\u003d26433","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"4e692ee67803e3e599da8ef5fd7beae60f0f6401","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"5fffc0dd_b2f2960b","updated":"2024-08-30 14:02:49.000000000","message":"+1 until dan responds to your comments","commit_id":"e794b3459134bf440537a0e683dc1bddb28f2672"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"e303c9d88a8eb7b2aac807dc3333679ac51f9a87","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"8dd2c537_2d599f24","updated":"2024-09-01 06:30:53.000000000","message":"recheck \nssh timeout; server took  longer to boot\n```\nchecking http://169.254.169.254/2009-04-04/instance-id\nfailed 1/20: up 27.40. request failed\nfailed 2/20: up 29.83. request failed\nfailed 3/20: up 32.13. request failed\nfailed 4/20: up 34.43. request failed\nfailed 5/20: up 83.73. request failed\nfailed 6/20: up 133.04. request failed```","commit_id":"7d4bda6d2593f8c8a027d6617a263995d3c0f50b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a2875f304f28d6950c30fc79e19603216cf67cc4","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":11,"id":"8623881c_9c54383e","updated":"2024-08-30 16:53:39.000000000","message":"while the logic is mostly the same as the previous patch sets i agree with dan that this is much improved via both better naming and more explicit comment to capture and express the intent\n\nthanks for bearing with us.","commit_id":"7d4bda6d2593f8c8a027d6617a263995d3c0f50b"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"60b312049f214501346985b8a22df7152f2c85e3","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"96687405_5a77c20a","updated":"2024-09-02 07:51:32.000000000","message":"I\u0027ve updated the commit message to be hopefully clearer.","commit_id":"644c58eabc2b967b5599c34ae63b33008ccd4198"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"116ec5474b6d873075fe17b958a8de9d1b74824f","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"a9d63798_675fccf1","updated":"2024-09-06 13:25:04.000000000","message":"We have two new failures.\n\nOne failure is an error from placement causing a build failure: resource provider generation conflict\nsetUpClass (tempest.api.compute.servers.test_server_rescue.ServerRescueTestJSON)\n  622939f3-3c11-4fb9-af3e-d18fef4a1e2a  \n  http://openstack-ci-logs.global.cloud.sap/openstack-nova-910627-mx6kx/index.html#file\u003dn-cpu-1.service.log\u0026highlight\u003d5652\n  \n \n The other is that an instance cannot get the metadata for tagged device. Not sure, what went wrong there.","commit_id":"644c58eabc2b967b5599c34ae63b33008ccd4198"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"46df35b4da14d17344b8e820809dc21e9fda6f03","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":12,"id":"5815d5a2_c678d8c2","updated":"2024-09-03 12:27:11.000000000","message":"there is no code change here just commit message so readding +w","commit_id":"644c58eabc2b967b5599c34ae63b33008ccd4198"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"51c3a9b4819f9f47d5e260ce8eeffaf7c1ac9754","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"9f39b3d8_c43e52c2","updated":"2024-09-18 07:02:52.000000000","message":"recheck - nova-tox-functional-py312 simply timed out while it was not yet finished (aborted during log upload)","commit_id":"6a3ca95a36f4cae7a5b73b9f5663ef2c605f8bbb"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"fcb829fb2698e25700aaef55843dcb9f6c1c6865","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"fffae3be_7568ac80","updated":"2024-09-23 09:47:59.000000000","message":"recheck cli unexpected keyword argument \u0027admin_password\u0027","commit_id":"6a3ca95a36f4cae7a5b73b9f5663ef2c605f8bbb"},{"author":{"_account_id":34860,"name":"Amit Uniyal","email":"auniyal@redhat.com","username":"auniyal"},"change_message_id":"7f4bd3b82e6de4e780aa1004d5632e0e8bfa6a56","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":13,"id":"00c2cd9a_c2f7e088","updated":"2024-10-03 02:44:59.000000000","message":"recheck unrealted fail, also this was passing in earlier checks/runs.\nfrom nova-grenade-multinod ServerActionsTestOtherB test_shelve_unshelve_server\nfailed to reach ACTIVE status and task state \"None\" within the required time (196 s)","commit_id":"6a3ca95a36f4cae7a5b73b9f5663ef2c605f8bbb"}],"nova/tests/unit/virt/vmwareapi/test_driver_api.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"1455c83f2ddf01a95c4a798519e5e61e72b921b2","unresolved":true,"context_lines":[{"line_number":1235,"context_line":"        self._iso_disk_type_created(flavor\u003d\u0027m1.micro\u0027)"},{"line_number":1236,"context_line":"        self.assertRaises(error_util.NoRootDiskDefined,"},{"line_number":1237,"context_line":"                          self.conn.snapshot, self.context, self.instance,"},{"line_number":1238,"context_line":"                          \"Test-Snapshot\", lambda *args, **kwargs: None)"},{"line_number":1239,"context_line":""},{"line_number":1240,"context_line":"    def test_snapshot_non_existent(self):"},{"line_number":1241,"context_line":"        self._create_instance()"}],"source_content_type":"text/x-python","patch_set":5,"id":"3b317a04_a55ff67e","line":1238,"updated":"2024-05-14 18:02:20.000000000","message":"you cant boot a vm without a root disk\ni either has to have a local root disk or be boot form volume\n\nis this testing boot form volume?","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"83eb5a615611a6a4e4036b0a6f30d6b1ff72d770","unresolved":true,"context_lines":[{"line_number":1235,"context_line":"        self._iso_disk_type_created(flavor\u003d\u0027m1.micro\u0027)"},{"line_number":1236,"context_line":"        self.assertRaises(error_util.NoRootDiskDefined,"},{"line_number":1237,"context_line":"                          self.conn.snapshot, self.context, self.instance,"},{"line_number":1238,"context_line":"                          \"Test-Snapshot\", lambda *args, **kwargs: None)"},{"line_number":1239,"context_line":""},{"line_number":1240,"context_line":"    def test_snapshot_non_existent(self):"},{"line_number":1241,"context_line":"        self._create_instance()"}],"source_content_type":"text/x-python","patch_set":5,"id":"e7c95f62_6478c1f8","line":1238,"in_reply_to":"3b317a04_a55ff67e","updated":"2024-06-11 12:51:23.000000000","message":"\u003e you cant boot a vm without a root disk\n\nWell, it depends on your terminology I suspect.\nThis tests when you boot an ISO from glance. I presume, the original author saw the emphasis on the root not being *disk*.\n\nNotice though, that the change is purely formatting.\n\nThere is no extra test for boot from volume, because the disks there is now no functional difference, and the code handles them the same.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"311661415ea6411f8b57aaf9465f02b0e0413032","unresolved":false,"context_lines":[{"line_number":1235,"context_line":"        self._iso_disk_type_created(flavor\u003d\u0027m1.micro\u0027)"},{"line_number":1236,"context_line":"        self.assertRaises(error_util.NoRootDiskDefined,"},{"line_number":1237,"context_line":"                          self.conn.snapshot, self.context, self.instance,"},{"line_number":1238,"context_line":"                          \"Test-Snapshot\", lambda *args, **kwargs: None)"},{"line_number":1239,"context_line":""},{"line_number":1240,"context_line":"    def test_snapshot_non_existent(self):"},{"line_number":1241,"context_line":"        self._create_instance()"}],"source_content_type":"text/x-python","patch_set":5,"id":"311ebeef_fba98323","line":1238,"in_reply_to":"a8f3ab29_acaacc82","updated":"2024-06-25 16:08:31.000000000","message":"oh good point on it just being formating we proably shoudl drop it form this change if you need to respin.\n\ni suspect this is this a bug but its not relevent to what your actully changing in this patch so we can ignore it","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"8d7a57339774d05bd113604420df270f9df4af46","unresolved":true,"context_lines":[{"line_number":1235,"context_line":"        self._iso_disk_type_created(flavor\u003d\u0027m1.micro\u0027)"},{"line_number":1236,"context_line":"        self.assertRaises(error_util.NoRootDiskDefined,"},{"line_number":1237,"context_line":"                          self.conn.snapshot, self.context, self.instance,"},{"line_number":1238,"context_line":"                          \"Test-Snapshot\", lambda *args, **kwargs: None)"},{"line_number":1239,"context_line":""},{"line_number":1240,"context_line":"    def test_snapshot_non_existent(self):"},{"line_number":1241,"context_line":"        self._create_instance()"}],"source_content_type":"text/x-python","patch_set":5,"id":"a8f3ab29_acaacc82","line":1238,"in_reply_to":"e7c95f62_6478c1f8","updated":"2024-06-25 15:49:26.000000000","message":"Short reminder.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"}],"nova/virt/vmwareapi/error_util.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"719e4bd7d60fbee5572daa361edbe02307178452","unresolved":true,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class SnapshotFromISO(exception.NovaException):"},{"line_number":29,"context_line":"    msg_fmt \u003d _(\"Snapshot from ISO.\")"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class PbmDefaultPolicyUnspecified(exception.Invalid):"}],"source_content_type":"text/x-python","patch_set":2,"id":"9bc3bed6_9e0540ad","line":29,"updated":"2024-03-01 15:15:13.000000000","message":"you proably should provide more info then SnapshotFromISO\n\nwhy is that an error? the excption messager should say.\n\nthe debug log is much more clear\n\n LOG.debug(\"Root disk is an ISO. Unable to snapshot.\",\n                          instance\u003dinstance)\n                          \ni.e. this is a problem with snapshoting the root disk because it was an iso not a volume snapshot or issue with shelve.\n\nmy inclination is to say this sound like a limitation in how vmware is handling booting form an iso  rahter then a general limit of snapshots and isos so we shoudl be a litte more verbose in the excption message.","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"83eb5a615611a6a4e4036b0a6f30d6b1ff72d770","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class SnapshotFromISO(exception.NovaException):"},{"line_number":29,"context_line":"    msg_fmt \u003d _(\"Snapshot from ISO.\")"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class PbmDefaultPolicyUnspecified(exception.Invalid):"}],"source_content_type":"text/x-python","patch_set":2,"id":"c659e1b0_a2827e1c","line":29,"in_reply_to":"17a9cb6d_6035b6bc","updated":"2024-06-11 12:51:23.000000000","message":"Done","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"6cb244cee3f63c97804ec2ba446233e76aa2f13b","unresolved":true,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class SnapshotFromISO(exception.NovaException):"},{"line_number":29,"context_line":"    msg_fmt \u003d _(\"Snapshot from ISO.\")"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class PbmDefaultPolicyUnspecified(exception.Invalid):"}],"source_content_type":"text/x-python","patch_set":2,"id":"e513521b_3c9fd507","line":29,"in_reply_to":"9bc3bed6_9e0540ad","updated":"2024-03-01 15:43:37.000000000","message":"I suspect it is actually possible to snapshot the ISO, because it is mounted as a disk in VMware, so in fact there should be nothing special there.\n\nI\u0027ve changed the message to express that hopefully more clearly.","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"91db42212fe4a5de473dedf6d27a983380451d12","unresolved":false,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class SnapshotFromISO(exception.NovaException):"},{"line_number":29,"context_line":"    msg_fmt \u003d _(\"Snapshot from ISO.\")"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class PbmDefaultPolicyUnspecified(exception.Invalid):"}],"source_content_type":"text/x-python","patch_set":2,"id":"7026d683_d5e721c8","line":29,"in_reply_to":"e513521b_3c9fd507","updated":"2024-03-01 16:16:24.000000000","message":"Acknowledged","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"69f6e30779842b64e45fbaa4c0e388c9261b5d4e","unresolved":true,"context_lines":[{"line_number":26,"context_line":""},{"line_number":27,"context_line":""},{"line_number":28,"context_line":"class SnapshotFromISO(exception.NovaException):"},{"line_number":29,"context_line":"    msg_fmt \u003d _(\"Snapshot from ISO.\")"},{"line_number":30,"context_line":""},{"line_number":31,"context_line":""},{"line_number":32,"context_line":"class PbmDefaultPolicyUnspecified(exception.Invalid):"}],"source_content_type":"text/x-python","patch_set":2,"id":"17a9cb6d_6035b6bc","line":29,"in_reply_to":"e513521b_3c9fd507","updated":"2024-03-04 11:05:56.000000000","message":"The issue lied somewhere else. The fake api implementation in the unit tests ignored the device type, and attached the ISO as a VirtualDisk, while the actual API call is attaching the ISO as a VirtualCdRom.\n\nFixing the implementation in the test to simply take the specified device fixes the test without changes to the exception handling.","commit_id":"6930fad82f029af0ae52db8467a934dfbee5cc86"}],"nova/virt/vmwareapi/vm_util.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"84caa4f3d6390daeee965635e97aac3298d6dfd8","unresolved":true,"context_lines":[{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if device.backing.__class__.__name__ \u003d\u003d \\"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\":"},{"line_number":692,"context_line":"                if not first_device \\"},{"line_number":693,"context_line":"                    or first_device.controllerKey \u003e device.controllerKey \\"},{"line_number":694,"context_line":"                    or first_device.controllerKey \u003d\u003d device.controllerKey \\"},{"line_number":695,"context_line":"                        and first_device.unitNumber \u003e device.unitNumber:"}],"source_content_type":"text/x-python","patch_set":1,"id":"b35d2ee7_82053ab2","line":692,"range":{"start_line":692,"start_character":36,"end_line":692,"end_character":37},"updated":"2024-02-29 14:03:43.000000000","message":"we try to avoid using \\\n\nplease use () instead.\n\ni know there are some usages of \\ in this file but those really shoudl be fixed at some ponit.\n\nsince the vmware dirver has littel to no maintance for the last few year its never been worth the time to clean this up but can you try to avoid addign any new usage of \\","commit_id":"2665d854a4de5ade94e7dfd1c8d880882b6ef818"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"f54c3b11b3d15709ed3e557894ed8c18f472480e","unresolved":false,"context_lines":[{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if device.backing.__class__.__name__ \u003d\u003d \\"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\":"},{"line_number":692,"context_line":"                if not first_device \\"},{"line_number":693,"context_line":"                    or first_device.controllerKey \u003e device.controllerKey \\"},{"line_number":694,"context_line":"                    or first_device.controllerKey \u003d\u003d device.controllerKey \\"},{"line_number":695,"context_line":"                        and first_device.unitNumber \u003e device.unitNumber:"}],"source_content_type":"text/x-python","patch_set":1,"id":"cded283b_7e0e42ae","line":692,"range":{"start_line":692,"start_character":36,"end_line":692,"end_character":37},"in_reply_to":"b35d2ee7_82053ab2","updated":"2024-02-29 14:11:01.000000000","message":"Done","commit_id":"2665d854a4de5ade94e7dfd1c8d880882b6ef818"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8152d5a9daba25f68ee74b4ae589b26d69b328b4","unresolved":true,"context_lines":[{"line_number":687,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":688,"context_line":"    for device in hardware_devices:"},{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"}],"source_content_type":"text/x-python","patch_set":5,"id":"a3d00645_acfb817c","line":691,"range":{"start_line":690,"start_character":16,"end_line":691,"end_character":52},"updated":"2024-06-25 18:24:21.000000000","message":"by the way can we just do an isInstance check here instead.\n\nthe code was doing device.backing.__class__.__name__ before\nbut that does not reallly mena its correct.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b1944908b5b7abd1d2f7374ff1e227f725bf89e1","unresolved":false,"context_lines":[{"line_number":687,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":688,"context_line":"    for device in hardware_devices:"},{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"}],"source_content_type":"text/x-python","patch_set":5,"id":"03a0f0ee_ed8cee17","line":691,"range":{"start_line":690,"start_character":16,"end_line":691,"end_character":52},"in_reply_to":"388e5fb2_5db6183c","updated":"2024-08-30 16:56:07.000000000","message":"Yes, I was going to say this earlier: some mass conversion of these to a `is_class_equal()` helper or something to put all the string compares in one place and make it more like `isinstance()`. I\u0027m really kinda surprised there\u0027s no some better way to do this (especially since it\u0027s trivial to create classes on the fly from the proper parent in python), but...alas.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ccc1921b7196489c0e8512ec86747a88a33f0ade","unresolved":true,"context_lines":[{"line_number":687,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":688,"context_line":"    for device in hardware_devices:"},{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"}],"source_content_type":"text/x-python","patch_set":5,"id":"8aa3ea8a_95f8c9fe","line":691,"range":{"start_line":690,"start_character":16,"end_line":691,"end_character":52},"in_reply_to":"3cc05ad9_4a3133da","updated":"2024-08-27 16:42:19.000000000","message":"Agree this is really hacky, even with the argument about the auto-generation. It was in the code before, so can\u0027t hold Fabian responsible I guess, but I definitely would want a better solution than this for new code.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"82ae0d4b0710559a53797e5908e84f2b0b5a5e0e","unresolved":true,"context_lines":[{"line_number":687,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":688,"context_line":"    for device in hardware_devices:"},{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"}],"source_content_type":"text/x-python","patch_set":5,"id":"dcb80436_debef5d4","line":691,"range":{"start_line":690,"start_character":16,"end_line":691,"end_character":52},"in_reply_to":"3cc05ad9_4a3133da","updated":"2024-08-27 16:51:50.000000000","message":"for the sake of reviewing that patch, I\u0027m happy with having the modification enveloppe to be as small as possible. I\u0027m cool with continuing to check the class name by the current mechanism and not try to optimize with any python internal methods.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"7a221f0d59e435d040c6a4f3a4f3dde6cbe7f903","unresolved":true,"context_lines":[{"line_number":687,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":688,"context_line":"    for device in hardware_devices:"},{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"}],"source_content_type":"text/x-python","patch_set":5,"id":"3cc05ad9_4a3133da","line":691,"range":{"start_line":690,"start_character":16,"end_line":691,"end_character":52},"in_reply_to":"a3d00645_acfb817c","updated":"2024-07-02 08:38:25.000000000","message":"The problem here is, that the class is dynamically generated from WSDL when the server connects, the class itself is hidden behind a factory method of the SOAP library. I am not even sure, how to get the class object for that and would have to dive into the internals of the library for that.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a2875f304f28d6950c30fc79e19603216cf67cc4","unresolved":false,"context_lines":[{"line_number":687,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":688,"context_line":"    for device in hardware_devices:"},{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"}],"source_content_type":"text/x-python","patch_set":5,"id":"388e5fb2_5db6183c","line":691,"range":{"start_line":690,"start_character":16,"end_line":691,"end_character":52},"in_reply_to":"dcb80436_debef5d4","updated":"2024-08-30 16:53:39.000000000","message":"i feel like there should be a future \"hide the ugly patch\" whwere we hide all of these comparisons behaid a function but i agree this is not in scope fo this patch so ill resovle this thread.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8152d5a9daba25f68ee74b4ae589b26d69b328b4","unresolved":true,"context_lines":[{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"}],"source_content_type":"text/x-python","patch_set":5,"id":"7b5bb4c4_0fbe0c9f","line":692,"range":{"start_line":692,"start_character":20,"end_line":692,"end_character":23},"updated":"2024-06-25 18:24:21.000000000","message":"nit: you shoudl do\n\nif (first_disk is None or\n\na boolean comparison relying on the bool(None) convertion to False shoudl be avoid when you are initialising to None","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"7a221f0d59e435d040c6a4f3a4f3dde6cbe7f903","unresolved":false,"context_lines":[{"line_number":689,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":690,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":691,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"},{"line_number":692,"context_line":"                if (not first_device or"},{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"}],"source_content_type":"text/x-python","patch_set":5,"id":"eeae19eb_c4015996","line":692,"range":{"start_line":692,"start_character":20,"end_line":692,"end_character":23},"in_reply_to":"7b5bb4c4_0fbe0c9f","updated":"2024-07-02 08:38:25.000000000","message":"Done","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8152d5a9daba25f68ee74b4ae589b26d69b328b4","unresolved":true,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"eca566e6_2a0dd594","line":705,"updated":"2024-06-25 18:24:21.000000000","message":"should we haven else here and raise an exepction if we do not have a first_device ?\n\nwe should never get here but without first_device being set os its proably not required.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ccc1921b7196489c0e8512ec86747a88a33f0ade","unresolved":true,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"d442458e_e6c7be4e","line":705,"in_reply_to":"3caa667d_926512e6","updated":"2024-08-27 16:42:19.000000000","message":"It looks to me like before we\u0027d be much more likely to have set `vmdk_device` to something by the time we dropped out of the loop, so I think sean\u0027s concern is relevant and not just refactoring. I\u0027d guess you\u0027d say that now we\u0027re in a better position to have set it because the first such device will match, but I don\u0027t know what the likelihood of a instance having no disk, or one without the backing info you\u0027re checking for. All of these values will be the defaults (like capacity being zero, etc). Under what circumstances would that be *not* an error condition?","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d5b635a60b5b6c2f1f1d6a5cd86b4d9866479b04","unresolved":false,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"e58fdc3d_e3c85adc","line":705,"in_reply_to":"5f9dbff7_e4a60ef4","updated":"2024-08-30 16:01:28.000000000","message":"\u003e https://github.com/openstack/nova/blob/cf71be0ef07d8634659bd699850658a750f91f42/nova/virt/vmwareapi/vmops.py#L992-L995\n\nThanks. I think this is very late and very disconnected from where the first issue happened, but I understand that\u0027s how the current code is.\n\n\u003e The logic makes it much more likely to find a disk. That\u0027s what the patch tries to address.\n\nOkay, it seems like it\u0027s more restrictive than what is there now and not less, but I\u0027ll take your word for it.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"e1e4ef7357c77903a3ede5f72f3805726bc6d6fb","unresolved":true,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"5f9dbff7_e4a60ef4","line":705,"in_reply_to":"9d755938_1ce8debf","updated":"2024-08-30 15:30:26.000000000","message":"That particular function is used in the image import and it creates the VM with a single disk for the import a couple of lines before https://github.com/openstack/nova/blob/master/nova/virt/vmwareapi/images.py#L344-L345.\nThe only reason you could not find a disk at that VM would be failed image import, and that itself raises an error.\n\n\nMost of the actual checks are in vmops.py\n\n- https://github.com/openstack/nova/blob/cf71be0ef07d8634659bd699850658a750f91f42/nova/virt/vmwareapi/vmops.py#L992-L995\n- https://github.com/openstack/nova/blob/cf71be0ef07d8634659bd699850658a750f91f42/nova/virt/vmwareapi/vmops.py#L1415-L1419\n- https://github.com/openstack/nova/blob/cf71be0ef07d8634659bd699850658a750f91f42/nova/virt/vmwareapi/vmops.py#L1415-L1419\n- https://github.com/openstack/nova/blob/cf71be0ef07d8634659bd699850658a750f91f42/nova/virt/vmwareapi/vmops.py#L1481-L1484\n\n- https://github.com/openstack/nova/blob/cf71be0ef07d8634659bd699850658a750f91f42/nova/virt/vmwareapi/vmops.py#L1547-L1551\n\n\n\n\u003e I think, however, the point is that if this change makes it any more likely that we find nothing, that seems like a problem.\n\nThe logic makes it much more likely to find a disk. That\u0027s what the patch tries to address.\n- One is that the disks created by cinder (i.e. boot-from-volume), where not recognized.\n- The other being that if the disks got renamed, they also wouldn\u0027t be found. I understand that is  out-of-scope for Nova. Still, that is another edge case, which is now covered.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"23d1fd3be6fe16b8e22aea4bd4f98fe193ba3004","unresolved":true,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"9d755938_1ce8debf","line":705,"in_reply_to":"a9e9dfa3_e3664617","updated":"2024-08-30 14:26:35.000000000","message":"Can you point to where that is? The couple of references I chased to this led me to thing like this:\n\nhttps://github.com/openstack/nova/blob/master/nova/virt/vmwareapi/images.py#L363\n\nWhere we take the result as success and return an element of the NamedTuple, like capacity_in_bytes. I haven\u0027t chased another level up the stack, but the above reference is a \"fetch the image\" method which surely seems like it shouldn\u0027t succeed if we didn\u0027t actually do the thing. So the caller of this, which calls the thing you modified, is checking for \u003d\u003d0 as the error condition and doing something sane? Can you point me to it for reference?\n\nI think, however, the point is that if this change makes it any more likely that we find nothing, that seems like a problem.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"aab77292556a5944370b0f6f7c4fc94ad8881f54","unresolved":true,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"a9e9dfa3_e3664617","line":705,"in_reply_to":"d442458e_e6c7be4e","updated":"2024-08-30 13:22:31.000000000","message":"It is an error condition. The point is though that where the error is checked and handled, it  error done so by checking for the default values.\n\nI am not making a case that it should stay so in general, but that I consider it out of scope for this change as it changes various places which are not related to #2055411.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"7a221f0d59e435d040c6a4f3a4f3dde6cbe7f903","unresolved":true,"context_lines":[{"line_number":702,"context_line":"        vmdk_file_path \u003d first_device.backing.fileName"},{"line_number":703,"context_line":"        capacity_in_bytes \u003d _get_device_capacity(first_device)"},{"line_number":704,"context_line":"        vmdk_controller_key \u003d first_device.controllerKey"},{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"}],"source_content_type":"text/x-python","patch_set":5,"id":"3caa667d_926512e6","line":705,"in_reply_to":"eca566e6_2a0dd594","updated":"2024-07-02 08:38:25.000000000","message":"Yes, probably it would be a cleaner solution as it is currently done. But in my mind, that is refactoring and it would make this change grow quite a bit.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"8152d5a9daba25f68ee74b4ae589b26d69b328b4","unresolved":true,"context_lines":[{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"},{"line_number":709,"context_line":"                    capacity_in_bytes, first_device)"},{"line_number":710,"context_line":""},{"line_number":711,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"1c13a257_ca0bd19f","line":708,"range":{"start_line":708,"start_character":11,"end_line":708,"end_character":19},"updated":"2024-06-25 18:24:21.000000000","message":"this is a named tuple so it will accpet none as device but im not sure if other code will handel that.\nthe exsithg logic shoudl have been grading agaisnt this anyway so its proabl yfine but i just tought i woudl raise this point.","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"7a221f0d59e435d040c6a4f3a4f3dde6cbe7f903","unresolved":false,"context_lines":[{"line_number":705,"context_line":"        disk_type \u003d _get_device_disk_type(first_device)"},{"line_number":706,"context_line":""},{"line_number":707,"context_line":"    adapter_type \u003d adapter_type_dict.get(vmdk_controller_key)"},{"line_number":708,"context_line":"    return VmdkInfo(vmdk_file_path, adapter_type, disk_type,"},{"line_number":709,"context_line":"                    capacity_in_bytes, first_device)"},{"line_number":710,"context_line":""},{"line_number":711,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"f3138412_b5a9cef9","line":708,"range":{"start_line":708,"start_character":11,"end_line":708,"end_character":19},"in_reply_to":"1c13a257_ca0bd19f","updated":"2024-07-02 08:38:25.000000000","message":"Acknowledged","commit_id":"0758850dcf1374571b3fe9ec7eb16121ec3a2993"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"ccc1921b7196489c0e8512ec86747a88a33f0ade","unresolved":true,"context_lines":[{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"},{"line_number":696,"context_line":"                    first_device \u003d device"},{"line_number":697,"context_line":"        elif device.__class__.__name__ in CONTROLLER_TO_ADAPTER_TYPE:"},{"line_number":698,"context_line":"            adapter_type_dict[device.key] \u003d CONTROLLER_TO_ADAPTER_TYPE["},{"line_number":699,"context_line":"                device.__class__.__name__]"}],"source_content_type":"text/x-python","patch_set":7,"id":"ab3ad2a5_561549e3","line":696,"updated":"2024-08-27 16:42:19.000000000","message":"Shouldn\u0027t we break here to avoid processing the rest of the loop?\n\nAlso, definitely could use some comments above this massive conditional to explain the logic.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"e1e4ef7357c77903a3ede5f72f3805726bc6d6fb","unresolved":true,"context_lines":[{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"},{"line_number":696,"context_line":"                    first_device \u003d device"},{"line_number":697,"context_line":"        elif device.__class__.__name__ in CONTROLLER_TO_ADAPTER_TYPE:"},{"line_number":698,"context_line":"            adapter_type_dict[device.key] \u003d CONTROLLER_TO_ADAPTER_TYPE["},{"line_number":699,"context_line":"                device.__class__.__name__]"}],"source_content_type":"text/x-python","patch_set":7,"id":"9b7f4fe3_87f09ce2","line":696,"in_reply_to":"2e748766_ab1b5e5e","updated":"2024-08-30 15:30:26.000000000","message":"I\u0027ve renamed it to `lowest_disk_device` and extracted the comparison to a function called `_is_before_in_boot_order`. Hopefully that makes it more concise.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d5b635a60b5b6c2f1f1d6a5cd86b4d9866479b04","unresolved":false,"context_lines":[{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"},{"line_number":696,"context_line":"                    first_device \u003d device"},{"line_number":697,"context_line":"        elif device.__class__.__name__ in CONTROLLER_TO_ADAPTER_TYPE:"},{"line_number":698,"context_line":"            adapter_type_dict[device.key] \u003d CONTROLLER_TO_ADAPTER_TYPE["},{"line_number":699,"context_line":"                device.__class__.__name__]"}],"source_content_type":"text/x-python","patch_set":7,"id":"65962517_aff54169","line":696,"in_reply_to":"9b7f4fe3_87f09ce2","updated":"2024-08-30 16:01:28.000000000","message":"Acknowledged","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":24434,"name":"Fabian Wiesel","email":"fabian.wiesel@sap.com","username":"fwiesel"},"change_message_id":"aab77292556a5944370b0f6f7c4fc94ad8881f54","unresolved":true,"context_lines":[{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"},{"line_number":696,"context_line":"                    first_device \u003d device"},{"line_number":697,"context_line":"        elif device.__class__.__name__ in CONTROLLER_TO_ADAPTER_TYPE:"},{"line_number":698,"context_line":"            adapter_type_dict[device.key] \u003d CONTROLLER_TO_ADAPTER_TYPE["},{"line_number":699,"context_line":"                device.__class__.__name__]"}],"source_content_type":"text/x-python","patch_set":7,"id":"e75b89f3_76496739","line":696,"in_reply_to":"ab3ad2a5_561549e3","updated":"2024-08-30 13:22:31.000000000","message":"\u003e Also, definitely could use some comments above this massive conditional to explain the logic.\n\nI\u0027ve added it and explained the logic there. But to make it short, it is the first device as defined in the set of conditionals, not the first device as defined by the consumed iterator. Only after having iterated over all devices, we know which one is actually the first in that order.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"23d1fd3be6fe16b8e22aea4bd4f98fe193ba3004","unresolved":true,"context_lines":[{"line_number":693,"context_line":"                    first_device.controllerKey \u003e device.controllerKey or"},{"line_number":694,"context_line":"                    first_device.controllerKey \u003d\u003d device.controllerKey and"},{"line_number":695,"context_line":"                        first_device.unitNumber \u003e device.unitNumber):"},{"line_number":696,"context_line":"                    first_device \u003d device"},{"line_number":697,"context_line":"        elif device.__class__.__name__ in CONTROLLER_TO_ADAPTER_TYPE:"},{"line_number":698,"context_line":"            adapter_type_dict[device.key] \u003d CONTROLLER_TO_ADAPTER_TYPE["},{"line_number":699,"context_line":"                device.__class__.__name__]"}],"source_content_type":"text/x-python","patch_set":7,"id":"2e748766_ab1b5e5e","line":696,"in_reply_to":"e75b89f3_76496739","updated":"2024-08-30 14:26:35.000000000","message":"Okay I got it, once you\u0027ve found *a* `first_device` you have to keep going because you may find something with a lower \"score\". IMHO, it would be better to rename the variable to make it more clear what\u0027s going on (i.e. \"lower\" or \"lowest\" or \"min\" instead of \"first\"). I also don\u0027t think that\u0027s really clear enough in the comments, and someone could easily come along and think they\u0027re making a performance improvement by adding the break.","commit_id":"6f8d7661241df6d8fe5f99d8a1c0ea253a5d4f3c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6c891baf0c50339747724ab747dc2e1b0fe44955","unresolved":true,"context_lines":[{"line_number":701,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":702,"context_line":"    for device in hardware_devices:"},{"line_number":703,"context_line":"        # For the disk-devices: Check if the current disk-device is before the"},{"line_number":704,"context_line":"        #    lowest_disk_device in the boot order"},{"line_number":705,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":706,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":707,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"}],"source_content_type":"text/x-python","patch_set":11,"id":"465ccf34_80c24db1","line":704,"updated":"2024-08-30 15:17:12.000000000","message":"I think this (the rename, and breaking the logic into a helper) substantially changes (for the better) the mental picture of what you\u0027re trying to achieve here, thanks.","commit_id":"7d4bda6d2593f8c8a027d6617a263995d3c0f50b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d5b635a60b5b6c2f1f1d6a5cd86b4d9866479b04","unresolved":false,"context_lines":[{"line_number":701,"context_line":"    adapter_type_dict \u003d {}"},{"line_number":702,"context_line":"    for device in hardware_devices:"},{"line_number":703,"context_line":"        # For the disk-devices: Check if the current disk-device is before the"},{"line_number":704,"context_line":"        #    lowest_disk_device in the boot order"},{"line_number":705,"context_line":"        if device.__class__.__name__ \u003d\u003d \"VirtualDisk\":"},{"line_number":706,"context_line":"            if (device.backing.__class__.__name__ \u003d\u003d"},{"line_number":707,"context_line":"                    \"VirtualDiskFlatVer2BackingInfo\"):"}],"source_content_type":"text/x-python","patch_set":11,"id":"b292429a_50e34215","line":704,"in_reply_to":"465ccf34_80c24db1","updated":"2024-08-30 16:01:28.000000000","message":"Done","commit_id":"7d4bda6d2593f8c8a027d6617a263995d3c0f50b"}]}