| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-06-17 | |||
| 10:23:45 | stephenfin | bhagyashris: Just replied to that http://lists.openstack.org/pipermail/openstack-discuss/2019-June/007172.html | |
| 10:24:10 | stephenfin | bhagyashris: tl;dr: don't worry about it because we can rely on host aggregrates | |
| 10:25:08 | stephenfin | If an operator is using them, then pinned instance will always land on hosts in the pinned aggregrate (or whatever it's called). The VCPU inventory of these instances will never be consumed | |
| 10:25:14 | stephenfin | *of these hosts | |
| 10:26:12 | bhagyashris | stephenfin: I have to request to Matt as well. As he has given -2 and abandoned the patches: https://review.opendev.org/#/q/project:openstack/nova+topic:bp/cpu-resources I have submitted earlier | |
| 10:26:54 | stephenfin | If an operator is not using them, then things will be broken, but they were broken before | |
| 10:27:05 | stephenfin | bhagyashris: I can restore those for you if you're planning to re-use them? | |
| 10:27:55 | bhagyashris | stephenfin: yeah I am plaining to use that patches so that will be great if you can restore it :) | |
| 10:28:32 | sean-k-mooney | bhagyashris: the impmenation you did last cycle was not the same as the current spec. they were abandond becayse they had not been updated in many months and the spec was not approved for last cycle | |
| 10:28:34 | bhagyashris | stephenfin: checking your reply.... | |
| 10:29:00 | stephenfin | bhagyashris: Hmm, yeah, looking at them, perhaps it would be better to submit new reviews | |
| 10:32:57 | bhagyashris | stephenfin: yeah but the base implementation has not that much change so I was thinking to push on the same. Only the upgrade patch that anyways not uploaded yet. But if you want to push the new patches then I will push new reviews | |
| 10:34:01 | stephenfin | Yeah, probably easier to just change the commit ID. I don't see that much context that would be useful going forward | |
| 10:34:17 | stephenfin | bhagyashris: Do you think you'll have patches available by the end of this week? | |
| 10:34:25 | stephenfin | At least WIP ones | |
| 10:35:06 | bhagyashris | stephenfin: ok I will push the new patches. Sure I will submit the WIP patches by end of this week | |
| 10:35:07 | sean-k-mooney | looking at the old patches i dont see any logic to soft pin floating instance to the shared_cpu_set | |
| 10:35:20 | sean-k-mooney | i assume that still needs to be written | |
| 10:43:19 | bhagyashris | stephenfin: I just want to ask about your reply that you said the host aggregrates should also be configured correctly that means if user request instance having the flavor extra specs ``resources:PCPU=1 and resources:VPU=1`` on upgrade node then in that case it will report both the VCPU and PCPU resources | |
| 10:43:40 | stephenfin | bhagyashris: No, we're not supporting that at the moment | |
| 10:43:57 | stephenfin | We can support it in U but not in Train | |
| 10:44:38 | stephenfin | Well, we could support it in Train with a big warning that operators need to do their homework, but the spec doesn't encompass that yet | |
| 10:44:51 | sean-k-mooney | stephenfin: its out of scope of the spec | |
| 10:44:54 | bhagyashris | stephenfin: means that flavor (having ``resources:PCPU=1 and resources:VCPU=1``) should have "aggregate_instance_extra_specs:pinned": "true" extra specs as well | |
| 10:45:03 | sean-k-mooney | bhagyashris: no | |
| 10:45:21 | sean-k-mooney | bhagyashris: in train we do not plan to allow host to have both pinned and non pinned cpus | |
| 10:45:23 | stephenfin | As noted in the other email, that was dropped from the spec to focus on getting the important piece - PCPU tracking in placement - done | |
| 10:45:27 | sean-k-mooney | *instances | |
| 10:45:33 | stephenfin | yeah, what sean-k-mooney said | |
| 10:45:49 | sean-k-mooney | host can be mixed but instances cannot, in train | |
| 10:47:41 | bhagyashris | sean-k-mooney : means operator can set either dedicated_set or shared_set only and not the both the set attime ? | |
| 10:47:55 | stephenfin | They can set both | |
| 10:48:04 | sean-k-mooney | mixing pinned and non pinned cpus in a singel instance was not allowed in the spec last cycle either. it was part fo a different spec. | |
| 10:48:05 | stephenfin | cpu_shared_set is also used for emulator threads | |
| 10:48:20 | stephenfin | sean-k-mooney: Nah, it was part of the spec but I scrubbed references to it | |
| 10:48:37 | stephenfin | What wasn't included was the NUMA stuff | |
| 10:48:41 | sean-k-mooney | stephenfin: hum maybe the other spec got folded in at some point | |
| 10:48:43 | stephenfin | *was never included | |
| 10:48:47 | stephenfin | Quite possibly, yeah | |
| 10:49:39 | stephenfin | bhagyashris: We're totally for supporting instances with pinned and unpinned instances in the future but not yet. This is just too complex already without adding more complexity | |
| 10:49:46 | stephenfin | and not having it makes upgrades simpler, as you note | |
| 10:50:42 | stephenfin | So if you have patches adding this functionality, I would leave them to the end of the series and wouldn't focus your attention on them yet. They won't merge so you'd be wasting effort | |
| 10:51:20 | bhagyashris | stephenfin: if operator can set both the set say cpu_dedicated_set and cpu_shared_set then in that case inventory reported in the placement will be PCPU and VCPU right? | |
| 10:51:35 | stephenfin | Yes | |
| 10:52:00 | sean-k-mooney | bhagyashris: what i was refering to before about soft pinning is that when you are mixing pinned guests with floating guests we need to ensure the floating guest only float over teh cpu_shared_set when cpu_dedicated_set is defiend | |
| 10:52:15 | sean-k-mooney | bhagyashris: yep | |
| 10:52:22 | stephenfin | and, in theory, they could stop using host aggregates to separate pinned instances from non-pinned instances at that point | |
| 10:52:49 | stephenfin | because the non-pinned instances will only float on the cores listed in cpu_shared_set | |
| 10:53:03 | stephenfin | Or "soft pinning", as sean-k-mooney points out | |
| 10:53:22 | sean-k-mooney | its the same as what we do for numa | |
| 10:53:31 | sean-k-mooney | if you dont also enable cpu pinning | |
| 10:53:44 | sean-k-mooney | we confine the cpus to float over a numa node | |
| 10:54:05 | stephenfin | yup, though we shouldn't care about NUMA affinity for the non-NUMA and non-pinned case | |
| 10:54:14 | sean-k-mooney | correct | |
| 10:54:40 | stephenfin | sean-k-mooney: This reinforces how important artom's NUMA live migration work is though | |
| 10:54:59 | stephenfin | we'll need that to make sure the soft pinning information is recalculated when we live migrate | |
| 10:56:01 | sean-k-mooney | stephenfin: yes we will we already have accounted for that in his work | |
| 10:56:14 | sean-k-mooney | we support the soft pinning case for numa | |
| 10:56:20 | stephenfin | awesome | |
| 10:56:33 | sean-k-mooney | so we just need to ensure that we also pass back the info for non pinned guests | |
| 10:56:42 | stephenfin | Yup | |
| 10:56:44 | stephenfin | bhagyashris: Any other questions before I disappear for lunch? | |
| 10:56:57 | sean-k-mooney | so there should be a change in the cpu pinning spec that extend what he is doing for shared instances | |
| 10:57:47 | sean-k-mooney | * not spec but patch series | |
| 10:58:15 | bhagyashris | stephenfin: but the host aggregates will come into picture only when the host are under that aggregate and that aggregate having meta data pinned and flavor also need to set the extra specs "aggregate_instance_extra_specs:pinned": "true" but as per the new implementation and flavor having new flavor extra specs (``resources:PCPU=1 and resources:VCPU=1``) and dont have "aggregate_instance_extra_specs:pinned": "true" extra spec | |
| 10:58:15 | bhagyashris | s so in that case it will consume both the inventories | |
| 10:59:06 | sean-k-mooney | "resources:PCPU=1 and resources:VCPU=1" is not valid in train | |
| 10:59:23 | sean-k-mooney | bhagyashris: we can have the api reject instance with that config | |
| 10:59:29 | stephenfin | Yeah, you need to stop worrying about that. We can't and won't support it in Train | |
| 10:59:43 | stephenfin | In fact, we should have an explicit check to prevent that | |
| 11:00:18 | stephenfin | Until we get to U and submit a new spec for adding this functionality | |
| 11:00:38 | sean-k-mooney | stephenfin: bhagyashris ya we should alex_xu pointed out that there is already a check in the api tha validates the numa and pinning extra_specs are valide so we can extend it | |
| 11:02:27 | bhagyashris | stephenfin, sean-k-mooney : but ie user request it separately say in the first request he request resources:PCPU=1 and in second request he request resources:VCPU=1 then it will consume the resources form the same cpu_set | |
| 11:02:48 | sean-k-mooney | bhagyashris: what second request | |
| 11:02:57 | sean-k-mooney | do you mean two different vms? | |
| 11:03:08 | bhagyashris | sean-k-mooney : yes | |
| 11:03:14 | sean-k-mooney | that is fine | |
| 11:03:39 | sean-k-mooney | we will be allowing one vm with pinning and another with out pinning on the same host | |
| 11:04:01 | bhagyashris | sean-k-mooney : plz check this I have detailed the scenario here : http://lists.openstack.org/pipermail/openstack-discuss/2019-June/007144.html | |
| 11:04:02 | sean-k-mooney | if and only if it has inveotry of both VCPU and PCPUs | |
| 11:04:34 | sean-k-mooney | bhagyashris: yes as i pointed out that is supported | |
| 11:04:54 | bhagyashris | sean-k-mooney : if that is fine then this is the violation to pinning feature | |
| 11:05:33 | sean-k-mooney | bhagyashris: its not, it would be if and only if you have not set cpu_dedicated_set | |
| 11:06:39 | sean-k-mooney | you will not be ably to support mixing vms until after you have cpu_dedicated_set and removed the use of vcpu_pinned_set | |
| 11:07:42 | sean-k-mooney | which mean operators should continue to use host aggrages to prevent mixing until they have modified there config to use the new feature | |
| 11:07:51 | bhagyashris | sean-k-mooney : but as mentioned in the spec if we have vcpu_pin_set set in the Stein and now it's upgraded to Train then will report both the inventory PCPU and VCPU and after that if user request two vim separately as we discussed above then | |
| 11:09:00 | bhagyashris | sean-k-mooney : I that case it will consume resources from same CPUs and I think that is wrong | |
| 11:09:11 | sean-k-mooney | we know | |
| 11:09:24 | sean-k-mooney | and we are going to allow that as that is the pre train behaviro | |
| 11:10:00 | sean-k-mooney | we are aware that in that case the floating instace can float over the pinned instances | |
| 11:10:29 | sean-k-mooney | but mixing vms on the same host will only be supported after you have defiend cpu_dedicated_set | |
| 11:10:31 | bhagyashris | sean-k-mooney : yeah and thats my concern | |
| 11:10:43 | sean-k-mooney | bhagyashris: we intentionlly want to allow that | |
| 11:11:04 | bhagyashris | sean-k-mooney : ok | |
| 11:11:04 | sean-k-mooney | bhagyashris: we need to allow it to allow inplace upgrade of nodes with running instances | |
| 11:14:12 | sean-k-mooney | one of the requirements for edge workload is we need to be able to do inplace upgrades without draining a host of instaces. going for Train to U we will remove the vcpu_pin_set option which means before upgrading to U you will need to updated your config to remove it. | |
| 11:15:34 | bhagyashris | sean-k-mooney : just want to clear some points 1. As we discussed above that the floating instace can float over the pinned instances is allowed for train release | |
| 11:15:56 | sean-k-mooney | its only allowed if cpu_dedicated_set is not defined | |
| 11:16:38 | bhagyashris | sean-k-mooney : if user request the both the resources:PCPU and VCPu resources in the same request then that should get rejected | |
| 11:17:16 | sean-k-mooney | if cpu_dedicated_set is defiend VCPU inventory will only be reported if cpu_shared_set is defiend. if both cpu_shared_set and cpu_dedicated_set are defiend then nova will soft pin floating cores to the cpu_shared_set cpus | |