Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-08-30
15:40:21 openstackgerrit Matt Riedemann proposed openstack/osc-placement master: Fix the bug link in the readme https://review.openstack.org/499206
16:07:32 melwitt mdbooth, kashyap: I'd appreciate it if one of you could sanity check a patch of mine where I'm trying to save an updated domain XML after a volume swap https://review.openstack.org/#/c/498983
16:07:43 kashyap melwitt: Hi
16:07:51 kashyap Will check
16:08:33 melwitt thanks kashyap. the concern on the patch is whether it's correct to read the live config and use it to redefine the domain, or if that has any potential pitfalls
16:09:14 kashyap melwitt: Noted. I'd normally check w/ danpb on the live config thing, but let me see if I can figure it out first :-)
16:13:30 stephenfin mriedem: Is osc-placement something we're supporting? I was going to fix the docs for doc-migration a while back, but there's only one commit and it was 5 months ago
16:14:17 melwitt kashyap: k. I tested the patch locally with devstack and it seemed to work fine, i.e. the instance soft-rebooted successfully and I see the domain in 'virsh list'
16:14:44 sdague stephenfin: I thought mriedem was going to trigger a release
16:14:48 sdague it's not released yet
16:14:56 kashyap melwitt: Good. But just to note -- the blockRebase() API behaviour documented is still true
16:15:30 stephenfin sdague: Oh, maybe that's it. Wonder if the docs need to be fixed at some point so?
16:16:00 kashyap melwitt: Please report your testing there. I'll comment in a few
16:16:24 kashyap melwitt: Ah, you _did_ report
16:16:36 melwitt kashyap: right. I understand from that comment that the domain has to be undefined first. the sanity check I'm looking for is whether redefining it by reading the live config is "ok"
16:17:03 melwitt the old code was reading the persistent config before the volume update and then just writing it back
16:17:05 kashyap melwitt: Yes, for blockRebase() that's the prerequisite.
16:17:13 kashyap "that" being undefininng the domain
16:17:22 mriedem huh?
16:17:24 mriedem what release?
16:17:47 kashyap mriedem: Is that question ot me, or to stephenfin?
16:19:30 mriedem neither
16:19:31 kashyap melwitt: Yes, using live config is correct. And, Nova in general anyways reconstructs the instance guest XML, that's also why Nova doesn't allow people editing guest XML randomly -- it needs to be explicitly modelled into NOva
16:19:36 mriedem to sean diggity dague
16:19:49 mriedem stephenfin: sdague: i have a release request up
16:20:32 mriedem https://review.openstack.org/#/c/498924/
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

Earlier   Later