)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Stephen Finucane \u003csfinucan@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2018-07-03 16:50:17 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"hardware: Start accounting for networks in NUMA placement"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Modify the \u0027numa_fit_instance_to_host\u0027 function to take networks into"},{"line_number":10,"context_line":"account. We don\u0027t yet do anything with this at runtime, given that we\u0027re"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"5f7c97a3_18e1e5fc","line":7,"range":{"start_line":7,"start_character":48,"end_line":7,"end_character":57},"updated":"2018-07-05 14:36:01.000000000","message":"You might consider changing this to \"fitting\" or something. Placement is a loaded word you know, and I clicked on this patch expecting to see something very different :)","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"16ab4d243ff1b1da8bb8f43b46b911e53234e2d1","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Stephen Finucane \u003csfinucan@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2018-07-03 16:50:17 +0100"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"hardware: Start accounting for networks in NUMA placement"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Modify the \u0027numa_fit_instance_to_host\u0027 function to take networks into"},{"line_number":10,"context_line":"account. We don\u0027t yet do anything with this at runtime, given that we\u0027re"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"5f7c97a3_14675bec","line":7,"range":{"start_line":7,"start_character":48,"end_line":7,"end_character":57},"in_reply_to":"5f7c97a3_18e1e5fc","updated":"2018-07-06 15:10:49.000000000","message":"Good point :) Done","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":7,"context_line":"hardware: Start accounting for networks in NUMA placement"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Modify the \u0027numa_fit_instance_to_host\u0027 function to take networks into"},{"line_number":10,"context_line":"account. We don\u0027t yet do anything with this at runtime, given that we\u0027re"},{"line_number":11,"context_line":"not populating host NUMA network information, but that can come later."},{"line_number":12,"context_line":"It also doesn\u0027t stop us from unit testing things, which is nice."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"5f7c97a3_a34f6268","line":10,"range":{"start_line":10,"start_character":9,"end_line":10,"end_character":54},"updated":"2018-07-05 14:36:01.000000000","message":"Actually, you\u0027re skipping all hosts with this now right? Won\u0027t we never find a host that fits after this change, before we start reporting (and roll to that code on all nodes)?","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"16ab4d243ff1b1da8bb8f43b46b911e53234e2d1","unresolved":false,"context_lines":[{"line_number":7,"context_line":"hardware: Start accounting for networks in NUMA placement"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"Modify the \u0027numa_fit_instance_to_host\u0027 function to take networks into"},{"line_number":10,"context_line":"account. We don\u0027t yet do anything with this at runtime, given that we\u0027re"},{"line_number":11,"context_line":"not populating host NUMA network information, but that can come later."},{"line_number":12,"context_line":"It also doesn\u0027t stop us from unit testing things, which is nice."},{"line_number":13,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":12,"id":"5f7c97a3_74a32f00","line":10,"range":{"start_line":10,"start_character":9,"end_line":10,"end_character":54},"in_reply_to":"5f7c97a3_a34f6268","updated":"2018-07-06 15:10:49.000000000","message":"I think I worded this poorly. It\u0027s the modifications to this function that we\u0027re not doing anything with - we still call that function so things are essentially working as they were","commit_id":"99839bc0de586565148b444369b88880492bc96e"}],"nova/tests/unit/virt/test_hardware.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"08da90ae7fb345b96824c39b2afd9ee15f0354a8","unresolved":false,"context_lines":[{"line_number":3339,"context_line":"    def test_missing_networks(self):"},{"line_number":3340,"context_line":"        \"\"\"Validate behavior with a physical network without affinity."},{"line_number":3341,"context_line":""},{"line_number":3342,"context_line":"        If we haven\u0027t recorded NUMA affinity for a given physical network, we"},{"line_number":3343,"context_line":"        clearly shouldn\u0027t fail to build."},{"line_number":3344,"context_line":"        \"\"\""},{"line_number":3345,"context_line":"        network_metadata \u003d objects.NetworkMetadata("},{"line_number":3346,"context_line":"            physnets\u003dset([\u0027baz\u0027]), tunneled\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_98a6f9ac","line":3343,"range":{"start_line":3342,"start_character":8,"end_line":3343,"end_character":40},"updated":"2018-07-10 21:45:30.000000000","message":"Seems kind of weird to me - as a user, I\u0027m requesting a server  affined to network baz which isn\u0027t being reported as available on this host, but the host passes, thus my request may or may not be satisfied and I won\u0027t know until I realize the performance gains I thought I\u0027d get aren\u0027t there.\n\nI realize this is straddling upgrades so that operators don\u0027t have to report all physical network affinity for their hosts to still support instances that request NUMA, but...it\u0027s weird.\n\nWhat if *none* of the hosts are configured to provide physnet \u0027baz\u0027? Shouldn\u0027t we fail that build at some point? Does that happen during the claim in the resource tracker thus triggering a reschedule?","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7d80291f51794e26fb16b4fa312609b1a56818d4","unresolved":false,"context_lines":[{"line_number":3339,"context_line":"    def test_missing_networks(self):"},{"line_number":3340,"context_line":"        \"\"\"Validate behavior with a physical network without affinity."},{"line_number":3341,"context_line":""},{"line_number":3342,"context_line":"        If we haven\u0027t recorded NUMA affinity for a given physical network, we"},{"line_number":3343,"context_line":"        clearly shouldn\u0027t fail to build."},{"line_number":3344,"context_line":"        \"\"\""},{"line_number":3345,"context_line":"        network_metadata \u003d objects.NetworkMetadata("},{"line_number":3346,"context_line":"            physnets\u003dset([\u0027baz\u0027]), tunneled\u003dFalse)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_58b86177","line":3343,"range":{"start_line":3342,"start_character":8,"end_line":3343,"end_character":40},"in_reply_to":"5f7c97a3_98a6f9ac","updated":"2018-07-10 21:57:28.000000000","message":"OK dansmith straightened me out in IRC:\n\n(4:45:53 PM) mriedem: dansmith: question in the numa aware vswitch hardware change here https://review.openstack.org/#/c/564448/14/nova/tests/unit/virt/test_hardware.py@3343\n(4:46:45 PM) dansmith: mriedem: see my comment on that in an earlier PS?\n(4:46:58 PM) mriedem: nope\n(4:47:07 PM) dansmith: mriedem: I think the deal is, the physnet may be available on the host, but the operator hasn\u0027t said that it\u0027s particularly affined to some node\n(4:47:24 PM) dansmith: I stumbled on that as well, but worked out the reasoning from the comments and the tests\n(4:48:08 PM) mriedem: but the point is,\n(4:48:20 PM) dansmith: I dunno if that would just be \"the operator doesn\u0027t care enough to configure the thing\" or if there\u0027s some reason why it may not be affined, like it\u0027s too dynamic or something\n(4:48:30 PM) mriedem: as a user i can say i want to be affined to baz and if the operator hasn\u0027t reported baz, we\u0027ll still do the build even though you might not be affined to baz\n(4:48:49 PM) mriedem: i\u0027m thinking of it like inventory - is that the wrong way to think about this?\n(4:48:59 PM) dansmith: no, that comment says that the user has requested network baz, and the compute node will do it,\n(4:49:04 PM) mriedem: i see stephenfin has also dropped the patch that does any RT claims code\n(4:49:08 PM) dansmith: but there is no *numa* affinity between a numa node and network baz\n(4:49:38 PM) mriedem: \u0027compute node will do it\u0027 as in, wire up the ports on network baz\n(4:49:43 PM) dansmith: yeah\n(4:49:51 PM) dansmith: the user doesn\u0027t request numa affinity to a network,\n(4:50:33 PM) dansmith: they request some network, and maybe some numa layout, but this process makes sure we select a favorable numa pinning of guest nodes to host nodes such that they won\u0027t end up on no affined nodes to the network they want\n(4:51:19 PM) mriedem: assuming the operator has exposed that affinity info on the host...\n(4:51:21 PM) mriedem: yeah?\n(4:51:57 PM) dansmith: has configured it, yeah\n(4:52:13 PM) dansmith: the user doesn\u0027t really know anything about this behind the scenes optimization\n(4:52:23 PM) dansmith: they may know that their network performance isn\u0027t what it should be,\n(4:52:35 PM) dansmith: but they ask for high-level things like being connected to network baz,\n(4:52:48 PM) dansmith: and expect the backend is configuring their numa layout and pinning properly\n(4:52:52 PM) dansmith: right now that doesn\u0027t happen,\n(4:53:06 PM) dansmith: and if an operator doesn\u0027t or can\u0027t configure that information, they\u0027ll get what they get today\n(4:53:47 PM) mriedem: can they, or can\u0027t they, throw a fit when they don\u0027t like what they get\n(4:53:58 PM) mriedem: ignore that\n(4:54:51 PM) mriedem: ok that helps anyway, i was approaching this whole bp as a pass/fail type thing (hence the inventory thinking) rather than an optimization\n(4:55:17 PM) dansmith: well, the hardware fitting thing is pass/fail\n(4:55:18 PM) dansmith: meaning,\n(4:55:27 PM) dansmith: \"these nodes pass/fail my test for good-enough\"\n(4:55:33 PM) mriedem: so if i want baz and only foo and bar are reported, i get whatever. but if i want foo and foo is reported somewhere, scheduler picks that one specifically.\n(4:55:37 PM) mriedem: right\n(4:55:38 PM) dansmith: but in the case of no information, they pass as they do today\n(4:55:47 PM) dansmith: yeah\n(4:56:00 PM) mriedem: ok yeah - i should have read the tests when reviewing the code earlier","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"08da90ae7fb345b96824c39b2afd9ee15f0354a8","unresolved":false,"context_lines":[{"line_number":3359,"context_line":"            self.host, [self.host.cells[0]], network_metadata)"},{"line_number":3360,"context_line":"        self.assertTrue(supports)"},{"line_number":3361,"context_line":""},{"line_number":3362,"context_line":"        # ...while it should fail for the other host NUMA node"},{"line_number":3363,"context_line":"        supports \u003d hw._numa_cells_support_network_metadata("},{"line_number":3364,"context_line":"            self.host, [self.host.cells[1]], network_metadata)"},{"line_number":3365,"context_line":"        self.assertFalse(supports)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_58d701f6","line":3362,"range":{"start_line":3362,"start_character":8,"end_line":3362,"end_character":62},"updated":"2018-07-10 21:45:30.000000000","message":"And unlike the baz case above, it fails because another cell does provide foo right?","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d517cec9d1d8a1da7d360bec830b6fa06c583409","unresolved":false,"context_lines":[{"line_number":3359,"context_line":"            self.host, [self.host.cells[0]], network_metadata)"},{"line_number":3360,"context_line":"        self.assertTrue(supports)"},{"line_number":3361,"context_line":""},{"line_number":3362,"context_line":"        # ...while it should fail for the other host NUMA node"},{"line_number":3363,"context_line":"        supports \u003d hw._numa_cells_support_network_metadata("},{"line_number":3364,"context_line":"            self.host, [self.host.cells[1]], network_metadata)"},{"line_number":3365,"context_line":"        self.assertFalse(supports)"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_cb4f6b8a","line":3362,"range":{"start_line":3362,"start_character":8,"end_line":3362,"end_character":62},"in_reply_to":"5f7c97a3_58d701f6","updated":"2018-07-13 16:42:51.000000000","message":"\u0027zactly","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"08da90ae7fb345b96824c39b2afd9ee15f0354a8","unresolved":false,"context_lines":[{"line_number":3392,"context_line":"    def test_multiple_networks(self):"},{"line_number":3393,"context_line":"        \"\"\"Validate behavior with multiple networks."},{"line_number":3394,"context_line":""},{"line_number":3395,"context_line":"        If a we request multiple networks that are spread across host NUMA"},{"line_number":3396,"context_line":"        nodes, we\u0027re going to need to use multiple host instances."},{"line_number":3397,"context_line":"        \"\"\""},{"line_number":3398,"context_line":"        network_metadata \u003d objects.NetworkMetadata("}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_38e6c5e9","line":3395,"range":{"start_line":3395,"start_character":11,"end_line":3395,"end_character":12},"updated":"2018-07-10 21:45:30.000000000","message":"nix","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"d517cec9d1d8a1da7d360bec830b6fa06c583409","unresolved":false,"context_lines":[{"line_number":3392,"context_line":"    def test_multiple_networks(self):"},{"line_number":3393,"context_line":"        \"\"\"Validate behavior with multiple networks."},{"line_number":3394,"context_line":""},{"line_number":3395,"context_line":"        If a we request multiple networks that are spread across host NUMA"},{"line_number":3396,"context_line":"        nodes, we\u0027re going to need to use multiple host instances."},{"line_number":3397,"context_line":"        \"\"\""},{"line_number":3398,"context_line":"        network_metadata \u003d objects.NetworkMetadata("}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_2e04ed13","line":3395,"range":{"start_line":3395,"start_character":11,"end_line":3395,"end_character":12},"in_reply_to":"5f7c97a3_38e6c5e9","updated":"2018-07-13 16:42:51.000000000","message":"Done in a follow-up","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"}],"nova/virt/hardware.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1541,"context_line":""},{"line_number":1542,"context_line":""},{"line_number":1543,"context_line":"def _numa_cells_support_network_requests("},{"line_number":1544,"context_line":"        host_topology,  # type: objects.NUMATopology"},{"line_number":1545,"context_line":"        chosen_cells,  # type: List[objects.NUMACell]"},{"line_number":1546,"context_line":"        network_requests  # type: objects.NetworkMetadata"},{"line_number":1547,"context_line":"        ):"},{"line_number":1548,"context_line":"    # type: (...) -\u003e bool"},{"line_number":1549,"context_line":"    \"\"\"Determine whether the cells can accept the network requests."},{"line_number":1550,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_18ca456d","line":1547,"range":{"start_line":1544,"start_character":0,"end_line":1547,"end_character":10},"updated":"2018-07-05 14:36:01.000000000","message":"Is this format necessary for your type annotation stuff? It makes my eyes bleed :/","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":11869,"name":"zhubingbing","email":"zhu.bingbing@99cloud.net","username":"zhubingbing"},"change_message_id":"4883ef87cb8226bc634a38a2a2f3a7b640e45829","unresolved":false,"context_lines":[{"line_number":1541,"context_line":""},{"line_number":1542,"context_line":""},{"line_number":1543,"context_line":"def _numa_cells_support_network_requests("},{"line_number":1544,"context_line":"        host_topology,  # type: objects.NUMATopology"},{"line_number":1545,"context_line":"        chosen_cells,  # type: List[objects.NUMACell]"},{"line_number":1546,"context_line":"        network_requests  # type: objects.NetworkMetadata"},{"line_number":1547,"context_line":"        ):"},{"line_number":1548,"context_line":"    # type: (...) -\u003e bool"},{"line_number":1549,"context_line":"    \"\"\"Determine whether the cells can accept the network requests."},{"line_number":1550,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_407436d0","line":1547,"range":{"start_line":1544,"start_character":0,"end_line":1547,"end_character":10},"in_reply_to":"5f7c97a3_18ca456d","updated":"2018-07-06 05:31:00.000000000","message":"agree","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1553,"context_line":"    :param chosen_cells: List of NUMACells to extract possible network NUMA"},{"line_number":1554,"context_line":"        affinity from"},{"line_number":1555,"context_line":"    :param network_requests: The combined summary of physnets and tunneled"},{"line_number":1556,"context_line":"        networks required by this topology."},{"line_number":1557,"context_line":""},{"line_number":1558,"context_line":"    :return: True if any NUMA affinity constraints for requested networks can"},{"line_number":1559,"context_line":"        be satisfied, else False"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_789ed969","line":1556,"range":{"start_line":1556,"start_character":42,"end_line":1556,"end_character":43},"updated":"2018-07-05 14:36:01.000000000","message":"...or None?","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1560,"context_line":"    \"\"\""},{"line_number":1561,"context_line":"    required_physnets \u003d None"},{"line_number":1562,"context_line":"    if network_requests and \u0027physnets\u0027 in network_requests:"},{"line_number":1563,"context_line":"        required_physnets \u003d set(network_requests.physnets)"},{"line_number":1564,"context_line":""},{"line_number":1565,"context_line":"    required_tunnel \u003d None"},{"line_number":1566,"context_line":"    if network_requests and \u0027tunneled\u0027 in network_requests:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_d894cd43","line":1563,"range":{"start_line":1563,"start_character":28,"end_line":1563,"end_character":31},"updated":"2018-07-05 14:36:01.000000000","message":"physnets is already a set now right?","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1570,"context_line":"        # if there are no tunnel networks required, our job is done here"},{"line_number":1571,"context_line":"        if not required_tunnel:"},{"line_number":1572,"context_line":"            return True"},{"line_number":1573,"context_line":""},{"line_number":1574,"context_line":"        # if there are, we need to make sure we can satisfy that requirement"},{"line_number":1575,"context_line":"        for host_cell in chosen_cells:"},{"line_number":1576,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_7876d90e","line":1573,"updated":"2018-07-05 14:36:01.000000000","message":"This is a little hard to parse, but if we\u0027ve made it to this line, then there are no required physnets, but tunnel _is_ required...","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1573,"context_line":""},{"line_number":1574,"context_line":"        # if there are, we need to make sure we can satisfy that requirement"},{"line_number":1575,"context_line":"        for host_cell in chosen_cells:"},{"line_number":1576,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1577,"context_line":"                continue"},{"line_number":1578,"context_line":""},{"line_number":1579,"context_line":"            if host_cell.network_metadata.tunneled:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_98917549","line":1576,"updated":"2018-07-05 14:36:01.000000000","message":"This means we don\u0027t know, right? Like because an older host hasn\u0027t reported the new format yet? If so, won\u0027t this cause us to not be able to schedule anything if we do an upgrade where we roll the control plane before any computes have been upgraded?","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1578,"context_line":""},{"line_number":1579,"context_line":"            if host_cell.network_metadata.tunneled:"},{"line_number":1580,"context_line":"                return True"},{"line_number":1581,"context_line":""},{"line_number":1582,"context_line":"    # identify requested physnets that have an affinity to any of our chosen"},{"line_number":1583,"context_line":"    # host NUMA cells"},{"line_number":1584,"context_line":"    for host_cell in chosen_cells:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_585afd81","line":1581,"updated":"2018-07-05 14:36:01.000000000","message":"If we\u0027re here then there are required physnets...","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1583,"context_line":"    # host NUMA cells"},{"line_number":1584,"context_line":"    for host_cell in chosen_cells:"},{"line_number":1585,"context_line":"        if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1586,"context_line":"            continue"},{"line_number":1587,"context_line":""},{"line_number":1588,"context_line":"        required_physnets -\u003d required_physnets.intersection("},{"line_number":1589,"context_line":"            host_cell.network_metadata.physnets)"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_1850055e","line":1586,"updated":"2018-07-05 14:36:01.000000000","message":"Same comment as above on L1576","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1587,"context_line":""},{"line_number":1588,"context_line":"        required_physnets -\u003d required_physnets.intersection("},{"line_number":1589,"context_line":"            host_cell.network_metadata.physnets)"},{"line_number":1590,"context_line":""},{"line_number":1591,"context_line":"    if not required_physnets:"},{"line_number":1592,"context_line":"        # if there are no tunnel networks required, our job is done here"},{"line_number":1593,"context_line":"        if not required_tunnel:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_18d565b7","line":1590,"updated":"2018-07-05 14:36:01.000000000","message":"So if we get here, there are required physnets that aren\u0027t covered by these cells right? Which means we may need to fail...","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1601,"context_line":"            if host_cell.network_metadata.tunneled:"},{"line_number":1602,"context_line":"                return True"},{"line_number":1603,"context_line":""},{"line_number":1604,"context_line":"    # however, if we still require some level of NUMA affinity, we need to make"},{"line_number":1605,"context_line":"    # sure one of the other NUMA cells isn\u0027t providing that"},{"line_number":1606,"context_line":"    for host_cell in host_topology.cells:"},{"line_number":1607,"context_line":"        if \u0027network_metadata\u0027 not in host_cell:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_f8ae692d","line":1604,"updated":"2018-07-05 14:36:01.000000000","message":"Okay, I think I\u0027m a bit lost here.\n\nIt seems like you should be able to simplify this by doing the physnet calculations first (which may be a no-op if that list is empty). Then, move on to checking the tunneled thing afterwards? I\u0027m not sure that bailing out before the physnet calcuation is really useful as an optimization, given the way it makes it look like you do the tunneled bit three times in this function.","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"16ab4d243ff1b1da8bb8f43b46b911e53234e2d1","unresolved":false,"context_lines":[{"line_number":1601,"context_line":"            if host_cell.network_metadata.tunneled:"},{"line_number":1602,"context_line":"                return True"},{"line_number":1603,"context_line":""},{"line_number":1604,"context_line":"    # however, if we still require some level of NUMA affinity, we need to make"},{"line_number":1605,"context_line":"    # sure one of the other NUMA cells isn\u0027t providing that"},{"line_number":1606,"context_line":"    for host_cell in host_topology.cells:"},{"line_number":1607,"context_line":"        if \u0027network_metadata\u0027 not in host_cell:"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_d46fa326","line":1604,"in_reply_to":"5f7c97a3_f8ae692d","updated":"2018-07-06 15:10:49.000000000","message":"Yup, makes sense. I\u0027ll do that now","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1623,"context_line":"        instance_topology,  # type: objects.InstanceNUMATopology"},{"line_number":1624,"context_line":"        limits\u003dNone,  # type: objects.NUMATopologyLimits"},{"line_number":1625,"context_line":"        pci_requests\u003dNone,  # type: objects.InstancePCIRequests"},{"line_number":1626,"context_line":"        pci_stats\u003dNone,  # type: pci_stats_.PciDeviceStats"},{"line_number":1627,"context_line":"        ):"},{"line_number":1628,"context_line":"    \"\"\"Fit the instance topology onto the host topology."},{"line_number":1629,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_9865f51f","line":1626,"updated":"2018-07-05 14:36:01.000000000","message":"Is this related to this patch? Seems like not. Also -\u003e :(","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"16ab4d243ff1b1da8bb8f43b46b911e53234e2d1","unresolved":false,"context_lines":[{"line_number":1623,"context_line":"        instance_topology,  # type: objects.InstanceNUMATopology"},{"line_number":1624,"context_line":"        limits\u003dNone,  # type: objects.NUMATopologyLimits"},{"line_number":1625,"context_line":"        pci_requests\u003dNone,  # type: objects.InstancePCIRequests"},{"line_number":1626,"context_line":"        pci_stats\u003dNone,  # type: pci_stats_.PciDeviceStats"},{"line_number":1627,"context_line":"        ):"},{"line_number":1628,"context_line":"    \"\"\"Fit the instance topology onto the host topology."},{"line_number":1629,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_d41a43c5","line":1626,"in_reply_to":"5f7c97a3_9865f51f","updated":"2018-07-06 15:10:49.000000000","message":"It was necessary before but no longer is. I\u0027ll strip it","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1659,"context_line":"    if \u0027emulator_threads_policy\u0027 in instance_topology:"},{"line_number":1660,"context_line":"        emulator_threads_policy \u003d instance_topology.emulator_threads_policy"},{"line_number":1661,"context_line":""},{"line_number":1662,"context_line":"    network_requests \u003d None"},{"line_number":1663,"context_line":"    if limits and \u0027networks\u0027 in limits:"},{"line_number":1664,"context_line":"        network_requests \u003d limits.networks"},{"line_number":1665,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_58443d73","line":1662,"updated":"2018-07-05 14:36:01.000000000","message":"This is confusing is it not? We have pci_requests here (which is the actual information from the request) and we\u0027re near other uses of network_requests which is actual NetworkRequests from the user. should this  not be network_details or something?","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"16ab4d243ff1b1da8bb8f43b46b911e53234e2d1","unresolved":false,"context_lines":[{"line_number":1659,"context_line":"    if \u0027emulator_threads_policy\u0027 in instance_topology:"},{"line_number":1660,"context_line":"        emulator_threads_policy \u003d instance_topology.emulator_threads_policy"},{"line_number":1661,"context_line":""},{"line_number":1662,"context_line":"    network_requests \u003d None"},{"line_number":1663,"context_line":"    if limits and \u0027networks\u0027 in limits:"},{"line_number":1664,"context_line":"        network_requests \u003d limits.networks"},{"line_number":1665,"context_line":""}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_94e60b4a","line":1662,"in_reply_to":"5f7c97a3_58443d73","updated":"2018-07-06 15:10:49.000000000","message":"It is. I\u0027ve renamed this network_metadata","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9240b5f167f15c2330e7175aea8819bd83042072","unresolved":false,"context_lines":[{"line_number":1707,"context_line":""},{"line_number":1708,"context_line":"        if pci_requests and pci_stats and not pci_stats.support_requests("},{"line_number":1709,"context_line":"                pci_requests, chosen_instance_cells):"},{"line_number":1710,"context_line":"            continue"},{"line_number":1711,"context_line":""},{"line_number":1712,"context_line":"        if network_requests and not _numa_cells_support_network_requests("},{"line_number":1713,"context_line":"                host_topology, chosen_host_cells, network_requests):"}],"source_content_type":"text/x-python","patch_set":12,"id":"5f7c97a3_832586b0","line":1710,"updated":"2018-07-05 14:36:01.000000000","message":"Some of the refactor in here, like this logic flip would have been nice to have separate just so it\u0027s clear what is changing with the new network-based fitting...","commit_id":"99839bc0de586565148b444369b88880492bc96e"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1538,"context_line":""},{"line_number":1539,"context_line":"def _numa_cells_support_network_metadata("},{"line_number":1540,"context_line":"        host_topology,  # type: objects.NUMATopology"},{"line_number":1541,"context_line":"        chosen_cells,  # type: List[objects.NUMACell]"},{"line_number":1542,"context_line":"        network_metadata  # type: objects.NetworkMetadata"},{"line_number":1543,"context_line":"        ):"},{"line_number":1544,"context_line":"    # type: (...) -\u003e bool"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_706d7665","line":1541,"range":{"start_line":1541,"start_character":8,"end_line":1541,"end_character":20},"updated":"2018-07-06 18:12:07.000000000","message":"nit: name this chosen_host_cells to be consistent with your renames below since we also have a chosen_instance_cells in scope when this called.","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1538,"context_line":""},{"line_number":1539,"context_line":"def _numa_cells_support_network_metadata("},{"line_number":1540,"context_line":"        host_topology,  # type: objects.NUMATopology"},{"line_number":1541,"context_line":"        chosen_cells,  # type: List[objects.NUMACell]"},{"line_number":1542,"context_line":"        network_metadata  # type: objects.NetworkMetadata"},{"line_number":1543,"context_line":"        ):"},{"line_number":1544,"context_line":"    # type: (...) -\u003e bool"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_0f4f2b35","line":1541,"range":{"start_line":1541,"start_character":8,"end_line":1541,"end_character":20},"in_reply_to":"5f7c97a3_706d7665","updated":"2018-07-09 09:51:12.000000000","message":"Done","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1559,"context_line":""},{"line_number":1560,"context_line":"    required_physnets \u003d None"},{"line_number":1561,"context_line":"    if \u0027physnets\u0027 in network_metadata:"},{"line_number":1562,"context_line":"        required_physnets \u003d set(network_metadata.physnets)"},{"line_number":1563,"context_line":""},{"line_number":1564,"context_line":"    required_tunnel \u003d None"},{"line_number":1565,"context_line":"    if \u0027tunneled\u0027 in network_metadata:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_f0a8a68b","line":1562,"range":{"start_line":1562,"start_character":28,"end_line":1562,"end_character":58},"updated":"2018-07-06 18:12:07.000000000","message":"This is already a Set(String) field so I\u0027m not sure why you need to re-cast to a set() here.","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"43fa358cfc7ff80957c23830fc1cb0424ba999ae","unresolved":false,"context_lines":[{"line_number":1559,"context_line":""},{"line_number":1560,"context_line":"    required_physnets \u003d None"},{"line_number":1561,"context_line":"    if \u0027physnets\u0027 in network_metadata:"},{"line_number":1562,"context_line":"        required_physnets \u003d set(network_metadata.physnets)"},{"line_number":1563,"context_line":""},{"line_number":1564,"context_line":"    required_tunnel \u003d None"},{"line_number":1565,"context_line":"    if \u0027tunneled\u0027 in network_metadata:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_ea909d44","line":1562,"range":{"start_line":1562,"start_character":28,"end_line":1562,"end_character":58},"in_reply_to":"5f7c97a3_2f4a2723","updated":"2018-07-09 10:20:58.000000000","message":"Actually, I need it to avoid modifying the original object. I\u0027ve added a comment to this effect","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1559,"context_line":""},{"line_number":1560,"context_line":"    required_physnets \u003d None"},{"line_number":1561,"context_line":"    if \u0027physnets\u0027 in network_metadata:"},{"line_number":1562,"context_line":"        required_physnets \u003d set(network_metadata.physnets)"},{"line_number":1563,"context_line":""},{"line_number":1564,"context_line":"    required_tunnel \u003d None"},{"line_number":1565,"context_line":"    if \u0027tunneled\u0027 in network_metadata:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_2f4a2723","line":1562,"range":{"start_line":1562,"start_character":28,"end_line":1562,"end_character":58},"in_reply_to":"5f7c97a3_f0a8a68b","updated":"2018-07-09 09:51:12.000000000","message":"Yup, missed that in the previous review","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1561,"context_line":"    if \u0027physnets\u0027 in network_metadata:"},{"line_number":1562,"context_line":"        required_physnets \u003d set(network_metadata.physnets)"},{"line_number":1563,"context_line":""},{"line_number":1564,"context_line":"    required_tunnel \u003d None"},{"line_number":1565,"context_line":"    if \u0027tunneled\u0027 in network_metadata:"},{"line_number":1566,"context_line":"        required_tunnel \u003d network_metadata.tunneled"},{"line_number":1567,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_1089a224","line":1564,"range":{"start_line":1564,"start_character":22,"end_line":1564,"end_character":26},"updated":"2018-07-06 18:12:07.000000000","message":"False would be a bit more natural but it doesn\u0027t matter either way.","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1561,"context_line":"    if \u0027physnets\u0027 in network_metadata:"},{"line_number":1562,"context_line":"        required_physnets \u003d set(network_metadata.physnets)"},{"line_number":1563,"context_line":""},{"line_number":1564,"context_line":"    required_tunnel \u003d None"},{"line_number":1565,"context_line":"    if \u0027tunneled\u0027 in network_metadata:"},{"line_number":1566,"context_line":"        required_tunnel \u003d network_metadata.tunneled"},{"line_number":1567,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_cf5e13e8","line":1564,"range":{"start_line":1564,"start_character":22,"end_line":1564,"end_character":26},"in_reply_to":"5f7c97a3_1089a224","updated":"2018-07-09 09:51:12.000000000","message":"Done","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1572,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1573,"context_line":"                continue"},{"line_number":1574,"context_line":""},{"line_number":1575,"context_line":"            required_physnets -\u003d required_physnets.intersection("},{"line_number":1576,"context_line":"                host_cell.network_metadata.physnets)"},{"line_number":1577,"context_line":""},{"line_number":1578,"context_line":"        # however, if we still require some level of NUMA affinity, we need"},{"line_number":1579,"context_line":"        # to make sure one of the other NUMA cells isn\u0027t providing that"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_903db22d","line":1576,"range":{"start_line":1575,"start_character":12,"end_line":1576,"end_character":52},"updated":"2018-07-06 18:12:07.000000000","message":"This is kind of hurting my brain. Let me see if I understand this with an example.\n\nLet\u0027s say required_physnets starts as physnet1 and physnet2. These are the things the user is requesting.\n\nNow let\u0027s say our chosen host_cell1 in this case provides physnet1 but not physnet2. Based on that, we\u0027d end up with required_physnets \u003d phsynet1 because we filter out physnet2 since the host_cell1 doesn\u0027t provide it.\n\nNow let\u0027s say another non-chosen host_cell2 provides physnet2 but not physnet1. Based on the logic on L1584 below, we\u0027d eventually return True because we don\u0027t filter out.\n\nBut if host_cell2 does provide physnet1, then we\u0027d return False because there is an intersection of what\u0027s requested and what\u0027s provided by another host cell.\n\nIs that a realistic scenario? I guess I\u0027m not sure why we filter out host_cell1 if it provides physnet1 and host_cell2 also provides physnet1. I\u0027d think we should filter out the host cell if it doesn\u0027t provide all of the requested phsynets (all-or-none claim), but maybe I just don\u0027t understand how this is filtering/fitting is supposed to work.","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1572,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1573,"context_line":"                continue"},{"line_number":1574,"context_line":""},{"line_number":1575,"context_line":"            required_physnets -\u003d required_physnets.intersection("},{"line_number":1576,"context_line":"                host_cell.network_metadata.physnets)"},{"line_number":1577,"context_line":""},{"line_number":1578,"context_line":"        # however, if we still require some level of NUMA affinity, we need"},{"line_number":1579,"context_line":"        # to make sure one of the other NUMA cells isn\u0027t providing that"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_af59578d","line":1576,"range":{"start_line":1575,"start_character":12,"end_line":1576,"end_character":52},"in_reply_to":"5f7c97a3_903db22d","updated":"2018-07-09 09:51:12.000000000","message":"\u003e This is kind of hurting my brain. Let me see if I understand this\n \u003e with an example.\n \u003e \n \u003e Let\u0027s say required_physnets starts as physnet1 and physnet2. These\n \u003e are the things the user is requesting.\n \u003e \n \u003e Now let\u0027s say our chosen host_cell1 in this case provides physnet1\n \u003e but not physnet2. Based on that, we\u0027d end up with required_physnets\n \u003e \u003d phsynet1 because we filter out physnet2 since the host_cell1\n \u003e doesn\u0027t provide it.\n\nNo, you\u0027d end up with required_physnets \u003d physnet2. Note that the operator is \u0027-\u003d\u0027 - not \u0027\u003d\u0027.\n\n \u003e Now let\u0027s say another non-chosen host_cell2 provides physnet2 but\n \u003e not physnet1. Based on the logic on L1584 below, we\u0027d eventually\n \u003e return True because we don\u0027t filter out.\n\nWe\u0027d return False because \u0027required_physnets \u003d physnet2\u0027 and another non-chosen host cell does provide one of these.\n\n \u003e But if host_cell2 does provide physnet1, then we\u0027d return False\n \u003e because there is an intersection of what\u0027s requested and what\u0027s\n \u003e provided by another host cell.\n \u003e \n \u003e Is that a realistic scenario? I guess I\u0027m not sure why we filter\n \u003e out host_cell1 if it provides physnet1 and host_cell2 also provides\n \u003e physnet1. I\u0027d think we should filter out the host cell if it\n \u003e doesn\u0027t provide all of the requested phsynets (all-or-none claim),\n \u003e but maybe I just don\u0027t understand how this is filtering/fitting is\n \u003e supposed to work.\n\nWhat I\u0027m trying to do here is (a) handle networks that we don\u0027t care about NUMA affinity for and (b) avoid breaking upgrades. Say we have two physnets - physnet_data and physnet_mgmt. We don\u0027t care about NUMA affinity for the latter because it\u0027s a management network and performance isn\u0027t a concern. As such, the operator can simply not define NUMA affinity for this network and we\u0027ll completely ignore it. This is also necessary to allow upgrade. If we weren\u0027t able to handle undefined affinities, every single user would have to configure dummy values or they wouldn\u0027t be able to boot instances with NUMA functionality.\n\nDoes that make sense? I can add additional comments to try and explain the logic a little better","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1577,"context_line":""},{"line_number":1578,"context_line":"        # however, if we still require some level of NUMA affinity, we need"},{"line_number":1579,"context_line":"        # to make sure one of the other NUMA cells isn\u0027t providing that"},{"line_number":1580,"context_line":"        for host_cell in host_topology.cells:"},{"line_number":1581,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1582,"context_line":"                continue"},{"line_number":1583,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_1005227a","line":1580,"range":{"start_line":1580,"start_character":12,"end_line":1580,"end_character":21},"updated":"2018-07-06 18:12:07.000000000","message":"How do you know that this isn\u0027t in chosen_cells?","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1577,"context_line":""},{"line_number":1578,"context_line":"        # however, if we still require some level of NUMA affinity, we need"},{"line_number":1579,"context_line":"        # to make sure one of the other NUMA cells isn\u0027t providing that"},{"line_number":1580,"context_line":"        for host_cell in host_topology.cells:"},{"line_number":1581,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1582,"context_line":"                continue"},{"line_number":1583,"context_line":""}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_afce3729","line":1580,"range":{"start_line":1580,"start_character":12,"end_line":1580,"end_character":21},"in_reply_to":"5f7c97a3_1005227a","updated":"2018-07-09 09:51:12.000000000","message":"I don\u0027t but it doesn\u0027t matter. We\u0027ve already filtered out physnets that _are_ attached to one of the cells in chosen_cells so anything that\u0027s left won\u0027t exist. Dropping chosen cells from this list was more complicated than I liked so I didn\u0027t bother doing it","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1582,"context_line":"                continue"},{"line_number":1583,"context_line":""},{"line_number":1584,"context_line":"            if required_physnets.intersection("},{"line_number":1585,"context_line":"                    set(host_cell.network_metadata.physnets)):"},{"line_number":1586,"context_line":"                return False"},{"line_number":1587,"context_line":""},{"line_number":1588,"context_line":"    if required_tunnel:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_50c07a2b","line":1585,"range":{"start_line":1585,"start_character":20,"end_line":1585,"end_character":23},"updated":"2018-07-06 18:12:07.000000000","message":"This shouldn\u0027t be necessary, and you\u0027re not doing it on L1576 either.","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1582,"context_line":"                continue"},{"line_number":1583,"context_line":""},{"line_number":1584,"context_line":"            if required_physnets.intersection("},{"line_number":1585,"context_line":"                    set(host_cell.network_metadata.physnets)):"},{"line_number":1586,"context_line":"                return False"},{"line_number":1587,"context_line":""},{"line_number":1588,"context_line":"    if required_tunnel:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_4fd9a3f1","line":1585,"range":{"start_line":1585,"start_character":20,"end_line":1585,"end_character":23},"in_reply_to":"5f7c97a3_50c07a2b","updated":"2018-07-09 09:51:12.000000000","message":"Done","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1595,"context_line":"            if host_cell.network_metadata.tunneled:"},{"line_number":1596,"context_line":"                return True"},{"line_number":1597,"context_line":""},{"line_number":1598,"context_line":"        # however, if we still require some level of NUMA affinity, we need to"},{"line_number":1599,"context_line":"        # make sure one of the other NUMA cells isn\u0027t providing that"},{"line_number":1600,"context_line":"        for host_cell in host_topology.cells:"},{"line_number":1601,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_f059868c","line":1598,"updated":"2018-07-06 18:12:07.000000000","message":"Why do we need this? Can\u0027t we just have this:\n\nfor host_cell in chosen_cells:\n    ...\n    if host_cell.network_metadata.tunneled:\n        return True\n# If none of the chosen host cells provide a tunneled network,\n# then we need to reject these host cells for this request.\nreturn False","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1595,"context_line":"            if host_cell.network_metadata.tunneled:"},{"line_number":1596,"context_line":"                return True"},{"line_number":1597,"context_line":""},{"line_number":1598,"context_line":"        # however, if we still require some level of NUMA affinity, we need to"},{"line_number":1599,"context_line":"        # make sure one of the other NUMA cells isn\u0027t providing that"},{"line_number":1600,"context_line":"        for host_cell in host_topology.cells:"},{"line_number":1601,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_eff18f6b","line":1598,"in_reply_to":"5f7c97a3_f059868c","updated":"2018-07-09 09:51:12.000000000","message":"Because of the above concerns around networks we don\u0027t wish to provide NUMA affinity for and not wanting to break upgrades","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1601,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1602,"context_line":"                continue"},{"line_number":1603,"context_line":""},{"line_number":1604,"context_line":"            if required_tunnel and host_cell.network_metadata.tunneled:"},{"line_number":1605,"context_line":"                return False"},{"line_number":1606,"context_line":""},{"line_number":1607,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_f080a6c0","line":1604,"range":{"start_line":1604,"start_character":15,"end_line":1604,"end_character":30},"updated":"2018-07-06 18:12:07.000000000","message":"This is always True at this point because of L1588.","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"12231aa70429d165da82fa21541080906e19fe2b","unresolved":false,"context_lines":[{"line_number":1601,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1602,"context_line":"                continue"},{"line_number":1603,"context_line":""},{"line_number":1604,"context_line":"            if required_tunnel and host_cell.network_metadata.tunneled:"},{"line_number":1605,"context_line":"                return False"},{"line_number":1606,"context_line":""},{"line_number":1607,"context_line":"    return True"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_afe717ab","line":1604,"range":{"start_line":1604,"start_character":15,"end_line":1604,"end_character":30},"in_reply_to":"5f7c97a3_f080a6c0","updated":"2018-07-09 09:51:12.000000000","message":"Done","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"f351632ba7bde8fe6485b55f1381cab5874c5acc","unresolved":false,"context_lines":[{"line_number":1684,"context_line":"                break"},{"line_number":1685,"context_line":"            if got_cell is None:"},{"line_number":1686,"context_line":"                break"},{"line_number":1687,"context_line":"            chosen_host_cells.append(host_cell)"},{"line_number":1688,"context_line":"            chosen_instance_cells.append(got_cell)"},{"line_number":1689,"context_line":""},{"line_number":1690,"context_line":"        if len(chosen_instance_cells) !\u003d len(host_cell_perm):"},{"line_number":1691,"context_line":"            continue"},{"line_number":1692,"context_line":""},{"line_number":1693,"context_line":"        if pci_requests and pci_stats and not pci_stats.support_requests("},{"line_number":1694,"context_line":"                pci_requests, chosen_instance_cells):"},{"line_number":1695,"context_line":"            continue"},{"line_number":1696,"context_line":""},{"line_number":1697,"context_line":"        if network_metadata and not _numa_cells_support_network_metadata("},{"line_number":1698,"context_line":"                host_topology, chosen_host_cells, network_metadata):"}],"source_content_type":"text/x-python","patch_set":13,"id":"5f7c97a3_d051ea0e","line":1695,"range":{"start_line":1687,"start_character":12,"end_line":1695,"end_character":20},"updated":"2018-07-06 18:12:07.000000000","message":"I\u0027m not sure why this refactor all needs to be in this same change. It clutters up the review. I guess you could make the argument about splitting out chosen_host_cells but isn\u0027t that just host_cell_perm without the rename?","commit_id":"f9b6e3b4029f591987e3da7cc4475434c29e335b"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"8e9682e2685a6ccc688af5082005cf9a071f67b8","unresolved":false,"context_lines":[{"line_number":1581,"context_line":"        # however, if we still require some level of NUMA affinity, we need"},{"line_number":1582,"context_line":"        # to make sure one of the other NUMA cells isn\u0027t providing that; note"},{"line_number":1583,"context_line":"        # that NUMA affinity might not be provided for all physnets so we are"},{"line_number":1584,"context_line":"        # in effect skipping these"},{"line_number":1585,"context_line":"        for host_cell in host_topology.cells:"},{"line_number":1586,"context_line":"            if \u0027network_metadata\u0027 not in host_cell:"},{"line_number":1587,"context_line":"                continue"}],"source_content_type":"text/x-python","patch_set":14,"id":"5f7c97a3_bab0b2fe","line":1584,"updated":"2018-07-09 14:29:59.000000000","message":"Hmm, couldn\u0027t we just stop here and return False if required_physnets has not be drawn down to empty? If we had required_physnets that have not been matched to any in chosen_nodes, we\u0027re not able to provide the desired affinity, right?\n\nOr are you specifically trying to allow the case here where not all physnets are tied to a specific node, and as long as there is no potentially-better arrangement (where some in host_nodes *would* provide the affinity we need)?\n\nI guess that\u0027s what this comment says, but it didn\u0027t resonate with me until I wrote this all out and then compared my thoughts to the comment above.\n\n[later]\n\nYeah, confirmed by the tests I guess.","commit_id":"d985d662617c3dfc0c0dde1046c6ee739f3b1d0b"}]}