Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-07-28
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: func: Add CinderFixture to _IntegratedTestBase https://review.opendev.org/743535
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: compute: Don't delete the original attachment during pre LM rollback https://review.opendev.org/743319
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: Add regression tests for bug #1889108 https://review.opendev.org/743289
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
16:00:55 stephenfin the name is poor, admittedly
16:14:52 artom stephenfin, doh :( A ctrl-] would not have been that hard, on my part
16:15:39 sean-k-mooney what does ctrl-] do
16:15:43 sean-k-mooney a find?
16:15:48 sean-k-mooney got do definition
16:15:53 artom Go to definition, in vim with ctags
16:15:59 sean-k-mooney ah ok
16:16:15 artom Admittedly, it sometimes messes up if there are multiple methods called the same thing
16:22:22 openstackgerrit Stephen Finucane proposed openstack/nova master: Track CPU scheduler policy during live migration https://review.opendev.org/743588
16:22:37 stephenfin artom: follow-up there, if you're feeling generous
16:25:19 artom stephenfin, so... does it really make sense to do this, given we only support 1 policy now?
16:27:16 stephenfin Maybe. Maybe not. Given how trivial it is to do, I'd rather knock it off now than have to worry about it when we really do need it
16:27:41 stephenfin at that point we'd be down to needing service version checks, or whatever the new hotness is by then
16:29:01 artom stephenfin, so... this would be for a case where the scheduler is set on a per-host basis?
16:29:18 artom And we want to support live migration between different hosts/schedulers?
16:29:32 artom Wouldn't we do it as an image property/flavor extra spec?
16:30:29 sean-k-mooney stephenfin: given we dont support other schduler polcies and dont plan to intoduce that in the near term do we really need this cange
16:30:30 stephenfin if we did have different scheduler policies they'd likely be done via an image prop/extra spec, yes
16:30:52 sean-k-mooney stephenfin: right but we dont plan on allowing user to request that
16:31:23 artom stephenfin, so if it's set in the flavor, do we need this change?
16:31:34 artom As you can see, I'm having trouble being convinced :)
16:31:46 sean-k-mooney if it was in the flavor or image no
16:31:57 sean-k-mooney we woudl only need it if it was set by a host level config option
16:32:17 stephenfin artom: I'm confused. What would change?
16:32:17 artom sean-k-mooney, yep, and I think you and I would be against such a thing
16:32:28 artom Precisely because it would require stephenfin's patch :P
16:32:52 artom stephenfin, I'm saying - if the scheduler is set via flavor extra spec, we don't need to update it as part of the live migration XML update
16:33:02 sean-k-mooney stephenfin: i guess the issue is right now i dont think your change adds anything useful
16:33:15 artom Because the destination host should be able to accept the existing scheduler
16:33:27 artom (Presumably because we've scheduled to it because of a trait or something)
16:33:35 sean-k-mooney we have no plans to expose sched_scheduler to the user or operator as something that can be set in anyway
16:33:52 stephenfin artom: oh, perhaps the difference here is that I'm saying we shouldn't be _updating_ the XML elements
16:34:09 stephenfin we should be regenerating them based on authoritative sources that tell us what they should be set to
16:34:16 stephenfin I figured the value of doing this is to provide such an authoritative source for what the scheduler policy should be

Earlier   Later