Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2017-12-14
15:41:24 artom I agree that the migration might have more side effects than the load by UUID patch
15:41:25 mriedem i know but i love it so
15:41:27 dansmith and this is something that has been this way since the release and which we've only _just_ heard about
15:41:27 artom Er, less
15:41:44 mriedem yeah, because people are just now upgraded
15:41:44 artom So if we're going to backport something, it might actually be safer to do just the migration
15:41:46 mriedem *upgrading
15:42:02 mriedem well, people with big ass server groups apparently :)
15:42:14 artom Also good point - there's like a 1-2 year lag between upstream and what's actually running
15:42:36 dansmith well, there are definitely people already on newton and beyond, but I understand that reasoning
15:43:02 dansmith mriedem: fwiw, there were only four such instances in a database of tens of thousands that were affected here
15:43:15 dansmith mriedem: and it seemed like it was a very specific case, which might have been testing and/or a script run amok
15:43:26 dansmith just for the data point
15:43:43 mriedem ok
15:44:10 dansmith the fact that we're crystallizing known-bad server groups in the database at creation time is almost more concerning to me than the overrun case
15:44:48 artom Known-bad?
15:46:18 dansmith artom: yeah, so we're capturing the server group (and specifically its members) at the time in which we create/write the reqspec for an instance
15:46:44 mriedem i'm fully on board with fixing that
15:46:48 dansmith so lets say you do this: nova boot --group foo; nova boot --group foo
15:46:49 dansmith the first one has an InstanceGroup with .members=[justme]
15:46:49 artom Ah, and some of those can get deleted, so we're left with a group with non existing members
15:46:54 dansmith the second one is complete
15:47:06 dansmith there is no reason we should be persisting that and then ever using it ever, but we are
15:47:25 dansmith artom: either stale members, or incomplete, depending on when you snapshotted it
15:47:55 artom I'm trying to think if anyone could be relying of this behaviour for whatever reason
15:48:41 artom This is used in stuff like the group (anti)affinity filters, right?
15:48:43 cfriesen dansmith: mriedem: for what it's worth, for reliability you have to update the server group members before rescheduling anyways, otherwise you're working off stale data
15:48:57 gibi mriedem: could you check back to https://review.openstack.org/#/c/527444/ I start seeing the related bug appearing more and more on the gate
15:48:59 dansmith cfriesen: right
15:49:16 mriedem gibi: yeah
15:49:32 gibi mriedem: thanks
15:51:12 bauzas dansmith: hola, what's the point about the request spec ?
15:51:19 artom So are we coming to the consensus that we fix the stale group membership in master, but to play it safe only backport the db migration?
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

Earlier   Later