)]}'
{"nova/virt/hardware.py":[{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3af74a0623b74fa9203a88c023defae65cbf0b0b","unresolved":false,"context_lines":[{"line_number":1109,"context_line":"    # against itself on any NUMA cell"},{"line_number":1110,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1111,"context_line":"        fields.CPUAllocationPolicy.DEDICATED,"},{"line_number":1112,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1113,"context_line":"    ):"},{"line_number":1114,"context_line":"        required_cpus \u003d len(instance_cell.pcpuset) + cpuset_reserved"},{"line_number":1115,"context_line":"        if required_cpus \u003e len(host_cell.pcpuset):"},{"line_number":1116,"context_line":"            LOG.debug(\u0027Not enough host cell CPUs to fit instance cell; \u0027"},{"line_number":1117,"context_line":"                      \u0027required: %(required)d + %(cpuset_reserved)d as \u0027"},{"line_number":1118,"context_line":"                      \u0027overhead, actual: %(actual)d\u0027, {"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_d9ba586f","line":1115,"range":{"start_line":1112,"start_character":0,"end_line":1115,"end_character":50},"updated":"2020-10-06 17:38:35.000000000","message":"so if it mixed required_cpus is fiortst set to \n\n len(instance_cell.pcpuset) + cpuset_reserved and we test it against  len(host_cell.pcpuset)","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3af74a0623b74fa9203a88c023defae65cbf0b0b","unresolved":false,"context_lines":[{"line_number":1124,"context_line":""},{"line_number":1125,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1126,"context_line":"        fields.CPUAllocationPolicy.SHARED,"},{"line_number":1127,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1128,"context_line":"        None,"},{"line_number":1129,"context_line":"    ):"},{"line_number":1130,"context_line":"        required_cpus \u003d len(instance_cell.cpuset)"},{"line_number":1131,"context_line":"        if required_cpus \u003e len(host_cell.cpuset):"},{"line_number":1132,"context_line":"            LOG.debug(\u0027Not enough host cell CPUs to fit instance cell; \u0027"},{"line_number":1133,"context_line":"                      \u0027required: %(required)d, actual: %(actual)d\u0027, {"},{"line_number":1134,"context_line":"                          \u0027required\u0027: len(instance_cell.cpuset),"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_f9b75c84","line":1131,"range":{"start_line":1127,"start_character":6,"end_line":1131,"end_character":49},"updated":"2020-10-06 17:38:35.000000000","message":"then we set it to \nrequired_cpus \u003d len(instance_cell.cpuset)\n\nand test it against  len(host_cell.cpuset):","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ea55a7824db917348efebdcf5cf84f9260156380","unresolved":false,"context_lines":[{"line_number":1124,"context_line":""},{"line_number":1125,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1126,"context_line":"        fields.CPUAllocationPolicy.SHARED,"},{"line_number":1127,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1128,"context_line":"        None,"},{"line_number":1129,"context_line":"    ):"},{"line_number":1130,"context_line":"        required_cpus \u003d len(instance_cell.cpuset)"},{"line_number":1131,"context_line":"        if required_cpus \u003e len(host_cell.cpuset):"},{"line_number":1132,"context_line":"            LOG.debug(\u0027Not enough host cell CPUs to fit instance cell; \u0027"},{"line_number":1133,"context_line":"                      \u0027required: %(required)d, actual: %(actual)d\u0027, {"},{"line_number":1134,"context_line":"                          \u0027required\u0027: len(instance_cell.cpuset),"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_f61627a7","line":1131,"range":{"start_line":1127,"start_character":6,"end_line":1131,"end_character":49},"in_reply_to":"9f560f44_e2c30957","updated":"2020-10-08 15:12:21.000000000","message":"\u003e so host_cell.cpuset is the cpu_share_set not the full set of cores\n \u003e on a cell?\n\nYes.\n\n \u003e the name is not great in that case. this would have been the cores\n \u003e in the vcpu_pin_set for that numa node in the past.\n\nIt\u0027s worse. From Train onwards, if you set \u0027vcpu_pin_set\u0027 then both \u0027cpuset\u0027 and \u0027pcpuset\u0027 will be set to the same value. That was just necessary to avoid crazy object changes. That\u0027ll go away once we drop support for \u0027vcpu_pin_set\u0027","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a86b9805a4227882175836a0a8ec53e569a07431","unresolved":false,"context_lines":[{"line_number":1124,"context_line":""},{"line_number":1125,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1126,"context_line":"        fields.CPUAllocationPolicy.SHARED,"},{"line_number":1127,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1128,"context_line":"        None,"},{"line_number":1129,"context_line":"    ):"},{"line_number":1130,"context_line":"        required_cpus \u003d len(instance_cell.cpuset)"},{"line_number":1131,"context_line":"        if required_cpus \u003e len(host_cell.cpuset):"},{"line_number":1132,"context_line":"            LOG.debug(\u0027Not enough host cell CPUs to fit instance cell; \u0027"},{"line_number":1133,"context_line":"                      \u0027required: %(required)d, actual: %(actual)d\u0027, {"},{"line_number":1134,"context_line":"                          \u0027required\u0027: len(instance_cell.cpuset),"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_e2c30957","line":1131,"range":{"start_line":1127,"start_character":6,"end_line":1131,"end_character":49},"in_reply_to":"9f560f44_f9b75c84","updated":"2020-10-08 12:50:17.000000000","message":"so host_cell.cpuset is the cpu_share_set not the full set of cores on a cell?\n\nthe name is not great in that case. this would have been the cores in the vcpu_pin_set for that numa node in the past.","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"3af74a0623b74fa9203a88c023defae65cbf0b0b","unresolved":false,"context_lines":[{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1140,"context_line":"        fields.CPUAllocationPolicy.DEDICATED,"},{"line_number":1141,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1142,"context_line":"    ):"},{"line_number":1143,"context_line":"        LOG.debug(\u0027Pinning has been requested\u0027)"},{"line_number":1144,"context_line":"        required_cpus \u003d len(instance_cell.pcpuset) + cpuset_reserved"},{"line_number":1145,"context_line":"        if required_cpus \u003e host_cell.avail_pcpus:"},{"line_number":1146,"context_line":"            LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":1147,"context_line":"                      \u0027Oversubscription is not possible with pinned \u0027"},{"line_number":1148,"context_line":"                      \u0027instances. Required: %(required)d (%(vcpus)d + \u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_396ef4fd","line":1145,"range":{"start_line":1141,"start_character":2,"end_line":1145,"end_character":49},"updated":"2020-10-06 17:38:35.000000000","message":"then we set it back to \n\n len(instance_cell.pcpuset) + cpuset_reserved\n\nand check it aginst host_cell.avail_pcpus\n\n\n\nwe dont ever actully check the number of vcpus/shared cpus here\n\nand we check pcpus both agsit the total pcpuset and the aviabel pcpus.\n\ni think it woudl be better to pull out mixed out of these 3 ifss and just have 3 functions one for each of dedicated, mixed and shared/none\n\nwhat do you think?","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"a86b9805a4227882175836a0a8ec53e569a07431","unresolved":false,"context_lines":[{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1140,"context_line":"        fields.CPUAllocationPolicy.DEDICATED,"},{"line_number":1141,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1142,"context_line":"    ):"},{"line_number":1143,"context_line":"        LOG.debug(\u0027Pinning has been requested\u0027)"},{"line_number":1144,"context_line":"        required_cpus \u003d len(instance_cell.pcpuset) + cpuset_reserved"},{"line_number":1145,"context_line":"        if required_cpus \u003e host_cell.avail_pcpus:"},{"line_number":1146,"context_line":"            LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":1147,"context_line":"                      \u0027Oversubscription is not possible with pinned \u0027"},{"line_number":1148,"context_line":"                      \u0027instances. Required: %(required)d (%(vcpus)d + \u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_e24949b8","line":1145,"range":{"start_line":1141,"start_character":2,"end_line":1145,"end_character":49},"in_reply_to":"9f560f44_0eb589ed","updated":"2020-10-08 12:50:17.000000000","message":"actully i was suggesting putting the check into 3 functions\nthen doing\n\ntry:\nif instance_cell.cpu_policy \u003d\u003d fields.CPUAllocationPolicy.DEDICATED:\n    validate_instance_host_oversubscition(host_cell, instance_cell)\n    validate_instance_free_pcpus_(host_cell,instance_cell)\nelif\ninstance_cell.cpu_policy \u003d\u003d fields.CPUAllocationPolicy.MIXED:\n    validate_instance_host_oversubscition(host_cell, instance_cell)\n    validate_instance_free_pcpus(host_cell,instance_cell)\n    validate_instace_shared_cpus(host_cell,instance_cell)\nelse:\n    validate_instace_shared_cpus(host_cell,instance_cell)\nexcept ValueError:\n  return None\n\nso there woudl be some duplication but not alot.\n\n\nregarding checks A and C\n\ncheck A) Is the number of PCPUs in instance cell \u003e total PCPUs on host cell\ncheck C) Is the number of PCPUs in instance cell \u003e free PCPUs on host cell\n\nthe free PCPUS in a cell would also always be less then the total PCPUS so if you check the  free pCPUS in a cell it does that transitivly. so the validate_instance_host_oversubscition(host_cell, instance_cell) or check A is technically duplication. of the later check even if they have differnet debug messages.\n\nso this would jsut be\n\ntry:\nif instance_cell.cpu_policy \u003d\u003d fields.CPUAllocationPolicy.DEDICATED:\n    validate_instance_free_pcpus_(host_cell,instance_cell)\nelif\ninstance_cell.cpu_policy \u003d\u003d fields.CPUAllocationPolicy.MIXED:\n    validate_instance_free_pcpus(host_cell,instance_cell)\n    validate_instace_shared_cpus(host_cell,instance_cell)\nelse:\n    validate_instace_shared_cpus(host_cell,instance_cell)\nexcept ValueError:\n  return None\n\nwhat your checking is not wrong as far as i can see but the if are just a little obscure","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"8f1f301e351982ad9fd1aacf47c853d5fa485669","unresolved":false,"context_lines":[{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1140,"context_line":"        fields.CPUAllocationPolicy.DEDICATED,"},{"line_number":1141,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1142,"context_line":"    ):"},{"line_number":1143,"context_line":"        LOG.debug(\u0027Pinning has been requested\u0027)"},{"line_number":1144,"context_line":"        required_cpus \u003d len(instance_cell.pcpuset) + cpuset_reserved"},{"line_number":1145,"context_line":"        if required_cpus \u003e host_cell.avail_pcpus:"},{"line_number":1146,"context_line":"            LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":1147,"context_line":"                      \u0027Oversubscription is not possible with pinned \u0027"},{"line_number":1148,"context_line":"                      \u0027instances. Required: %(required)d (%(vcpus)d + \u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_0eb589ed","line":1145,"range":{"start_line":1141,"start_character":2,"end_line":1145,"end_character":49},"in_reply_to":"9f560f44_396ef4fd","updated":"2020-10-08 10:40:36.000000000","message":"\u003e then we set it back to\n \u003e \n \u003e len(instance_cell.pcpuset) + cpuset_reserved\n \u003e \n \u003e and check it aginst host_cell.avail_pcpus\n \u003e \n \u003e \n \u003e \n \u003e we dont ever actully check the number of vcpus/shared cpus here\n\nWe do. That\u0027s the check at https://review.opendev.org/#/c/756101/1/nova/virt/hardware.py@1131\n\n \u003e and we check pcpus both agsit the total pcpuset and the aviabel\n \u003e pcpus.\n\nSee below. This is correct. The first check is against total PCPUs because we want to prevent overcommitting against yourself (a host node has 4 cores and an instance requests 6), while the second check is against free PCPUs because we want to prevent overcommitting against other instances (a host node has 4 cores and we\u0027re requesting 4, but 2 are already used by another instance). Those are different errors with different error messages, hence the different checks.\n\n \u003e i think it woudl be better to pull out mixed out of these 3 ifss\n \u003e and just have 3 functions one for each of dedicated, mixed and\n \u003e shared/none\n \u003e \n \u003e what do you think?\n\nI don\u0027t think that\u0027s wise, as it will result in more duplication than we currently have. Just to reiterate, there are a couple of checks we want to do, depending on the policy.\n\nFor \u0027shared\u0027, there is a single check:\n\ncheck B) Is the number of VCPUs in instance cell \u003e total VCPUs on host cell\n\nFor \u0027dedicated\u0027, there are two checks:\n\ncheck A) Is the number of PCPUs in instance cell \u003e total PCPUs on host cell\ncheck C) Is the number of PCPUs in instance cell \u003e free PCPUs on host cell\n\n(the second check is necessary here since VCPUs can overcommit, and we handle that elsewhere, but PCPUs can\u0027t so we must check that here)\n\nFor \u0027mixed\u0027, both VCPU and PCPU are present so it\u0027s naturally a combination of all three checks above:\n\ncheck A) Is the number of PCPUs in instance cell \u003e total PCPUs on host cell\ncheck B) Is the number of VCPUs in instance cell \u003e total VCPUs on host cell (*)\ncheck C) Is the number of PCPUs in instance cell \u003e free PCPUs on host cell\n\n(*) this is what was missing, and what we\u0027re trying to fix here\n\nCurrently, the checks themselves are not duplicated. Rather, we guard entry to the individual checks based on the active policy. You\u0027re suggesting duplicating the individual checks. That\u0027s a lot more duplication and not something I think we should be doing. Maybe try doing it yourself locally to see what I mean?","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ea55a7824db917348efebdcf5cf84f9260156380","unresolved":false,"context_lines":[{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1140,"context_line":"        fields.CPUAllocationPolicy.DEDICATED,"},{"line_number":1141,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1142,"context_line":"    ):"},{"line_number":1143,"context_line":"        LOG.debug(\u0027Pinning has been requested\u0027)"},{"line_number":1144,"context_line":"        required_cpus \u003d len(instance_cell.pcpuset) + cpuset_reserved"},{"line_number":1145,"context_line":"        if required_cpus \u003e host_cell.avail_pcpus:"},{"line_number":1146,"context_line":"            LOG.debug(\u0027Not enough available CPUs to schedule instance. \u0027"},{"line_number":1147,"context_line":"                      \u0027Oversubscription is not possible with pinned \u0027"},{"line_number":1148,"context_line":"                      \u0027instances. Required: %(required)d (%(vcpus)d + \u0027"}],"source_content_type":"text/x-python","patch_set":1,"id":"9f560f44_f6f2a76e","line":1145,"range":{"start_line":1141,"start_character":2,"end_line":1145,"end_character":49},"in_reply_to":"9f560f44_e24949b8","updated":"2020-10-08 15:12:21.000000000","message":"\u003e the free PCPUS in a cell would also always be less then the total\n \u003e PCPUS so if you check the  free pCPUS in a cell it does that\n \u003e transitivly.\n\nYes, you\u0027d lose a little clarity in the error message but that is reasonable, yes. I\u0027m personally okay with this as-is though.\n\n \u003e what your checking is not wrong as far as i can see but the if are\n \u003e just a little obscure\n\nPerhaps, though I\u0027m not a fan of lots of mini functions though as I find it harder to load the context. In any case, that\u0027s a much bigger rework than what is warranted for a backport. I think your concerns (based on misunderstanding the value of \u0027cpuset\u0027) have been addressed?","commit_id":"f0f451ac96fd463a1cd3af124ae8a20b9b6a06cd"},{"author":{"_account_id":11604,"name":"sean mooney","email":"smooney@redhat.com","username":"sean-k-mooney"},"change_message_id":"d6a7895d826b8541729e35aede3f423873dd2810","unresolved":false,"context_lines":[{"line_number":1122,"context_line":"                      })"},{"line_number":1123,"context_line":"            return None"},{"line_number":1124,"context_line":""},{"line_number":1125,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1126,"context_line":"        fields.CPUAllocationPolicy.SHARED,"},{"line_number":1127,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1128,"context_line":"        None,"},{"line_number":1129,"context_line":"    ):"},{"line_number":1130,"context_line":"        required_cpus \u003d len(instance_cell.cpuset)"},{"line_number":1131,"context_line":"        if required_cpus \u003e len(host_cell.cpuset):"},{"line_number":1132,"context_line":"            LOG.debug(\u0027Not enough host cell CPUs to fit instance cell; \u0027"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_b94b2484","line":1129,"range":{"start_line":1125,"start_character":4,"end_line":1129,"end_character":6},"updated":"2020-10-06 17:39:19.000000000","message":"left the comment on ps1 by mistatke but they still apply","commit_id":"b030022dd3403a7375037b5f0dddc7ac861e7f5b"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"88789789dcc60cc8aae255ed482cd049a43d86c5","unresolved":false,"context_lines":[{"line_number":1171,"context_line":"            LOG.debug(\u0027Failed to map instance cell CPUs to host cell CPUs\u0027)"},{"line_number":1172,"context_line":"            return None"},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"    elif limits:"},{"line_number":1175,"context_line":"        LOG.debug(\u0027No pinning requested, considering limitations on usable cpu\u0027"},{"line_number":1176,"context_line":"                  \u0027 and memory\u0027)"},{"line_number":1177,"context_line":"        cpu_usage \u003d host_cell.cpu_usage + len(instance_cell.cpuset)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_cff1a56c","line":1174,"range":{"start_line":1174,"start_character":4,"end_line":1174,"end_character":8},"updated":"2020-10-12 03:12:55.000000000","message":"emm...this is a mistake also? we should check the shared cpu usage here when mixed dedicated and shared cpu in single numa node.","commit_id":"b030022dd3403a7375037b5f0dddc7ac861e7f5b"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"7d4ca13540409a586d5d5ef573113416a8bb36bc","unresolved":false,"context_lines":[{"line_number":1171,"context_line":"            LOG.debug(\u0027Failed to map instance cell CPUs to host cell CPUs\u0027)"},{"line_number":1172,"context_line":"            return None"},{"line_number":1173,"context_line":""},{"line_number":1174,"context_line":"    elif limits:"},{"line_number":1175,"context_line":"        LOG.debug(\u0027No pinning requested, considering limitations on usable cpu\u0027"},{"line_number":1176,"context_line":"                  \u0027 and memory\u0027)"},{"line_number":1177,"context_line":"        cpu_usage \u003d host_cell.cpu_usage + len(instance_cell.cpuset)"}],"source_content_type":"text/x-python","patch_set":4,"id":"9f560f44_6491989a","line":1174,"range":{"start_line":1174,"start_character":4,"end_line":1174,"end_character":8},"in_reply_to":"9f560f44_cff1a56c","updated":"2020-10-12 10:22:59.000000000","message":"Done","commit_id":"b030022dd3403a7375037b5f0dddc7ac861e7f5b"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"73e97ae23f8189f69eca92ceaef99097282d52c3","unresolved":false,"context_lines":[{"line_number":1124,"context_line":""},{"line_number":1125,"context_line":"    if instance_cell.cpu_policy in ("},{"line_number":1126,"context_line":"        fields.CPUAllocationPolicy.SHARED,"},{"line_number":1127,"context_line":"        fields.CPUAllocationPolicy.MIXED,"},{"line_number":1128,"context_line":"        None,"},{"line_number":1129,"context_line":"    ):"},{"line_number":1130,"context_line":"        required_cpus \u003d len(instance_cell.cpuset)"}],"source_content_type":"text/x-python","patch_set":5,"id":"9f560f44_7ba812d8","line":1127,"range":{"start_line":1127,"start_character":8,"end_line":1127,"end_character":41},"updated":"2020-10-12 11:54:36.000000000","message":"So this is the real change. The MIXED needs to be handled in both cases.","commit_id":"82528c83acec494926214e86c7bd56d63c388d81"}]}