Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2022-07-27
10:31:46 kashyap Hehe
10:31:54 kashyap Thank you!
10:32:32 kashyap bauzas: Can you also ACK the backports, then, please?
10:32:43 bauzas I can
10:34:16 kashyap Thx
10:34:35 kashyap sean-k-mooney[m]: Unrelated - I see this is still active, can you pls ACK this too: "[nova/libvirt] Support for checking and enabling SMM when needed
10:34:38 kashyap "
10:34:39 kashyap https://review.opendev.org/c/openstack/nova/+/849610
10:35:02 kashyap (Cc: gibi)
10:36:42 opendevreview Kashyap Chamarthy proposed openstack/nova stable/wallaby: Add a workaround to skip hypervisor version check on LM https://review.opendev.org/c/openstack/nova/+/851206
10:40:28 sean-k-mooney kashyap: i wasnt going to review teh wallaby backport until the xena one had merged
10:40:35 sean-k-mooney https://review.opendev.org/c/openstack/nova/+/849676
10:40:36 kashyap Sure
10:40:45 kashyap But it's the same thing
10:41:09 sean-k-mooney yes but it cant merge until that one does so i review them in order
10:41:12 kashyap Sigh, also this stable/yoga backport is not merged yet - https://review.opendev.org/c/openstack/nova/+/845045 (libvirt: Add a workaround to skip compareCPU() on destination
10:41:15 kashyap )
10:41:23 kashyap sean-k-mooney: Oh, sure
11:44:54 opendevreview Merged openstack/nova master: Allow unshelve to a specific host (Compute API part) https://review.opendev.org/c/openstack/nova/+/831507
11:45:01 opendevreview Merged openstack/nova master: Allow unshelve to a specific host (REST API part) https://review.opendev.org/c/openstack/nova/+/845897
11:45:06 gibi \o/
11:46:51 sean-k-mooney stephenfin: gibi how do we want to proceed with the mock change?
11:46:58 gibi sean-k-mooney: good question
11:47:13 sean-k-mooney im kind of sad that we chose to delay it last cycle
11:47:16 sean-k-mooney to drop 3.6
11:47:18 gibi sean-k-mooney: I'm not sure I will have enough time today to try to fix the duplicate mock issue
11:47:27 sean-k-mooney and now we added 3.10 which is causing other issues
11:47:40 gibi I don't know if stephenfin looked at it yet or not
11:49:19 gibi I can be convinced to let it land and fix the 310 issue separately I just affraid that we will forget about it and only realize it again when we want to make the py310 job votinh
11:49:23 gibi g
11:50:18 sean-k-mooney ya thats a valid concen
11:50:32 sean-k-mooney the 3.10 fixes shoudl be a seperate patch in my opipion
11:50:41 sean-k-mooney it would be nice ot have it in the seriese
11:50:42 gibi I agree with that
11:50:49 sean-k-mooney but not sure how much work it is
11:51:02 sean-k-mooney im also not partically happy with the decision they made in 3.10
11:51:03 gibi me neither but I feel like it is not a oneliner
11:51:31 sean-k-mooney double mocking is perhaps ineffenct but i think it should be allowed
11:51:48 sean-k-mooney so im sad they made that breakign change
11:52:32 gibi lets wait for stephenfin to chime in. Also I can try to look into it tomorrow to see how big is the issue
11:52:36 gibi then we can reconsider
11:52:48 sean-k-mooney we might need to add a helper funciton that check if its mocked and only create a new mock if not
11:52:59 gibi I would go and remove the duplicates if possible
11:53:08 gibi but I don't know how hard that will be
11:53:17 sean-k-mooney yes if we can do that once
11:53:25 sean-k-mooney then we will never hit this again because it will fail
11:53:35 gibi the helper feels like a coverup
11:54:00 sean-k-mooney im just worried about loading the context of is this mocked
11:54:27 sean-k-mooney again with the new behaivor it shoudl be obvious i guess because it will fail
11:54:34 gibi yeah
11:56:48 sean-k-mooney i just hope this does not cause shared global state
11:57:11 sean-k-mooney the fixture should be set up for every test
11:57:37 sean-k-mooney but we likely will have to replace part of them in the test where its double mocking
11:58:02 sean-k-mooney as i feel like we double mock somethimes when the fixture does not provide exactly what we want
11:58:09 sean-k-mooney i know i have done that at least once
11:58:32 gibi yes, I think that is the "normal" case when we double mock. I did that before
11:58:42 gibi so in this case the fixture should expose the mock
11:58:54 gibi so the test case can use it instead of remocking it
11:59:18 sean-k-mooney well we can jsut create a new mock object and assign it
11:59:27 sean-k-mooney instaed of having it patch it again
11:59:40 gibi we cannot create the new Mock as autospec will fail
11:59:54 gibi as it will try to spec what is already mocked
12:00:08 sean-k-mooney we can with mock.Mock()
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

Earlier   Later