)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1c1b697da8e175eb9d04f65abca38ac02c44267e","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Sahid Orentino Ferdjaoui \u003csahid.ferdjaoui@redhat.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2017-01-06 06:17:01 -0500"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"numa: fit instance toplogy with cpus reserved onto cores"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In this commit we update method to pack a requested guest NUMA node"},{"line_number":10,"context_line":"topology with CPUs reserved onto a host NUMA node."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":15,"id":"ba5201f7_31b9f515","line":7,"updated":"2017-01-06 18:09:55.000000000","message":"s/toplogy/topology/","commit_id":"a9344f6df70a40c68950db93b2a54d0537989c62"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e938bb5aeae0fb71771313d7f9cfeb6a16ab022d","unresolved":false,"context_lines":[{"line_number":7,"context_line":"numa: fit instance topology with cpus reserved onto cores"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"In this commit we update method to pack a requested guest NUMA node"},{"line_number":10,"context_line":"topology with CPUs reserved onto a host NUMA node."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"implements blueprint libvirt-emulator-threads-policy"},{"line_number":13,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":27,"id":"9a30ddce_378ddf7b","line":10,"updated":"2017-03-08 22:21:50.000000000","message":"would be nice to mention in the commit message (and summary, frankly) exactly *what* you\u0027re updating the method to do. In the case of this patch, you\u0027re updating the packing method to account for emulator threads that are reserved on the host.","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"}],"nova/tests/unit/virt/test_hardware.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1c1b697da8e175eb9d04f65abca38ac02c44267e","unresolved":false,"context_lines":[{"line_number":2851,"context_line":""},{"line_number":2852,"context_line":""},{"line_number":2853,"context_line":"class CPUSOverheadCellTestCase(test.NoDBTestCase):"},{"line_number":2854,"context_line":"    def _test_overhead(self, overhead):"},{"line_number":2855,"context_line":"        host_topo \u003d objects.NUMACell(id\u003d0, cpuset\u003dset([0, 1, 2]),"},{"line_number":2856,"context_line":"                                     memory\u003d2048, memory_usage\u003d0, siblings\u003d[],"},{"line_number":2857,"context_line":"                                     mempages\u003d[], pinned_cpus\u003dset([]))"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba5201f7_51d67928","line":2854,"updated":"2017-01-06 18:09:55.000000000","message":"Would be good to s/overhead/cpus_reserved/ throughout these changes.","commit_id":"a9344f6df70a40c68950db93b2a54d0537989c62"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"76eca457823a2a7753e9e34d08afc3e190779434","unresolved":false,"context_lines":[{"line_number":2873,"context_line":""},{"line_number":2874,"context_line":""},{"line_number":2875,"context_line":"class CPUSReservedCellTestCase(test.NoDBTestCase):"},{"line_number":2876,"context_line":"    def _test_reserved(self, reserved):"},{"line_number":2877,"context_line":"        host_topo \u003d objects.NUMACell(id\u003d0, cpuset\u003dset([0, 1, 2]),"},{"line_number":2878,"context_line":"                                     memory\u003d2048, memory_usage\u003d0, siblings\u003d[],"},{"line_number":2879,"context_line":"                                     mempages\u003d[], pinned_cpus\u003dset([]))"}],"source_content_type":"text/x-python","patch_set":22,"id":"5a3905b3_dee0df98","line":2876,"updated":"2017-01-23 13:48:28.000000000","message":"The name of this method made think there would be an assertion here, not a return value.","commit_id":"2644e115a173f9867a6d1bc44363aef5646607de"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"23268093358f0f7e6bc36b03ccc2c4eab880ec9b","unresolved":false,"context_lines":[{"line_number":2873,"context_line":""},{"line_number":2874,"context_line":""},{"line_number":2875,"context_line":"class CPUSReservedCellTestCase(test.NoDBTestCase):"},{"line_number":2876,"context_line":"    def _test_reserved(self, reserved):"},{"line_number":2877,"context_line":"        host_topo \u003d objects.NUMACell(id\u003d0, cpuset\u003dset([0, 1, 2]),"},{"line_number":2878,"context_line":"                                     memory\u003d2048, memory_usage\u003d0, siblings\u003d[],"},{"line_number":2879,"context_line":"                                     mempages\u003d[], pinned_cpus\u003dset([]))"}],"source_content_type":"text/x-python","patch_set":22,"id":"5a3905b3_51be38d5","line":2876,"in_reply_to":"5a3905b3_dee0df98","updated":"2017-01-25 08:08:47.000000000","message":"I just noticed your comment, if i have to respin I will change the name","commit_id":"2644e115a173f9867a6d1bc44363aef5646607de"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e938bb5aeae0fb71771313d7f9cfeb6a16ab022d","unresolved":false,"context_lines":[{"line_number":2893,"context_line":""},{"line_number":2894,"context_line":"    def test_reserved_exceeded(self):"},{"line_number":2895,"context_line":"        inst_cell \u003d self._test_reserved(reserved\u003d2)"},{"line_number":2896,"context_line":"        self.assertIsNone(inst_cell)"},{"line_number":2897,"context_line":""},{"line_number":2898,"context_line":""},{"line_number":2899,"context_line":"class CPURealtimeTestCase(test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_2a4bb6b1","line":2896,"updated":"2017-03-08 22:21:50.000000000","message":"We will need a unit test that contains a host topology that includes \u003e1 NUMA cell in order to test the assertion from the spec that the emulator threads will *always* be pinned to a core contained in NUMA node 0.","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"b70880dbedce4c827e4073be8d0cf9ad69ca79e9","unresolved":false,"context_lines":[{"line_number":2893,"context_line":""},{"line_number":2894,"context_line":"    def test_reserved_exceeded(self):"},{"line_number":2895,"context_line":"        inst_cell \u003d self._test_reserved(reserved\u003d2)"},{"line_number":2896,"context_line":"        self.assertIsNone(inst_cell)"},{"line_number":2897,"context_line":""},{"line_number":2898,"context_line":""},{"line_number":2899,"context_line":"class CPURealtimeTestCase(test.NoDBTestCase):"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_1701a405","line":2896,"in_reply_to":"9a30ddce_2a4bb6b1","updated":"2017-03-09 11:42:23.000000000","message":"No no, in this commit we update the general logic based on *a* host NUMA node. That method \u0027numa_fit_instance_cell_with_pinning\u0027 takes a specific host NUMA node, requirements of an instance NUMA node, and the number of additional pCPUs which will have to be reserved on that specific host NUMA node.\n\nI think I understand why it\u0027s confusing, I should have used \u0027host_cell\u0027 instead of \u0027host_topo\u0027 (I will update the name)\n\nIn the next patch we will reserve one additional pCPU only for the instance NUMA node 0 and assert that [0].\n\n[0] https://review.openstack.org/#/c/400876/28/nova/tests/unit/virt/test_hardware.py","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"}],"nova/virt/hardware.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":673,"context_line":"                          describing the pinning requirements of the"},{"line_number":674,"context_line":"                          instance"},{"line_number":675,"context_line":"    :param threads_per_core: number of threads per core in host\u0027s cell"},{"line_number":676,"context_line":"    :param cpus_overhead: number of pCPUs reserved for overhead"},{"line_number":677,"context_line":""},{"line_number":678,"context_line":"    :returns: An instance of objects.InstanceNUMACell containing the"},{"line_number":679,"context_line":"              pinning information, and potentially a new topology to be"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_bf3adbcb","line":676,"range":{"start_line":676,"start_character":11,"end_line":676,"end_character":24},"updated":"2016-11-23 14:38:34.000000000","message":"This sounds odd - could we use \u0027overhead_cpus\u0027 instead","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":673,"context_line":"                          describing the pinning requirements of the"},{"line_number":674,"context_line":"                          instance"},{"line_number":675,"context_line":"    :param threads_per_core: number of threads per core in host\u0027s cell"},{"line_number":676,"context_line":"    :param cpus_overhead: number of pCPUs reserved for overhead"},{"line_number":677,"context_line":""},{"line_number":678,"context_line":"    :returns: An instance of objects.InstanceNUMACell containing the"},{"line_number":679,"context_line":"              pinning information, and potentially a new topology to be"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_c176e33d","line":676,"in_reply_to":"5a74a57a_bf3adbcb","updated":"2016-12-01 16:11:15.000000000","message":"If more contributors think that needs to be changed, I will do. From my point of view it\u0027s understable like it is and I don\u0027t want to lost engineering time on such change right now.","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":729,"context_line":"                                                        threads_per_core))"},{"line_number":730,"context_line":""},{"line_number":731,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores, cpus_overhead\u003d0):"},{"line_number":732,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping.\"\"\""},{"line_number":733,"context_line":"        if threads_no * len(sibling_set) \u003c len(instance_cores) + cpus_overhead:"},{"line_number":734,"context_line":"            return None, []"},{"line_number":735,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_9f735fb1","line":732,"range":{"start_line":732,"start_character":11,"end_line":732,"end_character":43},"updated":"2016-11-23 14:38:34.000000000","message":"Could you expand this docstring (including parameters) as this is a good deal more complicated that it used to be","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":729,"context_line":"                                                        threads_per_core))"},{"line_number":730,"context_line":""},{"line_number":731,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores, cpus_overhead\u003d0):"},{"line_number":732,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping.\"\"\""},{"line_number":733,"context_line":"        if threads_no * len(sibling_set) \u003c len(instance_cores) + cpus_overhead:"},{"line_number":734,"context_line":"            return None, []"},{"line_number":735,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_4199f36d","line":732,"in_reply_to":"5a74a57a_9f735fb1","updated":"2016-12-01 16:11:15.000000000","message":"Done","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":733,"context_line":"        if threads_no * len(sibling_set) \u003c len(instance_cores) + cpus_overhead:"},{"line_number":734,"context_line":"            return None, []"},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":737,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":738,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":739,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_7fa5a302","line":736,"range":{"start_line":736,"start_character":23,"end_line":736,"end_character":28},"updated":"2016-11-23 14:38:34.000000000","message":"Why do you need this? itertools.chain works with iterators, right?","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":733,"context_line":"        if threads_no * len(sibling_set) \u003c len(instance_cores) + cpus_overhead:"},{"line_number":734,"context_line":"            return None, []"},{"line_number":735,"context_line":""},{"line_number":736,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":737,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":738,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":739,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_5ccf18b0","line":736,"in_reply_to":"5a74a57a_7fa5a302","updated":"2016-12-01 16:11:15.000000000","message":"Yes but the problem is line 743 and for python3, because it\u0027s an iterator I can\u0027t do the \u0027pcpu in sib\u0027","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":737,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":738,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":739,"context_line":""},{"line_number":740,"context_line":"        # Updates the pCPUs used based on the previous compute of"},{"line_number":741,"context_line":"        # vCPUs pinning to compute the pCPUs used for overhead."},{"line_number":742,"context_line":"        for vcpu, pcpu in vcpus_pinning:"},{"line_number":743,"context_line":"            for sib in usable_cores:"},{"line_number":744,"context_line":"                if pcpu in sib:"},{"line_number":745,"context_line":"                    sib.remove(pcpu)"},{"line_number":746,"context_line":"        overhead \u003d list(itertools.chain(*usable_cores))[:cpus_overhead]"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"        return vcpus_pinning, overhead"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_9ff8bfe7","line":745,"range":{"start_line":740,"start_character":0,"end_line":745,"end_character":36},"updated":"2016-11-23 14:38:34.000000000","message":"I\u0027ve looked at this a couple of times, but I still don\u0027t get what\u0027s happening here. Could you expand the comment?","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":742,"context_line":"        for vcpu, pcpu in vcpus_pinning:"},{"line_number":743,"context_line":"            for sib in usable_cores:"},{"line_number":744,"context_line":"                if pcpu in sib:"},{"line_number":745,"context_line":"                    sib.remove(pcpu)"},{"line_number":746,"context_line":"        overhead \u003d list(itertools.chain(*usable_cores))[:cpus_overhead]"},{"line_number":747,"context_line":""},{"line_number":748,"context_line":"        return vcpus_pinning, overhead"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_268ded31","line":745,"in_reply_to":"5a74a57a_9ff8bfe7","updated":"2016-12-01 16:11:15.000000000","message":"Done","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":763,"context_line":"        LOG.debug(\"User did not specify a thread policy. Using default \""},{"line_number":764,"context_line":"                  \"for %d cores\", len(instance_cell))"},{"line_number":765,"context_line":""},{"line_number":766,"context_line":"    overhead \u003d []"},{"line_number":767,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":768,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":769,"context_line":"        # make sure we have at least one fully free core"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_7f41230b","line":766,"range":{"start_line":766,"start_character":4,"end_line":766,"end_character":17},"updated":"2016-11-23 14:38:34.000000000","message":"can you declare this on line 691, along with pinning?","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":763,"context_line":"        LOG.debug(\"User did not specify a thread policy. Using default \""},{"line_number":764,"context_line":"                  \"for %d cores\", len(instance_cell))"},{"line_number":765,"context_line":""},{"line_number":766,"context_line":"    overhead \u003d []"},{"line_number":767,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":768,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":769,"context_line":"        # make sure we have at least one fully free core"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_61034f74","line":766,"in_reply_to":"5a74a57a_7f41230b","updated":"2016-12-01 16:11:15.000000000","message":"Why ?","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":773,"context_line":"                      \u0027for the isolate policy without this.\u0027)"},{"line_number":774,"context_line":"            return"},{"line_number":775,"context_line":""},{"line_number":776,"context_line":"        pinning, overhead \u003d _get_pinning("},{"line_number":777,"context_line":"            1,  # we only want to \"use\" one thread per core"},{"line_number":778,"context_line":"            sibling_sets[threads_per_core],"},{"line_number":779,"context_line":"            instance_cell.cpuset,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_9f143ff6","line":776,"range":{"start_line":776,"start_character":8,"end_line":776,"end_character":41},"updated":"2016-11-23 14:38:34.000000000","message":"This seems like we\u0027re overloading the \u0027_get_pinning\u0027 function to do stuff it doesn\u0027t care about. Isn\u0027t it possible to add a distinct \u0027_get_overhead\u0027 function? If not, this function should at least be renamed.","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":773,"context_line":"                      \u0027for the isolate policy without this.\u0027)"},{"line_number":774,"context_line":"            return"},{"line_number":775,"context_line":""},{"line_number":776,"context_line":"        pinning, overhead \u003d _get_pinning("},{"line_number":777,"context_line":"            1,  # we only want to \"use\" one thread per core"},{"line_number":778,"context_line":"            sibling_sets[threads_per_core],"},{"line_number":779,"context_line":"            instance_cell.cpuset,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_0688f121","line":776,"in_reply_to":"5a74a57a_9f143ff6","updated":"2016-12-01 16:11:15.000000000","message":"_get_pinning is returning the pinning used for the guest. It\u0027s exactly what it\u0027s done here.","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":820,"context_line":"    instance_cell.pin_vcpus(*pinning)"},{"line_number":821,"context_line":"    instance_cell.cpu_topology \u003d topology"},{"line_number":822,"context_line":"    instance_cell.id \u003d host_cell_id"},{"line_number":823,"context_line":"    instance_cell.cpus_overhead \u003d set(overhead)"},{"line_number":824,"context_line":"    return instance_cell"},{"line_number":825,"context_line":""},{"line_number":826,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_5f6387b1","line":823,"range":{"start_line":823,"start_character":34,"end_line":823,"end_character":38},"updated":"2016-11-23 14:38:34.000000000","message":"Does this need to be a set?","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":820,"context_line":"    instance_cell.pin_vcpus(*pinning)"},{"line_number":821,"context_line":"    instance_cell.cpu_topology \u003d topology"},{"line_number":822,"context_line":"    instance_cell.id \u003d host_cell_id"},{"line_number":823,"context_line":"    instance_cell.cpus_overhead \u003d set(overhead)"},{"line_number":824,"context_line":"    return instance_cell"},{"line_number":825,"context_line":""},{"line_number":826,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_c61a7917","line":823,"in_reply_to":"5a74a57a_5f6387b1","updated":"2016-12-01 16:11:15.000000000","message":"Yes, since the driver layer will have to use that field to pick the pCPUs used to isolate emulator threads on.","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"f57db9614e47eb01f26f4b759f046fefbbe12a4f","unresolved":false,"context_lines":[{"line_number":840,"context_line":"    if host_cell.avail_cpus \u003c len(instance_cell.cpuset) + cpus_overhead:"},{"line_number":841,"context_line":"        LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":842,"context_line":"                  \u0027Oversubscription is not possible with pinned instances. \u0027"},{"line_number":843,"context_line":"                  \u0027Required: %(required)s + %(cpus_overhead)d, \u0027"},{"line_number":844,"context_line":"                  \u0027actual: %(actual)s\u0027,"},{"line_number":845,"context_line":"                  {\u0027required\u0027: len(instance_cell.cpuset),"},{"line_number":846,"context_line":"                   \u0027actual\u0027: host_cell.avail_cpus,"}],"source_content_type":"text/x-python","patch_set":2,"id":"5a74a57a_bfa3bb9d","line":843,"range":{"start_line":843,"start_character":29,"end_line":843,"end_character":64},"updated":"2016-11-23 14:38:34.000000000","message":"IMO, this would be better formatted like so:\n\n    \u0027Required: %(total_req)d (%(pin_req)d + %(overhead_req)d), \u0027\n\nor similar. This gives you the breakdown of the totals while still showing the actual total required","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"4acfb5032797ed285bf4041649803721ce8d5f08","unresolved":false,"context_lines":[{"line_number":840,"context_line":"    if host_cell.avail_cpus \u003c len(instance_cell.cpuset) + cpus_overhead:"},{"line_number":841,"context_line":"        LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":842,"context_line":"                  \u0027Oversubscription is not possible with pinned instances. \u0027"},{"line_number":843,"context_line":"                  \u0027Required: %(required)s + %(cpus_overhead)d, \u0027"},{"line_number":844,"context_line":"                  \u0027actual: %(actual)s\u0027,"},{"line_number":845,"context_line":"                  {\u0027required\u0027: len(instance_cell.cpuset),"},{"line_number":846,"context_line":"                   \u0027actual\u0027: host_cell.avail_cpus,"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a71b18c_01953be3","line":843,"in_reply_to":"5a74a57a_bfa3bb9d","updated":"2016-12-01 16:11:15.000000000","message":"Not sure if that is really relevant but If i have to respin I will add the total of \u0027vCPUs + cpus_overhead\u0027","commit_id":"ecc224c48168cb5911bd4e09bd2dac92b0ee7001"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1c1b697da8e175eb9d04f65abca38ac02c44267e","unresolved":false,"context_lines":[{"line_number":749,"context_line":""},{"line_number":750,"context_line":"        # Determines the pCPUs used for overhead"},{"line_number":751,"context_line":"        overhead \u003d list(itertools.chain(*usable_cores))[:cpus_reserved]"},{"line_number":752,"context_line":"        LOG.debug(\"Computing pinning: pCPUs overhead: %s\", overhead)"},{"line_number":753,"context_line":""},{"line_number":754,"context_line":"        return vcpus_pinning, overhead"},{"line_number":755,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"ba5201f7_91fd4170","line":752,"updated":"2017-01-06 18:09:55.000000000","message":"Recommend combining the two debug messages into one, and removing the mention of overhead. How about getting rid of line 748 and here doing something like this?\n\n msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \"\n           \"%(usable_cores)s, reserved pCPUs: %(reserved)s\")\n msg_args \u003d {\n     \u0027usable_cores\u0027: usable_cores,\n     \u0027reserved\u0027: overhead,\n }\n LOG.info(msg, msg_args)","commit_id":"a9344f6df70a40c68950db93b2a54d0537989c62"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"d8427ce30d3c279a7a96740e52b94dd6bf8d7f30","unresolved":false,"context_lines":[{"line_number":749,"context_line":""},{"line_number":750,"context_line":"        # Determines the pCPUs used for overhead"},{"line_number":751,"context_line":"        overhead \u003d list(itertools.chain(*usable_cores))[:cpus_reserved]"},{"line_number":752,"context_line":"        LOG.debug(\"Computing pinning: pCPUs overhead: %s\", overhead)"},{"line_number":753,"context_line":""},{"line_number":754,"context_line":"        return vcpus_pinning, overhead"},{"line_number":755,"context_line":""}],"source_content_type":"text/x-python","patch_set":15,"id":"ba5201f7_7047d04b","line":752,"in_reply_to":"ba5201f7_91fd4170","updated":"2017-01-10 08:48:37.000000000","message":"Yes totally better, based on your comment I changed a little bit the log messages, please let me know if that is ok for you","commit_id":"a9344f6df70a40c68950db93b2a54d0537989c62"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"1c1b697da8e175eb9d04f65abca38ac02c44267e","unresolved":false,"context_lines":[{"line_number":769,"context_line":"        LOG.debug(\"User did not specify a thread policy. Using default \""},{"line_number":770,"context_line":"                  \"for %d cores\", len(instance_cell))"},{"line_number":771,"context_line":""},{"line_number":772,"context_line":"    overhead \u003d []"},{"line_number":773,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":774,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":775,"context_line":"        # make sure we have at least one fully free core"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba5201f7_f13dad2b","line":772,"updated":"2017-01-06 18:09:55.000000000","message":"here and throughout, recommend s/overhead/reserved/ just to be consistent with the object attribute name now.\n\nIt might also be a good idea to call the variable num_cpu_reserved that refers to the number of pCPUs to reserve as opposed to cpus_reserved which refers to the *set* of the physical CPU cores that are reserved...","commit_id":"a9344f6df70a40c68950db93b2a54d0537989c62"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"d8427ce30d3c279a7a96740e52b94dd6bf8d7f30","unresolved":false,"context_lines":[{"line_number":769,"context_line":"        LOG.debug(\"User did not specify a thread policy. Using default \""},{"line_number":770,"context_line":"                  \"for %d cores\", len(instance_cell))"},{"line_number":771,"context_line":""},{"line_number":772,"context_line":"    overhead \u003d []"},{"line_number":773,"context_line":"    if (instance_cell.cpu_thread_policy \u003d\u003d"},{"line_number":774,"context_line":"            fields.CPUThreadAllocationPolicy.ISOLATE):"},{"line_number":775,"context_line":"        # make sure we have at least one fully free core"}],"source_content_type":"text/x-python","patch_set":15,"id":"ba5201f7_19dcef73","line":772,"in_reply_to":"ba5201f7_f13dad2b","updated":"2017-01-10 08:48:37.000000000","message":"Yes it\u0027s very true, I should not mix cpus_reserved which is a set and the num of cpus we want to reserve \u0027num_cpu_reserved\u0027\n\nI corrected that point and addressed your comments","commit_id":"a9344f6df70a40c68950db93b2a54d0537989c62"},{"author":{"_account_id":8802,"name":"Vladik Romanovsky","email":"vromanso@redhat.com","username":"vladikr"},"change_message_id":"72f509f7baab31c428b179c8b75003c85be05264","unresolved":false,"context_lines":[{"line_number":752,"context_line":"            for vcpu, pcpu in vcpus_pinning:"},{"line_number":753,"context_line":"                for sib in usable_cores:"},{"line_number":754,"context_line":"                    if pcpu in sib:"},{"line_number":755,"context_line":"                        sib.remove(pcpu)"},{"line_number":756,"context_line":""},{"line_number":757,"context_line":"            # Determines the pCPUs reserved for hypervisor"},{"line_number":758,"context_line":"            cpus_reserved \u003d set(list("}],"source_content_type":"text/x-python","patch_set":18,"id":"5a3905b3_e7405e0a","line":755,"range":{"start_line":755,"start_character":24,"end_line":755,"end_character":40},"updated":"2017-01-19 04:36:50.000000000","message":"For some reason, I was sure we want to reserve a full core and not just one sibling, but I can\u0027t any mentioning about it in the spec atm.","commit_id":"341a021b0166edfa77511773e596dd79e8d67d13"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"a633a30f2710285ebccea080d7d9cf99da0de19c","unresolved":false,"context_lines":[{"line_number":728,"context_line":"        return fractions.gcd(threads_per_core, _orphans(instance_cell,"},{"line_number":729,"context_line":"                                                        threads_per_core))"},{"line_number":730,"context_line":""},{"line_number":731,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores,"},{"line_number":732,"context_line":"                     num_cpu_reserved\u003d0):"},{"line_number":733,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping and pCPUs reserved for hypervisor\"\"\""},{"line_number":734,"context_line":"        if threads_no * len(sibling_set) \u003c ("}],"source_content_type":"text/x-python","patch_set":20,"id":"5a3905b3_d2bd8fd8","line":731,"updated":"2017-01-20 12:03:54.000000000","message":"Do we have any benefit from having _get_pinning inside _pack_instance_onto_cores. Can you make it non-nested function?","commit_id":"98a01a303d86a0976d178c55191f5f673696eb5f"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"cbe8f4f99bfbe8de8c50010dbe808844d5eb1a60","unresolved":false,"context_lines":[{"line_number":728,"context_line":"        return fractions.gcd(threads_per_core, _orphans(instance_cell,"},{"line_number":729,"context_line":"                                                        threads_per_core))"},{"line_number":730,"context_line":""},{"line_number":731,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores,"},{"line_number":732,"context_line":"                     num_cpu_reserved\u003d0):"},{"line_number":733,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping and pCPUs reserved for hypervisor\"\"\""},{"line_number":734,"context_line":"        if threads_no * len(sibling_set) \u003c ("}],"source_content_type":"text/x-python","patch_set":20,"id":"5a3905b3_bc04284a","line":731,"in_reply_to":"5a3905b3_610403c0","updated":"2017-01-21 19:17:05.000000000","message":"My concern was function became more complex and we couldn\u0027t test it directly, for me benefit is ability to test, but in this case, yes, we can definitely test it via _pack_instance_onto_cores.","commit_id":"98a01a303d86a0976d178c55191f5f673696eb5f"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"85be9b4fb9fc6d4a7616607d07c68083ddab9844","unresolved":false,"context_lines":[{"line_number":728,"context_line":"        return fractions.gcd(threads_per_core, _orphans(instance_cell,"},{"line_number":729,"context_line":"                                                        threads_per_core))"},{"line_number":730,"context_line":""},{"line_number":731,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores,"},{"line_number":732,"context_line":"                     num_cpu_reserved\u003d0):"},{"line_number":733,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping and pCPUs reserved for hypervisor\"\"\""},{"line_number":734,"context_line":"        if threads_no * len(sibling_set) \u003c ("}],"source_content_type":"text/x-python","patch_set":20,"id":"5a3905b3_09f52582","line":731,"in_reply_to":"5a3905b3_d2bd8fd8","updated":"2017-01-20 13:53:02.000000000","message":"I would say it\u0027s not really related to this change, that can be discussed on an other specific patch","commit_id":"98a01a303d86a0976d178c55191f5f673696eb5f"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"778a17b239d66105ee3e2dfb065b9f534e47e003","unresolved":false,"context_lines":[{"line_number":728,"context_line":"        return fractions.gcd(threads_per_core, _orphans(instance_cell,"},{"line_number":729,"context_line":"                                                        threads_per_core))"},{"line_number":730,"context_line":""},{"line_number":731,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores,"},{"line_number":732,"context_line":"                     num_cpu_reserved\u003d0):"},{"line_number":733,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping and pCPUs reserved for hypervisor\"\"\""},{"line_number":734,"context_line":"        if threads_no * len(sibling_set) \u003c ("}],"source_content_type":"text/x-python","patch_set":20,"id":"5a3905b3_610403c0","line":731,"in_reply_to":"5a3905b3_d2bd8fd8","updated":"2017-01-20 17:13:56.000000000","message":"It was originally like this... not sure there\u0027s much benefit to having it as a non-nested function.","commit_id":"98a01a303d86a0976d178c55191f5f673696eb5f"},{"author":{"_account_id":21813,"name":"Andrey Volkov","email":"m@amadev.ru","username":"avolkov"},"change_message_id":"a633a30f2710285ebccea080d7d9cf99da0de19c","unresolved":false,"context_lines":[{"line_number":749,"context_line":"        cpus_reserved \u003d None"},{"line_number":750,"context_line":"        if num_cpu_reserved:"},{"line_number":751,"context_line":"            # Updates the pCPUs used based on the previous mapping"},{"line_number":752,"context_line":"            for vcpu, pcpu in vcpus_pinning:"},{"line_number":753,"context_line":"                for sib in usable_cores:"},{"line_number":754,"context_line":"                    if pcpu in sib:"},{"line_number":755,"context_line":"                        sib.remove(pcpu)"}],"source_content_type":"text/x-python","patch_set":20,"id":"5a3905b3_f5ab5587","line":752,"updated":"2017-01-20 12:03:54.000000000","message":"Can we do something like:\n\nused_cores \u003d {pcpu for _, pcpu in vcpus_pinning}\nusable_cores \u003d [set(sib) - used_cores for sib in usable_cores]","commit_id":"98a01a303d86a0976d178c55191f5f673696eb5f"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"85be9b4fb9fc6d4a7616607d07c68083ddab9844","unresolved":false,"context_lines":[{"line_number":749,"context_line":"        cpus_reserved \u003d None"},{"line_number":750,"context_line":"        if num_cpu_reserved:"},{"line_number":751,"context_line":"            # Updates the pCPUs used based on the previous mapping"},{"line_number":752,"context_line":"            for vcpu, pcpu in vcpus_pinning:"},{"line_number":753,"context_line":"                for sib in usable_cores:"},{"line_number":754,"context_line":"                    if pcpu in sib:"},{"line_number":755,"context_line":"                        sib.remove(pcpu)"}],"source_content_type":"text/x-python","patch_set":20,"id":"5a3905b3_a936d14f","line":752,"in_reply_to":"5a3905b3_f5ab5587","updated":"2017-01-20 13:53:02.000000000","message":"Hum probably...","commit_id":"98a01a303d86a0976d178c55191f5f673696eb5f"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7f1875c3247b8e164920e27c228dd4b69dcfc621","unresolved":false,"context_lines":[{"line_number":735,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":736,"context_line":"            return None, None"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":739,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":740,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":741,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":23,"id":"5a3905b3_829b0872","line":738,"range":{"start_line":738,"start_character":28,"end_line":738,"end_character":76},"updated":"2017-01-25 14:19:10.000000000","message":"Boy, does Nikola like functional programming","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"93ccd246c3604c4b4381721ce7b1ba64529827bd","unresolved":false,"context_lines":[{"line_number":735,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":736,"context_line":"            return None, None"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":739,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":740,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":741,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":23,"id":"3a461143_53dc56e4","line":738,"in_reply_to":"3a461143_17c2c885","updated":"2017-01-26 17:11:28.000000000","message":"@sahid: Stephen was merely pointing out that Nikola uses a lot of functional programming constructs and paradigms in his code style, which is different from most of the rest of Nova\u0027s code style, that\u0027s all. Not bad or good, just different.","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"82559c040df66f6d4a3a2dd1b8cf738fb4d38ab1","unresolved":false,"context_lines":[{"line_number":735,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":736,"context_line":"            return None, None"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":739,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":740,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":741,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":23,"id":"3a461143_c2031d72","line":738,"in_reply_to":"3a461143_17c2c885","updated":"2017-01-26 14:11:21.000000000","message":"Ah, sorry. That was a snarky comment that should have been followed by a smiley face to indicate as much [1]. Sorry for any confusion\n\n[1] https://en.wikipedia.org/wiki/Poe\u0027s_law","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"9226683846290fd9c3ebaa70c2b4d13b1b3f3bfc","unresolved":false,"context_lines":[{"line_number":735,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":736,"context_line":"            return None, None"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":739,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":740,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":741,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":23,"id":"3a461143_17c2c885","line":738,"in_reply_to":"5a3905b3_829b0872","updated":"2017-01-26 13:09:19.000000000","message":"What do you mean by such comment? Nikola did a great work, he implemented something new and complex which is used by a bunch of industries and even if we can make this better we should try to keep humility on what has been implemented.","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7f1875c3247b8e164920e27c228dd4b69dcfc621","unresolved":false,"context_lines":[{"line_number":845,"context_line":""},{"line_number":846,"context_line":""},{"line_number":847,"context_line":"def _numa_fit_instance_cell_with_pinning(host_cell, instance_cell,"},{"line_number":848,"context_line":"                                         num_cpu_reserved\u003d0):"},{"line_number":849,"context_line":"    \"\"\"Determine if cells can be pinned to a host cell."},{"line_number":850,"context_line":""},{"line_number":851,"context_line":"    :param host_cell: objects.NUMACell instance - the host cell that"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a461143_40acbeed","line":848,"range":{"start_line":848,"start_character":40,"end_line":848,"end_character":59},"updated":"2017-01-25 14:19:10.000000000","message":"The way I understand this, we will only ever be reserving one core per instance, right? If that\u0027s the case, why is packing instance cores a cell-level operation. I imagine we\u0027d have a lot of calls like this:\n\n  _numa_fit_instance_cell_with_pinning(host_0, guest_0, 1)\n  _numa_fit_instance_cell_with_pinning(host_1, guest_1, 0)\n  _numa_fit_instance_cell_with_pinning(host_2, guest_2, 0)\n  ...\n  _numa_fit_instance_cell_with_pinning(host_N, guest_N, 0)\n\ni.e. every non-cell0 call to this function will have num_cpu_reserved\u003d0. Wouldn\u0027t it be much less convoluted to add a specific \"fit overhead\" method along the lines of:\n\n  _numa_fit_instance_cell_with_pinning(host_0, guest_0)\n  _numa_fit_instance_cell_with_pinning(host_1, guest_1)\n  _numa_fit_instance_cell_with_pinning(host_2, guest_2)\n  ...\n  _numa_fit_instance_cell_with_pinning(host_N, guest_N)\n  _numa_fit_instance_overhead(host_0, 1)\n\nOr am I missing some subtlety?","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"9226683846290fd9c3ebaa70c2b4d13b1b3f3bfc","unresolved":false,"context_lines":[{"line_number":845,"context_line":""},{"line_number":846,"context_line":""},{"line_number":847,"context_line":"def _numa_fit_instance_cell_with_pinning(host_cell, instance_cell,"},{"line_number":848,"context_line":"                                         num_cpu_reserved\u003d0):"},{"line_number":849,"context_line":"    \"\"\"Determine if cells can be pinned to a host cell."},{"line_number":850,"context_line":""},{"line_number":851,"context_line":"    :param host_cell: objects.NUMACell instance - the host cell that"}],"source_content_type":"text/x-python","patch_set":23,"id":"3a461143_c95c2828","line":848,"in_reply_to":"3a461143_40acbeed","updated":"2017-01-26 13:09:19.000000000","message":"If you look the method _get_pinning you can see the relations between the pCPUs usage, vCPUs mapping and pCPUs reserved for overhead and why it\u0027s more complex to separate them. So except the fact that your solution is an other way to do it and implies probably more changes, it does not make any differences.","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"acae0b79f7ce9ea73bcb675b31a480f5917e7fc1","unresolved":false,"context_lines":[{"line_number":845,"context_line":""},{"line_number":846,"context_line":""},{"line_number":847,"context_line":"def _numa_fit_instance_cell_with_pinning(host_cell, instance_cell,"},{"line_number":848,"context_line":"                                         num_cpu_reserved\u003d0):"},{"line_number":849,"context_line":"    \"\"\"Determine if cells can be pinned to a host cell."},{"line_number":850,"context_line":""},{"line_number":851,"context_line":"    :param host_cell: objects.NUMACell instance - the host cell that"}],"source_content_type":"text/x-python","patch_set":23,"id":"9a30ddce_ba5a824e","line":848,"in_reply_to":"3a461143_c95c2828","updated":"2017-03-09 10:22:05.000000000","message":"\u003e If you look the method _get_pinning you can see the relations between the pCPUs usage, vCPUs mapping and pCPUs reserved for overhead and why it\u0027s more complex to separate them.\n\nI\u0027m looking but I don\u0027t see the relationship you\u0027re talking about? I see only two lines of actual shared code - the \u0027usable_cores\u0027 and \u0027vcpus_pinning\u0027 lines - plus some logs, all of which could easily go in a common function. Other than that, well over 50% of the code in that function now applies to the \u0027num_cpu_reserved\u0027 case meaning there\u0027s a new function required. These should be separated.","commit_id":"7bcebc3093ce78356bcf73e40cff32a599daf056"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2b72660612859494514237c1692e20d06e5dbb29","unresolved":false,"context_lines":[{"line_number":650,"context_line":"                              instance_cell,"},{"line_number":651,"context_line":"                              host_cell_id,"},{"line_number":652,"context_line":"                              threads_per_core\u003d1,"},{"line_number":653,"context_line":"                              num_cpu_reserved\u003d0):"},{"line_number":654,"context_line":"    \"\"\"Pack an instance onto a set of siblings."},{"line_number":655,"context_line":""},{"line_number":656,"context_line":"    Calculate the pinning for the given instance and its topology,"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_faff9aa7","line":653,"range":{"start_line":653,"start_character":30,"end_line":653,"end_character":48},"updated":"2017-03-09 10:23:16.000000000","message":"I think this came up in a previous review, but can you remind me again why this is an integer and not a boolean? We only support reserving a single CPU, right? If there\u0027s a good reason for this, could you document it in the commit message and/or docstring?","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e938bb5aeae0fb71771313d7f9cfeb6a16ab022d","unresolved":false,"context_lines":[{"line_number":741,"context_line":""},{"line_number":742,"context_line":"    def _get_pinning(threads_no, sibling_set, instance_cores,"},{"line_number":743,"context_line":"                     num_cpu_reserved\u003d0):"},{"line_number":744,"context_line":"        \"\"\"Generate a CPU-vCPU pin mapping and pCPUs reserved for hypervisor\"\"\""},{"line_number":745,"context_line":"        if threads_no * len(sibling_set) \u003c ("},{"line_number":746,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":747,"context_line":"            return None, None"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_176e1b95","line":744,"updated":"2017-03-08 22:21:50.000000000","message":"Please expand this docstring with a description of the parameters and the returned values and their types. See the docstrings above for the _orphans() and _threads() functions and how detailed they are? Let\u0027s try to get similar level of descriptiveness here.","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e938bb5aeae0fb71771313d7f9cfeb6a16ab022d","unresolved":false,"context_lines":[{"line_number":746,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":747,"context_line":"            return None, None"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":750,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":751,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":752,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_b7e1cf00","line":749,"updated":"2017-03-08 22:21:50.000000000","message":"This is where a nice, meaty code comment would come in very handy. Remember that people read code 100X more often than it is written, and reading this code is incredibly difficult as it stands now.","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2b72660612859494514237c1692e20d06e5dbb29","unresolved":false,"context_lines":[{"line_number":746,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":747,"context_line":"            return None, None"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":750,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":751,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":752,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_fa9b1a70","line":749,"in_reply_to":"9a30ddce_b7e1cf00","updated":"2017-03-09 10:23:16.000000000","message":"Per comments in PS23 [1], I really think we\u0027re overloading the purpose of this function to do two mostly independent things. This would be smaller and easier to read were we to keep separate functions for pinning cores and pinning emulator threads.\n\n[1] https://review.openstack.org/#/c/400875/23/nova/virt/hardware.py@848","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"b70880dbedce4c827e4073be8d0cf9ad69ca79e9","unresolved":false,"context_lines":[{"line_number":746,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":747,"context_line":"            return None, None"},{"line_number":748,"context_line":""},{"line_number":749,"context_line":"        usable_cores \u003d list(map(lambda s: list(s)[:threads_no], sibling_set))"},{"line_number":750,"context_line":"        vcpus_pinning \u003d list(zip(sorted(instance_cores),"},{"line_number":751,"context_line":"                                 itertools.chain(*usable_cores)))"},{"line_number":752,"context_line":"        msg \u003d _LI(\"Computed NUMA topology CPU pinning: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_37d7884c","line":749,"in_reply_to":"9a30ddce_fa9b1a70","updated":"2017-03-09 11:42:23.000000000","message":"Sure Jay, seems reasonable to add a comment here.","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e938bb5aeae0fb71771313d7f9cfeb6a16ab022d","unresolved":false,"context_lines":[{"line_number":767,"context_line":""},{"line_number":768,"context_line":"            # Determines the pCPUs reserved for hypervisor"},{"line_number":769,"context_line":"            cpuset_reserved \u003d set(list("},{"line_number":770,"context_line":"                itertools.chain(*usable_cores))[:num_cpu_reserved])"},{"line_number":771,"context_line":"            msg \u003d _LI(\"Computed NUMA topology reserved pCPUs: usable pCPUs: \""},{"line_number":772,"context_line":"                      \"%(usable_cores)s, reserved pCPUs: %(cpuset_reserved)s\")"},{"line_number":773,"context_line":"            msg_args \u003d {"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_4abd4a67","line":770,"updated":"2017-03-08 22:21:50.000000000","message":"The spec asserts that in order to keep things \"predictable for users\", the emulator threads will always be pinned to a physical core in NUMA cell 0.\n\nI don\u0027t see anything in this code that raises an exception if the physical core(s) selected by this routine don\u0027t end up on NUMA cell 0.","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"2b72660612859494514237c1692e20d06e5dbb29","unresolved":false,"context_lines":[{"line_number":767,"context_line":""},{"line_number":768,"context_line":"            # Determines the pCPUs reserved for hypervisor"},{"line_number":769,"context_line":"            cpuset_reserved \u003d set(list("},{"line_number":770,"context_line":"                itertools.chain(*usable_cores))[:num_cpu_reserved])"},{"line_number":771,"context_line":"            msg \u003d _LI(\"Computed NUMA topology reserved pCPUs: usable pCPUs: \""},{"line_number":772,"context_line":"                      \"%(usable_cores)s, reserved pCPUs: %(cpuset_reserved)s\")"},{"line_number":773,"context_line":"            msg_args \u003d {"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_ba78c22c","line":770,"in_reply_to":"9a30ddce_4abd4a67","updated":"2017-03-09 10:23:16.000000000","message":"This would be easier to assert on were these functions separate. See comments on PS23 [1]\n\n[1] https://review.openstack.org/#/c/400875/23/nova/virt/hardware.py@848","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"b70880dbedce4c827e4073be8d0cf9ad69ca79e9","unresolved":false,"context_lines":[{"line_number":767,"context_line":""},{"line_number":768,"context_line":"            # Determines the pCPUs reserved for hypervisor"},{"line_number":769,"context_line":"            cpuset_reserved \u003d set(list("},{"line_number":770,"context_line":"                itertools.chain(*usable_cores))[:num_cpu_reserved])"},{"line_number":771,"context_line":"            msg \u003d _LI(\"Computed NUMA topology reserved pCPUs: usable pCPUs: \""},{"line_number":772,"context_line":"                      \"%(usable_cores)s, reserved pCPUs: %(cpuset_reserved)s\")"},{"line_number":773,"context_line":"            msg_args \u003d {"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_777f4066","line":770,"in_reply_to":"9a30ddce_ba78c22c","updated":"2017-03-09 11:42:23.000000000","message":"We do not have to assert anything related to the NUMA node 0 here. In the next patch we will pass to that method \u0027num_cpu_reserved\u003d1\u0027 for the instance NUMA 0 when emulator thread isolation is enabled [0].\n\n[0] https://review.openstack.org/#/c/400876/","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"e938bb5aeae0fb71771313d7f9cfeb6a16ab022d","unresolved":false,"context_lines":[{"line_number":872,"context_line":"    :returns: objects.InstanceNUMACell instance with pinning information,"},{"line_number":873,"context_line":"              or None if instance cannot be pinned to the given host"},{"line_number":874,"context_line":"    \"\"\""},{"line_number":875,"context_line":"    if host_cell.avail_cpus \u003c len(instance_cell.cpuset) + num_cpu_reserved:"},{"line_number":876,"context_line":"        LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":877,"context_line":"                  \u0027Oversubscription is not possible with pinned instances. \u0027"},{"line_number":878,"context_line":"                  \u0027Required: %(required)d (%(vcpus)d + %(num_cpu_reserved)d), \u0027"}],"source_content_type":"text/x-python","patch_set":27,"id":"9a30ddce_0a5a724f","line":875,"updated":"2017-03-08 22:21:50.000000000","message":"nit: You could clean up lines 875 and 880 by declaring a local variable \"required_cpus\" above here.\n\n required_cpus \u003d len(instance_cell.cpu_set) + num_cpu_reserved\n if host_cell.avail_cpus \u003c required_cpus:\n     ...","commit_id":"9094f8d6ea06939791e4dde326c033d0050d2f8a"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"40a9fc9bdbddf182e5228ceda9085a606361cd40","unresolved":false,"context_lines":[{"line_number":666,"context_line":"    will not currently try to look for a fit that maximizes number of"},{"line_number":667,"context_line":"    siblings, but will simply rely on the iteration ordering and picking"},{"line_number":668,"context_line":"    the first viable placement."},{"line_number":669,"context_line":""},{"line_number":670,"context_line":"    :param available_siblings: list of sets of CPU IDs corresponding to"},{"line_number":671,"context_line":"                               available siblings per core"},{"line_number":672,"context_line":"    :param instance_cell: An instance of objects.InstanceNUMACell"}],"source_content_type":"text/x-python","patch_set":28,"id":"3a1ff146_13e77613","line":669,"updated":"2017-03-14 15:37:25.000000000","message":"Add a note here explaining that this also handles the pinning of CPUs used for emulator thread, and why it\u0027s necessary to do this.\n\nYou can probably add the note on why this is necessary here","commit_id":"2c13aa14da92532ad8262f9de97513a5cfe39345"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ebc0e27259e3da516ff5452bc70c9cc0c4041e71","unresolved":false,"context_lines":[{"line_number":758,"context_line":"        NOTE: Depending on how host is configured (HT/non-HT) a thread can"},{"line_number":759,"context_line":"              be considered as an entire core."},{"line_number":760,"context_line":"        \"\"\""},{"line_number":761,"context_line":"        if threads_no * len(sibling_set) \u003c ("},{"line_number":762,"context_line":"                len(instance_cores) + num_cpu_reserved):"},{"line_number":763,"context_line":"            return None, None"},{"line_number":764,"context_line":""},{"line_number":765,"context_line":"        # Determines usable cores according the \"threads number\""},{"line_number":766,"context_line":"        # constraint."}],"source_content_type":"text/x-python","patch_set":28,"id":"5a22e58a_fc2a9f7d","line":763,"range":{"start_line":761,"start_character":8,"end_line":763,"end_character":29},"updated":"2017-03-14 13:14:38.000000000","message":"There\u0027s nothing dictating that CPU thread policies should apply to emulator threads. However, in the isolate case, the combining of these would enforce a requirements X + Y *fully free* (i.e. all threads unused) pCPUs, where X is the number of instance vCPUs and Y is the number of emulator threads. What we should instead be checking is that we have X fully free cores, and Y other free cores, regardless of what else is on them. This is not a significant problem here, seeing as we only support a single reserved CPU, but it will make scheduling incredibly tough as we add support for more than one reserved CPU. This will also affect all other cases except \u0027prefer\u0027, albeit to a lesser degree.\n\nThis doesn\u0027t need to be fixed now, but I think a note about this effect should be included for future authors of this function.","commit_id":"2c13aa14da92532ad8262f9de97513a5cfe39345"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"0cc00fa9b77e2022750f053cf83112e5efaf2085","unresolved":false,"context_lines":[{"line_number":655,"context_line":""},{"line_number":656,"context_line":"    Calculate the pinning for the given instance and its topology,"},{"line_number":657,"context_line":"    making sure that hyperthreads of the instance match up with those"},{"line_number":658,"context_line":"    of the host when the pinning takes effect. Also ensure that the"},{"line_number":659,"context_line":"    physical cores reserved for hypervisor on this host NUMA node do"},{"line_number":660,"context_line":"    not break any thread policies."},{"line_number":661,"context_line":""},{"line_number":662,"context_line":"    Currently the strategy for packing is to prefer siblings and try use"},{"line_number":663,"context_line":"    cores evenly by using emptier cores first. This is achieved by the"}],"source_content_type":"text/x-python","patch_set":30,"id":"1a1ced50_ffc92d5a","line":660,"range":{"start_line":658,"start_character":47,"end_line":660,"end_character":34},"updated":"2017-03-21 14:53:35.000000000","message":"I\u0027d have liked a little more detail about this, given how difficult it was to tease this out. We can address this after the fact now though","commit_id":"7bc93ed40294fea21a0d5911226ab0121221f0bf"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"9c1e8b56b7f93a2a402dfed59aab80933b77f6c8","unresolved":false,"context_lines":[{"line_number":752,"context_line":""},{"line_number":753,"context_line":"        :param threads_no: Number of host threads per cores which can"},{"line_number":754,"context_line":"                           be used to pin vCPUs according to the"},{"line_number":755,"context_line":"                           policies :param sibling_set: List of"},{"line_number":756,"context_line":"                           available threads per host cores on a"},{"line_number":757,"context_line":"                           specific host NUMA node."},{"line_number":758,"context_line":"        :param instance_cores: Set of vCPUs requested."}],"source_content_type":"text/x-python","patch_set":31,"id":"ffe62b97_7a7a32f1","line":755,"updated":"2017-03-28 16:51:36.000000000","message":"nit: :param sibling_set: should start on a new line.","commit_id":"330042f50fb6ca1b523b4b282a8a5c106919bc82"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"9c1e8b56b7f93a2a402dfed59aab80933b77f6c8","unresolved":false,"context_lines":[{"line_number":805,"context_line":"            # Determines the pCPUs reserved for hypervisor"},{"line_number":806,"context_line":"            #"},{"line_number":807,"context_line":"            # For usable_cores\u003d[(), (4, 5)], num_cpu_reserved\u003d1"},{"line_number":808,"context_line":"            # cpuset_reserved\u003d[4]"},{"line_number":809,"context_line":"            cpuset_reserved \u003d set(list("},{"line_number":810,"context_line":"                itertools.chain(*usable_cores))[:num_cpu_reserved])"},{"line_number":811,"context_line":"            msg \u003d _LI(\"Computed NUMA topology reserved pCPUs: usable pCPUs: \""}],"source_content_type":"text/x-python","patch_set":31,"id":"ffe62b97_bc2e778f","line":808,"updated":"2017-03-28 16:51:36.000000000","message":"technically cpuset_reserved\u003dset([4])","commit_id":"330042f50fb6ca1b523b4b282a8a5c106919bc82"}]}