Fill fault from all nova cells in GET servers/detail

[imitevm]

No description provided.

[joker-at-work]

def _key(x):
  return getattr(x, 'id', None) if x else None

inst_mappings = sorted(inst_mappings, key=_key)
for im, uuids in itertools.groupby(inst_mappings, key=_key):

Is that the same? If yes, please change it as it's much shorter and still more expressive in what it does.

[imitevm]

No, it's not the same but I'll rework the code to look in a similar fashion.

[imitevm]

Done.

[joker-at-work]

Both your instances have the same cell here. If we want to upstream this - and we should - we need a proper test-case. Best thing is to have a test-case that can be run without your changes, but fails, so we can convince upstream that there's a bug. It should then work with your changes obviously ;)

[imitevm]

Done. Improved test to cover different cells.

[joker-at-work]

I don't think that's the right way to adjust the test. I ran your test on the old code and it's complaining about not seeing calls with all contexts made. I think it would be better, if we would add another test_fill_faults_multicell instead of changing test_fill_faults. This test should contain multiple instances in multiple cells, some without cell_mapping. At least one fault per cell. The test is basically the same and you can keep the assert_has_calls if you like, but that's an implementation detail I wouldn't check.Instead, the earlier check for faults should show that not all faults for the uuids were collected (without applying your patch).

[joker-at-work]

Sorry, forgot to add: the code-changes otherwise look fine.

[imitevm]

I don't think that's the right way to adjust the test. I ran your test on the old code and it's complaining about not seeing calls with all contexts made. I think it would be better, if we would add another test_fill_faults_multicell instead of changing test_fill_faults. This test should contain multiple instances in multiple cells, some without cell_mapping. At least one fault per cell. The test is basically the same and you can keep the assert_has_calls if you like, but that's an implementation detail I wouldn't check.Instead, the earlier check for faults should show that not all faults for the uuids were collected (without applying your patch).

Done.

[joker-at-work]

If I run the tests, test_fill_faults_multicell fails as expected and nicely without your path. But with your patch, test_fill_faults fails for me. Can you confirm?

nova.tests.unit.objects.test_instance.TestInstanceListObject.test_fill_faults
-----------------------------------------------------------------------------

Captured pythonlogging:
~~~~~~~~~~~~~~~~~~~~~~~
    2019-12-06 15:00:44,590 WARNING [oslo_config.cfg] Config option key_manager.api_class  is deprecated. Use option key_manager.backend instead.
    2019-12-06 15:00:46,208 INFO [248_add_expire_reservations_index] Skipped adding reservations_deleted_expire_idx because an equivalent index already exists.
    

Captured traceback:
~~~~~~~~~~~~~~~~~~~
    Traceback (most recent call last):
      File "/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py", line 1305, in patched
        return func(*args, **keywargs)
      File "nova/tests/unit/objects/test_instance.py", line 1899, in test_fill_faults
        self.assertEqual([uuids.db_fault_1], list(faulty))
      File "/nova/.tox/py27/local/lib/python2.7/site-packages/testtools/testcase.py", line 411, in assertEqual
        self.assertThat(observed, matcher, message)
      File "/nova/.tox/py27/local/lib/python2.7/site-packages/testtools/testcase.py", line 498, in assertThat
        raise mismatch_error
    testtools.matchers._impl.MismatchError: ['bcc28756-bdbf-47fc-9879-0b1fd97f1360'] != []

[joker-at-work]

Do I understand this correctly: cell_id can be None and im.cell_mapping can be None, too? Should we have an if cell_id is None: continue in the for-loop?

[imitevm]

Yes. PR updated.

[joker-at-work]

You're missing a test for this. You updated the tests, so the cell-mapping is there, but nothing failed before, so we're missing a test.