| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-14 | |||
| 17:26:19 | openstackgerrit | Matt Riedemann proposed openstack/nova master: WIP: Add confirm_snapshot_based_resize_at_source compute method https://review.opendev.org/637058 | |
| 17:51:00 | openstackgerrit | Son Pham proposed openstack/nova-specs master: [WIP]Add blueprint snapshot-vms-with-state of-processes https://review.opendev.org/688522 | |
| 18:00:37 | openstackgerrit | Son Pham proposed openstack/nova-specs master: [WIP]Add blueprint snapshot-vms-with-state-of-processes https://review.opendev.org/686948 | |
| 18:01:31 | mriedem | bauzas: here are a set of queens changes that are good to go: https://review.opendev.org/#/c/676285/ https://review.opendev.org/#/c/675355/ https://review.opendev.org/#/c/682722/ https://review.opendev.org/680873 https://review.opendev.org/#/c/676500/ | |
| 18:11:21 | dansmith | mriedem: I guess you didn't squash that fixup patch into your second patch when you rebased just now eh/ | |
| 18:12:10 | dansmith | doesn't look like anything is running yet, so you could do that without losing much still I think | |
| 18:12:36 | mriedem | https://review.opendev.org/#/c/678951/ you mean? | |
| 18:13:27 | mriedem | at some point the thing it was FUP'ing ( https://review.opendev.org/#/c/627890/ ) had +2s on it a couple of times so i figured it was easier to not squash for those +2s to come back as a diff check | |
| 18:14:30 | mriedem | i can if you want, i'm sure at this point a diff isn't trivial for gibi or stephenfin anymore anyway | |
| 18:14:32 | dansmith | mriedem: yeah, but rebased on master now so there's a ton of churn in the patches from there now | |
| 18:14:41 | dansmith | yeah | |
| 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 | |