| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-02-27 | |||
| 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 | melwitt | request spec, what's persisted? what isn't? it's a surprise! | |
| 20:12:57 | mriedem | https://github.com/openstack/nova/commit/6e49019fae80586c4bbb8a7281600cf6140c176a is queens btw | |
| 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 | |
| 20:21:36 | melwitt | oh wait, no | |
| 20:21:38 | mriedem | i don't think we need the conductor ones | |
| 20:21:42 | mriedem | in https://review.openstack.org/#/c/638574/3 | |
| 20:21:46 | melwitt | since I added soft-deleted instances to the migration | |
| 20:21:48 | mriedem | we want the one on GET /servers/{server_id} | |
| 20:22:01 | dansmith | if we do it opportunitstically, just do it on load, not in 100 places, IMHO | |
| 20:22:03 | dansmith | mriedem: right | |
| 20:22:25 | mriedem | i can't speak for the soft deleted restore thing | |
| 20:22:37 | melwitt | ok, thanks. I pre-emptive optimizationed myself :( | |
| 20:23:12 | openstackgerrit | Eric Fried proposed openstack/nova master: Better handle live migration abort https://review.openstack.org/635440 | |
| 20:23:15 | mriedem | if someone is building servers between the time the scheduler/conductor is new and the api is old, and they miss that race, the data migration can handle it | |
| 20:23:24 | melwitt | no, I think we don't need the soft deleted restore anymore because I included soft deleted in the nova-manage migration. since they could be restored theoretically any time in the future | |
| 20:24:40 | mriedem | so handle create and GET and that's good enough | |
| 20:24:44 | mriedem | data migration takes care of the rest | |
| 20:24:56 | melwitt | ok, I'll clean that up. thanks | |
| 20:30:19 | melwitt | that was an emotional roller coaster | |
| 20:31:24 | openstackgerrit | Merged openstack/nova master: Fix the api sample docs for microversion 2.68 https://review.openstack.org/639707 | |
| 20:45:26 | openstackgerrit | Eric Fried proposed openstack/nova master: WIP: Privatize SchedulerReportClient HTTP wrappers https://review.openstack.org/639818 | |
| 20:45:39 | efried | melwitt: FYI as discussed earlier ^ | |
| 21:00:05 | melwitt | efried: ack | |
| 21:03:46 | mriedem | melwitt: ok i've gone a few more patches up in your series | |
| 21:03:51 | mriedem | to the point of the wip and stopped there | |
| 21:05:15 | melwitt | mriedem: been reading through the comments. thanks. indeed I was trying to account for a scenario like, "what if they run nova-manage in the middle of the upgrade but not at the end, how to catch the stragglers". but I realize I might be suffering from not knowing something like, nova-manage is always run once at the end of the upgrade too | |
| 21:05:54 | mriedem | you run it until there are no more to process | |
| 21:05:58 | mriedem | and then when we drop the compat code, | |
| 21:06:01 | mriedem | we put in a blocker migration | |
| 21:06:42 | mriedem | there is no defined time that operators have to run online_data_migrations, they can run it whenever | |
| 21:06:58 | melwitt | well, if it's run in the middle of the upgrade and run at the exact wrong time, it would come back as "all done" because of the not-yet-scheduled old instance mappings, if there are any | |
| 21:06:58 | mriedem | it should be idempotent, save for the ones that create a marker | |
| 21:07:28 | melwitt | yeah, that's what I had thought, no defined time or way to run online_data_migrations | |
| 21:07:43 | mriedem | but then your counting quota code is going to catch that and do the legacy counting method right? | |
| 21:07:58 | openstackgerrit | Michael Still proposed openstack/nova master: Cleanup no longer required filters and add a release note. https://review.openstack.org/639826 | |
| 21:08:01 | mriedem | and presumably log something if they haven't opted out of counting from placement b/c of edge or something? | |
| 21:08:08 | melwitt | yes | |
| 21:08:16 | mriedem | so i don't think we need to be super paranoid about it | |
| 21:08:24 | mriedem | and we'll add a blocker schema migration before dropping the compat code | |
| 21:08:28 | melwitt | ok, I see. that's helpful | |
| 21:08:32 | mriedem | i.e. you can't upgrade to U if you haven't done your homework | |
| 21:08:43 | melwitt | right | |
| 21:08:54 | mriedem | of course i'd like dansmith to check my work her | |
| 21:08:55 | mriedem | *her | |
| 21:08:57 | mriedem | di | |
| 21:08:59 | mriedem | here | |
| 21:09:23 | mriedem | also, | |
| 21:09:36 | dansmith | I refuse to touch your her | |
| 21:09:38 | mriedem | the GET /servers/{server_id} should catch any that miss that very tight winow | |
| 21:09:41 | mriedem | *window | |