| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-07-19 | |||
| 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 | |
| 15:40:20 | sean-k-mooney | in the hugepage case if there are not enoguht free spawning the qemu instace fail on the dest and we abort the migration before we even pause the vm on the source | |
| 15:41:02 | sean-k-mooney | the same is true if the cores dont exist on the dest | |
| 15:41:22 | sean-k-mooney | so the only issue that is not caght by that is two vms being pinned to the same host | |
| 15:41:27 | sean-k-mooney | * core | |
| 15:41:48 | alex_xu | can we check the src and dst service version in the conductor? | |
| 15:42:14 | openstackgerrit | Artom Lifshitz proposed openstack/nova master: libvirt: move checking CONF.my_ip to init_host() https://review.opendev.org/671471 | |
| 15:42:18 | sean-k-mooney | we can but why would we | |
| 15:42:22 | alex_xu | emm...I guess we need another param for the rpc | |
| 15:42:36 | artom | alex_xu, that's what I had at some point, Dan talked me out of it | |
| 15:42:53 | sean-k-mooney | we need the data that is pass back from can live migrate on dest | |