| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-12-13 | |||
| 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.. :) | |
| 15:57:42 | efried | the traits cyborg is requesting today are going into the device profile request groups, which isn't where this capability trait is. | |
| 15:58:12 | efried | And the place those request groups are being built from is kinda far from where the root_required will need to be put in. | |
| 15:58:21 | efried | which is why I said "not sure where that will go". | |
| 15:58:30 | dansmith | aight | |
| 15:58:43 | efried | but this is all very solvable, and moves the ball forward wrt using placement features for what we made them for. | |
| 15:58:51 | dansmith | ack | |
| 15:59:10 | efried | viz. string suffixes and rg/rp mappings | |
| 15:59:23 | dansmith | efried: could we do the microversion bump early in the series and get that merged so we don't add a piece to Sundar's puzzle? | |
| 15:59:31 | efried | for sure | |
| 15:59:42 | efried | I can probably work on that. | |
| 15:59:48 | efried | Sundar: ^ | |
| 15:59:56 | dansmith | that would be good, and would be moving the ball a little instead of increasing the weight of the ball | |
| 15:59:56 | efried | I'll try to hack it up this afternoon. | |
| 16:00:04 | Sundar | efried: dansmith: Thanks | |
| 16:00:35 | efried | sean-k-mooney: Are you about today? If not, I can fix up that func test on the image props sync patch and get that moving forward. | |
| 16:01:17 | sean-k-mooney | im around for about an hour but i wont get to it before i head to a team dinner | |
| 16:01:53 | efried | okay. It'll be trivial 1c changes to half a dozen test cases, so I'll still feel good about +2ing. | |
| 16:03:39 | sean-k-mooney | oh i forgot to update the notification samples | |
| 16:04:31 | sean-k-mooney | the notifiction really are a pain | |
| 16:07:22 | efried | yeah, I'm sure they're not worth the trouble. | |
| 16:08:15 | openstackgerrit | Eric Fried proposed openstack/nova master: Sync ImageMetaPropsPayload fields https://review.opendev.org/698613 | |
| 16:08:17 | efried | sean-k-mooney: ^ | |
| 16:08:40 | sean-k-mooney | :) i was just trying to run them locally to reporduce but ill go back to backporting. thanks | |
| 16:11:17 | openstackgerrit | Eric Fried proposed openstack/nova master: DRY: Build ImageMetaPropsPayload from ImageMetaProps https://review.opendev.org/698803 | |
| 16:11:22 | efried | sean-k-mooney: and ^ | |
| 16:11:31 | dansmith | mriedem: are you about to push a change to this? https://review.opendev.org/#/c/638046/56/nova/conductor/tasks/cross_cell_migrate.py | |
| 16:11:42 | efried | gibi, dansmith: That should be an easy win if you could take a quick look | |
| 16:11:50 | efried | https://review.opendev.org/698803 | |
| 16:11:53 | mriedem | dansmith: no those are fup'ed already https://review.opendev.org/#/c/698935/ | |