Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-08-30
16:20:38 melwitt kashyap: fwiw I got the idea to do it that way partly from this, which is reading live config and defining domain with it https://github.com/openstack/nova/tree/master/nova/virt/libvirt#L7134-L7136
16:20:46 mriedem but i'm not sure how backed up the release team is on processing new release requests
16:20:48 mriedem smcginnis: ^ ?
16:21:03 smcginnis mriedem: stable releases?
16:21:24 smcginnis Oh, queens already.
16:21:29 kashyap melwitt: That link is a bit briken :-)
16:21:37 stephenfin mriedem: aha, well the docs don't fit into the style defined by that doc-migration thingy, if that matters. I doubt it does though
16:21:41 smcginnis mriedem: We were holding off until Pike wrapped up. Should be able to get that going now.
16:21:42 mriedem stephenfin: otherwise yes, osc-placement is nova-core for now https://review.openstack.org/#/admin/projects/openstack/osc-placement,access
16:21:42 kashyap melwitt: Meanwhile I'm double-checking with the author of the blockRebase() API, Eric Blake.
16:21:58 stephenfin cool. I'll take a look at that patch so
16:22:07 mriedem stephenfin: i want to move the osc-placement patches forward now because i want to use it in our ci post test hook
16:22:17 melwitt kashyap: gah, sorry. https://github.com/openstack/nova/blob/master/nova/virt/libvirt/driver.py#L7134-L7136
16:22:34 melwitt kashyap: sweet, thanks
16:22:41 stephenfin mriedem: yup, done
16:22:52 mriedem http://lists.openstack.org/pipermail/openstack-dev/2017-August/121618.html
16:23:11 kashyap melwitt: Yes, that approach looks correct to me
16:24:29 melwitt kashyap: my other concern is, if something goes wrong during the blockRebase or resize and an exception is raised, could reading the live config at the end possibly be corrupted and bad to define the domain with?
16:25:49 melwitt so I was wondering if we should save the original persistent config at the beginning like it was originally and write that back in case of exceptions, else write using the live config. or if it's safe to just write using the live config in the finally: block
16:27:14 kashyap melwitt: What is the error scenario you're looking w.r.t blockRebase()?
16:27:55 kashyap (Just to quickly recap: blockRebase() moves data from backing files into overlays.)
16:28:44 kashyap melwitt: Hmm, I was first about to suggest to save the domain config before undefining
16:28:51 melwitt kashyap: no specific scenario. just say if rebase raised an exception and we go to the finally: block and read the live config and define the domain with it, could we have potentially written a bad config
16:29:13 kashyap melwitt: Because, that's what we suggest manual users of the API on libvirt-users list. So that they have a copy of the guest definition at the _time_ of blockRebase()
16:29:49 melwitt yeah. that's what I was thinking, maybe save it first and then if an exception occurs write the original, else write the new one read from live
16:30:45 kashyap melwitt: Yes, that is more robust. Otherwise, re-defining from live config means, all the guest configuration until that point that the user was expecting to stay will be lost
16:30:49 kashyap Do you agree?
16:31:04 kashyap s/that the/the/
16:33:21 melwitt kashyap: I'm not sure TBH (whether anything would be lost). in the success scenario we need a way to reflect the changed volume in the persistent config. I don't know how to do that other than re-defining from live config
16:33:36 mdbooth melwitt: So I may have missed the boat here
16:33:37 kashyap melwitt: Okay, let's wait for Eric's response, I checked w/ him on OFTC before
16:33:43 kashyap Or mdbooth is here!
16:33:52 mdbooth melwitt: I'm looking at https://review.openstack.org/#/c/491630/3/nova/virt/libvirt/guest.py
16:34:15 mdbooth This function is really, really weird
16:34:55 mdbooth But iiuc it's purpose is to keep executing detach_device() until get_device_conf_func() doesn't return anything
16:35:21 mdbooth Except that we've over-complicated it by pulling out the initial execution of get_device_conf_func()
16:35:44 mdbooth We could throw away the top half of the function, including the code added in your patch, and I think it would achieve the desired result
16:35:57 mdbooth The bug only occurs because the first iteration is weird
16:37:08 mdbooth Also, there's no reason for that function to return a function, as both its callers simply execute it immediately
16:37:37 mdbooth Also, RetryDecorator over-complicates and already over-complex function
16:38:07 melwitt yeah. I'm guessing the reason it's like that is because if the detach from persistent fails, you don't want to retry that
16:38:27 melwitt live is the one it wants to retry for some time
16:41:13 mdbooth I double-checked, btw, that we're definitely retrieving the live xml desc (we are)
16:42:36 mdbooth melwitt: If that function is called and persistent=True, live=False
16:42:48 mdbooth And the device currently exists
16:43:26 mdbooth I think it will wait for timeout and return DeviceDetachFailed
16:44:22 melwitt I think there's no timeout in that case because it will raise on the first call (raise before the returned function is called)
16:44:40 mdbooth It won't raise on the first call if the device is present in persistent config
16:46:19 mdbooth Another weird thing there is that 'live' is pinned before it starts executing
16:46:38 mdbooth Whereas it may actually change during the call
16:46:51 melwitt it will raise on the first call if it fails to detach it
16:47:27 mdbooth So if we've got a persistent domain which is stopped, we'll have persistent=True, live=False
16:47:34 openstackgerrit Merged openstack/nova master: Reduce (notification) test duplication https://review.openstack.org/391428
16:48:05 mdbooth The first execution will succeed
16:48:17 mdbooth Actually... the second execution will fail because there's no domain
16:48:23 mdbooth We don't handle that
16:48:30 mdbooth So it'll likely explode
16:48:31 melwitt and then it'll try to detach the live even though live is False? oh
16:49:12 mdbooth Ah, no, we'll get instance disappeared
16:51:33 mriedem i haven't read scrollback, but did https://review.openstack.org/#/q/I8cd056fa17184a98c31547add0e9fb2d363d0908,n,z regress something?
16:51:52 mdbooth mriedem: I don't think so, no
16:52:01 kashyap melwitt: About your other reivew - 498983, you're right: to get the swapped volume reflected, of course you need to use the live config.
16:52:09 kashyap melwitt: And your test (noted in the review) proves it, too.
16:52:31 mdbooth mriedem: I'm just trying to convince myself it solves the problem
16:53:22 mriedem cells v2 meeting in 7 minutes
16:53:31 melwitt kashyap: thanks for the sanity check. I'm thinking I'll add to the patch save of the original XML to write back in the exception case, if rebase/resize fails
16:53:32 mriedem in #openstack-meeting-3
16:53:53 mdbooth melwitt: Do you happen to know what XMLDesc(0) returns for an inactive domain?
16:54:13 kashyap melwitt: Yep, saving the original XML to handle the failure path sounds correct.
16:54:45 melwitt mdbooth: you mean a stopped instance? no actually. I would also wonder what the rebase does in that case too
16:55:13 mdbooth I'm thinking in the context of detach_volume currently
16:56:00 mdbooth My feeling is that the use of the live flag is itself wrong
16:56:21 melwitt mdbooth: it solved the problem in a repro environment that I tested
16:57:05 mdbooth melwitt: Right, I just suspect it could be both simpler and more robust
16:57:18 mdbooth It looks like a haven for edge cases, and we closed one
16:58:37 mdbooth For eg, 'live' could change during the call
16:58:42 mdbooth Because the domain is stopped, for eg
16:58:45 melwitt mdbooth: sorry, that was in response to "trying to convince myself it solves the problem." I agree with you that there's probably a better way to refactor that function
16:59:07 melwitt it's bug prone for sure
16:59:48 openstackgerrit Eric Berglund proposed openstack/nova master: PowerVM Driver: config drive https://review.openstack.org/409404
17:00:11 mdbooth melwitt: Yeah, I'm mid-stream here :) Not entirely sure where I'm going with this.
17:00:45 dansmith mriedem: cells meeting?
17:00:46 melwitt I think you're just surveying the horror
17:00:47 mdbooth It's just such an over-complex function, I spent way too much time thinking about it already
17:06:06 kashyap melwitt: I know you're multi-tasking between two reviews. For later, some related notes I did a brain-dump on debugging blockRebase() -> _swap_volume() -- http://lists.openstack.org/pipermail/openstack-dev/2016-October/105158.html
17:06:40 kashyap melwitt: Even after staring at these libvirt / QEMU block APIs, damned if I fully wrapped my head around. Just when I think I got a little hang of something, there it comes ... another corner case that I didn't think of
17:09:39 andreaf jamespage: I have a change in Tempest that if merged would break some nova-lxd integration tests from the in-tree tempest plugin
17:10:28 andreaf jamespage: but I don't see those tests running anywhere in nova-lxd gate, so I was wondering if I can just change tempest and propose a patch to fix nova-lxd afterward?
17:23:29 jamespage andreaf: +1 that's good with me
17:23:55 jamespage andreaf: sorry for no response yesterday - just back from hols so catchup was a bit extreme
17:24:09 andreaf jamespage: cool - so is that right that tests are not running in any CI right now?
17:24:32 andreaf jamespage: np thanks for your reply
17:24:39 jamespage andreaf: we have a tempest devstack gate, but I don't think the in-tree tests actually get executed
17:25:27 andreaf jamespage: yeah that was what I found as well
17:28:47 openstackgerrit Elod Illes proposed openstack/nova master: WIP: delete after failed evac https://review.openstack.org/499237
17:29:50 melwitt kashyap: cool, thanks for the link
17:31:24 openstackgerrit Ed Leafe proposed openstack/nova master: WIP - Add ${Destination} and ${Destination}List objects https://review.openstack.org/499239
17:31:24 openstackgerrit Ed Leafe proposed openstack/nova master: Add alternate hosts https://review.openstack.org/486215
17:37:58 mriedem alaski: did you or someone else at some point have a doc with the known gaps in what we track for reporting instance action events?
17:38:02 mriedem i could have sworn you did

Earlier   Later