| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-09-11 | |||
| 09:11:36 | efried | aspiers: We keep learning the same lessons over and over again but not reacting | |
| 09:11:48 | stephenfin | aspiers: bug people and be as responsive as possible, pretty much | |
| 09:12:13 | alex_xu | stephenfin: emm...I guess because we have no way to force that, the upgrade-check can't check all the hosts' configuration file | |
| 09:12:15 | aspiers | stephenfin: pretty sure I did a lot of bugging - sorry about that ;-) | |
| 09:13:13 | stephenfin | alex_xu: Indeed. Tbh, I don't think it's an issue anyway | |
| 09:13:15 | efried | aspiers: I see the main problems being 1) we commit to too much stuff per release, and 2) we *somehow* need more consistent core engagement throughout the cycle (and it would be nice if less "bugging" was required to achieve that) | |
| 09:13:39 | stephenfin | The NUMATopologyFilter will save us | |
| 09:14:43 | alex_xu | stephenfin: what do you mean "you can't require nova.conf changes simply to ugprade to a new major version"? | |
| 09:14:59 | efried | fwiw my plan for ussuri is to be strict about 1. This was the first release where I had an employer with real business commitments based on the assumption that "approved spec" actually means a thing is expected to get done. (And that's the cycle I had to run for PTL, shoot-self-in-foot) | |
| 09:15:19 | aspiers | efried: yeah. It feels like more resources are required for reviewing in general and the cores are being pulled in too many directions at once | |
| 09:15:32 | efried | ...and therefore the first cycle I really felt the pain of stuff slipping | |
| 09:15:35 | aspiers | ouch | |
| 09:15:50 | stephenfin | alex_xu: I mean if you have Stein-based deployment that's not emitting deprecation warnings, you should be able to upgrade it to Train without changing nova.conf at all and it'll just work | |
| 09:16:08 | aspiers | I can relate - I felt a lot of pressure to land SEV in Train (original plan with AMD was Stein), although it turns out a lot of that pressure was just from me | |
| 09:16:22 | stephenfin | It might start issuing new warnings saying "you will need to do X before you upgrade to U" but that's it | |
| 09:16:27 | kashyap | aspiers: Nice on SEV completion, that's one fo the first things I was looking if it was all merged this morning. | |
| 09:16:34 | stephenfin | efried: Right? ^ | |
| 09:16:44 | aspiers | kashyap: thanks a lot for all your help! | |
| 09:17:34 | efried | stephenfin: that is my understanding, yes | |
| 09:17:40 | stephenfin | Sweet | |
| 09:17:45 | alex_xu | stephenfin: emm...right, we have rule about the configuration file should works with same as before | |
| 09:18:01 | stephenfin | alex_xu: So yeah, tl;dr: we can insist '[compute] cpu_dedicated_set' is configured before U | |
| 09:18:12 | stephenfin | and add an upgrade check to ensure that | |
| 09:18:17 | stephenfin | but not T | |
| 09:18:19 | kashyap | aspiers: NP; glad to be useful. Now I can bug you for Secure Boot stuff -- turning out to be more work than I anticipated :/ | |
| 09:18:27 | stephenfin | because we gave them no warning and no migration path in Stein | |
| 09:18:50 | stephenfin | alex_xu: However, yeah, I don't think this is in an issue. Consider this | |
| 09:19:58 | aspiers | kashyap: bug soon as I'm on vacation next week ;-) | |
| 09:20:01 | stephenfin | Stein-based compute node: reports VCPU, uses NUMATopology.cpuset to determine what cores can be pinned to (with the NUMATopologyFilter simply copying 'cpuset' to 'pcpuset' to keep that passing) | |
| 09:20:16 | kashyap | aspiers: Heh, noted; well-deserved | |
| 09:21:19 | stephenfin | Train-based compute node without new config options: reports VCPU, uses NUMATopology.pcpuset (p!) to determine cores can be pinned to but this == NUMATopology.cpuset (so no need for NUMATopologyFilter to copy 'cpuset' to 'pcpuset') | |
| 09:21:49 | alex_xu | yup | |
| 09:22:04 | stephenfin | Train-base compute node with new config options: reports PCPU, uses NUMATopology.pcpuset (p!) to determine cores can be pinned to and this is unique (again, no need for NUMATopologyFilter to copy 'cpuset' to 'pcpuset') | |
| 09:22:05 | stephenfin | sooo | |
| 09:22:23 | stephenfin | from a placement perspective, things have changed | |
| 09:23:00 | luyao | efried: do you need a unittest for this? https://review.opendev.org/#/c/678450/11/nova/compute/manager.py@2224 | |
| 09:23:10 | stephenfin | but from a NUMATopologyFilter (nova-scheduler) and nova-compute perspective, all three will work perfectly fine together | |
| 09:23:28 | alex_xu | yup, agree | |
| 09:24:01 | efried | luyao: was there a unit test for the exception case before? If so, it should be retained. If not, it wouldn't hurt to add one, but I'm not sure it's necessary. stephenfin, what do you think? | |
| 09:24:38 | luyao | efried: there was one before | |
| 09:24:50 | stephenfin | alex_xu: the _only_ thing that can hurt us is Train-based compute nodes that have no CPU configuration or have new config options but only for shared CPUs ('[compute] cpu_shared_set') | |
| 09:25:06 | efried | luyao: okay, so if it needs to be adjusted based on the code having been moved, do that, but don't remove the unit test. | |
| 09:25:22 | stephenfin | In that case, the second request to placement will return these hosts since they're reporting VCPU. However, they're _really_ reporting VCPU and don't have any PCPU inventory | |
| 09:25:27 | alex_xu | stephenfin: why | |
| 09:25:34 | stephenfin | but, like I said, the NUMATopologyFilter will protect us from that | |
| 09:26:05 | stephenfin | since NUMATopology.pcpuset will be set to None and therefore it will fail to pin | |
| 09:26:27 | luyao | efried: okay | |
| 09:26:56 | alex_xu | stephenfin: ok | |
| 09:26:58 | stephenfin | and obviously if NUMATopologyFilter is disabled for some reason, the instance will fail to schedule when it lands on the host | |
| 09:27:29 | alex_xu | stephenfin: that make senses I think | |
| 09:27:37 | alex_xu | stephenfin: have you see the resize case I wrote at https://review.opendev.org/#/c/681383/2 | |
| 09:27:41 | stephenfin | alex_xu: Phew, glad you understood. It's really hard to put this stuff into words without waffling or going down ratholes :-D | |
| 09:27:42 | stephenfin | Looking | |
| 09:27:53 | alex_xu | stephenfin: hah | |
| 09:28:40 | stephenfin | alex_xu: Could we call that out as a known issue? | |
| 09:29:22 | alex_xu | stephenfin: maybe, I'm thinking whether we request PCPU, and fallback to VCPU, can make that better. | |
| 09:29:30 | alex_xu | but I feel tired, my brain works very slow... | |
| 09:30:14 | stephenfin | Yeah, I guess we'd need to implement the retry logic at a higher level | |
| 09:30:29 | alex_xu | stephenfin: ah, I guess that doesn't work. The problem is placement always can get you same host allocation candidates, but the request is reject by the scheduler filters | |
| 09:31:00 | stephenfin | Yup. So if we wanted to solve that, we'd have to wrap both the call to placement and the call to the scheduler filters | |
| 09:31:15 | stephenfin | and the attempt to schedule to the guest too, in case the user doesn't have NUMATopologyFilter enabled | |
| 09:31:54 | stephenfin | that sounds like a lot of effort for corner case that will only happen while an operator is in the process of upgrading their configuration | |
| 09:33:20 | alex_xu | I feel I just tired | |
| 09:33:20 | stephenfin | alex_xu: If it gets too late for you, I can take over https://review.opendev.org/#/c/681383/ | |
| 09:34:02 | alex_xu | stephenfin: sorry, I need to take a rest little bit. you can free to take over those two patches. I probably online after one or two hours. | |
| 09:34:11 | stephenfin | Sounds good (y) | |
| 09:35:21 | efried | luyao: btw, here's what I did to that provider tree unit test when I was messing around with it locally: http://paste.openstack.org/show/775098/ | |
| 09:35:40 | efried | that last assertTrue fails with copy.copy but succeeds with deepcopy. | |
| 09:38:33 | luyao | efried: get it, thanks. | |
| 09:39:21 | efried | luyao: note that https://review.opendev.org/#/c/678453/26 is in merge conflict (probably aspiers' fault :P ) so you'll need to rebase again. | |
| 09:40:21 | luyao | efried: haha :D | |
| 09:49:43 | luyao | efried: I check the unittests for get_allocations, it tests the function that we move the get_allocations_xx out of | |
| 09:49:53 | luyao | efried: so I have to remove it | |
| 09:50:17 | efried | luyao: then I say just leave that part of the patch as is | |
| 09:50:42 | efried | luyao: stephenfin and I have already reviewed it and accepted it this way, so it's not worth the trouble of changing. | |
| 09:52:26 | luyao | efried: Okay | |
| 09:54:40 | luyao | efried: sorry, confirm again, need try/except or drop? | |
| 09:56:10 | efried | luyao: Sorry, this has gotten confusing. | |
| 09:56:10 | efried | Remove the try/except. I.e. keep this part of the patch as it appears in PS22 https://review.opendev.org/#/c/678450/22/nova/compute/manager.py@2215 | |
| 09:57:04 | luyao | efried: Okay, thanks. | |
| 09:58:56 | efried | luyao: Okay, I've finished re-reviewing the series (except for https://review.opendev.org/#/c/678455/, which I'm still not going to touch) | |
| 09:59:19 | efried | stephenfin: Do you need my re-look at any of the cpu-resources patches at this point? | |
| 09:59:29 | stephenfin | efried: In about a half hour, yeah | |
| 09:59:36 | stephenfin | I think the quota problem is solved | |
| 09:59:53 | efried | sweet | |
| 09:59:56 | stephenfin | So it's just replacing the scheduler conf option with retry logic for placement left | |
| 10:00:02 | stephenfin | almost done with tests | |
| 10:03:35 | efried | luyao: note response on the defaultdict thing -- I still don't think you need it https://review.opendev.org/#/c/678449/21/nova/compute/provider_tree.py@70 | |
| 10:06:58 | luyao | efried: you're right. :) | |
| 10:07:13 | efried | phew. I like being right | |
| 10:08:26 | bauzas | stephenfin: I wonder something | |
| 10:08:54 | bauzas | stephenfin: if we ask for an o.vo field not existing, can we just do something like 'if field not in my_object:' ? | |
| 10:09:07 | bauzas | IIRC, we need to use a specific o.vo method | |
| 10:09:13 | bauzas | unless the field is set to None | |
| 10:09:14 | stephenfin | bauzas: You can do that, yeah | |
| 10:09:17 | stephenfin | For some time now | |
| 10:09:33 | stephenfin | you used to need to use is_attr_set or something like that | |
| 10:09:37 | bauzas | ok, I had concerns about the virt.hardware module in https://review.opendev.org/#/c/671800/ | |
| 10:09:44 | stephenfin | but __contains__ was implemented many moons ago | |
| 10:09:59 | stephenfin | anywhere is particular? | |