)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"f292460bd07de489a78dcf361771a20f4947e163","unresolved":false,"context_lines":[{"line_number":10,"context_line":"MoveClaim object that, in subsequent patches, will be used during live"},{"line_number":11,"context_line":"migrations to claim resources on the destination."},{"line_number":12,"context_line":""},{"line_number":13,"context_line":"Implements blueprint bp/numa-aware-live-migration"},{"line_number":14,"context_line":"Change-Id: I3bafebe86991f457569efcd7b6f130c5a0080437"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":24,"id":"5fc1f717_d8e7c9ab","line":13,"range":{"start_line":13,"start_character":21,"end_line":13,"end_character":24},"updated":"2019-03-26 21:42:44.000000000","message":"strike this so it links properly","commit_id":"6981ce2b4561972b29535e6191a991148de85727"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"53f7de1b3b259d9c7e012d30bf308cce8c158dc9","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Artom Lifshitz \u003califshit@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2019-07-03 09:00:55 +0300"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[WIP] Introduce live_migration_claim()"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Add live_migration_claim() to the resource tracker. It returns a"},{"line_number":10,"context_line":"MoveClaim object that, in subsequent patches, will be used during live"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":26,"id":"7faddb67_d2ef51ae","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":6},"updated":"2019-07-04 10:43:18.000000000","message":"Why is this still WIP? Any chance you could note it here to set (my) expectations? :)","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"d823c26bba0ebcc8d65a3b025f48659221711541","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Artom Lifshitz \u003califshit@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2019-07-03 09:00:55 +0300"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"[WIP] Introduce live_migration_claim()"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Add live_migration_claim() to the resource tracker. It returns a"},{"line_number":10,"context_line":"MoveClaim object that, in subsequent patches, will be used during live"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":26,"id":"7faddb67_3494928b","line":7,"range":{"start_line":7,"start_character":0,"end_line":7,"end_character":6},"in_reply_to":"7faddb67_d2ef51ae","updated":"2019-07-04 11:21:57.000000000","message":"WIP until the whole series is ready. Currently the new instance_numa_topology isn\u0027t being saved correctly. This was pointed out by Windriver at the end of Stein.","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"}],"nova/compute/claims.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"05664cddf98b325c1149d218c7076ae634e2ccc4","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        # migration and the instance has hugepages. In those cases, it\u0027s the"},{"line_number":271,"context_line":"        # instance\u0027s actual page size. If the hw:mem_page_size flavor extra"},{"line_number":272,"context_line":"        # spec is `large` or `any`, the instance could move from a host with 1G"},{"line_number":273,"context_line":"        # pages to a host with 2M pages, for example. When an instance is"},{"line_number":274,"context_line":"        # running, changing its pagesize is only supported on x86 - and even"},{"line_number":275,"context_line":"        # then, post-copy live migration requires the same pagesize on source"},{"line_number":276,"context_line":"        # and dest. For this reason, the page size is passed to the claim so"},{"line_number":277,"context_line":"        # that it can reject hosts that don\u0027t support the instance\u0027s current"},{"line_number":278,"context_line":"        # page size."},{"line_number":279,"context_line":"        self.context \u003d context"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_e1c57f0b","line":276,"range":{"start_line":273,"start_character":54,"end_line":276,"end_character":20},"updated":"2019-02-24 22:23:51.000000000","message":"This is uncomfortable libvirt-specific for non-virt driver specific code. I guess huge pages are only supported by the libvirt driver at this time? And for the x86 comment - I\u0027m assuming that is dependent on a specific version of qemu/libvirt but might not be true in the future for other architectures.","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"6bcc26bfb10b852665fb8944ea64fd7a5fb2767f","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        # migration and the instance has hugepages. In those cases, it\u0027s the"},{"line_number":271,"context_line":"        # instance\u0027s actual page size. If the hw:mem_page_size flavor extra"},{"line_number":272,"context_line":"        # spec is `large` or `any`, the instance could move from a host with 1G"},{"line_number":273,"context_line":"        # pages to a host with 2M pages, for example. When an instance is"},{"line_number":274,"context_line":"        # running, changing its pagesize is only supported on x86 - and even"},{"line_number":275,"context_line":"        # then, post-copy live migration requires the same pagesize on source"},{"line_number":276,"context_line":"        # and dest. For this reason, the page size is passed to the claim so"},{"line_number":277,"context_line":"        # that it can reject hosts that don\u0027t support the instance\u0027s current"},{"line_number":278,"context_line":"        # page size."},{"line_number":279,"context_line":"        self.context \u003d context"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_94d702c0","line":276,"range":{"start_line":273,"start_character":54,"end_line":276,"end_character":20},"in_reply_to":"9fdfeff1_147b128c","updated":"2019-02-25 15:40:15.000000000","message":"\u003cdanpb\u003e artom: the non-x86  restriction isn\u0027t specific to libvirt\n\u003cdanpb\u003e artom: its simply the case that certain architecture won\u0027t support it\n\nand\n\n\u003cdanpb\u003e artom: ok, post-copy restriction could be relaxed in future if anyone cares enough todo the work","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"5dd5ca753c8720800c58e8f144ada59ffada1eac","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        # migration and the instance has hugepages. In those cases, it\u0027s the"},{"line_number":271,"context_line":"        # instance\u0027s actual page size. If the hw:mem_page_size flavor extra"},{"line_number":272,"context_line":"        # spec is `large` or `any`, the instance could move from a host with 1G"},{"line_number":273,"context_line":"        # pages to a host with 2M pages, for example. When an instance is"},{"line_number":274,"context_line":"        # running, changing its pagesize is only supported on x86 - and even"},{"line_number":275,"context_line":"        # then, post-copy live migration requires the same pagesize on source"},{"line_number":276,"context_line":"        # and dest. For this reason, the page size is passed to the claim so"},{"line_number":277,"context_line":"        # that it can reject hosts that don\u0027t support the instance\u0027s current"},{"line_number":278,"context_line":"        # page size."},{"line_number":279,"context_line":"        self.context \u003d context"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_b1aecee4","line":276,"range":{"start_line":273,"start_character":54,"end_line":276,"end_character":20},"in_reply_to":"9fdfeff1_71186675","updated":"2019-02-25 21:24:11.000000000","message":"We don\u0027t even need the driver, really. It\u0027s stored in instance_numa_topology, so we could just move the DB call rom [1] into @property numa_topology and remove `pagesize` from all the __init__ paths.\n\n[1] https://review.openstack.org/#/c/634606/29/nova/compute/manager.py@6106","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"138257d271e65c49bdc855fbec0cea35afc333b2","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        # migration and the instance has hugepages. In those cases, it\u0027s the"},{"line_number":271,"context_line":"        # instance\u0027s actual page size. If the hw:mem_page_size flavor extra"},{"line_number":272,"context_line":"        # spec is `large` or `any`, the instance could move from a host with 1G"},{"line_number":273,"context_line":"        # pages to a host with 2M pages, for example. When an instance is"},{"line_number":274,"context_line":"        # running, changing its pagesize is only supported on x86 - and even"},{"line_number":275,"context_line":"        # then, post-copy live migration requires the same pagesize on source"},{"line_number":276,"context_line":"        # and dest. For this reason, the page size is passed to the claim so"},{"line_number":277,"context_line":"        # that it can reject hosts that don\u0027t support the instance\u0027s current"},{"line_number":278,"context_line":"        # page size."},{"line_number":279,"context_line":"        self.context \u003d context"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_71186675","line":276,"range":{"start_line":273,"start_character":54,"end_line":276,"end_character":20},"in_reply_to":"9fdfeff1_94d702c0","updated":"2019-02-25 21:17:18.000000000","message":"This might be crazy, but the Claim object gets a handle to the ResourceTracker which has a handle to the ComputeDriver so if we really wanted, we could call a function on self.tracker.driver.get_pagesize() or something if we need virt-driver specific information for a claim, rather than pass in a pagesize and assume it\u0027s kosher for all virt drivers. Similar to how the overhead calculation works (the ResourceTracker calls a method on the virt driver to pass that to the claim).","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"9fe6847e64c736ab2703356e0b50cf77a60b8559","unresolved":false,"context_lines":[{"line_number":270,"context_line":"        # migration and the instance has hugepages. In those cases, it\u0027s the"},{"line_number":271,"context_line":"        # instance\u0027s actual page size. If the hw:mem_page_size flavor extra"},{"line_number":272,"context_line":"        # spec is `large` or `any`, the instance could move from a host with 1G"},{"line_number":273,"context_line":"        # pages to a host with 2M pages, for example. When an instance is"},{"line_number":274,"context_line":"        # running, changing its pagesize is only supported on x86 - and even"},{"line_number":275,"context_line":"        # then, post-copy live migration requires the same pagesize on source"},{"line_number":276,"context_line":"        # and dest. For this reason, the page size is passed to the claim so"},{"line_number":277,"context_line":"        # that it can reject hosts that don\u0027t support the instance\u0027s current"},{"line_number":278,"context_line":"        # page size."},{"line_number":279,"context_line":"        self.context \u003d context"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_147b128c","line":276,"range":{"start_line":273,"start_character":54,"end_line":276,"end_character":20},"in_reply_to":"9fdfeff1_e1c57f0b","updated":"2019-02-25 15:28:52.000000000","message":"\u003e This is uncomfortable libvirt-specific for non-virt driver specific\n \u003e code.\n\nYeah, it\u0027s ugly. I don\u0027t see a way around it, however :( In order to properly fit the instance on the host, the claim needs to know the real numeric pagesize, not just `large` or `any`.\n\n \u003e I guess huge pages are only supported by the libvirt driver\n \u003e at this time?\n\nI think you\u0027re right:\n\n  [artom@zoe nova]$ grep -l page -R nova/virt/ | grep -v hardware\n  nova/virt/hyperv/hostops.py\n  nova/virt/libvirt/guest.py\n  nova/virt/libvirt/config.py\n  nova/virt/libvirt/driver.py\n  nova/virt/libvirt/volume/vzstorage.py\n  nova/virt/libvirt/migration.py\n\nAnd the hyperv stuff is compression_pages, whatever that is.\n\n \u003e And for the x86 comment - I\u0027m assuming that is\n \u003e dependent on a specific version of qemu/libvirt but might not be\n \u003e true in the future for other architectures.\n\nWe should ask Dan Berrange - he\u0027s the one who explained the pagesize live migration limitations to me.","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"05664cddf98b325c1149d218c7076ae634e2ccc4","unresolved":false,"context_lines":[{"line_number":325,"context_line":"        self.instance.drop_migration_context()"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"    def __str__(self):"},{"line_number":328,"context_line":"        return (\"[Claim: %d MB memory, %d GB disk, \""},{"line_number":329,"context_line":"                \"%s topology]\" % (self.memory_mb, self.disk_gb,"},{"line_number":330,"context_line":"                                  self.claimed_numa_topology))"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_01c9a323","line":328,"updated":"2019-02-24 22:23:51.000000000","message":"Why isn\u0027t vcpus in here?","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"9fe6847e64c736ab2703356e0b50cf77a60b8559","unresolved":false,"context_lines":[{"line_number":325,"context_line":"        self.instance.drop_migration_context()"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"    def __str__(self):"},{"line_number":328,"context_line":"        return (\"[Claim: %d MB memory, %d GB disk, \""},{"line_number":329,"context_line":"                \"%s topology]\" % (self.memory_mb, self.disk_gb,"},{"line_number":330,"context_line":"                                  self.claimed_numa_topology))"}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_219147f2","line":328,"in_reply_to":"9fdfeff1_01c9a323","updated":"2019-02-25 15:28:52.000000000","message":"I copy-pasted the existing L60 method and added the NUMA topology. I guess the only strong opinion I have is for all implementation of __str__ to be consistent, so we should either add CPUs to both, or leave it out here.","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"d823c26bba0ebcc8d65a3b025f48659221711541","unresolved":false,"context_lines":[{"line_number":57,"context_line":"    def abort(self):"},{"line_number":58,"context_line":"        pass"},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def __str__(self):"},{"line_number":61,"context_line":"        return \"[Claim: %d MB memory, %d GB disk]\" % (self.memory_mb,"},{"line_number":62,"context_line":"                self.disk_gb)"},{"line_number":63,"context_line":""}],"source_content_type":"text/x-python","patch_set":26,"id":"7faddb67_942d4648","line":60,"updated":"2019-07-04 11:21:57.000000000","message":"Override of this.","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"53f7de1b3b259d9c7e012d30bf308cce8c158dc9","unresolved":false,"context_lines":[{"line_number":300,"context_line":"    def numa_topology(self):"},{"line_number":301,"context_line":"        numa_topology \u003d hardware.numa_get_constraints(self.instance_type,"},{"line_number":302,"context_line":"                                                      self.image_meta)"},{"line_number":303,"context_line":"        # NOTE(artom) If we\u0027re live migrating an instance with a NUMA topology"},{"line_number":304,"context_line":"        # using the libvirt driver, the pagesize needs to remain the"},{"line_number":305,"context_line":"        # same as on the source. x86 is the only architecture to support"},{"line_number":306,"context_line":"        # changing the pagesize with the instance running, and even then"},{"line_number":307,"context_line":"        # post-copy live migration requires a constant pagesize. Override what"},{"line_number":308,"context_line":"        # numa_get_constraints() gave us with the page size from"},{"line_number":309,"context_line":"        # instance.numa_topology."},{"line_number":310,"context_line":"        if (numa_topology and self.migration and"},{"line_number":311,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":312,"context_line":"            for cell in numa_topology.cells:"}],"source_content_type":"text/x-python","patch_set":26,"id":"7faddb67_b2c41542","line":309,"range":{"start_line":303,"start_character":0,"end_line":309,"end_character":33},"updated":"2019-07-04 10:43:18.000000000","message":"How can this change for live migration, given the image metadata and flavor can\u0027t change? Maybe add the explanation here","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"d823c26bba0ebcc8d65a3b025f48659221711541","unresolved":false,"context_lines":[{"line_number":300,"context_line":"    def numa_topology(self):"},{"line_number":301,"context_line":"        numa_topology \u003d hardware.numa_get_constraints(self.instance_type,"},{"line_number":302,"context_line":"                                                      self.image_meta)"},{"line_number":303,"context_line":"        # NOTE(artom) If we\u0027re live migrating an instance with a NUMA topology"},{"line_number":304,"context_line":"        # using the libvirt driver, the pagesize needs to remain the"},{"line_number":305,"context_line":"        # same as on the source. x86 is the only architecture to support"},{"line_number":306,"context_line":"        # changing the pagesize with the instance running, and even then"},{"line_number":307,"context_line":"        # post-copy live migration requires a constant pagesize. Override what"},{"line_number":308,"context_line":"        # numa_get_constraints() gave us with the page size from"},{"line_number":309,"context_line":"        # instance.numa_topology."},{"line_number":310,"context_line":"        if (numa_topology and self.migration and"},{"line_number":311,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":312,"context_line":"            for cell in numa_topology.cells:"}],"source_content_type":"text/x-python","patch_set":26,"id":"7faddb67_d4be5efe","line":309,"range":{"start_line":303,"start_character":0,"end_line":309,"end_character":33},"in_reply_to":"7faddb67_b2c41542","updated":"2019-07-04 11:21:57.000000000","message":"Done","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"53f7de1b3b259d9c7e012d30bf308cce8c158dc9","unresolved":false,"context_lines":[{"line_number":324,"context_line":"            instance_type\u003dself.instance_type)"},{"line_number":325,"context_line":"        self.instance.drop_migration_context()"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"    def __str__(self):"},{"line_number":328,"context_line":"        return (\"[Claim: %d MB memory, %d GB disk, \""},{"line_number":329,"context_line":"                \"%s topology]\" % (self.memory_mb, self.disk_gb,"},{"line_number":330,"context_line":"                                  self.claimed_numa_topology))"}],"source_content_type":"text/x-python","patch_set":26,"id":"7faddb67_b2d97523","line":330,"range":{"start_line":327,"start_character":0,"end_line":330,"end_character":62},"updated":"2019-07-04 10:43:18.000000000","message":"Does this not happen automatically? Also, you probably should be overriding repr and not str [1] and describe the actual object and field names. Something like:\n\n    return \u0027\u003cMoveClaim memory_mb\u003d%d, disk_gb\u003d%r, claimed_numa_topology\u003d%r\u003e\u0027 % (...)\n\n[1] https://stackoverflow.com/questions/1436703/difference-between-str-and-repr","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"d823c26bba0ebcc8d65a3b025f48659221711541","unresolved":false,"context_lines":[{"line_number":324,"context_line":"            instance_type\u003dself.instance_type)"},{"line_number":325,"context_line":"        self.instance.drop_migration_context()"},{"line_number":326,"context_line":""},{"line_number":327,"context_line":"    def __str__(self):"},{"line_number":328,"context_line":"        return (\"[Claim: %d MB memory, %d GB disk, \""},{"line_number":329,"context_line":"                \"%s topology]\" % (self.memory_mb, self.disk_gb,"},{"line_number":330,"context_line":"                                  self.claimed_numa_topology))"}],"source_content_type":"text/x-python","patch_set":26,"id":"7faddb67_742c4a47","line":330,"range":{"start_line":327,"start_character":0,"end_line":330,"end_character":62},"in_reply_to":"7faddb67_b2d97523","updated":"2019-07-04 11:21:57.000000000","message":"This exists only because __str__() on L60 needs to be overridden, as it only outputs memory_mb and disk_gb.","commit_id":"fab84ed1a9fdef0bfab174d18e1627214ef7b335"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6387281ecadddb5c37b0d046388e9a02e0d07570","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        self._claim_test(resources, limits)"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    @property"},{"line_number":76,"context_line":"    def numa_topology(self):"},{"line_number":77,"context_line":"        if self._numa_topology_loaded:"},{"line_number":78,"context_line":"            return self._numa_topology"},{"line_number":79,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_9306dc85","line":76,"updated":"2019-08-21 16:35:50.000000000","message":"Re-reading this, I think it would be better to not override this property in MoveClaim. Instead, change L80 to call a helper method, which would grab this from the instance in the base class, and do your other stuff in MoveClaim. Them we get the caching here and avoid the discrepancy between the base class and MoveClaim in that self._numa_topology_loaded will never be True, and self._numa_topology will never be set.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2d4dc7eea0a9bd78617722cf0548045380210f1","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        self._claim_test(resources, limits)"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    @property"},{"line_number":76,"context_line":"    def numa_topology(self):"},{"line_number":77,"context_line":"        if self._numa_topology_loaded:"},{"line_number":78,"context_line":"            return self._numa_topology"},{"line_number":79,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_d64be2ad","line":76,"in_reply_to":"7faddb67_9306dc85","updated":"2019-08-21 17:26:46.000000000","message":"So in the new patch set that I will eventually push, the overriding won\u0027t be necessary because the live migration will get refused if the source and dest have different page sizes.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a96d77a14bcdbdb2e57940aa065caef9b4b58c36","unresolved":false,"context_lines":[{"line_number":73,"context_line":"        self._claim_test(resources, limits)"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":"    @property"},{"line_number":76,"context_line":"    def numa_topology(self):"},{"line_number":77,"context_line":"        if self._numa_topology_loaded:"},{"line_number":78,"context_line":"            return self._numa_topology"},{"line_number":79,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_5905e50e","line":76,"in_reply_to":"7faddb67_d64be2ad","updated":"2019-08-21 17:37:05.000000000","message":"I like it :)","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":166,"context_line":"    \"\"\""},{"line_number":167,"context_line":"    def __init__(self, context, instance, nodename, instance_type, image_meta,"},{"line_number":168,"context_line":"                 tracker, resources, pci_requests, limits\u003dNone,"},{"line_number":169,"context_line":"                 migration\u003dNone):"},{"line_number":170,"context_line":"        self.context \u003d context"},{"line_number":171,"context_line":"        self.instance_type \u003d instance_type"},{"line_number":172,"context_line":"        if isinstance(image_meta, dict):"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_1a6806f2","line":169,"range":{"start_line":169,"start_character":17,"end_line":169,"end_character":31},"updated":"2019-08-16 17:53:53.000000000","message":"Seems like this probably shouldn\u0027t be optional right? I would expect us to be able to always pass in a migration for any operation where we\u0027re doing a move claim. We might only _use_ it for live migration right now, but I think it\u0027ll be less confusing in the future if this is only set/stored for the live case.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2d4dc7eea0a9bd78617722cf0548045380210f1","unresolved":false,"context_lines":[{"line_number":166,"context_line":"    \"\"\""},{"line_number":167,"context_line":"    def __init__(self, context, instance, nodename, instance_type, image_meta,"},{"line_number":168,"context_line":"                 tracker, resources, pci_requests, limits\u003dNone,"},{"line_number":169,"context_line":"                 migration\u003dNone):"},{"line_number":170,"context_line":"        self.context \u003d context"},{"line_number":171,"context_line":"        self.instance_type \u003d instance_type"},{"line_number":172,"context_line":"        if isinstance(image_meta, dict):"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_e40bf6d1","line":169,"range":{"start_line":169,"start_character":17,"end_line":169,"end_character":31},"in_reply_to":"7faddb67_1a6806f2","updated":"2019-08-21 17:26:46.000000000","message":"Done","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":183,"context_line":"                                                      self.image_meta)"},{"line_number":184,"context_line":"        # NOTE(artom) If we\u0027re live migrating an instance with a NUMA topology"},{"line_number":185,"context_line":"        # using the libvirt driver, the pagesize needs to remain the"},{"line_number":186,"context_line":"        # same as on the source. X86 (and only x86) does support"},{"line_number":187,"context_line":"        # changing the pagesize with the instance running, but post-copy live"},{"line_number":188,"context_line":"        # migration requires a constant pagesize. While the image metadata and"},{"line_number":189,"context_line":"        # flavor themselves cannot change, if hw:mem_page_size is \u0027large\u0027, the"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_1a41e663","line":186,"range":{"start_line":186,"start_character":33,"end_line":186,"end_character":36},"updated":"2019-08-16 17:53:53.000000000","message":"I guess this is capitalized because it starts a sentence, but it looks weird for this to be capitalized (at all, and) because it\u0027s followed by an uncapitalized one so close. Can you just restructure the sentence so it starts with something else?\n\nYes, I know, I have problems.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2d4dc7eea0a9bd78617722cf0548045380210f1","unresolved":false,"context_lines":[{"line_number":183,"context_line":"                                                      self.image_meta)"},{"line_number":184,"context_line":"        # NOTE(artom) If we\u0027re live migrating an instance with a NUMA topology"},{"line_number":185,"context_line":"        # using the libvirt driver, the pagesize needs to remain the"},{"line_number":186,"context_line":"        # same as on the source. X86 (and only x86) does support"},{"line_number":187,"context_line":"        # changing the pagesize with the instance running, but post-copy live"},{"line_number":188,"context_line":"        # migration requires a constant pagesize. While the image metadata and"},{"line_number":189,"context_line":"        # flavor themselves cannot change, if hw:mem_page_size is \u0027large\u0027, the"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_6474e636","line":186,"range":{"start_line":186,"start_character":33,"end_line":186,"end_character":36},"in_reply_to":"7faddb67_1a41e663","updated":"2019-08-21 17:26:46.000000000","message":"*suspicious Fry face*\n\nOk, done.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":184,"context_line":"        # NOTE(artom) If we\u0027re live migrating an instance with a NUMA topology"},{"line_number":185,"context_line":"        # using the libvirt driver, the pagesize needs to remain the"},{"line_number":186,"context_line":"        # same as on the source. X86 (and only x86) does support"},{"line_number":187,"context_line":"        # changing the pagesize with the instance running, but post-copy live"},{"line_number":188,"context_line":"        # migration requires a constant pagesize. While the image metadata and"},{"line_number":189,"context_line":"        # flavor themselves cannot change, if hw:mem_page_size is \u0027large\u0027, the"},{"line_number":190,"context_line":"        # destination host could calculate a pagesize different from the"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_ba51b232","line":187,"range":{"start_line":187,"start_character":10,"end_line":187,"end_character":57},"updated":"2019-08-16 17:53:53.000000000","message":"When you say x86, I think you mean x86_32. Does x86_64 *not* support changing the page size?","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        # same as on the source. X86 (and only x86) does support"},{"line_number":187,"context_line":"        # changing the pagesize with the instance running, but post-copy live"},{"line_number":188,"context_line":"        # migration requires a constant pagesize. While the image metadata and"},{"line_number":189,"context_line":"        # flavor themselves cannot change, if hw:mem_page_size is \u0027large\u0027, the"},{"line_number":190,"context_line":"        # destination host could calculate a pagesize different from the"},{"line_number":191,"context_line":"        # source. Override what numa_get_constraints() gave us with the page"},{"line_number":192,"context_line":"        # size from instance.numa_topology."},{"line_number":193,"context_line":"        if (numa_topology and self.migration and"},{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_da234ebd","line":191,"range":{"start_line":189,"start_character":43,"end_line":191,"end_character":16},"updated":"2019-08-16 17:53:53.000000000","message":"Presumably we could also just refuse to do such a migration, right? On the one hand, fewer restrictions are better, but on the other...if I migrate and then reboot the instance, it will probably get a different page size (if, as you say, large means something else on the new host)...","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a96d77a14bcdbdb2e57940aa065caef9b4b58c36","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        # same as on the source. X86 (and only x86) does support"},{"line_number":187,"context_line":"        # changing the pagesize with the instance running, but post-copy live"},{"line_number":188,"context_line":"        # migration requires a constant pagesize. While the image metadata and"},{"line_number":189,"context_line":"        # flavor themselves cannot change, if hw:mem_page_size is \u0027large\u0027, the"},{"line_number":190,"context_line":"        # destination host could calculate a pagesize different from the"},{"line_number":191,"context_line":"        # source. Override what numa_get_constraints() gave us with the page"},{"line_number":192,"context_line":"        # size from instance.numa_topology."},{"line_number":193,"context_line":"        if (numa_topology and self.migration and"},{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_390ee931","line":191,"range":{"start_line":189,"start_character":43,"end_line":191,"end_character":16},"in_reply_to":"7faddb67_c4235a04","updated":"2019-08-21 17:37:05.000000000","message":"Yep, and also, in the future I think we could likely allow it if migrating to a new-enough compute which has some sort of logic to make a good decision, since the claim and check would happen on the dest.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2d4dc7eea0a9bd78617722cf0548045380210f1","unresolved":false,"context_lines":[{"line_number":186,"context_line":"        # same as on the source. X86 (and only x86) does support"},{"line_number":187,"context_line":"        # changing the pagesize with the instance running, but post-copy live"},{"line_number":188,"context_line":"        # migration requires a constant pagesize. While the image metadata and"},{"line_number":189,"context_line":"        # flavor themselves cannot change, if hw:mem_page_size is \u0027large\u0027, the"},{"line_number":190,"context_line":"        # destination host could calculate a pagesize different from the"},{"line_number":191,"context_line":"        # source. Override what numa_get_constraints() gave us with the page"},{"line_number":192,"context_line":"        # size from instance.numa_topology."},{"line_number":193,"context_line":"        if (numa_topology and self.migration and"},{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_c4235a04","line":191,"range":{"start_line":189,"start_character":43,"end_line":191,"end_character":16},"in_reply_to":"7faddb67_da234ebd","updated":"2019-08-21 17:26:46.000000000","message":"Yeah, probably safer to just abort the claim if we find we\u0027re on a host with a different page size. It also simplifies the comment above.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":190,"context_line":"        # destination host could calculate a pagesize different from the"},{"line_number":191,"context_line":"        # source. Override what numa_get_constraints() gave us with the page"},{"line_number":192,"context_line":"        # size from instance.numa_topology."},{"line_number":193,"context_line":"        if (numa_topology and self.migration and"},{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":195,"context_line":"            for cell in numa_topology.cells:"},{"line_number":196,"context_line":"                cell.pagesize \u003d self.instance.numa_topology.cells[0].pagesize"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_9a29569d","line":193,"range":{"start_line":193,"start_character":12,"end_line":193,"end_character":44},"updated":"2019-08-16 17:53:53.000000000","message":"How about you:\n\n1. Bail early above if numa_topology is None\n2. Take my suggestion above so you don\u0027t have to be defensive about self.migration being set?","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2d4dc7eea0a9bd78617722cf0548045380210f1","unresolved":false,"context_lines":[{"line_number":190,"context_line":"        # destination host could calculate a pagesize different from the"},{"line_number":191,"context_line":"        # source. Override what numa_get_constraints() gave us with the page"},{"line_number":192,"context_line":"        # size from instance.numa_topology."},{"line_number":193,"context_line":"        if (numa_topology and self.migration and"},{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":195,"context_line":"            for cell in numa_topology.cells:"},{"line_number":196,"context_line":"                cell.pagesize \u003d self.instance.numa_topology.cells[0].pagesize"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_fb50650c","line":193,"range":{"start_line":193,"start_character":12,"end_line":193,"end_character":44},"in_reply_to":"7faddb67_9a29569d","updated":"2019-08-21 17:26:46.000000000","message":"Rewrote the whole thing, this doesn\u0027t really apply anymore.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":195,"context_line":"            for cell in numa_topology.cells:"},{"line_number":196,"context_line":"                cell.pagesize \u003d self.instance.numa_topology.cells[0].pagesize"},{"line_number":197,"context_line":"        return numa_topology"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def abort(self):"},{"line_number":200,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_3504f313","line":197,"updated":"2019-08-16 17:53:53.000000000","message":"So just to be clear...for regular migration this property will be \"just what is calculated from the flavor, if any\" and for live, it will be \"what we calculate from the flavor, but with page sizes set to what the instance is currently using\"  right?\n\nSeems like this distinction should be in a docstring on this method.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6387281ecadddb5c37b0d046388e9a02e0d07570","unresolved":false,"context_lines":[{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":195,"context_line":"            for cell in numa_topology.cells:"},{"line_number":196,"context_line":"                cell.pagesize \u003d self.instance.numa_topology.cells[0].pagesize"},{"line_number":197,"context_line":"        return numa_topology"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def abort(self):"},{"line_number":200,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_9379fcfc","line":197,"in_reply_to":"7faddb67_3504f313","updated":"2019-08-21 16:35:50.000000000","message":"Also, thinking about this more.. is it possible that the numa topology we calculate from the host will be such that just jamming the current page_size into that structure will result in something that doesn\u0027t work? Like, what if we migrate from a system with an 8k page to a system that only supports 4k (making up numbers here)? We\u0027ll jam the existing 8k page size into the topology that we calculate from hardware, which may or may not work, right? I guess my point is, maybe we should revert to the suggestion above of just not letting you move between machines with different default page sizes?","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2d4dc7eea0a9bd78617722cf0548045380210f1","unresolved":false,"context_lines":[{"line_number":194,"context_line":"                self.migration.migration_type \u003d\u003d \u0027live-migration\u0027):"},{"line_number":195,"context_line":"            for cell in numa_topology.cells:"},{"line_number":196,"context_line":"                cell.pagesize \u003d self.instance.numa_topology.cells[0].pagesize"},{"line_number":197,"context_line":"        return numa_topology"},{"line_number":198,"context_line":""},{"line_number":199,"context_line":"    def abort(self):"},{"line_number":200,"context_line":"        \"\"\"Compute operation requiring claimed resources has failed or"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_bb566d04","line":197,"in_reply_to":"7faddb67_3504f313","updated":"2019-08-21 17:26:46.000000000","message":"Ditto.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f7fa0d167ccd23815ba1d9fa93e0d171dacf198a","unresolved":false,"context_lines":[{"line_number":173,"context_line":"        self.image_meta \u003d image_meta"},{"line_number":174,"context_line":"        super(MoveClaim, self).__init__(context, instance, nodename, tracker,"},{"line_number":175,"context_line":"                                        resources, pci_requests,"},{"line_number":176,"context_line":"                                        migration\u003dmigration, limits\u003dlimits)"},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"    @property"},{"line_number":179,"context_line":"    def numa_topology(self):"}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_0f7a94b9","line":176,"range":{"start_line":176,"start_character":40,"end_line":176,"end_character":59},"updated":"2019-08-23 14:03:19.000000000","message":"Hmm, it\u0027s weird to me that we take migration as a claim all the way up in the no-op base class. But whatever, as long as MoveClaim requires it I think it makes sense.","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"bdac4d6e8f008a2fbf0d920ec4f0551714479d6a","unresolved":false,"context_lines":[{"line_number":173,"context_line":"        self.image_meta \u003d image_meta"},{"line_number":174,"context_line":"        super(MoveClaim, self).__init__(context, instance, nodename, tracker,"},{"line_number":175,"context_line":"                                        resources, pci_requests,"},{"line_number":176,"context_line":"                                        migration\u003dmigration, limits\u003dlimits)"},{"line_number":177,"context_line":""},{"line_number":178,"context_line":"    @property"},{"line_number":179,"context_line":"    def numa_topology(self):"}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_8015f14b","line":176,"range":{"start_line":176,"start_character":40,"end_line":176,"end_character":59},"in_reply_to":"7faddb67_0f7a94b9","updated":"2019-08-23 16:26:36.000000000","message":"Yeah, I found it weird when I saw L35 as well.","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f7fa0d167ccd23815ba1d9fa93e0d171dacf198a","unresolved":false,"context_lines":[{"line_number":199,"context_line":"        page size to a host with a 2M page size, for example. This is not"},{"line_number":200,"context_line":"        something we want to support, so fail the claim if the page sizes are"},{"line_number":201,"context_line":"        different."},{"line_number":202,"context_line":"        \"\"\""},{"line_number":203,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":204,"context_line":"                self.instance.numa_topology):"},{"line_number":205,"context_line":"            if (self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":206,"context_line":"                    self.instance.numa_topology.cells[0].pagesize):"},{"line_number":207,"context_line":"                return (_(\u0027Requested page size is different from current \u0027"},{"line_number":208,"context_line":"                          \u0027page size.\u0027))"},{"line_number":209,"context_line":""}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_0f285495","line":206,"range":{"start_line":202,"start_character":0,"end_line":206,"end_character":67},"updated":"2019-08-23 14:03:19.000000000","message":"This is a nit, but I\u0027d just combine this all into one conditional instead of all the nesting. Also, you\u0027re only checking cells[0].pagesize,  but you were copying all the cells before. Will they always be the same on every cell or do we need to check?\n\nI like this failure better than the silent copy before though.","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"bdac4d6e8f008a2fbf0d920ec4f0551714479d6a","unresolved":false,"context_lines":[{"line_number":199,"context_line":"        page size to a host with a 2M page size, for example. This is not"},{"line_number":200,"context_line":"        something we want to support, so fail the claim if the page sizes are"},{"line_number":201,"context_line":"        different."},{"line_number":202,"context_line":"        \"\"\""},{"line_number":203,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":204,"context_line":"                self.instance.numa_topology):"},{"line_number":205,"context_line":"            if (self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":206,"context_line":"                    self.instance.numa_topology.cells[0].pagesize):"},{"line_number":207,"context_line":"                return (_(\u0027Requested page size is different from current \u0027"},{"line_number":208,"context_line":"                          \u0027page size.\u0027))"},{"line_number":209,"context_line":""}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_2a197b79","line":206,"range":{"start_line":202,"start_character":0,"end_line":206,"end_character":67},"in_reply_to":"7faddb67_0f285495","updated":"2019-08-23 16:26:36.000000000","message":"They\u0027ll always be the same, page size is a per-host thing.","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"64da00bf943e996c683edcfd608359ca357a3e53","unresolved":false,"context_lines":[{"line_number":186,"context_line":"            instance_type\u003dself.instance_type)"},{"line_number":187,"context_line":"        self.instance.drop_migration_context()"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":"    # TODO(artom) This entire method needs to go away, as we need to start"},{"line_number":190,"context_line":"    # using MoveClaim for all live migration resources."},{"line_number":191,"context_line":"    def _test_pci(self):"},{"line_number":192,"context_line":"        \"\"\"Live migration for PCI devices is only supported for Neutron SRIOV"}],"source_content_type":"text/x-python","patch_set":44,"id":"7faddb67_2362cfda","line":189,"updated":"2019-08-27 21:44:05.000000000","message":"I guess I\u0027m either supremely thick or supremely arrogant - I know this isn\u0027t what we said on IRC, but I still don\u0027t see the value in repeating [1] (but not the rest of conductor\u0027s _check_can_migrate_pci()?) here. Implementation-wise, I\u0027d have to override Claim\u0027s _test_pci() in MoveClaim regardless, as it seems wrong to check for migration_type\u003d\u003d\u0027live-migration\u0027 in a Claim that\u0027s not supposed to be about instances moving around. What\u0027s wrong with just skipping _test_pci() if this is a live migration?\n\n[1] https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L203-L221","commit_id":"6e922e882dbbc2bd02343a024f23f375e6b9028f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"76c8dec5c4a2bbc28ddc851a955b31dcbefd5ecc","unresolved":false,"context_lines":[{"line_number":186,"context_line":"            instance_type\u003dself.instance_type)"},{"line_number":187,"context_line":"        self.instance.drop_migration_context()"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":"    # TODO(artom) This entire method needs to go away, as we need to start"},{"line_number":190,"context_line":"    # using MoveClaim for all live migration resources."},{"line_number":191,"context_line":"    def _test_pci(self):"},{"line_number":192,"context_line":"        \"\"\"Live migration for PCI devices is only supported for Neutron SRIOV"}],"source_content_type":"text/x-python","patch_set":44,"id":"7faddb67_43326b60","line":189,"in_reply_to":"7faddb67_2362cfda","updated":"2019-08-27 22:32:02.000000000","message":"Thinking about this some more, I was approaching this from a \"how do I make this work\" angle, and not \"what does a MoveClaim for a live migration mean angle. The latter gives me more clarity, will respin.","commit_id":"6e922e882dbbc2bd02343a024f23f375e6b9028f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":169,"context_line":"        super(MoveClaim, self).__init__(context, instance, nodename, tracker,"},{"line_number":170,"context_line":"                                        resources, pci_requests,"},{"line_number":171,"context_line":"                                        limits\u003dlimits)"},{"line_number":172,"context_line":"        self.migration \u003d None"},{"line_number":173,"context_line":""},{"line_number":174,"context_line":"    @property"},{"line_number":175,"context_line":"    def numa_topology(self):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_fcea0a72","side":"PARENT","line":172,"updated":"2019-08-28 19:11:35.000000000","message":"And this is removed b/c it was redundant.","commit_id":"b5c86ebb0c23f203ee04d9373ce045bac04b2db3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    \"\"\"For use with compute drivers that do not support resource tracking.\"\"\""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self, *args, **kwargs):"},{"line_number":35,"context_line":"        self.migration \u003d kwargs.pop(\u0027migration\u0027, None)"},{"line_number":36,"context_line":"        self.claimed_numa_topology \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __enter__(self):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_7c485ab2","line":35,"updated":"2019-08-28 19:11:35.000000000","message":"Funny this was here but unused.\n\n(later)\n\nOh right it\u0027s used later in the compute manager, gross:\n\nhttps://github.com/openstack/nova/blob/3fe79c0bdb9666057afa69938b2bd6c93d9785be/nova/compute/manager.py#L4426","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":32,"context_line":"    \"\"\"For use with compute drivers that do not support resource tracking.\"\"\""},{"line_number":33,"context_line":""},{"line_number":34,"context_line":"    def __init__(self, *args, **kwargs):"},{"line_number":35,"context_line":"        self.migration \u003d kwargs.pop(\u0027migration\u0027, None)"},{"line_number":36,"context_line":"        self.claimed_numa_topology \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def __enter__(self):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_0205177a","line":35,"in_reply_to":"7faddb67_7c485ab2","updated":"2019-08-30 15:15:51.000000000","message":"Oh and it\u0027s vestigial because prep_resize gets a Migration from conductor since Queens:\n\nhttps://github.com/openstack/nova/blob/3fe79c0bdb9666057afa69938b2bd6c93d9785be/nova/compute/manager.py#L4386","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":58,"context_line":"    \"\"\""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def __init__(self, context, instance, nodename, tracker, resources,"},{"line_number":61,"context_line":"                 pci_requests, migration\u003dNone, limits\u003dNone):"},{"line_number":62,"context_line":"        super(Claim, self).__init__(migration\u003dmigration)"},{"line_number":63,"context_line":"        # Stash a copy of the instance at the current point of time"},{"line_number":64,"context_line":"        self.instance \u003d instance.obj_clone()"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_5ce23ea2","line":61,"updated":"2019-08-28 19:11:35.000000000","message":"nit: not sure why you don\u0027t add the new kwarg after limits but that\u0027s just me.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":58,"context_line":"    \"\"\""},{"line_number":59,"context_line":""},{"line_number":60,"context_line":"    def __init__(self, context, instance, nodename, tracker, resources,"},{"line_number":61,"context_line":"                 pci_requests, migration\u003dNone, limits\u003dNone):"},{"line_number":62,"context_line":"        super(Claim, self).__init__(migration\u003dmigration)"},{"line_number":63,"context_line":"        # Stash a copy of the instance at the current point of time"},{"line_number":64,"context_line":"        self.instance \u003d instance.obj_clone()"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_6d90a2ee","line":61,"in_reply_to":"7faddb67_5ce23ea2","updated":"2019-08-29 00:54:47.000000000","message":"For the MoveClaim it\u0027s a required positional arg, so I put it in the same order here (though here it\u0027s still optional).","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        if isinstance(limits, objects.SchedulerLimits):"},{"line_number":103,"context_line":"            limits \u003d limits.to_dict()"},{"line_number":104,"context_line":""},{"line_number":105,"context_line":"        # If an individual limit is None, the resource will be considered"},{"line_number":106,"context_line":"        # unlimited:"},{"line_number":107,"context_line":"        numa_topology_limit \u003d limits.get(\u0027numa_topology\u0027)"},{"line_number":108,"context_line":""},{"line_number":109,"context_line":"        reasons \u003d [self._test_numa_topology(resources, numa_topology_limit),"},{"line_number":110,"context_line":"                   self._test_pci()]"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cf7bce78","line":107,"range":{"start_line":105,"start_character":8,"end_line":107,"end_character":57},"updated":"2019-08-28 19:11:35.000000000","message":"I guess we don\u0027t do this for numa or pci huh?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":124,"context_line":"                return _(\u0027Claim pci failed\u0027)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":127,"context_line":"        host_topology \u003d (resources.numa_topology"},{"line_number":128,"context_line":"                         if \u0027numa_topology\u0027 in resources else None)"},{"line_number":129,"context_line":"        requested_topology \u003d self.numa_topology"},{"line_number":130,"context_line":"        if host_topology:"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_6f8ada64","line":127,"range":{"start_line":127,"start_character":25,"end_line":127,"end_character":48},"updated":"2019-08-28 19:11:35.000000000","message":"Ugh, I was trying to figure out what this is since it\u0027s not clear at all - resources is actually a ComputeNode object. Note to self to rename that thing.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d0ea8cc42a443bd9b2d468fd8bde80328c16795d","unresolved":false,"context_lines":[{"line_number":124,"context_line":"                return _(\u0027Claim pci failed\u0027)"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":127,"context_line":"        host_topology \u003d (resources.numa_topology"},{"line_number":128,"context_line":"                         if \u0027numa_topology\u0027 in resources else None)"},{"line_number":129,"context_line":"        requested_topology \u003d self.numa_topology"},{"line_number":130,"context_line":"        if host_topology:"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_44d79c24","line":127,"range":{"start_line":127,"start_character":25,"end_line":127,"end_character":48},"in_reply_to":"7faddb67_6f8ada64","updated":"2019-08-30 15:35:30.000000000","message":"\u003e Ugh, I was trying to figure out what this is since it\u0027s not clear\n \u003e at all - resources is actually a ComputeNode object. Note to self\n \u003e to rename that thing.\n\nhttps://review.opendev.org/#/c/679470/","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":187,"context_line":"        self.instance.drop_migration_context()"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":"    def _test_pci(self):"},{"line_number":190,"context_line":"        if self.migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":191,"context_line":"            return super(MoveClaim, self)._test_pci()"},{"line_number":192,"context_line":"        elif self._pci_requests.requests:"},{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_3cbf4261","line":190,"updated":"2019-08-28 19:11:35.000000000","message":"Document the logic here - why is live migration different?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":187,"context_line":"        self.instance.drop_migration_context()"},{"line_number":188,"context_line":""},{"line_number":189,"context_line":"    def _test_pci(self):"},{"line_number":190,"context_line":"        if self.migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":191,"context_line":"            return super(MoveClaim, self)._test_pci()"},{"line_number":192,"context_line":"        elif self._pci_requests.requests:"},{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_4d8b661d","line":190,"in_reply_to":"7faddb67_3cbf4261","updated":"2019-08-29 00:54:47.000000000","message":"Done","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    def _test_pci(self):"},{"line_number":190,"context_line":"        if self.migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":191,"context_line":"            return super(MoveClaim, self)._test_pci()"},{"line_number":192,"context_line":"        elif self._pci_requests.requests:"},{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"},{"line_number":194,"context_line":"                if (pci_request.source !\u003d"},{"line_number":195,"context_line":"                        objects.InstancePCIRequest.NEUTRON_PORT):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_5c579e27","line":192,"updated":"2019-08-28 19:11:35.000000000","message":"It would be good to document something in here about why we\u0027re failing on non-VIF PCI requests. I mean, I know SRIOV PCI requests are supported for live migration and handled differently outside of the MoveClaim, but before this we didn\u0027t explicitly fail if you had flavor-defined PCI requests and attempted a live migration right? Or do we actually fail if you attempt that somewhere else today?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    def _test_pci(self):"},{"line_number":190,"context_line":"        if self.migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":191,"context_line":"            return super(MoveClaim, self)._test_pci()"},{"line_number":192,"context_line":"        elif self._pci_requests.requests:"},{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"},{"line_number":194,"context_line":"                if (pci_request.source !\u003d"},{"line_number":195,"context_line":"                        objects.InstancePCIRequest.NEUTRON_PORT):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_8da5dea1","line":192,"in_reply_to":"7faddb67_5c579e27","updated":"2019-08-29 00:54:47.000000000","message":"\u003e Or do we actually fail if you attempt that somewhere else today?\n\nAs of SRIOV live migration, we do: [1]. But I don\u0027t think there was anything there before [1] landed, so I guess we blew up in fun ways?\n\nIn any case, I added a docstring that I hope addresses this point and the previous one.\n\n[1] https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L203-L237","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":189,"context_line":"    def _test_pci(self):"},{"line_number":190,"context_line":"        if self.migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":191,"context_line":"            return super(MoveClaim, self)._test_pci()"},{"line_number":192,"context_line":"        elif self._pci_requests.requests:"},{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"},{"line_number":194,"context_line":"                if (pci_request.source !\u003d"},{"line_number":195,"context_line":"                        objects.InstancePCIRequest.NEUTRON_PORT):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_c2513f0f","line":192,"in_reply_to":"7faddb67_8da5dea1","updated":"2019-08-30 15:15:51.000000000","message":"\u003e As of SRIOV live migration, we do: [1]. But I don\u0027t think there was anything there before [1] landed, so I guess we blew up in fun ways?\n\nYeah OK I guess. Failing explicitly is better anyway. Though it still kind of sucks that we don\u0027t fail higher in the stack, like the API or conductor because we\u0027ll just retry a bunch of a computes during live migration and eventually fail them all. Though I suppose the support is virt-driver specific, but I don\u0027t know of any other virt drivers (hyper-v would probably be closest) that would support live migration with pci devices. Anyway, that\u0027s unrelated to this change really.\n\n(later)\n\nActually, do we just always fail in conductor now regardless of computes supporting non-SRIOV PCI devices?\n\nhttps://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L203\n\nLooks like that\u0027s the case. That validation is not host specific so we\u0027ll still hit that for each alternate dest host, which is a waste of time, but it\u0027s something that could be resolved separately from this.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"},{"line_number":194,"context_line":"                if (pci_request.source !\u003d"},{"line_number":195,"context_line":"                        objects.InstancePCIRequest.NEUTRON_PORT):"},{"line_number":196,"context_line":"                    return (_(\u0027Non-VIF related PCI requests for instance are \u0027"},{"line_number":197,"context_line":"                              \u0027not supported for live migration.\u0027))"},{"line_number":198,"context_line":"            # TODO(artom) At this point, once we\u0027ve made sure we only have"},{"line_number":199,"context_line":"            # NEUTRON_PORT (aka SRIOV) PCI requests, we should check whether"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_dc438e6b","line":196,"range":{"start_line":196,"start_character":60,"end_line":196,"end_character":72},"updated":"2019-08-28 19:11:35.000000000","message":"nit: probably just remove this? \u0027for instance\u0027 without a uuid of the instance in scope sounds like \u0027for example\u0027.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":193,"context_line":"            for pci_request in self._pci_requests.requests:"},{"line_number":194,"context_line":"                if (pci_request.source !\u003d"},{"line_number":195,"context_line":"                        objects.InstancePCIRequest.NEUTRON_PORT):"},{"line_number":196,"context_line":"                    return (_(\u0027Non-VIF related PCI requests for instance are \u0027"},{"line_number":197,"context_line":"                              \u0027not supported for live migration.\u0027))"},{"line_number":198,"context_line":"            # TODO(artom) At this point, once we\u0027ve made sure we only have"},{"line_number":199,"context_line":"            # NEUTRON_PORT (aka SRIOV) PCI requests, we should check whether"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cd767640","line":196,"range":{"start_line":196,"start_character":60,"end_line":196,"end_character":72},"in_reply_to":"7faddb67_dc438e6b","updated":"2019-08-29 00:54:47.000000000","message":"I just lifted this from the conductor code [1], can rephrase.\n\n[1] https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L216-L221","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3213216d4f27799802ac31f54ad5d120c8e5d9d6","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # for example _claim_pci_for_instance_vifs() in the compute"},{"line_number":203,"context_line":"            # manager. So we do nothing here to avoid stepping on that code\u0027s"},{"line_number":204,"context_line":"            # toes, but ideally MoveClaim would be used for all live migration"},{"line_number":205,"context_line":"            # resource claims."},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _test_live_migration_page_size(self):"},{"line_number":208,"context_line":"        \"\"\"Tests that the current page size and the requested page size are the"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_e6811aa2","line":205,"updated":"2019-08-28 14:21:15.000000000","message":"I don\u0027t really understand why we need to call this out separately. Can we not call _test_pci() for the SRIOV case? Or, hmm, are the devices already claimed out of the pci tracker such that this test would fail because we\u0027ve already consumed the things we\u0027re checking for?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"efbdb29a543f6fdfb4b17bc4d0ad4415367bf306","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # for example _claim_pci_for_instance_vifs() in the compute"},{"line_number":203,"context_line":"            # manager. So we do nothing here to avoid stepping on that code\u0027s"},{"line_number":204,"context_line":"            # toes, but ideally MoveClaim would be used for all live migration"},{"line_number":205,"context_line":"            # resource claims."},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _test_live_migration_page_size(self):"},{"line_number":208,"context_line":"        \"\"\"Tests that the current page size and the requested page size are the"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_f9a53c04","line":205,"in_reply_to":"7faddb67_e6811aa2","updated":"2019-08-28 16:46:14.000000000","message":"\u003e Or, hmm, are the\n \u003e devices already claimed out of the pci tracker such that this test\n \u003e would fail because we\u0027ve already consumed the things we\u0027re checking\n \u003e for?\n\nThis. Though I suppose if we do the claims before the SRIOV live migration code (which is how it\u0027s now - https://review.opendev.org/#/c/634606/60/nova/compute/manager.py@6458 is before https://review.opendev.org/#/c/634606/60/nova/compute/manager.py@6461), we should be safe, as the claims only test, they don\u0027t allocate (allocation is done in _move_claim() in the resource tracker). But... is that sort of ordering something we want to rely on?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # for example _claim_pci_for_instance_vifs() in the compute"},{"line_number":203,"context_line":"            # manager. So we do nothing here to avoid stepping on that code\u0027s"},{"line_number":204,"context_line":"            # toes, but ideally MoveClaim would be used for all live migration"},{"line_number":205,"context_line":"            # resource claims."},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _test_live_migration_page_size(self):"},{"line_number":208,"context_line":"        \"\"\"Tests that the current page size and the requested page size are the"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_fc6d2a03","line":205,"in_reply_to":"7faddb67_f9a53c04","updated":"2019-08-28 19:11:35.000000000","message":"\u003e But... is that sort of ordering something we want to rely on?\n\nNo, this is all already very tightly coupled and brittle and crappy. I think eventually moving the SRIOV PCI claims stuff in-line with the other move claims stuff so live migration + SRIOV isn\u0027t a special unicorn would eventually avoid some of that weird tight coupling, right? But that\u0027s not something that\u0027s going to get done anytime soon.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":202,"context_line":"            # for example _claim_pci_for_instance_vifs() in the compute"},{"line_number":203,"context_line":"            # manager. So we do nothing here to avoid stepping on that code\u0027s"},{"line_number":204,"context_line":"            # toes, but ideally MoveClaim would be used for all live migration"},{"line_number":205,"context_line":"            # resource claims."},{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _test_live_migration_page_size(self):"},{"line_number":208,"context_line":"        \"\"\"Tests that the current page size and the requested page size are the"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cddf5636","line":205,"in_reply_to":"7faddb67_fc6d2a03","updated":"2019-08-29 00:54:47.000000000","message":"\u003e But that\u0027s not something that\u0027s going to get\n \u003e done anytime soon.\n\nI wouldn\u0027t be so pessimistic - Sean and/or myself will push for one of us to work on that in U.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":206,"context_line":""},{"line_number":207,"context_line":"    def _test_live_migration_page_size(self):"},{"line_number":208,"context_line":"        \"\"\"Tests that the current page size and the requested page size are the"},{"line_number":209,"context_line":"        same. This only applies for live migrations when the hw:mem_page_size"},{"line_number":210,"context_line":"        extra spec has been set to a non-numeric value (like \u0027large\u0027). That"},{"line_number":211,"context_line":"        would in theory allow an instance to live migrate from a host with a 1M"},{"line_number":212,"context_line":"        page size to a host with a 2M page size, for example. This is not"},{"line_number":213,"context_line":"        something we want to support, so fail the claim if the page sizes are"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_0f5ec6e8","line":210,"range":{"start_line":209,"start_character":14,"end_line":210,"end_character":70},"updated":"2019-08-28 19:11:35.000000000","message":"OK and we get this from hardware._get_numa_pagesize_constraint which is called from hardware.numa_get_constraints and...","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        \"\"\""},{"line_number":216,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":217,"context_line":"                self.instance.numa_topology and"},{"line_number":218,"context_line":"                self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":219,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_8f71567a","line":218,"range":{"start_line":218,"start_character":40,"end_line":218,"end_character":43},"updated":"2019-08-28 19:11:35.000000000","message":"...only check the first cell is OK since this:\n\nhttps://github.com/openstack/nova/blob/3fe79c0bdb9666057afa69938b2bd6c93d9785be/nova/virt/hardware.py#L1573\n\nsays that we only support the same pagesize across all cells - might be good to add a comment about why we only need to check the first cell so we don\u0027t forget that.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        \"\"\""},{"line_number":216,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":217,"context_line":"                self.instance.numa_topology and"},{"line_number":218,"context_line":"                self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":219,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_5c055e13","line":218,"range":{"start_line":218,"start_character":16,"end_line":218,"end_character":34},"updated":"2019-08-28 19:11:35.000000000","message":"This is calling numa_get_constraints which could return None, right? At which point you have a NoneType error here.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"7e9a46028a48697324f4c51775843902fc9e4f1f","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        \"\"\""},{"line_number":216,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":217,"context_line":"                self.instance.numa_topology and"},{"line_number":218,"context_line":"                self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":219,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_e47ec819","line":218,"range":{"start_line":218,"start_character":16,"end_line":218,"end_character":34},"in_reply_to":"7faddb67_42eeafb5","updated":"2019-08-30 16:04:27.000000000","message":"\u003e Did you add a test to ensure that ordering? Or I guess just a test\n \u003e where hardware.numa_get_constraints returns None during a live\n \u003e migration claim? I guess maybe you\u0027d know you called it out of\n \u003e order because you\u0027d get an error when self.claimed_numa_topology\n \u003e isn\u0027t defined.\n\nThe one test we could do is make _test_numa_topology fail, and then make sure _test_live_migration_page_size doesn\u0027t get called. Because if the former passes, then claimed_numa_topology has been set.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        \"\"\""},{"line_number":216,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":217,"context_line":"                self.instance.numa_topology and"},{"line_number":218,"context_line":"                self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":219,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_42eeafb5","line":218,"range":{"start_line":218,"start_character":16,"end_line":218,"end_character":34},"in_reply_to":"7faddb67_4dc84620","updated":"2019-08-30 15:15:51.000000000","message":"\u003e So, I instead made it so that _test_numa_topology only calls this\n \u003e method if super()._test_numa_topology() succeeded and we\u0027re sure we\n \u003e have a claimed_numa_topology, and then made _test_pci() use the\n \u003e claimed_numa_topology.\n\nDid you add a test to ensure that ordering? Or I guess just a test where hardware.numa_get_constraints returns None during a live migration claim? I guess maybe you\u0027d know you called it out of order because you\u0027d get an error when self.claimed_numa_topology isn\u0027t defined.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        \"\"\""},{"line_number":216,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":217,"context_line":"                self.instance.numa_topology and"},{"line_number":218,"context_line":"                self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":219,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_4dc84620","line":218,"range":{"start_line":218,"start_character":16,"end_line":218,"end_character":34},"in_reply_to":"7faddb67_5c055e13","updated":"2019-08-29 00:54:47.000000000","message":"So, I instead made it so that _test_numa_topology only calls this method if super()._test_numa_topology() succeeded and we\u0027re sure we have a claimed_numa_topology, and then made _test_pci() use the claimed_numa_topology.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":215,"context_line":"        \"\"\""},{"line_number":216,"context_line":"        if (self.migration.migration_type \u003d\u003d \u0027live-migration\u0027 and"},{"line_number":217,"context_line":"                self.instance.numa_topology and"},{"line_number":218,"context_line":"                self.numa_topology.cells[0].pagesize !\u003d"},{"line_number":219,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cdd3d6d2","line":218,"range":{"start_line":218,"start_character":40,"end_line":218,"end_character":43},"in_reply_to":"7faddb67_8f71567a","updated":"2019-08-29 00:54:47.000000000","message":"Done","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":224,"context_line":"        return (self._test_live_migration_page_size() or"},{"line_number":225,"context_line":"                super(MoveClaim, self)._test_numa_topology(resources, limit))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_ef09ea2a","line":223,"updated":"2019-08-28 19:11:35.000000000","message":"Could really benefit from a comment on the logic here.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":220,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":221,"context_line":"                      \u0027page size.\u0027))"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":224,"context_line":"        return (self._test_live_migration_page_size() or"},{"line_number":225,"context_line":"                super(MoveClaim, self)._test_numa_topology(resources, limit))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cd8196a2","line":223,"in_reply_to":"7faddb67_ef09ea2a","updated":"2019-08-29 00:54:47.000000000","message":"Done","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":221,"context_line":"                      \u0027page size.\u0027))"},{"line_number":222,"context_line":""},{"line_number":223,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":224,"context_line":"        return (self._test_live_migration_page_size() or"},{"line_number":225,"context_line":"                super(MoveClaim, self)._test_numa_topology(resources, limit))"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cf1f4e59","line":224,"range":{"start_line":224,"start_character":21,"end_line":224,"end_character":51},"updated":"2019-08-28 19:11:35.000000000","message":"So this either returns None or a Message object, right? If it returns None, we call _test_numa_topology in the parent class, but if it returns the Message object, we return that? Oh right I see, if these test methods return a non-None value it means the claim failed (L112).\n\nGod I love not documenting anything.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":238,"context_line":"        \"\"\"Test whether this host can accept the instance\u0027s NUMA topology. The"},{"line_number":239,"context_line":"        _test methods return None on success, and a string explaining the"},{"line_number":240,"context_line":"        reason on failure. So we call up to the normal Claim\u0027s"},{"line_number":241,"context_line":"        _test_numa_topology(), and if we get nothing back we test the page"},{"line_number":242,"context_line":"        size."}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_42430fb1","line":239,"range":{"start_line":239,"start_character":52,"end_line":239,"end_character":58},"updated":"2019-08-30 15:15:51.000000000","message":"nit: technically it\u0027s a Message _() object but...","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b1a010e1420c3834f123fc6ec2188143afe874b1","unresolved":false,"context_lines":[{"line_number":236,"context_line":""},{"line_number":237,"context_line":"    def _test_numa_topology(self, resources, limit):"},{"line_number":238,"context_line":"        \"\"\"Test whether this host can accept the instance\u0027s NUMA topology. The"},{"line_number":239,"context_line":"        _test methods return None on success, and a string explaining the"},{"line_number":240,"context_line":"        reason on failure. So we call up to the normal Claim\u0027s"},{"line_number":241,"context_line":"        _test_numa_topology(), and if we get nothing back we test the page"},{"line_number":242,"context_line":"        size."}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_045a04f1","line":239,"range":{"start_line":239,"start_character":52,"end_line":239,"end_character":58},"in_reply_to":"7faddb67_42430fb1","updated":"2019-08-30 15:35:09.000000000","message":"Done","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                  self)._test_numa_topology(resources, limit)"},{"line_number":246,"context_line":"        if numa_test_failure:"},{"line_number":247,"context_line":"            return numa_test_failure"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            return self._test_live_migration_page_size()"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_0213d7b9","line":248,"updated":"2019-08-30 15:15:51.000000000","message":"nit: you can remove this else","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b1a010e1420c3834f123fc6ec2188143afe874b1","unresolved":false,"context_lines":[{"line_number":245,"context_line":"                                  self)._test_numa_topology(resources, limit)"},{"line_number":246,"context_line":"        if numa_test_failure:"},{"line_number":247,"context_line":"            return numa_test_failure"},{"line_number":248,"context_line":"        else:"},{"line_number":249,"context_line":"            return self._test_live_migration_page_size()"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_c46f0c90","line":248,"in_reply_to":"7faddb67_0213d7b9","updated":"2019-08-30 15:35:09.000000000","message":"Done","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"d4e807de60bda84b2069035a336abb09d3f2578d","unresolved":false,"context_lines":[{"line_number":229,"context_line":"                self.instance.numa_topology and"},{"line_number":230,"context_line":"                # NOTE(artom) We only support a single page size across all"},{"line_number":231,"context_line":"                # cells, checking cell 0 is sufficient."},{"line_number":232,"context_line":"                self.claimed_numa_topology.cells[0].pagesize !\u003d"},{"line_number":233,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":234,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":235,"context_line":"                      \u0027page size.\u0027))"},{"line_number":236,"context_line":""}],"source_content_type":"text/x-python","patch_set":50,"id":"7faddb67_67fe28b0","line":233,"range":{"start_line":232,"start_character":16,"end_line":233,"end_character":63},"updated":"2019-09-04 07:51:15.000000000","message":"this can be stopped at NUMATopology filter also?","commit_id":"dac259b15a6507c2e4e274e3b8614f77c61f93e2"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"2e0502f7d70bad0978b4aa423d05ae8fccee2991","unresolved":false,"context_lines":[{"line_number":229,"context_line":"                self.instance.numa_topology and"},{"line_number":230,"context_line":"                # NOTE(artom) We only support a single page size across all"},{"line_number":231,"context_line":"                # cells, checking cell 0 is sufficient."},{"line_number":232,"context_line":"                self.claimed_numa_topology.cells[0].pagesize !\u003d"},{"line_number":233,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":234,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":235,"context_line":"                      \u0027page size.\u0027))"},{"line_number":236,"context_line":""}],"source_content_type":"text/x-python","patch_set":50,"id":"7faddb67_a7e66015","line":233,"range":{"start_line":232,"start_character":16,"end_line":233,"end_character":63},"in_reply_to":"7faddb67_67fe28b0","updated":"2019-09-04 08:01:09.000000000","message":"oh, we can\u0027t, we don\u0027t have original instance info.","commit_id":"dac259b15a6507c2e4e274e3b8614f77c61f93e2"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5c04ea54b282d1a461046c42722c5f0d198ee4ed","unresolved":false,"context_lines":[{"line_number":229,"context_line":"                self.instance.numa_topology and"},{"line_number":230,"context_line":"                # NOTE(artom) We only support a single page size across all"},{"line_number":231,"context_line":"                # cells, checking cell 0 is sufficient."},{"line_number":232,"context_line":"                self.claimed_numa_topology.cells[0].pagesize !\u003d"},{"line_number":233,"context_line":"                self.instance.numa_topology.cells[0].pagesize):"},{"line_number":234,"context_line":"            return (_(\u0027Requested page size is different from current \u0027"},{"line_number":235,"context_line":"                      \u0027page size.\u0027))"},{"line_number":236,"context_line":""}],"source_content_type":"text/x-python","patch_set":50,"id":"5faad753_ec0e09e4","line":233,"range":{"start_line":232,"start_character":16,"end_line":233,"end_character":63},"in_reply_to":"7faddb67_a7e66015","updated":"2019-09-09 17:42:23.000000000","message":"we could do some checks in the numa toplogy filter but sicne we dont claim the hugepage at that point we would have to check agian here anyway since we coudl race and a host can have multiple pools so even if we did check there we need to check here.","commit_id":"dac259b15a6507c2e4e274e3b8614f77c61f93e2"}],"nova/compute/resource_tracker.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7fb84c156badf8a359402f18ebf9015ee90d4ac5","unresolved":false,"context_lines":[{"line_number":73,"context_line":"    return False"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def _is_trackable_migration(migration):"},{"line_number":77,"context_line":"    # Only look at resize/migrate migration and evacuation records"},{"line_number":78,"context_line":"    # NOTE(danms): RT should probably examine live migration"},{"line_number":79,"context_line":"    # records as well and do something smart. However, ignore"},{"line_number":80,"context_line":"    # those for now to avoid them being included in below calculations."},{"line_number":81,"context_line":"    return migration.migration_type in (\u0027resize\u0027, \u0027migration\u0027,"},{"line_number":82,"context_line":"                                        \u0027evacuation\u0027, \u0027live-migration\u0027)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"def _normalize_inventory_from_cn_obj(inv_data, cn):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_79af607d","line":82,"range":{"start_line":76,"start_character":0,"end_line":82,"end_character":71},"updated":"2019-02-19 17:12:49.000000000","message":"I\u0027m guessing this entire function is unnecessary now (are there any other types of migration possible?). If not, the comments at least need to be updated/removed","commit_id":"193bfbe26156368c2b101781bd742eb9d2ba826f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"03a479ed989a400186129510ed6a2e8ac74a91b6","unresolved":false,"context_lines":[{"line_number":73,"context_line":"    return False"},{"line_number":74,"context_line":""},{"line_number":75,"context_line":""},{"line_number":76,"context_line":"def _is_trackable_migration(migration):"},{"line_number":77,"context_line":"    # Only look at resize/migrate migration and evacuation records"},{"line_number":78,"context_line":"    # NOTE(danms): RT should probably examine live migration"},{"line_number":79,"context_line":"    # records as well and do something smart. However, ignore"},{"line_number":80,"context_line":"    # those for now to avoid them being included in below calculations."},{"line_number":81,"context_line":"    return migration.migration_type in (\u0027resize\u0027, \u0027migration\u0027,"},{"line_number":82,"context_line":"                                        \u0027evacuation\u0027, \u0027live-migration\u0027)"},{"line_number":83,"context_line":""},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"def _normalize_inventory_from_cn_obj(inv_data, cn):"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_baeee888","line":82,"range":{"start_line":76,"start_character":0,"end_line":82,"end_character":71},"in_reply_to":"9fdfeff1_79af607d","updated":"2019-02-21 16:23:31.000000000","message":"Done","commit_id":"193bfbe26156368c2b101781bd742eb9d2ba826f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7fb84c156badf8a359402f18ebf9015ee90d4ac5","unresolved":false,"context_lines":[{"line_number":253,"context_line":"                                limits\u003dlimits)"},{"line_number":254,"context_line":""},{"line_number":255,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":256,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":257,"context_line":"                             pagesize\u003dNone):"},{"line_number":258,"context_line":"        # Flavor and image cannot change during a live migration."},{"line_number":259,"context_line":"        instance_type \u003d instance.flavor"},{"line_number":260,"context_line":"        image_meta \u003d instance.image_meta"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_d9168ca8","line":257,"range":{"start_line":256,"start_character":0,"end_line":257,"end_character":44},"updated":"2019-02-19 17:12:49.000000000","message":"A docstring would be super","commit_id":"193bfbe26156368c2b101781bd742eb9d2ba826f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"03a479ed989a400186129510ed6a2e8ac74a91b6","unresolved":false,"context_lines":[{"line_number":253,"context_line":"                                limits\u003dlimits)"},{"line_number":254,"context_line":""},{"line_number":255,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":256,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":257,"context_line":"                             pagesize\u003dNone):"},{"line_number":258,"context_line":"        # Flavor and image cannot change during a live migration."},{"line_number":259,"context_line":"        instance_type \u003d instance.flavor"},{"line_number":260,"context_line":"        image_meta \u003d instance.image_meta"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_9aebe476","line":257,"range":{"start_line":256,"start_character":0,"end_line":257,"end_character":44},"in_reply_to":"9fdfeff1_d9168ca8","updated":"2019-02-21 16:23:31.000000000","message":"Done","commit_id":"193bfbe26156368c2b101781bd742eb9d2ba826f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"05664cddf98b325c1149d218c7076ae634e2ccc4","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                                limits\u003dlimits)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             pagesize\u003dNone):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_c1d2bbd1","line":247,"range":{"start_line":247,"start_character":8,"end_line":247,"end_character":28},"updated":"2019-02-24 22:23:51.000000000","message":"We don\u0027t get limits during a live migration claim? We run the NUMATopologyFilter during scheduling to pick the host, which would set limits on the resulting Selection object from the scheduler, but I guess those just aren\u0027t passed down to the destination host for the claim. Should they be?","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"9fe6847e64c736ab2703356e0b50cf77a60b8559","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                                limits\u003dlimits)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             pagesize\u003dNone):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_e1181f85","line":247,"range":{"start_line":247,"start_character":8,"end_line":247,"end_character":28},"in_reply_to":"9fdfeff1_c1d2bbd1","updated":"2019-02-25 15:28:52.000000000","message":"Thinking about your question actually answered a question I had about what limits are for: they\u0027re for when allocation ratios are set centrally on the controller only, and not on each compute host, right?\n\nSo yeah, we should pass them to the claim. Done.","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"98a6f44e3efbca8b390b2595d8ce82954b810898","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                                limits\u003dlimits)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             pagesize\u003dNone):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_6da5ec9e","line":247,"range":{"start_line":247,"start_character":8,"end_line":247,"end_character":28},"in_reply_to":"9fdfeff1_e1181f85","updated":"2019-02-25 20:57:03.000000000","message":"They\u0027re calculated by the controller and passed down to the compute to tell it what to claim. That is partially for the behavior where ratios are centrally-managed, but also for the case where you used to be able to just disable a scheduler filter and cause all the computes to stop accounting for that thing. So, if you don\u0027t want to schedule on Disk (because it\u0027s never been right), you can disable that filter and that will cause computes to never do the Disk part of the claim stuff and just assume things will be fine.\n\nIn more general terms, it\u0027s the way the scheduler communicates to the compute what it picked.","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"05664cddf98b325c1149d218c7076ae634e2ccc4","unresolved":false,"context_lines":[{"line_number":290,"context_line":"                         instance is running, changing its pagesize is only"},{"line_number":291,"context_line":"                         supported on x86 - and even then, post-copy live"},{"line_number":292,"context_line":"                         migration requires the same pagesize on source and"},{"line_number":293,"context_line":"                         dest. For this reason, we pass the page size to the"},{"line_number":294,"context_line":"                         claim so that it can reject hosts that don\u0027t support"},{"line_number":295,"context_line":"                         the instance\u0027s current page size."},{"line_number":296,"context_line":"        :returns: A Claim ticket representing the reserved resources.  This"},{"line_number":297,"context_line":"        should be turned into finalize  a resource claim or free"},{"line_number":298,"context_line":"        resources after the compute operation is finished."}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_81d4b3bc","line":295,"range":{"start_line":293,"start_character":31,"end_line":295,"end_character":58},"updated":"2019-02-24 22:23:51.000000000","message":"Why wasn\u0027t the host filtered out by the NUMATopologyFilter before we get this far?","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"9fe6847e64c736ab2703356e0b50cf77a60b8559","unresolved":false,"context_lines":[{"line_number":290,"context_line":"                         instance is running, changing its pagesize is only"},{"line_number":291,"context_line":"                         supported on x86 - and even then, post-copy live"},{"line_number":292,"context_line":"                         migration requires the same pagesize on source and"},{"line_number":293,"context_line":"                         dest. For this reason, we pass the page size to the"},{"line_number":294,"context_line":"                         claim so that it can reject hosts that don\u0027t support"},{"line_number":295,"context_line":"                         the instance\u0027s current page size."},{"line_number":296,"context_line":"        :returns: A Claim ticket representing the reserved resources.  This"},{"line_number":297,"context_line":"        should be turned into finalize  a resource claim or free"},{"line_number":298,"context_line":"        resources after the compute operation is finished."}],"source_content_type":"text/x-python","patch_set":18,"id":"9fdfeff1_740ceb38","line":295,"range":{"start_line":293,"start_character":31,"end_line":295,"end_character":58},"in_reply_to":"9fdfeff1_81d4b3bc","updated":"2019-02-25 15:28:52.000000000","message":"The filter works with flavor extra specs, so if it\u0027s `large`, any host that fullfils this will pass, including both 1G and 2M hosts.","commit_id":"cbb2c58b49e9bd5cd8ab2b05bc4c056c4951d047"},{"author":{"_account_id":19124,"name":"Gerry Kopec","email":"gerry.kopec@windriver.com","username":"gkopec"},"change_message_id":"408ec2e0fb3e509b357aa88b312b18d9fcc3ecc9","unresolved":false,"context_lines":[{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    if (vm in [vm_states.ACTIVE, vm_states.STOPPED]"},{"line_number":69,"context_line":"            and task in ("},{"line_number":70,"context_line":"                task_states.resizing_states + task_states.rebuild_states)):"},{"line_number":71,"context_line":"        return True"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"    return False"}],"source_content_type":"text/x-python","patch_set":24,"id":"5fc1f717_33ba70de","line":70,"updated":"2019-03-05 18:57:59.000000000","message":"Also need to update here (or maybe _update_usage_from_migrations()) to include instances in MIGRATING task state. From testing this change, I observed that the resource audit will lose the claim from an in-progress live migration on the destination node.","commit_id":"6981ce2b4561972b29535e6191a991148de85727"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2903b192243d65584daa0016082711160b5a6f97","unresolved":false,"context_lines":[{"line_number":67,"context_line":""},{"line_number":68,"context_line":"    if (vm in [vm_states.ACTIVE, vm_states.STOPPED]"},{"line_number":69,"context_line":"            and task in ("},{"line_number":70,"context_line":"                task_states.resizing_states + task_states.rebuild_states)):"},{"line_number":71,"context_line":"        return True"},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"    return False"}],"source_content_type":"text/x-python","patch_set":24,"id":"5fc1f717_ffec1990","line":70,"in_reply_to":"5fc1f717_33ba70de","updated":"2019-03-12 22:49:38.000000000","message":"Or just rebase on top of this: https://review.openstack.org/#/c/560467/","commit_id":"6981ce2b4561972b29535e6191a991148de85727"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":1095,"context_line":"        \"\"\"Update usage for a single migration.  The record may"},{"line_number":1096,"context_line":"        represent an incoming or outbound migration."},{"line_number":1097,"context_line":"        \"\"\""},{"line_number":1098,"context_line":"        if not _is_trackable_migration(migration):"},{"line_number":1099,"context_line":"            return"},{"line_number":1100,"context_line":""},{"line_number":1101,"context_line":"        uuid \u003d migration.instance_uuid"},{"line_number":1102,"context_line":"        LOG.info(\"Updating resource usage from migration %s\", migration.uuid,"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_f5117b54","side":"PARENT","line":1099,"range":{"start_line":1098,"start_character":0,"end_line":1099,"end_character":18},"updated":"2019-08-16 17:53:53.000000000","message":"What about migrations that are in progress or unfinished across an upgrade boundary? Not likely that we\u0027d expect to upgrade nova while a live migration is actually in progress, but we could easily have some failed/incomplete/stalled ones that were stuck across an upgrade boundary. I haven\u0027t gone looking, but might removing this cause us to get some math wrong after the upgrade?","commit_id":"6b7d0caad86fe32ffc49a8672de1eb7258f3b919"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"fca455741a3be76968aff1205b834f958bf60c9d","unresolved":false,"context_lines":[{"line_number":1095,"context_line":"        \"\"\"Update usage for a single migration.  The record may"},{"line_number":1096,"context_line":"        represent an incoming or outbound migration."},{"line_number":1097,"context_line":"        \"\"\""},{"line_number":1098,"context_line":"        if not _is_trackable_migration(migration):"},{"line_number":1099,"context_line":"            return"},{"line_number":1100,"context_line":""},{"line_number":1101,"context_line":"        uuid \u003d migration.instance_uuid"},{"line_number":1102,"context_line":"        LOG.info(\"Updating resource usage from migration %s\", migration.uuid,"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_2490fc4d","side":"PARENT","line":1099,"range":{"start_line":1098,"start_character":0,"end_line":1099,"end_character":18},"in_reply_to":"7faddb67_f3db90e6","updated":"2019-08-21 19:01:37.000000000","message":"Actually it\u0027s more involved than that. The current version of [1] only does the claim if there\u0027s NUMA-y stuff going on (which you\u0027ve actually commented on).\n\nSo I think 2 things need to change:\n\nIn [1], always do a claim for live migrations, and if the instance has no NUMA topology it\u0027s essentially a noop, since all the other resources have moved to placement (right? Not sure about PCI)\n\nHere, move those trackable migrations to [1], because as you said, removing the checks without actually doing the claim will introduce discrepancies.\n\n[1] https://review.opendev.org/#/c/634606/50/nova/compute/manager.py","commit_id":"6b7d0caad86fe32ffc49a8672de1eb7258f3b919"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6387281ecadddb5c37b0d046388e9a02e0d07570","unresolved":false,"context_lines":[{"line_number":1095,"context_line":"        \"\"\"Update usage for a single migration.  The record may"},{"line_number":1096,"context_line":"        represent an incoming or outbound migration."},{"line_number":1097,"context_line":"        \"\"\""},{"line_number":1098,"context_line":"        if not _is_trackable_migration(migration):"},{"line_number":1099,"context_line":"            return"},{"line_number":1100,"context_line":""},{"line_number":1101,"context_line":"        uuid \u003d migration.instance_uuid"},{"line_number":1102,"context_line":"        LOG.info(\"Updating resource usage from migration %s\", migration.uuid,"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_f3db90e6","side":"PARENT","line":1099,"range":{"start_line":1098,"start_character":0,"end_line":1099,"end_character":18},"in_reply_to":"7faddb67_f5117b54","updated":"2019-08-21 16:35:50.000000000","message":"Also, re-reading this today... aren\u0027t you removing the safeguards in place in this patch, before we fully implement the rest of the plumbing for claiming during LM? If this patch lands but none of the above ones do, are we going to go back to improper accounting of these migrations?","commit_id":"6b7d0caad86fe32ffc49a8672de1eb7258f3b919"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6387281ecadddb5c37b0d046388e9a02e0d07570","unresolved":false,"context_lines":[{"line_number":254,"context_line":"        return self._move_claim(context, instance, instance_type, nodename,"},{"line_number":255,"context_line":"                                migration, move_type\u003d\u0027live-migration\u0027,"},{"line_number":256,"context_line":"                                image_meta\u003dimage_meta, limits\u003dlimits)"},{"line_number":257,"context_line":""},{"line_number":258,"context_line":"    def _move_claim(self, context, instance, new_instance_type, nodename,"},{"line_number":259,"context_line":"                    migration, move_type\u003dNone, image_meta\u003dNone, limits\u003dNone):"},{"line_number":260,"context_line":"        \"\"\"Indicate that resources are needed for a move to this host."}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_33512863","line":257,"updated":"2019-08-21 16:35:50.000000000","message":"This is, AFAICT, not called anywhere in this patch and is also untested, right?","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"c95f30a54a0a3e55b264c2822b9733f8de0056ed","unresolved":false,"context_lines":[{"line_number":254,"context_line":"        return self._move_claim(context, instance, instance_type, nodename,"},{"line_number":255,"context_line":"                                migration, move_type\u003d\u0027live-migration\u0027,"},{"line_number":256,"context_line":"                                image_meta\u003dimage_meta, limits\u003dlimits)"},{"line_number":257,"context_line":""},{"line_number":258,"context_line":"    def _move_claim(self, context, instance, new_instance_type, nodename,"},{"line_number":259,"context_line":"                    migration, move_type\u003dNone, image_meta\u003dNone, limits\u003dNone):"},{"line_number":260,"context_line":"        \"\"\"Indicate that resources are needed for a move to this host."}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_50eb6177","line":257,"in_reply_to":"7faddb67_33512863","updated":"2019-08-22 20:19:27.000000000","message":"Done","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":308,"context_line":"        claim \u003d claims.MoveClaim(context, instance, nodename,"},{"line_number":309,"context_line":"                                 new_instance_type, image_meta, self, cn,"},{"line_number":310,"context_line":"                                 new_pci_requests, limits\u003dlimits,"},{"line_number":311,"context_line":"                                 migration\u003dmigration)"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"        claimed_pci_devices_objs \u003d []"},{"line_number":314,"context_line":"        if self.pci_tracker:"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_d52d5f8f","line":311,"range":{"start_line":311,"start_character":33,"end_line":311,"end_character":52},"updated":"2019-08-16 17:53:53.000000000","message":"Yeah, so this will always be set, right? So we can just move it before the kwargs and make it something we can depend on.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"fca455741a3be76968aff1205b834f958bf60c9d","unresolved":false,"context_lines":[{"line_number":308,"context_line":"        claim \u003d claims.MoveClaim(context, instance, nodename,"},{"line_number":309,"context_line":"                                 new_instance_type, image_meta, self, cn,"},{"line_number":310,"context_line":"                                 new_pci_requests, limits\u003dlimits,"},{"line_number":311,"context_line":"                                 migration\u003dmigration)"},{"line_number":312,"context_line":""},{"line_number":313,"context_line":"        claimed_pci_devices_objs \u003d []"},{"line_number":314,"context_line":"        if self.pci_tracker:"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_7908e125","line":311,"range":{"start_line":311,"start_character":33,"end_line":311,"end_character":52},"in_reply_to":"7faddb67_d52d5f8f","updated":"2019-08-21 19:01:37.000000000","message":"Done","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"38cf2a4c0fafc4d776bbd0f9b69994a87135e1b4","unresolved":false,"context_lines":[{"line_number":378,"context_line":"        migration.dest_compute \u003d self.host"},{"line_number":379,"context_line":"        migration.dest_node \u003d nodename"},{"line_number":380,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":381,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":382,"context_line":"            migration.status \u003d \u0027pre-migrating\u0027"},{"line_number":383,"context_line":"        migration.save()"},{"line_number":384,"context_line":""}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_351dd361","line":381,"updated":"2019-08-16 17:53:53.000000000","message":"Probably need a comment above, explaining that cold migrations have a couple more states and that we push cold migrations into this state when we\u0027ve determined where they\u0027re going.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"c95f30a54a0a3e55b264c2822b9733f8de0056ed","unresolved":false,"context_lines":[{"line_number":378,"context_line":"        migration.dest_compute \u003d self.host"},{"line_number":379,"context_line":"        migration.dest_node \u003d nodename"},{"line_number":380,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":381,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":382,"context_line":"            migration.status \u003d \u0027pre-migrating\u0027"},{"line_number":383,"context_line":"        migration.save()"},{"line_number":384,"context_line":""}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_6d1006ad","line":381,"in_reply_to":"7faddb67_351dd361","updated":"2019-08-22 20:19:27.000000000","message":"Done","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f7fa0d167ccd23815ba1d9fa93e0d171dacf198a","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                                limits\u003dlimits)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             limits):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_6a1e9660","line":247,"updated":"2019-08-23 14:03:19.000000000","message":"AFAICT, this is dead code you\u0027re adding right? It would be better to add this where we\u0027re going to use it, I think, since it\u0027s not a massive addition. But, I\u0027ll keep reading up the stack.","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"bdac4d6e8f008a2fbf0d920ec4f0551714479d6a","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                                limits\u003dlimits)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             limits):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_0a86ff66","line":247,"in_reply_to":"7faddb67_6a1e9660","updated":"2019-08-23 16:26:36.000000000","message":"Yeah, not actually used in anything besides tests. All in the interest of patch size and reviewability.","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f7fa0d167ccd23815ba1d9fa93e0d171dacf198a","unresolved":false,"context_lines":[{"line_number":316,"context_line":"                    new_pci_requests.requests.append(request)"},{"line_number":317,"context_line":"        claim \u003d claims.MoveClaim(context, instance, nodename,"},{"line_number":318,"context_line":"                                 new_instance_type, image_meta, self, cn,"},{"line_number":319,"context_line":"                                 new_pci_requests, migration\u003dmigration,"},{"line_number":320,"context_line":"                                 limits\u003dlimits)"},{"line_number":321,"context_line":""},{"line_number":322,"context_line":"        claimed_pci_devices_objs \u003d []"}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_ef03780f","line":319,"range":{"start_line":319,"start_character":51,"end_line":319,"end_character":61},"updated":"2019-08-23 14:03:19.000000000","message":"nix this","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"bdac4d6e8f008a2fbf0d920ec4f0551714479d6a","unresolved":false,"context_lines":[{"line_number":316,"context_line":"                    new_pci_requests.requests.append(request)"},{"line_number":317,"context_line":"        claim \u003d claims.MoveClaim(context, instance, nodename,"},{"line_number":318,"context_line":"                                 new_instance_type, image_meta, self, cn,"},{"line_number":319,"context_line":"                                 new_pci_requests, migration\u003dmigration,"},{"line_number":320,"context_line":"                                 limits\u003dlimits)"},{"line_number":321,"context_line":""},{"line_number":322,"context_line":"        claimed_pci_devices_objs \u003d []"}],"source_content_type":"text/x-python","patch_set":37,"id":"7faddb67_caf467f4","line":319,"range":{"start_line":319,"start_character":51,"end_line":319,"end_character":61},"in_reply_to":"7faddb67_ef03780f","updated":"2019-08-23 16:26:36.000000000","message":"Done","commit_id":"e9231a0de36b8b726570226c90e0f79e6fc3b4a0"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"beb6a5dfb03f2ecdf84d71c9ef8f753cc0aa83c6","unresolved":false,"context_lines":[{"line_number":301,"context_line":""},{"line_number":302,"context_line":"        cn \u003d self.compute_nodes[nodename]"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_f3491e66","line":304,"updated":"2019-08-23 19:06:53.000000000","message":"Added this in PS39 after it dawned on me that SRIOV live migration is a thing, and talking to Sean on IRC. Not actually sure this is necessary, so manual testing is needed, but wanted to put this in here to indicate that it\u0027s a thing we need to consider.","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"be48a61c94953b1c4e5f4151496cf2c78bf82bd0","unresolved":false,"context_lines":[{"line_number":301,"context_line":""},{"line_number":302,"context_line":"        cn \u003d self.compute_nodes[nodename]"},{"line_number":303,"context_line":""},{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_89ae602d","line":304,"in_reply_to":"7faddb67_f3491e66","updated":"2019-08-26 19:43:57.000000000","message":"we do need this.","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1306f62a1b6b54d2bb8c2cab1373728930b7cd3d","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_4207e5ca","line":307,"updated":"2019-08-26 14:30:18.000000000","message":"Might this result in going from one machine with PCI devices to another with none? I\u0027m not sure how live migration with SRIOV works, but I would expect to copy or generate new requests on/for the other side...","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"be48a61c94953b1c4e5f4151496cf2c78bf82bd0","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_044a555a","line":307,"in_reply_to":"7faddb67_02e9edb5","updated":"2019-08-26 19:43:57.000000000","message":"for sriov migration we do several checks.\nfirst we check that both the source and dest host support sriov migration in the conductor which required neutron multiple port binding support. if we detect the compute nodes are new enough, we then attempt to claim sriov device in\ncheck_can_live_migrate_destination if we cant we fail at that point. if we can we store the pci address info in the port profile in teh  vif objects in the migration_data.\n\nhttps://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L49","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"a4e3b6124ca0e1662d50753deeecbd2024b80dff","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_843e3789","line":307,"in_reply_to":"7faddb67_044a555a","updated":"2019-08-27 13:41:05.000000000","message":"So are you saying that we check to see whether instance.pci_requests is empty before we get to compute manager for live migration?\n\nI guess I\u0027m asking because I\u0027m not sure why we get an empty PCIRequests for live-migration and make that assumption here, instead of just always getting it from the DB, even if just to grab/confirm an empty one. It makes me nervous that if the check was wrong or skipped or something that we\u0027d get here and use an empty object when we shouldn\u0027t.","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a2529102fd4d19800bd60aa78d9b3431913a863b","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_02e9edb5","line":307,"in_reply_to":"7faddb67_4207e5ca","updated":"2019-08-26 14:43:40.000000000","message":"We handle SRIOV VIFs in check_can_live_migrate_destination right after we do the live migration claim: https://review.opendev.org/#/c/634606/54/nova/compute/manager.py@6462\n\nSean\u0027s the authority here, so he can keep me honest.","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcfad7424635a4ebd27c525b222ea5b467f06aa0","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_6b31830a","line":307,"in_reply_to":"7faddb67_5f9ec969","updated":"2019-08-27 20:02:14.000000000","message":"testing this it does not work correctly.\nwe end up saving this and nulling out the pci requests in the instance_extra table","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"465df40cbd9fa97a946e21bb400ea1f2d61b83c4","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_5f9ec969","line":307,"in_reply_to":"7faddb67_843e3789","updated":"2019-08-27 16:18:07.000000000","message":"I\u0027m not entirely sure that I understand your question, but what I\u0027m doing here is giving the MoveClaim an empty pci_requests if this is a live migration, in order to avoid triggering [1], because SRIOV live migration is handled later [2], and we don\u0027t want the claim to refuse the migration based on pci_requests. Does the ordering around [2] change anything? Check vifs first, then do the claim, or do the claim, then check vifs? I suppose if we do the vifs first, once we get to the claim we would feel \"safer\" to ignore pci_requests?\n\n[1] https://review.opendev.org/#/c/635669/39/nova/compute/claims.py@146\n[2] https://review.opendev.org/#/c/634606/58/nova/compute/manager.py@6461","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"be48a61c94953b1c4e5f4151496cf2c78bf82bd0","unresolved":false,"context_lines":[{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"},{"line_number":311,"context_line":"            # pci device in the resize phase. In the future we would like to"},{"line_number":312,"context_line":"            # optimise this."},{"line_number":313,"context_line":"            new_pci_requests \u003d pci_request.get_pci_requests_from_flavor("},{"line_number":314,"context_line":"                new_instance_type)"},{"line_number":315,"context_line":"            new_pci_requests.instance_uuid \u003d instance.uuid"},{"line_number":316,"context_line":"            # On resize merge the SR-IOV ports pci_requests"},{"line_number":317,"context_line":"            # with the new instance flavor pci_requests."}],"source_content_type":"text/x-python","patch_set":39,"id":"7faddb67_0960300d","line":314,"range":{"start_line":313,"start_character":0,"end_line":314,"end_character":34},"updated":"2019-08-26 19:43:57.000000000","message":"if we dont already have a check somewhere for this we should be expcitly blocking live migration with flavor based pci passthough but that is not related to numa live migration.","commit_id":"0d2b0c76f86a2104d2218ea14c684c8d496d2bc9"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"fcfad7424635a4ebd27c525b222ea5b467f06aa0","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_6b1ae37c","line":307,"range":{"start_line":307,"start_character":8,"end_line":307,"end_character":67},"updated":"2019-08-27 20:02:14.000000000","message":"this does not work\nthe xml is update correctly\nhttp://paste.openstack.org/show/765671/\nand the numa info is in the migration context\nhttp://paste.openstack.org/show/765743/\n\nbut the pci_request in instance extra is empty\n\nmysql\u003e select pci_requests from  instance_extra as ie where ie.instance_uuid\u003d\u0027b304fedf-aa31-4990-9a74-29729eed336d\u0027;\n+--------------+\n| pci_requests |\n+--------------+\n| []           |\n+--------------+\n1 row in set (0.00 sec)","commit_id":"17a6c330e446b44731dafd0367a8466adb2c24a3"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"72d4ecddecb67341210a7366206e8de63be03c22","unresolved":false,"context_lines":[{"line_number":304,"context_line":"        # NOTE(artom) Bypass PCI requests if this is a live migration. Only"},{"line_number":305,"context_line":"        # Neutron SRIOV is supported for live migrations, and that\u0027s handled by"},{"line_number":306,"context_line":"        # _claim_pci_for_instance_vifs() in the compute manager."},{"line_number":307,"context_line":"        new_pci_requests \u003d objects.InstancePCIRequests(requests\u003d[])"},{"line_number":308,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":309,"context_line":"            # TODO(moshele): we are recreating the pci requests even if there"},{"line_number":310,"context_line":"            # was no change on resize. This will cause allocating the old/new"}],"source_content_type":"text/x-python","patch_set":43,"id":"7faddb67_562b0e32","line":307,"range":{"start_line":307,"start_character":8,"end_line":307,"end_character":67},"in_reply_to":"7faddb67_6b1ae37c","updated":"2019-08-27 20:21:07.000000000","message":"Yep, this was my concern and suspicion, and the subject of the earlier discussion on this patch. Thanks for confirming.\n\nWe need to figure out a way around clobbering this data in the database obviously. I also think we should be moving all of this claim stuff more in the direction of being the same for live and cold migrations. It sounds like the SRIOV migration stuff was done as a one-off, which means it\u0027s different from everything else, and thus this hack which we now know isn\u0027t okay.","commit_id":"17a6c330e446b44731dafd0367a8466adb2c24a3"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"64da00bf943e996c683edcfd608359ca357a3e53","unresolved":false,"context_lines":[{"line_number":314,"context_line":"            for request in instance.pci_requests.requests:"},{"line_number":315,"context_line":"                if request.source \u003d\u003d objects.InstancePCIRequest.NEUTRON_PORT:"},{"line_number":316,"context_line":"                    new_pci_requests.requests.append(request)"},{"line_number":317,"context_line":""},{"line_number":318,"context_line":"        claim \u003d claims.MoveClaim(context, instance, nodename,"},{"line_number":319,"context_line":"                                 new_instance_type, image_meta, self, cn,"},{"line_number":320,"context_line":"                                 new_pci_requests, migration, limits\u003dlimits)"}],"source_content_type":"text/x-python","patch_set":44,"id":"7faddb67_e35b571f","line":317,"updated":"2019-08-27 21:44:05.000000000","message":"Derp - next respin.","commit_id":"6e922e882dbbc2bd02343a024f23f375e6b9028f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":244,"context_line":"                                limits\u003dlimits)"},{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             limits):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_6ffbbabd","line":247,"range":{"start_line":247,"start_character":64,"end_line":247,"end_character":73},"updated":"2019-08-28 19:11:35.000000000","message":"This can be required and expected to be not None because we know conductor is going to create it for us.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             limits):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        :param context: The request context."}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_cf0c2eee","line":248,"range":{"start_line":248,"start_character":29,"end_line":248,"end_character":35},"updated":"2019-08-28 19:11:35.000000000","message":"nit: can\u0027t this be None in case the scheduler is bypassed like in the forced host live migration case?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":245,"context_line":""},{"line_number":246,"context_line":"    @utils.synchronized(COMPUTE_RESOURCE_SEMAPHORE)"},{"line_number":247,"context_line":"    def live_migration_claim(self, context, instance, nodename, migration,"},{"line_number":248,"context_line":"                             limits):"},{"line_number":249,"context_line":"        \"\"\"Builds a MoveClaim for a live migration."},{"line_number":250,"context_line":""},{"line_number":251,"context_line":"        :param context: The request context."}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_c3764540","line":248,"range":{"start_line":248,"start_character":29,"end_line":248,"end_character":35},"in_reply_to":"7faddb67_cf0c2eee","updated":"2019-08-29 00:54:47.000000000","message":"The way this will be wired in eventually, a None default is not required here, because the conductor will pass us something [1]. But you\u0027re right that said something could be None. However, all we do here is pass the limits to the MoveClaim, which is set up to handle limits being None [2]\n\n[1] https://review.opendev.org/#/c/634606/60/nova/conductor/tasks/live_migrate.py\n[2] https://review.opendev.org/#/c/635669/45/nova/compute/claims.py@99","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":312,"context_line":"        # with the new instance flavor pci_requests."},{"line_number":313,"context_line":"        if instance.pci_requests:"},{"line_number":314,"context_line":"            for request in instance.pci_requests.requests:"},{"line_number":315,"context_line":"                if request.source \u003d\u003d objects.InstancePCIRequest.NEUTRON_PORT:"},{"line_number":316,"context_line":"                    new_pci_requests.requests.append(request)"},{"line_number":317,"context_line":"        claim \u003d claims.MoveClaim(context, instance, nodename,"},{"line_number":318,"context_line":"                                 new_instance_type, image_meta, self, cn,"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_6fe97ae1","line":315,"range":{"start_line":315,"start_character":16,"end_line":315,"end_character":77},"updated":"2019-08-28 19:11:35.000000000","message":"Oh where have I seen this magical unicorn lately?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":319,"context_line":"                                 new_pci_requests, migration, limits\u003dlimits)"},{"line_number":320,"context_line":""},{"line_number":321,"context_line":"        claimed_pci_devices_objs \u003d []"},{"line_number":322,"context_line":"        # TODO(artom) The second part of this if should not be necessary, but"},{"line_number":323,"context_line":"        # since SRIOV live migration is currently handled elsewhere - see for"},{"line_number":324,"context_line":"        # example _claim_pci_for_instance_vifs() in the compute manager - we"},{"line_number":325,"context_line":"        # don\u0027t do any PCI claims if this is a live migration to avoid stepping"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_2fbdc2d6","line":322,"range":{"start_line":322,"start_character":46,"end_line":322,"end_character":48},"updated":"2019-08-28 19:11:35.000000000","message":"nit: condition","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":319,"context_line":"                                 new_pci_requests, migration, limits\u003dlimits)"},{"line_number":320,"context_line":""},{"line_number":321,"context_line":"        claimed_pci_devices_objs \u003d []"},{"line_number":322,"context_line":"        # TODO(artom) The second part of this if should not be necessary, but"},{"line_number":323,"context_line":"        # since SRIOV live migration is currently handled elsewhere - see for"},{"line_number":324,"context_line":"        # example _claim_pci_for_instance_vifs() in the compute manager - we"},{"line_number":325,"context_line":"        # don\u0027t do any PCI claims if this is a live migration to avoid stepping"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_03b3fdea","line":322,"range":{"start_line":322,"start_character":46,"end_line":322,"end_character":48},"in_reply_to":"7faddb67_2fbdc2d6","updated":"2019-08-29 00:54:47.000000000","message":"Done","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3213216d4f27799802ac31f54ad5d120c8e5d9d6","unresolved":false,"context_lines":[{"line_number":325,"context_line":"        # don\u0027t do any PCI claims if this is a live migration to avoid stepping"},{"line_number":326,"context_line":"        # on that code\u0027s toes. Ideally, MoveClaim/this method would be used for"},{"line_number":327,"context_line":"        # all live migration resource claims."},{"line_number":328,"context_line":"        if self.pci_tracker and migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":329,"context_line":"            # NOTE(jaypipes): ComputeNode.pci_device_pools is set below"},{"line_number":330,"context_line":"            # in _update_usage_from_instance()."},{"line_number":331,"context_line":"            claimed_pci_devices_objs \u003d self.pci_tracker.claim_instance("}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_066b16f1","line":328,"range":{"start_line":328,"start_character":28,"end_line":328,"end_character":76},"updated":"2019-08-28 14:21:15.000000000","message":"I don\u0027t see a test for this","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"3213216d4f27799802ac31f54ad5d120c8e5d9d6","unresolved":false,"context_lines":[{"line_number":329,"context_line":"            # NOTE(jaypipes): ComputeNode.pci_device_pools is set below"},{"line_number":330,"context_line":"            # in _update_usage_from_instance()."},{"line_number":331,"context_line":"            claimed_pci_devices_objs \u003d self.pci_tracker.claim_instance("},{"line_number":332,"context_line":"                    context, new_pci_requests, claim.claimed_numa_topology)"},{"line_number":333,"context_line":"        claimed_pci_devices \u003d objects.PciDeviceList("},{"line_number":334,"context_line":"                objects\u003dclaimed_pci_devices_objs)"},{"line_number":335,"context_line":""}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_86e2a69f","line":332,"updated":"2019-08-28 14:21:15.000000000","message":"So before, you were passing this an empty new_pci_requests which was effectively skipping this step for the instance, right? It seems like this change that you have now is *more* explicit in the \"we don\u0027t claim pci devices for live migration.\" If that\u0027s right, then that\u0027s an improvement, IMHO.\n\nHowever, just to be clear, we will still end up updating pci_tracker about the devices this instance ends up using, right? Does the \"SRIOV migration alternate reality code\" end up calling into this elsewhere to ensure the accounting is correct?","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"01b970fa993c93d3eeba497fb593a47e35ee93ee","unresolved":false,"context_lines":[{"line_number":329,"context_line":"            # NOTE(jaypipes): ComputeNode.pci_device_pools is set below"},{"line_number":330,"context_line":"            # in _update_usage_from_instance()."},{"line_number":331,"context_line":"            claimed_pci_devices_objs \u003d self.pci_tracker.claim_instance("},{"line_number":332,"context_line":"                    context, new_pci_requests, claim.claimed_numa_topology)"},{"line_number":333,"context_line":"        claimed_pci_devices \u003d objects.PciDeviceList("},{"line_number":334,"context_line":"                objects\u003dclaimed_pci_devices_objs)"},{"line_number":335,"context_line":""}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_af8d3236","line":332,"in_reply_to":"7faddb67_86e2a69f","updated":"2019-08-28 19:11:35.000000000","message":"\u003e So before, you were passing this an empty new_pci_requests which\n \u003e was effectively skipping this step for the instance, right? It\n \u003e seems like this change that you have now is *more* explicit in the\n \u003e \"we don\u0027t claim pci devices for live migration.\" If that\u0027s right,\n \u003e then that\u0027s an improvement, IMHO.\n\nBy before you mean this right?\n\nhttps://review.opendev.org/#/c/635669/43/nova/compute/resource_tracker.py@307\n\nDefinitely agree that being more explicit is better - clunky, but better.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"efbdb29a543f6fdfb4b17bc4d0ad4415367bf306","unresolved":false,"context_lines":[{"line_number":329,"context_line":"            # NOTE(jaypipes): ComputeNode.pci_device_pools is set below"},{"line_number":330,"context_line":"            # in _update_usage_from_instance()."},{"line_number":331,"context_line":"            claimed_pci_devices_objs \u003d self.pci_tracker.claim_instance("},{"line_number":332,"context_line":"                    context, new_pci_requests, claim.claimed_numa_topology)"},{"line_number":333,"context_line":"        claimed_pci_devices \u003d objects.PciDeviceList("},{"line_number":334,"context_line":"                objects\u003dclaimed_pci_devices_objs)"},{"line_number":335,"context_line":""}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_f97e9c68","line":332,"in_reply_to":"7faddb67_86e2a69f","updated":"2019-08-28 16:46:14.000000000","message":"Yeah, the SRIOV live migration claiming of PCI devices is done in https://review.opendev.org/#/c/634606/60/nova/compute/manager.py@6458, called from https://review.opendev.org/#/c/634606/60/nova/compute/manager.py@6465","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a074c78e5b9185f8ad713d708ae2ba193f948ab6","unresolved":false,"context_lines":[{"line_number":393,"context_line":"        migration.dest_node \u003d nodename"},{"line_number":394,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":395,"context_line":"        # NOTE(artom) Live-migrations don\u0027t have the pre-migrating state."},{"line_number":396,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":397,"context_line":"            migration.status \u003d \u0027pre-migrating\u0027"},{"line_number":398,"context_line":"        migration.save()"},{"line_number":399,"context_line":""}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_2fde2210","line":396,"updated":"2019-08-28 19:21:09.000000000","message":"I don\u0027t see a test for this, and since the migration status phases are so undefined I probably would have never noticed. This is about as close as I think we have to a documented migration state machine:\n\nhttps://specs.openstack.org/openstack/nova-specs/specs/rocky/approved/list-show-all-server-migration-types.html#proposed-change\n\nSo the only time we\u0027ll see pre-migrating is resize, cold migration or evacuate.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"a7bdadfe937cc0c6c84a25ceb59f0924b37d091c","unresolved":false,"context_lines":[{"line_number":393,"context_line":"        migration.dest_node \u003d nodename"},{"line_number":394,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":395,"context_line":"        # NOTE(artom) Live-migrations don\u0027t have the pre-migrating state."},{"line_number":396,"context_line":"        if migration.migration_type !\u003d \u0027live-migration\u0027:"},{"line_number":397,"context_line":"            migration.status \u003d \u0027pre-migrating\u0027"},{"line_number":398,"context_line":"        migration.save()"},{"line_number":399,"context_line":""}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_23607935","line":396,"in_reply_to":"7faddb67_2fde2210","updated":"2019-08-29 00:54:47.000000000","message":"https://review.opendev.org/#/c/635669/45/nova/tests/unit/compute/test_resource_tracker.py@2844 was supposed to be it, but I made a mistake and wrote claim.migration.migration_type instead of claim.migration.status. It also led me down a mini rabbit whole of where exactly we set the status for live migration Migration objects, which I\u0027ve added to the NOTE on L395.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"518c8f77ecd1aa36c1ed8adebd7c5693138f1315","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":396,"context_line":"        # NOTE(artom) Migration objects for live migrations are created with"},{"line_number":397,"context_line":"        # status \u0027accepted\u0027 by the conductor in live_migrate_instance(). We"},{"line_number":398,"context_line":"        # technically shouldn\u0027t need to set it again here, but explicitly"},{"line_number":399,"context_line":"        # covering all possibilities is deemed worth it."},{"line_number":400,"context_line":"        if migration.migration_type \u003d\u003d \u0027live-migration\u0027:"},{"line_number":401,"context_line":"            migration.status \u003d \u0027accepted\u0027"},{"line_number":402,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_a7469520","line":399,"range":{"start_line":398,"start_character":59,"end_line":399,"end_character":56},"updated":"2019-08-30 14:10:31.000000000","message":"I\u0027m not really sure this is better. If I\u0027m going looking for where we set a migration to accepted status, I\u0027ll find this one and the other one. If I find this one first, I might not keep looking to find the other one, and surmise that this is where the status actually changes. Not sure it\u0027s worth a respin right now, but I don\u0027t really think it\u0027s better like this.","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f889f0a826806aec24584472ac83767e730b78de","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":396,"context_line":"        # NOTE(artom) Migration objects for live migrations are created with"},{"line_number":397,"context_line":"        # status \u0027accepted\u0027 by the conductor in live_migrate_instance(). We"},{"line_number":398,"context_line":"        # technically shouldn\u0027t need to set it again here, but explicitly"},{"line_number":399,"context_line":"        # covering all possibilities is deemed worth it."},{"line_number":400,"context_line":"        if migration.migration_type \u003d\u003d \u0027live-migration\u0027:"},{"line_number":401,"context_line":"            migration.status \u003d \u0027accepted\u0027"},{"line_number":402,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_24bb80e5","line":399,"range":{"start_line":398,"start_character":59,"end_line":399,"end_character":56},"in_reply_to":"7faddb67_242dc045","updated":"2019-08-30 15:36:56.000000000","message":"\u003e we\u0027d have to assert \"\u0027status\u0027 not in migration\u0027\n\nNo you don\u0027t - just use a Migration object with a status already set, like you would in the real world (set it to \u0027accepted\u0027 and assert it\u0027s not change to \u0027pre-migrating\u0027 in the test).","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"7e9a46028a48697324f4c51775843902fc9e4f1f","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":396,"context_line":"        # NOTE(artom) Migration objects for live migrations are created with"},{"line_number":397,"context_line":"        # status \u0027accepted\u0027 by the conductor in live_migrate_instance(). We"},{"line_number":398,"context_line":"        # technically shouldn\u0027t need to set it again here, but explicitly"},{"line_number":399,"context_line":"        # covering all possibilities is deemed worth it."},{"line_number":400,"context_line":"        if migration.migration_type \u003d\u003d \u0027live-migration\u0027:"},{"line_number":401,"context_line":"            migration.status \u003d \u0027accepted\u0027"},{"line_number":402,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_4418fcd1","line":399,"range":{"start_line":398,"start_character":59,"end_line":399,"end_character":56},"in_reply_to":"7faddb67_24bb80e5","updated":"2019-08-30 16:04:27.000000000","message":"Err, yeah. Done.","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b1a010e1420c3834f123fc6ec2188143afe874b1","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":396,"context_line":"        # NOTE(artom) Migration objects for live migrations are created with"},{"line_number":397,"context_line":"        # status \u0027accepted\u0027 by the conductor in live_migrate_instance(). We"},{"line_number":398,"context_line":"        # technically shouldn\u0027t need to set it again here, but explicitly"},{"line_number":399,"context_line":"        # covering all possibilities is deemed worth it."},{"line_number":400,"context_line":"        if migration.migration_type \u003d\u003d \u0027live-migration\u0027:"},{"line_number":401,"context_line":"            migration.status \u003d \u0027accepted\u0027"},{"line_number":402,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_242dc045","line":399,"range":{"start_line":398,"start_character":59,"end_line":399,"end_character":56},"in_reply_to":"7faddb67_a2ff6332","updated":"2019-08-30 15:35:09.000000000","message":"Full disclosure: it makes the test on [1] seem less weird - without this logic, we\u0027d have to assert \"\u0027status\u0027 not in migration\u0027, which while technically correct for the unit test, makes no sense in the wider context. Looks we we hate this but not enough to remove, so I\u0027d like to leave this in.\n\n[1] https://review.opendev.org/#/c/635669/47/nova/tests/unit/compute/test_resource_tracker.py@2845","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":395,"context_line":"        migration.dest_host \u003d self.driver.get_host_ip_addr()"},{"line_number":396,"context_line":"        # NOTE(artom) Migration objects for live migrations are created with"},{"line_number":397,"context_line":"        # status \u0027accepted\u0027 by the conductor in live_migrate_instance(). We"},{"line_number":398,"context_line":"        # technically shouldn\u0027t need to set it again here, but explicitly"},{"line_number":399,"context_line":"        # covering all possibilities is deemed worth it."},{"line_number":400,"context_line":"        if migration.migration_type \u003d\u003d \u0027live-migration\u0027:"},{"line_number":401,"context_line":"            migration.status \u003d \u0027accepted\u0027"},{"line_number":402,"context_line":"        else:"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_a2ff6332","line":399,"range":{"start_line":398,"start_character":59,"end_line":399,"end_character":56},"in_reply_to":"7faddb67_a7469520","updated":"2019-08-30 15:15:51.000000000","message":"Yeah I don\u0027t really care for _claim_existing_migration where we\u0027re twiddling things on something already created that we\u0027re not really sure about.\n\nReally the only time this changes is for evacuate since the API creates the Migration with status \u0027accepted\u0027 and then this will update it to \u0027pre-migrating\u0027.\n\nFor resize and cold migrate, conductor creates the migration with status \u0027pre-migrating\u0027 which is just set again here.\n\nI think it\u0027d be cleaner if we removed this, since it\u0027s mostly vestigial for resize/cold migrate before migration-based allocations in Queens, but then we\u0027d still have to set the migration status to pre-migrating for evacuate which would be it\u0027s own special unicorn which sucks.\n\nAnyway, at least the comment helps, but I\u0027d probably just leave this lie you had it in PS45 logic-wise, meaning don\u0027t change the status for live-migration.","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"}],"nova/tests/unit/compute/test_claims.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"6387281ecadddb5c37b0d046388e9a02e0d07570","unresolved":false,"context_lines":[{"line_number":336,"context_line":"                self.context, self.instance, _NODENAME, instance_type,"},{"line_number":337,"context_line":"                image_meta, self.tracker, self.resources, requests,"},{"line_number":338,"context_line":"                limits\u003dlimits,"},{"line_number":339,"context_line":"                migration\u003dobjects.Migration(migration_type\u003d\u0027live-migration\u0027))"},{"line_number":340,"context_line":"        return get_claim()"},{"line_number":341,"context_line":""},{"line_number":342,"context_line":"    @mock.patch(\u0027nova.objects.Instance.drop_migration_context\u0027)"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_13042c49","line":339,"updated":"2019-08-21 16:35:50.000000000","message":"Doesn\u0027t this kinda change the behavior of all the tests in this test case to be live migration? Shouldn\u0027t we subclass this test with one that provides a live migration object and leave this one cold?","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"c95f30a54a0a3e55b264c2822b9733f8de0056ed","unresolved":false,"context_lines":[{"line_number":336,"context_line":"                self.context, self.instance, _NODENAME, instance_type,"},{"line_number":337,"context_line":"                image_meta, self.tracker, self.resources, requests,"},{"line_number":338,"context_line":"                limits\u003dlimits,"},{"line_number":339,"context_line":"                migration\u003dobjects.Migration(migration_type\u003d\u0027live-migration\u0027))"},{"line_number":340,"context_line":"        return get_claim()"},{"line_number":341,"context_line":""},{"line_number":342,"context_line":"    @mock.patch(\u0027nova.objects.Instance.drop_migration_context\u0027)"}],"source_content_type":"text/x-python","patch_set":36,"id":"7faddb67_6d5ec67a","line":339,"in_reply_to":"7faddb67_13042c49","updated":"2019-08-22 20:19:27.000000000","message":"Done.\n\nAnd writing a new test class is actually much easier than trying to work with the existing one. I have no idea why they were jumping through so many hoops.","commit_id":"7eb0ac45d3291858414e5342f8f3fa776d814f3d"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5c04ea54b282d1a461046c42722c5f0d198ee4ed","unresolved":false,"context_lines":[{"line_number":375,"context_line":"                objects.InstancePCIRequest(alias_name\u003d\u0027fake-alias\u0027)]),"},{"line_number":376,"context_line":"            objects.Migration(migration_type\u003d\u0027live-migration\u0027), None)"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"    def test_live_migration_page_size(self):"},{"line_number":379,"context_line":"        instance_type \u003d self._fake_instance_type()"},{"line_number":380,"context_line":"        instance \u003d self._fake_instance()"},{"line_number":381,"context_line":"        instance.numa_topology \u003d objects.InstanceNUMATopology("}],"source_content_type":"text/x-python","patch_set":50,"id":"5faad753_ecffe964","line":378,"range":{"start_line":378,"start_character":8,"end_line":378,"end_character":37},"updated":"2019-09-09 17:42:23.000000000","message":"note i think this code is correct but i have not manually checked this configurastion.\n\ni will redeploy a second settup on a hardware that supprot 1G page. my current test hardware only support 2MB pages","commit_id":"dac259b15a6507c2e4e274e3b8614f77c61f93e2"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"619cdf25b182109ef5045e4c056c88a2338fc568","unresolved":false,"context_lines":[{"line_number":375,"context_line":"                objects.InstancePCIRequest(alias_name\u003d\u0027fake-alias\u0027)]),"},{"line_number":376,"context_line":"            objects.Migration(migration_type\u003d\u0027live-migration\u0027), None)"},{"line_number":377,"context_line":""},{"line_number":378,"context_line":"    def test_live_migration_page_size(self):"},{"line_number":379,"context_line":"        instance_type \u003d self._fake_instance_type()"},{"line_number":380,"context_line":"        instance \u003d self._fake_instance()"},{"line_number":381,"context_line":"        instance.numa_topology \u003d objects.InstanceNUMATopology("}],"source_content_type":"text/x-python","patch_set":50,"id":"5faad753_8c4c35c8","line":378,"range":{"start_line":378,"start_character":8,"end_line":378,"end_character":37},"in_reply_to":"5faad753_ecffe964","updated":"2019-09-09 17:48:13.000000000","message":"I have a func test for it: https://review.opendev.org/#/c/672595/45/nova/tests/functional/libvirt/test_numa_live_migration.py@585","commit_id":"dac259b15a6507c2e4e274e3b8614f77c61f93e2"}],"nova/tests/unit/compute/test_resource_tracker.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7fb84c156badf8a359402f18ebf9015ee90d4ac5","unresolved":false,"context_lines":[{"line_number":3303,"context_line":"class TestIsTrackableMigration(test.NoDBTestCase):"},{"line_number":3304,"context_line":"    def test_true(self):"},{"line_number":3305,"context_line":"        mig \u003d objects.Migration()"},{"line_number":3306,"context_line":"        for mig_type in (\u0027resize\u0027, \u0027migration\u0027, \u0027evacuation\u0027,"},{"line_number":3307,"context_line":"                         \u0027live-migration\u0027):"},{"line_number":3308,"context_line":"            mig.migration_type \u003d mig_type"},{"line_number":3309,"context_line":""},{"line_number":3310,"context_line":"            self.assertTrue(resource_tracker._is_trackable_migration(mig))"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_9902446f","line":3307,"range":{"start_line":3306,"start_character":0,"end_line":3307,"end_character":43},"updated":"2019-02-19 17:12:49.000000000","message":"This whole test case can go along with the function, I suspect","commit_id":"193bfbe26156368c2b101781bd742eb9d2ba826f"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"03a479ed989a400186129510ed6a2e8ac74a91b6","unresolved":false,"context_lines":[{"line_number":3303,"context_line":"class TestIsTrackableMigration(test.NoDBTestCase):"},{"line_number":3304,"context_line":"    def test_true(self):"},{"line_number":3305,"context_line":"        mig \u003d objects.Migration()"},{"line_number":3306,"context_line":"        for mig_type in (\u0027resize\u0027, \u0027migration\u0027, \u0027evacuation\u0027,"},{"line_number":3307,"context_line":"                         \u0027live-migration\u0027):"},{"line_number":3308,"context_line":"            mig.migration_type \u003d mig_type"},{"line_number":3309,"context_line":""},{"line_number":3310,"context_line":"            self.assertTrue(resource_tracker._is_trackable_migration(mig))"}],"source_content_type":"text/x-python","patch_set":12,"id":"9fdfeff1_1aa5b447","line":3307,"range":{"start_line":3306,"start_character":0,"end_line":3307,"end_character":43},"in_reply_to":"9fdfeff1_9902446f","updated":"2019-02-21 16:23:31.000000000","message":"Done","commit_id":"193bfbe26156368c2b101781bd742eb9d2ba826f"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"a074c78e5b9185f8ad713d708ae2ba193f948ab6","unresolved":false,"context_lines":[{"line_number":2837,"context_line":"            mock.patch.object(objects.Instance, \u0027save\u0027),"},{"line_number":2838,"context_line":"            mock.patch.object(self.rt, \u0027_update\u0027),"},{"line_number":2839,"context_line":"            mock.patch.object(self.rt.pci_tracker, \u0027claim_instance\u0027),"},{"line_number":2840,"context_line":"        ) as (mock_form_instance, mock_migration_save, mock_instance_save,"},{"line_number":2841,"context_line":"              mock_update, mock_pci_claim_instance):"},{"line_number":2842,"context_line":"            claim \u003d self.rt.live_migration_claim(ctxt, instance, _NODENAME,"},{"line_number":2843,"context_line":"                                                 migration, limits\u003dNone)"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_6fa5fa9e","line":2840,"range":{"start_line":2840,"start_character":19,"end_line":2840,"end_character":23},"updated":"2019-08-28 19:21:09.000000000","message":"from","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"efbdb29a543f6fdfb4b17bc4d0ad4415367bf306","unresolved":false,"context_lines":[{"line_number":2846,"context_line":"            self.assertIn(\u0027migration_context\u0027, instance)"},{"line_number":2847,"context_line":"            mock_update.assert_called_with("},{"line_number":2848,"context_line":"                mock.ANY, _COMPUTE_NODE_FIXTURES[0])"},{"line_number":2849,"context_line":"            mock_pci_claim_instance.assert_not_called()"},{"line_number":2850,"context_line":""},{"line_number":2851,"context_line":""},{"line_number":2852,"context_line":"class TestUpdateUsageFromMigration(test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_79df0c66","line":2849,"updated":"2019-08-28 16:46:14.000000000","message":"So this is the test you\u0027re not seeing - if we change \u0027live-migration\u0027 to \u0027migration\u0027 on L2830 (and mock out _update_usage_from_migrations because cold migrations are trackable and cause that method to be called), this assertion will fail. I did this locally before pushing, but by all means try this out to keep me honest.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"62e43e5420a14156b7cd873c5bd77e16862100e6","unresolved":false,"context_lines":[{"line_number":2846,"context_line":"            self.assertIn(\u0027migration_context\u0027, instance)"},{"line_number":2847,"context_line":"            mock_update.assert_called_with("},{"line_number":2848,"context_line":"                mock.ANY, _COMPUTE_NODE_FIXTURES[0])"},{"line_number":2849,"context_line":"            mock_pci_claim_instance.assert_not_called()"},{"line_number":2850,"context_line":""},{"line_number":2851,"context_line":""},{"line_number":2852,"context_line":"class TestUpdateUsageFromMigration(test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":45,"id":"7faddb67_794dcc39","line":2849,"in_reply_to":"7faddb67_79df0c66","updated":"2019-08-28 16:48:53.000000000","message":"Ah, thanks.","commit_id":"605aeec28ca1f40efc80b5b098abffda62cb24bd"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"44e3034e45e8147f53ced4e289b66e7e401e020c","unresolved":false,"context_lines":[{"line_number":2841,"context_line":"              mock_update, mock_pci_claim_instance):"},{"line_number":2842,"context_line":"            claim \u003d self.rt.live_migration_claim(ctxt, instance, _NODENAME,"},{"line_number":2843,"context_line":"                                                 migration, limits\u003dNone)"},{"line_number":2844,"context_line":"            self.assertEqual(claim.migration.id, 42)"},{"line_number":2845,"context_line":"            self.assertEqual(claim.migration.status, \u0027accepted\u0027)"},{"line_number":2846,"context_line":"            self.assertIn(\u0027migration_context\u0027, instance)"},{"line_number":2847,"context_line":"            mock_update.assert_called_with("},{"line_number":2848,"context_line":"                mock.ANY, _COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_a28443b2","line":2845,"range":{"start_line":2844,"start_character":12,"end_line":2845,"end_character":64},"updated":"2019-08-30 15:15:51.000000000","message":"nit: swap these args (expected, actual) - it matters in the MismatchError message if this assertion fails.","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"b1a010e1420c3834f123fc6ec2188143afe874b1","unresolved":false,"context_lines":[{"line_number":2841,"context_line":"              mock_update, mock_pci_claim_instance):"},{"line_number":2842,"context_line":"            claim \u003d self.rt.live_migration_claim(ctxt, instance, _NODENAME,"},{"line_number":2843,"context_line":"                                                 migration, limits\u003dNone)"},{"line_number":2844,"context_line":"            self.assertEqual(claim.migration.id, 42)"},{"line_number":2845,"context_line":"            self.assertEqual(claim.migration.status, \u0027accepted\u0027)"},{"line_number":2846,"context_line":"            self.assertIn(\u0027migration_context\u0027, instance)"},{"line_number":2847,"context_line":"            mock_update.assert_called_with("},{"line_number":2848,"context_line":"                mock.ANY, _COMPUTE_NODE_FIXTURES[0])"}],"source_content_type":"text/x-python","patch_set":47,"id":"7faddb67_641658f5","line":2845,"range":{"start_line":2844,"start_character":12,"end_line":2845,"end_character":64},"in_reply_to":"7faddb67_a28443b2","updated":"2019-08-30 15:35:09.000000000","message":"Done","commit_id":"d5b21adef02e42a49eaa864b9039b5daa18a2d27"}]}