| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-19 | |||
| 15:04:18 | artom | See https://review.opendev.org/#/c/634606/44/nova/compute/manager.py@6482 | |
| 15:04:50 | artom | It's not a migrate_data class problem, it's "we need to call the driver twice, once for check_can_live_migrate_destination, and once after we've done claims" | |
| 15:05:04 | artom | In my patch, that second call is very use-case specific "get_dst_numa_config" | |
| 15:05:36 | sean-k-mooney | for what its woth we do something similar for sriov | |
| 15:05:39 | artom | alex_xu will need to solve the same problem, so rather than adding "get_vpmem_config" or whatever to the driver interface, we're trying to come up with a more generic single method for that second call to driver | |
| 15:06:22 | alex_xu | I'm trying to figure out why you need the migration_data for the claim | |
| 15:06:40 | artom | Other way around, we need to claim to update the migrate data | |
| 15:06:51 | artom | Because we get the NUMA topology on the dest host from the claim | |
| 15:09:42 | alex_xu | artom: looks like you only want to know whether instance has numa topo https://review.opendev.org/#/c/634606/44/nova/compute/manager.py@6470 | |
| 15:10:13 | sean-k-mooney | for sriov migration we added https://review.opendev.org/#/c/620115/35/nova/compute/resource_tracker.py which we callin in https://review.opendev.org/#/c/620115/35/nova/conductor/tasks/live_migrate.py | |
| 15:10:28 | sean-k-mooney | * in check_can_live_migrate_destination | |
| 15:11:34 | artom | alex_xu, in my case, yeah | |
| 15:12:29 | alex_xu | artom: I don't understand this https://review.opendev.org/#/c/634606/44/nova/virt/libvirt/driver.py@7463 | |
| 15:13:00 | artom | alex_xu, it's to handle the N/N-1 problem | |
| 15:13:04 | alex_xu | what different with we check instance.numa_topology at dest host compute manager | |
| 15:13:13 | alex_xu | oh, upgrade issue | |
| 15:13:18 | artom | Exactly | |
| 15:13:35 | sean-k-mooney | alex_xu old host wont populate that field | |
| 15:13:38 | artom | But you're right, once we get to U or V or whenever, that's no longer needed | |
| 15:13:55 | openstackgerrit | **** proposed openstack/nova master: Nova: node should be deleted when last service is deleted https://review.opendev.org/671731 | |
| 15:13:58 | sean-k-mooney | well actuly ignore that | |
| 15:14:46 | sean-k-mooney | they wont populate it but we need it for other reasons | |
| 15:14:47 | alex_xu | do we need that LM to be fixed in the middle of S to T upgrade | |
| 15:15:05 | alex_xu | if not, then we can just enable the numa LM by the rpc version | |
| 15:15:10 | artom | Are you asking whether we need to support S -> T NUMA LM? | |
| 15:15:31 | alex_xu | we can only support that LM when all the nodes upgrade to T | |
| 15:15:35 | sean-k-mooney | alex_xu: we need to assume that not all nodes will be upgrade at teh same time | |
| 15:15:43 | artom | We do, it's apparently a big thing during upgrades, operators migrate workloads off compute nodes being upgraded | |
| 15:16:02 | sean-k-mooney | alex_xu: that was discussed at lenght in the spec | |
| 15:16:28 | artom | Or if you're asking "why not check min service version" to determine whether we can NUMA LM... | |
| 15:16:37 | sean-k-mooney | alex_xu: https://specs.openstack.org/openstack/nova-specs/specs/train/approved/numa-aware-live-migration.html#upgrade-impact | |
| 15:16:48 | artom | Yeah, we could do that, and at point it's what was done in the code, but I think dansmith came up with this way, which personally I find more elegant | |
| 15:17:44 | alex_xu | the min service version is also require all the nodes upgrade to T, right? | |
| 15:17:50 | artom | Yeah | |
| 15:18:30 | artom | Although perhaps, in the interest of not adding driver interface methods, we could go back to checking min service version, and then we could do the claim before driver.check_can_live_migrate_at_destinaton | |
| 15:18:33 | artom | (I think) | |
| 15:18:52 | sean-k-mooney | we should not do the claim before that | |
| 15:19:07 | sean-k-mooney | driver.check_can_live_migrate_at_destinaton is where the claim should be done | |
| 15:19:29 | alex_xu | that check means, only the source node is new version, then we can do LM? | |
| 15:19:53 | sean-k-mooney | alex_xu: we can live migrate but we wont update the guest xml | |
| 15:20:24 | sean-k-mooney | so it will be jsut as broken as before | |
| 15:20:38 | sean-k-mooney | but it will live migrate succesfully | |
| 15:21:06 | alex_xu | sean-k-mooney: so in upgrade, we need to LM from the old to new, then LM from new back to old | |
| 15:21:24 | sean-k-mooney | you dont need too | |
| 15:21:30 | sean-k-mooney | we just wont break that | |
| 15:22:00 | sean-k-mooney | the sriov migration feature is coded such that it falls back to the old behavior if the info is not available to use teh new flow | |
| 15:22:28 | sean-k-mooney | the new flow is basically only used when both are new enough but its explained in the table in the spec | |
| 15:23:32 | alex_xu | but I still can't migrate the instance from old to new...that break the first step of upgrade | |
| 15:23:42 | sean-k-mooney | you can | |
| 15:23:55 | alex_xu | you said the xml won't update | |
| 15:24:00 | sean-k-mooney | correct | |
| 15:24:11 | sean-k-mooney | it use the old live migration workflow | |
| 15:24:13 | artom | It's impossible to update the XML unless both source and dest support it | |
| 15:24:24 | artom | So we do best effort and revert to previous half-broken behaviour | |
| 15:24:48 | alex_xu | the instance won't be break? | |
| 15:24:54 | artom | It might :) | |
| 15:25:08 | artom | That's why it's currently half-broken | |
| 15:25:08 | sean-k-mooney | they havent been broken for the last 10 releases | |
| 15:25:09 | alex_xu | hah | |
| 15:25:25 | sean-k-mooney | what might happen is two vms get pinned to the same cores | |
| 15:25:36 | sean-k-mooney | but that could happen before. | |
| 15:25:51 | artom | It might actually break if it's pinned to CPUs that don't exist on the dest | |
| 15:26:14 | sean-k-mooney | if the cores do not exist or there is not enough hugepages libvirt fails the migrtion due to the qemu error | |
| 15:26:14 | alex_xu | yea, so...we should ask the user to do that | |
| 15:26:17 | sean-k-mooney | and it auto reverts | |
| 15:26:25 | alex_xu | s/should/shouldn't/ | |
| 15:26:28 | sean-k-mooney | that is the same behavior as today | |
| 15:27:10 | alex_xu | I remember we have a patch stop that LM api call by checking the instance has numa | |
| 15:27:19 | sean-k-mooney | yep we merged that | |
| 15:27:25 | artom | Yeah, it's now disabled by default | |
| 15:28:03 | sean-k-mooney | so to live migrarte form S->T they need to re enabeld it | |
| 15:28:21 | sean-k-mooney | although i think artom is removing that option in T? | |
| 15:28:27 | sean-k-mooney | or is that puhsed to U | |
| 15:28:31 | alex_xu | so...we will tell the user, now you can LM your instance again when you upgrade from S to T, but you instance may gone based on lucky :) | |
| 15:28:41 | artom | sean-k-mooney, it's being deprecated but not removed, because of the upgrade impact | |
| 15:28:56 | artom | We don't want to turn it on by default unless we're sure the whole cloud supports it | |
| 15:29:33 | sean-k-mooney | alex_xu: the important thing is we do not regres or change any live migration behavior with artom code | |
| 15:30:01 | alex_xu | sean-k-mooney: I agree with that...but doesn't mean we should encourge the user to do that | |
| 15:30:14 | alex_xu | special for danger thing | |
| 15:30:22 | sean-k-mooney | we are not encuragin people to do it | |
| 15:30:38 | sean-k-mooney | if you have numa instance you should cold migrate them or do an inplace upgrade | |
| 15:30:50 | sean-k-mooney | that is what we will be telling customers | |
| 15:31:11 | alex_xu | yes, so we needn't to support src to dest numa instance LM :) | |
| 15:31:18 | alex_xu | for s to t | |
| 15:31:41 | sean-k-mooney | alex_xu: orginally we didnt | |
| 15:32:17 | sean-k-mooney | but both i belive dansmith and mriderman wanted the spec changed to the current form | |
| 15:32:33 | artom | (Oh man, M. Riderman) | |
| 15:32:39 | artom | I am lolling right now | |
| 15:34:21 | sean-k-mooney | actully it look like dansmith didnt review that spec but i know we spoke to him about it | |
| 15:35:35 | sean-k-mooney | anyway for sriov live migration since it never worked before we have a hard check in the conductor | |
| 15:35:47 | sean-k-mooney | and fail if both nodes are not knew enough | |
| 15:36:13 | sean-k-mooney | since live migration wiht hugepages is a thing we support for ovs-dpdk we did not want to break it by blocking it | |
| 15:36:52 | openstackgerrit | Merged openstack/nova master: Bump the openstackdocstheme extension to 1.20 https://review.opendev.org/671694 | |
| 15:37:48 | alex_xu | I guess the only reason, the user know they have same type machine, and the instances won't break with pinning to same pcpu | |
| 15:37:54 | sean-k-mooney | alex_xu: uncontidionally blocking old to new migration would be a regression. | |
| 15:38:06 | alex_xu | ah.. | |
| 15:38:22 | sean-k-mooney | well if the instance is not pinned but just has hugepages we dont want to break that | |
| 15:38:33 | alex_xu | I see the reason now | |
| 15:38:37 | artom | alex_xu, yeah, I think that's explicitly called out somewhere: NUMA LM is only "guaranteed" to work if you're migrating to an "empty" identical machine, identically configured, | |
| 15:39:27 | alex_xu | got it | |