Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-10-14
18:14:49 mriedem none of this code is impacted much from master...
18:14:58 dansmith I think it makes sense to just do it now
18:14:58 mriedem well, this conductor task code
18:15:14 dansmith oh, fair I guess. I just eye-diffed the bottom one and there was a lot of churn, but true about this one
18:15:35 mriedem https://review.opendev.org/#/c/627890/53..59 it's not really trivial anyway
18:15:37 dansmith anyway, whatever, the reason for doing it was a +2 on there in August, so seems safe to squash now but whatever
18:15:49 mriedem yeah i will
18:16:13 mriedem i'll take most any excuse to take a break from reviewing these queens backports...
18:22:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Add prep_snapshot_based_resize_at_source compute method https://review.opendev.org/634832
18:22:14 openstackgerrit Matt Riedemann proposed openstack/nova master: Add PrepResizeAtDestTask https://review.opendev.org/627890
18:22:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Add finish_snapshot_based_resize_at_dest compute method https://review.opendev.org/635080
18:22:15 openstackgerrit Matt Riedemann proposed openstack/nova master: Add PrepResizeAtSourceTask https://review.opendev.org/627891
18:22:16 openstackgerrit Matt Riedemann proposed openstack/nova master: Execute CrossCellMigrationTask from MigrationTask https://review.opendev.org/635668
18:22:16 openstackgerrit Matt Riedemann proposed openstack/nova master: Add FinishResizeAtDestTask https://review.opendev.org/635646
18:22:17 openstackgerrit Matt Riedemann proposed openstack/nova master: Filter duplicates from compute API get_migrations_sorted() https://review.opendev.org/636224
18:22:17 openstackgerrit Matt Riedemann proposed openstack/nova master: Plumb allow_cross_cell_resize into compute API resize() https://review.opendev.org/635684
18:22:18 openstackgerrit Matt Riedemann proposed openstack/nova master: Handle target host cross-cell cold migration in conductor https://review.opendev.org/642591
18:22:18 openstackgerrit Matt Riedemann proposed openstack/nova master: Start functional testing for cross-cell resize https://review.opendev.org/636253
18:22:19 openstackgerrit Matt Riedemann proposed openstack/nova master: Add zones wrinkle to TestMultiCellMigrate https://review.opendev.org/643450
18:22:19 openstackgerrit Matt Riedemann proposed openstack/nova master: Validate image/create during cross-cell resize functional testing https://review.opendev.org/642592
18:22:20 openstackgerrit Matt Riedemann proposed openstack/nova master: Refresh instance in MigrationTask.execute Exception handler https://review.opendev.org/669012
18:22:20 openstackgerrit Matt Riedemann proposed openstack/nova master: Add negative test for cross-cell finish_resize failing https://review.opendev.org/643451
18:22:21 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Add confirm_snapshot_based_resize_at_source compute method https://review.opendev.org/637058
18:22:21 openstackgerrit Matt Riedemann proposed openstack/nova master: Add negative test for prep_snapshot_based_resize_at_source failing https://review.opendev.org/669013
18:22:22 openstackgerrit Matt Riedemann proposed openstack/nova master: Add confirm_snapshot_based_resize conductor RPC method https://review.opendev.org/637075
18:22:22 openstackgerrit Matt Riedemann proposed openstack/nova master: WIP: Add ConfirmResizeTask https://review.opendev.org/637070
18:33:23 dansmith mriedem: so that filter patch that added 2.80,
18:33:34 dansmith can I rebased that on master real quick since it needs a recheck anyway?
18:33:46 dansmith otherwise I can't run tests easily because it doesn't have the rpc changes
18:33:51 dansmith (which are in master already)
18:34:20 dansmith rebasing it will only take ten minutes away from its queue time and it's behind your massive set anyway
18:34:37 mriedem sure
18:34:42 mriedem i'll re-approve
18:35:32 openstackgerrit Dan Smith proposed openstack/nova master: Filter migrations by user_id/project_id https://review.opendev.org/674243
18:35:33 openstackgerrit Dan Smith proposed openstack/nova master: Add image caching API for aggregates https://review.opendev.org/687140
18:35:33 openstackgerrit Dan Smith proposed openstack/nova master: Add cache_images() to conductor https://review.opendev.org/687139
18:35:48 dansmith done
18:49:20 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix legacy issues in filter migrations by user_id/project_id https://review.opendev.org/682198
18:54:35 openstackgerrit Dan Smith proposed openstack/nova master: Add image caching API for aggregates https://review.opendev.org/687140
19:07:35 openstackgerrit melanie witt proposed openstack/nova master: Remove redundant call to get/create default security group https://review.opendev.org/688206
19:35:33 dansmith mriedem: question for you here: https://review.opendev.org/#/c/627890/60/nova/conductor/tasks/cross_cell_migrate.py@495
19:36:04 dansmith I didn't -1 in case you had a good reason or excuse, but figured it was worth pointing out if you hadn't thought about it
19:49:08 mriedem replied
19:49:28 mriedem i've got some negative tests for things failing higher in the stack but not the scenario you're asking about
19:49:31 mriedem it wouldn't be hard to add though
19:53:03 sean-k-mooney mriedem: while that would be good i think its also i higher bar of testing then we have of similarly hairy failure modes.
19:54:01 sean-k-mooney i have wondered in the past if we should have a delete --force and change delete to raise a error with --force when the vm is in task state that are potentially hard to clean up
19:55:29 sean-k-mooney the issue with that is force deleteion is somehting i proably woudl want only my admin to do sicne they are the ones that woudl have to clean it up but as a use if i dont want a thing any more i should be able to say that
19:55:40 sean-k-mooney espcially since im being billed for it in most cases
20:00:17 sean-k-mooney which reminds me we shoud get around to merging this so we dont leak vifs when we delete the vm while its spwaning https://review.opendev.org/#/c/663382/
20:06:50 sean-k-mooney dansmith: melwitt could one of ye review ^ efried since you looked at that ages ago if you get a chance that would be cool too
20:07:47 efried sean-k-mooney: are you +1 at this point?
20:08:11 sean-k-mooney im rereviewing it now to determind what matt changed
20:08:24 sean-k-mooney but it think it ws just the unit tests
20:08:53 mriedem trying to catch up, was on a call
20:08:55 sean-k-mooney but it was rebased so doing eye ball diff manually
20:09:45 mriedem so (1) sean-k-mooney you're saying fault injection testing per dansmith's request is a higher bar of testing than we usually have, right? if so i agree but i also agree we should do that kind of testing while it's fresh in our minds so i don't have a problem adding it
20:10:22 mriedem and (2) https://review.opendev.org/#/c/663382/ the last time i looked at that there were questions about how you can even get into that situation - did anyone (stephen?) sort that out?
20:10:27 sean-k-mooney mriedem: yes im not saying we should not do it just we normally have not been that proactive
20:11:12 sean-k-mooney i just notice that mdbooth updated it and so maybe that is a ping to early
20:11:33 dansmith mriedem: sorry didn't see you replied.. fault injection would be a tempest-level test, I'm more looking for a test where you make the save() delete the instance and then fail like save would have to make sure you call all the cleanups you expect
20:12:34 mriedem dansmith: maybe fault injection is a bad word but that's essentially what i proposed in my reply, stub resize_claim on the dest to (through the api) delete the instance from the source, which will trigger a failure in conductor and rollback the instance from the target cell db and the test would assert the various cleanups
20:12:40 sean-k-mooney mriedem: it looks like nothing substantive has change so i need to parse all the comments again to confim if there was still any outstanding questions
20:12:56 dansmith mriedem: ack
20:14:11 mriedem sean-k-mooney: the reply from stephen is the last time i looked at it https://review.opendev.org/#/c/663382/4/nova/compute/manager.py@2506
20:14:15 mriedem "open to ideas here"
20:14:46 mriedem if there is a way to recreate that issue, then i'd say write a functional test to recreate it and then lay the fix on top to show it's fixed
20:15:09 mriedem which goes back to our historically bad level of testing which is why we have requests for fault injection and more complicated test scenarios in my cross-cell resize series above
20:15:23 mriedem *bad level of testing for non-happy path scenarios
20:15:49 sean-k-mooney mriedem: right
20:16:05 sean-k-mooney which is exeactly why this bug laid dorment for so long
20:16:41 sean-k-mooney i know how the issue happend in the customer env
20:16:59 sean-k-mooney it would be hard to recreate but maybe not imposible
20:18:11 sean-k-mooney ill try an think about how to recreate in a functionla test although mdbooth might also be looking into that
20:20:20 mriedem note that it's not against the rules to have a functional test to recreate a bug which stubs something to inject the failure mode,
20:20:35 mriedem we do that and still do it with functional tests to make sure the api / service / db interaction is what we'd expect
20:21:01 sean-k-mooney ya i was wondign if i could use a spy or mock to inject the fault at the correct time
20:21:10 mriedem e.g. the test would do something like start creating an instance, stub something to then call the api to delete the instance, and then go back to spawning which would fail and make sure we deallocaten networks properly
20:27:57 mriedem sean-k-mooney: i left a comment to that effect on the review
20:28:49 sean-k-mooney cool
20:29:02 mriedem dansmith: so on https://review.opendev.org/#/c/627890/60/nova/conductor/tasks/cross_cell_migrate.py@495 - the functional tests start later in the series and in at least one or two cases gibi has asked a "what happens if x fails" question like you are here and i've added a test later in the series before he proceeds,
20:29:08 mriedem are you ok with that here as well?
20:33:49 dansmith mriedem: with adding it later you mean?
20:36:12 mriedem dansmith: no i mean i'd add it to the series and you'd hold off on +2ing that change until it's up and shows things are cool
20:36:23 dansmith sure
20:36:23 dansmith that's what I mean
20:36:35 mriedem alright
20:36:47 mriedem that'll be my tomrrow
20:36:49 mriedem *tomorrow
20:44:34 sean-k-mooney i think its https://github.com/openstack/nova/blob/stable/queens/nova/compute/manager.py#L2164-L2167
20:46:18 sean-k-mooney if we get the delete after we call _update_instance_after_spawn but before we call instance.save(expected_task_state=task_states.SPAWNING)
20:46:29 sean-k-mooney we will raise the unexpected task state error.
20:46:52 sean-k-mooney i could simulated tat by mocking _update_instance_after_spawn to raise it but it would be a bit of a hack
20:47:23 sean-k-mooney or maybe better
20:47:47 sean-k-mooney have _update_instance_after_spawn inject the delete
20:49:05 sean-k-mooney effefctly the race only happen in the interaval betwen reciving the libvirt event to notify that the spawn has completed and updating the db with that state
20:56:09 openstackgerrit Merged openstack/nova master: Deprecate [api]auth_strategy and noauth2 https://review.opendev.org/687373
20:56:16 openstackgerrit Merged openstack/nova master: Leave brackets on Ceph IP addresses for libguestfs https://review.opendev.org/649405
20:57:06 openstackgerrit Merged openstack/nova stable/queens: Fix wrong assertions in unit tests https://review.opendev.org/677390
21:05:48 sean-k-mooney no that is not the point were it happens... we handel the exception in that case.

Earlier   Later