| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-19 | |||
| 14:49:34 | mriedem | COMPUTE_TRUSTED_CERTS is another https://github.com/openstack/os-traits/blob/master/os_traits/compute/__init__.py#L19 | |
| 14:49:50 | aspiers | got it | |
| 14:50:07 | mriedem | the *only* driver i'm not sure about is ironic since i think nova proxies the traits from the ironic node to placement | |
| 14:50:13 | mriedem | i'm not sure if those are made CUSTOM_ | |
| 14:50:51 | mriedem | that happens here https://github.com/openstack/nova/blob/master/nova/virt/ironic/driver.py#L831 | |
| 14:50:59 | mriedem | looks like the traits on the node are just passed through as-is | |
| 14:51:02 | mriedem | so they can be standard or custom | |
| 14:51:07 | mriedem | jroll: ^ is that accurate? | |
| 14:51:30 | mriedem | https://developer.openstack.org/api-ref/baremetal/?expanded=show-node-details-detail#node-traits-nodes | |
| 14:51:39 | mriedem | "Traits can be either standard or custom." | |
| 14:52:46 | aspiers | mriedem: so the puzzle I'm currently facing with tests is this: I want to remove a CUSTOM_ trait in order to test that the periodic update replaces it correctly. But if I delete using the placement API fixtures then the trait is still in the provider tree, so it doesn't recognise that the traits are out of sync due to https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1046 | |
| 14:53:22 | aspiers | if I comment out that return then it works, but I'm not sure if that will break something else | |
| 14:54:23 | jroll | mriedem: they can only be custom: https://developer.openstack.org/api-ref/baremetal/#node-traits-nodes | |
| 14:54:27 | jroll | er wait | |
| 14:54:32 | mriedem | so set_traits_for_provider is being called with the traits from the provider tree in the compute driver rather than the traits from the placement API? | |
| 14:54:34 | jroll | no, they can be both, you're right | |
| 14:55:34 | aspiers | mriedem: correct | |
| 14:55:51 | aspiers | (I think anyway; my head is beginning to hurt a bit at this stage) | |
| 14:56:07 | mriedem | aspiers: that sounds like an efried question | |
| 14:56:11 | aspiers | OK | |
| 14:56:12 | mriedem | and i'm about to jump on a call | |
| 14:56:38 | aspiers | OK np, thanks for the help | |
| 14:56:40 | mriedem | eric should be well rested after his presidents day celebrations | |
| 14:56:43 | efried | aspiers: gimme a sec | |
| 14:56:46 | aspiers | haha | |
| 14:56:59 | efried | yeah, it wasn't what you would call a relaxing day. | |
| 14:57:15 | mriedem | was someone out protesting the border wall again | |
| 14:57:49 | aspiers | %-/ | |
| 14:58:02 | efried | aspiers: should I read back or you wanna summarize? | |
| 14:58:13 | aspiers | I'll try to summarise | |
| 14:58:17 | efried | or that | |
| 14:58:26 | stephenfin | kashyap: What Georg Hoesch has pointed out here looks correct https://review.openstack.org/#/c/632507/12/nova/virt/libvirt/driver.py | |
| 14:58:34 | aspiers | it looks to me like set_traits_for_provider() is currently designed as a push from the provider tree to placement, rather than a bidirectional sync | |
| 14:58:35 | stephenfin | kashyap: If you agree, I'm happy to +2 as-is | |
| 14:58:44 | stephenfin | (Don't know how far you got in splitting up that patch) | |
| 14:59:07 | kashyap | stephenfin: Yeah, just read that. In the last 2 hours I came to the conclusion that we should untangle it | |
| 14:59:27 | kashyap | stephenfin: Phew. I was just about to admit myself to the "sanity ward" | |
| 14:59:29 | aspiers | :) | |
| 14:59:31 | efried | aspiers: true story. Up to now, virt drivers have been fully responsible for knowing what traits compute "owns". | |
| 14:59:42 | kashyap | stephenfin: That consoles stuff is mind-warping (to my weak brain, at least) | |
| 14:59:55 | aspiers | efried: so the puzzle I'm currently facing with tests is this: I want to remove a CUSTOM_ trait in test_servers.py in order to test that the periodic update replaces it correctly. But if I delete using the placement API fixtures then the trait is still in the provider tree, so it doesn't recognise that the traits are out of sync due to | |
| 14:59:56 | stephenfin | kashyap: It's a maze. So many conditionals | |
| 14:59:57 | aspiers | https://github.com/openstack/nova/blob/master/nova/scheduler/client/report.py#L1046 | |
| 14:59:58 | kashyap | And I'm glad I Cced Georg to also double-check. He writes thorough comments | |
| 15:00:03 | efried | aspiers: This will be (I think) the first time code outside of the virt driver, but inside n-cpu, will assert "ownership" of traits. | |
| 15:00:16 | stephenfin | kashyap: So, happy for me to go ahead and approve this as-is? | |
| 15:00:21 | kashyap | stephenfin: Yeah, did you see the reworked table? It was a cathartic relief to turn the tri-state to binary | |
| 15:00:24 | kashyap | stephenfin: Yes, please | |
| 15:00:50 | stephenfin | done and done | |
| 15:00:54 | efried | aspiers: okay, I understand the problem. Give us a tick to suss it out... | |
| 15:01:12 | kashyap | stephenfin: Excellent. | |
| 15:02:10 | kashyap | stephenfin: Unrelated: on the top-most patch, on that nuking of 'if' conditional from _create_pty_device() -- I noticed it, had a draft comment to myself, before I saw yours. | |
| 15:02:14 | kashyap | Will address that, too | |
| 15:02:48 | stephenfin | kashyap: ack | |
| 15:03:49 | aspiers | efried: this probably doesn't help, but this is how things work with the capabilities patch from mriedem / me: https://goo.gl/haARAc | |
| 15:04:14 | aspiers | I didn't get as far as modelling the sync with placement | |
| 15:04:27 | aspiers | because I didn't expect to need to change that | |
| 15:04:40 | efried | aspiers: Okay, so this is pretty tricky. We implemented a SIGHUP trap to trigger the report client to flush its cache and re-pull everything from placement. The operator is *supposed* to SIGHUP n-cpu any time they make an "out-of-band" change like the ones you're simulating here. | |
| 15:05:02 | efried | aspiers: So technically what you need to do is call ComputeManager.reset() before you hit the periodic and your test will work. | |
| 15:05:18 | aspiers | so that reset is more heavyweight than a periodic update? | |
| 15:05:22 | efried | aspiers: But it points to an interesting real-world issue, which is: what if the operator doesn't do that? | |
| 15:05:27 | aspiers | right | |
| 15:05:45 | efried | aspiers: Yes, it triggers the report client to blow away its provider tree cache completely, so on the periodic it will go re-retrieve everything. | |
| 15:05:58 | aspiers | do we really need that have_traits_changed() check? is it more than an optimisation? | |
| 15:06:37 | efried | it is exactly an optimiz^Hsation, so that we don't have to query placement every periodic as to whether things have changed. | |
| 15:06:46 | aspiers | ah yeah | |
| 15:06:51 | aspiers | how often is the periodic? | |
| 15:07:00 | efried | uh | |
| 15:07:17 | aspiers | I guess with 10k compute nodes it would be a problem even if it was hourly | |
| 15:08:11 | efried | yes, exactly. And in fact we implemented a way to switch off the association refresh entirely. So there's a timer that's separate from the periodic timer, and if that one expires, we go re-pull anyway. | |
| 15:08:36 | aspiers | hrm | |
| 15:08:37 | efried | but CERN wound up having to set that to ~inf or it brought their data center to its knees. | |
| 15:08:42 | aspiers | haha | |
| 15:09:03 | efried | and in fact, we found we don't need it | |
| 15:09:11 | efried | so we enabled setting it to zero, meaning "off". | |
| 15:09:16 | efried | this just merged a couple weeks ago. | |
| 15:09:28 | aspiers | interesting | |
| 15:10:14 | efried | so what would happen is, if the report client felt like it needed to update anything about the traits after your oob update, it would bounce on a generation conflict, re-GET, redrive its tweak, and redo the push. | |
| 15:10:16 | aspiers | so my test could just call self.compute.manager.reportclient.clear_provider_cache() ? | |
| 15:10:28 | efried | yes. And that's the right thing... for your test. | |
| 15:10:29 | aspiers | or reset() directly | |
| 15:10:43 | efried | I'm just not super happy with the real-world issue it points to. | |
| 15:10:49 | aspiers | agreed | |
| 15:12:24 | aspiers | I guess longer term traits from compute drivers could be marked as read-only? | |
| 15:12:33 | aspiers | so then they couldn't be deleted via placement API | |
| 15:12:39 | aspiers | even CUSTOM_* ones | |
| 15:13:10 | aspiers | only the driver which originated it could delete it | |
| 15:13:25 | aspiers | or traits could have an owner associated explicitly | |
| 15:14:06 | aspiers | so then creation and deletion would always both one-way pushes from provider tree to placement | |
| 15:15:20 | efried | aspiers: Have a gander at this: https://github.com/openstack/nova/blob/master/nova/tests/functional/libvirt/test_report_cpu_traits.py | |
| 15:15:48 | kashyap | stephenfin: What do you mean by a "UT"? | |
| 15:15:49 | efried | aspiers: Yes, we discussed those things a bit at a PTG or two, decided on this. | |
| 15:15:57 | stephenfin | kashyap: Unit Test | |
| 15:16:05 | kashyap | Ah, thanks. :-) | |
| 15:16:05 | stephenfin | I'd assume, anyway | |
| 15:16:27 | kashyap | stephenfin: Yes, you win, because the `wtf` tool actually detects the acronym | |
| 15:16:45 | kashyap | $> rpm -qf `which wtf` | |
| 15:16:46 | kashyap | bsd-games-2.17-57.fc28.x86_64 | |
| 15:17:03 | kashyap | It's super-tiny RPM. And comes with an acryonym file | |
| 15:18:25 | aspiers | efried: I found this post-Dublin conversation http://eavesdrop.openstack.org/irclogs/%23openstack-nova/%23openstack-nova.2018-03-12.log.html#t2018-03-12T16:09:22 | |