| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-18 | |||
| 11:17:57 | kashyap | bauzas: On the full SHA-1, note how I also put in the commit _summary_ -- so there is no confusion at all :-) | |
| 11:18:31 | kashyap | If one is not putting the one-line commit summary next to the short SHA-1, then I agree that adding a full SHA-1 would be useful | |
| 11:21:58 | kashyap | Can do the rel note, if you think it's useful | |
| 11:42:59 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: qemu: Make disk image conversion dramatically faster https://review.openstack.org/640781 | |
| 11:43:00 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: vzstorage: Use 'writeback' QEMU cache mode https://review.openstack.org/643376 | |
| 11:43:00 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: Use 'writeback' QEMU cache mode for Nova instances https://review.openstack.org/641981 | |
| 11:43:01 | openstackgerrit | Kashyap Chamarthy proposed openstack/nova master: libvirt: smbfs: Use 'writeback' QEMU cache mode https://review.openstack.org/643377 | |
| 11:43:28 | kashyap | bauzas: ^ Added the rel note, and the full SHA1 | |
| 11:49:49 | kashyap | Thanks for the review! | |
| 12:45:26 | yaawang | kashyap: hi, have you seen the review of https://review.openstack.org/#/c/642030/ :) | |
| 12:46:52 | kashyap | yaawang: Will look at it shortly. | |
| 12:50:07 | yaawang | Thanks | |
| 13:30:05 | bauzas | efried: when you're back, please look at https://review.openstack.org/#/c/640781/ | |
| 13:36:51 | efried | bauzas: looking... | |
| 13:37:48 | efried | bauzas, kashyap: +A | |
| 13:37:54 | bauzas | ta | |
| 13:40:52 | mriedem | efried: bauzas: kashyap: https://review.openstack.org/#/c/641981/ | |
| 13:40:56 | mriedem | the default changed right? | |
| 13:41:29 | bauzas | mriedem: lemme look | |
| 13:41:45 | kashyap | mriedem: Yes, mea culpa | |
| 13:41:56 | kashyap | That's why I was badgering other reviewers if I missed anything else | |
| 13:42:05 | kashyap | efried: Let me quickly update the help file | |
| 13:42:15 | bauzas | mriedem: ah, good point about the conf opt | |
| 13:42:18 | kashyap | mriedem: There's already a rel note in the other patch; but I'll expand for this, too | |
| 13:42:28 | bauzas | mriedem: I already asked for a relnote in the below change | |
| 13:42:35 | bauzas | yeah this | |
| 13:42:39 | mriedem | they are different changes though | |
| 13:42:50 | kashyap | True | |
| 13:42:58 | kashyap | One is for image conversion, the other is for Nova instances. | |
| 13:43:27 | bauzas | okay, passing a new PS | |
| 13:43:42 | kashyap | Also that help text needs an update (as a separate change, later). I had a lengthy e-mail chain with the QEMU Block Layer folks. Based on that our docs can do an update | |
| 13:43:43 | bauzas | for stopping the CI | |
| 13:43:53 | efried | I didn't realize there was a conf option. It wasn't apparent in the code. | |
| 13:44:06 | openstackgerrit | Sylvain Bauza proposed openstack/nova master: libvirt: Use 'writeback' QEMU cache mode for Nova instances https://review.openstack.org/641981 | |
| 13:44:20 | bauzas | kashyap: ^ | |
| 13:44:30 | bauzas | it blocks the CI | |
| 13:44:59 | efried | wow, I'm still not seeing how that attribute gets set from that conf option. | |
| 13:45:52 | kashyap | Also the whole cache modes help text needs an update (as a separate change), based on a lengthy e-mail chain with the QEMU Block Layer folks. | |
| 13:46:09 | kashyap | Err, disregard that duplicate | |
| 13:51:58 | kashyap | efried: See this: | |
| 13:51:59 | kashyap | LOG.warning('Invalid cachemode %(cache_mode)s specified ' | |
| 13:51:59 | kashyap | if cache_mode not in self.valid_cachemodes: | |
| 13:51:59 | kashyap | disk_type, sep, cache_mode = mode_str.partition('=') | |
| 13:51:59 | kashyap | for mode_str in CONF.libvirt.disk_cachemodes: | |
| 13:52:02 | kashyap | 'for disk type %(disk_type)s.', | |
| 13:52:04 | kashyap | {'cache_mode': cache_mode, 'disk_type': disk_type}) | |
| 13:52:07 | kashyap | continue | |
| 13:52:10 | kashyap | self.disk_cachemodes[disk_type] = cache_mode | |
| 13:52:12 | efried | n-sch meeting in 8 minutes in #openstack-meeting-alt | |
| 13:52:50 | efried | kashyap: Yeah, but I still don't see how that's feeding into the self._disk_cachemode attr | |
| 13:57:05 | mriedem | is it just me or does this seem somewhat risky to be rushing in right before rc1? | |
| 13:57:14 | mriedem | given the complication | |
| 13:57:39 | openstackgerrit | Lance Bragstad proposed openstack/nova master: Remove obsolete policy configuration details from docs https://review.openstack.org/643949 | |
| 13:59:15 | mriedem | in other news, i'm +2 on gibi's bw provider nova docs https://review.openstack.org/#/c/642064/ | |
| 13:59:31 | bauzas | mriedem: what do you make you feel rushing ? | |
| 13:59:52 | bauzas | kashyap's change ? If so, I just stopped the gate run given your good comments | |
| 13:59:57 | bauzas | and I'm fine with discussing it | |
| 14:00:18 | mriedem | we're a few days from RC1, this is a latent bug, and does not seem trivial | |
| 14:00:24 | mriedem | hence it seems risky to me, | |
| 14:00:28 | kashyap | mriedem: I'm fine delaying it | |
| 14:00:31 | bauzas | fair enough, we can take time | |
| 14:00:39 | bauzas | I didn't wanted to rush | |
| 14:00:41 | mriedem | i.e. i don't know if this is totally fine with NFS deployments (not that i particularly care about NFS) | |
| 14:00:59 | mriedem | i know writeback is recommended for rbd users, so is there a bias here? | |
| 14:01:02 | kashyap | mriedem: On that topic, it is. I ran this past the QEMU Block Layer folks over a lengthy thread | |
| 14:01:16 | mriedem | kashyap: yeah i trust you've done your homework | |
| 14:01:19 | kashyap | See the commit message for on the question of NFS | |
| 14:01:20 | mriedem | given the commit message | |
| 14:01:37 | mriedem | well, which commit message? | |
| 14:01:44 | kashyap | 1 sec | |
| 14:01:52 | mriedem | i guess you mean https://review.openstack.org/#/c/640781/9//COMMIT_MSG | |
| 14:01:55 | mriedem | which i also commented on | |
| 14:02:03 | mriedem | but again, those are different patches | |
| 14:02:34 | mriedem | one is image conversion and one is the disk config for local disks yeah? | |
| 14:02:36 | kashyap | mriedem: Ah, just saw your comment. So, that applies to both the patches | |
| 14:02:56 | kashyap | So I should add remark in the patch that changes it for the Nova instances? | |
| 14:03:18 | kashyap | s/?// | |
| 14:03:51 | mriedem | probably, but it's saying that if the host crashes during writes to NFS, it doesn't matter - but with writethrough each write gets an fsync yeah? | |
| 14:03:53 | kashyap | I checked with Kevin Wolf, he said in _both_ cases: (a) for image conversion; and (b) for Nova instances | |
| 14:03:54 | mriedem | and with writeback they wouldn't | |
| 14:03:59 | mriedem | so you'd lose those writes on NFS right? | |
| 14:04:04 | kashyap | ... 'writeback' is fine even in NFS. | |
| 14:04:21 | kashyap | mriedem: Yes, with 'writethrough', each write gets an fsync(). | |
| 14:04:21 | mriedem | does this all assume some minimum version of qemu for that safety behavior? | |
| 14:04:28 | mriedem | b/c as noted, danpb wrote this code | |
| 14:05:18 | kashyap | mriedem: Yes, danpb also incidentally saw and said: "Nova is using the wrong cache mode in both cases, conversion & for guests" | |
| 14:05:23 | kashyap | On minimum version -- | |
| 14:05:56 | bauzas | mriedem: apologies if you felt I trampled your -1 by a new PS, it was just for stopping the gate run, i should have added my own -1 with it | |
| 14:06:02 | kashyap | mriedem: We have it since QEMU 1.1 (released in 2012). I noted it in the commit message with references: see lines 67-71 | |
| 14:07:50 | mriedem | kashyap: ack yeah ok i glossed over that | |
| 14:08:16 | mriedem | but that is also true in the non-image conversion case? | |
| 14:09:03 | kashyap | mriedem: Yest that's what the QEMU Block Layer folks said. Let me confirm once more. | |
| 14:10:09 | kashyap | mriedem: Okay, yes. even for non-image conversion case, 'writeback' is correct. | |
| 14:10:29 | mriedem | so let me ask another way, in what case would anyone use writethrough? | |
| 14:10:38 | kashyap | Good question | |
| 14:11:20 | kashyap | mriedem: You'd use it if your guest OS really really sucks and doesn't issue fluses where data must be safe on disk. | |
| 14:11:25 | kashyap | But note: | |
| 14:12:46 | kashyap | Such "suck OSes" are things RHEL-4 or even older versions of Windows | |
| 14:13:17 | kashyap | FWIW, the commit message and the text I've written is also reviewed by QEMU folks; I wasn't riffing on it :-) | |
| 14:15:53 | mriedem | i'm not sure there is a way to make this configuration per-guest in nova, | |