| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-13 | |||
| 14:45:50 | stephenfin | It was pretty much just functional tests once I got past the remaining conductor tasks. Need to go find someone else now :) | |
| 15:02:41 | openstackgerrit | Eric Fried proposed openstack/nova master: DRY: Build ImageMetaPropsPayload from ImageMetaProps https://review.opendev.org/698803 | |
| 15:10:25 | KeithMnemonic | dansmith: Thanks for pushing that patch through. Do you think you could please also do that last cherry pick in that set? https://review.opendev.org/#/c/690735/2 | |
| 15:11:06 | dansmith | are we still doing stuff for pike? | |
| 15:11:18 | dansmith | mriedem: | |
| 15:13:02 | mriedem | it's in EM (as is queens) so we can if people have a need, | |
| 15:13:19 | mriedem | i know suse cares about it and so does ericsson since they have products based on pike | |
| 15:13:37 | mriedem | and as you know rhosp cares about queens (or at least the internal CI/CD stuff at RHOSP cares about queens) | |
| 15:13:48 | dansmith | yeah, already did the queens one | |
| 15:14:25 | dansmith | EM means "we can throw anything in there anyone cares about without inspecting it or worrying about breakage" right? | |
| 15:14:49 | mriedem | heh, not really | |
| 15:15:03 | mriedem | it just means it's there if people want a common place to backport, we can merge things, but we don't have to | |
| 15:15:13 | dansmith | KeithMnemonic: can you please +1 those patches with "I stake my reputation on the fitness of this patch as I have tested it fully" ? | |
| 15:15:31 | mriedem | https://docs.openstack.org/project-team-guide/stable-branches.html#extended-maintenance | |
| 15:15:56 | dansmith | mriedem: right, I thought we said EM would be like a dumping ground, where we wouldn't try to keep tests working, etc | |
| 15:16:07 | mriedem | "There is no statement about the level of testing and upgrades from Extended Maintenance are not supported within the Community." | |
| 15:16:20 | mriedem | i wouldn't say we don't try, | |
| 15:16:24 | mriedem | but we don't bend over backwards | |
| 15:16:48 | mriedem | if tests are failing for some reason and there is a trivial way to fix, sure, but if things are chronically busted and no one is working on it or it's a lot of work then it's fine to EOL | |
| 15:17:21 | mriedem | iow, EM == best minimal effort, buyer beware | |
| 15:17:40 | Sundar | efried: Re. https://review.opendev.org/#/c/631244/51/nova/conductor/manager.py@1578, when you say: "FWIW, I still don't like this.", what is 'this'? | |
| 15:18:07 | mriedem | dansmith: i've also told KeithMnemonic that suse can just cherry-pick those for their internal pike series and not be blocked on us | |
| 15:18:36 | dansmith | mriedem: yeah, it'd be nice if he'd at least +1 that he wants them and that they've confirmed they're good or something | |
| 15:18:42 | KeithMnemonic | dansmith let me test in on a pike cloud i have and will update the bug. | |
| 15:19:00 | KeithMnemonic | mriedem: we have that cherry pick ready locally if needed | |
| 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 | Sundar | efried: The trait can be used by Cyborg to not publish inventory. | |
| 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: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 | dansmith | Sundar: you can just do A and -W it if you want | |
| 15:33:16 | stephenfin | I was sure you did but I can't find it | |
| 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. | |