| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-10 | |||
| 21:23:16 | bauzas | looks not | |
| 21:23:49 | bauzas | that said, a question | |
| 21:24:05 | bauzas | if we spawn, do we have the same cache ? | |
| 21:24:08 | bauzas | mriedem: ^ | |
| 21:24:49 | mriedem | i don't understand the question | |
| 21:25:40 | bauzas | we would run multiple greenlets, right? | |
| 21:25:54 | bauzas | so my question is about the node cache | |
| 21:26:06 | bauzas | do we share the same node cache object between greenlets ? | |
| 21:26:55 | mriedem | oh eventlet spawn | |
| 21:27:00 | mriedem | not driver.spawn | |
| 21:28:21 | bauzas | yup eventlet.spawn_n even | |
| 21:28:59 | bauzas | maybe it's a stupid question, but I'm not remembering if coroutines accept to just share the same objects | |
| 21:29:59 | mriedem | _pike_flavor_migration is passed to the spawn and updates self._migrated_instance_uuids and yes i'd assume that's all pointing back to self | |
| 21:30:01 | mriedem | as the same object | |
| 21:30:10 | mriedem | otherwise that would be crazy | |
| 21:32:03 | mriedem | you see things getting hit in the logs | |
| 21:32:03 | mriedem | http://logs.openstack.org/54/487954/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial-nv/041c03a/logs/screen-n-cpu.txt.gz#_Aug_09_19_31_21_252127 | |
| 21:32:11 | mriedem | because the ironic jobs don't yet set resource_class on the nodes | |
| 21:32:37 | bauzas | mriedem: I should explain more my concern | |
| 21:32:50 | bauzas | mriedem: say we have pike flavor migration run that takes more than 60 secs | |
| 21:33:03 | bauzas | then, we would have 2 concurrent migrations | |
| 21:33:13 | bauzas | that looks okay to me because we check the cache | |
| 21:33:53 | bauzas | but the question I wonder is whether all the greenlets share the same cache, so when the first migration updates the cache, the latter gets the updates | |
| 21:34:06 | bauzas | maybe it's pointless, and I'm just silly | |
| 21:34:17 | bauzas | but I'm just thinking out loud | |
| 21:34:51 | mriedem | both greenlets should be working on the same self._migrated_instance_uuids | |
| 21:34:56 | mriedem | melwitt: edleafe: ^? | |
| 21:35:44 | bauzas | mriedem: looking at StackOverflow, looks like yup | |
| 21:35:47 | melwitt | yeah, I think the self._migrated_instance_uuids would be shared between them (if one was still running longer than 60 sec). so maybe we need to synchronize access to that set? | |
| 21:36:20 | mriedem | oh if only we could be using synchronized collections from java! | |
| 21:37:34 | mriedem | what's the worst that would happen here? | |
| 21:37:47 | mriedem | wouldn't we just double migrate the same instance.flavor.extra_spec? | |
| 21:37:52 | melwitt | that's what I was trying to think about. | |
| 21:38:03 | mriedem | if resource_key in specs: | |
| 21:38:03 | mriedem | # The compute must have been restarted, and the instance.flavor | |
| 21:38:03 | mriedem | # has already been migrated | |
| 21:38:03 | mriedem | continue | |
| 21:38:06 | mriedem | ^ should save it | |
| 21:38:06 | bauzas | mriedem: melwitt: well, the more I think about the problem, the more I think it wouldn't be a prolem | |
| 21:38:16 | bauzas | at least because it's for flavors | |
| 21:38:20 | melwitt | yeah | |
| 21:38:33 | bauzas | not sure operators have a lot of flavors needing more than 60 secs for a migration | |
| 21:39:17 | mriedem | well, unless you're hitting rpc timeouts on sending updates to conductor or something | |
| 21:39:26 | bauzas | and if so, well, not a problem given the current code which is not synchronised but failproof | |
| 21:39:39 | melwitt | it's a good point to think about though. I think mriedem is right that it would skip an already migrated one even if it got a stale view of the shared set | |
| 21:43:02 | melwitt | and set doesn't raise if you add the same element twice, just a no-op | |
| 21:44:06 | mriedem | looks like dtantsur|afk's change in ironic to test this is blowing up during scheduling http://logs.openstack.org/68/476968/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial/02053cf/logs/screen-n-sch.txt.gz#_Aug_09_20_35_41_533621 | |
| 21:44:20 | mriedem | edleafe: i left a couple of comments/questions in https://review.openstack.org/#/c/487954/ | |
| 21:46:09 | mriedem | hmm, the compute node should be reporting inventory to placement which would include the CUSTOM_BAREMETAL resource class | |
| 21:46:09 | mriedem | http://logs.openstack.org/68/476968/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial/02053cf/logs/screen-n-sch.txt.gz#_Aug_09_20_35_41_533621 | |
| 21:50:36 | mriedem | looks like we don't get inventory data before that | |
| 21:55:28 | mriedem | i'm a bit torn on if we should land the nova change w/o actually seeing this working in the devstack test patch for ironic | |
| 21:55:40 | mriedem | and i have to run right now anyway to pick up maya | |
| 21:56:25 | melwitt | should I remove my vote for now? | |
| 21:56:30 | mriedem | melwitt: no it's ok | |
| 21:56:40 | mriedem | i'm on the fence about just sending it in and then fixing later if there is something wrong | |
| 21:56:49 | melwitt | k | |
| 21:56:55 | mriedem | would be nice if someone from ironic could say, i tested this manually and it's fine | |
| 21:57:00 | mriedem | edleafe: ^ did anyone test manually? | |
| 21:57:12 | edleafe | mriedem: not that I know of | |
| 21:58:38 | mriedem | ok i'll ask in -ironic but i think most of that team is gone for the day | |
| 21:58:42 | mriedem | bbibab | |
| 21:58:44 | mriedem | *bbiab even | |
| 22:01:02 | openstackgerrit | Merged openstack/nova master: Improve stable-api doc with current API state https://review.openstack.org/489926 | |
| 22:18:09 | bauzas | folks, see you tomorrow | |
| 22:18:27 | bauzas | will look at the branch if any | |
| 22:27:37 | edleafe | So it looks like when Ironic sets the resource_class for a node, it doesn't do anything to create that in Placement | |
| 22:28:00 | edleafe | which would explain http://logs.openstack.org/68/476968/12/check/gate-tempest-dsvm-ironic-ipa-wholedisk-bios-agent_ipmitool-tinyipa-ubuntu-xenial/02053cf/logs/screen-n-sch.txt.gz#_Aug_09_20_35_41_533621 | |
| 22:33:51 | edleafe | mriedem: should I add code to ensure that the custom resource class for a node exists in that migration? I don't see anything in ironic where it is created | |
| 22:33:58 | kevinbenton | mriedem: kernel panic in VM http://logs.openstack.org/10/488510/33/gate/gate-tempest-dsvm-neutron-full-ubuntu-xenial/8b65cd3/logs/testr_results.html.gz | |
| 22:34:04 | kevinbenton | mriedem: how often does that happen? | |
| 22:36:39 | mriedem | edleafe: no, not in your change. it's used in the scheduler. the custom resource class is created in placement via the periodic updates in the RT | |
| 22:36:44 | mriedem | via the get_inventory() method to the ironic driver | |
| 22:37:57 | edleafe | mriedem: so that's not being run before the test failure above | |
| 22:38:59 | mriedem | edleafe: it's run on start of the compute service | |
| 22:39:03 | mriedem | and in the update_available_resource periodic, | |
| 22:39:09 | mriedem | the problem is we're not reporting any inventory for the node | |
| 22:39:27 | mriedem | so yeah, maybe the problem is a chicken and egg issue, idk | |
| 22:39:39 | mriedem | does the inventory not show up until we have a node that we're tracking with an instance? | |
| 22:39:49 | mriedem | and we don't have the instance w/o the custom resource class that the node is using | |
| 22:40:32 | edleafe | the error isn't that there is no inventory; it's that there is no such resource class | |
| 22:40:33 | mriedem | that interaction is a black box to me right now | |
| 22:40:42 | mriedem | edleafe: because we didn't PUT any inventory | |
| 22:40:48 | mriedem | which would create the custom resource class from the node we're tracking | |
| 22:41:00 | mriedem | the admin could pre-create the custom resource classes, sure | |
| 22:41:11 | mriedem | but nova is also trying to create them if they don't already exist in placement and we have inventory for them | |
| 22:41:36 | mriedem | i'm no baremetal expert though, so i do'nt know the order in which things need to happen here to auto-create the custom resource class | |
| 22:41:45 | edleafe | well, just trying to figure out how to fix this. Gotta run out in a few minutes | |
| 22:42:05 | mriedem | i don't know if there is something to fix on the nova side, | |
| 22:42:15 | mriedem | and i haven't dug into the devstack changes in the ironic WIP patch | |
| 22:42:30 | edleafe | they're selecting a flavor with the custom RC, but placement is barfing on that since it never got created. | |
| 22:42:30 | mriedem | so i might just throw this in rc1 | |
| 22:42:33 | mriedem | and deal with any issues in rc2 | |
| 22:42:35 | edleafe | yeah | |
| 22:42:47 | edleafe | i'm not sure that's the best response from placement | |
| 22:43:06 | mriedem | rather than just not return allocation candidates you mean? | |
| 22:43:13 | edleafe | I mean I understand preventing typos and stuff | |
| 22:43:37 | mriedem | it seems ok, you're asking for allocation_candidates filtering on something which doesn't exist | |
| 22:44:00 | mriedem | and you should probably make sure you can ask for something that's in GET /resource_classes | |
| 22:44:16 | mriedem | it's a client side error somewhere | |