)]}'
{"nova/exception.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"31400ead62dd5a4a0bae254f4e2babbf49a57bf0","unresolved":false,"context_lines":[{"line_number":2152,"context_line":"                \"%(error)s\")"},{"line_number":2153,"context_line":""},{"line_number":2154,"context_line":""},{"line_number":2155,"context_line":"class ResourceProviderSyncFailed(NovaException):"},{"line_number":2156,"context_line":"    msg_fmt \u003d _(\"Failed to synchronize the placement service with resource \""},{"line_number":2157,"context_line":"                \"provider information supplied by the compute host.\")"},{"line_number":2158,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_58bfa9cf","line":2155,"updated":"2018-02-19 15:04:08.000000000","message":"Just looking at this third ResourceProvider*Failed in a row makes me wonder whether we\u0027re either creating too many specific exceptions or maybe we need a parent of some kind here.\n\nAre we going to do something different when we handle each of these cases?","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6918e2f8b63f221f5c066c975dd90f9ad9cc3930","unresolved":false,"context_lines":[{"line_number":2152,"context_line":"                \"%(error)s\")"},{"line_number":2153,"context_line":""},{"line_number":2154,"context_line":""},{"line_number":2155,"context_line":"class ResourceProviderSyncFailed(NovaException):"},{"line_number":2156,"context_line":"    msg_fmt \u003d _(\"Failed to synchronize the placement service with resource \""},{"line_number":2157,"context_line":"                \"provider information supplied by the compute host.\")"},{"line_number":2158,"context_line":""}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_9fdc3075","line":2155,"in_reply_to":"3fa0c359_58bfa9cf","updated":"2018-02-19 19:29:24.000000000","message":"Probably not; I guess they\u0027re really just separated to provide distinct messages, though that could be done when they\u0027re constructed too; and we probably get all the context we need from the stack trace regardless.  But yes, as noted in a TODO in the code, I think a parent (maybe mixin) would be nice.  Not sure what to call it, though.  And its scope may wind up looking a little weird.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"}],"nova/scheduler/client/report.py":[{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"fb8a364b7783f39ebfc4e3becaffa6f4b774f59b","unresolved":false,"context_lines":[{"line_number":1313,"context_line":"        # Do provider deletion first, since it has the best chance of failing"},{"line_number":1314,"context_line":"        # for non-generation-conflict reasons (i.e. allocations)."},{"line_number":1315,"context_line":"        for uuid_to_remove in old_uuids - new_uuids:"},{"line_number":1316,"context_line":"            # TODO(efried): Factor this out into a helper method?"},{"line_number":1317,"context_line":"            resp \u003d self.delete(\u0027/resource_providers/%s\u0027 % uuid_to_remove)"},{"line_number":1318,"context_line":"            if not resp:"},{"line_number":1319,"context_line":"                msg \u003d (\"Failed to delete resource provider with UUID %(uuid)s \""}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_6486fabb","line":1316,"updated":"2018-01-18 14:33:32.000000000","message":"yeah, prolly helpful.","commit_id":"d0057441737c0ecd84b78560e00f35a929f70b04"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"a71b1c1b320cf33e4a16ec05ea43a7d50203402d","unresolved":false,"context_lines":[{"line_number":1337,"context_line":""},{"line_number":1338,"context_line":"        # Now create (or load) any \"new\" providers"},{"line_number":1339,"context_line":"        for uuid_to_add in new_uuids - old_uuids:"},{"line_number":1340,"context_line":"            provider \u003d new_tree.snapshot(uuid_to_add)"},{"line_number":1341,"context_line":"            self._ensure_resource_provider("},{"line_number":1342,"context_line":"                provider.uuid, name\u003dprovider.name,"},{"line_number":1343,"context_line":"                parent_provider_uuid\u003dprovider.parent_uuid)"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_dfe23bd5","line":1340,"range":{"start_line":1340,"start_character":32,"end_line":1340,"end_character":40},"updated":"2018-01-18 14:43:08.000000000","message":"So I see how you\u0027re using that proposed snapshot() method here. I\u0027m not sure it\u0027s a great name for the method.\n\nHow about ProviderTree.data() instead and return a dict instead of an object? That might seem a little c++ stdlib-ish, but it describes exactly what we want, which is just grab the underlying provider data for a provider with the specified UUID.\n\nKeep in mind that new_tree is fully owned by this method scope, so there\u0027s zero danger in other threads modifying new_tree. So calling the method \"snapshot()\" seems odd as it implies a copy is being made or that there are some concerns about concurrent modifications to the data. In this case, there are no such concerns, so I\u0027d prefer to just name the method data() to indicate that the method simply returns the data for the provider...","commit_id":"d0057441737c0ecd84b78560e00f35a929f70b04"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"fb8a364b7783f39ebfc4e3becaffa6f4b774f59b","unresolved":false,"context_lines":[{"line_number":1354,"context_line":"            # terms of cache and error handling."},{"line_number":1355,"context_line":"            self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1356,"context_line":"            self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1357,"context_line":"            self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1358,"context_line":""},{"line_number":1359,"context_line":"    @safe_connect"},{"line_number":1360,"context_line":"    def get_allocations_for_consumer(self, consumer):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_1f442346","line":1357,"updated":"2018-01-18 14:33:32.000000000","message":"so the above implementation looks pretty good. the only thing I would like to see added is a generation check for all delete and update operations.\n\nFor example, for deletes, we could do something like this:\n\n to_delete \u003d old_uuids - new_uuids\n for uuid_to_remove in to_delete:\n     old_gen \u003d self._provider_tree.provider_generation(old_uuid)\n     new_gen \u003d new_tree.provider_generation(old_uuid)\n     if old_gen \u003e new_gen:\n         LOG.warn(\"Attempting to delete provider %s but another \"\n                  \"thread updated. Skipping delete.\", uuid_to_remove)\n         continue\n     self._delete_provider(uuid_to_remove)","commit_id":"d0057441737c0ecd84b78560e00f35a929f70b04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6914ec1b57c2ccffbedf13c80981b565299ea994","unresolved":false,"context_lines":[{"line_number":1354,"context_line":"            # terms of cache and error handling."},{"line_number":1355,"context_line":"            self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1356,"context_line":"            self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1357,"context_line":"            self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1358,"context_line":""},{"line_number":1359,"context_line":"    @safe_connect"},{"line_number":1360,"context_line":"    def get_allocations_for_consumer(self, consumer):"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_b1bd23b8","line":1357,"in_reply_to":"7f96bb07_1f442346","updated":"2018-01-18 15:19:52.000000000","message":"My thought was that, if we get a generation conflict at *any* point in this process, we bail out completely, resync our local cache, and then let the next iteration from resource tracker redrive the whole shebang.","commit_id":"d0057441737c0ecd84b78560e00f35a929f70b04"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"3a4bd02119a9253b4cbdbeb2254ee85ec1f2590d","unresolved":false,"context_lines":[{"line_number":1355,"context_line":"            self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1356,"context_line":"            self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1357,"context_line":"            self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1358,"context_line":""},{"line_number":1359,"context_line":"    @safe_connect"},{"line_number":1360,"context_line":"    def get_allocations_for_consumer(self, consumer):"},{"line_number":1361,"context_line":"        url \u003d \u0027/allocations/%s\u0027 % consumer"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_a8c73e96","line":1358,"updated":"2018-01-16 19:04:17.000000000","message":"There should be no need to walk the descendants explicitly here, right?  I\u0027m covering all the providers by virtue of having gotten all the UUIDs; it doesn\u0027t matter if they\u0027re not done in order.","commit_id":"d0057441737c0ecd84b78560e00f35a929f70b04"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"fb8a364b7783f39ebfc4e3becaffa6f4b774f59b","unresolved":false,"context_lines":[{"line_number":1355,"context_line":"            self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1356,"context_line":"            self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1357,"context_line":"            self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1358,"context_line":""},{"line_number":1359,"context_line":"    @safe_connect"},{"line_number":1360,"context_line":"    def get_allocations_for_consumer(self, consumer):"},{"line_number":1361,"context_line":"        url \u003d \u0027/allocations/%s\u0027 % consumer"}],"source_content_type":"text/x-python","patch_set":3,"id":"7f96bb07_9fa6f30e","line":1358,"in_reply_to":"7f96bb07_a8c73e96","updated":"2018-01-18 14:33:32.000000000","message":"correct.","commit_id":"d0057441737c0ecd84b78560e00f35a929f70b04"},{"author":{"_account_id":9172,"name":"Radoslav Gerganov","email":"rgerganov@vmware.com","username":"rgerganov"},"change_message_id":"0b16e256bb04396669a7c692b767c6aa5305b235","unresolved":false,"context_lines":[{"line_number":1396,"context_line":"                # terms of cache and error handling."},{"line_number":1397,"context_line":"                self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1398,"context_line":"                self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1399,"context_line":"                self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1400,"context_line":"            except exception.PlacementAPIConflict:"},{"line_number":1401,"context_line":"                # The failing method already logged."},{"line_number":1402,"context_line":"                cache_invalidate(uuid)"}],"source_content_type":"text/x-python","patch_set":6,"id":"7f96bb07_ca49c932","line":1399,"updated":"2018-01-22 12:21:47.000000000","message":"if there are no aggregates (i.e. newp.aggregates is None), placement will throw an error here","commit_id":"3664074d13ccc4867d830847b2bea868ec47c755"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"5d6c58e96d1010f2e9eb64dfdf369717ca59b1d6","unresolved":false,"context_lines":[{"line_number":1396,"context_line":"                # terms of cache and error handling."},{"line_number":1397,"context_line":"                self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1398,"context_line":"                self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1399,"context_line":"                self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1400,"context_line":"            except exception.PlacementAPIConflict:"},{"line_number":1401,"context_line":"                # The failing method already logged."},{"line_number":1402,"context_line":"                cache_invalidate(uuid)"}],"source_content_type":"text/x-python","patch_set":6,"id":"7f96bb07_5cb57ded","line":1399,"in_reply_to":"7f96bb07_86b41537","updated":"2018-01-22 18:46:38.000000000","message":"Checked through the code, and indeed it can only be some form of a set() and set_aggregates_for_provider is okay with that.  It would also be okay with None because of aggregates \u003d aggregates or [] in the called method.","commit_id":"3664074d13ccc4867d830847b2bea868ec47c755"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"a4adfc61d42fa428615e5ddd24a4f6d92fa17522","unresolved":false,"context_lines":[{"line_number":1396,"context_line":"                # terms of cache and error handling."},{"line_number":1397,"context_line":"                self._update_inventory_attempt(uuid, newp.inventory)"},{"line_number":1398,"context_line":"                self.set_traits_for_provider(uuid, newp.traits)"},{"line_number":1399,"context_line":"                self.set_aggregates_for_provider(uuid, newp.aggregates)"},{"line_number":1400,"context_line":"            except exception.PlacementAPIConflict:"},{"line_number":1401,"context_line":"                # The failing method already logged."},{"line_number":1402,"context_line":"                cache_invalidate(uuid)"}],"source_content_type":"text/x-python","patch_set":6,"id":"7f96bb07_86b41537","line":1399,"in_reply_to":"7f96bb07_ca49c932","updated":"2018-01-22 18:27:58.000000000","message":"\"No aggregates\" would be newp.aggregates \u003d\u003d set(), which should be fine here and behave correctly.  It should be impossible to get None into newp.aggregates, .traits, or .inventory (without accessing private stuff).  Am I missing that code path?","commit_id":"3664074d13ccc4867d830847b2bea868ec47c755"},{"author":{"_account_id":9172,"name":"Radoslav Gerganov","email":"rgerganov@vmware.com","username":"rgerganov"},"change_message_id":"f7a314cc7c64a00b9e26d120b4ea01f9e67515f9","unresolved":false,"context_lines":[{"line_number":1378,"context_line":"                s.success \u003d False"},{"line_number":1379,"context_line":"                # Invalidate the caches"},{"line_number":1380,"context_line":"                try:"},{"line_number":1381,"context_line":"                    self._provider_tree.remove(rp_uuid)"},{"line_number":1382,"context_line":"                except ValueError:"},{"line_number":1383,"context_line":"                    pass"},{"line_number":1384,"context_line":"                self.association_refresh_time.pop(rp_uuid, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7f96bb07_1de1ef9b","line":1381,"updated":"2018-01-24 11:42:41.000000000","message":"Silently removing parts of the tree here can have very bad implications. When we do this, we basically invalidate the local cached tree and we may fail to sync later on. Consider the following example which I had very hard time to debug:\n1. virt driver creates a nested RP and sets some traits on it\n2. this method creates the nested RP (line 1417) but fails to set the trait (line 1448); the nested RP is removed from the local tree (line 1381)\n3. now the local tree contains only the root RP (no errors logged/raised!)\n4. the next time virt driver returns the nested RP, we try to create it again (because it is missing locally) and we fail because it already exists in placement\n\nGiven that we use the local tree as a baseline when making changes to placement, I think that refreshing it every time before asking the driver for changes (https://review.openstack.org/#/c/536902) will make our lives easier.","commit_id":"03e913645ad332ca5b263ebae899a1d4d79797bc"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d598554ff48974e0a9d173d43c57f0a42b893b5f","unresolved":false,"context_lines":[{"line_number":1378,"context_line":"                s.success \u003d False"},{"line_number":1379,"context_line":"                # Invalidate the caches"},{"line_number":1380,"context_line":"                try:"},{"line_number":1381,"context_line":"                    self._provider_tree.remove(rp_uuid)"},{"line_number":1382,"context_line":"                except ValueError:"},{"line_number":1383,"context_line":"                    pass"},{"line_number":1384,"context_line":"                self.association_refresh_time.pop(rp_uuid, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7f96bb07_554e6b38","line":1381,"in_reply_to":"7f96bb07_1de1ef9b","updated":"2018-01-24 13:18:35.000000000","message":"As discussed [1], I suspect the problem may actually be that we\u0027re leaving stale association_refresh_time entries for the descendants, so they don\u0027t get restored in the recursive part of _refresh_associations on the next walk through.  This will be made moot by [2], but in the meantime, to prove that\u0027s the issue, we can try changing this section to:\n\n try:\n     uuids_to_invalidate \u003d self._provider_tree.get_provider_uuids(rp_uuid)\n     self._provider_tree.remove(rp_uuid)\n except ValueError:\n     pass\n for uuid in uuids_to_invalidate:\n     self.association_refresh_time.pop(uuid, None)\n\n[1] http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2018-01-24.log.html#t2018-01-24T12:58:43\n[2] https://review.openstack.org/#/c/535517/","commit_id":"03e913645ad332ca5b263ebae899a1d4d79797bc"},{"author":{"_account_id":9172,"name":"Radoslav Gerganov","email":"rgerganov@vmware.com","username":"rgerganov"},"change_message_id":"28cfbe62e5c62ed104d11a60a379d8e4ee08bbb7","unresolved":false,"context_lines":[{"line_number":1378,"context_line":"                s.success \u003d False"},{"line_number":1379,"context_line":"                # Invalidate the caches"},{"line_number":1380,"context_line":"                try:"},{"line_number":1381,"context_line":"                    self._provider_tree.remove(rp_uuid)"},{"line_number":1382,"context_line":"                except ValueError:"},{"line_number":1383,"context_line":"                    pass"},{"line_number":1384,"context_line":"                self.association_refresh_time.pop(rp_uuid, None)"}],"source_content_type":"text/x-python","patch_set":7,"id":"7f96bb07_6ec6349d","line":1381,"in_reply_to":"7f96bb07_554e6b38","updated":"2018-01-24 15:05:37.000000000","message":"I have written a func test for this scenario here: https://review.openstack.org/#/c/537406","commit_id":"03e913645ad332ca5b263ebae899a1d4d79797bc"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7c4e593a3287f2c9257d4b99b0384bba0ff9001a","unresolved":false,"context_lines":[{"line_number":299,"context_line":"        # more sensitive to this than other APIs in the OpenStack"},{"line_number":300,"context_line":"        # ecosystem."},{"line_number":301,"context_line":"        kwargs \u003d {\u0027microversion\u0027: version}"},{"line_number":302,"context_line":"        if data is not None:"},{"line_number":303,"context_line":"            kwargs[\u0027json\u0027] \u003d data"},{"line_number":304,"context_line":"        return self._client.put(url, raise_exc\u003dFalse, **kwargs)"},{"line_number":305,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5f93b717_eb187076","line":302,"updated":"2018-01-25 00:08:58.000000000","message":"This is the fix from [1], which has been sitting in the gate for a few days.  Consider using a Depends-On instead?\n\n[1] https://review.openstack.org/#/c/536545/","commit_id":"d69b476dfaa3c549f8c9fc770ad387ec929f6f11"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7c4e593a3287f2c9257d4b99b0384bba0ff9001a","unresolved":false,"context_lines":[{"line_number":656,"context_line":"        # At this point, the whole tree exists in the local cache."},{"line_number":657,"context_line":""},{"line_number":658,"context_line":"        for rp_to_refresh in rps_to_refresh:"},{"line_number":659,"context_line":"            self._refresh_and_get_inventory(rp_to_refresh[\u0027uuid\u0027])"},{"line_number":660,"context_line":"            self._refresh_associations("},{"line_number":661,"context_line":"                rp_to_refresh[\u0027uuid\u0027],"},{"line_number":662,"context_line":"                generation\u003drp_to_refresh.get(\u0027generation\u0027), force\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5f93b717_8bfe8c03","line":659,"updated":"2018-01-25 00:08:58.000000000","message":"I need to explain this.","commit_id":"d69b476dfaa3c549f8c9fc770ad387ec929f6f11"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"31400ead62dd5a4a0bae254f4e2babbf49a57bf0","unresolved":false,"context_lines":[{"line_number":678,"context_line":""},{"line_number":679,"context_line":"        for rp_to_refresh in rps_to_refresh:"},{"line_number":680,"context_line":"            # NOTE(efried): _refresh_associations doesn\u0027t refresh inventory"},{"line_number":681,"context_line":"            # (yet) - see that method\u0027s docstring for the why."},{"line_number":682,"context_line":"            self._refresh_and_get_inventory(context, rp_to_refresh[\u0027uuid\u0027])"},{"line_number":683,"context_line":"            self._refresh_associations("},{"line_number":684,"context_line":"                context, rp_to_refresh[\u0027uuid\u0027],"}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_5809e992","line":681,"updated":"2018-02-19 15:04:08.000000000","message":"with a name like that, one would hope it never does?","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6918e2f8b63f221f5c066c975dd90f9ad9cc3930","unresolved":false,"context_lines":[{"line_number":678,"context_line":""},{"line_number":679,"context_line":"        for rp_to_refresh in rps_to_refresh:"},{"line_number":680,"context_line":"            # NOTE(efried): _refresh_associations doesn\u0027t refresh inventory"},{"line_number":681,"context_line":"            # (yet) - see that method\u0027s docstring for the why."},{"line_number":682,"context_line":"            self._refresh_and_get_inventory(context, rp_to_refresh[\u0027uuid\u0027])"},{"line_number":683,"context_line":"            self._refresh_associations("},{"line_number":684,"context_line":"                context, rp_to_refresh[\u0027uuid\u0027],"}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_df2158a1","line":681,"in_reply_to":"3fa0c359_5809e992","updated":"2018-02-19 19:29:24.000000000","message":"Which, _refresh_associations?  Perhaps poorly named, then, because in the long run I hope _refresh_associations(rp) will refresh all the information associated with rp.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"31400ead62dd5a4a0bae254f4e2babbf49a57bf0","unresolved":false,"context_lines":[{"line_number":1369,"context_line":"            \"\"\""},{"line_number":1370,"context_line":"            class Status(object):"},{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_b8558d52","line":1372,"updated":"2018-02-19 15:04:08.000000000","message":"what\u0027s an \u0027s\u0027?","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6918e2f8b63f221f5c066c975dd90f9ad9cc3930","unresolved":false,"context_lines":[{"line_number":1369,"context_line":"            \"\"\""},{"line_number":1370,"context_line":"            class Status(object):"},{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_b2728448","line":1372,"in_reply_to":"3fa0c359_b8558d52","updated":"2018-02-19 19:29:24.000000000","message":"Would you like me to spell out `status`?","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"31400ead62dd5a4a0bae254f4e2babbf49a57bf0","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_5817a9a4","line":1374,"updated":"2018-02-19 15:04:08.000000000","message":"Yes, do that.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"8e9c0f59697534eb3c6b75aae1e7999819d74e30","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_df3e93b8","line":1374,"in_reply_to":"3fa0c359_3f02bc0a","updated":"2018-03-16 21:45:22.000000000","message":"Thinking through this some more, I don\u0027t think it makes sense for these to be subclasses of a parent exception that\u0027s specific to prompting cache invalidation - because they could surely be used in other contexts where that\u0027s not a thing.  I could possibly see a really generic PlacementException as a parent...\n\nIn any case, I think the waters here are sufficiently muddy to warrant deferring that to a separate patch.  Agree?","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6918e2f8b63f221f5c066c975dd90f9ad9cc3930","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_3f02bc0a","line":1374,"in_reply_to":"3fa0c359_5817a9a4","updated":"2018-02-19 19:29:24.000000000","message":"Any recommendations for what it should be called?\n\nCacheInvalidatingPlacementException?","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"1eff870638447619dde8610853b46018b2592887","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_257219e5","line":1374,"in_reply_to":"df7087c5_02b02b8b","updated":"2018-03-17 15:08:22.000000000","message":"\u003e If we get an error like a ResourceProviderUpdateConflict in update\n \u003e A the resource provider wrapping the request that called that is\n \u003e removed from the local version of that providers tree (along with\n \u003e all its children). This truncated tree will get passed to the\n \u003e virtdriver next time around (in update B) the provider loop and the\n \u003e virt driver will say \"things missing, please add x, y, and z\".\n\nIt *shouldn\u0027t* happen this way.  The next time through, before we call update_provider_tree, we\u0027ll be calling get_provider_tree_and_ensure_root, which (eventually) calls _refresh_associations, which *should* be restoring the invalidated providers to the cache.  I can see an exception to this if upt previously added a provider that\u0027s not associated via nesting or aggregate with anything else - but a) I don\u0027t think it should do that; and b) if it did, upt\u0027s reaction to that (presumably to re-add that provider) would be okay.\n\n \u003e Does this mean that between A and B there is a potential\n \u003e misrepresentation of resource providers on the placement side for a\n \u003e space of time (we added or remove some but not all things)?\n\nYes, if an error occurs in this method - whether we choose best-effort or first-failure - the placement database will be out of sync with reality for some time.\n\n \u003e If so, is that bad?\n\nNo idea.  Though short of trying to implement some kind of transactional behavior (rollbacks - ugh - and what happens if the rollback fails?) I don\u0027t see a way around it.\n\nOff the top, it seems like the worst result would be incorrect scheduling attempts resulting in failures (and intra-cell retries) when the actual allocations are attempted, similar to if async allocations are made between GET /ac and PUT /allocations.\n\n \u003e More generally (that is without regard to however code is doing\n \u003e what it is doing): What does it mean when a nested resource\n \u003e provider managing client fails to create a correct representation\n \u003e of its complex resources to the placement service? Should it:\n\nSomewhere between this:\n\n \u003e * not care, we\u0027ll fix it soon\n\nand this:\n\n \u003e * we\u0027ll cross that bridge later\n\n \u003e Maybe it\u0027s not a concern, but I figure it is better to think out\n \u003e loud.\n\nYup, totally agree.  Talking it through, I\u0027m pretty convinced that we can KISS and it\u0027ll be okay.  Trying to make it airtight would result in an order of magnitude more and more complicated code - which would *still* have holes that are simply impossible to close.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"de13b057e69ab69336a2f7467db09cd40fa82785","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_02b02b8b","line":1374,"in_reply_to":"df7087c5_08cb6f72","updated":"2018-03-17 11:09:04.000000000","message":"\u003e I can see your point here.  I again go back to the fact that this\n \u003e grouping of exceptions is really only applicable to this one\n \u003e method, so it feels wrong to have it defined in any other scope.\n \u003e \n \u003e \u003e If catch_all were defined outside of its containing method and\n \u003e \u003e didnt have that long list I\u0027d probably be moaning a lot less.\n \u003e \n \u003e Ditto above.\n\nProbably not much point in arguing about pythonic style all day, so I\u0027ll just let these go.\n\n\nThanks for the detailed explanation of what the code is doing, but that wasn\u0027t really the thrust of my (probably poorly expressed because tired) query. What I\u0027m trying to understand is the implications of\n\n \u003e As currently\n \u003e written, that failure bubbles up in the RT and results in a log\n \u003e message, but not much else.\n\nand the relative safety thereof. Basically by having a conversation about it.\n\nIf we get an error like a ResourceProviderUpdateConflict in update A the resource provider wrapping the request that called that is removed from the local version of that providers tree (along with all its children). This truncated tree will get passed to the virtdriver next time around (in update B) the provider loop and the virt driver will say \"things missing, please add x, y, and z\".\n\nDoes this mean that between A and B there is a potential misrepresentation of resource providers on the placement side for a space of time (we added or remove some but not all things)? If so, is that bad?\n\nMore generally (that is without regard to however code is doing what it is doing): What does it mean when a nested resource provider managing client fails to create a correct representation of its complex resources to the placement service? Should it:\n\n* not care, we\u0027ll fix it soon\n* notice that things have gone wrong and attempt to truncate all parts of the tree that it owns so that the resources are not inappropriately used or represented by either placement or nova-scheduler\n* we\u0027ll cross that bridge later\n* not worry, networks and services and robust and reliable\n* something else\n\nMaybe it\u0027s not a concern, but I figure it is better to think out loud.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"42a384556c0901ff7b874bc90dabbc54524cb31d","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_ed108d52","line":1374,"in_reply_to":"df7087c5_42f628fc","updated":"2018-03-16 23:29:23.000000000","message":"\u003e Yeah, the thing is that it\u0027s going to be totally specific to this\n \u003e code path, so a base exception doesn\u0027t seem appropriate.  And\n \u003e putting the list anywhere else would just make it harder to read\n \u003e IMO.\n\nI guess that\u0027s a way we differ. I want methods to be short. If there\u0027s a static lump of exceptions that is effectively in the way but is THE_EXCEPTIONS_I_SOMETIMES_CARE_ABOUT_HERE, then I want that symbol, not the exceptions, for the sake of short. Most of the time I don\u0027t actually care about what the exceptions are.\n\nBut this isn\u0027t something I\u0027d -1 on, at least not in this file.\n\n \u003e I\u0027ll start by saying that, IF you accept that we want to do a\n \u003e best-effort algorithm, this context manager was the tidiest way I\n \u003e found (after quite a bit of experimentation) to make it clear and\n \u003e readable.\n\nIf catch_all were defined outside of its containing method and didnt have that long list I\u0027d probably be moaning a lot less.\n\n \u003e Now, I\u0027m totally willing to entertain the thought of switching to\n \u003e more of a first-failure algorithm.  I guess I\u0027d just be worried\n \u003e about how much we\u0027re deferring the final resolution of the whole\n \u003e picture if we have to wait for the next period to retry each\n \u003e failure.  The other thing that concerns me is that, if there\u0027s one\n \u003e piece in here that\u0027s semi-permanently broken, it\u0027d cause some\n \u003e unpredictable subset of the rest of the pieces to remain out of\n \u003e sync.  Whereas with best-effort, only that one broken piece will be\n \u003e broken (and the log will say which one it is, once per period).\n\nI may be misunderstanding the context of this, my comment was based on making the assumption that if we have a failure here we\u0027re going to fail the entire ProviderTree and resync from scratch on the next loop round the periodic job. If that\u0027s the case, then I can\u0027t understand why we wouldn\u0027t want to fail fast.\n\nIf that\u0027s _not_ the case, what is going to happen?\n\n \u003e I\u0027d like to hear what others think.\n\nIndeed","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"c48943a6100a2421ca442e3465e8d5822994a2c4","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_df6a131a","line":1374,"in_reply_to":"df7087c5_df3e93b8","updated":"2018-03-16 22:09:07.000000000","message":"sure, if you like, I don\u0027t have a strong preference on how it\u0027s done. My concern was twofold:\n\n* this is a long list of exceptions and that\u0027s  a bit wordy, either a base exception or a constant list kept elsewhere would make this more readable\n* looking at things again now, doing the exception handling with a context manager this way is interesting but doesn\u0027t feel particularly idiomatic for python, nor, to me does it follow the principle of least surprise: if there is an exception, you still continue doing the other operations and only at the end to do you a sync failed raise. Is that wise? If we\u0027re going to revalidate the whole tree on sync failure (are we?) then isn\u0027t it best to fail early?\n\nIt\u0027s late for me and I\u0027m very tired so I may be talking crazy.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"070845fd5a2e21f65bc77c9c79dd94b8db841c9e","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_42f628fc","line":1374,"in_reply_to":"df7087c5_df6a131a","updated":"2018-03-16 22:43:59.000000000","message":"\u003e * this is a long list of exceptions and that\u0027s  a bit wordy, either\n \u003e a base exception or a constant list kept elsewhere would make this\n \u003e more readable\n\nYeah, the thing is that it\u0027s going to be totally specific to this code path, so a base exception doesn\u0027t seem appropriate.  And putting the list anywhere else would just make it harder to read IMO.\n\n \u003e * looking at things again now, doing the exception handling with a\n \u003e context manager this way is interesting but doesn\u0027t feel\n \u003e particularly idiomatic for python, nor, to me does it follow the\n \u003e principle of least surprise: if there is an exception, you still\n \u003e continue doing the other operations and only at the end to do you a\n \u003e sync failed raise. Is that wise? If we\u0027re going to revalidate the\n \u003e whole tree on sync failure (are we?) then isn\u0027t it best to fail\n \u003e early?\n\nI\u0027ll start by saying that, IF you accept that we want to do a best-effort algorithm, this context manager was the tidiest way I found (after quite a bit of experimentation) to make it clear and readable.\n\nNow, I\u0027m totally willing to entertain the thought of switching to more of a first-failure algorithm.  I guess I\u0027d just be worried about how much we\u0027re deferring the final resolution of the whole picture if we have to wait for the next period to retry each failure.  The other thing that concerns me is that, if there\u0027s one piece in here that\u0027s semi-permanently broken, it\u0027d cause some unpredictable subset of the rest of the pieces to remain out of sync.  Whereas with best-effort, only that one broken piece will be broken (and the log will say which one it is, once per period).\n\nI\u0027d like to hear what others think.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"011775785f0f1a491e208bab5ef51d122a772f3c","unresolved":false,"context_lines":[{"line_number":1371,"context_line":"                success \u003d True"},{"line_number":1372,"context_line":"            s \u003d Status()"},{"line_number":1373,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1374,"context_line":"            # can inherit."},{"line_number":1375,"context_line":"            helper_exceptions \u003d ("},{"line_number":1376,"context_line":"                exception.InvalidResourceClass,"},{"line_number":1377,"context_line":"                exception.InventoryInUse,"}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_08cb6f72","line":1374,"in_reply_to":"df7087c5_ed108d52","updated":"2018-03-17 01:55:18.000000000","message":"\u003e I guess that\u0027s a way we differ. I want methods to be short. If\n \u003e there\u0027s a static lump of exceptions that is effectively in the way\n \u003e but is THE_EXCEPTIONS_I_SOMETIMES_CARE_ABOUT_HERE, then I want that\n \u003e symbol, not the exceptions, for the sake of short. Most of the time\n \u003e I don\u0027t actually care about what the exceptions are.\n\nI can see your point here.  I again go back to the fact that this grouping of exceptions is really only applicable to this one method, so it feels wrong to have it defined in any other scope.\n\n \u003e If catch_all were defined outside of its containing method and\n \u003e didnt have that long list I\u0027d probably be moaning a lot less.\n\nDitto above.\n\n \u003e I may be misunderstanding the context of this, my comment was based\n \u003e on making the assumption that if we have a failure here we\u0027re going\n \u003e to fail the entire ProviderTree and resync from scratch on the next\n \u003e loop round the periodic job. If that\u0027s the case, then I can\u0027t\n \u003e understand why we wouldn\u0027t want to fail fast.\n \u003e \n \u003e If that\u0027s _not_ the case, what is going to happen?\n\nOh, yeah, that\u0027s totally important to understand.  As the docstring says, this is *not* atomic/transactional.  So if as a result of this method we would need to do 10 different placement calls, as currently written we\u0027ll do 10 placement calls, even if two of them fail.  The successful eight will update the placement database for their respective providers.  The failing two will invalidate the cache for theirs (so next time around we\u0027ll re-GET \u0027em), and trigger the overall method to report a failure.  As currently written, that failure bubbles up in the RT and results in a log message, but not much else.  But the important point to note is that the next resync (assuming nothing else has changed) would only detect those two previously-failing bits as still being out of sync, and make those calls.\n\nContrasting with a first-failure algorithm, whereby we would bail out of this method at the *first* of those two failures, meaning that some unknown/undefined/indeterminate number N of the 10 placement calls actually got invoked, and N-1 of them succeeded.  Again, the failure would bubble up in the RT and result in logging-but-not-much-else; and the next time around, we would probably be trying that previously-failing call first.  If it succeeds, we would proceed with the remaining 10-N calls until that second one failed (which maybe it wouldn\u0027t this time around, who knows) at which point we may *still* have some indeterminate number of the original 10 untried.\n\nTL;DR: The way it\u0027s written, only the failing calls are left on the table, as opposed to the failing calls plus \"some others\".\n\nAgain, I\u0027m willing to entertain the idea that that\u0027s not The Right Thing.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7dd1427c433b233a86bc98c875c1cf7893ac8597","unresolved":false,"context_lines":[{"line_number":1402,"context_line":"        # Helper methods herein will be updating the local cache (this is"},{"line_number":1403,"context_line":"        # intentional) so we need to grab up front any data we need to operate"},{"line_number":1404,"context_line":"        # on in its \"original\" form."},{"line_number":1405,"context_line":"        old_tree \u003d self._provider_tree"},{"line_number":1406,"context_line":"        old_uuids \u003d old_tree.get_provider_uuids()"},{"line_number":1407,"context_line":"        new_uuids \u003d new_tree.get_provider_uuids()"},{"line_number":1408,"context_line":""},{"line_number":1409,"context_line":"        # Do provider deletion first, since it has the best chance of failing"},{"line_number":1410,"context_line":"        # for non-generation-conflict reasons (i.e. allocations)."}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_53d69f29","line":1407,"range":{"start_line":1405,"start_character":0,"end_line":1407,"end_character":49},"updated":"2018-02-19 22:03:26.000000000","message":"Consider the ironic case.  What we\u0027re passing to update_provider_tree right now may include *one or more* of the full set of ironic nodes (consider the first run, before we\u0027ve cached \u0027em all).  It\u0027s also conceivable [1] that the cache changes between update_provider_tree and update_from_provider_tree in a way that would end up causing us to delete other nodes here.\n\nShould we limit what we pass to update_provider_tree to just the specific node (and associated sharing providers)?  Or should we limit this code to operating only thereon?\n\n[1] Okay, probably not now considering the big lock and linearity of the RT algorithm, but eventually...","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"8e9c0f59697534eb3c6b75aae1e7999819d74e30","unresolved":false,"context_lines":[{"line_number":1402,"context_line":"        # Helper methods herein will be updating the local cache (this is"},{"line_number":1403,"context_line":"        # intentional) so we need to grab up front any data we need to operate"},{"line_number":1404,"context_line":"        # on in its \"original\" form."},{"line_number":1405,"context_line":"        old_tree \u003d self._provider_tree"},{"line_number":1406,"context_line":"        old_uuids \u003d old_tree.get_provider_uuids()"},{"line_number":1407,"context_line":"        new_uuids \u003d new_tree.get_provider_uuids()"},{"line_number":1408,"context_line":""},{"line_number":1409,"context_line":"        # Do provider deletion first, since it has the best chance of failing"},{"line_number":1410,"context_line":"        # for non-generation-conflict reasons (i.e. allocations)."}],"source_content_type":"text/x-python","patch_set":22,"id":"df7087c5_1f264b6a","line":1407,"range":{"start_line":1405,"start_character":0,"end_line":1407,"end_character":49},"in_reply_to":"3fa0c359_53d69f29","updated":"2018-03-16 21:45:22.000000000","message":"Having thought this through further, I think I\u0027m okay with it as it stands.  The stipulation is that we\u0027re relying completely on the COMPUTE_RESOURCE_SEMAPHORE in the resource tracker.  Since it\u0027s held across the calls to get_provider_tree_and_ensure_root, update_provider_tree, and update_from_provider_tree, we can be assured that the picture of the overall set of ironic node RPs won\u0027t change.  And with the caveats in the spec, docstring, and devref, update_provider_tree shouldn\u0027t muck with the other compute node RPs that\u0027ll appear in the provider tree.\n\nTo be clear, we\u0027re saying that, in the general case, the provider_tree passed to update_provider_tree will contain the RPs associated with all the compute nodes under the purview of this host.  For everything other than ironic, that\u0027s one.  For ironic, that can be more than one.\n\nI should clarify that in the spec...","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"3595faa98400be757783d4182f231edd2b594c00","unresolved":false,"context_lines":[{"line_number":1405,"context_line":"            True or False.  Saves having to do try/except for every helper call"},{"line_number":1406,"context_line":"            below."},{"line_number":1407,"context_line":"            \"\"\""},{"line_number":1408,"context_line":"            class Status(object):"},{"line_number":1409,"context_line":"                success \u003d True"},{"line_number":1410,"context_line":"            s \u003d Status()"},{"line_number":1411,"context_line":"            # TODO(efried): Make a base exception class from which all these"},{"line_number":1412,"context_line":"            # can inherit."}],"source_content_type":"text/x-python","patch_set":25,"id":"df7087c5_7886fdbb","line":1409,"range":{"start_line":1408,"start_character":12,"end_line":1409,"end_character":30},"updated":"2018-03-22 15:21:08.000000000","message":"I like this technique you\u0027ve used here. Nice work.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"b16e5de2fbcaca491df8585ecaad7d5b7ac584f9","unresolved":false,"context_lines":[{"line_number":1432,"context_line":"                    self._provider_tree.remove(rp_uuid)"},{"line_number":1433,"context_line":"                except ValueError:"},{"line_number":1434,"context_line":"                    pass"},{"line_number":1435,"context_line":"                self.association_refresh_time.pop(rp_uuid, None)"},{"line_number":1436,"context_line":""},{"line_number":1437,"context_line":"        # Overall indicator of success.  Will be set to False on any exception."},{"line_number":1438,"context_line":"        success \u003d True"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_55f089fe","line":1435,"updated":"2018-03-29 17:02:44.000000000","message":"I\u0027m not seeing test coverage of the catch_call functionality in this patch and also only saw one case of a test that checks for ResourceProviderSyncFailed in the next patch.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f4822372fdddace2cb3648f8af724cf77dfa0654","unresolved":false,"context_lines":[{"line_number":1432,"context_line":"                    self._provider_tree.remove(rp_uuid)"},{"line_number":1433,"context_line":"                except ValueError:"},{"line_number":1434,"context_line":"                    pass"},{"line_number":1435,"context_line":"                self.association_refresh_time.pop(rp_uuid, None)"},{"line_number":1436,"context_line":""},{"line_number":1437,"context_line":"        # Overall indicator of success.  Will be set to False on any exception."},{"line_number":1438,"context_line":"        success \u003d True"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_41e09dcd","line":1435,"in_reply_to":"bf659307_3f0fdd37","updated":"2018-04-02 20:19:22.000000000","message":"I was definitely missing how certain individual exceptions were being raised in the functional tests. Thanks for highlighting those in the other file.\n\nHere, what I was imagining was maybe a @ddt decorated test that raises each exception and then asserts that check that the RP is removed from the provider tree and the association_refresh_time is updated as expected in the event of any of these exceptions. And that it yields the expected s.success \u003d False.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e5c9c671e39ecc82da9c0bdb752c3ce8cc308b98","unresolved":false,"context_lines":[{"line_number":1432,"context_line":"                    self._provider_tree.remove(rp_uuid)"},{"line_number":1433,"context_line":"                except ValueError:"},{"line_number":1434,"context_line":"                    pass"},{"line_number":1435,"context_line":"                self.association_refresh_time.pop(rp_uuid, None)"},{"line_number":1436,"context_line":""},{"line_number":1437,"context_line":"        # Overall indicator of success.  Will be set to False on any exception."},{"line_number":1438,"context_line":"        success \u003d True"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_3f0fdd37","line":1435,"in_reply_to":"bf659307_55f089fe","updated":"2018-03-30 19:02:39.000000000","message":"There are three pieces of functional test in this patch (starting around [1]) that trigger failures funneling through catch_all, verified by checking for ResourceProviderSyncFailed each time.  I marked \u0027em up in that file.\n\nIs there more you\u0027d like to see?\n\n[1] https://review.openstack.org/#/c/533821/25/nova/tests/functional/api/openstack/placement/test_report_client.py@794","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":20722,"name":"Yikun Jiang","email":"yikunkero@gmail.com","username":"yikunkero"},"change_message_id":"bb8df7ce4aaa04e69fa71326d16dfe15fd01f7a4","unresolved":false,"context_lines":[{"line_number":1490,"context_line":"            success \u003d success and status.success"},{"line_number":1491,"context_line":""},{"line_number":1492,"context_line":"        if not success:"},{"line_number":1493,"context_line":"            raise exception.ResourceProviderSyncFailed()"},{"line_number":1494,"context_line":""},{"line_number":1495,"context_line":"    @safe_connect"},{"line_number":1496,"context_line":"    def get_allocations_for_consumer(self, context, consumer):"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_7e61e5a8","line":1493,"range":{"start_line":1493,"start_character":12,"end_line":1493,"end_character":56},"updated":"2018-03-30 03:24:28.000000000","message":"I\u0027m not sure should we add some detail message in here to show which branch is failed.\n\nBecause the user or dev only knows sync failed happend, but don\u0027t know the detail info.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e5c9c671e39ecc82da9c0bdb752c3ce8cc308b98","unresolved":false,"context_lines":[{"line_number":1490,"context_line":"            success \u003d success and status.success"},{"line_number":1491,"context_line":""},{"line_number":1492,"context_line":"        if not success:"},{"line_number":1493,"context_line":"            raise exception.ResourceProviderSyncFailed()"},{"line_number":1494,"context_line":""},{"line_number":1495,"context_line":"    @safe_connect"},{"line_number":1496,"context_line":"    def get_allocations_for_consumer(self, context, consumer):"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_3f8a7dbe","line":1493,"range":{"start_line":1493,"start_character":12,"end_line":1493,"end_character":56},"in_reply_to":"bf659307_7e61e5a8","updated":"2018-03-30 19:02:39.000000000","message":"If we get here:\n\n- We experienced at least one failure, but possibly more than one;\n- We don\u0027t know (weren\u0027t keeping track of) which failures they were;\n- That\u0027s okay because the code paths that generate the original exceptions that lead to !success already do their own logging.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"}],"nova/tests/functional/api/openstack/placement/test_report_client.py":[{"author":{"_account_id":11564,"name":"Chris Dent","email":"cdent@anticdent.org","username":"chdent"},"change_message_id":"31400ead62dd5a4a0bae254f4e2babbf49a57bf0","unresolved":false,"context_lines":[{"line_number":688,"context_line":"                self.client._get_inventory("},{"line_number":689,"context_line":"                    self.context, uuids.cn)[\u0027inventories\u0027])"},{"line_number":690,"context_line":""},{"line_number":691,"context_line":"    def test_update_from_provider_tree(self):"},{"line_number":692,"context_line":"        \"\"\"A \"realistic\" walk through the lifecycle of a compute node provider"},{"line_number":693,"context_line":"        tree."},{"line_number":694,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_be733580","line":691,"updated":"2018-02-19 15:04:08.000000000","message":"A thing we\u0027re going to want to think about, eventually but not immediately, is that this entire file is mostly testing something that is not \"within\" placement.\n\nI put it here originally because it was using the placement interceptor, but at some point there\u0027s going to be a bigger boundary.\n\nNothing that needs to be addressed now, just (yet another) think to remember.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"6918e2f8b63f221f5c066c975dd90f9ad9cc3930","unresolved":false,"context_lines":[{"line_number":688,"context_line":"                self.client._get_inventory("},{"line_number":689,"context_line":"                    self.context, uuids.cn)[\u0027inventories\u0027])"},{"line_number":690,"context_line":""},{"line_number":691,"context_line":"    def test_update_from_provider_tree(self):"},{"line_number":692,"context_line":"        \"\"\"A \"realistic\" walk through the lifecycle of a compute node provider"},{"line_number":693,"context_line":"        tree."},{"line_number":694,"context_line":"        \"\"\""}],"source_content_type":"text/x-python","patch_set":22,"id":"3fa0c359_1f6020a5","line":691,"in_reply_to":"3fa0c359_be733580","updated":"2018-02-19 19:29:24.000000000","message":"You\u0027re just referring to the api/openstack/placement part of the path leading up to this, right?  Cause yeah, in the eventually, we definitely want this exact suite running against some kind of faked-out placement (the \"interceptor\"? not the \"interceptor\"? what do test_scheduler and test_report_client use?) exactly as it is now.","commit_id":"d363b8d85eb2b3bf3de0e42b8dd50eeceb56a9b8"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"c5b3c07bf16fc8f6a9ac0d46b5c872d5f105de5d","unresolved":false,"context_lines":[{"line_number":860,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":861,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":862,"context_line":"            self.assertRaises("},{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_ff749c00","line":863,"range":{"start_line":863,"start_character":26,"end_line":863,"end_character":52},"updated":"2018-03-19 12:03:26.000000000","message":"Why should we map all the exceptions to this one? Wouldn\u0027t we want to know if one more attempt would success or it would fail forever because of the wrong traits/resource class?","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"b11b675f3c7029320c8b815d170d7df127876143","unresolved":false,"context_lines":[{"line_number":860,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":861,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":862,"context_line":"            self.assertRaises("},{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_c93a2fcc","line":863,"range":{"start_line":863,"start_character":26,"end_line":863,"end_character":52},"in_reply_to":"df7087c5_15037111","updated":"2018-03-20 02:20:36.000000000","message":"\u003e Who\u0027s \"we\" in this question?\n\u003e Are you suggesting that the original exception be bubbled up?\n\nYup, I just wondered if there are cases that the virt driver (, resource tracker or anything who use this update_from_provider_tree function) should change the next action depending on the kind of the exceptions.","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"78aec90c665a587f12acbc8a801b97e05e3a6d41","unresolved":false,"context_lines":[{"line_number":860,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":861,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":862,"context_line":"            self.assertRaises("},{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_85cff87d","line":863,"range":{"start_line":863,"start_character":26,"end_line":863,"end_character":52},"in_reply_to":"df7087c5_b1095932","updated":"2018-03-22 04:42:39.000000000","message":"\u003e  Nothing about the way the exception is handled in this patch locks us down.\n\nSure.","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d9b402a4e5759257b67388d52d8da9ad3418855e","unresolved":false,"context_lines":[{"line_number":860,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":861,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":862,"context_line":"            self.assertRaises("},{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_b1095932","line":863,"range":{"start_line":863,"start_character":26,"end_line":863,"end_character":52},"in_reply_to":"df7087c5_c93a2fcc","updated":"2018-03-20 13:24:35.000000000","message":"It would be the resource tracker [1].  At the moment, that\u0027s designed to allow the exception to bubble up from here until it eventually gets logged (but otherwise ignored) and we just hit this code again the next time the periodic task runs _update.\n\nWe may want to react more granularly in the future. Instinctively, I\u0027m not loving the idea of putting localized conditionals at that level of the resource tracker, though.  I think I would probably want to do it here within update_from_provider_tree.\n\nBut in any case, we\u0027re free to make such changes in the future.  Nothing about the way the exception is handled in this patch locks us down.\n\n[1] https://review.openstack.org/#/c/520246/53/nova/compute/resource_tracker.py@883","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"0072a546f299e61be5aeac0bb351165508d310fe","unresolved":false,"context_lines":[{"line_number":860,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":861,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":862,"context_line":"            self.assertRaises("},{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_15037111","line":863,"range":{"start_line":863,"start_character":26,"end_line":863,"end_character":52},"in_reply_to":"df7087c5_ff749c00","updated":"2018-03-19 13:42:32.000000000","message":"Who\u0027s \"we\" in this question?\n\nIf the code could/would do something differently based on those different conditions, we could write that logic within update_from_provider_tree itself.  Or, as you say, we could make the raised exceptions more granular.  But for now, we\u0027re only going so far as to log the original exception so the admin could conceivably figure out what the problem is.\n\nPerhaps that\u0027s not your question, though.  Are you suggesting that the original exception be bubbled up?  As written, there may be more than one such exception.\n\nOr are you suggesting that we should somehow try to classify exceptions as \"retryable\" or not?  I wouldn\u0027t know how to do that.","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"c5b3c07bf16fc8f6a9ac0d46b5c872d5f105de5d","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."},{"line_number":867,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":868,"context_line":"            # The out-of-band change is blown away, as it should be."},{"line_number":869,"context_line":"            assert_ptrees_equal()"}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_ff1fbcb4","line":866,"updated":"2018-03-19 12:03:26.000000000","message":"Sounds that it detects generation conflicts between report client\u0027s cache and placement but it doesn\u0027t catch the conflicts between new_tree and cache/placement.\n\n... why not? (Why is this desirable?) ... RPs in new_tree never have generation? ... why not?","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"b11b675f3c7029320c8b815d170d7df127876143","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."},{"line_number":867,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":868,"context_line":"            # The out-of-band change is blown away, as it should be."},{"line_number":869,"context_line":"            assert_ptrees_equal()"}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_6909a3db","line":866,"in_reply_to":"df7087c5_356234da","updated":"2018-03-20 02:20:36.000000000","message":"Thank you so much for the detail explanation. It\u0027s now cleared. Then on the failures, should the report client return the updated current tree to the virt driver?","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"d9b402a4e5759257b67388d52d8da9ad3418855e","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."},{"line_number":867,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":868,"context_line":"            # The out-of-band change is blown away, as it should be."},{"line_number":869,"context_line":"            assert_ptrees_equal()"}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_c1399aa3","line":866,"in_reply_to":"df7087c5_6909a3db","updated":"2018-03-20 13:24:35.000000000","message":"We invalidate the cache for the failing providers.  Then the next time the periodic task runs, we\u0027ll re-fetch those providers, so they\u0027re up to date (barring yet more asynchronous changes of course) for the next time we call update_provider_tree.","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":25625,"name":"Tetsuro Nakamura","email":"tetsuro.nakamura.bc@hco.ntt.co.jp","username":"tetsuro0907"},"change_message_id":"78aec90c665a587f12acbc8a801b97e05e3a6d41","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."},{"line_number":867,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":868,"context_line":"            # The out-of-band change is blown away, as it should be."},{"line_number":869,"context_line":"            assert_ptrees_equal()"}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_a58f9c2c","line":866,"in_reply_to":"df7087c5_c1399aa3","updated":"2018-03-22 04:42:39.000000000","message":"Somehow I had been badly misunderstanding that the virt driver refers a different object\u0027s instance from what report client refers... Now I understand. Sorry for the noise, but thanks.","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"0072a546f299e61be5aeac0bb351165508d310fe","unresolved":false,"context_lines":[{"line_number":863,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":864,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":865,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":866,"context_line":"            # generation and so the next attempt should succeed."},{"line_number":867,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":868,"context_line":"            # The out-of-band change is blown away, as it should be."},{"line_number":869,"context_line":"            assert_ptrees_equal()"}],"source_content_type":"text/x-python","patch_set":24,"id":"df7087c5_356234da","line":866,"in_reply_to":"df7087c5_ff1fbcb4","updated":"2018-03-19 13:42:32.000000000","message":"Right, the virt driver (update_provider_tree) is not responsible for dealing with generations at all.  It doesn\u0027t have the context for that.  The report client\u0027s cached data is populated with generations based on when it was pulled from placement.  That\u0027s the picture we send to update_provider_tree, so that\u0027s what the virt driver sees as \"current\".  When virt makes changes, and the report client (update_from_provider_tree) goes to flush them, it\u0027s the report client that has to detect if the placement DB has moved on since the report client pulled the data.","commit_id":"0ee42838beae7d6003d75d4c9da8c3c507412c61"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e5c9c671e39ecc82da9c0bdb752c3ce8cc308b98","unresolved":false,"context_lines":[{"line_number":805,"context_line":"                         \u0027allocation_ratio\u0027: 8.0,"},{"line_number":806,"context_line":"                     }), None)"},{"line_number":807,"context_line":"            self.assertRaises("},{"line_number":808,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":809,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":810,"context_line":"            # The inventory update didn\u0027t get synced..."},{"line_number":811,"context_line":"            self.assertIsNone(self.client._get_inventory("}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_1f02190c","line":808,"range":{"start_line":808,"start_character":16,"end_line":808,"end_character":53},"updated":"2018-03-30 19:02:39.000000000","message":"catch_all caught InvalidResourceClass here.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f4822372fdddace2cb3648f8af724cf77dfa0654","unresolved":false,"context_lines":[{"line_number":809,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":810,"context_line":"            # The inventory update didn\u0027t get synced..."},{"line_number":811,"context_line":"            self.assertIsNone(self.client._get_inventory("},{"line_number":812,"context_line":"                self.context, uuids.grandchild1_1))"},{"line_number":813,"context_line":"            # ...and the grandchild was removed from the cache"},{"line_number":814,"context_line":"            self.assertFalse("},{"line_number":815,"context_line":"                self.client._provider_tree.exists(\u0027grandchild1_1\u0027))"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_0134e548","line":812,"updated":"2018-04-02 20:19:22.000000000","message":"Is this the verification that the association_refresh_time had the RP removed from it because of the raised exception?","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"bc26d91754062a05add01d3ac517b7683151dacd","unresolved":false,"context_lines":[{"line_number":809,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":810,"context_line":"            # The inventory update didn\u0027t get synced..."},{"line_number":811,"context_line":"            self.assertIsNone(self.client._get_inventory("},{"line_number":812,"context_line":"                self.context, uuids.grandchild1_1))"},{"line_number":813,"context_line":"            # ...and the grandchild was removed from the cache"},{"line_number":814,"context_line":"            self.assertFalse("},{"line_number":815,"context_line":"                self.client._provider_tree.exists(\u0027grandchild1_1\u0027))"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_a1f73959","line":812,"in_reply_to":"bf659307_0134e548","updated":"2018-04-02 20:31:04.000000000","message":"No, this is just ensuring that the (bogus) inventory from above didn\u0027t get pushed to placement.\n\nI\u0027m really not doing anything to validate the association refresh time.  But we\u0027re going to quit using it [1] so... is it worth it?\n\n[1] https://review.openstack.org/#/c/535517/ - see comments on PS26","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"dd6c581c5229d7924784ecbaf34ce05e06b62f1b","unresolved":false,"context_lines":[{"line_number":809,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":810,"context_line":"            # The inventory update didn\u0027t get synced..."},{"line_number":811,"context_line":"            self.assertIsNone(self.client._get_inventory("},{"line_number":812,"context_line":"                self.context, uuids.grandchild1_1))"},{"line_number":813,"context_line":"            # ...and the grandchild was removed from the cache"},{"line_number":814,"context_line":"            self.assertFalse("},{"line_number":815,"context_line":"                self.client._provider_tree.exists(\u0027grandchild1_1\u0027))"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_7c5306e3","line":812,"in_reply_to":"bf659307_a1f73959","updated":"2018-04-02 20:43:40.000000000","message":"If it\u0027s going to be removed soon, then no, I don\u0027t think it\u0027s so important to add here.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"f4822372fdddace2cb3648f8af724cf77dfa0654","unresolved":false,"context_lines":[{"line_number":812,"context_line":"                self.context, uuids.grandchild1_1))"},{"line_number":813,"context_line":"            # ...and the grandchild was removed from the cache"},{"line_number":814,"context_line":"            self.assertFalse("},{"line_number":815,"context_line":"                self.client._provider_tree.exists(\u0027grandchild1_1\u0027))"},{"line_number":816,"context_line":""},{"line_number":817,"context_line":"            # Fix that problem so we can try the next one"},{"line_number":818,"context_line":"            new_tree.update_inventory("}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_81f23599","line":815,"updated":"2018-04-02 20:19:22.000000000","message":"Okay, so this asserts the RP got removed from the provider tree.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e5c9c671e39ecc82da9c0bdb752c3ce8cc308b98","unresolved":false,"context_lines":[{"line_number":832,"context_line":"                                                \u0027HW_CPU_X86_AVX2\u0027,"},{"line_number":833,"context_line":"                                                \u0027MOTSUC_FOO\u0027])"},{"line_number":834,"context_line":"            self.assertRaises("},{"line_number":835,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":836,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":837,"context_line":"            # Placement didn\u0027t get updated"},{"line_number":838,"context_line":"            self.assertEqual(set([\u0027HW_CPU_X86_AVX\u0027, \u0027HW_CPU_X86_AVX2\u0027]),"}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_8a0b4922","line":835,"range":{"start_line":835,"start_character":16,"end_line":835,"end_character":52},"updated":"2018-03-30 19:02:39.000000000","message":"catch_all caught TraitCreationFailed here.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"e5c9c671e39ecc82da9c0bdb752c3ce8cc308b98","unresolved":false,"context_lines":[{"line_number":864,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":865,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":866,"context_line":"            self.assertRaises("},{"line_number":867,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":868,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":869,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":870,"context_line":"            # generation and so the next attempt should succeed."}],"source_content_type":"text/x-python","patch_set":25,"id":"bf659307_4a11b151","line":867,"range":{"start_line":867,"start_character":16,"end_line":867,"end_character":52},"updated":"2018-03-30 19:02:39.000000000","message":"catch_all caught ResourceProviderUpdateConflict here.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"3595faa98400be757783d4182f231edd2b594c00","unresolved":false,"context_lines":[{"line_number":850,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":851,"context_line":"            assert_ptrees_equal()"},{"line_number":852,"context_line":""},{"line_number":853,"context_line":"            # Let\u0027s cause a conflict error by doing an \"out-of-band\" update"},{"line_number":854,"context_line":"            gen \u003d self.client._provider_tree.data(uuids.ssp).generation"},{"line_number":855,"context_line":"            self.assertTrue(self.client.put("},{"line_number":856,"context_line":"                \u0027/resource_providers/%s/traits\u0027 % uuids.ssp,"},{"line_number":857,"context_line":"                {\u0027resource_provider_generation\u0027: gen,"},{"line_number":858,"context_line":"                 \u0027traits\u0027: [\u0027MISC_SHARES_VIA_AGGREGATE\u0027, \u0027STORAGE_DISK_HDD\u0027]},"},{"line_number":859,"context_line":"                version\u003d\u00271.6\u0027))"},{"line_number":860,"context_line":""},{"line_number":861,"context_line":"            # Now if we try to modify the traits, we should fail and invalidate"},{"line_number":862,"context_line":"            # the cache..."},{"line_number":863,"context_line":"            new_tree.update_traits(uuids.ssp, [\u0027MISC_SHARES_VIA_AGGREGATE\u0027,"},{"line_number":864,"context_line":"                                               \u0027STORAGE_DISK_SSD\u0027,"},{"line_number":865,"context_line":"                                               \u0027CUSTOM_FAST\u0027])"},{"line_number":866,"context_line":"            self.assertRaises("},{"line_number":867,"context_line":"                exception.ResourceProviderSyncFailed,"},{"line_number":868,"context_line":"                self.client.update_from_provider_tree, self.context, new_tree)"},{"line_number":869,"context_line":"            # ...but the next iteration will refresh the cache with the latest"},{"line_number":870,"context_line":"            # generation and so the next attempt should succeed."},{"line_number":871,"context_line":"            self.client.update_from_provider_tree(self.context, new_tree)"},{"line_number":872,"context_line":"            # The out-of-band change is blown away, as it should be."},{"line_number":873,"context_line":"            assert_ptrees_equal()"},{"line_number":874,"context_line":""},{"line_number":875,"context_line":"            # Let\u0027s delete some stuff"},{"line_number":876,"context_line":"            new_tree.remove(uuids.ssp)"}],"source_content_type":"text/x-python","patch_set":25,"id":"df7087c5_d84ce99f","line":873,"range":{"start_line":853,"start_character":0,"end_line":873,"end_character":33},"updated":"2018-03-22 15:21:08.000000000","message":"Nice.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"},{"author":{"_account_id":7,"name":"Jay Pipes","email":"jaypipes@gmail.com","username":"jaypipes"},"change_message_id":"3595faa98400be757783d4182f231edd2b594c00","unresolved":false,"context_lines":[{"line_number":893,"context_line":"            for uuid in (uuids.root, uuids.child1, uuids.grandchild1_1,"},{"line_number":894,"context_line":"                         uuids.ssp):"},{"line_number":895,"context_line":"                resp \u003d self.client.get(\u0027/resource_providers/%s\u0027 % uuid)"},{"line_number":896,"context_line":"                self.assertEqual(404, resp.status_code)"}],"source_content_type":"text/x-python","patch_set":25,"id":"df7087c5_d831490d","line":896,"updated":"2018-03-22 15:21:08.000000000","message":"awesome functional tests, Eric, thank you.\n\nMy only suggestion would be to name the providers something like numa_cell0, pf1, etc instead of child1, grandchild1 etc.\n\nBut that\u0027s a femto-nit. The tests are great as-is.","commit_id":"aa23d4832712ec453edf0e2dc74a93ba6ed93c39"}],"nova/tests/unit/scheduler/client/test_report.py":[{"author":{"_account_id":14070,"name":"Eric Fried","email":"openstack@fried.cc","username":"efried"},"change_message_id":"7c4e593a3287f2c9257d4b99b0384bba0ff9001a","unresolved":false,"context_lines":[],"source_content_type":"","patch_set":9,"id":"5f93b717_ab510811","updated":"2018-01-25 00:08:58.000000000","message":"Still three known failures in here.  I ran out of time.","commit_id":"d69b476dfaa3c549f8c9fc770ad387ec929f6f11"}]}