)]}'
{"/PATCHSET_LEVEL":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"116f167d_fac28ada","updated":"2022-07-11 10:45:04.000000000","message":"there likely is enough here to consider a respin but i dont think its strictly required. im going to continute code review fo the  later patches and we can revisit this but over all this looks ok to me.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"09c9f766fafd902fee65f4cb0d7cb7c6d3afde00","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":18,"id":"c55a6aec_8846cb80","updated":"2022-08-19 11:59:34.000000000","message":"Tidy 👌","commit_id":"4b877ba1394a94a09b790b05c7d6b8f6ed3e50b5"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"f12e581587e4495a973a38231803adf85e9d7ec7","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"43ffd593_89bbd13b","updated":"2022-08-25 19:08:07.000000000","message":"recheck parent landed","commit_id":"953f1eef19139a6b7b47d56802f211008ca72e7c"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"f2a0cb8fba23cf9824a1ff2b3e77e13ba7245785","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":20,"id":"7329433e_4501fc5d","updated":"2022-08-25 11:27:50.000000000","message":"simple rebase readding +w","commit_id":"953f1eef19139a6b7b47d56802f211008ca72e7c"}],"doc/source/admin/pci-passthrough.rst":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"09c9f766fafd902fee65f4cb0d7cb7c6d3afde00","unresolved":true,"context_lines":[{"line_number":344,"context_line":""},{"line_number":345,"context_line":"PCI tracking in Placement"},{"line_number":346,"context_line":"-------------------------"},{"line_number":347,"context_line":"Since nova 26.0.0 (Zed) PCI passthrough device inventories are tracked in"},{"line_number":348,"context_line":"Placement. If a PCI device exists on the hypervisor and"},{"line_number":349,"context_line":"matches one of the device specifications configured via"},{"line_number":350,"context_line":":oslo.config:option:`pci.device_spec` then Placement will have a representation"}],"source_content_type":"text/x-rst","patch_set":18,"id":"47447a31_d7c70174","line":347,"updated":"2022-08-19 11:59:34.000000000","message":"This would make a really good \u0027versionchanged\u0027 directive at the top of this file. Something like:\n\n  .. versionchanged:: 26.0.0 (Zed)\n\n      PCI passthrough device inventories are now tracked in Placement. For more\n      information, refer to :ref:`pci-tracking-in-placement`.\n\n(You\u0027d need to add an anchor here if you use that verbatim, obviously)","commit_id":"4b877ba1394a94a09b790b05c7d6b8f6ed3e50b5"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"853153b06edcbddf8a32a72637758a2b2a5130cd","unresolved":false,"context_lines":[{"line_number":344,"context_line":""},{"line_number":345,"context_line":"PCI tracking in Placement"},{"line_number":346,"context_line":"-------------------------"},{"line_number":347,"context_line":"Since nova 26.0.0 (Zed) PCI passthrough device inventories are tracked in"},{"line_number":348,"context_line":"Placement. If a PCI device exists on the hypervisor and"},{"line_number":349,"context_line":"matches one of the device specifications configured via"},{"line_number":350,"context_line":":oslo.config:option:`pci.device_spec` then Placement will have a representation"}],"source_content_type":"text/x-rst","patch_set":18,"id":"803803cf_74fd83ad","line":347,"in_reply_to":"47447a31_d7c70174","updated":"2022-08-30 11:20:06.000000000","message":"Done in a FUP","commit_id":"4b877ba1394a94a09b790b05c7d6b8f6ed3e50b5"}],"nova/compute/pci_placement_translator.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":32,"context_line":"# Devs with these type need to have a parent and that parent is the one"},{"line_number":33,"context_line":"# that mapped  to a placement RP"},{"line_number":34,"context_line":"CHILD_TYPES \u003d ("},{"line_number":35,"context_line":"    fields.PciDeviceType.SRIOV_VF, fields.PciDeviceType.VDPA)"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def _is_placement_tracking_enabled() -\u003e bool:"}],"source_content_type":"text/x-python","patch_set":9,"id":"2e7690ca_6a0661d4","line":35,"range":{"start_line":35,"start_character":35,"end_line":35,"end_character":60},"updated":"2022-07-11 10:45:04.000000000","message":"ack, \nthis is good from a future-proofing point of view but is not strictly needed\nsince vdpa devices can only be requested via a neutron port today and not via the PCI alias, you don\u0027t strictly have to support that initially.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":32,"context_line":"# Devs with these type need to have a parent and that parent is the one"},{"line_number":33,"context_line":"# that mapped  to a placement RP"},{"line_number":34,"context_line":"CHILD_TYPES \u003d ("},{"line_number":35,"context_line":"    fields.PciDeviceType.SRIOV_VF, fields.PciDeviceType.VDPA)"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def _is_placement_tracking_enabled() -\u003e bool:"}],"source_content_type":"text/x-python","patch_set":9,"id":"81d58289_04f9d3a3","line":35,"range":{"start_line":35,"start_character":35,"end_line":35,"end_character":60},"in_reply_to":"2e7690ca_6a0661d4","updated":"2022-07-19 18:45:38.000000000","message":"Ack","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def _is_placement_tracking_enabled() -\u003e bool:"},{"line_number":39,"context_line":"    # This know false to act as a feature flag while we develop the feature"},{"line_number":40,"context_line":"    # step by step. It will be replaced with a config check when the feature is"},{"line_number":41,"context_line":"    # ready for production."},{"line_number":42,"context_line":"    #"}],"source_content_type":"text/x-python","patch_set":9,"id":"3097d8d2_13c72f27","line":39,"range":{"start_line":39,"start_character":5,"end_line":39,"end_character":21},"updated":"2022-07-11 10:45:04.000000000","message":"nit: This is false","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def _is_placement_tracking_enabled() -\u003e bool:"},{"line_number":39,"context_line":"    # This know false to act as a feature flag while we develop the feature"},{"line_number":40,"context_line":"    # step by step. It will be replaced with a config check when the feature is"},{"line_number":41,"context_line":"    # ready for production."},{"line_number":42,"context_line":"    #"}],"source_content_type":"text/x-python","patch_set":9,"id":"bc4ca481_5fb8d763","line":39,"range":{"start_line":39,"start_character":5,"end_line":39,"end_character":21},"in_reply_to":"3097d8d2_13c72f27","updated":"2022-07-19 18:45:38.000000000","message":"Done","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":43,"context_line":"    # return CONF.pci.report_in_placement"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    # Test code will mock this function to enable the feature in the test env"},{"line_number":46,"context_line":"    return False"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"}],"source_content_type":"text/x-python","patch_set":9,"id":"a7d7048a_b4e354c1","line":46,"range":{"start_line":46,"start_character":11,"end_line":46,"end_character":16},"updated":"2022-07-11 10:45:04.000000000","message":"just so you dont have too change this locally i would have been tempted to\neither make this an env parmater or add the config option befoer this and default it to false.\n\nbut this is fine too i guess.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":43,"context_line":"    # return CONF.pci.report_in_placement"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":"    # Test code will mock this function to enable the feature in the test env"},{"line_number":46,"context_line":"    return False"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"}],"source_content_type":"text/x-python","patch_set":9,"id":"b7ced02f_30bad772","line":46,"range":{"start_line":46,"start_character":11,"end_line":46,"end_character":16},"in_reply_to":"a7d7048a_b4e354c1","updated":"2022-07-19 18:45:38.000000000","message":"I debated this myself too. I rejected adding the config early as that would mean somebody might try to turn it to True before the code can produce a consistent result. \n\nI can add an ENV param if we want to test this separately in devstack. Right now I have the config adding patch on top of the series, so if we pull that down then this is already configurable in devstack.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":46,"context_line":"    return False"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"},{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"a9a89854_104b496a","line":49,"range":{"start_line":49,"start_character":4,"end_line":49,"end_character":19},"updated":"2022-07-11 10:45:04.000000000","message":"i kind of feel like this could be in the PciDevice object as a property.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8304a32dfefaaa9ad4ffebe016e0d2f4fc2a2e17","unresolved":false,"context_lines":[{"line_number":46,"context_line":"    return False"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"},{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"11ff8ef5_efde7761","line":49,"range":{"start_line":49,"start_character":4,"end_line":49,"end_character":19},"in_reply_to":"963926ac_c9a10477","updated":"2022-07-20 16:53:21.000000000","message":"Ack","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":true,"context_lines":[{"line_number":46,"context_line":"    return False"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"},{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"bcc4e68d_a35cb241","line":49,"range":{"start_line":49,"start_character":4,"end_line":49,"end_character":19},"in_reply_to":"a9a89854_104b496a","updated":"2022-07-19 18:45:38.000000000","message":"While right now this only depends on the PciDevice object, from the next patch where we add the config override for the resource class, this code will depend both on the PciDevice and on the PciDeviceSpec matched to the device. \n// later after reading the comments on the later patches too\nWhile I understand your suggestion that it would be better to connect the PciDevice with the PciDeviceSpec and handle _get_rc_for_dev via PciDevice directly. I think that is a major change potentially affecting the legacy behavior. So I would like to keep _get_rc_for_dev separate from PciDevice for now.\n\nI can try to do a refactor top of this series to see how bad would be to do the move but I would like to keep that trial independent from the main thread of this work for now.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc714981f9ed471ed160b54fcc31ff678f6d635e","unresolved":true,"context_lines":[{"line_number":46,"context_line":"    return False"},{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"},{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"963926ac_c9a10477","line":49,"range":{"start_line":49,"start_character":4,"end_line":49,"end_character":19},"in_reply_to":"bcc4e68d_a35cb241","updated":"2022-07-20 09:54:57.000000000","message":"latere in the series i think i start finding this more objectionable as it gets used more and expanded.\n\nso long term i would prefer to clean this up but it coudl be done later in the seriese.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"},{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"class PciResourceProvider:"}],"source_content_type":"text/x-python","patch_set":9,"id":"45fa96c3_48f67655","line":50,"range":{"start_line":50,"start_character":11,"end_line":50,"end_character":57},"updated":"2022-07-11 10:45:04.000000000","message":"this is the default RC but i think your missing gettign the RC if its set in the config?\n\nin such a case it woudl be a generic tag on the devsepc and rather then havign that logic being external to the class i think makeing this a propery that will\ncheck if the RC tag is present in teh devspec or fallback to this as the default is a better approch.\ndo you have the support for the operator provieded RC in a later patch or elsewhere in this patch?","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":47,"context_line":""},{"line_number":48,"context_line":""},{"line_number":49,"context_line":"def _get_rc_for_dev(dev: pci_device.PciDevice) -\u003e str:"},{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"class PciResourceProvider:"}],"source_content_type":"text/x-python","patch_set":9,"id":"a94cdc86_d9e9c500","line":50,"range":{"start_line":50,"start_character":11,"end_line":50,"end_character":57},"in_reply_to":"45fa96c3_48f67655","updated":"2022-07-19 18:45:38.000000000","message":"Support for config driven RC name is added in the next patch https://review.opendev.org/c/openstack/nova/+/846218/9/nova/compute/pci_placement_translator.py#95","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"class PciResourceProvider:"},{"line_number":54,"context_line":"    \"\"\"A PCI Resource Provider\"\"\""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"}],"source_content_type":"text/x-python","patch_set":9,"id":"7b1028b6_8c34ce40","line":53,"range":{"start_line":53,"start_character":6,"end_line":53,"end_character":25},"updated":"2022-07-11 10:45:04.000000000","message":"cool do we normally create a class to model a provider like this?\n\nin any case, this works for me as it nicely encapsulates things.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":50,"context_line":"    return f\"CUSTOM_PCI_{dev.vendor_id}_{dev.product_id}\""},{"line_number":51,"context_line":""},{"line_number":52,"context_line":""},{"line_number":53,"context_line":"class PciResourceProvider:"},{"line_number":54,"context_line":"    \"\"\"A PCI Resource Provider\"\"\""},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"}],"source_content_type":"text/x-python","patch_set":9,"id":"8920e5f5_a93c408a","line":53,"range":{"start_line":53,"start_character":6,"end_line":53,"end_character":25},"in_reply_to":"7b1028b6_8c34ce40","updated":"2022-07-19 18:45:38.000000000","message":"No, we don\u0027t have such model for other devices in nova. But as you observed I needed a place where I encapsulate logic that does the mapping between one or more PCI child devs and an RP, or a single PCI parent dev and a single PR.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"},{"line_number":57,"context_line":"        self.name \u003d name"},{"line_number":58,"context_line":"        self.parent_dev \u003d None"},{"line_number":59,"context_line":"        self.children_devs: ty.List[pci_device.PciDevice] \u003d []"},{"line_number":60,"context_line":"        self.resource_class: ty.Optional[str] \u003d None"},{"line_number":61,"context_line":"        self.traits: ty.Optional[ty.Set[str]] \u003d None"}],"source_content_type":"text/x-python","patch_set":9,"id":"5558adf8_f19226c3","line":58,"updated":"2022-07-11 10:45:04.000000000","message":"do you also want to have a nullable parent_rp","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc714981f9ed471ed160b54fcc31ff678f6d635e","unresolved":true,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"},{"line_number":57,"context_line":"        self.name \u003d name"},{"line_number":58,"context_line":"        self.parent_dev \u003d None"},{"line_number":59,"context_line":"        self.children_devs: ty.List[pci_device.PciDevice] \u003d []"},{"line_number":60,"context_line":"        self.resource_class: ty.Optional[str] \u003d None"},{"line_number":61,"context_line":"        self.traits: ty.Optional[ty.Set[str]] \u003d None"}],"source_content_type":"text/x-python","patch_set":9,"id":"48efc981_1f68e21f","line":58,"in_reply_to":"162437d7_15856366","updated":"2022-07-20 09:54:57.000000000","message":"ack.  i was really jsut wondering if you wanted to pass in a reference to an already constuced part resouce provider or provider uuid but ok we dont need it right now","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8304a32dfefaaa9ad4ffebe016e0d2f4fc2a2e17","unresolved":false,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"},{"line_number":57,"context_line":"        self.name \u003d name"},{"line_number":58,"context_line":"        self.parent_dev \u003d None"},{"line_number":59,"context_line":"        self.children_devs: ty.List[pci_device.PciDevice] \u003d []"},{"line_number":60,"context_line":"        self.resource_class: ty.Optional[str] \u003d None"},{"line_number":61,"context_line":"        self.traits: ty.Optional[ty.Set[str]] \u003d None"}],"source_content_type":"text/x-python","patch_set":9,"id":"555fc01b_e722abb5","line":58,"in_reply_to":"48efc981_1f68e21f","updated":"2022-07-20 16:53:21.000000000","message":"Done in a separate patch on top of the series.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":true,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"},{"line_number":57,"context_line":"        self.name \u003d name"},{"line_number":58,"context_line":"        self.parent_dev \u003d None"},{"line_number":59,"context_line":"        self.children_devs: ty.List[pci_device.PciDevice] \u003d []"},{"line_number":60,"context_line":"        self.resource_class: ty.Optional[str] \u003d None"},{"line_number":61,"context_line":"        self.traits: ty.Optional[ty.Set[str]] \u003d None"}],"source_content_type":"text/x-python","patch_set":9,"id":"162437d7_15856366","line":58,"in_reply_to":"5558adf8_f19226c3","updated":"2022-07-19 18:45:38.000000000","message":"Right now every PCI RP is under the compute RP directly. So this is handled generically at L171-172 by the PlacementView.update_provider_tree() call. This way I don\u0027t have to push the same parent to all PciResourceProvider class. \n\nSure, one can argue that PciResourceProvider should create itself in provider_tree instead of assuming that PlacementView pre-creates it. I can move the creation to PciResourceProvider if you think that make the model more logical. (I would do it in a separate refactor on top of the current series to avoid some of the rebase pains :)","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":108,"context_line":"    def _get_rp_name_for_address(self, addr: str) -\u003e str:"},{"line_number":109,"context_line":"        return f\"{self.root_rp_name}_{addr.upper()}\""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def _get_rp(self, rp_name: str) -\u003e PciResourceProvider:"},{"line_number":112,"context_line":"        try:"},{"line_number":113,"context_line":"            return self.rps[rp_name]"},{"line_number":114,"context_line":"        except KeyError:"}],"source_content_type":"text/x-python","patch_set":9,"id":"fb55cc8b_f1710578","line":111,"range":{"start_line":111,"start_character":8,"end_line":111,"end_character":15},"updated":"2022-07-11 10:45:04.000000000","message":"nit: _get_or_create_rp","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":108,"context_line":"    def _get_rp_name_for_address(self, addr: str) -\u003e str:"},{"line_number":109,"context_line":"        return f\"{self.root_rp_name}_{addr.upper()}\""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def _get_rp(self, rp_name: str) -\u003e PciResourceProvider:"},{"line_number":112,"context_line":"        try:"},{"line_number":113,"context_line":"            return self.rps[rp_name]"},{"line_number":114,"context_line":"        except KeyError:"}],"source_content_type":"text/x-python","patch_set":9,"id":"74588990_f1952223","line":111,"range":{"start_line":111,"start_character":8,"end_line":111,"end_character":15},"in_reply_to":"fb55cc8b_f1710578","updated":"2022-07-19 18:45:38.000000000","message":"changed it to _ensure_rp now here (it was renamed later originally, but I pulled that change back to here where it belongs)","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":109,"context_line":"        return f\"{self.root_rp_name}_{addr.upper()}\""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def _get_rp(self, rp_name: str) -\u003e PciResourceProvider:"},{"line_number":112,"context_line":"        try:"},{"line_number":113,"context_line":"            return self.rps[rp_name]"},{"line_number":114,"context_line":"        except KeyError:"},{"line_number":115,"context_line":"            self.rps[rp_name] \u003d PciResourceProvider(rp_name)"},{"line_number":116,"context_line":"            return self._get_rp(rp_name)"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _add_child(self, dev: pci_device.PciDevice) -\u003e None:"},{"line_number":119,"context_line":"        if not dev.parent_addr:"}],"source_content_type":"text/x-python","patch_set":9,"id":"94e487c7_5481f734","line":116,"range":{"start_line":112,"start_character":2,"end_line":116,"end_character":40},"updated":"2022-07-11 10:45:04.000000000","message":"i would prefer to use set_default instead of catching the key error\nhttps://docs.python.org/3/library/stdtypes.html#dict.setdefault\n\nthe init fucntion of PciResourceProvider is cheap so i dont think we care\nabout the cost of consutcting the default.\n\nreturn self.rps.setdefault(rp_name,  PciResourceProvider(rp_name))\n\nalternitivly i would do if we dont want the constuctor overhead\n\nrp \u003d self.rps.get(rp_name)\nreturn (\n    rp if rp is not None else\n    self.rps.setdefault(rp_name,  PciResourceProvider(rp_name))\n)","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":109,"context_line":"        return f\"{self.root_rp_name}_{addr.upper()}\""},{"line_number":110,"context_line":""},{"line_number":111,"context_line":"    def _get_rp(self, rp_name: str) -\u003e PciResourceProvider:"},{"line_number":112,"context_line":"        try:"},{"line_number":113,"context_line":"            return self.rps[rp_name]"},{"line_number":114,"context_line":"        except KeyError:"},{"line_number":115,"context_line":"            self.rps[rp_name] \u003d PciResourceProvider(rp_name)"},{"line_number":116,"context_line":"            return self._get_rp(rp_name)"},{"line_number":117,"context_line":""},{"line_number":118,"context_line":"    def _add_child(self, dev: pci_device.PciDevice) -\u003e None:"},{"line_number":119,"context_line":"        if not dev.parent_addr:"}],"source_content_type":"text/x-python","patch_set":9,"id":"0c6428e5_c31672d8","line":116,"range":{"start_line":112,"start_character":2,"end_line":116,"end_character":40},"in_reply_to":"94e487c7_5481f734","updated":"2022-07-19 18:45:38.000000000","message":"yeah, good idea. the constructor is cheap so lets use setdefault","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":184,"context_line":"    inventories and allocations needs to exist in placement and create the"},{"line_number":185,"context_line":"    missing peaces."},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"    It returns True if not just the provider_tree but also allocations needed"},{"line_number":188,"context_line":"    to be changed."},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    :param allocations:"},{"line_number":191,"context_line":"            Dict of allocation data of the form:"}],"source_content_type":"text/x-python","patch_set":9,"id":"b39ee4fc_055f0bb1","line":188,"range":{"start_line":187,"start_character":4,"end_line":188,"end_character":18},"updated":"2022-07-11 10:45:04.000000000","message":"we might want to consider returning an enum instead if we think we ill need to\ndiffernciate between allcoations need healing and provider tree need to be created.\n\nbut i guess this works for now","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":184,"context_line":"    inventories and allocations needs to exist in placement and create the"},{"line_number":185,"context_line":"    missing peaces."},{"line_number":186,"context_line":""},{"line_number":187,"context_line":"    It returns True if not just the provider_tree but also allocations needed"},{"line_number":188,"context_line":"    to be changed."},{"line_number":189,"context_line":""},{"line_number":190,"context_line":"    :param allocations:"},{"line_number":191,"context_line":"            Dict of allocation data of the form:"}],"source_content_type":"text/x-python","patch_set":9,"id":"5c52a69f_e787b759","line":188,"range":{"start_line":187,"start_character":4,"end_line":188,"end_character":18},"in_reply_to":"b39ee4fc_055f0bb1","updated":"2022-07-19 18:45:38.000000000","message":"yeah. I defer this decision until there are real code modifying the allocation here.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"09c9f766fafd902fee65f4cb0d7cb7c6d3afde00","unresolved":false,"context_lines":[{"line_number":32,"context_line":"# Devs with these type need to have a parent and that parent is the one"},{"line_number":33,"context_line":"# that mapped  to a placement RP"},{"line_number":34,"context_line":"CHILD_TYPES \u003d ("},{"line_number":35,"context_line":"    fields.PciDeviceType.SRIOV_VF, fields.PciDeviceType.VDPA)"},{"line_number":36,"context_line":""},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"def _is_placement_tracking_enabled() -\u003e bool:"}],"source_content_type":"text/x-python","patch_set":18,"id":"45bb5cd6_a8a7940d","line":35,"updated":"2022-08-19 11:59:34.000000000","message":"nit: both of these would fit on one line I think","commit_id":"4b877ba1394a94a09b790b05c7d6b8f6ed3e50b5"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"09c9f766fafd902fee65f4cb0d7cb7c6d3afde00","unresolved":true,"context_lines":[{"line_number":55,"context_line":""},{"line_number":56,"context_line":"    def __init__(self, name: str) -\u003e None:"},{"line_number":57,"context_line":"        self.name \u003d name"},{"line_number":58,"context_line":"        self.parent_dev \u003d None"},{"line_number":59,"context_line":"        self.children_devs: ty.List[pci_device.PciDevice] \u003d []"},{"line_number":60,"context_line":"        self.resource_class: ty.Optional[str] \u003d None"},{"line_number":61,"context_line":"        self.traits: ty.Optional[ty.Set[str]] \u003d None"}],"source_content_type":"text/x-python","patch_set":18,"id":"43bdb547_c15ea983","line":58,"updated":"2022-08-19 11:59:34.000000000","message":"nit:\n\n  self.parent_dev: ty.Optional[pci_device.PciDevice] \u003d None","commit_id":"4b877ba1394a94a09b790b05c7d6b8f6ed3e50b5"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a768e2554190ee75db1f66495beda8dc496657ff","unresolved":true,"context_lines":[{"line_number":153,"context_line":"            # The device is allocated to an instance, so we need to make sure"},{"line_number":154,"context_line":"            # the device will be allocated to the instance in placement too"},{"line_number":155,"context_line":"            # FIXME(gibi): During migration the source host allocation should"},{"line_number":156,"context_line":"            # be tight to the migration_uuid as consumer in placement. But"},{"line_number":157,"context_line":"            # the PciDevice.instance_uuid is still pointing to the"},{"line_number":158,"context_line":"            # instance_uuid both on the source and the dest. So we need to"},{"line_number":159,"context_line":"            # check for running migrations."}],"source_content_type":"text/x-python","patch_set":19,"id":"a0917a1d_bce1950a","line":156,"updated":"2022-08-23 17:33:57.000000000","message":"nit: be tied to","commit_id":"ec21b0e89eeddaf91a079fa72f40a52bd6d59e5b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"853153b06edcbddf8a32a72637758a2b2a5130cd","unresolved":false,"context_lines":[{"line_number":153,"context_line":"            # The device is allocated to an instance, so we need to make sure"},{"line_number":154,"context_line":"            # the device will be allocated to the instance in placement too"},{"line_number":155,"context_line":"            # FIXME(gibi): During migration the source host allocation should"},{"line_number":156,"context_line":"            # be tight to the migration_uuid as consumer in placement. But"},{"line_number":157,"context_line":"            # the PciDevice.instance_uuid is still pointing to the"},{"line_number":158,"context_line":"            # instance_uuid both on the source and the dest. So we need to"},{"line_number":159,"context_line":"            # check for running migrations."}],"source_content_type":"text/x-python","patch_set":19,"id":"663b8908_1b8f9527","line":156,"in_reply_to":"a0917a1d_bce1950a","updated":"2022-08-30 11:20:06.000000000","message":"Dropped the whole conditional in a FUP the allocation handling is done in a separate call to the PlacementView.","commit_id":"ec21b0e89eeddaf91a079fa72f40a52bd6d59e5b"}],"nova/compute/resource_tracker.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":1269,"context_line":"        # PCI allocation without placement being involved until the prefilter"},{"line_number":1270,"context_line":"        # is enabled. So we need to be ready to heal PCI allocations at"},{"line_number":1271,"context_line":"        # every call not just at startup."},{"line_number":1272,"context_line":"        driver_reshaped \u003d (allocs is not None)"},{"line_number":1273,"context_line":"        if not allocs:"},{"line_number":1274,"context_line":"            # update_provider_tree_for_pci always needs the allocations to see"},{"line_number":1275,"context_line":"            # if something is missing"},{"line_number":1276,"context_line":"            allocs \u003d self.reportclient.get_allocations_for_provider_tree("},{"line_number":1277,"context_line":"                context, nodename)"},{"line_number":1278,"context_line":""},{"line_number":1279,"context_line":"        pci_reshaped \u003d pci_placement_translator.update_provider_tree_for_pci("},{"line_number":1280,"context_line":"            prov_tree, nodename, self.pci_tracker, allocs)"}],"source_content_type":"text/x-python","patch_set":9,"id":"87d41033_7b7c836f","line":1277,"range":{"start_line":1272,"start_character":7,"end_line":1277,"end_character":34},"updated":"2022-07-11 10:45:04.000000000","message":"if we always need them i would  remove \nallocs \u003d self.reportclient.get_allocations_for_provider_tree(\n                context, nodename)\n                \non line 1231 and move it to line 1220\n\nthere isnt really any point in defering getting the allcoations to here given all code paths will now use it in the succes case.\n\nand that will allow you to drop this block.\n\nyes techinally we will lookup the allocation when we would raise an excption because a reshape is needed but its not startup however it think that is ok\n\nsince that should be rare.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":1269,"context_line":"        # PCI allocation without placement being involved until the prefilter"},{"line_number":1270,"context_line":"        # is enabled. So we need to be ready to heal PCI allocations at"},{"line_number":1271,"context_line":"        # every call not just at startup."},{"line_number":1272,"context_line":"        driver_reshaped \u003d (allocs is not None)"},{"line_number":1273,"context_line":"        if not allocs:"},{"line_number":1274,"context_line":"            # update_provider_tree_for_pci always needs the allocations to see"},{"line_number":1275,"context_line":"            # if something is missing"},{"line_number":1276,"context_line":"            allocs \u003d self.reportclient.get_allocations_for_provider_tree("},{"line_number":1277,"context_line":"                context, nodename)"},{"line_number":1278,"context_line":""},{"line_number":1279,"context_line":"        pci_reshaped \u003d pci_placement_translator.update_provider_tree_for_pci("},{"line_number":1280,"context_line":"            prov_tree, nodename, self.pci_tracker, allocs)"}],"source_content_type":"text/x-python","patch_set":9,"id":"fd777b43_bd1c63e0","line":1277,"range":{"start_line":1272,"start_character":7,"end_line":1277,"end_character":34},"in_reply_to":"87d41033_7b7c836f","updated":"2022-07-19 18:45:38.000000000","message":"Make sense. Done.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":1279,"context_line":"        pci_reshaped \u003d pci_placement_translator.update_provider_tree_for_pci("},{"line_number":1280,"context_line":"            prov_tree, nodename, self.pci_tracker, allocs)"},{"line_number":1281,"context_line":""},{"line_number":1282,"context_line":"        if not driver_reshaped and not pci_reshaped:"},{"line_number":1283,"context_line":"            # reset allocs as we don\u0027t need to trigger reshape at"},{"line_number":1284,"context_line":"            # reportclient.update_from_provider_tree"},{"line_number":1285,"context_line":"            allocs \u003d None"},{"line_number":1286,"context_line":""},{"line_number":1287,"context_line":"        self.provider_tree \u003d prov_tree"},{"line_number":1288,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"7188091f_96e0a4cd","line":1285,"range":{"start_line":1282,"start_character":6,"end_line":1285,"end_character":25},"updated":"2022-07-11 10:45:04.000000000","message":"this feels easy to forget to do.\nbut ok.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":1279,"context_line":"        pci_reshaped \u003d pci_placement_translator.update_provider_tree_for_pci("},{"line_number":1280,"context_line":"            prov_tree, nodename, self.pci_tracker, allocs)"},{"line_number":1281,"context_line":""},{"line_number":1282,"context_line":"        if not driver_reshaped and not pci_reshaped:"},{"line_number":1283,"context_line":"            # reset allocs as we don\u0027t need to trigger reshape at"},{"line_number":1284,"context_line":"            # reportclient.update_from_provider_tree"},{"line_number":1285,"context_line":"            allocs \u003d None"},{"line_number":1286,"context_line":""},{"line_number":1287,"context_line":"        self.provider_tree \u003d prov_tree"},{"line_number":1288,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"c582b0dc_be161d9a","line":1285,"range":{"start_line":1282,"start_character":6,"end_line":1285,"end_character":25},"in_reply_to":"7188091f_96e0a4cd","updated":"2022-07-19 18:45:38.000000000","message":"This is a consequences of reportclient.update_from_provider_tree choosing between simple RP update or full reshape based on the value of allocs. I can move this code closer to the actual update_from_provider_tree call to make it more explicit.\n\nDone.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"}],"nova/tests/functional/libvirt/test_pci_in_placement.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        # There will be:"},{"line_number":79,"context_line":"        # * two type-PCI devs (slot 0 and 1)"},{"line_number":80,"context_line":"        # * two type-PFs (slot 2 and 3) with two type-VFs each"},{"line_number":81,"context_line":"        pci_info \u003d fakelibvirt.HostPCIDevicesInfo("},{"line_number":82,"context_line":"            num_pci\u003d2, num_pfs\u003d2, num_vfs\u003d4)"},{"line_number":83,"context_line":"        self.start_compute(hostname\u003d\"compute1\", pci_info\u003dpci_info)"}],"source_content_type":"text/x-python","patch_set":9,"id":"572a2bdf_84b36717","line":80,"range":{"start_line":80,"start_character":8,"end_line":80,"end_character":62},"updated":"2022-07-11 10:45:04.000000000","message":"# * two type-PFs (slot 2 and 3) with two type-VFs each\n\n# * two type-PFs (slot 2 and 3) one with and without VFs","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":true,"context_lines":[{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        # There will be:"},{"line_number":79,"context_line":"        # * two type-PCI devs (slot 0 and 1)"},{"line_number":80,"context_line":"        # * two type-PFs (slot 2 and 3) with two type-VFs each"},{"line_number":81,"context_line":"        pci_info \u003d fakelibvirt.HostPCIDevicesInfo("},{"line_number":82,"context_line":"            num_pci\u003d2, num_pfs\u003d2, num_vfs\u003d4)"},{"line_number":83,"context_line":"        self.start_compute(hostname\u003d\"compute1\", pci_info\u003dpci_info)"}],"source_content_type":"text/x-python","patch_set":9,"id":"adedf8c5_a4151631","line":80,"range":{"start_line":80,"start_character":8,"end_line":80,"end_character":62},"in_reply_to":"572a2bdf_84b36717","updated":"2022-07-19 18:45:38.000000000","message":"I\u0027m not sure what you suggest here. Should I add an extra PFs that has no VFs to the test case?","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc714981f9ed471ed160b54fcc31ff678f6d635e","unresolved":true,"context_lines":[{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        # There will be:"},{"line_number":79,"context_line":"        # * two type-PCI devs (slot 0 and 1)"},{"line_number":80,"context_line":"        # * two type-PFs (slot 2 and 3) with two type-VFs each"},{"line_number":81,"context_line":"        pci_info \u003d fakelibvirt.HostPCIDevicesInfo("},{"line_number":82,"context_line":"            num_pci\u003d2, num_pfs\u003d2, num_vfs\u003d4)"},{"line_number":83,"context_line":"        self.start_compute(hostname\u003d\"compute1\", pci_info\u003dpci_info)"}],"source_content_type":"text/x-python","patch_set":9,"id":"c82348a6_a5582308","line":80,"range":{"start_line":80,"start_character":8,"end_line":80,"end_character":62},"in_reply_to":"adedf8c5_a4151631","updated":"2022-07-20 09:54:57.000000000","message":"im saying the comment is wrong and you should chaange it form \n\n# * two type-PFs (slot 2 and 3) with two type-VFs each\n\nto \n\n\n# * two type-PFs (slot 2 and 3) one with and without VFs\n\nslot 0 and 1 are type pci\n                \"0000:81:00.0\": {self.PCI_RC: 1},\n                \"0000:81:01.0\": {self.PCI_RC: 1},\n                \nslot 2 is a PF with no VFS\n\"0000:81:02.0\": {self.PF_RC: 1},\nslot3 is a PF with 2 VFS\n# Note that the VF inventory is reported on the parent PF\n\"0000:81:03.0\": {self.VF_RC: 2},\n\nso this test does not constuct\n* two type-PFs (slot 2 and 3) with two type-VFs each\n                                                  ^\n                                                  each is the problem\nit constuct \ntwo type-PFs (slot 2 without VFs and 3 wit 2 VFs)\n\nso the test is fine and im not asking you to add a new testcase just asking you to fix the comment.\n\ni guess you were describing \n        pci_info \u003d fakelibvirt.HostPCIDevicesInfo(\n            num_pci\u003d2, num_pfs\u003d2, num_vfs\u003d4)\n            \nwhich will emulate   two type-PFs (slot 2 and 3) with two type-VFs each\n\nrahter then descibing what will be filtered by the device_spec but that was not clear when reading it orginally to me.\n\ni guess that should have been clear form context of where this comment is placed but i was expecting you to descirp the toptloy you were going to assert is reported.\n\nperhaps you cloud rework the test as follows \n\n        # The fake libvirt host will emulate:\n        # * two type-PCI devs (slot 0 and 1)\n        # * two type-PFs (slot 2 and 3) with two type-VFs each\n        pci_info \u003d fakelibvirt.HostPCIDevicesInfo(\n            num_pci\u003d2, num_pfs\u003d2, num_vfs\u003d4)\n            \n        # the emulated devies will then be filtered by the\n        # device_spec\n        device_spec \u003d self._to_device_spec_conf(\n            [\n                # type-PCI will match two devs (slot 0, 1)\n                {\n                    \"vendor_id\": fakelibvirt.PCI_VEND_ID,\n                    \"product_id\": fakelibvirt.PCI_PROD_ID,\n                },\n                # type-PF will match one PF\n                {\n                    \"vendor_id\": fakelibvirt.PCI_VEND_ID,\n                    \"product_id\": fakelibvirt.PF_PROD_ID,\n                    \"address\": \"0000:81:02.0\",\n                },\n                # type-VF will match two VFs but not their PFs\n                {\n                    \"vendor_id\": fakelibvirt.PCI_VEND_ID,\n                    \"product_id\": fakelibvirt.VF_PROD_ID,\n                    \"address\": \"0000:81:03.*\",\n                },\n            ]\n        )\n        self.flags(group\u003d\u0027pci\u0027, device_spec\u003ddevice_spec)\n        self.start_compute(hostname\u003d\"compute1\", pci_info\u003dpci_info)\n        \n        # Finally we assert that only the filtered devices are\n        # reported to placement.\n        self.assert_placement_pci_view(\n            \"compute1\",\n            inventories\u003d{\n                \"0000:81:00.0\": {self.PCI_RC: 1},\n                \"0000:81:01.0\": {self.PCI_RC: 1},\n                \"0000:81:02.0\": {self.PF_RC: 1},\n                # Note that the VF inventory is reported on the parent PF\n                \"0000:81:03.0\": {self.VF_RC: 2},\n            },\n            traits\u003d{\n                \"0000:81:00.0\": [],\n                \"0000:81:01.0\": [],\n                \"0000:81:02.0\": [],\n                \"0000:81:03.0\": [],\n            },\n        )\n        \ni think this ordering of the test woudl read better.\n\nthe logic you have currently is correct however.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"8304a32dfefaaa9ad4ffebe016e0d2f4fc2a2e17","unresolved":false,"context_lines":[{"line_number":77,"context_line":""},{"line_number":78,"context_line":"        # There will be:"},{"line_number":79,"context_line":"        # * two type-PCI devs (slot 0 and 1)"},{"line_number":80,"context_line":"        # * two type-PFs (slot 2 and 3) with two type-VFs each"},{"line_number":81,"context_line":"        pci_info \u003d fakelibvirt.HostPCIDevicesInfo("},{"line_number":82,"context_line":"            num_pci\u003d2, num_pfs\u003d2, num_vfs\u003d4)"},{"line_number":83,"context_line":"        self.start_compute(hostname\u003d\"compute1\", pci_info\u003dpci_info)"}],"source_content_type":"text/x-python","patch_set":9,"id":"ce8a03e5_8ce3e4fd","line":80,"range":{"start_line":80,"start_character":8,"end_line":80,"end_character":62},"in_reply_to":"c82348a6_a5582308","updated":"2022-07-20 16:53:21.000000000","message":"OK. I got your point now. I made the necessary clarification","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":97,"context_line":"                \"0000:81:02.0\": [],"},{"line_number":98,"context_line":"                \"0000:81:03.0\": [],"},{"line_number":99,"context_line":"            },"},{"line_number":100,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":9,"id":"09dd03a0_8fcdd6d7","line":100,"updated":"2022-07-11 10:45:04.000000000","message":"+1","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":97,"context_line":"                \"0000:81:02.0\": [],"},{"line_number":98,"context_line":"                \"0000:81:03.0\": [],"},{"line_number":99,"context_line":"            },"},{"line_number":100,"context_line":"        )"}],"source_content_type":"text/x-python","patch_set":9,"id":"6eb8e320_3c3b690d","line":100,"in_reply_to":"09dd03a0_8fcdd6d7","updated":"2022-07-19 18:45:38.000000000","message":"Ack","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c2afb5fcb70c6722f8f9200c7f3b7f2140f4a55d","unresolved":true,"context_lines":[{"line_number":33,"context_line":"    # Just placeholders to satisfy the base class. The real value will be"},{"line_number":34,"context_line":"    # redefined by the tests"},{"line_number":35,"context_line":"    PCI_DEVICE_SPEC \u003d []"},{"line_number":36,"context_line":"    PCI_ALIAS \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def setUp(self):"},{"line_number":39,"context_line":"        super().setUp()"}],"source_content_type":"text/x-python","patch_set":17,"id":"8fdde92f_1c72cea4","line":36,"updated":"2022-08-08 18:19:50.000000000","message":"set this to \"\" as that is the default. None is not a valid value and cause:\n  \n  HTTP exception thrown: Invalid PCI alias definition: \u0027NoneType\u0027 object is not iterable","commit_id":"5f4128b1882d85c2086ed8c66cac8d81c3ab59b0"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a768e2554190ee75db1f66495beda8dc496657ff","unresolved":true,"context_lines":[{"line_number":33,"context_line":"    # Just placeholders to satisfy the base class. The real value will be"},{"line_number":34,"context_line":"    # redefined by the tests"},{"line_number":35,"context_line":"    PCI_DEVICE_SPEC \u003d []"},{"line_number":36,"context_line":"    PCI_ALIAS \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def setUp(self):"},{"line_number":39,"context_line":"        super().setUp()"}],"source_content_type":"text/x-python","patch_set":17,"id":"fea61046_362de7ce","line":36,"in_reply_to":"8fdde92f_1c72cea4","updated":"2022-08-23 17:33:57.000000000","message":"so this still needs to be adressed but it can be fixed later as its just an inconsitency in the test setup.","commit_id":"5f4128b1882d85c2086ed8c66cac8d81c3ab59b0"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"853153b06edcbddf8a32a72637758a2b2a5130cd","unresolved":false,"context_lines":[{"line_number":33,"context_line":"    # Just placeholders to satisfy the base class. The real value will be"},{"line_number":34,"context_line":"    # redefined by the tests"},{"line_number":35,"context_line":"    PCI_DEVICE_SPEC \u003d []"},{"line_number":36,"context_line":"    PCI_ALIAS \u003d None"},{"line_number":37,"context_line":""},{"line_number":38,"context_line":"    def setUp(self):"},{"line_number":39,"context_line":"        super().setUp()"}],"source_content_type":"text/x-python","patch_set":17,"id":"9ce28889_a996c13f","line":36,"in_reply_to":"fea61046_362de7ce","updated":"2022-08-30 11:20:06.000000000","message":"Has already been fixed by a later patch in the series","commit_id":"5f4128b1882d85c2086ed8c66cac8d81c3ab59b0"}],"nova/tests/functional/libvirt/test_pci_sriov_servers.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"5f658d0cfb04304923e99fe8c96940851798d97c","unresolved":true,"context_lines":[{"line_number":84,"context_line":"        compute_rp_uuid \u003d self.compute_rp_uuids[hostname]"},{"line_number":85,"context_line":"        rps \u003d self._get_all_rps_in_a_tree(compute_rp_uuid)"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"        # we have the root provider as extra"},{"line_number":88,"context_line":"        self.assertEqual("},{"line_number":89,"context_line":"            len(inventories),"},{"line_number":90,"context_line":"            len(rps) - 1,"}],"source_content_type":"text/x-python","patch_set":9,"id":"2e3160e6_f61fd46d","line":87,"range":{"start_line":87,"start_character":8,"end_line":87,"end_character":44},"updated":"2022-07-11 10:45:04.000000000","message":"# we have the root provider as extra\n \n -\u003e\n \n  # rps also contains the root provider so we subtract 1","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":84,"context_line":"        compute_rp_uuid \u003d self.compute_rp_uuids[hostname]"},{"line_number":85,"context_line":"        rps \u003d self._get_all_rps_in_a_tree(compute_rp_uuid)"},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"        # we have the root provider as extra"},{"line_number":88,"context_line":"        self.assertEqual("},{"line_number":89,"context_line":"            len(inventories),"},{"line_number":90,"context_line":"            len(rps) - 1,"}],"source_content_type":"text/x-python","patch_set":9,"id":"c7ba721c_e17b8ef2","line":87,"range":{"start_line":87,"start_character":8,"end_line":87,"end_character":44},"in_reply_to":"2e3160e6_f61fd46d","updated":"2022-07-19 18:45:38.000000000","message":"Done","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"17db54ed44b695bd93a8e96b57004dc9f7ff8abf","unresolved":true,"context_lines":[{"line_number":1350,"context_line":""},{"line_number":1351,"context_line":"    def setUp(self):"},{"line_number":1352,"context_line":"        super().setUp()"},{"line_number":1353,"context_line":"        patcher \u003d mock.patch("},{"line_number":1354,"context_line":"            \"nova.compute.pci_placement_translator.\""},{"line_number":1355,"context_line":"            \"_is_placement_tracking_enabled\","},{"line_number":1356,"context_line":"            return_value\u003dTrue"},{"line_number":1357,"context_line":"        )"},{"line_number":1358,"context_line":"        self.addCleanup(patcher.stop)"},{"line_number":1359,"context_line":"        patcher.start()"},{"line_number":1360,"context_line":""},{"line_number":1361,"context_line":"    def test_create_server_with_pci_dev_and_numa(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"44be93f4_c63e84b9","line":1358,"range":{"start_line":1353,"start_character":7,"end_line":1358,"end_character":37},"updated":"2022-07-12 06:14:19.000000000","message":"by the way im not sure we want to hard code this to true.\n\nperhaps we do but i wonder if we want to duplicate the testing and only do this in a subclass?\n\na very high percentage fo the logic will be independed of placment tacking so im not conviced that we need to duplicate it with this set to true and false\nbut im wondering are we loosing coverage of the falase case taht we should maintian or if that wont really add value.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"dc714981f9ed471ed160b54fcc31ff678f6d635e","unresolved":false,"context_lines":[{"line_number":1350,"context_line":""},{"line_number":1351,"context_line":"    def setUp(self):"},{"line_number":1352,"context_line":"        super().setUp()"},{"line_number":1353,"context_line":"        patcher \u003d mock.patch("},{"line_number":1354,"context_line":"            \"nova.compute.pci_placement_translator.\""},{"line_number":1355,"context_line":"            \"_is_placement_tracking_enabled\","},{"line_number":1356,"context_line":"            return_value\u003dTrue"},{"line_number":1357,"context_line":"        )"},{"line_number":1358,"context_line":"        self.addCleanup(patcher.stop)"},{"line_number":1359,"context_line":"        patcher.start()"},{"line_number":1360,"context_line":""},{"line_number":1361,"context_line":"    def test_create_server_with_pci_dev_and_numa(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"39fe5186_36ab1dae","line":1358,"range":{"start_line":1353,"start_character":7,"end_line":1358,"end_character":37},"in_reply_to":"0cf805a4_e6ecd501","updated":"2022-07-20 09:54:57.000000000","message":"ok we likely have enough coverage then without runing in both configurations.\nbut if we have a regression in the future we might need to revisit","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"531cd405061eb18d069ce76b876d5d18d0d3cc74","unresolved":false,"context_lines":[{"line_number":1350,"context_line":""},{"line_number":1351,"context_line":"    def setUp(self):"},{"line_number":1352,"context_line":"        super().setUp()"},{"line_number":1353,"context_line":"        patcher \u003d mock.patch("},{"line_number":1354,"context_line":"            \"nova.compute.pci_placement_translator.\""},{"line_number":1355,"context_line":"            \"_is_placement_tracking_enabled\","},{"line_number":1356,"context_line":"            return_value\u003dTrue"},{"line_number":1357,"context_line":"        )"},{"line_number":1358,"context_line":"        self.addCleanup(patcher.stop)"},{"line_number":1359,"context_line":"        patcher.start()"},{"line_number":1360,"context_line":""},{"line_number":1361,"context_line":"    def test_create_server_with_pci_dev_and_numa(self):"}],"source_content_type":"text/x-python","patch_set":9,"id":"0cf805a4_e6ecd501","line":1358,"range":{"start_line":1353,"start_character":7,"end_line":1358,"end_character":37},"in_reply_to":"44be93f4_c63e84b9","updated":"2022-07-19 18:45:38.000000000","message":"The code is organized in a way that the flag only adds new logic, but does not remove any legacy logic. So, I don\u0027t think it adds much value to have test with disabled flag. If I modified the legacy pci tracking code under the flag then I would agree to duplicate the test code to see if legacy works with flag disabled.","commit_id":"eff0df6a9811c17cf780f0bb0502ab69a6d1478b"}]}