| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-19 | |||
| 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 | stephenfin | I'd assume, anyway | |
| 15:16:05 | kashyap | Ah, thanks. :-) | |
| 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 | |
| 15:19:13 | aspiers | I was reading through to try to catch up on a year's worth of thinking (not to mention several decades of nova experience...) | |
| 15:19:41 | aspiers | it was linked from https://review.openstack.org/#/c/552122/4 | |
| 15:20:01 | aspiers | efried: but OK, for now I'll just do reset() | |
| 15:20:51 | efried | aspiers: Yeah, that's the right thing. Not sure what to do about the other. Maybe we just leave it alone and hope it doesn't become a problem. Operators always do what they're supposed to, right? | |
| 15:21:35 | aspiers | haha yeah sure, always ;-) | |
| 15:28:36 | aspiers | efried: it works! | |
| 15:30:08 | efried | woot! | |
| 15:31:07 | aspiers | It's been an unexpectedly rough 24 hours on this. Took me many hours yesterday trawling through with pdb to find this microversion issue https://review.openstack.org/#/c/637657/ | |
| 15:31:39 | aspiers | Once things go wrong in the middleware that's a whole can of voodoo worms which I'm not used to opening up | |
| 15:33:22 | efried | holy unnecessary shim, Batman | |
| 15:33:40 | efried | I remember last time I encountered these fake_* methods I griped about how they should just diaf. | |
| 15:33:41 | aspiers | X-) | |
| 15:35:02 | efried | aspiers: in fact, I may demand that we do that instead of the fix you've proposed. | |
| 15:36:00 | aspiers | That would make me sad, because it would introduce a big new dependency to my SEV work | |
| 15:36:36 | efried | aspiers: It wouldn't be a big thang; just needs someone to do it. And since I'm the one being difficult... | |
| 15:36:40 | aspiers | I'm trying to do the right thing by landing the capabilities-to-traits feature and then implementing the SEV trait on top of it | |
| 15:37:06 | aspiers | rather than just tweaking the libvirt driver's u_p_t() | |
| 15:37:48 | efried | okay, I hear you. | |
| 15:37:50 | aspiers | The latter would be the easy way out, but I wanted to give something useful upstream before asking for "our" feature work to be accepted | |
| 15:37:54 | efried | Give me half an hour? | |
| 15:38:06 | aspiers | Oh wow, well sure, if you're that fast :) | |
| 15:38:23 | jangutter | kashyap: wtf's pretty nifty. It's got dry, crud, rest and a bunch of others that are tricky to google. | |
| 15:38:24 | efried | aspiers: I remember thinking it would be pretty easy. 25 minutes of that is going to be refreshing myself on the code. | |
| 15:38:27 | aspiers | It sounded to my inexperienced ears like it would take days | |
| 15:38:39 | efried | aspiers: What test were you running when you hit this? | |
| 15:38:48 | kashyap | jangutter: Yeah. | |
| 15:38:52 | aspiers | efried: the patch which sits on top of it | |
| 15:39:02 | aspiers | efried: https://review.openstack.org/#/c/538498/10 | |
| 15:39:16 | aspiers | but let me upload a working PS 11 | |
| 15:39:30 | aspiers | which will even pass PEP8 *shock* :) | |
| 15:39:37 | openstackgerrit | sean mooney proposed openstack/nova master: libvirt: auto detach/attach sriov ports on migration https://review.openstack.org/629589 | |
| 15:39:46 | efried | aspiers: test_servers presumably? | |
| 15:39:48 | aspiers | turns out that tox -e flake8 pretends to do something useful but actually checks nothing at all ... | |
| 15:39:52 | aspiers | correct | |
| 15:40:06 | aspiers | stestr --test-path=./nova/tests/functional run -n nova.tests.functional.test_servers.TraitsTrackingTests.test_resource_provider_traits | |
| 15:40:16 | aspiers | if you want the quicker version | |
| 15:40:24 | efried | okay, thanks. | |
| 15:40:48 | kashyap | stephenfin: I'm still a bit squirmy about this whole muddling with s390x, and its console devices. Because both "sclp" and "sclplm" are for different purposes, per s390x maintainers. And I don't have access to a machine to test it all | |
| 15:40:54 | efried | aspiers: btw, if you install tox 3.1.1 (uh, which I guess I forced you to do anyway), tox -e functional -- test_resource_provider_traits should be almost as quick. | |
| 15:40:58 | kashyap | So I was just poking the code to see until it poked me back... | |
| 15:41:15 | aspiers | efried: yeah I figure that out, but I find that tox still adds unacceptable overhead | |
| 15:41:28 | efried | for test discovery? | |
| 15:41:30 | aspiers | the {posargs} trick is handy though | |
| 15:41:31 | kashyap | stephenfin: Anyway, so far from our code audit it is mergeable; so no need to belabour more on it now. </EOM> | |
| 15:41:38 | aspiers | not just test discovery | |