)]}'
{"specs/train/approved/pre-filter-disabled-computes.rst":[{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2d3348af996dc5e4ea6824ead693e89dd9baf239","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"As a developer, I want to pre-filter disabled computes in placement which"},{"line_number":36,"context_line":"should be faster (in SQL) than the legacy ``ComputeFilter`` running over the"},{"line_number":37,"context_line":"results in python and eventually allow me to deprecate the ``ComputeFilter``."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"As a user, I want to be able to create and resize servers without hitting"},{"line_number":40,"context_line":"NoValidHost errors because the cloud is performing a rolling upgrade and has"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_b88da532","line":37,"updated":"2019-06-05 11:21:02.000000000","message":"I really didn\u0027t see this in the testing I did before, well it was the writing of debug logs that dominated the processing time. I don\u0027t remember trying with debug logs turned off.\n\nThe issues I hit were more that the random subset of hosts you get includes too many that are ruled out by the ComputeFilter, so you end up picking from what a small set of hosts that are left over, and as such make poor decisions.\n\nThe bigger issue was eventlet meaning you didn\u0027t get to process the results you get back from the DB until you had made DB queries for all the other requests. Reducing the greenthread on the scheduler actually helped with this issue. If it takes long enough, by the time you process the list all the hosts are considered dead because the data you have on when they last did the heartbeat to the scheduler is too old. Maybe that is what large deploys are seeing?\n\n... but frankly the idea is still sound, so I don\u0027t care, just sharing what we found when we dug deeply on this.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4fa2a258acf11b8aff3869261d4a0ecc997e09a6","unresolved":false,"context_lines":[{"line_number":34,"context_line":""},{"line_number":35,"context_line":"As a developer, I want to pre-filter disabled computes in placement which"},{"line_number":36,"context_line":"should be faster (in SQL) than the legacy ``ComputeFilter`` running over the"},{"line_number":37,"context_line":"results in python and eventually allow me to deprecate the ``ComputeFilter``."},{"line_number":38,"context_line":""},{"line_number":39,"context_line":"As a user, I want to be able to create and resize servers without hitting"},{"line_number":40,"context_line":"NoValidHost errors because the cloud is performing a rolling upgrade and has"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_04448cb6","line":37,"in_reply_to":"9fb8cfa7_b88da532","updated":"2019-06-05 14:27:52.000000000","message":"Well the ComputeFilter itself should be fast once it gets the HostState objects loaded up, but I guess the bigger point is we want to reduce the number of HostState objects earlier (ask better questions of placement, make better decisions, etc) so the ComputeFilter doesn\u0027t have to run through as many hosts it\u0027s just going to filter out.\n\nThe issue CERN reported is that because they reduce the limit on the number of allocation candidates they get back from placement, they are sometimes getting back mostly / only disabled computes so they are all getting filtered out and get a NoValidHost error as a result. With this we\u0027d pre-filter those disabled computes so the small limited set they get back would pass through the ComputeFilter (assuming the service is reported as being up still).","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":82,"context_line":"related compute node resource providers in placement appropriately. For"},{"line_number":83,"context_line":"example, if compute service A is disabled or forced-down, the trait will be"},{"line_number":84,"context_line":"added. When compute service A is enabled or no longer forced-down, the trait"},{"line_number":85,"context_line":"will be removed."},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"Scheduler changes"},{"line_number":88,"context_line":"-----------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_a9aecbe4","line":85,"updated":"2019-06-04 17:18:54.000000000","message":"Down and Disabled are different. AFAIK, force-down is purely about the down-ness and not the disabled-ness of the service record, right? We should never try to make a call (or cast) to a compute that is down or is being forced down, because that service is already down or fenced. Are you confusing the overlap here or are you proposing something else I\u0027m missing?","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f76ae773cd7f5052c633cd1dc89c0a3822667e09","unresolved":false,"context_lines":[{"line_number":82,"context_line":"related compute node resource providers in placement appropriately. For"},{"line_number":83,"context_line":"example, if compute service A is disabled or forced-down, the trait will be"},{"line_number":84,"context_line":"added. When compute service A is enabled or no longer forced-down, the trait"},{"line_number":85,"context_line":"will be removed."},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"Scheduler changes"},{"line_number":88,"context_line":"-----------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_2f765ba6","line":85,"in_reply_to":"9fb8cfa7_64e6ba47","updated":"2019-06-04 19:05:32.000000000","message":"I think that leaving computefilter\u0027s behavior for train (potentially always on for train, or off if we add a config toggle) makes sense. After train, we should be able to remove the disabledness check from it directly.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":82,"context_line":"related compute node resource providers in placement appropriately. For"},{"line_number":83,"context_line":"example, if compute service A is disabled or forced-down, the trait will be"},{"line_number":84,"context_line":"added. When compute service A is enabled or no longer forced-down, the trait"},{"line_number":85,"context_line":"will be removed."},{"line_number":86,"context_line":""},{"line_number":87,"context_line":"Scheduler changes"},{"line_number":88,"context_line":"-----------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_64e6ba47","line":85,"in_reply_to":"9fb8cfa7_a9aecbe4","updated":"2019-06-04 18:50:41.000000000","message":"I was approaching this from the standpoint of how the ComputeFilter works today, which checks for both disabled [1] and down [2].\n\nYou raise a good point about forced-down and not calling the compute in that case, which would be wrong in my PoC here [3].\n\nIf we come out of this spec saying the pre-filter is only for disabled computes and not down computes, then I don\u0027t have to worry about the service group API or forced-down stuff at all but it also means the ComputeFilter must remain for post-placement filtering of down computes - and the disabled check in that filter could be redundant unless we use the new conf option for the pre-filter (added in this blueprint) to modify the ComputeFilter to *not* check disabled status but only \"is up\" status. If we don\u0027t add a sync CLI though for upgrades or something we\u0027d not want to change the ComputeFilter though since it\u0027d be the only thing that could filter out older disabled computes that don\u0027t have the trait yet.\n\n[1] https://github.com/openstack/nova/blob/a4792bba405fd1024ee2ebd6640fb601bd456e9b/nova/scheduler/filters/compute_filter.py#L38\n[2] https://github.com/openstack/nova/blob/a4792bba405fd1024ee2ebd6640fb601bd456e9b/nova/scheduler/filters/compute_filter.py#L44\n[3] https://review.opendev.org/#/c/654596/2/nova/compute/api.py","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":91,"context_line":"``query_placement_for_enabled_computes`` which by default will disable the"},{"line_number":92,"context_line":"filter. When enabled, the filter will modify the RequestSpec to forbid"},{"line_number":93,"context_line":"providers with the ``COMPUTE_STATUS_DISABLED`` trait. The changes to the"},{"line_number":94,"context_line":"RequestSpec will not be persisted."},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"ServiceGroup API changes"},{"line_number":97,"context_line":"------------------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_691c536a","line":94,"updated":"2019-06-04 17:18:54.000000000","message":"Is it necessary to allow this to be disabled? On the other filters, we\u0027re querying for a thing that might not exist yet if the computes haven\u0027t been upgraded to expose it. For this, the worst case is that it reduces to what we have today. I think maybe just making this be always-on is fine right?","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f76ae773cd7f5052c633cd1dc89c0a3822667e09","unresolved":false,"context_lines":[{"line_number":91,"context_line":"``query_placement_for_enabled_computes`` which by default will disable the"},{"line_number":92,"context_line":"filter. When enabled, the filter will modify the RequestSpec to forbid"},{"line_number":93,"context_line":"providers with the ``COMPUTE_STATUS_DISABLED`` trait. The changes to the"},{"line_number":94,"context_line":"RequestSpec will not be persisted."},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"ServiceGroup API changes"},{"line_number":97,"context_line":"------------------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_2f841b5e","line":94,"in_reply_to":"9fb8cfa7_240522c3","updated":"2019-06-04 19:05:32.000000000","message":"\u003e I thought you\u0027ve said before that you think all of these\n \u003e pre-filters should be configurable so deployers can opt into\n \u003e enabling them (like the traditional scheduler filters).\n\nI did, but all the filters that existed at that time were positive not negative. This one doesn\u0027t break or filter out anything we might want to keep based on complex logic, external setup, or upgraded-ness.\n\n \u003e In thinking through the upgrade here, if we have an old compute\n \u003e that is disabled but doesn\u0027t have the trait, then yeah we have the\n \u003e same issue as today: placement could return that node as a\n \u003e candidate but it\u0027d just get filtered out by the ComputeFilter, so\n \u003e not a big deal.\n\nRight, which makes this quite different from the other prefilters we\u0027ve had before.\n\n \u003e One worry is if nova and placement get out of sync somehow and the\n \u003e pre-filter starts excluding candidates that aren\u0027t really disabled,\n \u003e e.g. let\u0027s say you disable a node, we add the disabled trait, then\n \u003e you enable the node, but when we go to remove the trait the rpc\n \u003e call is dropped or something and then the trait is left on the\n \u003e provider in placement and we\u0027ll continue to exclude that provider\n \u003e during scheduling. But if we add a \"nova-manage placement\n \u003e sync_compute_status\" CLI then that becomes less of a worry.\n\nIsn\u0027t the compute service going to assert the state of its disabled trait on every u-p-t interval? IMHO, that eliminates the need for an external sync command, and largely removes the concern over them being out of sync (for more than a few minutes). A compute service restart will trigger that immediately for anything that gets out of sync due to crazy happenings.\n\n \u003e On the whole it doesn\u0027t matter to me too much either way if the\n \u003e pre-filter is configurable or not. The ComputeFilter can be\n \u003e excluded but I don\u0027t know who would do that. Also, as noted above,\n \u003e if we have a config option for the pre-filter we could remove the\n \u003e redundancy on the disabled check in the ComputeFilter (although\n \u003e that redundancy is likely nearly 0 time to perform since it\u0027s just\n \u003e a simple conditional check on a resource [HostState] we already\n \u003e have in scope).\n\nYep. I\u0027d argue for always-on unless someone comes up with a good reason not to, but I also won\u0027t freak out if we feel the need to have a do_the_obviously_better_thing\u003dTrue config option :)","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ee7cbc587b71db6473b946362ebb02a9c89f6273","unresolved":false,"context_lines":[{"line_number":91,"context_line":"``query_placement_for_enabled_computes`` which by default will disable the"},{"line_number":92,"context_line":"filter. When enabled, the filter will modify the RequestSpec to forbid"},{"line_number":93,"context_line":"providers with the ``COMPUTE_STATUS_DISABLED`` trait. The changes to the"},{"line_number":94,"context_line":"RequestSpec will not be persisted."},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"ServiceGroup API changes"},{"line_number":97,"context_line":"------------------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_be66faec","line":94,"in_reply_to":"9fb8cfa7_2f841b5e","updated":"2019-06-04 21:15:10.000000000","message":"\u003e Isn\u0027t the compute service going to assert the state of its disabled trait on every u-p-t interval? IMHO, that eliminates the need for an external sync command, and largely removes the concern over them being out of sync (for more than a few minutes). A compute service restart will trigger that immediately for anything that gets out of sync due to crazy happenings.\n\nAs discussed in IRC:\n\nhttp://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-06-04.log.html#t2019-06-04T19:10:58\n\nI hadn\u0027t initially planned on adding anything for the upt stuff but had noted it as an alternative (or something we could build in). But yeah I can make that a main change in the spec to deal with:\n\n1. upgrades\n2. sync issues\n3. dropped calls\n\n\u003e Yep. I\u0027d argue for always-on unless someone comes up with a good reason not to, but I also won\u0027t freak out if we feel the need to have a do_the_obviously_better_thing\u003dTrue config option :)\n\nOK I\u0027ll remove the config option from the spec as part of the proposed change but mention it in the Alternatives section in case someone wants to debate it.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":91,"context_line":"``query_placement_for_enabled_computes`` which by default will disable the"},{"line_number":92,"context_line":"filter. When enabled, the filter will modify the RequestSpec to forbid"},{"line_number":93,"context_line":"providers with the ``COMPUTE_STATUS_DISABLED`` trait. The changes to the"},{"line_number":94,"context_line":"RequestSpec will not be persisted."},{"line_number":95,"context_line":""},{"line_number":96,"context_line":"ServiceGroup API changes"},{"line_number":97,"context_line":"------------------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_240522c3","line":94,"in_reply_to":"9fb8cfa7_691c536a","updated":"2019-06-04 18:50:41.000000000","message":"I thought you\u0027ve said before that you think all of these pre-filters should be configurable so deployers can opt into enabling them (like the traditional scheduler filters).\n\nIn thinking through the upgrade here, if we have an old compute that is disabled but doesn\u0027t have the trait, then yeah we have the same issue as today: placement could return that node as a candidate but it\u0027d just get filtered out by the ComputeFilter, so not a big deal.\n\nOne worry is if nova and placement get out of sync somehow and the pre-filter starts excluding candidates that aren\u0027t really disabled, e.g. let\u0027s say you disable a node, we add the disabled trait, then you enable the node, but when we go to remove the trait the rpc call is dropped or something and then the trait is left on the provider in placement and we\u0027ll continue to exclude that provider during scheduling. But if we add a \"nova-manage placement sync_compute_status\" CLI then that becomes less of a worry.\n\nOn the whole it doesn\u0027t matter to me too much either way if the pre-filter is configurable or not. The ComputeFilter can be excluded but I don\u0027t know who would do that. Also, as noted above, if we have a config option for the pre-filter we could remove the redundancy on the disabled check in the ComputeFilter (although that redundancy is likely nearly 0 time to perform since it\u0027s just a simple conditional check on a resource [HostState] we already have in scope).","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"6eab4756af9291736b3957e08ad3a61dbb7683df","unresolved":false,"context_lines":[{"line_number":99,"context_line":"The ``ComputeFilter`` filters out hosts that are disabled (either manually or"},{"line_number":100,"context_line":"automatically in the case of the libvirt driver), are manually forced-down or"},{"line_number":101,"context_line":"are automatically reported as \"down\". The `is_up check`_ is based on a call to"},{"line_number":102,"context_line":"the service group API. This is used to automatically report that a service is"},{"line_number":103,"context_line":"down if it has not reported into the service group API in some configurable"},{"line_number":104,"context_line":"time interval."},{"line_number":105,"context_line":""},{"line_number":106,"context_line":".. todo:: Need to think about what to do about the service group API. Do we"}],"source_content_type":"text/x-rst","patch_set":2,"id":"bfb3d3c7_5bd44020","line":103,"range":{"start_line":102,"start_character":53,"end_line":103,"end_character":4},"updated":"2019-05-28 08:09:18.000000000","message":"you mean to report the trait at here? I didn\u0027t get what ServiceGroup API changes you proposed.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4185e5cdc7f3f658ca4fb0bd0fcb11d6c68729e6","unresolved":false,"context_lines":[{"line_number":99,"context_line":"The ``ComputeFilter`` filters out hosts that are disabled (either manually or"},{"line_number":100,"context_line":"automatically in the case of the libvirt driver), are manually forced-down or"},{"line_number":101,"context_line":"are automatically reported as \"down\". The `is_up check`_ is based on a call to"},{"line_number":102,"context_line":"the service group API. This is used to automatically report that a service is"},{"line_number":103,"context_line":"down if it has not reported into the service group API in some configurable"},{"line_number":104,"context_line":"time interval."},{"line_number":105,"context_line":""},{"line_number":106,"context_line":".. todo:: Need to think about what to do about the service group API. Do we"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_7e2167fb","line":103,"range":{"start_line":102,"start_character":53,"end_line":103,"end_character":4},"in_reply_to":"bfb3d3c7_5bd44020","updated":"2019-06-04 16:48:31.000000000","message":"That\u0027s what the following todo is for, I\u0027m not sure yet either. The way I wrote the PoC (based on some discussion in IRC when this problem has come up before) was to centralize the trait management in the compute service so both the API and the compute (in the case the hypervisor goes down or comes up - for the libvirt driver) would use the same code. But there are issues with trying to use the centralized code in the compute from the service group API, but if we want to replace the ComputeFilter with this new trait, then the service group API has to be involved as well I think because as noted here the ComputeFilter checks both the enabled/disabled status and the \"is up\" value which is based on (1) the user forcing the service down or (2) the compute not reporting in after some configurable time interval.\n\nAlternative 3 also gets into these same weeds and basically asks the question, should all three areas (REST API, compute, service group API) manage the trait directly rather than try and centralize the trait management in one place (the compute)? Because if the service group API has to do that anyway, and the API *might* have to do it (at least temporarily) during a rolling upgrade with an older compute, then we might as well just have each component manage the trait directly from the start rather than try to centralize on the compute.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":115,"context_line":"          pass of this and punt to saying we just rely on external-to-nova"},{"line_number":116,"context_line":"          tooling enabling/disabling the compute service in the API when it"},{"line_number":117,"context_line":"          detects that a host is down (essentially duplicating the work of the"},{"line_number":118,"context_line":"          service group API)?"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"Known issues"},{"line_number":121,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_e9560307","line":118,"updated":"2019-06-04 17:18:54.000000000","message":"I\u0027m confused. Are you talking about the case where the compute is up and running but unable to update its service record (because of a MQ outage or something)? If so, that seems like a really small edge case not worthy of too much concern. The more interesting case, IMHO, is whether or not we try to reflect automatic down-ness (i.e. service check timed out) in placement or not. It\u0027s hard to know who should do that, since the compute service is not running. I\u0027d punt that until later though and leave this spec about disabled-ness.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f76ae773cd7f5052c633cd1dc89c0a3822667e09","unresolved":false,"context_lines":[{"line_number":115,"context_line":"          pass of this and punt to saying we just rely on external-to-nova"},{"line_number":116,"context_line":"          tooling enabling/disabling the compute service in the API when it"},{"line_number":117,"context_line":"          detects that a host is down (essentially duplicating the work of the"},{"line_number":118,"context_line":"          service group API)?"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"Known issues"},{"line_number":121,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_af97ab90","line":118,"in_reply_to":"9fb8cfa7_04e25e04","updated":"2019-06-04 19:05:32.000000000","message":"Yep, I think making this spec limited to disabled makes sense. Tackle the down case in another spec, which I\u0027m happy to also help think on.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":115,"context_line":"          pass of this and punt to saying we just rely on external-to-nova"},{"line_number":116,"context_line":"          tooling enabling/disabling the compute service in the API when it"},{"line_number":117,"context_line":"          detects that a host is down (essentially duplicating the work of the"},{"line_number":118,"context_line":"          service group API)?"},{"line_number":119,"context_line":""},{"line_number":120,"context_line":"Known issues"},{"line_number":121,"context_line":"------------"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_04e25e04","line":118,"in_reply_to":"9fb8cfa7_e9560307","updated":"2019-06-04 18:50:41.000000000","message":"\u003e I\u0027m confused. Are you talking about the case where the compute is\n \u003e up and running but unable to update its service record (because of\n \u003e a MQ outage or something)? If so, that seems like a really small\n \u003e edge case not worthy of too much concern.\n\nAnd that\u0027s the model_disconnected\u003dTrue case right?\n\n \u003e The more interesting case, IMHO, is whether or not we try to reflect automatic down-ness\n \u003e (i.e. service check timed out) in placement or not. It\u0027s hard to\n \u003e know who should do that, since the compute service is not running.\n \u003e I\u0027d punt that until later though and leave this spec about\n \u003e disabled-ness.\n\nI was trying to handle both, but agree it\u0027s messy, which is why this spec kind of went off into the weeds (the quandary I mentioned in IRC), and I didn\u0027t even think about the service group API stuff until I started writing the spec - the PoC change doesn\u0027t deal with that at all. If we want to punt on that it certainly makes this easier for me, but as noted before we\u0027d still need to have the ComputeFilter, and maybe that\u0027s fine as long as we\u0027re pre-filtering enough of the disabled computes to get over the hump of the current problem with getting back a smaller limited set of candidates that are all disabled (CERN\u0027s issue).","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5cfc0bd5f23cb902723e5927e35e7b13d269a8bb","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"* We could provide a ``nova-manage placement sync_compute_status`` CLI which"},{"line_number":132,"context_line":"  operators could run if they think the compute status and provider trait are"},{"line_number":133,"context_line":"  not aligned."},{"line_number":134,"context_line":""},{"line_number":135,"context_line":".. _set_host_enabled: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/compute/rpcapi.py#L891"},{"line_number":136,"context_line":".. _xenapi driver: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/virt/xenapi/host.py#L121"}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_9c175ea4","line":133,"updated":"2019-05-08 22:07:05.000000000","message":"If we\u0027re willing to have that, maybe take it further (probably not now, but perhaps later):\n\n    nova-manage placement sync_compute [-v] (--status|--inventory|--allocaitons|--all)","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"f76ae773cd7f5052c633cd1dc89c0a3822667e09","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"* We could provide a ``nova-manage placement sync_compute_status`` CLI which"},{"line_number":132,"context_line":"  operators could run if they think the compute status and provider trait are"},{"line_number":133,"context_line":"  not aligned."},{"line_number":134,"context_line":""},{"line_number":135,"context_line":".. _set_host_enabled: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/compute/rpcapi.py#L891"},{"line_number":136,"context_line":".. _xenapi driver: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/virt/xenapi/host.py#L121"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_6f91b399","line":133,"in_reply_to":"9fb8cfa7_0489fe21","updated":"2019-06-04 19:05:32.000000000","message":"The compute manager is going to do said healing every u-p-t interval right?","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"ee7cbc587b71db6473b946362ebb02a9c89f6273","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"* We could provide a ``nova-manage placement sync_compute_status`` CLI which"},{"line_number":132,"context_line":"  operators could run if they think the compute status and provider trait are"},{"line_number":133,"context_line":"  not aligned."},{"line_number":134,"context_line":""},{"line_number":135,"context_line":".. _set_host_enabled: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/compute/rpcapi.py#L891"},{"line_number":136,"context_line":".. _xenapi driver: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/virt/xenapi/host.py#L121"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_beab5ad4","line":133,"in_reply_to":"9fb8cfa7_6f91b399","updated":"2019-06-04 21:15:10.000000000","message":"\u003e The compute manager is going to do said healing every u-p-t\n \u003e interval right?\n\nPer above I\u0027ll update the spec to do that yes.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"* We could provide a ``nova-manage placement sync_compute_status`` CLI which"},{"line_number":132,"context_line":"  operators could run if they think the compute status and provider trait are"},{"line_number":133,"context_line":"  not aligned."},{"line_number":134,"context_line":""},{"line_number":135,"context_line":".. _set_host_enabled: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/compute/rpcapi.py#L891"},{"line_number":136,"context_line":".. _xenapi driver: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/virt/xenapi/host.py#L121"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_0489fe21","line":133,"in_reply_to":"9fb8cfa7_a9dc8b74","updated":"2019-06-04 18:50:41.000000000","message":"There is the potential issue I mentioned above about nova and placement getting out of sync wrt the trait (if the RPC call is dropped), which is likely more rare, but there is also the case of upgrades and older computes which are disabled not having that trait - wouldn\u0027t we want to \"heal\" those? Otherwise they\u0027d be disabled but not have the trait so only the ComputeFilter would catch them as today - maybe an OK trade-off.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"* We could provide a ``nova-manage placement sync_compute_status`` CLI which"},{"line_number":132,"context_line":"  operators could run if they think the compute status and provider trait are"},{"line_number":133,"context_line":"  not aligned."},{"line_number":134,"context_line":""},{"line_number":135,"context_line":".. _set_host_enabled: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/compute/rpcapi.py#L891"},{"line_number":136,"context_line":".. _xenapi driver: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/virt/xenapi/host.py#L121"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_a9dc8b74","line":133,"in_reply_to":"dfbec78f_4067a726","updated":"2019-06-04 17:18:54.000000000","message":"IMHO, and taking into account my clarification of down vs disabled above, I think this isn\u0027t an issue. The API should always talk to the compute to communicate disabled-ness, let the compute report to placement that it\u0027s disabled. No need to direct report that trait from the API during manual disable, separate from automatic disable.\n\nDown-ness would always need to be from the API, at least for the up-\u003edown transition. But like I say, that\u0027s a little ickier, so let\u0027s not mingle that into this spec about disabled-ness.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"cfffdd10746a08daccee67d9de76a460031b287a","unresolved":false,"context_lines":[{"line_number":130,"context_line":""},{"line_number":131,"context_line":"* We could provide a ``nova-manage placement sync_compute_status`` CLI which"},{"line_number":132,"context_line":"  operators could run if they think the compute status and provider trait are"},{"line_number":133,"context_line":"  not aligned."},{"line_number":134,"context_line":""},{"line_number":135,"context_line":".. _set_host_enabled: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/compute/rpcapi.py#L891"},{"line_number":136,"context_line":".. _xenapi driver: https://opendev.org/openstack/nova/src/tag/19.0.0/nova/virt/xenapi/host.py#L121"}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_4067a726","line":133,"in_reply_to":"dfbec78f_9c175ea4","updated":"2019-05-09 19:21:38.000000000","message":"Yeah we already have heal_allocations and sync_aggregates, this is another type of command like that. I could see unifying them at some point, but on the other hand the option matrix on a unified CLI could get crazy.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":144,"context_line":""},{"line_number":145,"context_line":"#. Rather than using a forbidden trait, we could hard-code a resource provider"},{"line_number":146,"context_line":"   aggregate UUID in nova and add/remove compute node resource providers"},{"line_number":147,"context_line":"   to/from that aggregate in placement as the service is disabled/enabled."},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"   * Pros: Aggregates may be more natural since they are a grouping of"},{"line_number":150,"context_line":"     providers."}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_69e69347","line":147,"updated":"2019-06-04 17:18:54.000000000","message":"-2","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":144,"context_line":""},{"line_number":145,"context_line":"#. Rather than using a forbidden trait, we could hard-code a resource provider"},{"line_number":146,"context_line":"   aggregate UUID in nova and add/remove compute node resource providers"},{"line_number":147,"context_line":"   to/from that aggregate in placement as the service is disabled/enabled."},{"line_number":148,"context_line":""},{"line_number":149,"context_line":"   * Pros: Aggregates may be more natural since they are a grouping of"},{"line_number":150,"context_line":"     providers."}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_a4cfd27d","line":147,"in_reply_to":"9fb8cfa7_69e69347","updated":"2019-06-04 18:50:41.000000000","message":"But not -5? :P","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":163,"context_line":"   resource class on that provider, like what the ironic driver does when a"},{"line_number":164,"context_line":"   node is undergoing maintenance and should be taken out of scheduling"},{"line_number":165,"context_line":"   consideration. [2]_"},{"line_number":166,"context_line":""},{"line_number":167,"context_line":"   * Pros: No new traits, can just follow the ironic driver pattern."},{"line_number":168,"context_line":""},{"line_number":169,"context_line":"   * Cons: Ironic node resource providers are expected to have a single"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_89e3c733","line":166,"updated":"2019-06-04 17:18:54.000000000","message":"-1.9","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5cfc0bd5f23cb902723e5927e35e7b13d269a8bb","unresolved":false,"context_lines":[{"line_number":172,"context_line":"     they are reporting at least three resource classes (VCPU, MEMORY_MB and"},{"line_number":173,"context_line":"     DISK_GB) so it would be more complicated to set reserved \u003d total on all"},{"line_number":174,"context_line":"     of those classes. Furthermore, changing the inventory is not configurable"},{"line_number":175,"context_line":"     like a request filter is."},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"   Long-term, we could consider changing the ironic driver node maintenance"},{"line_number":178,"context_line":"   code to just set/unset the ``COMPUTE_STATUS_DISABLED`` trait."}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_1c236e07","line":175,"updated":"2019-05-08 22:07:05.000000000","message":"yeah","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5cfc0bd5f23cb902723e5927e35e7b13d269a8bb","unresolved":false,"context_lines":[{"line_number":175,"context_line":"     like a request filter is."},{"line_number":176,"context_line":""},{"line_number":177,"context_line":"   Long-term, we could consider changing the ironic driver node maintenance"},{"line_number":178,"context_line":"   code to just set/unset the ``COMPUTE_STATUS_DISABLED`` trait."},{"line_number":179,"context_line":""},{"line_number":180,"context_line":"#. Rather than the ``os-services`` API synchronously calling the"},{"line_number":181,"context_line":"   ``set_host_enabled`` method on the compute service, the API could just"}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_fc277a11","line":178,"updated":"2019-05-08 22:07:05.000000000","message":"yeah","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5cfc0bd5f23cb902723e5927e35e7b13d269a8bb","unresolved":false,"context_lines":[{"line_number":179,"context_line":""},{"line_number":180,"context_line":"#. Rather than the ``os-services`` API synchronously calling the"},{"line_number":181,"context_line":"   ``set_host_enabled`` method on the compute service, the API could just"},{"line_number":182,"context_line":"   toggle the trait on the affected providers directly."},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":185,"context_line":"     the enabled or forced-down status of the service. This could also"}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_1c684e1f","line":182,"updated":"2019-05-08 22:07:05.000000000","message":"Since the dependency on placement is no longer optional, this makes pretty good sense to me, but I would think that: less RPC is a positive.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":179,"context_line":""},{"line_number":180,"context_line":"#. Rather than the ``os-services`` API synchronously calling the"},{"line_number":181,"context_line":"   ``set_host_enabled`` method on the compute service, the API could just"},{"line_number":182,"context_line":"   toggle the trait on the affected providers directly."},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":185,"context_line":"     the enabled or forced-down status of the service. This could also"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_695453cb","line":182,"in_reply_to":"bfb3d3c7_fbe4140f","updated":"2019-06-04 17:18:54.000000000","message":"I don\u0027t like this because I think it confuses the who-owns-this notion, and introduces a race. That race should be resolved via generations, but requires the compute to notice that conflict, refresh its own disabled-ness notion, and restart.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":5754,"name":"Alex Xu","email":"hejie.xu@intel.com","username":"xuhj"},"change_message_id":"6eab4756af9291736b3957e08ad3a61dbb7683df","unresolved":false,"context_lines":[{"line_number":179,"context_line":""},{"line_number":180,"context_line":"#. Rather than the ``os-services`` API synchronously calling the"},{"line_number":181,"context_line":"   ``set_host_enabled`` method on the compute service, the API could just"},{"line_number":182,"context_line":"   toggle the trait on the affected providers directly."},{"line_number":183,"context_line":""},{"line_number":184,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":185,"context_line":"     the enabled or forced-down status of the service. This could also"}],"source_content_type":"text/x-rst","patch_set":2,"id":"bfb3d3c7_fbe4140f","line":182,"in_reply_to":"dfbec78f_1c684e1f","updated":"2019-05-28 08:09:18.000000000","message":"+1","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5cfc0bd5f23cb902723e5927e35e7b13d269a8bb","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":185,"context_line":"     the enabled or forced-down status of the service. This could also"},{"line_number":186,"context_line":"     side-step a potential race between trying to disable or force-down"},{"line_number":187,"context_line":"     the compute service when a hardware alert has kicked off automation"},{"line_number":188,"context_line":"     because the host might be about to fail, i.e. the RPC call from the"},{"line_number":189,"context_line":"     API to the compute service might fail if the compute is already dead."}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_9c651e48","line":186,"range":{"start_line":186,"start_character":5,"end_line":186,"end_character":31},"updated":"2019-05-08 22:07:05.000000000","message":"this is an excellent point\n\nIf the compute is already dead for whatever reason, its useful to be able to declare it disabled without needing its attention","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":8864,"name":"Artom Lifshitz","email":"notartom@gmail.com","username":"artom"},"change_message_id":"0bd87461d8c4dfdee9bc7c05735f69ebf98af67d","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":185,"context_line":"     the enabled or forced-down status of the service. This could also"},{"line_number":186,"context_line":"     side-step a potential race between trying to disable or force-down"},{"line_number":187,"context_line":"     the compute service when a hardware alert has kicked off automation"},{"line_number":188,"context_line":"     because the host might be about to fail, i.e. the RPC call from the"},{"line_number":189,"context_line":"     API to the compute service might fail if the compute is already dead."}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_d7d5b1e6","line":186,"in_reply_to":"dfbec78f_9c651e48","updated":"2019-05-08 23:26:59.000000000","message":"Yeah, presumably we\u0027d reuse this spec when a compute host is detected as down, and in that case we obviously can\u0027t call to the downed compute to set its own status in placement, so I\u0027d favor a centralized approach.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":183,"context_line":""},{"line_number":184,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":185,"context_line":"     the enabled or forced-down status of the service. This could also"},{"line_number":186,"context_line":"     side-step a potential race between trying to disable or force-down"},{"line_number":187,"context_line":"     the compute service when a hardware alert has kicked off automation"},{"line_number":188,"context_line":"     because the host might be about to fail, i.e. the RPC call from the"},{"line_number":189,"context_line":"     API to the compute service might fail if the compute is already dead."}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_c4cd265e","line":186,"in_reply_to":"dfbec78f_d7d5b1e6","updated":"2019-06-04 18:50:41.000000000","message":"If we punt on the down-ness issue in this spec, as it sounds like Dan wants to do (and would make my life much easier) then this is less of a concern because then I think disabled status is more about planned maintenance rather than emergency (forced-down / evacuate type scenario).","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":186,"context_line":"     side-step a potential race between trying to disable or force-down"},{"line_number":187,"context_line":"     the compute service when a hardware alert has kicked off automation"},{"line_number":188,"context_line":"     because the host might be about to fail, i.e. the RPC call from the"},{"line_number":189,"context_line":"     API to the compute service might fail if the compute is already dead."},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"   * Cons: Potential duplication of the code that manages the trait which could"},{"line_number":192,"context_line":"     violate the principle of single responsibility. Note that we might"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_690213b7","line":189,"updated":"2019-06-04 17:18:54.000000000","message":"You can make it a cast if you want to avoid the synchronous nature, and still have compute update placement. However, I think a blocking call is probably what people want here. They want to disable a compute and know that it has happened, kinda like server pause (which is synchronous). I think a sync call that returns success or failure is a lot more friendly to the admin than an \"okay sure, but poll to make sure it happens, and oh, if it never happens, I can\u0027t tell you I tried and failed. kthx.\"","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":186,"context_line":"     side-step a potential race between trying to disable or force-down"},{"line_number":187,"context_line":"     the compute service when a hardware alert has kicked off automation"},{"line_number":188,"context_line":"     because the host might be about to fail, i.e. the RPC call from the"},{"line_number":189,"context_line":"     API to the compute service might fail if the compute is already dead."},{"line_number":190,"context_line":""},{"line_number":191,"context_line":"   * Cons: Potential duplication of the code that manages the trait which could"},{"line_number":192,"context_line":"     violate the principle of single responsibility. Note that we might"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_64da3a18","line":189,"in_reply_to":"9fb8cfa7_690213b7","updated":"2019-06-04 18:50:41.000000000","message":"Yeah agree with that UX on blocking to make sure the thing happened.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"bec99b4bb7734f56730d059bb41eac2b8a37656a","unresolved":false,"context_lines":[{"line_number":205,"context_line":"     does not cover the case that the service itself is down (which would be"},{"line_number":206,"context_line":"     detected by the service group API). The ``update_provider_tree``"},{"line_number":207,"context_line":"     interface may need to change or the driver would have to get the"},{"line_number":208,"context_line":"     ``ComputeNode`` record to determine the disabled and force_down status."},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"#. Do nothing and instead focus efforts on optimizing the performance of the"},{"line_number":211,"context_line":"   nova scheduler which is likely the root cause that large deployments need"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_a9e6cb15","line":208,"updated":"2019-06-04 17:18:54.000000000","message":"This is not so bad because it\u0027s just a time lag between the disable event and the perf increase you get from not including it in the set. For non trivial deployment sizes, this would probably not be noticeable or detectable. Meaning, I doubt an operator is doing the following:\n\n 1. Boot an instance, check the full set of result hosts\n 2. Disable one compute\n 3. Boot another instance, diff the full set from step 1 (all within a minute or so)","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"5236a451c508ee96cea0bf37a1d1338c1bc12d7f","unresolved":false,"context_lines":[{"line_number":205,"context_line":"     does not cover the case that the service itself is down (which would be"},{"line_number":206,"context_line":"     detected by the service group API). The ``update_provider_tree``"},{"line_number":207,"context_line":"     interface may need to change or the driver would have to get the"},{"line_number":208,"context_line":"     ``ComputeNode`` record to determine the disabled and force_down status."},{"line_number":209,"context_line":""},{"line_number":210,"context_line":"#. Do nothing and instead focus efforts on optimizing the performance of the"},{"line_number":211,"context_line":"   nova scheduler which is likely the root cause that large deployments need"}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_c48526af","line":208,"in_reply_to":"9fb8cfa7_a9e6cb15","updated":"2019-06-04 18:50:41.000000000","message":"Sure, this is also something we could build in later if we find that we have sync issues between the API\u003c\u003ecompute\u003c\u003eplacement.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5cfc0bd5f23cb902723e5927e35e7b13d269a8bb","unresolved":false,"context_lines":[{"line_number":213,"context_line":"   optimizing the scheduler (which is something we should do anyway), part of"},{"line_number":214,"context_line":"   making scheduling faster in nova is dependent on nova asking placement"},{"line_number":215,"context_line":"   more informed questions and placement providing a smaller set of allocation"},{"line_number":216,"context_line":"   candidates, i.e. filter in SQL (placement) rather than in python (nova)."},{"line_number":217,"context_line":""},{"line_number":218,"context_line":".. _update_provider_tree: https://docs.openstack.org/nova/latest/reference/update-provider-tree.html"},{"line_number":219,"context_line":""}],"source_content_type":"text/x-rst","patch_set":2,"id":"dfbec78f_1cd60ec5","line":216,"updated":"2019-05-08 22:07:05.000000000","message":"both. would be good to see some profiling data from the nova-scheduler side","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"cef140f49097f085184952baf63b7c7f1a17c8d3","unresolved":false,"context_lines":[{"line_number":213,"context_line":"   optimizing the scheduler (which is something we should do anyway), part of"},{"line_number":214,"context_line":"   making scheduling faster in nova is dependent on nova asking placement"},{"line_number":215,"context_line":"   more informed questions and placement providing a smaller set of allocation"},{"line_number":216,"context_line":"   candidates, i.e. filter in SQL (placement) rather than in python (nova)."},{"line_number":217,"context_line":""},{"line_number":218,"context_line":".. _update_provider_tree: https://docs.openstack.org/nova/latest/reference/update-provider-tree.html"},{"line_number":219,"context_line":""}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_2047aebe","line":216,"in_reply_to":"9fb8cfa7_c4caf418","updated":"2019-06-13 13:21:07.000000000","message":"ah, OK, forgot we still do that DB call, which takes ages.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4fa2a258acf11b8aff3869261d4a0ecc997e09a6","unresolved":false,"context_lines":[{"line_number":213,"context_line":"   optimizing the scheduler (which is something we should do anyway), part of"},{"line_number":214,"context_line":"   making scheduling faster in nova is dependent on nova asking placement"},{"line_number":215,"context_line":"   more informed questions and placement providing a smaller set of allocation"},{"line_number":216,"context_line":"   candidates, i.e. filter in SQL (placement) rather than in python (nova)."},{"line_number":217,"context_line":""},{"line_number":218,"context_line":".. _update_provider_tree: https://docs.openstack.org/nova/latest/reference/update-provider-tree.html"},{"line_number":219,"context_line":""}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_c4caf418","line":216,"in_reply_to":"9fb8cfa7_f8eb1dd9","updated":"2019-06-05 14:27:52.000000000","message":"I\u0027m not sure it\u0027s the placement API call that is taking the time, it still could be iterating the enabled cells and pulling results (compute nodes) from those tables, plus their related instances to populate the HostState objects (oh and aggregates for each HostState object). Especially if we\u0027re not caching that information it\u0027s refreshed on each request. Some work has been done to try and optimize some of that (as referenced https://bugs.launchpad.net/nova/+bug/1737465) and Jay has a patch for more:\n\nhttps://review.opendev.org/#/c/623558/\n\nI haven\u0027t dug into much of that in awhile though.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2d3348af996dc5e4ea6824ead693e89dd9baf239","unresolved":false,"context_lines":[{"line_number":213,"context_line":"   optimizing the scheduler (which is something we should do anyway), part of"},{"line_number":214,"context_line":"   making scheduling faster in nova is dependent on nova asking placement"},{"line_number":215,"context_line":"   more informed questions and placement providing a smaller set of allocation"},{"line_number":216,"context_line":"   candidates, i.e. filter in SQL (placement) rather than in python (nova)."},{"line_number":217,"context_line":""},{"line_number":218,"context_line":".. _update_provider_tree: https://docs.openstack.org/nova/latest/reference/update-provider-tree.html"},{"line_number":219,"context_line":""}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_f8eb1dd9","line":216,"in_reply_to":"dfbec78f_1cd60ec5","updated":"2019-06-05 11:21:02.000000000","message":"FWIW, it always used to be the DB call that used up most of the time, I guess now its the API call to placement, but it would be good to know. After that (if you had debug logs on) it was the debug logging that took most of the time.\n\nThis is the tooling I used previously to check on some of this:\nhttps://github.com/openstack/nova/blob/c19d602f9be536ee8412bac0f0951a995424f25e/nova/tests/unit/scheduler/test_caching_scheduler.py#L300","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"2d3348af996dc5e4ea6824ead693e89dd9baf239","unresolved":false,"context_lines":[{"line_number":216,"context_line":"   candidates, i.e. filter in SQL (placement) rather than in python (nova)."},{"line_number":217,"context_line":""},{"line_number":218,"context_line":".. _update_provider_tree: https://docs.openstack.org/nova/latest/reference/update-provider-tree.html"},{"line_number":219,"context_line":""},{"line_number":220,"context_line":"Data model impact"},{"line_number":221,"context_line":"-----------------"},{"line_number":222,"context_line":""}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_f8d55de6","line":219,"updated":"2019-06-05 11:21:02.000000000","message":"We could query the Nova DB to get all the disabled and forced_down hosts, and send them as a \"not in tree\" filter? I guess that list gets really big really quickly, so it sucks too?","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"4fa2a258acf11b8aff3869261d4a0ecc997e09a6","unresolved":false,"context_lines":[{"line_number":216,"context_line":"   candidates, i.e. filter in SQL (placement) rather than in python (nova)."},{"line_number":217,"context_line":""},{"line_number":218,"context_line":".. _update_provider_tree: https://docs.openstack.org/nova/latest/reference/update-provider-tree.html"},{"line_number":219,"context_line":""},{"line_number":220,"context_line":"Data model impact"},{"line_number":221,"context_line":"-----------------"},{"line_number":222,"context_line":""}],"source_content_type":"text/x-rst","patch_set":2,"id":"9fb8cfa7_c4c194e6","line":219,"in_reply_to":"9fb8cfa7_f8d55de6","updated":"2019-06-05 14:27:52.000000000","message":"There is no !in_tree semantic in GET /allocation_candidates and yeah that would get pretty unwieldy pretty fast I\u0027d think.  What you\u0027re describing is really more akin to the first alternative which would be putting these types of computes into a forbidden aggregate.","commit_id":"bcf3e790e49ad678b66c5f93b06be7049eae27af"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7bd6a9935f5348c43592e4d11c2927b19f7dd5d3","unresolved":false,"context_lines":[{"line_number":71,"context_line":"driver`_ for use with the (now deprecated) `Update Host Status API`_."},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"This blueprint proposes to use that compute method to generically add/remove"},{"line_number":74,"context_line":"the ``COMPUTE_STATUS_DISABLED`` trait on the compute nodes managed by that"},{"line_number":75,"context_line":"service (note that for ironic a compute service host can manage multiple"},{"line_number":76,"context_line":"nodes)."},{"line_number":77,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_00d1c5fb","line":74,"range":{"start_line":74,"start_character":32,"end_line":74,"end_character":58},"updated":"2019-06-06 10:45:24.000000000","message":"Does it mean that the trait will be put on the root RPs representing the compute nodes? or all nested RPs as well?","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"914fe65feef7018d31eeb5fb265d6d844a91d50c","unresolved":false,"context_lines":[{"line_number":71,"context_line":"driver`_ for use with the (now deprecated) `Update Host Status API`_."},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"This blueprint proposes to use that compute method to generically add/remove"},{"line_number":74,"context_line":"the ``COMPUTE_STATUS_DISABLED`` trait on the compute nodes managed by that"},{"line_number":75,"context_line":"service (note that for ironic a compute service host can manage multiple"},{"line_number":76,"context_line":"nodes)."},{"line_number":77,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_d4d5abe6","line":74,"range":{"start_line":74,"start_character":32,"end_line":74,"end_character":58},"in_reply_to":"9fb8cfa7_00d1c5fb","updated":"2019-06-06 13:56:13.000000000","message":"Just the root. I haven\u0027t considered effectively disabling the entire tree at this point. Do we have any real supported scenarios in nova where you\u0027d be requesting resources during scheduling that don\u0027t involve the root compute node provider?","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d25f72fa72b891a6f43f46953e71dc437afd405a","unresolved":false,"context_lines":[{"line_number":71,"context_line":"driver`_ for use with the (now deprecated) `Update Host Status API`_."},{"line_number":72,"context_line":""},{"line_number":73,"context_line":"This blueprint proposes to use that compute method to generically add/remove"},{"line_number":74,"context_line":"the ``COMPUTE_STATUS_DISABLED`` trait on the compute nodes managed by that"},{"line_number":75,"context_line":"service (note that for ironic a compute service host can manage multiple"},{"line_number":76,"context_line":"nodes)."},{"line_number":77,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_bef77bd3","line":74,"range":{"start_line":74,"start_character":32,"end_line":74,"end_character":58},"in_reply_to":"9fb8cfa7_d4d5abe6","updated":"2019-06-10 19:03:19.000000000","message":"Added a sentence saying the trait will only be managed on the root provider. If we have a use case that requires managing the trait on the entire tree we can add that in the future.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7bd6a9935f5348c43592e4d11c2927b19f7dd5d3","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_c0fded3a","line":81,"updated":"2019-06-06 10:45:24.000000000","message":"What is the reason to implement this trait handling in the compute service? The \u0027disabled\u0027 state is persisted in the db so it could be handled from the conductor centrally. \n\nAlso I think today I can set a compute service to disable via the REST API while the compute service is not running. However in the proposed solution this will not be propagated to placement.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"2da0450919823b71ae2dde6ade5d99ac83eaf91f","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_f1a55c05","line":81,"in_reply_to":"9fb8cfa7_0b0d6136","updated":"2019-06-06 16:06:44.000000000","message":"\u003e The 202 is just to signal that we did something but not everything\n \u003e right? Two questions come out of this:\n\nThe 202 signals to the client that we accepted the request and will take action on it at some point. It means they need to poll for completion instead of assuming it\u0027s done. So in the case of the down compute, we\u0027ve recorded the disabledness, but it\u0027s not really in full effect until the future point when the compute comes up and processes the request (compute.disabled, which now is just a signal that we want it to disable itself). However, it\u0027s equivalent if the compute is down now, so we *could* return 200 I guess since the behavior *should* be the same.\n\n \u003e In thinking about this, if our answer for old-but-up computes\n \u003e before was \"ignore and let the trait get synced when those computes\n \u003e are upgraded and restarted\" then why is that really different from\n \u003e new-but-down computes? IOW, we didn\u0027t say we\u0027d change the response\n \u003e code in the old-but-up case so why change the response code in the\n \u003e new-but-down case, and instead just treat it like an old-but-up\n \u003e compute and say we\u0027ll sync the trait on restart of the compute and\n \u003e leave it at that? That way there is no microversion worry.\n\nI think the difference is that we\u0027d leave the computefilter behavior in place until the next cycle, which means setting compute.disabled is just as good as having the trait in place, thus the behavior hasn\u0027t changed from the disabling-user\u0027s perspective. But, if the compute is down, it can\u0027t be scheduled to anyway, and it will process the disable request pretty much immediately on startup (there _is_ a delay between setting the service up and processing things, which could cause the scheduler to send something there).\n\nAnyway, I can be persuaded to go either way on the 202/200 or always-200 thing.\n\n \u003e But like you said I\u0027d prefer to not embed business logic like this\n \u003e in the object code, especially when it\u0027s dealing with an external\n \u003e service.\n\nYep.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"92114eb4aa2afaa7d6a6929fad24a0890a150c3b","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_0b0d6136","line":81,"in_reply_to":"9fb8cfa7_2f22b2a5","updated":"2019-06-06 14:40:49.000000000","message":"\u003e Yep, this is a good point, although I don\u0027t think it changes my\n \u003e desire to have it be owned by the compute at all times other than\n \u003e when it can\u0027t be.\n \u003e \n \u003e So, we could go the local delete route, where we check for up-ness\n \u003e before we make the call and direct-disable it in placement if so.\n\nRight this is basically alternative 3 below except doing an up check to the service group API first. I would also like to avoid managing the trait in more than one place.\n\n \u003e We could also check the up-ness, and if so, not make the rpc call,\n \u003e but set the disabled state in the database and return 202 instead\n \u003e of 200. While the compute is down, the trait in placement doesn\u0027t\n \u003e matter. When the compute comes back, the initial u-p-t sync will\n \u003e see the disabled flag on its record, and set it in placement. I\n \u003e think this is my preference.\n\nThe 202 is just to signal that we did something but not everything right? Two questions come out of this:\n\n1) Would we also return 202 for disabling old-but-up computes?\n\n2) Would there be a microversion bump? I think that would be gross but according to our rules it would require a microversion:\n\nhttps://docs.openstack.org/nova/latest/contributor/microversions.html#when-do-i-need-a-new-microversion\n\nIn thinking about this, if our answer for old-but-up computes before was \"ignore and let the trait get synced when those computes are upgraded and restarted\" then why is that really different from new-but-down computes? IOW, we didn\u0027t say we\u0027d change the response code in the old-but-up case so why change the response code in the new-but-down case, and instead just treat it like an old-but-up compute and say we\u0027ll sync the trait on restart of the compute and leave it at that? That way there is no microversion worry.\n\n \u003e We could also embed this in the object itself, such that setting\n \u003e disabled and calling save() pokes placement (which I think is what\n \u003e Gibi is suggesting). That would work for whoever makes the actual\n \u003e call, however, I definitely prefer less magic of this sort in the\n \u003e objects, especially calling out to another service. I also really\n \u003e think that setting traits on a provider should be done by the owner\n \u003e of the provider (i.e. the compute service in this case) as much as\n \u003e and wherever possible.\n\nI also thought about embedding some of this in the Service object itself:\n\nhttps://review.opendev.org/#/c/654596/2/nova/compute/api.py@5121\n\nBut like you said I\u0027d prefer to not embed business logic like this in the object code, especially when it\u0027s dealing with an external service.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9776133af6531372bb42ba2109447342e195a5d1","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_65df3dd6","line":81,"in_reply_to":"9fb8cfa7_517d48e5","updated":"2019-06-10 13:51:35.000000000","message":"\u003e Maybe you\u0027re already saying the same thing and I\u0027m just\n \u003e not reading you clearly.\n\nI think we\u0027re saying the same thing(s).\n\n \u003e I understand the idea behind the 202 but I\u0027d really try to avoid it\n \u003e for this if possible *if* it means we need a new microversion for\n \u003e that behavior, because then it just gets all sorts of crazy\n \u003e complicated for what is essentially a best-effort kind of thing to\n \u003e optimize our allocation candidate filtering request to placement.\n\nI thought you said that _just_ that change didn\u0027t? Anyway, you\u0027d be the expert there, so I\u0027ll defer to your preference.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c06503431cbda0ade5e73dd7756599bfd037b287","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_7eba630a","line":81,"in_reply_to":"9fb8cfa7_65df3dd6","updated":"2019-06-10 18:46:24.000000000","message":"http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2019-06-10.log.html#t2019-06-10T15:40:22\n\ndansmith\tmriedem: are you going to update the pre-filter disabled computes spec?\t15:40\ndansmith\tI assume gibi is on vacay or something and hasn\u0027t circled back,\t15:40\ndansmith\tbut I figure he\u0027s probably fine with it as long as we have a plan for that (which we do)\t15:40\nmriedem\tdansmith: i was waiting for gibi\t15:41\ndansmith\tokay\t15:42\nmriedem\twhat was the plan again? :) on disable, check if down and if down, return 200 (like if it was an old compute) and let upt sync the trait on restart of the service\t15:43\n*** helenafm has quit IRC\t15:43\nmriedem\tif that\u0027s the plan i can write that edge case into the spec\t15:44\ndansmith\tyeah, always set compute.disabled\u003dTrue, not make the rpc call if it\u0027s down, return 200\t15:44\n\nBased on that I\u0027ll update the spec with a section about edge cases (old computes during rolling upgrade and down computes).","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"210d83c2e3bf777445c997463914ec12786fc370","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_2f22b2a5","line":81,"in_reply_to":"9fb8cfa7_b4fd973b","updated":"2019-06-06 14:06:35.000000000","message":"Yep, this is a good point, although I don\u0027t think it changes my desire to have it be owned by the compute at all times other than when it can\u0027t be.\n\nSo, we could go the local delete route, where we check for up-ness before we make the call and direct-disable it in placement if so.\n\nWe could also check the up-ness, and if so, not make the rpc call, but set the disabled state in the database and return 202 instead of 200. While the compute is down, the trait in placement doesn\u0027t matter. When the compute comes back, the initial u-p-t sync will see the disabled flag on its record, and set it in placement. I think this is my preference.\n\nWe could also embed this in the object itself, such that setting disabled and calling save() pokes placement (which I think is what Gibi is suggesting). That would work for whoever makes the actual call, however, I definitely prefer less magic of this sort in the objects, especially calling out to another service. I also really think that setting traits on a provider should be done by the owner of the provider (i.e. the compute service in this case) as much as and wherever possible.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"914fe65feef7018d31eeb5fb265d6d844a91d50c","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_b4fd973b","line":81,"in_reply_to":"9fb8cfa7_c0fded3a","updated":"2019-06-06 13:56:13.000000000","message":"\u003e What is the reason to implement this trait handling in the compute\n \u003e service? The \u0027disabled\u0027 state is persisted in the db so it could be\n \u003e handled from the conductor centrally.\n\nSingle ownership for one - there is already that method in the compute service to do something like this but was only implemented by the xenapi driver. The other thing the libvirt driver code that listens for hypervisor up/down events and disables/enables the compute service accordingly, I want to hook that into the same compute service code:\n\nhttps://review.opendev.org/#/c/654596/2/nova/virt/libvirt/driver.py\n\n \u003e Also I think today I can set a compute service to disable via the\n \u003e REST API while the compute service is not running. However in the\n \u003e proposed solution this will not be propagated to placement.\n\nHmm, I hadn\u0027t considered that but you\u0027re right. I\u0027m not sure how much people do that but you\u0027re right that it\u0027s possible. Thinking through that, what I\u0027d expect to happen if you stopped the compute service without disabling it is it would eventually not report into the service group API and be reported as \"down\" so the ComputeFilter would filter it out if placement returned it as a candidate anyway, which is really no worse off than we have today, right? Also, if you disabled the compute while it\u0027s stopped and then restarted it and it\u0027s still disabled in the DB, the upt code would sync the trait. So I don\u0027t think this really \"breaks\" that flow, it just means you wouldn\u0027t get the benefit of the pre-filter for those computes but the ComputeFilter would still filter them out.\n\nThe alternative here is that the API (or conductor?) would apply/remove the trait but we\u0027d still want it on the compute service as well for the upt sync and for the libvirt scenario, which means we\u0027d have duplication of management of the trait which is something I wanted to avoid since it makes the solution more complicated. There was some discussion about this in PS2 of the change.\n\nSo in summary you\u0027re right this wouldn\u0027t apply the trait from the API for already stopped compute services but it doesn\u0027t really break the scheduling aspect.\n\nMaybe your concern is that what breaks (or is new behavior) is if you try to disable a stopped compute the API will fail because the set_host_enabled RPC call would fail? I could see that being an issue, but we could likely handle it through an oslo.messaging error from the compute RPC API and/or by checking if the service is up first.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"90c7ea4727e7e050564fb46e5be1200a83d8ed46","unresolved":false,"context_lines":[{"line_number":78,"context_line":"The actual implementation will be part of the `ComputeVirtAPI`_ so that"},{"line_number":79,"context_line":"the libvirt driver has access to it when it automatically disables or enables"},{"line_number":80,"context_line":"the compute node based on events from the hypervisor. [1]_"},{"line_number":81,"context_line":""},{"line_number":82,"context_line":"update_provider_tree"},{"line_number":83,"context_line":"~~~~~~~~~~~~~~~~~~~~"},{"line_number":84,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_517d48e5","line":81,"in_reply_to":"9fb8cfa7_f1a55c05","updated":"2019-06-06 16:29:45.000000000","message":"\u003e I think the difference is that we\u0027d leave the computefilter behavior in place until the next cycle, which means setting compute.disabled is just as good as having the trait in place, thus the behavior hasn\u0027t changed from the disabling-user\u0027s perspective. But, if the compute is down, it can\u0027t be scheduled to anyway, and it will process the disable request pretty much immediately on startup (there _is_ a delay between setting the service up and processing things, which could cause the scheduler to send something there).\n\nWe had mentioned maybe dropping the \"is disabled\" check from the ComputeFilter in the U release since we\u0027d be pre-filtering those now - would that change if the API isn\u0027t setting the trait on down computes? If we removed the \"is disabled\" check from ComputeFilter in U then it\u0027s just checking \"is up\" which in this case might be good enough since in this scenario we didn\u0027t set the trait because the service was down. Sure there is a window where you could get scheduled to that compute (scheduling request hits that compute right after it\u0027s restarted so it\u0027s up but the trait isn\u0027t set yet), but that seems like such an edge case I wouldn\u0027t worry too much about it. Maybe you\u0027re already saying the same thing and I\u0027m just not reading you clearly.\n\n\u003e Anyway, I can be persuaded to go either way on the 202/200 or always-200 thing.\n\nI understand the idea behind the 202 but I\u0027d really try to avoid it for this if possible *if* it means we need a new microversion for that behavior, because then it just gets all sorts of crazy complicated for what is essentially a best-effort kind of thing to optimize our allocation candidate filtering request to placement.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7bd6a9935f5348c43592e4d11c2927b19f7dd5d3","unresolved":false,"context_lines":[{"line_number":96,"context_line":""},{"line_number":97,"context_line":"When the `os-services`_ API(s) are used to enable or disable a compute service,"},{"line_number":98,"context_line":"the API will synchronously call the compute service via the"},{"line_number":99,"context_line":"``set_host_enabled`` RPC method to reflect the trait on the"},{"line_number":100,"context_line":"related compute node resource providers in placement appropriately. For"},{"line_number":101,"context_line":"example, if compute service A is disabled, the trait will be added. When"},{"line_number":102,"context_line":"compute service A is enabled, the trait will be removed."}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_202089b0","line":99,"updated":"2019-06-06 10:45:24.000000000","message":"This will cause that setting the compute disabled while it is not running will not be possible any more.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"914fe65feef7018d31eeb5fb265d6d844a91d50c","unresolved":false,"context_lines":[{"line_number":96,"context_line":""},{"line_number":97,"context_line":"When the `os-services`_ API(s) are used to enable or disable a compute service,"},{"line_number":98,"context_line":"the API will synchronously call the compute service via the"},{"line_number":99,"context_line":"``set_host_enabled`` RPC method to reflect the trait on the"},{"line_number":100,"context_line":"related compute node resource providers in placement appropriately. For"},{"line_number":101,"context_line":"example, if compute service A is disabled, the trait will be added. When"},{"line_number":102,"context_line":"compute service A is enabled, the trait will be removed."}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_7495dfe3","line":99,"in_reply_to":"9fb8cfa7_202089b0","updated":"2019-06-06 13:56:13.000000000","message":"Not necessarily, see above. I think it probably just means we have to handle the service being down.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"d25f72fa72b891a6f43f46953e71dc437afd405a","unresolved":false,"context_lines":[{"line_number":96,"context_line":""},{"line_number":97,"context_line":"When the `os-services`_ API(s) are used to enable or disable a compute service,"},{"line_number":98,"context_line":"the API will synchronously call the compute service via the"},{"line_number":99,"context_line":"``set_host_enabled`` RPC method to reflect the trait on the"},{"line_number":100,"context_line":"related compute node resource providers in placement appropriately. For"},{"line_number":101,"context_line":"example, if compute service A is disabled, the trait will be added. When"},{"line_number":102,"context_line":"compute service A is enabled, the trait will be removed."}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_7ea7c3a8","line":99,"in_reply_to":"9fb8cfa7_7495dfe3","updated":"2019-06-10 19:03:19.000000000","message":"Adding a sub-section here about down computes.","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"7bd6a9935f5348c43592e4d11c2927b19f7dd5d3","unresolved":false,"context_lines":[{"line_number":173,"context_line":""},{"line_number":174,"context_line":"   * Pros: No blocking calls from the API to the compute service when changing"},{"line_number":175,"context_line":"     the disabled status of the service - although one could argue the blocking"},{"line_number":176,"context_line":"     nature proposed in the spec is advantageous so the admin gets confirmation"},{"line_number":177,"context_line":"     that the service is disabled and will be pre-filtered properly during"},{"line_number":178,"context_line":"     scheduling."},{"line_number":179,"context_line":""}],"source_content_type":"text/x-rst","patch_set":3,"id":"9fb8cfa7_40149d4e","line":176,"range":{"start_line":176,"start_character":36,"end_line":176,"end_character":48},"updated":"2019-06-06 10:45:24.000000000","message":"but backward incompatible","commit_id":"2ec5d345051ed853d51d1fd269129888522a3c8d"},{"author":{"_account_id":782,"name":"John Garbutt","email":"john@johngarbutt.com","username":"johngarbutt"},"change_message_id":"e763251e6d75ea02f336b9541cb52cad473eeaaf","unresolved":false,"context_lines":[{"line_number":114,"context_line":"is up using the `service group API`_. If the service is down, the API will not"},{"line_number":115,"context_line":"call the ``set_host_enabled`` compute method and instead just update the"},{"line_number":116,"context_line":"``services.disabled`` value in the DB as today and return. When the compute"},{"line_number":117,"context_line":"service is restarted, the `update_provider_tree`_ flow will sync the trait."},{"line_number":118,"context_line":""},{"line_number":119,"context_line":"Scheduler changes"},{"line_number":120,"context_line":"-----------------"}],"source_content_type":"text/x-rst","patch_set":4,"id":"9fb8cfa7_806a1a17","line":117,"updated":"2019-06-13 13:27:55.000000000","message":"It is super tempting to do the direct to placement update in this case, updating only the trait. For that \"its dead\" case, force it down, now evacuating all the servers. You can just delete the RP in placement after the force down call, I guess.\n\nI prefer this simpler starting point, as you have it now. Just thinking out loud.","commit_id":"11c5cca922fae4f31ce1d0ba391007a4b6a95b9b"}]}