| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2018-08-09 | |||
| 12:02:28 | lyarwood | mdbooth: but I don't think it stops us from calling qemu-img to get the virtual size | |
| 12:02:48 | mdbooth | lyarwood: Yes, you're right | |
| 12:03:28 | mdbooth | Something somewhere said it the regression was introduced in a particular change, and that change only added get_allocated_disk_size | |
| 12:03:48 | lyarwood | mdbooth: yeah the bug for this highlights that change first I think | |
| 12:04:23 | mdbooth | Ok. We could also eliminate get_disk_size, but that would be more complex | |
| 12:04:23 | lyarwood | mdbooth: that introduced the first call to qemu-img, then we noticed that broke LM so I introduced the virtual size call | |
| 12:04:48 | mdbooth | We'd have to cache it | |
| 12:05:07 | lyarwood | mdbooth: yeah I think we can do that for virtual size | |
| 12:05:10 | mdbooth | I think. Not hard, but harder. | |
| 12:05:19 | mdbooth | Possibly not worth it harder | |
| 12:05:52 | mdbooth | Incidentally, that is the only use of get_allocated_disk_size | |
| 12:06:07 | openstackgerrit | Merged openstack/nova master: Update nova network info when doing rebuild for evacuate operation https://review.openstack.org/382853 | |
| 12:06:46 | lyarwood | mdbooth: yeah as I introduced it to fix the original over commit issue a while ago | |
| 12:06:57 | lyarwood | mdbooth: where we originally used os.path.getsize | |
| 12:07:50 | lyarwood | mdbooth: I guess we could just use that for the virtual size for files and avoid the call to qemu-img | |
| 12:08:06 | lyarwood | mdbooth: and use your change to get the allocated size | |
| 12:08:11 | lyarwood | mdbooth: then everyone is happy | |
| 12:08:19 | mdbooth | lyarwood: Ok, so now it's a judgement call. The workaround is frankly ugly and puts the onus on users to fix it. However, there shouldn't be many of those users and it's technically simpler. | |
| 12:09:02 | mdbooth | I think os.path.getsize() is unreliable | |
| 12:09:16 | mdbooth | Depends what qcow2 allocation we use | |
| 12:09:24 | lyarwood | true | |
| 12:09:39 | mdbooth | qemu-img is the right tool to use for that | |
| 12:10:50 | mdbooth | lyarwood: So my fix is only going to half the performance impact. Is that enough? | |
| 12:11:17 | lyarwood | mdbooth: it's not going to change the impact | |
| 12:11:35 | lyarwood | mdbooth: we still make a single qemu-img call for the virtual size | |
| 12:11:41 | lyarwood | mdbooth: we only make one now | |
| 12:11:51 | mdbooth | lyarwood: Well we'll only call it once instead of twice, right? | |
| 12:12:04 | lyarwood | mdbooth: nope, https://review.openstack.org/#/c/589513/ reduced it down to one | |
| 12:12:49 | mdbooth | lyarwood: Ah, with that landed there are *no* calls to get_alloated_disk_size | |
| 12:12:58 | mdbooth | \o/ dead code | |
| 12:13:11 | lyarwood | mdbooth: well before you rm -rf it | |
| 12:13:39 | lyarwood | mdbooth: I still think we could use the getsize approach for raw disks | |
| 12:13:47 | lyarwood | mdbooth: and your stat call | |
| 12:14:22 | lyarwood | mdbooth: that way, no workaround, just an additional bugfix for RAW disks | |
| 12:14:58 | mdbooth | Honestly, I'd prefer to avoid doing anything complicated for a legacy code path. Adding if <raw>, elif <qcow2>, elif <lvm>... | |
| 12:15:10 | mdbooth | Doesn't seem like a good plan. I'll take your hack over that. | |
| 12:16:05 | lyarwood | mdbooth: well we already do that here anyway tbh | |
| 12:16:28 | lyarwood | disk_type == file driver_type == ploop etc | |
| 12:23:35 | mdbooth | lyarwood: Right, but we'd be adding at least 1 new code path. Also, we'd still be slow for qcow2 at least sometimes. | |
| 12:25:21 | mdbooth | lyarwood: I'm ambivalent here in case you hadn't picked up :) I'm not necessarily against your hack, just thinking if it's worth the effort to do better. | |
| 12:26:08 | lyarwood | mdbooth: yeah I really don't enjoy touching this stuff tbh as something always comes up but I think the stat/getsize approach for RAW is the best we can offer | |
| 12:27:25 | mdbooth | Incidentally, what scheme do we now have which isn't interested in actual disk usage? | |
| 12:27:30 | mdbooth | That seems odd. | |
| 12:27:55 | s10 | update_available_resource() with this call to get_allocated_disk_size is blocking some other operations only in post_live_migration: https://github.com/openstack/nova/commit/ab1e48f4683315db631be3f0995be6258edf6997 ? Do we really need this call here now? | |
| 12:29:31 | mdbooth | s10: Intuitively I'd say yes, but based on the same assumptions which tell me we should still be interested in actual disk usage. | |
| 12:29:46 | mdbooth | ...which we're apparently not. | |
| 12:30:10 | lyarwood | yeah placement should handle that now, so I think we can actually remove this? | |
| 12:30:23 | mdbooth | lyarwood: But how does placement handle it? | |
| 12:30:41 | mdbooth | Placement doesn't know anything about actual disk usage which the hypervisor didn't tell it. | |
| 12:31:11 | lyarwood | mdbooth: yeah but I didn't think that was coming from the RT but I'm likely wrong. | |
| 12:32:01 | mdbooth | Unless we deprecated disk overcommit? | |
| 12:39:20 | mdbooth | lyarwood: An alternate (but not necessarily 'better'): write virtual size into disk.info. It's possible to do this in a backwards compatible way. We would just read it out for virtual size, update it automatically if it's missing so we don't have to fetch it again, and use state for allocated size. | |
| 12:39:25 | mdbooth | s/state/stat/ | |
| 12:39:55 | mdbooth | lyarwood: It would be fast, accurate, and secure. | |
| 12:40:27 | mdbooth | It would also be a bit more complex, so only worth it if we continue to need the data. | |
| 12:40:57 | mdbooth | If a fast get_disk_size() is required ongoing, I think we should do ^^^. If not, I think we should go with the workaround. | |
| 12:47:34 | openstackgerrit | Lee Yarwood proposed openstack/nova master: WIP libvirt: rewrite _get_instance_disk_info_from_config https://review.openstack.org/589567 | |
| 12:47:36 | lyarwood | mdbooth: I'd rather keep this simple if possible, what about ^ | |
| 12:49:14 | mdbooth | lyarwood: That doesn't eliminate the qemu-img call, though | |
| 12:49:17 | mdbooth | For qcow2 | |
| 12:49:23 | lyarwood | mdbooth: yeah I don't think we can | |
| 12:49:35 | lyarwood | mdbooth: the issue was reported against RAW FWIW | |
| 12:49:43 | mdbooth | You can if you cache it | |
| 12:49:57 | lyarwood | true | |
| 12:50:25 | mdbooth | And then you've also got 1 less code path to test | |
| 12:50:42 | lyarwood | well you still need it if it isn't cached | |
| 12:50:47 | lyarwood | for now | |
| 12:50:55 | lyarwood | but longer term we can remove it | |
| 12:51:05 | mdbooth | Right, but you can put that in a utility call with separate tests | |
| 12:51:08 | lyarwood | mdbooth: are there util methods for reading/writing to disk.info btw? | |
| 12:51:18 | mdbooth | No, we'd need to pull it out of imagebackend | |
| 12:51:34 | mdbooth | (A good thing) | |
| 12:54:37 | mdbooth | lyarwood: But that's conditional on us continuing to need this stuff. If it has a limited shelf life it's not worth it. | |
| 12:55:17 | mdbooth | Although I still don't understand why we don't need allocated disk any more. | |
| 12:57:00 | lyarwood | mdbooth: well we still need it for LM | |
| 12:57:42 | lyarwood | mdbooth: tbh I'd rather land something simple like this as a bugfix and then work to switch to disk.info outside of this in a bp or something | |
| 12:59:12 | mdbooth | lyarwood: Sure. I'd prefer the workaround over the extra code paths for sure. | |
| 12:59:31 | lyarwood | mdbooth: wait, getting confused now, which workaround? | |
| 12:59:36 | lyarwood | mdbooth: os.stat? | |
| 12:59:43 | mdbooth | lyarwood: No, your original one. | |
| 12:59:50 | mdbooth | os.stat() doesn't fix it, we established that | |
| 13:01:05 | mdbooth | lyarwood: So... land your original workaround, with the disk.info thing in reserve. | |
| 13:01:34 | lyarwood | mdbooth: that workaround breaks LM | |
| 13:01:59 | lyarwood | mdbooth: that's why I'm suggesting using os.stat and os.path.getsize for RAW at least | |
| 13:02:28 | mdbooth | How does it break LM, btw? | |
| 13:03:00 | lyarwood | mdbooth: see my comment, LM with non-shared storage where we are creating images on the dest in pre_live_migration | |
| 13:03:42 | lyarwood | mdbooth: if we don't get an accurate virtual size we end up creating images that are too small | |
| 13:04:05 | lyarwood | https://review.openstack.org/#/c/589567/3 - my comment there sorry | |
| 13:04:19 | lyarwood | https://bugs.launchpad.net/nova/+bug/1770640 | |
| 13:04:19 | openstack | Launchpad bug 1770640 in nova (Ubuntu Bionic) "live block migration of instance with vfat config drive fails" [High,Fix committed] | |
| 13:10:41 | mdbooth | lyarwood: We shouldn't be live migrating a config disk anyway | |
| 13:10:46 | mdbooth | That sounds like a different bug | |
| 13:10:58 | mdbooth | We should just host->host copy it | |
| 13:11:35 | lyarwood | mdbooth: yeah the issue wasn't with the config disk but the main instance disk iirc | |
| 13:12:17 | mdbooth | Ok. | |
| 13:12:32 | lyarwood | hmm actually that's vdb | |
| 13:12:43 | lyarwood | but you can see we are mirroring | |
| 13:17:41 | odyssey4me | Hi folks. Is there a conf entry for the number of workers nova-scheduler fires up? | |
| 13:17:43 | mdbooth | lyarwood: Ok, now I understand the interaction. | |
| 13:17:53 | odyssey4me | I can't seem to find one in the references. | |