| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-27 | |||
| 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 | |
| 20:12:53 | dansmith | yeah, that's unfortunate | |
| 20:12:57 | mriedem | https://github.com/openstack/nova/commit/6e49019fae80586c4bbb8a7281600cf6140c176a is queens btw | |
| 20:12:57 | melwitt | request spec, what's persisted? what isn't? it's a surprise! | |
| 20:13:26 | dansmith | melwitt: right, let's migrate to make it consistent and remove this point of ambiguity for the future :) | |
| 20:13:32 | mriedem | oh shit i guess that was rocky | |
| 20:13:34 | mriedem | i thought it was older | |
| 20:13:47 | melwitt | YEAH told ya | |
| 20:14:03 | melwitt | it was started way before rocky though | |
| 20:14:20 | melwitt | maybe not way before, but in queens | |
| 20:17:21 | mriedem | we don't have a db migration for requestspec.user_id b/c it's a json blob | |
| 20:17:34 | mriedem | oh having said that... | |
| 20:17:41 | melwitt | wait what? json blob | |
| 20:17:42 | mriedem | you can't join on a blob | |
| 20:17:50 | dansmith | wait it's not a column? | |
| 20:18:00 | mriedem | nope | |
| 20:18:01 | mriedem | ha | |
| 20:18:05 | mriedem | man, all that work I did to do all the various user_id populations | |
| 20:18:07 | mriedem | oops | |
| 20:18:12 | mriedem | spec = Column(MediumText(), nullable=False) | |
| 20:18:17 | mriedem | everything in request spec is json blob | |
| 20:18:23 | mriedem | so you can't filter on a join | |
| 20:18:24 | mriedem | wahwah | |
| 20:18:39 | dansmith | isn't project_id a column on reqspec? | |
| 20:18:55 | mriedem | hell no | |
| 20:19:14 | mriedem | https://github.com/openstack/nova/blob/master/nova/db/sqlalchemy/api_models.py#L164 | |
| 20:20:00 | dansmith | ugh, okay, nevermind then | |
| 20:20:13 | mriedem | salt mines averted | |
| 20:20:28 | dansmith | so, if we keep that, | |
| 20:20:42 | dansmith | I'm not sure why we're patching it up in all these places, vs. just on create and doing a migration like normal | |
| 20:20:58 | melwitt | I was thinking to be opportunistic but maybe I went overboard | |
| 20:21:21 | dansmith | it's just a lot harder to reason about correctness | |
| 20:21:29 | melwitt | we do need one for a soft-deleted instance restore at least. but maybe none of the others | |