Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-12-13
15:19:13 dansmith KeithMnemonic: you haven't actually tested these?
15:19:21 KeithMnemonic but since the patch is still there and not pulled i figured it would be best to see if it can go in upstream
15:19:25 KeithMnemonic i did
15:19:50 KeithMnemonic on rocky
15:20:04 KeithMnemonic let me go back and do pike as well
15:20:08 KeithMnemonic it wont take me long
15:20:23 dansmith yeah, that's not the same
15:20:27 mriedem note that in at least one case i'm aware of, we had merged a backport to rocky and someone from canonical came back to me later saying it didn't actually fix the bug and they were right and we needed to follow up with a rocky only fix
15:20:28 KeithMnemonic ok fair enough
15:20:46 mriedem so, it happens
15:21:31 KeithMnemonic so that is funny, sort of i just got merged a cheey pick for a missing "prev" button on the horzion instance panel, that is actually broken in master
15:22:21 efried Sundar: I don't like "Hey, also send a required trait asserting that the compute is capable of accelerator stuff, because the fact that it's exposing inventory isn't sufficient"
15:23:05 dansmith efried: it's not exposing inventory, cyborg is exposing the inventory, right?
15:23:23 KeithMnemonic and mriedem they are telling me it is due to nova api behavior ;-)
15:23:38 efried dansmith, Sundar: If a rogue external agent (whether it's cyborg, neutron, random $service, an admin) goes around adding shit to the provider tree, all bets are off as far as I'm concerned.
15:23:38 Sundar efried: The trait can be used by Cyborg to not publish inventory.
15:23:58 efried Yes, that ^ is cyborg not being rogue, and it is sufficient IMO.
15:24:14 efried dansmith wants to not trust cyborg et al that far.
15:24:16 dansmith efried: then we should do a service version check
15:24:55 Sundar efried: does that not apply to any trait?
15:24:56 efried I'm not going to block on this. I appreciate the reasoning. I just don't like it. So you can ignore that part and focus on the "if you're going to do it, you need to do it this way" part of the comment.
15:24:57 dansmith efried: we have gone a long way with not breaking our internal RPC APIs, please give that the credit due, and let's use the tools (any of them) that we have to keep those guarantees strong
15:27:33 Sundar dansmith: We discussed 2 changes: A. Per-ARQ events B. Handling old compute nodes, one option being trait usage. I can do Part A now. Are you ok with that partial update?
15:28:15 Sundar Part B perhaps requires more discussion, and Ill do that later
15:28:51 dansmith Sundar: there are multiple parts of B that need to be done, the trait so that at least cyborg can tell, and then the second part is what we do on the nova side.. exposing the trait is not contentious, so you should do that
15:29:08 openstackgerrit Matt Riedemann proposed openstack/nova master: Fix accumulated non-docs nits for cross-cell-resize series https://review.opendev.org/698935
15:29:27 dansmith if you want to wait until january I will be glad to write the hard service version check
15:31:03 mriedem this kind of hard service version check when the trait isn't available? https://review.opendev.org/#/c/695220/4/nova/compute/api.py@3938
15:32:37 Sundar dansmith: OK. Just to be clear, you don't see the point in reviewing A alone now?
15:33:09 stephenfin smcginnis: Didn't you have a patch for the requirements tooling to allow me to do this? https://review.opendev.org/#/c/698895/1/doc/requirements.txt
15:33:16 stephenfin I was sure you did but I can't find it
15:33:16 dansmith Sundar: you can just do A and -W it if you want
15:33:42 dansmith Sundar: today is my last day in the office so it won't likely get done until Jan anyway
15:34:13 openstackgerrit Merged openstack/nova master: Move _get_request_group_mapping() to RequestSpec https://review.opendev.org/696541
15:34:19 dansmith mriedem: yes, that's the sort of thing I'm trying to convince efried is important
15:34:53 dansmith mriedem: the assertion is that cyborg should not add inventory for accels until the computes are upgraded, and that's enough to not break nova's internal and external APIs, and I'd rather us do our own internal check
15:35:05 dansmith since we have the tools and it's trivial to do
15:35:19 smcginnis stephenfin: Yes, that merged, but I think unfortunately you still need to keep the python_version marker.
15:35:20 dansmith the cyborg logic could easily be wrong, removed, altered, etc
15:36:16 stephenfin smcginnis: Ah, I thought it was fixed because [1] passed. Turned out that didn't run the requirements job /o\ [1] https://review.opendev.org/#/c/697004/
15:36:31 smcginnis Here's the requirements change: https://review.opendev.org/#/c/693631/
15:37:43 efried dansmith: in mriedem's example, he's going to remove the RPC version check once we can count on the compute exposing the trait.
15:37:55 stephenfin smcginnis: Thanks. Would a change to make the 'python_version' things optional if 'python_version' > 3.x make sense?
15:37:57 stephenfin Thinking out loud
15:38:27 efried dansmith: But he only needs that because the thing he's checking was still possible in a previous version; it just wasn't advertised via the trait.
15:38:29 smcginnis Yeah, I think so. That will give us a transition path away from py2 completely.
15:38:44 efried dansmith: so in this case we actually only need the trait check there.
15:38:46 stephenfin Kewl. I'll tackle that Monday
15:38:50 stephenfin Cheers :)
15:38:59 smcginnis stephenfin: +1
15:39:00 dansmith efried: as I said, I'm happy to check the trait
15:39:45 efried My point is, if we do it that way, we don't need to muck with RPC at all. (For that reason -- we may still want to muck with it to pass the arqs across the wire.)
15:40:30 dansmith efried: I think maybe there is some confusion going on
15:40:40 dansmith efried: there are three things we could do here to provide an internal-to-nova check:
15:40:42 efried The thing I was objecting to earlier was having to tack that same trait onto every GET /a_c. But meh, as long as you do it with root_required I'm content.
15:40:55 dansmith 1. Check the trait, or just put that in the request to placement
15:41:00 dansmith 2. Check the service version
15:41:13 dansmith 3. Put it into the RPC request and depend on the RPC layer to not send if not allowed
15:41:35 dansmith I think #3 is the right answer, but I've already said that I won't block on it, but I think we should do either 1a, 1b, or 2
15:41:49 dansmith all of which are trivially easy because we have infrastructure to make it so
15:42:09 dansmith if 1b (ask placement for the trait) is bad for some reason, then fine, although it seems like the least amount of overhead to me
15:42:26 efried Yes, I agree with you.
15:42:52 efried Are you also agreeing that 1b makes 1a/2/3 unnecessary?
15:43:01 efried If we can agree on that, then I'll get fully behind 1b.
15:43:06 dansmith I'm asking for one of those four options
15:43:09 efried cool
15:43:18 dansmith I thought you didn't want 1b?
15:43:21 efried Sundar: I retract my objection to 1b.
15:43:27 efried I didn't, but now I do.
15:43:39 dansmith man I'm glad we're not local
15:43:57 efried What, you gonna put clorox in my coffee?
15:43:59 dansmith I'd smack you, you'd punch me, our lawyers would get boats
15:44:10 openstackgerrit Matt Riedemann proposed openstack/nova master: Use COMPUTE_SAME_HOST_COLD_MIGRATE trait during migrate https://review.opendev.org/695220
15:44:11 efried I don't punch.
15:44:25 efried I thought you were asking for multiple redundant checks.
15:44:27 dansmith karate chop
15:44:42 efried now you're deliberately goading me
15:44:51 dansmith mriedem can cue up a youtube of the final scene of karate kid
15:45:27 dansmith efried: the only duplicity (which I don't consider duplicity) is us backing up the proper behavior of cyborg with a check, tht's all
15:45:45 mriedem https://www.youtube.com/watch?v=oomCIXGzsR0
15:46:32 dansmith mriedem: right, thanks
15:47:17 Sundar dansmith: efried: So, we are all agreed on 1b: Nova compute publishes a trait, Nova scheduler uses that in Placement query, we all party. Right?
15:47:36 dansmith I'm not partying with either of you, but yes, I'm fine with that
15:48:07 mriedem it occurs to me that daniel's secret is the same as defeating bald bull in punch out
15:48:25 Sundar dansmith: efried: Re. the idea that Cyborg should not publish inventory till n-c
15:48:25 Sundar unrequited? ;)
15:50:12 efried Sundar: ...till n-c advertises the trait. And yes, 1b.
15:51:25 Sundar efried: From an operator's POV, if the inventory gets published, she'd know at least that the devices are good and Cyborg is functioning, and they should look elsewhere to get the system functional.
15:53:24 efried I can live with that.
15:55:07 Sundar efried: dansmith: Ok I'll put in the trait definition. The changes for 1b presumably go in my current patch series.
15:55:16 efried yes
15:55:29 dansmith once the trait is there, it should be one line I'd think
15:55:30 efried so you'll need the os-traits chain
15:55:52 efried dansmith: I'm not actually sure where that line will go. But he also needs to bump the GET /a_c microversion.
15:55:58 efried ...which is like 12 lines.
15:56:11 efried (including test)
15:56:26 dansmith efried: why does he need a bump?
15:56:52 efried Because the trait needs to go into root_required. I'm not going to accept new code that hacks it into the unnumbered request group anymore.
15:57:17 dansmith oh, okay.. we have traits requested now of course, but I guess the point is this is a complicated request
15:57:34 dansmith well, service version check is pretty small and no external dependencies, but.. :)

Earlier   Later