)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"54434afd1e22aae0f9d5345c03c50aa9ab7843ea","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"a55a6846_49efa414","updated":"2024-10-11 14:48:36.000000000","message":"I\u0027ve just skimmed this, but I must say, I\u0027m pleasantly optimistic after reading it. I still kinda hate that it\u0027s based on all the things I hate about this code (the giant meta-dict being one example). But, if this is all it takes to get us close, I\u0027m pretty happy.","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0d06061dd911aa15008ddab898524beb71e7d02e","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":10,"id":"a486383d_c91f1a0c","in_reply_to":"a55a6846_49efa414","updated":"2024-10-11 21:52:26.000000000","message":"I can definitely understand that. The number of different dicts and the unreliability of what\u0027s in them is maddening a lot of the time. Honestly what has taken the most time with this.","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"54434afd1e22aae0f9d5345c03c50aa9ab7843ea","unresolved":true,"context_lines":[{"line_number":5326,"context_line":"            # ever be anything other than CONF.libvirt.images_type? Maybe base"},{"line_number":5327,"context_line":"            # it on the root disk image type?"},{"line_number":5328,"context_line":"            # Currently, this is accepting an image_type kwarg which is being"},{"line_number":5329,"context_line":"            # passed in as the image_type of the root disk."},{"line_number":5330,"context_line":"            config_disk \u003d self.image_backend.by_name("},{"line_number":5331,"context_line":"                instance, name,"},{"line_number":5332,"context_line":"                self._get_disk_config_image_type(image_type\u003dimage_type))"}],"source_content_type":"text/x-python","patch_set":10,"id":"b8fec955_20d3f81c","line":5329,"updated":"2024-10-11 14:48:36.000000000","message":"IMHO, it shouldn\u0027t even be images_type. I think we have some silly cases where we store the very tiny configdrive on things like ceph, IIRC, which doesn\u0027t make a whole lot of sense. I remember ages (AGES) ago, nova had another backend driver for some company\u0027s storage system and their smallest unit of allocation was 8GiB. The configdrive was stored there and thus would waste 7.99GiB for each instance :)\n\nNova can\u0027t operate without *some* local storage for things, so I think it would be reasonable to always assume we\u0027re storing that locally. I don\u0027t know how that might affect upgrades and existing instances, and we\u0027d have to check to make sure that migration worked properly (although that\u0027s going to be a thing with this anyway).","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0d06061dd911aa15008ddab898524beb71e7d02e","unresolved":true,"context_lines":[{"line_number":5326,"context_line":"            # ever be anything other than CONF.libvirt.images_type? Maybe base"},{"line_number":5327,"context_line":"            # it on the root disk image type?"},{"line_number":5328,"context_line":"            # Currently, this is accepting an image_type kwarg which is being"},{"line_number":5329,"context_line":"            # passed in as the image_type of the root disk."},{"line_number":5330,"context_line":"            config_disk \u003d self.image_backend.by_name("},{"line_number":5331,"context_line":"                instance, name,"},{"line_number":5332,"context_line":"                self._get_disk_config_image_type(image_type\u003dimage_type))"}],"source_content_type":"text/x-python","patch_set":10,"id":"3c8bc089_f7c97dd0","line":5329,"in_reply_to":"b8fec955_20d3f81c","updated":"2024-10-11 21:52:26.000000000","message":"\u003e IMHO, it shouldn\u0027t even be images_type. I think we have some silly cases where we store the very tiny configdrive on things like ceph, IIRC, which doesn\u0027t make a whole lot of sense. I remember ages (AGES) ago, nova had another backend driver for some company\u0027s storage system and their smallest unit of allocation was 8GiB. The configdrive was stored there and thus would waste 7.99GiB for each instance :)\n\nThat\u0027s ... awesome.\n\n\u003e Nova can\u0027t operate without *some* local storage for things, so I think it would be reasonable to always assume we\u0027re storing that locally. I don\u0027t know how that might affect upgrades and existing instances, and we\u0027d have to check to make sure that migration worked properly (although that\u0027s going to be a thing with this anyway).\n\nI could see doing that. The current logic (in `_get_disk_config_image_type()`) is that config drive is on RBD if the root disk is RBD else the config drive is `raw`.","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"54434afd1e22aae0f9d5345c03c50aa9ab7843ea","unresolved":true,"context_lines":[{"line_number":5834,"context_line":"        # default inline in the method, and not in the kwarg declaration."},{"line_number":5835,"context_line":"        if image_type is None:"},{"line_number":5836,"context_line":"            image_type \u003d ("},{"line_number":5837,"context_line":"                disk_info_mapping[\u0027image_type\u0027] or CONF.libvirt.images_type)"},{"line_number":5838,"context_line":"        disk_unit \u003d None"},{"line_number":5839,"context_line":"        disk \u003d self.image_backend.by_name("},{"line_number":5840,"context_line":"            instance, name, image_type, disk_info_mapping\u003ddisk_info_mapping)"}],"source_content_type":"text/x-python","patch_set":10,"id":"4c34a6a6_0d041622","line":5837,"updated":"2024-10-11 14:48:36.000000000","message":"Now that I\u0027ve seen this a few times I\u0027m wondering - why not just default this in the mapping at boot? Is it to make sure the unspecified disks get the host\u0027s images_type after a migration type operation?\n\nFeels to me like this is begging for disk_info_mapping to be an object, where we could wrap that sort of behavior into the object itself and not have to keep doing this everywhere. I also feel like I never know what is inside disk_info_mapping and an object would help.","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"873f48ec483ea13cc4ea40142a70b80e25f32a93","unresolved":true,"context_lines":[{"line_number":5834,"context_line":"        # default inline in the method, and not in the kwarg declaration."},{"line_number":5835,"context_line":"        if image_type is None:"},{"line_number":5836,"context_line":"            image_type \u003d ("},{"line_number":5837,"context_line":"                disk_info_mapping[\u0027image_type\u0027] or CONF.libvirt.images_type)"},{"line_number":5838,"context_line":"        disk_unit \u003d None"},{"line_number":5839,"context_line":"        disk \u003d self.image_backend.by_name("},{"line_number":5840,"context_line":"            instance, name, image_type, disk_info_mapping\u003ddisk_info_mapping)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1d24dbaa_31f3cf86","line":5837,"in_reply_to":"1c57dacd_11e1516b","updated":"2024-10-14 13:38:51.000000000","message":"I think you could make these an object that is less formal than an o.vo but that lets you get a handle on things. Something that behaves like a dict but also allows attribute access to the keys. Then log every time a dict-like reference is used and whittle away all the dict refs until you get them gone, then turn off dict compatibility.\n\nIt would be a lot of work, but it might be worth doing just to get a handle on everything. I guess there\u0027s no need to send this over the wire because it\u0027s just a representation of the local config in the xml mixed with the flavor or something?","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"56aa57b4704a1658a19d48fa94318e46e2106b38","unresolved":true,"context_lines":[{"line_number":5834,"context_line":"        # default inline in the method, and not in the kwarg declaration."},{"line_number":5835,"context_line":"        if image_type is None:"},{"line_number":5836,"context_line":"            image_type \u003d ("},{"line_number":5837,"context_line":"                disk_info_mapping[\u0027image_type\u0027] or CONF.libvirt.images_type)"},{"line_number":5838,"context_line":"        disk_unit \u003d None"},{"line_number":5839,"context_line":"        disk \u003d self.image_backend.by_name("},{"line_number":5840,"context_line":"            instance, name, image_type, disk_info_mapping\u003ddisk_info_mapping)"}],"source_content_type":"text/x-python","patch_set":10,"id":"07a653b1_2e19b71c","line":5837,"in_reply_to":"1d24dbaa_31f3cf86","updated":"2024-10-15 03:43:44.000000000","message":"I agree, I think making a less formal object would be a good way to add some structure and clarity that we are missing.\n\nYeah there is no need to send over the wire because it\u0027s ultimately based on the info in the BDM records. Everything else in there is derived from that or calculated alongside (like determining the \"next disk\" or the bus).","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0d06061dd911aa15008ddab898524beb71e7d02e","unresolved":true,"context_lines":[{"line_number":5834,"context_line":"        # default inline in the method, and not in the kwarg declaration."},{"line_number":5835,"context_line":"        if image_type is None:"},{"line_number":5836,"context_line":"            image_type \u003d ("},{"line_number":5837,"context_line":"                disk_info_mapping[\u0027image_type\u0027] or CONF.libvirt.images_type)"},{"line_number":5838,"context_line":"        disk_unit \u003d None"},{"line_number":5839,"context_line":"        disk \u003d self.image_backend.by_name("},{"line_number":5840,"context_line":"            instance, name, image_type, disk_info_mapping\u003ddisk_info_mapping)"}],"source_content_type":"text/x-python","patch_set":10,"id":"1c57dacd_11e1516b","line":5837,"in_reply_to":"4c34a6a6_0d041622","updated":"2024-10-11 21:52:26.000000000","message":"Yeah, you\u0027re probably right. It has been frustrating at times with the disk_info_mapping free-for-all with me making mistakes involving \"is this thing going to be set or in the dict at all\".\n\nI have wished multiple times that it were an object so ... yeah. All of these BDM related things would be so much easier if they were objects. I think the hard part (other than replacing everything) would be figuring out what the fields should be because not all fields seem to exist all the time and just untangling all of that.","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"54434afd1e22aae0f9d5345c03c50aa9ab7843ea","unresolved":true,"context_lines":[{"line_number":12364,"context_line":"                    swap_disk \u003d self.image_backend.by_name("},{"line_number":12365,"context_line":"                        instance, name, image_type\u003dinfo[\u0027image_type\u0027])"},{"line_number":12366,"context_line":"                    swap_disk.resize_image(info[\u0027size\u0027] * units.Mi)"},{"line_number":12367,"context_line":""},{"line_number":12368,"context_line":"    def finish_revert_migration("},{"line_number":12369,"context_line":"        self,"},{"line_number":12370,"context_line":"        context: nova.context.RequestContext,"}],"source_content_type":"text/x-python","patch_set":10,"id":"861ae864_f848e4be","line":12367,"updated":"2024-10-11 14:48:36.000000000","message":"I was going to comment that \"I\u0027m sure the devil is in the details when it comes to any move operations\" but... does this mean it works or almost works?","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"56aa57b4704a1658a19d48fa94318e46e2106b38","unresolved":true,"context_lines":[{"line_number":12364,"context_line":"                    swap_disk \u003d self.image_backend.by_name("},{"line_number":12365,"context_line":"                        instance, name, image_type\u003dinfo[\u0027image_type\u0027])"},{"line_number":12366,"context_line":"                    swap_disk.resize_image(info[\u0027size\u0027] * units.Mi)"},{"line_number":12367,"context_line":""},{"line_number":12368,"context_line":"    def finish_revert_migration("},{"line_number":12369,"context_line":"        self,"},{"line_number":12370,"context_line":"        context: nova.context.RequestContext,"}],"source_content_type":"text/x-python","patch_set":10,"id":"4fb6c23e_ae8fcdb3","line":12367,"in_reply_to":"00df9e19_114eceab","updated":"2024-10-15 03:43:44.000000000","message":"Yes it is related to the different filename only.\n\nI think the issue is that the backing file name is baked into the disk file as part of the qcow format. And we always let libvirt detect the backing file [1] when creating the guest, so it picks up what\u0027s in the file that has been copied over.\n\nI\u0027m trying some things locally to find a non crappy way to deal with it. So far nothing is looking like a slam dunk.\n\n[1] https://libvirt.org/kbase/backing_chains.html#basic-disk-setup","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"0d06061dd911aa15008ddab898524beb71e7d02e","unresolved":true,"context_lines":[{"line_number":12364,"context_line":"                    swap_disk \u003d self.image_backend.by_name("},{"line_number":12365,"context_line":"                        instance, name, image_type\u003dinfo[\u0027image_type\u0027])"},{"line_number":12366,"context_line":"                    swap_disk.resize_image(info[\u0027size\u0027] * units.Mi)"},{"line_number":12367,"context_line":""},{"line_number":12368,"context_line":"    def finish_revert_migration("},{"line_number":12369,"context_line":"        self,"},{"line_number":12370,"context_line":"        context: nova.context.RequestContext,"}],"source_content_type":"text/x-python","patch_set":10,"id":"b82e8153_e93b32a9","line":12367,"in_reply_to":"861ae864_f848e4be","updated":"2024-10-11 21:52:26.000000000","message":"So far, it almost works. I\u0027ve been testing locally and began some testing with whitebox to see what happens [1] and those tests pass in my local DevStack.\n\nCurrently, they are failing in CI for NoValidHost, I think bc it doesn\u0027t have capacity for the flavor I\u0027m trying to resize up to initially. I have been making each disk (root, eph, swap) different sizes to help validate that the right disk becomes the right size if that makes sense, so I have to do old flavor root 1GB eph 2GB \u003d\u003e root 2GB eph 3GB or similar and I think the latter is too much GB. I could be wrong though.\n\nThe problem I\u0027m having is actually around the file extensions on base images in image cache change earlier in the series. In this POC, a new instance with qcow2 disk will have a raw backing file named `ac66a5ca-4ea8-4b59-b07b-03643a5687b9.gpt`, for example. That becomes a problem when you try to resize or cold migrate from a new compute to an old compute in a grenade scenario. On the destination, there will not be and can\u0027t be a base image named `ac66a5ca-4ea8-4b59-b07b-03643a5687b9.gpt` so things fail with libvirt unable to find the backing file.\n\nWeirdly, live migration works because `pre_live_migration` will copy some things including the base image to the destination up front before migrating. Maybe we could just do that with cold migrate too ... not sure if there are any additional gotchas I haven\u0027t realized yet.\n\n[1] https://review.opendev.org/c/openstack/whitebox-tempest-plugin/+/931717","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"873f48ec483ea13cc4ea40142a70b80e25f32a93","unresolved":true,"context_lines":[{"line_number":12364,"context_line":"                    swap_disk \u003d self.image_backend.by_name("},{"line_number":12365,"context_line":"                        instance, name, image_type\u003dinfo[\u0027image_type\u0027])"},{"line_number":12366,"context_line":"                    swap_disk.resize_image(info[\u0027size\u0027] * units.Mi)"},{"line_number":12367,"context_line":""},{"line_number":12368,"context_line":"    def finish_revert_migration("},{"line_number":12369,"context_line":"        self,"},{"line_number":12370,"context_line":"        context: nova.context.RequestContext,"}],"source_content_type":"text/x-python","patch_set":10,"id":"00df9e19_114eceab","line":12367,"in_reply_to":"b82e8153_e93b32a9","updated":"2024-10-14 13:38:51.000000000","message":"That\u0027s related to the different filename entirely, not just that the old version doesn\u0027t have GPT support right? But yeah, that\u0027s going to be an upgrade thing you have to fix regardless I guess. Kinda seems like whatever assumption that the file we copy during cold migration and it\u0027s named the same thing we expect ... is a bad one.","commit_id":"a5cb7e8d658ab2295fb0dccd1a6a51b2a9528c6c"}]}