Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-08-30
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: Add alternate hosts https://review.openstack.org/486215
17:31:24 openstackgerrit Ed Leafe proposed openstack/nova master: WIP - Add ${Destination} and ${Destination}List objects https://review.openstack.org/499239
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
17:39:29 alaski I may have. You might be thinking of a spec opened by rosimata a while back looking to improve instance actions which listed some of the things it missed.
17:40:57 mriedem i also found https://blueprints.launchpad.net/nova/+spec/improve-instance-action-events-2 and something it depends on
17:41:17 alaski https://review.openstack.org/#/c/256743/ maybe
17:41:44 alaski https://review.openstack.org/#/q/project:openstack/nova-specs+owner:%22Brian+Rosmaita+%253Crosmaita.fossdev%2540gmail.com%253E%22
17:42:33 mriedem yup https://review.openstack.org/#/c/256743/6/specs/newton/approved/expand-instance-actions-coverage.rst it is
17:42:38 mriedem i remember seeing that and commenting on it
17:42:39 mriedem thanks
17:43:14 alaski np, glad to help
17:47:10 openstackgerrit Merged openstack/osc-placement master: Fix the bug link in the readme https://review.openstack.org/499206
17:50:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Hyper-V: Perform proper cleanup after cold migration https://review.openstack.org/486955
18:24:55 openstackgerrit Matt Riedemann proposed openstack/nova master: Cleanup allocations on invalid dest node during live migration https://review.openstack.org/498861
18:24:56 openstackgerrit Matt Riedemann proposed openstack/nova master: Refactor LiveMigrationTask._find_destination https://review.openstack.org/498874
18:44:43 mriedem huh, hitting a 500 in the PUT /allocatoins API
18:44:48 mriedem */allocations
18:44:58 mriedem index error shenanigans
18:45:58 mriedem Sending updated allocation [{'resource_provider': {'uuid': '7ab9dab7-65c6-4961-9403-c8fc50dedb6b'}, 'resources': {}}] for instance dc8a686c-ad92-48f3-8594-d00c6e671a1c after removing resources for 7ab9dab7-65c6-4961-9403-c8fc50dedb6b
18:46:57 mriedem remove_provider_from_instance_allocation thinks we're doing a resize to same host because there is only one compute provider because of the bug where we force the evacuate host and don't have allocations on the dest node
18:47:54 mriedem funny the jsonschema validation in PUT /allocations doesn't fail on resources being an empty dict
18:48:54 mriedem oh i guess we dont have schema validation for that

Earlier   Later