| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-07-28 | |||
| 15:49:21 | stephenfin | logging ftw | |
| 15:50:20 | kashyap | stephenfin: Here, right now I don't have a better phrasing; you might say it's not even required, let's see -- https://review.opendev.org/#/c/741473/1/nova/conf/libvirt.py@261 | |
| 15:54:11 | openstackgerrit | Ghanshyam Mann proposed openstack/nova master: Pass the actual target in volumes policy https://review.opendev.org/742779 | |
| 15:57:15 | artom | stephenfin, seems sane, left a note | |
| 16:00:35 | stephenfin | artom: https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L66-L67 | |
| 16:00:43 | stephenfin | https://github.com/openstack/nova/blob/master/nova/virt/libvirt/config.py#L53-L59 | |
| 16:00:55 | stephenfin | the name is poor, admittedly | |
| 16:14:52 | artom | stephenfin, doh :( A ctrl-] would not have been that hard, on my part | |
| 16:15:39 | sean-k-mooney | what does ctrl-] do | |
| 16:15:43 | sean-k-mooney | a find? | |
| 16:15:48 | sean-k-mooney | got do definition | |
| 16:15:53 | artom | Go to definition, in vim with ctags | |
| 16:15:59 | sean-k-mooney | ah ok | |
| 16:16:15 | artom | Admittedly, it sometimes messes up if there are multiple methods called the same thing | |
| 16:22:22 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Track CPU scheduler policy during live migration https://review.opendev.org/743588 | |
| 16:22:37 | stephenfin | artom: follow-up there, if you're feeling generous | |
| 16:25:19 | artom | stephenfin, so... does it really make sense to do this, given we only support 1 policy now? | |
| 16:27:16 | stephenfin | Maybe. Maybe not. Given how trivial it is to do, I'd rather knock it off now than have to worry about it when we really do need it | |
| 16:27:41 | stephenfin | at that point we'd be down to needing service version checks, or whatever the new hotness is by then | |
| 16:29:01 | artom | stephenfin, so... this would be for a case where the scheduler is set on a per-host basis? | |
| 16:29:18 | artom | And we want to support live migration between different hosts/schedulers? | |
| 16:29:32 | artom | Wouldn't we do it as an image property/flavor extra spec? | |
| 16:30:29 | sean-k-mooney | stephenfin: given we dont support other schduler polcies and dont plan to intoduce that in the near term do we really need this cange | |
| 16:30:30 | stephenfin | if we did have different scheduler policies they'd likely be done via an image prop/extra spec, yes | |
| 16:30:52 | sean-k-mooney | stephenfin: right but we dont plan on allowing user to request that | |
| 16:31:23 | artom | stephenfin, so if it's set in the flavor, do we need this change? | |
| 16:31:34 | artom | As you can see, I'm having trouble being convinced :) | |
| 16:31:46 | sean-k-mooney | if it was in the flavor or image no | |
| 16:31:57 | sean-k-mooney | we woudl only need it if it was set by a host level config option | |
| 16:32:17 | stephenfin | artom: I'm confused. What would change? | |
| 16:32:17 | artom | sean-k-mooney, yep, and I think you and I would be against such a thing | |
| 16:32:28 | artom | Precisely because it would require stephenfin's patch :P | |
| 16:32:52 | artom | stephenfin, I'm saying - if the scheduler is set via flavor extra spec, we don't need to update it as part of the live migration XML update | |
| 16:33:02 | sean-k-mooney | stephenfin: i guess the issue is right now i dont think your change adds anything useful | |
| 16:33:15 | artom | Because the destination host should be able to accept the existing scheduler | |
| 16:33:27 | artom | (Presumably because we've scheduled to it because of a trait or something) | |
| 16:33:35 | sean-k-mooney | we have no plans to expose sched_scheduler to the user or operator as something that can be set in anyway | |
| 16:33:52 | stephenfin | artom: oh, perhaps the difference here is that I'm saying we shouldn't be _updating_ the XML elements | |
| 16:34:09 | stephenfin | we should be regenerating them based on authoritative sources that tell us what they should be set to | |
| 16:34:16 | stephenfin | I figured the value of doing this is to provide such an authoritative source for what the scheduler policy should be | |
| 16:34:40 | sean-k-mooney | stephenfin: this value will always be fifo if realtime is enabled | |
| 16:34:45 | sean-k-mooney | otherwise it will be unset | |
| 16:34:57 | sean-k-mooney | so we shoudl not need any addtion info in the migrate_data | |
| 16:34:59 | artom | And if we ever want to support something other than fifo, it won't change during a live migration | |
| 16:35:01 | stephenfin | right, at the moment that's the case | |
| 16:35:11 | artom | Only during a rebuild with a different image | |
| 16:35:24 | stephenfin | artom: okay, but if it doesn't change how do I get that information | |
| 16:35:40 | artom | stephenfin, you don't need to? Keep the existing one | |
| 16:35:49 | stephenfin | I'm not keeping the original elements though | |
| 16:35:53 | artom | Oh, *facepalm* | |
| 16:35:56 | artom | I get it now | |
| 16:35:58 | sean-k-mooney | stephenfin: if hw:cpu_realtime=true or hw_cpu_realtime=true its fifo] | |
| 16:36:01 | stephenfin | I'm throwing them away and recalculating them | |
| 16:36:01 | artom | You're clobbering first | |
| 16:36:05 | stephenfin | yessss | |
| 16:36:15 | artom | So... don't clobber them :) | |
| 16:36:23 | artom | Or stash the stuff we know won't chang | |
| 16:36:23 | stephenfin | and parsing the existing XML feels gross | |
| 16:36:25 | artom | *change | |
| 16:36:32 | stephenfin | that's what I thought I was doing | |
| 16:36:40 | stephenfin | stashing it in the migrate_data object | |
| 16:36:54 | sean-k-mooney | stephenfin: you dont need to stash this | |
| 16:36:54 | openstackgerrit | Takashi Natsume proposed openstack/python-novaclient master: Add a cleanup for a server in a functional test https://review.opendev.org/743589 | |
| 16:36:56 | stephenfin | i.e. providing an authoritative source | |
| 16:37:07 | artom | Isn't that overkill? | |
| 16:37:14 | sean-k-mooney | you can compute it form the flavor and image | |
| 16:37:27 | stephenfin | sean-k-mooney: Yes, that's exactly what I do | |
| 16:37:30 | stephenfin | on the source side | |
| 16:37:35 | stephenfin | then I stash it in the migrate data | |
| 16:37:37 | stephenfin | :) | |
| 16:37:43 | sean-k-mooney | by they would you do that | |
| 16:37:54 | sean-k-mooney | you can comptue it form the falvor and image anyhwere its needed | |
| 16:38:02 | artom | You're proposing we add a a field to an object that does, like, 42 transits over the wire, instead of parsing some XML in a single line? | |
| 16:38:40 | stephenfin | yes | |
| 16:38:46 | artom | OK :) | |
| 16:38:49 | artom | I disagree :) | |
| 16:38:53 | sean-k-mooney | me too | |
| 16:39:00 | stephenfin | I'm saying our flavor and image metadata code is authoritative and the only thing we can trust | |
| 16:39:02 | sean-k-mooney | also you dont need to parse | |
| 16:39:12 | stephenfin | as evidenced by the fact this bug exists | |
| 16:39:19 | sean-k-mooney | what bug | |
| 16:39:34 | stephenfin | libvirt can do anything to our XML once we pass it off | |
| 16:39:54 | artom | stephenfin, true, but we can still have some expectations | |
| 16:40:10 | sean-k-mooney | oh we are generating <vcpusched vcpus="2-3" scheduler="fifo" priority="1"/> | |
| 16:40:12 | sean-k-mooney | <vcpusched vcpus="3" scheduler="fifo" priority="1"/> | |
| 16:40:17 | stephenfin | we should be trying to avoid introspecting that XML to try guess what was done previously | |
| 16:40:21 | stephenfin | sean-k-mooney: yup | |
| 16:40:25 | sean-k-mooney | ya you dont need to introspect the xml | |
| 16:40:30 | sean-k-mooney | or pass any data | |
| 16:40:40 | sean-k-mooney | you jst need to check the flavor and image extraspecs | |
| 16:40:41 | stephenfin | then I need to hardcode the policy | |
| 16:40:48 | sean-k-mooney | yes | |
| 16:40:57 | sean-k-mooney | its hardcoded today based on if you enable realtime | |
| 16:41:10 | artom | stephenfin, I mean, you're already "parsing" the XML when you're removing the existing vcpusched elements | |
| 16:41:26 | artom | Use that to stash the scheduler | |
| 16:41:40 | stephenfin | sean-k-mooney: right, in a function that's miles away | |
| 16:41:42 | artom | sean-k-mooney, I don't think we have the flavor/image in libvirt/migration.py, there would be plumbing required | |
| 16:41:53 | sean-k-mooney | we do | |
| 16:42:01 | sean-k-mooney | if we have the instance we have the flavor and image | |
| 16:42:22 | stephenfin | artom: I'm not parsing it though. I'm finding it and totally replacing it with my own freshly baked XML | |