| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-27 | |||
| 19:33:16 | efried | buck about to be passed | |
| 19:33:51 | jaypipes | damn | |
| 19:33:51 | mriedem | https://review.openstack.org/#/c/546713/ | |
| 19:33:53 | efried | I'm not really concerned about whodunit; what's the right thing here? Should we make it a rule that compute driver capability traits must be standard, and anything that's not will be ignored? | |
| 19:33:56 | mriedem | "I don't seehas_imagecachesupports_recreatesupports_migrate_to_same_host" | |
| 19:34:01 | mriedem | "None of those things are capabilities that a flavor or image would require." | |
| 19:34:33 | efried | that conversation sounds familiar | |
| 19:34:43 | efried | I was apparently convinced enough to +2 the thing. | |
| 19:34:52 | mriedem | "Eric Fried Mar 29, 2018 Patch Set 1: Code-Review+1Okay, I'm convinced we don't need the missing capabilities as traits. Thanks for your patience, all." | |
| 19:35:01 | efried | artom signed off on it too. | |
| 19:35:42 | mriedem | i think i would agree that if a compute driver capability is going to be reported as a trait, it should probably be a standard trait | |
| 19:35:42 | efried | so, fine, we decided we didn't need them to be standard, and the reason was because we didn't think they would need to be scheduled to. So the question remains: do we need to put them onto the compute node as traits? (Given ^ they would be CUSTOM_ fo sho) | |
| 19:35:49 | artom | efried, yeah, but that's like saying a kid ate some candy ;) | |
| 19:36:07 | artom | And in my defense, 'twas me who started this whole kerfuffle we're in now | |
| 19:36:27 | efried | yup, so it's only fair that the blame ended up back atcha. | |
| 19:36:54 | artom | But... was it a *useful* kerfuffle? | |
| 19:36:56 | mriedem | especially because if we added a new driver capability, like supports_trusted_certs, and before it's standard we report CUSTOM_SUPPORTS_TRUSTED_CERTS and then we have a standard trait and it becomes COMPUTE_TRUSTED_CERTS, that's going to be a weird situation for anyone building flavors requiring the former | |
| 19:37:07 | artom | Or did we just re-hash a past discussion to arrive at the same conclusion? | |
| 19:37:22 | efried | artom: No, this is a slightly new discussion and a decision that needs to be made. | |
| 19:37:29 | efried | but we did have to rehash the old discussion as a prerequisite :) | |
| 19:37:34 | artom | \o/ I'm useful! | |
| 19:37:38 | mriedem | i kind of also wish we weren't discussing the design of this ~1 week from feature freeze | |
| 19:37:49 | artom | Sorta | |
| 19:37:51 | efried | mriedem: Yes, there should be no reason we need to introduce a standard trait but wait to use it in nova. | |
| 19:37:54 | mriedem | because i'm going to say "sure whatever you want idk anymore" | |
| 19:38:02 | mriedem | and in a year we'll be saying "wtf did we do this?" | |
| 19:39:17 | efried | jaypipes: Do you agree with mriedem? To paraphrase: If we're going to tag the compute RP with a driver capability trait, it should be a standard trait; and conversely we should not tag the compute RP with a driver capability trait that is not standard; and therefore we should never tag the compute RP with a CUSTOM_* capability trait. | |
| 19:39:36 | jaypipes | reading... | |
| 19:40:06 | efried | (note use of 'capability' throughout - obviously the compute RP can get CUSTOM_* traits from elsewhere - admin, update_provider_tree, etc.) | |
| 19:42:10 | jaypipes | efried, mriedem: I feel the same as I did back then. If the thing is a capability of the compute node that an end user could feasibly want to influence scheduling, it should be a standardized os-trait. if it's just an internal implementation of the virt driver that isn't something an end-user would ever care about but is important for the virt driver infrastructure, just make it a simple attribute of the virt driver class. | |
| 19:43:01 | efried | okay, I'll mark up the review for aspiers | |
| 19:43:28 | mriedem | jaypipes: yeah i agree | |
| 19:43:45 | mriedem | as for why the patch was written how it was when i did a year ago, i can only guess (and already did) | |
| 19:43:49 | jaypipes | in the case of trusted certs, if this is something an end user will say "get me on a host that supports trusted certs", then it should be in os-traits as COMPUTE_TRUSTED_CERTS or whatever. | |
| 19:44:00 | mriedem | i believe it is | |
| 19:44:10 | jaypipes | and it sounds like end users do want that (via a flavor of course) so yeah. | |
| 19:44:20 | mriedem | https://github.com/openstack/os-traits/commit/3e0a6ea4f5045e0bdfe5a2e8097a184ba2c93c5a#diff-98123dc3728cad793e21fa497cf05723 | |
| 19:44:23 | mriedem | it's not really the end user | |
| 19:44:28 | mriedem | it's not something on a flavor or image | |
| 19:44:43 | mriedem | it's that the user says 'i want you to honor my request' and we send you to some compute that has no f'ing clue what that thing is | |
| 19:45:09 | mriedem | i.e. placement request filters | |
| 19:45:16 | mriedem | same for multiattach volumes and device tags | |
| 19:45:21 | jaypipes | that's scheduling though. :) | |
| 19:45:31 | jaypipes | but yes. | |
| 19:46:02 | mriedem | my point was just trusted certs are not a flavor/image thing | |
| 19:46:11 | mriedem | you can't build aggregates around them | |
| 19:47:13 | jaypipes | ack | |
| 19:48:41 | mriedem | efried: aspiers: ok documented https://review.openstack.org/#/c/538498/19/nova/virt/driver.py@1034 | |
| 19:49:23 | efried | mriedem: nice, race condition, correctly assumed "furiously" | |
| 19:52:33 | melwitt | the middle of being scheduled (and something older than rocky should be still in the middle of being scheduled) | |
| 19:52:33 | melwitt | mriedem, dansmith: thanks for the comments on the InstanceMapping.user_id patch. I just replied to mriedem. dansmith, build request doesn't have a user_id field, and request spec has had the user_id field only since rocky. do you think that's ok to rely on? I had been thinking we have to account for request specs older than rocky, but maybe that doesn't make sense because we're trying to account for old instance mappings that are in | |
| 19:52:57 | melwitt | *shouldn't | |
| 19:53:22 | dansmith | melwitt: right, it seems only worse to rely on something that has only existed since stein, but I thought it was on reqspec from well before that | |
| 19:53:23 | dansmith | either way, | |
| 19:53:37 | melwitt | it was added to reqspec in rocky | |
| 19:53:38 | dansmith | *all* IMs are missing it now, only some reqspecs are | |
| 19:53:40 | dansmith | okay | |
| 19:53:57 | melwitt | https://github.com/openstack/nova/commit/6e49019fae80586c4bbb8a7281600cf6140c176a | |
| 19:54:03 | dansmith | just seems like a joined query would be plenty fast, and then we're not just duplicating this again and again | |
| 19:54:10 | dansmith | once per instance per db should be enough :D | |
| 19:54:30 | dansmith | that's two years ago.. is that rocky? | |
| 19:54:54 | melwitt | yeah, I thought that would be older but I notice the included in version shows 18.0.0 | |
| 19:55:13 | dansmith | should be queens right? | |
| 19:55:25 | dansmith | er, actually | |
| 19:55:28 | dansmith | maybe before that? | |
| 19:55:30 | melwitt | must have taken a long time to merge. queens is 17.0.0 | |
| 19:55:34 | dansmith | queens was early '18 | |
| 19:55:50 | melwitt | it merged may 2018 | |
| 19:55:59 | melwitt | https://review.openstack.org/565340 | |
| 19:56:06 | dansmith | ohh, I see, the 2017 date is the first git date I guess | |
| 19:56:11 | melwitt | yeah | |
| 19:56:27 | dansmith | well, anyway, same argument I think | |
| 19:56:44 | melwitt | I'm totally cool with not caring about request specs older than rocky if I got that reassurance | |
| 19:57:10 | dansmith | regardless, you could drop the first two patches and just change how you query for it whenever you do that | |
| 19:58:08 | dansmith | we could fix up old reqspecs on server GET for that matter | |
| 19:58:33 | dansmith | change our IM query to be IM joined with reqspec.user_id, and if null, set it to the instance.user_id we get from the cell before returning to the user | |
| 19:58:44 | dansmith | that would be pretty low overhead I think, | |
| 19:59:18 | dansmith | or just tell people if you want this to be accurate, make sure your migrations are done, just like would have to be the case if we added a new column to IM | |
| 20:00:09 | melwitt | migration for what, request spec? the addition of user_id didn't come with a migration | |
| 20:02:32 | melwitt | dansmith ^ | |
| 20:02:55 | dansmith | melwitt: but you'd need one for your column, right? and I'm not sure why we don't have one for the reqspec one | |
| 20:03:06 | melwitt | oh, you mean me add one | |
| 20:03:10 | melwitt | yeah, ok | |
| 20:03:37 | melwitt | man, all that work I did to do all the various user_id populations | |
| 20:03:44 | dansmith | I'm saying just add one migration, for the field we have, which would be enough for all cases, instead of adding it to another place that has to also get patched up | |
| 20:04:02 | melwitt | I think I'm following now. thanks | |
| 20:04:24 | melwitt | back to the salt mines | |
| 20:06:12 | dansmith | melwitt: why do we not already have a migration for user_id on reqspec? | |
| 20:06:22 | dansmith | did that just get missed maybe? | |
| 20:06:42 | melwitt | dansmith: I dunno. probably wasn't considered important for already existing records | |
| 20:06:54 | dansmith | any idea why? | |
| 20:07:56 | melwitt | no, that's my guess | |
| 20:08:14 | melwitt | mriedem: do you happen to know why we didn't do nova-manage data migration for RequestSpec.user_id? https://review.openstack.org/565340 | |
| 20:08:33 | dansmith | seems really weird to me | |
| 20:08:46 | melwitt | aye | |
| 20:09:04 | dansmith | I think because it's for "out of tree filters" and thus wasn't really done completely | |
| 20:09:35 | dansmith | just another reason not to further proliferate "I need this duplicate info over here so I'll add it again" kinda stuff | |
| 20:10:08 | melwitt | yeah. I was thinking in the box | |
| 20:11:38 | mriedem | melwitt: was never added - as dansmith said, it was added for an out of tree filter (mgagne) that relied on the user_id and therefore a full-fledged data migration wasn't added for existing request specs because we hack them prior to scheduling during moves | |
| 20:11:48 | mriedem | but any persistence of that request_spec.user_id update is purely accidental | |