Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-14
15:51:49 dansmith bauzas: we're persisting InstanceGroup in RequestSpec with stale members for some reason.. why is that?
15:51:55 artom Though I suppose it's conceivable the db migration might not fix stuff either, if mediumtext is too small still
15:52:08 dansmith bauzas: this came because someone with a very large number of members in that group (at the time of that snapshot) can overrun a TEXT field
15:52:17 dansmith artom: mediumtext is very large I think
15:52:20 dansmith like 16mb or something
15:52:34 artom Seriously?
15:52:40 dansmith yup
15:52:50 artom Wait, so if we migrate a DB with like, thousands of rows, we're adding how much space requirement?
15:52:57 artom And how long will such a thing take?
15:53:12 dansmith artom: hence me not wanting to do it unnecessarily :)
15:53:16 dansmith we rewrite that whole table
15:53:21 bauzas dansmith: mmmmm
15:53:36 bauzas dansmith: AFAIK, we lazy-load the members group
15:53:44 bauzas but I could be wrong
15:53:46 cfriesen postgres has unlimited text fields.
15:53:50 dansmith bauzas: no, we write them all into requestspec
15:53:53 artom Yeha, so my SQL knowledge is fuzzy, but if we increase it to 16 MB we're essentially pre-allocating 16 MB for every row, correct?
15:53:57 dansmith bauzas: and explode the database
15:54:02 dansmith artom: yeah
15:54:15 dansmith artom: which is why I'd rather fix the problem, because 64kb should be enough I think
15:54:16 bauzas seriously? :(
15:54:27 dansmith bauzas: https://review.openstack.org/#/c/527799/3
15:54:54 bauzas looking
15:56:46 cfriesen dansmith: I'm seeing docs saying that text colums are stored separately from the row, so would it really preallocate 16MB for each row?
15:56:56 cfriesen (for mysql)
15:57:23 dansmith cfriesen: oh? I didn't think that's how it worked
15:57:56 cfriesen I'm *not* a mysql guy, so do your own research
15:58:36 artom Where's Monty when you need him
15:58:46 dansmith it doesn't matter that much anyway, as I think fixing the problem will fix the problem
15:58:55 dansmith artom: jaypipes knows a thing or two :)
15:59:08 dansmith it only adds fuel to the discussion though, we need to fix the real problem
15:59:36 artom I think we're just afraid fixing the problem might create another one
15:59:45 artom Like sending rebuild to the scheduler :)
15:59:46 cfriesen dansmith: agreed...I don't see any reason to persist the group members at time of booting
16:00:22 dansmith artom: well, that's why I want bauzas to look at it closely, but also, I think my fix is pretty straightforward
16:00:28 artom dansmith, yeah
16:00:42 cfriesen artom: you need to check the group members right before scheduling.
16:00:45 dansmith it didn't even require functional test change
16:01:06 artom I suppose there's no way for a request_spec to end up with an empty group in code that expects something there
16:01:23 mriedem artom: umm
16:01:41 mriedem it's request spec and groups and scheduler stuff,
16:01:43 artom dansmith, you know objects better than I do, other than _from_db_object we don't generate it in any other way?
16:01:46 mriedem there is totally a way to fuck this up
16:01:48 mriedem w/o knowing it
16:02:04 artom mriedem, oh I agree
16:02:10 artom Which is why I'm thinking about it :)
16:02:13 bauzas mriedem: dansmith: FWIW, just trying to look at earlier code
16:02:20 dansmith mriedem: nobody asserted otherwise I don't think :)
16:02:23 openstackgerrit Stephen Finucane proposed openstack/nova master: SchedulerReportClient._get_providers_in_aggregates https://review.openstack.org/521097
16:02:24 openstackgerrit Stephen Finucane proposed openstack/nova master: Aggregate ops on ProviderTree https://review.openstack.org/526216
16:02:24 openstackgerrit Stephen Finucane proposed openstack/nova master: Traits ops on ProviderTree https://review.openstack.org/521605
16:02:25 openstackgerrit Stephen Finucane proposed openstack/nova master: Track provider traits in report client https://review.openstack.org/521686
16:02:25 openstackgerrit Stephen Finucane proposed openstack/nova master: Move aggregates from report client to ProviderTree https://review.openstack.org/521685
16:02:26 openstackgerrit Stephen Finucane proposed openstack/nova master: Raise on API errors getting aggregates/traits https://review.openstack.org/526540
16:02:26 openstackgerrit Stephen Finucane proposed openstack/nova master: Track associated sharing RPs in report client https://review.openstack.org/526539
16:02:27 openstackgerrit Stephen Finucane proposed openstack/nova master: Track tree-associated providers in report client https://review.openstack.org/526541
16:02:27 openstackgerrit Stephen Finucane proposed openstack/nova master: ProviderTree.populate_from_iterable https://review.openstack.org/520756
16:02:28 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: ComputeDriver.update_provider_tree() https://review.openstack.org/521187
16:02:28 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: Scheduler[Report]Client.get_provider_tree https://review.openstack.org/521098
16:02:29 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: Use update_provider_tree from resource tracker https://review.openstack.org/520246
16:02:41 mriedem dansmith: maybe i misinterpreted "i think it basically boils down to (1) foundatoin doesn't want to pay or plan 2 ptgs, (2) people don't want to upgrade, or feel the pressure to upgrade, every 6 months and (3) people that are now going to multiple events (kubecon+) don't want to feel stretched"
16:02:44 mriedem oops
16:02:50 mriedem "artom: I suppose there's no way for a request_spec to end up with an empty group in code that expects something there"
16:03:07 artom mriedem, yeah, "suppose" being the key word there ;)
16:03:20 artom So, what if we move the loading by uuid into instance group?
16:03:31 artom That way there's definitely no way to end up with an empty one
16:03:32 dansmith mriedem: I think that just meant group and members is not always present, can be null, nothing is depending on that being set to something specific that we might mess up
16:03:34 dansmith which is true I think
16:03:50 bauzas dansmith: oh shit, 'members' is not lazy-loadable, 'hosts' is
16:03:51 artom dansmith, yep, you understood that
16:04:00 bauzas hence my confusion
16:04:22 bauzas dansmith: and since we persist the group object in the DB, yeah I understand the issue
16:04:40 dansmith artom: well, not sure what that matters, since you didn't snip it out before we wrote it into reqspec anyway
16:04:41 dansmith but yeah
16:04:43 dansmith er
16:04:47 dansmith that was for bauzas:
16:05:09 openstackgerrit Stephen Finucane proposed openstack/nova master: ProviderTree.populate_from_iterable https://review.openstack.org/520756
16:05:09 openstackgerrit Stephen Finucane proposed openstack/nova master: Raise on API errors getting aggregates/traits https://review.openstack.org/526540
16:05:10 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: Scheduler[Report]Client.get_provider_tree https://review.openstack.org/521098
16:05:10 openstackgerrit Stephen Finucane proposed openstack/nova master: Track tree-associated providers in report client https://review.openstack.org/526541
16:05:11 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: Use update_provider_tree from resource tracker https://review.openstack.org/520246
16:05:11 openstackgerrit Stephen Finucane proposed openstack/nova master: WIP: ComputeDriver.update_provider_tree() https://review.openstack.org/521187
16:06:20 artom dansmith, so what if we move the loading by uuid down to instance_group? That way we make sure we can never be in a situation with empty members in code that expects them...
16:06:23 artom Right?
16:06:46 dansmith artom: meaning make it lazy-loadable?
16:07:20 artom dansmith, would it make it lazy-loadable? Not necessarily...
16:07:30 artom Actually
16:07:32 artom Yeah it would
16:08:04 dansmith artom: so that won't work if we end up lazy-loading on the compute, because it'll be in a cell and can't get that info later
16:08:10 dansmith artom: we need to pre-load it
16:08:36 dansmith which is kinda the point of reqspec: to pass along everything the stuff downstream needs in order to move the instance
16:09:07 artom Oh right, cells
16:10:05 cfriesen dansmith: would your proposed change to update the instance_group when reading the request spec from DB cover the live-migration case as well? We had a local bugfix to cover that case as part of various affinity/antiaffinity race fixes that haven't made it upstream.
16:10:50 dansmith cfriesen: if I know what you're asking, then it should yeah
16:11:14 dansmith cfriesen: when we load the reqspec in the api before kicking off the live migration, it will have a fresh and accurate set of group members passed down to anything that gets the reqspec in the process
16:12:38 cfriesen dansmith: I'm looking at _find_destination() in live_migrate.py. It calls request_spec = self._get_request_spec_for_select_destinations()
16:15:53 cfriesen dansmith: okay, so looks like that request spec is normally pulled from the DB in compute.api.API.live_migrate(), so we should be good.
16:16:25 dansmith cfriesen: it kinda has to be since it's in the api db
16:17:42 bauzas dansmith: +2 on the fix
16:18:31 artom Semi-related thought

Earlier   Later