Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-07-27
12:00:13 gibi hm
12:00:14 sean-k-mooney we dont tend to use autospec much
12:00:29 sean-k-mooney we do in somecases but i rarely do
12:01:14 gibi A().foo = mock.Mock() is a monkeypatch that is not reverted automatically so that is a bad pattern
12:01:43 gibi also I think there is automatic autospecing in the mock lib (obviously not for mock.Mock() but for mock.patch decorator)
12:02:09 sean-k-mooney i dont think there is
12:02:14 sean-k-mooney or at least
12:02:21 sean-k-mooney its behavior shoudl be the same as the standard lib
12:02:28 sean-k-mooney so if there is it shoudl be in both
12:02:48 sean-k-mooney gibi: in the A().foo = mock.Mock()
12:03:08 sean-k-mooney we shoudl not need to revert that automaticlly if A is a mock already
12:03:52 gibi ahh, yeah, I ment there is automatic autospec in both lib and stdlib
12:04:56 gibi sean-k-mooney: yeah, if A().foo is already a mocked then that mock's patcher.stop() will revert the change. But the caller doing the A().foo = mock.Mock() does not know if foo is a Mock. So we establish a dangerous pattern
12:05:37 gibi if that pattern is used to other than a mocked field the we will leak global state between tests
12:06:22 sean-k-mooney gibi: right but we would only do that for the double mock case
12:06:40 sean-k-mooney but i agreee in general its bad
12:06:49 sean-k-mooney we have leaked state that way in the past
12:06:51 gibi yes, now, but then the next new dev came look at the code and think that A().foo = mock.Mock() is a generic pattern
12:07:05 sean-k-mooney where a mock was assinged to a stdlib function
12:07:25 sean-k-mooney so that is why i was wondering if we need a nova helper fucniton
12:07:34 sean-k-mooney to basiclaly always do the right thing
12:07:50 gibi but that mean all our mocking need to go through that helper
12:07:54 gibi that will be a massive change
12:08:19 sean-k-mooney perhaps i need to think about what pattern we can actully use that will work
12:08:36 gibi anyhow I'd like to do a non theorethicaly investigation with this
12:08:40 sean-k-mooney perhaps using the mock as a context manager
12:08:49 sean-k-mooney but not sure if that would still fail for the double mocking case
12:09:04 gibi btw this is why I remembered automatic autospeccing https://review.opendev.org/q/message:autospec+project:openstack/nova+status:merged
12:09:53 sean-k-mooney sure but that not automatic
12:10:06 sean-k-mooney that was exiplcitly enabling it and new right
12:10:11 gibi yeah, I missremembered
12:10:16 sean-k-mooney oh ok
12:10:54 gibi I will look at this mock thing tomorrow
12:11:10 gibi now I'm in a middle fighting with reshape + provider generation conflict
12:12:12 gibi I'm in the denial phase where I think my code is correct and reshape is broken :D
12:12:46 sean-k-mooney :)
12:13:08 sean-k-mooney maybe but if thats the case you will probly have to fix it in placment yes
12:13:17 sean-k-mooney so hopefully the issue is in your current code
12:13:21 gibi I know :D
12:13:24 sean-k-mooney that feels like less work
12:13:25 gibi hence "denial"
12:13:54 gibi anyhow I think this is the first case when we both create new RPs and reshape allocations at the same time
12:14:08 gibi from nova
12:14:55 gibi and RP creation is outside of the reshape code
12:15:02 sean-k-mooney if nova has not done it its likely no one else has
12:15:17 sean-k-mooney i dont think neutron or cyborg have used reshapes before
12:16:28 gibi me neither
12:50:54 bauzas I just capture the discussion
12:51:10 bauzas is it because unittest.mock is not on par with our mock lib ?
12:52:16 gibi it is that in py310 unittest.mock introduced a breaking change. The mock lib is a rolling backport of the upstream unittest.mock so at some point this change will appera in the standalone mock lib too
12:52:19 kashyap bauzas: Please put the stable/wallaby backport through: https://review.opendev.org/c/openstack/nova/+/851206
12:52:34 kashyap gibi ACKed it too; and you've ACKed the upstream main
12:53:39 gibi kashyap: we need to land the backports in order
12:53:44 gibi so we land the yoga first
12:53:48 gibi then xena
12:53:50 gibi then wallaby
12:54:03 kashyap gibi: Oh, yes. I mean to also implicitly look at 'em. :)
12:54:11 gibi ack
12:54:21 bauzas gibi: which breaking change ?
12:54:53 gibi if you mock a function twice, the second mocking attempt will fail
12:55:05 gibi as autospecing a mock is disallowed now
12:55:48 bauzas then we should wait for our mock lib
12:55:57 gibi what? why?
12:56:19 gibi why it is better to get the same breaking change later?
12:56:20 bauzas we should change the related tests first due to this
12:57:04 bauzas gibi: well, once our mock lib would be modified, then the CI wouldn't work and we would see it so we could not accept the new release
12:57:16 bauzas then we could modify the tests
12:57:23 bauzas for this new release
12:57:32 bauzas and then accepting the new release
12:58:07 gibi this is the same thing. If we want to accept py310 it means we need to accept all the breaking changes there.
12:59:13 gibi so we are only debating when to fix those duplicated mocks
12:59:27 bauzas at least we need to modify the only needed tests
12:59:29 gibi and as the mock switch is ready now I think we should fix it now
12:59:40 bauzas before changing to use unittest.mock
13:00:17 gibi why before? we can switch to unittest.mock and then fix the duplicated mock to get the py310 non voting job gren
13:00:20 gibi green again
13:01:06 gibi switching first make py310 job telling us where are the duplicates
13:01:55 admin1 i have nova using local disk , but glance and cinder uses ceph .. when using snapshots, i get broken pipe from nova compute .. is there any special way ceph needs to be configured for snapshots to work ?
13:12:03 sean-k-mooney admin1: no in that config nova should upload the snapshot via the glace http api
13:12:25 sean-k-mooney so it will create a snapshot using qemu-image then upload that disk
13:19:53 opendevreview Sylvain Bauza proposed openstack/nova master: api: Drop generating a keypair and add special chars to naming https://review.opendev.org/c/openstack/nova/+/849133
13:20:14 bauzas rebased my keypair api microversion due to unshelve_to_host merge ^
13:20:24 bauzas people can review it
13:27:56 opendevreview Kashyap Chamarthy proposed openstack/nova stable/victoria: Add a workaround to skip hypervisor version check on LM https://review.opendev.org/c/openstack/nova/+/851223
13:31:38 gibi bauzas: +2, do you have an python binding patch and or osc patch with the client side change?
13:31:45 bauzas not yet
13:31:53 opendevreview Kashyap Chamarthy proposed openstack/nova stable/victoria: Add a workaround to skip hypervisor version check on LM https://review.opendev.org/c/openstack/nova/+/851223
13:32:04 gibi Uggla: similar question to you too, do you have a client patch for the unshelve to host change?
13:32:08 gibi bauzas: ack
13:32:31 sean-k-mooney technically we dont need thos to merge the nova change but ya woudl be nice to see them sooner rather then later
13:32:38 Uggla gibi, yes give you the ids in one sec
13:32:39 gibi in the past we said we would at least like to see the client patches proposed (not merged) before we approve a microversion
13:32:45 gibi Uggla: awesome
13:32:49 sean-k-mooney ill be dropping soon but ill see if i have time to review before i do
13:33:32 opendevreview Kashyap Chamarthy proposed openstack/nova stable/ussuri: Add a workaround to skip hypervisor version check on LM https://review.opendev.org/c/openstack/nova/+/851224
13:34:22 Uggla gibi: https://review.opendev.org/c/openstack/python-novaclient/+/831651 and https://review.opendev.org/c/openstack/python-openstackclient/+/831902
13:34:33 gibi Uggla: thanks
13:35:03 Uggla gibi, there is also a related tempest test: 841088: Tempest test for checking unshelve to host | https://review.opendev.org/c/openstack/tempest/+/841088
13:35:16 gibi ack
13:38:26 opendevreview Kashyap Chamarthy proposed openstack/nova stable/train: Add a workaround to skip hypervisor version check on LM https://review.opendev.org/c/openstack/nova/+/851226

Earlier   Later