nova.tests.unit.virt.hyperv.test_serialproxy.SerialProxyTestCase.test_stop_serial_proxy failure

[Lee Yarwood]

https://1e1442a936d81c7126bc-fc1e7af78d7504e20bbc3359634b65b5.ssl.cf1.rackcdn.com/800313/1/gate/openstack-tox-lower-constraints/6ee3d8d/testr_results.html

2021-07-13 16:22:36,703 WARNING [oslo_policy.policy] JSON formatted policy_file support is deprecated since Victoria release. You need to use YAML format which will be default in future. You can use ``oslopolicy-convert-json-to-yaml`` tool to convert existing JSON-formatted policy file to YAML-formatted in backward compatible way: https://docs.openstack.org/oslo.policy/latest/cli/oslopolicy-convert-json-to-yaml.html.
2021-07-13 16:22:36,704 WARNING [oslo_policy.policy] JSON formatted policy_file support is deprecated since Victoria release. You need to use YAML format which will be default in future. You can use ``oslopolicy-convert-json-to-yaml`` tool to convert existing JSON-formatted policy file to YAML-formatted in backward compatible way: https://docs.openstack.org/oslo.policy/latest/cli/oslopolicy-convert-json-to-yaml.html.
2021-07-13 16:22:36,708 WARNING [oslo_policy.policy] Policy Rules ['os_compute_api:extensions', 'os_compute_api:os-floating-ip-pools', 'os_compute_api:os-quota-sets:defaults', 'os_compute_api:os-availability-zone:list', 'os_compute_api:limits', 'system_admin_api', 'system_reader_api', 'project_admin_api', 'project_member_api', 'project_reader_api', 'system_admin_or_owner', 'system_or_project_reader', 'os_compute_api:limits:other_project', 'os_compute_api:os-lock-server:unlock:unlock_override', 'os_compute_api:servers:create:zero_disk_flavor', 'compute:servers:resize:cross_cell'] specified in policy files are the same as the defaults provided by the service. You can remove these rules from policy files which will make maintenance easier. You can detect these redundant rules by ``oslopolicy-list-redundant`` tool also.
}}}

Traceback (most recent call last):
  File "/home/zuul/src/opendev.org/openstack/nova/nova/tests/unit/virt/hyperv/test_serialproxy.py", line 70, in test_stop_serial_proxy
    self._proxy._conn.shutdown.assert_called_once_with(socket.SHUT_RDWR)
  File "/home/zuul/src/opendev.org/openstack/nova/.tox/lower-constraints/lib/python3.8/site-packages/mock/mock.py", line 955, in assert_called_once_with
    raise AssertionError(msg)
AssertionError: Expected 'mock' to be called once. Called 2 times.
Calls: [call(), call(2)].

[Balazs Gibizer (balazs-gibizer)]

I saw a similar failure locally where the mock was called 14 times.

[Lee Yarwood (lyarwood)]

https://f2395408510981529daa-158a33a5449e12f6f494625dd8517fb1.ssl.cf1.rackcdn.com/803322/1/gate/openstack-tox-lower-constraints/2e3c0eb/testr_results.html

[Balazs Gibizer (balazs-gibizer)]

https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_4a3/796208/4/check/openstack-tox-py38/4a3c630/testr_results.html

[Balazs Gibizer (balazs-gibizer)]

There are native threading involved in the hyperv serialproxy implementation so that is my first suspect. No proof yet. But I can reproduce the issue locally if I run the whole unit test suite in a loop

[Balazs Gibizer (balazs-gibizer)]

I managed to find that if these two tests runs in the same executor then the proxy test fails

$ cat > test_list
nova.tests.unit.storage.test_rbd.RbdTestCase.test_cloneable
nova.tests.unit.virt.hyperv.test_serialproxy.SerialProxyTestCase.test_stop_serial_proxy
^D
$ source .tox/py38/bin/activate
$ stestr run --serial --load-list test_list

But I have no idea why

[Balazs Gibizer (balazs-gibizer)]

The self._proxy._conn.shutdown in [1] is the same instance that [2] because the Rados name is an alias of mock.Mock and that setup therefore sets shutdown on Mock class level not on instance level. The () is missing from after mock.Mock.

Patch will arrive soon

[1] https://github.com/openstack/nova/blob/10b1dc84f47a71061340f8e0ae0fe32dca44061a/nova/tests/unit/virt/hyperv/test_serialproxy.py#L70
[2] https://github.com/openstack/nova/blob/10b1dc84f47a71061340f8e0ae0fe32dca44061a/nova/tests/unit/storage/test_rbd.py#L122-L125

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/nova/+/805657

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.opendev.org/c/openstack/nova/+/805668

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/805657
Committed: https://opendev.org/openstack/nova/commit/930b7c992156733fbb4f598488605825d62ebc0c
Submitter: "Zuul (22348)"
Branch: master

commit 930b7c992156733fbb4f598488605825d62ebc0c
Author: Balazs Gibizer <email address hidden>
Date: Mon Aug 23 17:13:14 2021 +0200

    Avoid modifying the Mock class in test

    The rdb unit test defines a shutdown field on the Mock class
    unintentionally. This causes that a later test in the same
    executor expecting that mock.Mock().shutdown is a newly auto
    generated mock.Mock() but it founds that is an already used
    (called) object. This causes that the later test fails when
    asserting the number of calls on that mock.

    The original intention of the rbd unit test was to catch the
    instantiation of the Rados object in test so it set Rados = mock.Mock()
    so when the code under test called Rados() it actually called Mock().
    It worked but it has that huge side effect. Instead of this a proper
    mocking of the constructor can be done in two steps:

    rados_inst = mock.Mock()
    Rados = mock.Mock(return_value=rados_inst)

    This makes sure that every Rados() call will return rados_inst that is a
    mock without causing Mock class level side effect.

    Change-Id: If71620e808744736cb4fe3abda76d81a6335311b
    Closes-Bug: #1936849

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/wallaby
Review: https://review.opendev.org/c/openstack/nova/+/805759

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/victoria
Review: https://review.opendev.org/c/openstack/nova/+/805823

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/805759
Committed: https://opendev.org/openstack/nova/commit/c958fab901be97999c0d117faa31ab53e52a3371
Submitter: "Zuul (22348)"
Branch: stable/wallaby

commit c958fab901be97999c0d117faa31ab53e52a3371
Author: Balazs Gibizer <email address hidden>
Date: Mon Aug 23 17:13:14 2021 +0200

    Avoid modifying the Mock class in test

    The rdb unit test defines a shutdown field on the Mock class
    unintentionally. This causes that a later test in the same
    executor expecting that mock.Mock().shutdown is a newly auto
    generated mock.Mock() but it founds that is an already used
    (called) object. This causes that the later test fails when
    asserting the number of calls on that mock.

    The original intention of the rbd unit test was to catch the
    instantiation of the Rados object in test so it set Rados = mock.Mock()
    so when the code under test called Rados() it actually called Mock().
    It worked but it has that huge side effect. Instead of this a proper
    mocking of the constructor can be done in two steps:

    rados_inst = mock.Mock()
    Rados = mock.Mock(return_value=rados_inst)

    This makes sure that every Rados() call will return rados_inst that is a
    mock without causing Mock class level side effect.

    Change-Id: If71620e808744736cb4fe3abda76d81a6335311b
    Closes-Bug: #1936849
    (cherry picked from commit 930b7c992156733fbb4f598488605825d62ebc0c)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/805823
Committed: https://opendev.org/openstack/nova/commit/1eceeebfb25ccdacdb8c74c5171f0fa9e591ce82
Submitter: "Zuul (22348)"
Branch: stable/victoria

commit 1eceeebfb25ccdacdb8c74c5171f0fa9e591ce82
Author: Balazs Gibizer <email address hidden>
Date: Mon Aug 23 17:13:14 2021 +0200

    Avoid modifying the Mock class in test

    The rdb unit test defines a shutdown field on the Mock class
    unintentionally. This causes that a later test in the same
    executor expecting that mock.Mock().shutdown is a newly auto
    generated mock.Mock() but it founds that is an already used
    (called) object. This causes that the later test fails when
    asserting the number of calls on that mock.

    The original intention of the rbd unit test was to catch the
    instantiation of the Rados object in test so it set Rados = mock.Mock()
    so when the code under test called Rados() it actually called Mock().
    It worked but it has that huge side effect. Instead of this a proper
    mocking of the constructor can be done in two steps:

    rados_inst = mock.Mock()
    Rados = mock.Mock(return_value=rados_inst)

    This makes sure that every Rados() call will return rados_inst that is a
    mock without causing Mock class level side effect.

    Change-Id: If71620e808744736cb4fe3abda76d81a6335311b
    Closes-Bug: #1936849
    (cherry picked from commit 930b7c992156733fbb4f598488605825d62ebc0c)
    (cherry picked from commit c958fab901be97999c0d117faa31ab53e52a3371)

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/c/openstack/nova/+/805668
Committed: https://opendev.org/openstack/nova/commit/9f8cc2f03827c10f89ee652bc73790b3c58111d3
Submitter: "Zuul (22348)"
Branch: master

commit 9f8cc2f03827c10f89ee652bc73790b3c58111d3
Author: Balazs Gibizer <email address hidden>
Date: Mon Aug 23 19:23:31 2021 +0200

    Add two new hacking rules

    As the bug and fix If71620e808744736cb4fe3abda76d81a6335311b showed
    it is dangerous to forget instantiating the Mock class before it is
    used in the test as changes on the class directly leaks out from the
    test. In almost all the cases using Mock class directly is a bug and the
    author original intention is to use an instance instead, just forgot
    about the parents. So this patch adds two new hacking rules:

    N367: catches the case when Mock class is aliased in the test:
        self.mock_mystuff = mock.Mock

    N368: catches when mock.patch instructed to use the Mock class as
    replacement value during patching:
        mock.patch('Bar.foo', new=mock.Mock)

    For N367 the previous patch removed the last hit. For N368 this patch
    removes the two hits exists.

    Change-Id: Id42ca571b1569886ef47aa350369e9d2068e77bc
    Related-Bug: #1936849

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 24.0.0.0rc1 release candidate.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 22.3.0 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/nova 23.1.0 release.