| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-10-22 | |||
| 12:23:30 | gibi | efried: that will be harder I think | |
| 12:23:32 | efried | been at least three or four cases that could have made use of it. | |
| 12:23:51 | efried | since it was proposed | |
| 12:26:32 | gibi | efried: I think I saw two of those cases. It think not everybody sees the same value we thing the spy adds | |
| 12:27:21 | efried | gibi: I'm almost convinced it should be added to the mock library itself. If we did that, we could probably iron out those last pesky wrinkles that are annoying us... | |
| 12:40:37 | openstackgerrit | Alex Xu proposed openstack/nova master: Add test for the race between post live migration and update available resource https://review.opendev.org/690066 | |
| 12:41:16 | alex_xu | efried: ^ I feel the race is case due to the post live migration, not the pre live migration | |
| 12:41:39 | efried | alex_xu: ack, I was about to get around to trying to grok your comment :) | |
| 12:42:12 | efried | Did you see the repro though? That triggers the race by running the periodic before the dest claim. | |
| 12:42:53 | alex_xu | efried: yes, but I think that is due to the no numa instance without migration context, I feel it isn't race problem | |
| 12:43:32 | alex_xu | efried: we assign migration.dest and instance.migration_context under the same lock, so it shouldn't happen race | |
| 12:43:46 | efried | Only on cold migration | |
| 12:44:01 | efried | on live migration the former happens on the conductor, the latter on the compute. | |
| 12:44:02 | alex_xu | oh, I only see LM, didn't look at cold one | |
| 12:44:43 | alex_xu | but it should be same, since both cold migration and LM using same move claim | |
| 12:46:34 | alex_xu | efried: here is https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L306 and https://github.com/openstack/nova/blob/master/nova/compute/resource_tracker.py#L370 | |
| 12:49:54 | efried | alex_xu: cold migration uses _claim_existing_migration but live migration uses _create_migration -- or the other way around, now I can't remember :( | |
| 12:50:32 | alex_xu | really...i will hate that | |
| 12:51:47 | alex_xu | efried: quick grep the code, both cold and live using _claim_existing_migration | |
| 12:51:59 | alex_xu | they create the migration obj in conductor | |
| 12:52:09 | efried | alex_xu: If you can, probably best to wait for mriedem -- he understands these paths way better than I do. | |
| 12:52:39 | alex_xu | efried: yea, no hurry, whatever the fix is right | |
| 12:52:47 | efried | I put together these patches to further my understanding, but he was leading the way | |
| 12:53:40 | efried | Like, mission accomplished, I understand things a little bit better than I did before, but it's still like |...me..................mriedem..| | |
| 12:55:22 | efried | btw, I ran into the same COMPUTE_RESOURCE_SEMAPHORE deadlock while I was experimenting yesterday, and considered working around it by manually calling _populate_assigned_resources -- but I didn't think that was a realistic test. | |
| 12:56:41 | alex_xu | yea...that deadlock spend hours for me, totally not worth... | |
| 12:59:33 | efried | originally I was using a cold migration and injecting update_available_resource before _prep_resize, but that didn't trigger the problem. So I tried to inject it lower down, after _claim_existing_migration, and ran into the deadlock. | |
| 13:00:05 | efried | That's when mriedem suggested I instead use a live migration and inject before pre_live_migration instead. | |
| 13:00:14 | efried | That reproduced the race | |
| 13:00:24 | efried | or... *a* race, anyway :) | |
| 13:13:12 | bauzas | sean-k-mooney: ah sorry, was at the gym, will review it ASAP | |
| 13:16:19 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Fix policy doc for host_status and extended servers attribute https://review.opendev.org/689833 | |
| 13:16:56 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Fix policy doc for host_status and extended servers attribute https://review.opendev.org/689833 | |
| 13:16:57 | mriedem | hear ye hear ye we need stable/stein reviews https://review.opendev.org/#/q/status:open+project:openstack/nova+branch:stable/stein | |
| 13:17:11 | bauzas | mriedem: ack | |
| 13:17:19 | bauzas | sean-k-mooney: urgent revert, I guess ? | |
| 13:17:28 | bauzas | sean-k-mooney: I can fast-approve it as per our policy | |
| 13:21:10 | sean-k-mooney | bauzas: its not a pure revert | |
| 13:21:35 | sean-k-mooney | e.g. we updated the comments to reflect our understanding of why we cant make the chagne | |
| 13:22:03 | sean-k-mooney | bauzas: i also want to see the meloxn ci run an pass on it | |
| 13:22:26 | bauzas | sean-k-mooney: ergh ok | |
| 13:22:42 | bauzas | I'd have preferred a classic revert so we could spin another change | |
| 13:22:57 | bauzas | anyway, it's done | |
| 13:23:20 | sean-k-mooney | well we could just do a strait revert | |
| 13:23:40 | sean-k-mooney | and then apply this after on master to fix the comments | |
| 13:24:16 | sean-k-mooney | without the patch we get a traceback in nova while a vm is running with a macvtap interface | |
| 13:24:30 | sean-k-mooney | it does not break the vm | |
| 13:24:50 | sean-k-mooney | one of the periodic task that track resouce usage is unhappy | |
| 13:27:14 | bauzas | cool | |
| 13:44:59 | mriedem | gmann: in a multinode job, was is the "peers" group for hosts? and why would that need to be configured over just the "subnode" group vars to get configuration into the subnode compute? | |
| 13:45:57 | gmann | mriedem: i think that was old name i remember, later we did 'peers' -> 'subnode' | |
| 13:48:25 | mriedem | ok i was wondering why we have both subnode and peers configured in some multinode jobs and it sounds like we could just move all the 'peers' config to 'subnode' vars config | |
| 13:49:06 | mriedem | e.g. https://github.com/openstack/nova/blob/master/.zuul.yaml#L340-L352 | |
| 13:50:23 | gmann | let me check if any other reason to keep that. | |
| 13:51:30 | openstackgerrit | Matt Riedemann proposed openstack/nova master: Make nova-next multinode and drop tempest-slow-py3 https://review.opendev.org/683988 | |
| 13:57:19 | mriedem | efried: want to do the backports for https://review.opendev.org/#/c/689842/ and the func test patch below it? | |
| 13:59:28 | mriedem | sean-k-mooney: i'm assuming you'll be backporting this revert to train yeah? https://review.opendev.org/#/c/675776/ | |
| 14:00:02 | mriedem | not sure why that wasn't flagged for train-rc-potential | |
| 14:03:53 | mriedem | so we have 2 train regressions identified and fixed already, we should probably do a patch release soon | |
| 14:04:47 | mdbooth | This is weird. utils.synchronized(instance.uuid) seems to be ignored in my functional test | |
| 14:04:55 | sean-k-mooney | mriedem: yes i can do that if adrianc does not | |
| 14:05:24 | mdbooth | i.e. I'm hitting breakpoints in critical sections locked by the same key concurrently | |
| 14:05:39 | sean-k-mooney | mdbooth: we might be mocking the lock | |
| 14:05:51 | sean-k-mooney | or the file acess | |
| 14:06:07 | mdbooth | sean-k-mooney: Yeah, I was hoping somebody would shout if that was the case :) However, that's definitely where I'm heading next. | |
| 14:06:17 | mdbooth | But why would we do that? | |
| 14:06:22 | mriedem | https://github.com/openstack/nova/blob/master/nova/test.py#L239 | |
| 14:06:52 | sean-k-mooney | mdbooth: we make most things serial in the fucntional tests | |
| 14:06:54 | mdbooth | mriedem: Thanks! Why would you ever turn that off, btw? | |
| 14:07:07 | sean-k-mooney | so i guess locking is not always needed | |
| 14:07:10 | mdbooth | Except if you didn't want it to work | |
| 14:07:19 | mriedem | see the note above the condition | |
| 14:08:15 | mdbooth | mriedem: Guessing that was written in the days before functional/ ? | |
| 14:08:17 | sean-k-mooney | oh i guess it would also cause issues because we use the same instance uuids | |
| 14:08:28 | sean-k-mooney | e.g. for parallel test executions on the same host | |
| 14:08:48 | mdbooth | Because non-test code uses locks, and we're not going to 'fix' that code to not use locks | |
| 14:08:55 | mdbooth | Just so we can execute it from a test | |
| 14:10:44 | mdbooth | mriedem: For reference, the locks which are being ignored are the locks in _do_build_and_run_instance, and terminate_instance in ComputeManager | |
| 14:11:03 | mriedem | mdbooth: it's not before functional tests existed | |
| 14:11:07 | mdbooth | My test assumes that the locks works as expected. It would be weird to expect otherwise. | |
| 14:12:02 | sean-k-mooney | mdbooth: most fucntional tests assume that there are not cuncuretn action taking place | |
| 14:12:06 | mdbooth | It's kinda hard to write a test for a delete race when locks have different semantics in tests. | |
| 14:12:18 | sean-k-mooney | since all the service runn in the same interperter instnace | |
| 14:12:30 | mdbooth | Right. You can still have locks, though. | |
| 14:12:37 | mriedem | i would prefer that we don't have tests that have to rely on locks | |
| 14:12:44 | sean-k-mooney | yes but they would not race with anything | |
| 14:13:00 | mdbooth | Well the locks are critical to the semantics of the code | |
| 14:13:33 | sean-k-mooney | right but in any code path we should not have a singel action reneter the same critical section multiple times right | |
| 14:13:35 | mdbooth | Specifically, in this case they prevent the delete from completing while it is still building, which is the actual behaviour. | |
| 14:13:48 | sean-k-mooney | so the lock would always be aqired and never block | |
| 14:14:06 | mdbooth | If we disable locks, I have to write a race test which assumes that an instance can be completely deleted while it is still being built. | |
| 14:14:12 | mdbooth | That's obviously possible, but it's wrong. | |
| 14:14:20 | mdbooth | i.e. possible to write. | |
| 14:14:36 | mdbooth | But I'd be testing something which can't happen on a real system. | |
| 14:14:37 | sean-k-mooney | mdbooth: well the instnace can at least be partly deleted while its building | |
| 14:14:46 | mdbooth | sean-k-mooney: Absolutely, yes. | |
| 14:14:57 | mdbooth | sean-k-mooney: That's the race I'm writing a test for. | |
| 14:15:11 | sean-k-mooney | yep this is the thing stephen was working on | |
| 14:15:18 | sean-k-mooney | did you see the message i left | |
| 14:15:37 | mdbooth | sean-k-mooney: Did you leave it since this morning? | |