)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"a5600e7c30a723dd2a248de5ed850605514dd55d","unresolved":false,"context_lines":[{"line_number":12,"context_line":"that I would unfortunately not be able to isolate the claim logic in"},{"line_number":13,"context_line":"the scheduler manager. Instead, due to the way that the host state"},{"line_number":14,"context_line":"objects are passed into the filters and weighers and modified in the"},{"line_number":15,"context_line":"filter scheduler driver\u0027s _schedule() method, it is not possible to"},{"line_number":16,"context_line":"pull the logic for retrying a claim into a higher abstraction layer."},{"line_number":17,"context_line":"The coupling is just too tight between the state (host objects) and the"},{"line_number":18,"context_line":"calling interface itself (select_destinations())."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_86ba9a07","line":16,"range":{"start_line":15,"start_character":46,"end_line":16,"end_character":68},"updated":"2017-07-11 16:33:38.000000000","message":"I\u0027m not sure why that\u0027s not possible - it certainly seemed possible when you first described it. Perhaps if there were multiple eyes looking at this, a solution might have been seen.\n\nIAC, I\u0027m glad that we\u0027re back to claiming in the driver. It is the driver whose job it is to make the choice, and \"claims in the scheduler\" is all about that that choice is valid in the face of potential race conditions.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4750faf771535125ad6548c5929314124ed9d3b2","unresolved":false,"context_lines":[{"line_number":12,"context_line":"that I would unfortunately not be able to isolate the claim logic in"},{"line_number":13,"context_line":"the scheduler manager. Instead, due to the way that the host state"},{"line_number":14,"context_line":"objects are passed into the filters and weighers and modified in the"},{"line_number":15,"context_line":"filter scheduler driver\u0027s _schedule() method, it is not possible to"},{"line_number":16,"context_line":"pull the logic for retrying a claim into a higher abstraction layer."},{"line_number":17,"context_line":"The coupling is just too tight between the state (host objects) and the"},{"line_number":18,"context_line":"calling interface itself (select_destinations())."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_c9cae56c","line":16,"range":{"start_line":15,"start_character":46,"end_line":16,"end_character":68},"in_reply_to":"3f1d235d_86ba9a07","updated":"2017-07-11 17:05:05.000000000","message":"it\u0027s not possible \"due to the way that the host state\nobjects are passed into the filters and weighers and modified in the filter scheduler driver\u0027s _schedule() method\".\n\nIf the above is just trying to piss me off with a \"I told you so\" comment or a \"if only you\u0027d listened to me\" comment, you\u0027ve accomplished that.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"5201df05e6cb00d2769d95bdaeb863b5c155b92d","unresolved":false,"context_lines":[{"line_number":12,"context_line":"that I would unfortunately not be able to isolate the claim logic in"},{"line_number":13,"context_line":"the scheduler manager. Instead, due to the way that the host state"},{"line_number":14,"context_line":"objects are passed into the filters and weighers and modified in the"},{"line_number":15,"context_line":"filter scheduler driver\u0027s _schedule() method, it is not possible to"},{"line_number":16,"context_line":"pull the logic for retrying a claim into a higher abstraction layer."},{"line_number":17,"context_line":"The coupling is just too tight between the state (host objects) and the"},{"line_number":18,"context_line":"calling interface itself (select_destinations())."},{"line_number":19,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_3422cca5","line":16,"range":{"start_line":15,"start_character":46,"end_line":16,"end_character":68},"in_reply_to":"3f1d235d_c9cae56c","updated":"2017-07-11 17:35:59.000000000","message":"No, the above is trying to help you, given the time crunch. Having to start with another approach will only slow us down, and I\u0027d like to help as much as I can to avoid any delays.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"df390ac6c972e6e3a7a5e1bf3872a5619e2805be","unresolved":false,"context_lines":[{"line_number":18,"context_line":"calling interface itself (select_destinations())."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"And so this patch sets the stage for retrying claims from within the"},{"line_number":21,"context_line":"filter scheduler\u0027s _schedule() method -- which is the method where the"},{"line_number":22,"context_line":"state objects are being modified. We change the _schedule() method to"},{"line_number":23,"context_line":"return a dict, keyed by instance UUID, containing lists of hosts"},{"line_number":24,"context_line":"ordered by fitness. This dict will eventually be used to return the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_7a16f4a9","line":21,"updated":"2017-07-11 15:29:06.000000000","message":"I haven\u0027t made it into the actual code, but based on my experience with trying to understand the behaviors in the scheduler, this actually feels like a more intuitive fit to me. I suppose I might change my mind once I get into the code...","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"a5600e7c30a723dd2a248de5ed850605514dd55d","unresolved":false,"context_lines":[{"line_number":21,"context_line":"filter scheduler\u0027s _schedule() method -- which is the method where the"},{"line_number":22,"context_line":"state objects are being modified. We change the _schedule() method to"},{"line_number":23,"context_line":"return a dict, keyed by instance UUID, containing lists of hosts"},{"line_number":24,"context_line":"ordered by fitness. This dict will eventually be used to return the"},{"line_number":25,"context_line":"list of alternate hosts for each scheduled instance, but for now, and"},{"line_number":26,"context_line":"for the next patch which adds calls to placement to claim resources"},{"line_number":27,"context_line":"during the filter scheduler driver\u0027s _schedule() method, we keep the"},{"line_number":28,"context_line":"generation of alternate hosts isolated within the filter scheduler"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_86729a48","line":25,"range":{"start_line":24,"start_character":20,"end_line":25,"end_character":51},"updated":"2017-07-11 16:33:38.000000000","message":"I\u0027m not clear why the alternates need to be a separate thing. We can simply return a list of hosts, with the first being the \"chosen\" host against which allocations have been successfully made, and the rest being alternates. The cell conductor can then loop on this list, using logic like:\n\n  try:\n    build(hosts[0])\n  except BuidlFailed:\n    deallocate(hosts[0])\n    for host in hosts[1:]:\n      try:\n        allocate(host)\n      except CantAllocate:\n        continue\n      try:\n        build(host)\n      except BuidlFailed:\n        deallocate(host)\n        continue","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4750faf771535125ad6548c5929314124ed9d3b2","unresolved":false,"context_lines":[{"line_number":21,"context_line":"filter scheduler\u0027s _schedule() method -- which is the method where the"},{"line_number":22,"context_line":"state objects are being modified. We change the _schedule() method to"},{"line_number":23,"context_line":"return a dict, keyed by instance UUID, containing lists of hosts"},{"line_number":24,"context_line":"ordered by fitness. This dict will eventually be used to return the"},{"line_number":25,"context_line":"list of alternate hosts for each scheduled instance, but for now, and"},{"line_number":26,"context_line":"for the next patch which adds calls to placement to claim resources"},{"line_number":27,"context_line":"during the filter scheduler driver\u0027s _schedule() method, we keep the"},{"line_number":28,"context_line":"generation of alternate hosts isolated within the filter scheduler"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_e9948146","line":25,"range":{"start_line":24,"start_character":20,"end_line":25,"end_character":51},"in_reply_to":"3f1d235d_86729a48","updated":"2017-07-11 17:05:05.000000000","message":"Sure, that\u0027s fine by me. Not relevant to this patch, though.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"5201df05e6cb00d2769d95bdaeb863b5c155b92d","unresolved":false,"context_lines":[{"line_number":21,"context_line":"filter scheduler\u0027s _schedule() method -- which is the method where the"},{"line_number":22,"context_line":"state objects are being modified. We change the _schedule() method to"},{"line_number":23,"context_line":"return a dict, keyed by instance UUID, containing lists of hosts"},{"line_number":24,"context_line":"ordered by fitness. This dict will eventually be used to return the"},{"line_number":25,"context_line":"list of alternate hosts for each scheduled instance, but for now, and"},{"line_number":26,"context_line":"for the next patch which adds calls to placement to claim resources"},{"line_number":27,"context_line":"during the filter scheduler driver\u0027s _schedule() method, we keep the"},{"line_number":28,"context_line":"generation of alternate hosts isolated within the filter scheduler"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3f1d235d_147350b3","line":25,"range":{"start_line":24,"start_character":20,"end_line":25,"end_character":51},"in_reply_to":"3f1d235d_e9948146","updated":"2017-07-11 17:35:59.000000000","message":"It\u0027s meant to apply to the highlighted comment. IOW, you don\u0027t need a separate list for alternates. The pseudo-code was to illustrate why.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"4e9795a8acbb90c0377ff83025e08c9623cccc34","unresolved":false,"context_lines":[{"line_number":15,"context_line":"filter scheduler driver\u0027s _schedule() method, it is not possible to"},{"line_number":16,"context_line":"pull the logic for retrying a claim into a higher abstraction layer."},{"line_number":17,"context_line":"The coupling is just too tight between the state (host objects) and the"},{"line_number":18,"context_line":"calling interface itself (select_destinations())."},{"line_number":19,"context_line":""},{"line_number":20,"context_line":"And so this patch sets the stage for retrying claims from within the"},{"line_number":21,"context_line":"filter scheduler\u0027s _schedule() method -- which is the method where the"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":3,"id":"1f1a1f67_26a2fb9b","line":18,"updated":"2017-07-12 14:21:35.000000000","message":"The FilterScheduler (and CachingScheduler) is actually using a non-driver-specific interface for consuming host states which is basically HostState.consume_from_request()\n\nWhy should we consider the above as a necessary constraint ?\nWe could imagine that since a HostState object is basically tied to a root RP, we could do the claim there ?\n\nThat said, I agree with you, the retrying mechanism should necessary stay in the FilterScheduler driver. I don\u0027t really see that as something bad, since the other driver that real production folks use is CachingScheduler which is using the same methd for scheduling.\n\nThe only real problem would go with ChanceScheduler, but that\u0027s only a NOOP scheduler not implementing retries anyway, so is that really a problem worth mentioning in the commit msg?","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"}],"nova/scheduler/filter_scheduler.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"df390ac6c972e6e3a7a5e1bf3872a5619e2805be","unresolved":false,"context_lines":[{"line_number":50,"context_line":"            dict(request_spec\u003dspec_obj.to_legacy_request_spec_dict()))"},{"line_number":51,"context_line":""},{"line_number":52,"context_line":"        num_instances \u003d spec_obj.num_instances"},{"line_number":53,"context_line":"        hosts_by_instance_uuid \u003d self._schedule(context, spec_obj,"},{"line_number":54,"context_line":"            instance_uuids, provider_summaries)"},{"line_number":55,"context_line":""},{"line_number":56,"context_line":"        # Couldn\u0027t fulfill the request_spec"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_9a78b0f8","line":53,"updated":"2017-07-11 15:29:06.000000000","message":"good name, thanks","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"df390ac6c972e6e3a7a5e1bf3872a5619e2805be","unresolved":false,"context_lines":[{"line_number":119,"context_line":""},{"line_number":120,"context_line":"            host_subset_size \u003d CONF.filter_scheduler.host_subset_size"},{"line_number":121,"context_line":"            num_hosts \u003d min(host_subset_size, len(weighed_hosts))"},{"line_number":122,"context_line":"            random.shuffle(weighed_hosts[0:num_hosts])"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"            chosen_host \u003d weighed_hosts[0]"},{"line_number":125,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_fafd643b","line":122,"updated":"2017-07-11 15:29:06.000000000","message":"I\u0027ve heard many times why this is necessary, but still kind of facepalm every time I see it. Now might be a great time to add a comment that makes why explicit.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"f9f115dcb93d572105233880618e4ac1846ec423","unresolved":false,"context_lines":[{"line_number":119,"context_line":""},{"line_number":120,"context_line":"            host_subset_size \u003d CONF.filter_scheduler.host_subset_size"},{"line_number":121,"context_line":"            num_hosts \u003d min(host_subset_size, len(weighed_hosts))"},{"line_number":122,"context_line":"            random.shuffle(weighed_hosts[0:num_hosts])"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"            chosen_host \u003d weighed_hosts[0]"},{"line_number":125,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_a06d7e99","line":122,"in_reply_to":"3f1d235d_fafd643b","updated":"2017-07-11 15:34:52.000000000","message":"Done","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"a5600e7c30a723dd2a248de5ed850605514dd55d","unresolved":false,"context_lines":[{"line_number":121,"context_line":"            num_hosts \u003d min(host_subset_size, len(weighed_hosts))"},{"line_number":122,"context_line":"            random.shuffle(weighed_hosts[0:num_hosts])"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"            chosen_host \u003d weighed_hosts[0]"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"            LOG.debug(\"Selected host: %(host)s\", {\u0027host\u0027: chosen_host})"},{"line_number":127,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_e3d020fa","line":124,"updated":"2017-07-11 16:33:38.000000000","message":"This use of random.shuffle as written here on a slice of a list doesn\u0027t work:\n\n  \u003e\u003e import random\n  \u003e\u003e x \u003d [1,2,3,4,5,6,7,8,9,10]\n  \u003e\u003e random.shuffle(x[0:5])\n  \u003e\u003e print(x)\n  [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]\n  \u003e\u003e random.shuffle(x[0:5])\n  \u003e\u003e print(x)\n  [1, 2, 3, 4, 5, 6, 7, 8, 9, 10]\n\nrandom.shuffle only works on full lists. It would be simpler and cleaner to use the following:\n\n  chosen_host \u003d random.choice(weighed_hosts[0:num_hosts])\n  weighed_hosts.remove(chosen_host)","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4750faf771535125ad6548c5929314124ed9d3b2","unresolved":false,"context_lines":[{"line_number":121,"context_line":"            num_hosts \u003d min(host_subset_size, len(weighed_hosts))"},{"line_number":122,"context_line":"            random.shuffle(weighed_hosts[0:num_hosts])"},{"line_number":123,"context_line":""},{"line_number":124,"context_line":"            chosen_host \u003d weighed_hosts[0]"},{"line_number":125,"context_line":""},{"line_number":126,"context_line":"            LOG.debug(\"Selected host: %(host)s\", {\u0027host\u0027: chosen_host})"},{"line_number":127,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_a991e99b","line":124,"in_reply_to":"3f1d235d_e3d020fa","updated":"2017-07-11 17:05:05.000000000","message":"hmm, thanks for pointing this out. I wasn\u0027t aware of this and due to having to mock out random.shuffle() wasn\u0027t able to catch this.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"a5600e7c30a723dd2a248de5ed850605514dd55d","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            num_alternates \u003d num_hosts - 1"},{"line_number":129,"context_line":"            if num_alternates \u003e 0:"},{"line_number":130,"context_line":"                LOG.debug(\"Returning %d alternate destinations \""},{"line_number":131,"context_line":"                          \"for instance %s\", num_alternates, instance_uuid)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d weighed_hosts"},{"line_number":134,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_a380e88c","line":131,"updated":"2017-07-11 16:33:38.000000000","message":"You calculate and log this, but never actually do anything with it. Given your TODO on line 81, this should probably go in the subsequent patch, not here.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4750faf771535125ad6548c5929314124ed9d3b2","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            num_alternates \u003d num_hosts - 1"},{"line_number":129,"context_line":"            if num_alternates \u003e 0:"},{"line_number":130,"context_line":"                LOG.debug(\"Returning %d alternate destinations \""},{"line_number":131,"context_line":"                          \"for instance %s\", num_alternates, instance_uuid)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d weighed_hosts"},{"line_number":134,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_c9454517","line":131,"in_reply_to":"3f1d235d_a380e88c","updated":"2017-07-11 17:05:05.000000000","message":"Not sure what you\u0027re talking about, Ed. I\u0027m returning all the weighed hosts on line 133. weighed_hosts includes the chosen host and the alternates. The note on line 81 is about the select_destinations() method, not the _schedule() method.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"5201df05e6cb00d2769d95bdaeb863b5c155b92d","unresolved":false,"context_lines":[{"line_number":128,"context_line":"            num_alternates \u003d num_hosts - 1"},{"line_number":129,"context_line":"            if num_alternates \u003e 0:"},{"line_number":130,"context_line":"                LOG.debug(\"Returning %d alternate destinations \""},{"line_number":131,"context_line":"                          \"for instance %s\", num_alternates, instance_uuid)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d weighed_hosts"},{"line_number":134,"context_line":""}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_948c0080","line":131,"in_reply_to":"3f1d235d_c9454517","updated":"2017-07-11 17:35:59.000000000","message":"What I\u0027m talking about is you calculate \u0027num_alternates\u0027, but never use it for anything other than the log message. I simply don\u0027t understand why this calculation is in this patch if it isn\u0027t used.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"a5600e7c30a723dd2a248de5ed850605514dd55d","unresolved":false,"context_lines":[{"line_number":130,"context_line":"                LOG.debug(\"Returning %d alternate destinations \""},{"line_number":131,"context_line":"                          \"for instance %s\", num_alternates, instance_uuid)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d weighed_hosts"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"},{"line_number":136,"context_line":"            # will change for the next instance."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_c31f646e","line":133,"range":{"start_line":133,"start_character":52,"end_line":133,"end_character":65},"updated":"2017-07-11 16:33:38.000000000","message":"Or did you mean for this to be:\n\n  weighed_hosts[:num_alternates]","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4750faf771535125ad6548c5929314124ed9d3b2","unresolved":false,"context_lines":[{"line_number":130,"context_line":"                LOG.debug(\"Returning %d alternate destinations \""},{"line_number":131,"context_line":"                          \"for instance %s\", num_alternates, instance_uuid)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d weighed_hosts"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"},{"line_number":136,"context_line":"            # will change for the next instance."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_c9f305e7","line":133,"range":{"start_line":133,"start_character":52,"end_line":133,"end_character":65},"in_reply_to":"3f1d235d_c31f646e","updated":"2017-07-11 17:05:05.000000000","message":"No, I meant it to be the above, however I was incorrectly using random.shuffle() on a temporary slice object instead of a copy of the weighed_hosts list.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"5201df05e6cb00d2769d95bdaeb863b5c155b92d","unresolved":false,"context_lines":[{"line_number":130,"context_line":"                LOG.debug(\"Returning %d alternate destinations \""},{"line_number":131,"context_line":"                          \"for instance %s\", num_alternates, instance_uuid)"},{"line_number":132,"context_line":""},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d weighed_hosts"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"},{"line_number":136,"context_line":"            # will change for the next instance."}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_1412300a","line":133,"range":{"start_line":133,"start_character":52,"end_line":133,"end_character":65},"in_reply_to":"3f1d235d_c9f305e7","updated":"2017-07-11 17:35:59.000000000","message":"Well, you seem to be using CONF.filter_scheduler.host_subset_size for the number of alternates to return, when it has nothing to do with how many retries an operator has their system configured for. I think that CONF.scheduler.max_attempts would be a better choice.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"4e9795a8acbb90c0377ff83025e08c9623cccc34","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        hosts_by_instance_uuid \u003d {}"},{"line_number":103,"context_line":"        num_instances \u003d spec_obj.num_instances"},{"line_number":104,"context_line":"        for num in range(num_instances):"},{"line_number":105,"context_line":"            instance_uuid \u003d instance_uuids[num]"},{"line_number":106,"context_line":"            # Filter local hosts based on requirements ..."},{"line_number":107,"context_line":"            hosts \u003d self.host_manager.get_filtered_hosts(hosts,"},{"line_number":108,"context_line":"                    spec_obj, index\u003dnum)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_660d938f","line":105,"updated":"2017-07-12 14:21:35.000000000","message":"thinking out loud if we need to ensure somewhere that the length of that list of instance UUIDs is necessarly equal to num_instances...\n\nIn theory, that works, but should we be conservative?","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a5d27254350b4c2946fba2b1efdeee42ceaa1d47","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        hosts_by_instance_uuid \u003d {}"},{"line_number":103,"context_line":"        num_instances \u003d spec_obj.num_instances"},{"line_number":104,"context_line":"        for num in range(num_instances):"},{"line_number":105,"context_line":"            instance_uuid \u003d instance_uuids[num]"},{"line_number":106,"context_line":"            # Filter local hosts based on requirements ..."},{"line_number":107,"context_line":"            hosts \u003d self.host_manager.get_filtered_hosts(hosts,"},{"line_number":108,"context_line":"                    spec_obj, index\u003dnum)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_21f8a5c2","line":105,"in_reply_to":"1f1a1f67_21c5458f","updated":"2017-07-12 14:43:21.000000000","message":"in theory, yes :) it\u0027s a basic assumption in this code, but I\u0027m not sure if we have ever tested that assumption explicitly...","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"2361b1387c77ca2eb19f7c4699b0ec3aa21f05af","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        hosts_by_instance_uuid \u003d {}"},{"line_number":103,"context_line":"        num_instances \u003d spec_obj.num_instances"},{"line_number":104,"context_line":"        for num in range(num_instances):"},{"line_number":105,"context_line":"            instance_uuid \u003d instance_uuids[num]"},{"line_number":106,"context_line":"            # Filter local hosts based on requirements ..."},{"line_number":107,"context_line":"            hosts \u003d self.host_manager.get_filtered_hosts(hosts,"},{"line_number":108,"context_line":"                    spec_obj, index\u003dnum)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_a15f15e4","line":105,"in_reply_to":"1f1a1f67_660d938f","updated":"2017-07-12 14:41:55.000000000","message":"Meh, not sure :)","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9d17300481cecd0ff8e8a16f3b6d9db4b18f7d30","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        hosts_by_instance_uuid \u003d {}"},{"line_number":103,"context_line":"        num_instances \u003d spec_obj.num_instances"},{"line_number":104,"context_line":"        for num in range(num_instances):"},{"line_number":105,"context_line":"            instance_uuid \u003d instance_uuids[num]"},{"line_number":106,"context_line":"            # Filter local hosts based on requirements ..."},{"line_number":107,"context_line":"            hosts \u003d self.host_manager.get_filtered_hosts(hosts,"},{"line_number":108,"context_line":"                    spec_obj, index\u003dnum)"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_21c5458f","line":105,"in_reply_to":"1f1a1f67_660d938f","updated":"2017-07-12 14:41:59.000000000","message":"This is just instance_uuids, which is always going to be \u003d\u003d num_instances, no?","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"14aa69a894cd99ae578909d9cc6343465d2b94e7","unresolved":false,"context_lines":[{"line_number":129,"context_line":""},{"line_number":130,"context_line":"            # Return the chosen host plus a set of alternate destination hosts"},{"line_number":131,"context_line":"            num_alts \u003d min(CONF.scheduler.max_attempts, len(weighed_hosts))"},{"line_number":132,"context_line":"            alternates \u003d weighed_hosts[0:num_alts]"},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d [chosen_host] + alternates"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_2b552451","line":132,"range":{"start_line":132,"start_character":12,"end_line":132,"end_character":50},"updated":"2017-07-12 13:58:45.000000000","message":"All the alternates must be in the same cell as the chosen_host. Don\u0027t know if you will be adding that later; just wanted to note the need to check cells here.","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":7166,"name":"Sylvain Bauza","email":"sbauza@redhat.com","username":"sbauza"},"change_message_id":"4e9795a8acbb90c0377ff83025e08c9623cccc34","unresolved":false,"context_lines":[{"line_number":129,"context_line":""},{"line_number":130,"context_line":"            # Return the chosen host plus a set of alternate destination hosts"},{"line_number":131,"context_line":"            num_alts \u003d min(CONF.scheduler.max_attempts, len(weighed_hosts))"},{"line_number":132,"context_line":"            alternates \u003d weighed_hosts[0:num_alts]"},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d [chosen_host] + alternates"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_66ffd367","line":132,"range":{"start_line":132,"start_character":12,"end_line":132,"end_character":50},"in_reply_to":"1f1a1f67_2b552451","updated":"2017-07-12 14:21:35.000000000","message":"Yeah, I proposed something about that here previously in a now unnecessary change https://review.openstack.org/#/c/465175/7/nova/scheduler/filter_scheduler.py@141\n\nMaybe that could help.","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"2361b1387c77ca2eb19f7c4699b0ec3aa21f05af","unresolved":false,"context_lines":[{"line_number":129,"context_line":""},{"line_number":130,"context_line":"            # Return the chosen host plus a set of alternate destination hosts"},{"line_number":131,"context_line":"            num_alts \u003d min(CONF.scheduler.max_attempts, len(weighed_hosts))"},{"line_number":132,"context_line":"            alternates \u003d weighed_hosts[0:num_alts]"},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d [chosen_host] + alternates"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_6140dd08","line":132,"range":{"start_line":132,"start_character":12,"end_line":132,"end_character":50},"in_reply_to":"1f1a1f67_2b552451","updated":"2017-07-12 14:41:55.000000000","message":"Yeah, good point, Ed. This is getting a little long already, though, and I still want to add a number of new tests for this added code. OK if I add the cells stuff later in a patch that fully returns alternates from the select_destinations() method?","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"9d17300481cecd0ff8e8a16f3b6d9db4b18f7d30","unresolved":false,"context_lines":[{"line_number":129,"context_line":""},{"line_number":130,"context_line":"            # Return the chosen host plus a set of alternate destination hosts"},{"line_number":131,"context_line":"            num_alts \u003d min(CONF.scheduler.max_attempts, len(weighed_hosts))"},{"line_number":132,"context_line":"            alternates \u003d weighed_hosts[0:num_alts]"},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d [chosen_host] + alternates"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_e1906d98","line":132,"range":{"start_line":132,"start_character":12,"end_line":132,"end_character":50},"in_reply_to":"1f1a1f67_66ffd367","updated":"2017-07-12 14:41:59.000000000","message":"Yeah this is what we need here I think. We could add it later and just say this is broken for multiple cells, but it\u0027s probably worth doing it now so we\u0027re not just adding more cells-incompatible code.","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a5d27254350b4c2946fba2b1efdeee42ceaa1d47","unresolved":false,"context_lines":[{"line_number":129,"context_line":""},{"line_number":130,"context_line":"            # Return the chosen host plus a set of alternate destination hosts"},{"line_number":131,"context_line":"            num_alts \u003d min(CONF.scheduler.max_attempts, len(weighed_hosts))"},{"line_number":132,"context_line":"            alternates \u003d weighed_hosts[0:num_alts]"},{"line_number":133,"context_line":"            hosts_by_instance_uuid[instance_uuid] \u003d [chosen_host] + alternates"},{"line_number":134,"context_line":""},{"line_number":135,"context_line":"            # Now consume the resources so the filter/weights"}],"source_content_type":"text/x-python","patch_set":3,"id":"1f1a1f67_a12dd547","line":132,"range":{"start_line":132,"start_character":12,"end_line":132,"end_character":50},"in_reply_to":"1f1a1f67_e1906d98","updated":"2017-07-12 14:43:21.000000000","message":"OK, fair enough point. I\u0027ll work on adding the cells-limiting stuff here.","commit_id":"28290e45ddb120ccaef61a3280202028c7c85dd4"}],"nova/tests/unit/scheduler/test_filter_scheduler.py":[{"author":{"_account_id":1063,"name":"Ed Leafe","email":"ed@leafe.com","username":"ed-leafe"},"change_message_id":"a5600e7c30a723dd2a248de5ed850605514dd55d","unresolved":false,"context_lines":[{"line_number":325,"context_line":"    @mock.patch.object(filter_scheduler.FilterScheduler, \u0027_schedule\u0027)"},{"line_number":326,"context_line":"    def test_select_destinations_notifications(self, mock_schedule):"},{"line_number":327,"context_line":"        mock_schedule.return_value \u003d {"},{"line_number":328,"context_line":"            uuids.instance: [mock.Mock()],"},{"line_number":329,"context_line":"        }"},{"line_number":330,"context_line":""},{"line_number":331,"context_line":"        with mock.patch.object(self.driver.notifier, \u0027info\u0027) as mock_info:"},{"line_number":332,"context_line":"            expected \u003d {\u0027num_instances\u0027: 1,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_86173a46","line":329,"range":{"start_line":328,"start_character":8,"end_line":329,"end_character":9},"updated":"2017-07-11 16:33:38.000000000","message":"This statement should really just be one line, please. Unless you\u0027re trying to piss off Dan.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"4750faf771535125ad6548c5929314124ed9d3b2","unresolved":false,"context_lines":[{"line_number":325,"context_line":"    @mock.patch.object(filter_scheduler.FilterScheduler, \u0027_schedule\u0027)"},{"line_number":326,"context_line":"    def test_select_destinations_notifications(self, mock_schedule):"},{"line_number":327,"context_line":"        mock_schedule.return_value \u003d {"},{"line_number":328,"context_line":"            uuids.instance: [mock.Mock()],"},{"line_number":329,"context_line":"        }"},{"line_number":330,"context_line":""},{"line_number":331,"context_line":"        with mock.patch.object(self.driver.notifier, \u0027info\u0027) as mock_info:"},{"line_number":332,"context_line":"            expected \u003d {\u0027num_instances\u0027: 1,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3f1d235d_09e0ddf9","line":329,"range":{"start_line":328,"start_character":8,"end_line":329,"end_character":9},"in_reply_to":"3f1d235d_86173a46","updated":"2017-07-11 17:05:05.000000000","message":"It\u0027s a dict, not a function call. Many other places we set dict attributes use a closing parens on a separate line.","commit_id":"0e3f7f67a0569a2fa9c2f2542467c939e25e85cb"}]}