| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-07-10 | |||
| 14:58:26 | mriedem | in case it's not clear, mr minnesota is not a real thing, nor was i a contestant | |
| 14:58:39 | mriedem | but a guy can dream... | |
| 15:00:18 | gibi | melwitt: Can I help somehow with https://review.openstack.org/#/c/540258 ? | |
| 15:17:24 | kashyap | mriedem: Hehe, I know it's not a _real_ thing! Was just teasing :P | |
| 15:21:53 | openstackgerrit | Artom Lifshitz proposed openstack/nova stable/queens: Refactor _build_device_metadata https://review.openstack.org/581398 | |
| 15:21:54 | openstackgerrit | Artom Lifshitz proposed openstack/nova stable/queens: Consider hostdev devices when building metadata https://review.openstack.org/581399 | |
| 15:23:31 | mriedem | gibi: until we're using placement for affinity scheduling, i don't think we can ever do https://review.openstack.org/#/c/581349/ in tempest | |
| 15:23:55 | mriedem | it will always be racy - which is why we have the late affinity check in the compute (which is disabled in default devstack b/c we can't upcall to the api db) | |
| 15:24:21 | gibi | mriedem: that test does not intended to show a race condition | |
| 15:24:46 | gibi | mriedem: I can add sleep between the server create requests and it will still fail | |
| 15:25:37 | mriedem | sure, but when the bug is fixed and we're properly pulling group members, it would start to race | |
| 15:26:04 | mriedem | i'd been meaning to write a functional test for https://review.openstack.org/#/c/540258 but need to look at my notes - it's been awhile | |
| 15:26:33 | mriedem | "We need a new bug and test for the multi-cell affinity scenario where two instances are scheduled at the same time in the same affinity group. We need 2 cells with 1 host each, with exactly enough capacity to fit just one instance so that placement will fail the first request and throw it into the other host in the other cell. The late affinity check in the compute won't fail because it can't see the other member in the | |
| 15:26:33 | mriedem | oh i guess i was going to write a new bug for something that came up during review: | |
| 15:26:34 | mriedem | her cell, so it will think it's fine." | |
| 15:27:28 | mriedem | the tricky part there is our functional tests are able to make up-calls from nova-compute | |
| 15:27:34 | mriedem | we don't have a split mq in our tests | |
| 15:27:38 | mriedem | not sure how easy that would be to do | |
| 15:27:40 | mriedem | dansmith: ^ ? | |
| 15:28:33 | mriedem | well, maybe won't need split mq - just the instances being in different hosts in different cell dbs might do it | |
| 15:28:34 | dansmith | ....yeah | |
| 15:28:53 | dansmith | with the workaround flag you can disable the check right? | |
| 15:29:09 | mriedem | yes | |
| 15:29:54 | dansmith | although I guess that doesn't really do much, | |
| 15:29:58 | gibi | mriedem: do I understand correclty that on master server groups only work if the late check using upcall is enabled? | |
| 15:30:00 | dansmith | obviously if you disable that check it's not going to check | |
| 15:30:02 | mriedem | maybe that type of request wouldn't get past scheduling | |
| 15:30:03 | openstackgerrit | Matthew Booth proposed openstack/nova master: Remove irrelevant comment https://review.openstack.org/578821 | |
| 15:30:04 | openstackgerrit | Matthew Booth proposed openstack/nova master: Move static _get_power_off_values to compute_utils https://review.openstack.org/578822 | |
| 15:30:30 | mriedem | gibi: i don't think so... | |
| 15:30:34 | mriedem | tempest has an anti-affinity test | |
| 15:30:50 | gibi | mriedem: that is the test I changed to show that master is broken | |
| 15:30:50 | dansmith | mriedem: so, scheduler will look at group.members, get the instance.host for each, and affine to those hosts, right? | |
| 15:31:02 | mriedem | gibi: test_create_server_with_scheduler_hint_group_anti_affinity | |
| 15:31:03 | dansmith | which is lossy because those instances might be pre-scheduling | |
| 15:31:07 | dansmith | or rather, pre-build | |
| 15:31:24 | openstackgerrit | Chen proposed openstack/nova master: WIP https://review.openstack.org/581403 | |
| 15:32:06 | gibi | mriedem: that tempest test only works because it sends two server create in the same API request | |
| 15:32:17 | mriedem | right, because servers in separate requests is a known race | |
| 15:32:34 | gibi | mriedem: if you wait 10 minutes between the two requests it still doesnt work | |
| 15:32:37 | gibi | mriedem: I tried :) | |
| 15:32:46 | dansmith | gibi: really? why is that? | |
| 15:33:19 | dansmith | the filter is using a potentially stale version of server group, from the beginning of the request, I just noticed | |
| 15:33:22 | mriedem | yeah the HostState.instances dict should be populated per request | |
| 15:33:29 | dansmith | that shouldn't be a problem 10 minutes later though | |
| 15:33:36 | gibi | dansmith: this returns an empty list for the second scheduling https://github.com/openstack/nova/blob/c0350da4a1607d7aa113caceaefb5d29303c7eed/nova/objects/instance_group.py#L422 | |
| 15:33:51 | mdbooth | Just working on this patch: https://review.openstack.org/#/c/578846/ I called it out in the commit message: the cut/paste in there is horrible, but avoids a refactor. Am I going to get that landed as is, or do I need to look at refactoring the driver interface? | |
| 15:34:01 | dansmith | gibi: oh on multi-cell? | |
| 15:34:01 | mriedem | gibi: and i think that's the bug melwitt is fixing | |
| 15:34:05 | dansmith | that needs to stripe | |
| 15:34:06 | dansmith | yeah | |
| 15:34:24 | gibi | don't we deploy with multi cell _by default_ ? | |
| 15:34:39 | mdbooth | It goes without saying that I don't mind refactoring :) However, I'd also like to both land and backport the code. | |
| 15:34:55 | mriedem | gibi: in devstack yes | |
| 15:34:58 | dansmith | gibi: yeah, this would hit cell0 if your default is cell0, and always be empty | |
| 15:35:14 | mriedem | it's not really 'multi cell' so much as superconductor mode in devstack and how the services are configured | |
| 15:35:30 | gibi | let me rephrase. I our suggested deployment mode (cell_v2) the server group policies are broken | |
| 15:35:41 | gibi | that is my understanding | |
| 15:35:50 | dansmith | gibi: because your default connection is to cell0, which will never return the list of instances, yeah | |
| 15:36:03 | gibi | dansmith: thanks, that explains what I see | |
| 15:36:13 | dansmith | but striping across cells is what needs to happen, which is that bug afaik | |
| 15:36:48 | gibi | dansmith: yeah. this is why offered my help to melwitt above on the fix | |
| 15:36:59 | dansmith | gibi: are we in violent agreement? :) | |
| 15:37:06 | gibi | dansmith: yes | |
| 15:37:48 | gibi | I was not sure we are on the same page about the size of the problem. server groups are useless now | |
| 15:38:17 | dansmith | gibi: if your controllers are pointing at your main cell by default it'll be fine, fwiw | |
| 15:38:24 | dansmith | but it definitely needs changing | |
| 15:38:51 | mriedem | gibi: so, if you want to help, we need a functional test in-tree that recreates the regression | |
| 15:39:03 | mriedem | that's what i was -1 on the change for - the mock based unit tests had logic flaws in them | |
| 15:39:54 | gibi | dansmith: how can a deployer change the default cell to cell1? | |
| 15:40:08 | gibi | mriedem: ack. I offered a tempest test :) | |
| 15:40:10 | dansmith | gibi: [database]/connection :) | |
| 15:41:05 | gibi | dansmith: in the super conductor conductor conf? | |
| 15:41:17 | dansmith | gibi: in any controller config | |
| 15:41:33 | dansmith | gibi: you don't have to be running a superconductor layout with only one non-cell0 cell | |
| 15:41:58 | mriedem | gibi: yeah, but we can't use that tempest change long-term because of the otherwise known race issue | |
| 15:42:28 | gibi | dansmith: I see | |
| 15:42:32 | gibi | dansmith: thanks | |
| 15:43:18 | gibi | mriedem: is it racy even if the test waits for the first server to go to ACTIVE state before start creating the second one? | |
| 15:43:49 | mriedem | gibi: in that case maybe not | |
| 15:43:59 | mriedem | because per scheduling request, we should pull the set of instances per host | |
| 15:44:28 | mriedem | https://github.com/openstack/nova/blob/master/nova/scheduler/host_manager.py#L781 | |
| 15:44:49 | mriedem | that's if the compute isn't sending it's instance info to the scheduler (which it's not in default devstack b/c we disable that ability) | |
| 15:45:38 | gibi | OK, so the second scheduling will see the first instance sitting on the host | |
| 15:45:41 | mriedem | which reminds me of https://review.openstack.org/#/c/569247/ but haven't had good large scale performance testing to see if that's justified | |
| 15:45:47 | mriedem | gibi: it should yeah | |
| 15:46:09 | gibi | mriedem: then I don't see how the currently proposed tempest test be racy | |
| 15:47:28 | openstackgerrit | Jay Pipes proposed openstack/nova master: update project/user for consumer in allocation https://review.openstack.org/581139 | |
| 15:49:03 | mriedem | gibi: ok updated comments | |
| 15:50:15 | gibi | mriedem: ack, I can cover both single and multi request cases in tempest | |
| 15:50:49 | gibi | mriedem: if we have the tempest do you still require the functional test in melwitt's patch? | |
| 15:51:53 | mriedem | maybe not | |
| 15:52:06 | mriedem | does your tempest test also cover affinity? | |
| 15:52:42 | mriedem | looks like it doesn't for separate requests | |
| 15:52:44 | gibi | mriedem: both the affinity and the anti-affinity case fails now | |
| 15:53:00 | gibi | mriedem: both uses the same helper to create the servers | |
| 15:53:50 | gibi | mriedem: but in case of affinity we might want to fill the compute to see NoValid host | |
| 15:54:04 | mriedem | that's what i was going to try with a functional test | |
| 15:54:19 | mriedem | to set available vcpu inventory to 1 or something | |
| 15:54:34 | gibi | mriedem: sure we have plenty of such functional tests already but they are not cell aware and passing | |