Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2018-04-11
16:21:23 mriedem dansmith: i've just been putting off going through it again because it's hairy on stable
16:21:31 mriedem i'm not opposed to it no
16:21:33 dansmith it's big,
16:21:37 dansmith but it doesn't seem that hairy
16:21:56 mriedem the fact it was re-introducing a regression in the earlier PS indicates to me it's hairy
16:22:05 dansmith it's checking a thing and raising an exception to avoid you killing data puppies
16:22:29 dansmith okay I guess I don't quite know what that means
16:22:55 mriedem https://review.openstack.org/#/c/543569/1/nova/virt/libvirt/driver.py@1216
16:23:03 mriedem meaning it's easy to break stuff
16:23:34 dansmith ah I see
16:23:42 mriedem anyway, i told lyarwood i'd go through it again last week and forgot
16:23:44 dansmith weyall.. data corruption seems more bad
16:23:55 mriedem and with no more eol, we have time to fix it if breaks something
16:24:16 dansmith by "time" you mean "forever and ever" right?
16:24:31 odyssey4me mriedem eh? what sucks for me? :)
16:35:06 openstackgerrit sahid proposed openstack/nova master: libvirt: move version to string in utils https://review.openstack.org/560455
16:35:38 mriedem dansmith: i'll trade you https://review.openstack.org/#/c/536722/
16:36:08 dansmith mriedem: seems a little lopsided
16:39:33 mriedem sahid: comments inline
16:41:43 openstackgerrit Merged openstack/nova master: Expose shutdown retry interval as config setting https://review.openstack.org/552483
16:44:26 dansmith mriedem: you okay with this? https://review.openstack.org/#/c/484997/14/nova/conf/libvirt.py
16:44:42 dansmith I suggested something similar, but not exactly this.. although this works and is easier than what I suggested
16:44:54 dansmith it gets us config load-time verification of that parameter
16:44:57 dansmith instead of at runtime
16:45:12 dansmith but I don't think we have anywhere else that we subclass an opt (even though that's suggested by the oslo docs AFAICT)
16:45:46 mriedem you just pulled me out of https://review.openstack.org/#/c/543569/
16:46:05 dansmith it needs only a glance
16:46:22 mriedem so does https://review.openstack.org/#/c/560454/
16:47:18 mriedem agree with load-time config validation
16:47:44 mriedem wouldn't it be simple to do a subclass inline in that module
16:47:45 mriedem ?
16:49:05 dansmith mriedem: wrong link?
16:50:53 mriedem no, i asked for a review on that earlier - since it's kicking things out of CI on master
16:51:28 dansmith I'm confused
16:51:56 dansmith mriedem: melwitt tssurya: do we need a cells meeting today? I have nothing new
16:51:58 mriedem you asked me to review lee's thing, then while i was doing that, you asked me to look at sahid's thing, so in kind i threw that bug fix at you while you were reviewing sahid's thing :)
16:52:06 dansmith and I reviewed some of tssurya's patches to make myself feel better this morning
16:52:16 mriedem i dont think i have anything
16:52:24 tssurya me niether
16:52:26 melwitt I was just about to ask that. I don't think I have anything either
16:52:33 tssurya dansmith: thanks for the reviews
16:52:46 mriedem i reviewed tssurya's patch to delete RPs and host mappings when we delete compute services but my comments weren't controversial
16:52:51 dansmith sweet, cancelled thusly
16:53:52 tssurya jaypipes: I am still working on the func test, haven't got very far (dangling between understanding service fixtures) , however I will try again tomorrow and ping you if it becomes too much for me
16:54:23 mriedem tssurya: i can probably help with that,
16:54:40 mriedem could be a patch before yours in the series to show that we didn't delete HMs and RPs when we deleted a compute service, and then yours could build on top to assert that we do
16:55:10 dansmith yeah I like that
16:55:18 dansmith I would call that "mriedem style"
16:55:20 mriedem it would also keep the fix patch smaller
16:55:26 mriedem like gangham style?
16:55:28 imacdonn I can certainly related to getting all tangled up in fixtures :/
16:55:35 imacdonn relate*
16:55:44 dansmith mriedem: similar, yeah
16:55:59 tssurya mriedem: that would be really awesome for me, writing the test_services from stratch is causing me a headache because its my first go
16:56:42 tssurya imacdonn: :)
16:59:37 imacdonn mriedem dansmith speaking of which ... I tried to implement that compute-is-configured-to-use-placement check based on auth_type, but I'm back in unit-test hell ... 27 tests that start a compute service trip on it .. and if I set auth_type in the conf fixture, a bazillion other tests fail ... some due to auth_url not set
17:04:27 openstackgerrit Eric Berglund proposed openstack/nova master: PowerVM Driver: Localdisk https://review.openstack.org/549300
17:08:22 mriedem dansmith: lyarwood: done https://review.openstack.org/#/c/543569/
17:10:57 mriedem dansmith: you should hit https://review.openstack.org/#/c/559987/ while we wait for lee to fix that up
17:11:27 dansmith ya
17:19:14 mriedem dansmith: left some replies in https://review.openstack.org/#/c/536722/ based on earlier discussions
17:19:31 mriedem as for the cleanup routine, i think some of that is going to have to just be impl trial and error, but i left some ideas
17:20:03 dansmith mriedem: how is it not a new rpc call?
17:20:19 dansmith oh because live_migration_abort() I guess?
17:20:22 mriedem yes
17:20:29 mriedem read the rest of my comment :)
17:21:26 mriedem there is one thing i'm not totally sure about, and it's that http://pythonhosted.org/futures/#concurrent.futures.Executor.shutdown says "If wait is False then this method will return immediately and the resources associated with the executor will be freed when all pending futures are done executing. Regardless of the value of wait, the entire Python program will not exit until all pending futures are done executing."
17:21:53 cfriesen melwitt: lyarwood: I'm seeing something weird on Pike: http://paste.openstack.org/show/718967/ Basically heat creates an instance and attaches volumes to it, then on stack teardown it runs into problems detaching the volume from the instance. Eventually the token times out and it retries, only this time libvirt doesn't actually recognize the specified device (making me wonder whether it had actually been detached at this
17:21:55 cfriesen point.)
17:21:57 mriedem however, if we do the done callback and Futures.cancel() thing i laid out, i think the pool will exit once we cancel those futures
17:22:05 dansmith mriedem: so the thing that sucks about that, IMHO,
17:22:20 dansmith is it's a cast, and we could check the status of the migration before we make that call, but it might be different when it would land on the compute and do its thing
17:22:34 dansmith which maybe isn't a problem because it's just less permissive not more
17:23:38 mriedem dansmith: yeah, worst case is the api races and then the compute ignores the abort request, because i think it's ultimately the libvirt driver that has to ack the abort request
17:23:39 dansmith mriedem: and we'd have to have the migration in rpcapi in order to know the status and whether or not we need to abort based on theversion we're pinned to
17:24:37 mriedem oh because we don't pass the migration record itself to live_migration_abort, just the id
17:24:51 dansmith right
17:25:07 mriedem hmm, well, we have to bump the compute service version anyway, so we can do that in the same version bump
17:25:29 melwitt cfriesen: I think you might need this https://review.openstack.org/#/c/515008
17:25:42 mriedem i was just looking that up :)
17:25:52 dansmith mriedem: well you don't actually have to change the rpc call, you just have to change the interface to the rpcapi method
17:26:03 dansmith mriedem: or you do it based on service version in compute/api before you get that far
17:26:13 cfriesen melwitt: looks promising, thanks
17:26:34 mriedem right?
17:26:34 mriedem dansmith: right so pass migration object to rpcapi method, if can't send new version, send migration.id
17:26:51 mriedem and if it can't send the new version and the migration status is not 'running' it should blow up
17:27:02 mriedem which i think avoids us needing to do an explicit service version check in the API method
17:27:11 dansmith mriedem: no, pass migration to rpcapi, and it always sends migration id, but raises if the version is old and state is not "running" or whatever
17:27:19 mriedem ok
17:27:19 mriedem ah yeah
17:27:29 mriedem but these are impl details right? does the spec need this level of detail?
17:27:41 mriedem i assume it will be me and you reviewing the code
17:28:10 dansmith mriedem: well, I dunno, it papered over a lot of this
17:28:41 mriedem ok, up to you if you want to ask for more detail
17:28:56 mriedem if you do, i'd say link this irc conversation in there so Kevin_Zheng has it
17:29:32 dansmith well, since it's not as big of a change as I was thinking (just clarifying we're re-using the same) I guess I can just go ahead and approve it and see if he'll come back around to add some stuff
17:30:49 openstack bugzilla.redhat.com bug 1505595 in openstack-nova "Nova assumes that a volume is fully detached from the compute if the volume is not defined in the instance's libvirt definition" [High,Modified] - Assigned to sferdjao
17:30:49 melwitt cfriesen: this is the redhat bz where we ran into the problem. I think it was during a similar thing, a heat stack delete failing at the detach step for "not found" https://bugzilla.redhat.com/show_bug.cgi?id=1505595
17:33:57 cfriesen melwitt: sounds even more promising. what do you think of the idea of heat detecting the failure-to-detach by polling the instance action events?
17:35:19 mriedem cfriesen: that's how i implemented the tempest test for extend volume - pool instance action events until the extend volume action is complete

Earlier   Later