| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-12-14 | |||
| 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 | |
| 16:18:54 | artom | Will the code actually fix existing cases? | |
| 16:19:09 | dansmith | artom: it will, because: | |
| 16:19:12 | artom | Ah, yeah, it sets members to None when saving | |
| 16:19:12 | bauzas | artom: you mean upstream bugs :p | |
| 16:19:23 | dansmith | artom: if you are under the limit now, then you are good.. if you're over, this will make you fit now | |
| 16:19:35 | artom | bauzas, no, I meant our downstream bug | |
| 16:19:36 | bauzas | exactly | |
| 16:19:41 | dansmith | artom: and, the api will immediately stop reading the stale group after this fix, for reqspecs with stale data from before the fix | |
| 16:19:46 | bauzas | artom: I do understand but we're upstream :) | |
| 16:20:16 | artom | bauzas, well, yes, but it's still relevant to talk about whether we're actually fixing the existing problem, or just preventing it in the future | |
| 16:20:24 | bauzas | it'll pull fresh data indeed | |
| 16:20:39 | dansmith | that's why I did it that way | |
| 16:20:46 | jaypipes | cfriesen, artom: TEXT/MEDIUMTEXT/VARCHAR fields are only pre-allocated at full size when the field is used in a GROUP BY or ORDER BY expression. Otherwise, only a pointer to the data in the data file is used during the projection phase of the SELECT expression (when the rows are actually sent to the client) | |
| 16:20:51 | dansmith | because we have to account for all the broken data in the db now | |
| 16:21:00 | jaypipes | cfriesen, artom: this is for MySQL, not PostgreSQL | |
| 16:21:04 | bauzas | dansmith: and you did it the right way, thanks :) | |
| 16:21:17 | dansmith | jaypipes: we're talking about allocation on disk, not memory | |
| 16:21:39 | dansmith | jaypipes: like whether 1 byte in a MEDIUMTEXT takes up 16mb on disk | |
| 16:22:04 | jaypipes | dansmith: there's almost no difference between a TEXT, MEDIUMTEXT or VARCHAR(8000) w.r.t. on-disk storage requirements or layout. | |
| 16:22:11 | jaypipes | dansmith: for InnoDB that is. | |
| 16:22:12 | bauzas | dansmith: FWIW, I fixed the problem way in the past for hosts https://github.com/openstack/nova/commit/c48c1098cbcd7a9c7980d5fbe4668c38b16767f5 | |
| 16:22:31 | dansmith | jaypipes: okay | |
| 16:22:44 | bauzas | dansmith: but I didn't thought it would be a huge problem for members, given not a lot of operators use a long list of them | |
| 16:23:04 | jaypipes | dansmith: no, for all those field types, InnoDB stores a small amount of data in the data page itself and makes room (when needed) in separate extents by providing a pointer to that extent/page from the main data page. | |
| 16:23:43 | jaypipes | dansmith: the only thing that can be performance-wise a bad thing is if the size of those fields changes often (which isn't the case for these fields in Nova's DB) | |
| 16:23:44 | artom | So for MySQL/InnoDB a migration to mediumtext would actually make sense... | |
| 16:23:55 | jaypipes | artom: for InnoDB, yes. for NDB, no... | |
| 16:24:07 | jaypipes | artom: but that's a different problem entirely :) | |
| 16:24:24 | artom | Do we support NDB? | |
| 16:24:40 | jaypipes | artom: what is the current column type for this field you're talking about? | |
| 16:24:48 | bauzas | mriedem: jaypipes: https://review.openstack.org/#/c/527799/ looks legit to me | |
| 16:24:52 | dansmith | jaypipes: and what is the cost of resizing to medium from text? | |
| 16:25:01 | jaypipes | dansmith: virtually zero. | |
| 16:25:04 | artom | jaypipes, dansmith answered that for me | |
| 16:25:12 | dansmith | jaypipes: awesome, then we should do that too | |
| 16:25:19 | dansmith | artom, mriedem ^ | |
| 16:25:33 | dansmith | still need the real fix, but sounds like a backport is too good to pass up | |
| 16:25:36 | dansmith | of the migration I mean | |
| 16:25:37 | artom | dansmith, that assumes we only support InnoDB | |
| 16:25:38 | jaypipes | dansmith: I don't recommend mediumtext anyway. just set it to TEXT | |
| 16:25:46 | artom | Which... is it the case? | |
| 16:25:46 | dansmith | jaypipes: text isn't big enough | |
| 16:26:02 | artom | Surely some people are running Galera | |
| 16:26:05 | dansmith | artom: I think everyone would be running innodb in prod, AFAIK | |
| 16:26:07 | jaypipes | dansmith: TEXT > MEDIUMTEXT | |
| 16:26:21 | dansmith | jaypipes: um I don't think so | |
| 16:26:33 | jaypipes | sorry, yeah... | |
| 16:26:48 | dansmith | https://stackoverflow.com/questions/13932750/tinytext-text-mediumtext-and-longtext-maximum-storage-sizes | |
| 16:26:53 | jaypipes | was thinking LONGTEXT | |
| 16:27:00 | bauzas | yeah LONGTEXT no ? | |
| 16:27:05 | artom | Oh, Galera uses InnoDB | |
| 16:27:08 | bauzas | TEXT to LONGTEXT I mean | |
| 16:28:07 | jaypipes | dansmith, artom: what is the specific field we are talking about here? | |
| 16:28:10 | mriedem | dansmith: jaypipes: cool - thanks | |
| 16:28:13 | jaypipes | dansmith: is this metadata value? | |
| 16:28:24 | dansmith | jaypipes: request_specs.spec | |
| 16:28:29 | jaypipes | hmm. | |
| 16:28:40 | dansmith | jaypipes: it's a text we serialize the object into, and because we store instance_group.members, which can be huge, | |
| 16:28:41 | artom | Err, instance_group surely? | |
| 16:28:46 | dansmith | we can generate more than 64kb of data | |
| 16:28:59 | dansmith | artom: instance_group is in the spec | |
| 16:29:07 | bauzas | we serialize all at a whole | |
| 16:29:11 | artom | Ah, doh | |
| 16:29:15 | jaypipes | dansmith: k. then I'd recommend changing it to MEDIUMTEXT from TEXT. | |
| 16:29:23 | dansmith | jaypipes: that's what we're doing :) | |
| 16:29:34 | jaypipes | dansmith: ok, sorry for interrupting then :) | |
| 16:29:35 | dansmith | jaypipes: waaaaayyyy ahead of you bro | |
| 16:29:49 | dansmith | jaypipes: and when I say "we" I mean "artom" | |
| 16:29:52 | jaypipes | k | |
| 16:30:22 | artom | I, wha? | |
| 16:30:32 | jaypipes | yet another reason why storing denormalized data in a TEXT-y field is a bad idea... | |
| 16:30:39 | artom | I've barely had breakfast and it's 11:30, I'm not ahead of anything | |
| 16:30:47 | openstackgerrit | Merged openstack/nova master: Fix the bug report link of API Guide https://review.openstack.org/527660 | |
| 16:32:17 | dansmith | jaypipes: well, it has its place, IMHO, and the complexity required to store a reqspec normalized is not worth it since it's a snapshot | |
| 16:32:33 | bauzas | I remember we had a thought on that with alaski | |
| 16:33:18 | dansmith | reqspec is an archive snapshot really, which is fine for this, we just need to not snapshot an infinite amount of stale data into it ;) | |
| 16:33:35 | bauzas | and we said it was better to just store the whole object so we would totally decouple what we store from what we expose | |
| 16:33:48 | bauzas | dansmith: yeah, I still agree on it tbc | |
| 16:34:10 | jaypipes | dansmith: +W'd that patch | |
| 16:34:25 | bauzas | we just expose the instance UUID for querying reasons that's it | |
| 16:34:28 | dansmith | jaypipes: thanks | |
| 16:34:43 | bauzas | dansmith: is there another patch related to that about a DB schema modification ? | |
| 16:34:48 | dansmith | jaypipes: and thanks for being my fleshy mysql manual :P | |
| 16:34:59 | dansmith | bauzas: young artom will be presenting it forthwith | |
| 16:35:03 | bauzas | cool | |
| 16:35:10 | jaypipes | dansmith: sounds kinky. | |
| 16:35:19 | bauzas | oh man, I wish I could "OK jaypipes" on my phone | |
| 16:35:26 | jaypipes | lol | |
| 16:35:27 | artom | jaypipes, you got off easy, I'm his young turdgoat | |