Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-07-28
11:28:04 stephenfin so we're on the same page there
11:28:23 stephenfin however, 'list' also isn't useful
11:28:37 sean-k-mooney well that is actully the fully correct type
11:28:51 stephenfin Unless it's truly a generic container for anything, then I want to know what's in it
11:28:53 sean-k-mooney its technically ty.Dict[str,ty.Union[str, ty.Dict[str, ty.Union[str, ty.List[str], None]]]]
11:29:27 stephenfin I don't think I've written anything approaching that level of detail anywhere
11:29:34 sean-k-mooney ty.Dict is an alis for dict which is treated as ty.Dict[Any, Any]
11:29:44 sean-k-mooney you havent
11:29:46 whoami-rajat hi #openstack-nova , is there a way to use pdb in n-cpu service (DEVSTACK), i tried starting the service manually on terminal but it doesn't stop on the pdb
11:29:57 stephenfin and I'm not suggesting we do that here either
11:30:06 sean-k-mooney but that is what i would hav too change https://review.opendev.org/#/c/739131/8/nova/virt/libvirt/driver.py@6909 too
11:30:41 sean-k-mooney whoami-rajat: that is because we use eventlets
11:31:00 stephenfin nah, I didn't say that. I just said what was there was wrong. 'ty.Dict[str, ty.Any]' would work perfectly fine
11:31:05 sean-k-mooney to use it with pdb you have to disable monkeypatching of the threading api
11:31:06 stephenfin or ty.Dict, if you prefer
11:32:00 sean-k-mooney right so i have been using dict which is the same as ty.Dict which is ty.Dict[ty.Any, ty.Any]
11:32:00 whoami-rajat sean-k-mooney, ah, does disabling it has any side effect?
11:32:21 sean-k-mooney whoami-rajat: unfortunetly yes, we actullly have a couple of infinet loops
11:32:37 sean-k-mooney we normally only exit them via yeilding
11:32:58 sean-k-mooney so in practic it is quite hard to debug n-cpu directly
11:33:12 whoami-rajat :( , is rpdb a solution here? sean-k-mooney
11:33:28 sean-k-mooney i have never used that
11:33:58 stephenfin sean-k-mooney: Okay, then I retract my request not to use 'dict' in that case then
11:34:04 whoami-rajat ack. thanks for the info sean-k-mooney
11:34:20 stephenfin I'd still rather we did use 'ty.Dict[str, ty.Any]', since that's at least a little more information than we might otherwise have
11:34:24 stephenfin but it's not necessary
11:35:03 sean-k-mooney oh am well we can run with remote debuging but basically you have to deal with either eventlet monkey patching which means breakpoint might not work or disable monkey patch whcih means you might not get to the code you want
11:35:22 stephenfin I think my comments on the other places that 'ty.Any' is still used are valid though, since they're easy to type and actually helpful
11:35:28 sean-k-mooney stephenfin: ill go make most of the change you asked for
11:35:38 sean-k-mooney and then when the type get a bit too long
11:35:45 sean-k-mooney ill do type erasure
11:36:00 sean-k-mooney i can do ty.Dict[str, ty.Any]
11:36:09 stephenfin sounds good to me
11:36:33 sean-k-mooney to me by the way needing to do ty.Dict[str, ty.Any] is kind fo a code smell
11:37:06 sean-k-mooney either we should be returning classes rather then dicts
11:37:30 sean-k-mooney or we shoudl be breaking down the function as it likely has too many differnt posable return types
11:37:41 stephenfin totally agree
11:38:27 sean-k-mooney am other then typing and the comment you left
11:38:33 sean-k-mooney are you happy with the change other then that
11:38:56 sean-k-mooney i am hoping this will be the last revision so i can move on to the numa in placemnt stuff for a while
11:39:25 stephenfin I do think we could fold some of the functions into each other, since the separation feels a bit artificial rn, but I've said as much in the review and it's easy fix later
11:39:32 stephenfin so yeah, lgtm otherwise
11:40:03 sean-k-mooney ok im also planning to backport this at least to train so i was trying to keep this relitivly small
11:40:39 stephenfin Maybe drop the type hints or put them in a separate patch in that case? :)
11:41:00 sean-k-mooney well we had this debate a few versions ago
11:41:35 sean-k-mooney that we did not want to have to write worse code with out type hint jsut because we wanted to backport
11:41:50 stephenfin fair point
11:42:27 sean-k-mooney so im hesitent to set that precident as if i do ill be asked to do it for every bugfix
11:43:14 sean-k-mooney at which point its much less motivating to ever write type hints and i used them while writing the patch a few times so they are really useful IMO
11:43:37 sean-k-mooney e.g. i forgot what i was passing an looked at the type hint
11:56:00 sean-k-mooney stephenfin: by the way im not going to do it in this patch but what are your feeling on doing ty.Dict vs importing Dict directly and just using it
11:56:12 sean-k-mooney stephenfin: you seamed to be doing ty.<whatever>
11:56:29 sean-k-mooney so that is what im doing but the ty. gets a little tedious after a while
11:57:27 sean-k-mooney most of the examples i have seen do "from typeing import Dict,List,..."
11:58:18 sean-k-mooney long term do you think we should stick with import typeing as ty or move over to the other from typing style
12:46:34 lyarwood stephenfin: so I've actually managed to hit the same issue with live_migration as pre_live_migration in my func tests btw, looks like I also need the fix to cover this case
12:48:20 lyarwood oh wait I borked the test
12:48:30 lyarwood OS_DEBUG++
12:53:09 sean-k-mooney OS_DEBUG is for what disabling monkeypatching or extra output
12:53:22 sean-k-mooney its the latter right?
12:54:08 sean-k-mooney when running test with tox
12:54:13 lyarwood latter, had a typo in a mock and was confused until I saw the extra output
12:54:36 sean-k-mooney ya i have used it once or twice
12:54:48 sean-k-mooney i normally got to pdb instead
13:00:19 jsuchome lyarwood: https://review.opendev.org/#/c/743220/ is green, hooray!
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: Add regression tests for bug #1889108 https://review.opendev.org/743289
13:22:52 openstack bug 1889108 in OpenStack Compute (nova) "failures during driver.pre_live_migration remove source attachments during rollback" [High,In progress] https://launchpad.net/bugs/1889108 - Assigned to Lee Yarwood (lyarwood)
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/743319
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: func: Add live migration rollback volume attachment tests https://review.opendev.org/743534
13:22:52 openstackgerrit Lee Yarwood proposed openstack/nova master: func: Add CinderFixture to _IntegratedTestBase https://review.opendev.org/743535
13:23:34 lyarwood jsuchome: ack, I'll sort the actual test for that out later this eveningn
13:32:55 jsuchome thanks
13:41:36 openstackgerrit Alexandre Arents proposed openstack/nova master: Make _rebase_with_qemu_img() generic https://review.opendev.org/743537
14:52:02 gmann brinzhang0: let me know if you need help to introduce microverison in cyborg there are common code we can share from nova (copy for now but we should have it somewhere in common lib like oslo etc)
15:12:05 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Add new default roles in volumes policies https://review.opendev.org/742777
15:16:12 gmann stephenfin: thanks for all review on policy patches. ^^ updated one. rest nit i will fix if i need to re-spin let me know if anything you want me to fix in same commit.
15:22:48 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Add scope and new default roles in extensions policies https://review.opendev.org/743046
15:35:18 stephenfin gmann: ack
15:41:41 kashyap stephenfin: Now I know why you deferred that live migration options doc :D the sheer amount of scouring through the code to get the "web of relations" :D
15:42:06 kashyap More seriously, I've been looking through history and code reading to figure out WTH is going on
15:42:36 kashyap Either way, 'fun'
15:43:43 openstackgerrit Alexandre Arents proposed openstack/nova master: Rebase qcow2 images when unshelving an instance https://review.opendev.org/696084
15:43:58 stephenfin kashyap: I assume that means you've left something somewhere that I should go read? :)
15:44:23 kashyap stephenfin: Not yet; just writing an additional comment, besides what I wrote yesterday
15:44:55 kashyap stephenfin: I might need a better phrasing of the existing one from the author. It took me a few careful readings to parse it :D
15:45:34 stephenfin Aha, gotcha
15:45:40 stephenfin Looking forward to it :)
15:45:54 openstackgerrit Stephen Finucane proposed openstack/nova master: Handle multiple 'vcpusched' elements during live migrate https://review.opendev.org/743568
15:46:35 stephenfin artom: When you've a chance, could you sanity check that for me?
15:47:00 stephenfin I'll poke bauzas or lyarwood to review later
15:47:40 artom stephenfin, for sure
15:48:06 stephenfin ta
15:48:55 artom stephenfin, also, aren't you glad for those LOG.debug calls with the full XML dump? :)
15:49:18 stephenfin for sure
15:49:21 stephenfin logging ftw
15:50:20 kashyap stephenfin: Here, right now I don't have a better phrasing; you might say it's not even required, let's see -- https://review.opendev.org/#/c/741473/1/nova/conf/libvirt.py@261
15:54:11 openstackgerrit Ghanshyam Mann proposed openstack/nova master: Pass the actual target in volumes policy https://review.opendev.org/742779
15:57:15 artom stephenfin, seems sane, left a note
16:00:35 stephenfin artom: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L66-L67
16:00:43 stephenfin https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L53-L59

Earlier   Later