| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-08-31 | |||
| 12:20:09 | gibi | mriedem: in that bug it seems that when the evacuation fails the migration object is not set to error statae | |
| 12:21:55 | mriedem | gibi: unrelated | |
| 12:23:28 | mriedem | https://bugs.launchpad.net/nova/+bug/1714235 | |
| 12:23:28 | openstack | Launchpad bug 1714235 in OpenStack Compute (nova) "evacuate API does not restrict one from trying to evacuate to the source host" [Low,Confirmed] | |
| 12:23:30 | openstackgerrit | Eric Fried proposed openstack/nova master: Bump keystoneauth1 minimum to 3.2.0 https://review.openstack.org/499577 | |
| 12:25:17 | mriedem | gibi: for that bug you pointed out, if the evacuate failed, the instance.host should still be pointed at the source host | |
| 12:25:48 | mriedem | we should only be removing the instance allocation for the source node if the instance.host != CONF.host | |
| 12:26:20 | mriedem | _destroy_evacuated_instances doesn't seem to take that into account | |
| 12:28:43 | mriedem | gibi: actually it looks like that's a recent regression https://review.openstack.org/#/c/491808/ | |
| 12:32:16 | mriedem | maybe not, but it looks suspect | |
| 12:34:12 | mriedem | gibi: we could revert https://review.openstack.org/#/c/491808/ on top of https://review.openstack.org/#/c/498482/ and see what happens | |
| 12:36:05 | gibi | mriedem: thanks. I can try out the revert | |
| 12:38:51 | efried | sdague https://github.com/openstack/nova/blob/master/nova/cmd/status.py#L198 <== I can't see where we're checking for registered compute nodes in this method. Is this comment obsolete? | |
| 12:39:09 | efried | (or, more likely, am I missing something?) | |
| 12:39:29 | gibi | mriedem: hm, _destory_evacuated_instances does filter for the instance.host https://github.com/openstack/nova/blob/master/nova/compute/manager.py#L638 | |
| 12:40:21 | mriedem | that's finding the migrations | |
| 12:40:28 | mriedem | so it will find the migration that started from this source host | |
| 12:40:38 | mriedem | but that's not checking that the instance.host has changed to another host | |
| 12:40:40 | gibi | mriedem: ahh, yes, my bad | |
| 12:40:59 | mriedem | it's also weird that it's filtering on 'accepted' migrations, because that is set when the migration record is created in the api | |
| 12:41:00 | openstackgerrit | Lajos Katona proposed openstack/nova master: Add functional for live migrate delete https://review.openstack.org/499583 | |
| 12:41:07 | mriedem | so the migration might not actually be done | |
| 12:41:41 | gibi | mriedem: also the regression test shows that this migration never goes to error of failed state | |
| 12:41:48 | gibi | mriedem: it remains in accepted state | |
| 12:42:02 | mriedem | that was another question - how is the evacuate actually failing? | |
| 12:42:04 | gibi | mriedem: here is the regression test https://review.openstack.org/#/c/498482/ | |
| 12:42:07 | mriedem | is it failing in conductor? | |
| 12:42:15 | gibi | mriedem: scheduler fails to found a host | |
| 12:42:17 | mriedem | ah | |
| 12:42:18 | mriedem | yeah | |
| 12:43:14 | gibi | mriedem: here is one way to solve this https://review.openstack.org/#/c/499237/1/nova/conductor/manager.py (still WIP) | |
| 12:44:58 | mriedem | yeah that's pretty straight forward | |
| 12:45:21 | gibi | mriedem: btw is there a doc about the possible migration status values? I found both error and failed in the code | |
| 12:45:22 | mriedem | odd that we don't pass the migration record over rpc from api to conductor, but that would be a separate cleanup | |
| 12:45:32 | mriedem | i don't think there is really | |
| 12:46:08 | mriedem | i think takashi was cataloging some of that for his blueprint to list more than just in-progress live migrations out of the api | |
| 12:46:14 | mriedem | so he was defining what 'in-progress' meant | |
| 12:46:36 | gibi | mriedem: cool I fully support documenting the possible statuses | |
| 12:46:52 | mriedem | some of it is in here https://specs.openstack.org/openstack/nova-specs/specs/pike/approved/list-show-all-server-migration-types.html#proposed-change | |
| 12:46:53 | bauzas | mriedem: when you say "asking to evacuate to the same host", do you imply using the force flag or not ? | |
| 12:47:05 | mriedem | bauzas: sure | |
| 12:47:24 | mriedem | if force=False, you'd get NoValidHost | |
| 12:47:28 | mriedem | because of the ComputeFilter | |
| 12:47:34 | bauzas | correct | |
| 12:47:35 | mriedem | but if force=True, you'd bypass the scheduler | |
| 12:47:39 | mriedem | and fail the rpc cast to compute | |
| 12:47:45 | bauzas | but that's your fault | |
| 12:47:52 | bauzas | you *forced* | |
| 12:48:30 | mriedem | it just seems weird that we don't have that one line validation check in the api code | |
| 12:48:39 | mriedem | if host and host == instance.host: raise 400 | |
| 12:48:44 | bauzas | that said, I think there is a call made by the API verifying if the destination is alive before we call the conductor | |
| 12:48:54 | bauzas | for evacuate I mean | |
| 12:48:54 | mriedem | bauzas: yes there is | |
| 12:48:56 | mriedem | and that can pass | |
| 12:49:03 | mriedem | and you can call conductor on the same host as instance.host | |
| 12:49:13 | bauzas | wait | |
| 12:49:25 | bauzas | ah, nevremind | |
| 12:49:29 | mriedem | and eventually either it fails with NoValidHost and the instance state is reset (best case scenario), or we bypass the scheduler and rpc fails, and your instance is stuck in 'rebuilding' state | |
| 12:49:36 | bauzas | the API check is verifying the *source* | |
| 12:49:40 | mriedem | correct | |
| 12:50:06 | mriedem | you will fail either way, but a straight 400 is better than weird undefined failures once we've cast to compute | |
| 12:50:09 | mriedem | s/compute/conductor/ | |
| 12:50:28 | bauzas | well, when I wrote the original Newton spec about force flags and so on, I made it clear that if people are using 'force', they have to be super-cautious | |
| 12:50:40 | bauzas | that's what we said to them | |
| 12:50:53 | mriedem | how many operators do you think have read that spec? | |
| 12:50:58 | bauzas | the real problem was that pre-Newton, we weren't clear whether we were enforcing rules | |
| 12:51:18 | bauzas | I think I translated that in the API docs | |
| 12:51:26 | bauzas | but I could be missing that | |
| 12:51:27 | mriedem | this is the description of the force parameter in the api ref | |
| 12:51:27 | mriedem | "Force an evacuation by not verifying the provided destination host by the scheduler." | |
| 12:51:37 | mriedem | ^ is not, "holy shit, don't do this" | |
| 12:51:52 | mriedem | we should put a warning in there probably | |
| 12:52:36 | mriedem | also, | |
| 12:52:37 | bauzas | mriedem: I didn't wanted to be pedantic when I said about the spec, I just try to explain that I saw there was by that time I wrote the spec, a pretty clear consensus that if operators are providing destinations, they *have to* make sure it's an acceptable one | |
| 12:53:05 | bauzas | because it's anti-cloud | |
| 12:53:15 | bauzas | you specify a destination, fair enough | |
| 12:53:20 | mriedem | with https://review.openstack.org/#/c/499399/ now, we should probably seriously consider splitting the rebuild_instance conductor method / rpc api into rebuild_instance and evacuate_instance | |
| 12:53:26 | bauzas | but then, make sure it's a good one | |
| 12:53:30 | mriedem | because the if/else logic in there is getting pretty hairy | |
| 12:53:34 | sdague | efried: yeh, I don't know | |
| 12:54:21 | efried | sdague Just something I noticed while I was in the neighborhood; and you're git blamed on that comment :) | |
| 12:54:31 | bauzas | mriedem: there is a side concern to me: you can specify a destination but we don't tell whether it's case-sensitive or not | |
| 12:54:56 | bauzas | mriedem: and somewhere, it breaks | |
| 12:55:32 | mriedem | you'd get NoValidHost i'd thikn | |
| 12:55:37 | mriedem | since we'd filter out all hosts | |
| 12:55:43 | mriedem | since the ComputeNode.host doesn't match | |
| 12:56:01 | bauzas | there is a bug | |
| 12:56:07 | bauzas | wait, finding it | |
| 12:56:18 | mriedem | lemme guess, case insensitivity in mysql? | |
| 12:57:15 | bauzas | https://bugs.launchpad.net/nova/+bug/1709260 | |
| 12:57:17 | openstack | Launchpad bug 1709260 in OpenStack Compute (nova) "Addition of host to host-aggregate should be case -sensitive" [Low,In progress] - Assigned to Rajesh Tailor (ratailor) | |
| 12:57:29 | bauzas | it seems that DNS is case-insentive | |
| 12:57:38 | bauzas | case-insensitive | |
| 12:57:54 | bauzas | so in theory, we should accept to migrate to foo or FOO | |
| 12:58:04 | bauzas | but yeah, I guess it's because mysql | |
| 12:58:42 | bauzas | ratailor: around ? | |
| 12:58:50 | ratailor | bauzas, yep | |
| 12:58:56 | bauzas | ratailor: I feel I badly triaged your bug | |
| 12:59:12 | bauzas | ratailor: since DNS is case-insensitive, hostnames should be too | |
| 12:59:45 | ratailor | bauzas, I reproduced it, and found that mysql doesn't support case-sensitivity by-default. So I had to change the collation on related tables. | |