)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1dfa81bb29bb2cf5bdaafc81679c57897aca3131","unresolved":false,"context_lines":[{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder reports discard\u003dtrue capability then attach a new"},{"line_number":10,"context_line":"virtio-scsi device and enable discard on the attached drive."},{"line_number":11,"context_line":""},{"line_number":12,"context_line":"Change-Id: Icd2d20bb6906d9106df34a970b3d81b17bc3ec07"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":1,"id":"3a50d1a3_9b77ce01","line":11,"updated":"2015-07-27 18:00:48.000000000","message":"Is this fixing a bug?  Or is this for a blueprint?","commit_id":"4a16ef24b63a33ba64bd8e50ac0605d5bb8979f3"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"05e8b4ff6d0606b85559ca40b9e895dfe743551c","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Daniel Wilson \u003cdaniel.wilson@purestorage.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-07-31 11:51:05 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"libvirt: add discard support for attached volumes"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder reports discard\u003dtrue capability then add"},{"line_number":10,"context_line":"discard\u003dunmap to disk config setting on attached volume."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1a4dcd0f_66085bda","line":7,"updated":"2015-08-10 16:53:10.000000000","message":"When I originally saw this I just saw \u0027add support for *\u0027 which at this point is bad since we\u0027re past feature freeze.\n\nI didn\u0027t realize there were cinder parts related to this that needed to land in liberty.\n\nThis commit message is sorely lacking in detail.  If you\u0027re not going to have a spec or fleshed out blueprint, you should at least have some explanation of what this is for, references to the cinder change(s) for this and what it fixes - and if there are any minimum versions of libvirt/qemu required to use it - as I\u0027ve pointed out in the code.","commit_id":"1800c7fc7b9491bb28aa2464bd315ad5f5844d4c"},{"author":{"_account_id":13868,"name":"Danny Wilson","email":"daniel.wilson@purestorage.com","username":"dannywilson"},"change_message_id":"e151b53f3dce3f5ed4618341ef7cd567817b58c6","unresolved":false,"context_lines":[{"line_number":4,"context_line":"Commit:     Daniel Wilson \u003cdaniel.wilson@purestorage.com\u003e"},{"line_number":5,"context_line":"CommitDate: 2015-07-31 11:51:05 -0700"},{"line_number":6,"context_line":""},{"line_number":7,"context_line":"libvirt: add discard support for attached volumes"},{"line_number":8,"context_line":""},{"line_number":9,"context_line":"If Cinder reports discard\u003dtrue capability then add"},{"line_number":10,"context_line":"discard\u003dunmap to disk config setting on attached volume."}],"source_content_type":"text/x-gerrit-commit-message","patch_set":7,"id":"1a4dcd0f_95e9bdb9","line":7,"in_reply_to":"1a4dcd0f_66085bda","updated":"2015-08-11 23:30:44.000000000","message":"Done","commit_id":"1800c7fc7b9491bb28aa2464bd315ad5f5844d4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"93dc369f4eb4a45892aad3b98e412c780e42ff11","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This functionality requires properties in the glance image"},{"line_number":13,"context_line":"to enable creation of the correct scsi controller on the instance."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Required glance properties are"},{"line_number":16,"context_line":"hw_scsi_model\u003dvirtio-scsi"},{"line_number":17,"context_line":"hw_disk_bus\u003dscsi"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"5a2ca52d_bd0e1539","line":15,"updated":"2015-10-16 14:05:49.000000000","message":"How does this work with the CONF.libvirt.hw_disk_discard config option?\n\nhttps://github.com/openstack/nova/blob/12.0.0/nova/virt/libvirt/imagebackend.py#L66\n\nThat\u0027s checked in the virt driver, but not checked in the volume driver.  So if I want discard\u003dunmap for disk images, I have to set a config option in nova.  If I want discard\u003dunmap with volumes, I have to use (1) the right driver in cinder that supports it and (2) make sure the special properties on my image that I\u0027m using for the volume.  This is a bit confusing, we couldn\u0027t have consolidated this better?\n\nAlso, where are the glance properties validated?  The volume driver just blindly says it supports discard:\n\nhttps://review.openstack.org/#/c/205717/2/cinder/volume/drivers/pure.py\n\nBut I don\u0027t see anything checking that those properties are set on the image when trying to attach a volume with discard\u003dTrue in the connector properties.\n\nWe have the image metadata in the libvirt driver\u0027s attach_volume method:\n\nhttps://github.com/openstack/nova/blob/12.0.0/nova/virt/libvirt/driver.py#L1062\n\nSo I\u0027d think we could have some validation somewhere in this path such that if the connection_info[\u0027data\u0027][\u0027discard\u0027] \u003d\u003d True and those properties aren\u0027t set in the image metadata, we could do some logging.\n\nIt sounds like if you fail to set the properties correctly there is no effect, but that\u0027s a kind of silent fail, so I\u0027d think we\u0027d at least want a log message somewhere saying discard isn\u0027t going to happen on this image since the properties aren\u0027t set.","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"c495c1e3e51c6241b1b26daadbab4f9100f45580","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This functionality requires properties in the glance image"},{"line_number":13,"context_line":"to enable creation of the correct scsi controller on the instance."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Required glance properties are"},{"line_number":16,"context_line":"hw_scsi_model\u003dvirtio-scsi"},{"line_number":17,"context_line":"hw_disk_bus\u003dscsi"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":8,"id":"5a2ca52d_ec192623","line":15,"in_reply_to":"5a2ca52d_bd0e1539","updated":"2015-10-18 02:26:57.000000000","message":"In its current incarnation it does not do anything with hw_disk_discard. We need the glance properties to be set so that the instance will have the virtio-scsi bus created/configured for the vm. Without that Cinder cannot attach a volume and get any Trim commands.\n\nAn earlier version of this (see patchset 2(ish)) would actually just hot-plug the bus in if we need it to, but there was some push back because we couldn\u0027t know if the VM would actually support it.\n\nSo, what we can do is check if the VM is configured with the right type of bus (indirectly checking if the image had the right properties at the time we booted) and log a warning if the discard\u003dunmap business will silently fail (aka do nothing).\n\nSound ok?  Maybe we can hash this out in the spec review if we need more discussion.","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c888127f31b7a9a95f1cec687794f789d3972383","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This functionality requires properties in the glance image"},{"line_number":13,"context_line":"to enable creation of the correct SCSI controller on the instance."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Required glance properties are"},{"line_number":16,"context_line":"hw_scsi_model\u003dvirtio-scsi"},{"line_number":17,"context_line":"hw_disk_bus\u003dscsi"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"fa80f949_ca72b27c","line":15,"updated":"2015-11-03 18:31:26.000000000","message":"We should probably update this to be closer to the wording in the spec since this is known to work but is not the full list of possible support options.","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"ce31c4ee2f87041898185df09c0d86ed47e2b973","unresolved":false,"context_lines":[{"line_number":12,"context_line":"This functionality requires properties in the glance image"},{"line_number":13,"context_line":"to enable creation of the correct SCSI controller on the instance."},{"line_number":14,"context_line":""},{"line_number":15,"context_line":"Required glance properties are"},{"line_number":16,"context_line":"hw_scsi_model\u003dvirtio-scsi"},{"line_number":17,"context_line":"hw_disk_bus\u003dscsi"},{"line_number":18,"context_line":""}],"source_content_type":"text/x-gerrit-commit-message","patch_set":9,"id":"5a710552_793d69ba","line":15,"in_reply_to":"fa80f949_ca72b27c","updated":"2015-12-16 02:44:21.000000000","message":"Done","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"}],"nova/tests/unit/virt/libvirt/test_driver.py":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"93dc369f4eb4a45892aad3b98e412c780e42ff11","unresolved":false,"context_lines":[{"line_number":4736,"context_line":"    @mock.patch(\u0027nova.utils.get_image_from_system_metadata\u0027)"},{"line_number":4737,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.blockinfo.get_info_from_bdm\u0027)"},{"line_number":4738,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.get_domain\u0027)"},{"line_number":4739,"context_line":"    def test_attach_volume_with_vir_domain_affect_live_flag_with_discard("},{"line_number":4740,"context_line":"            self, mock_get_domain, mock_get_info, get_image):"},{"line_number":4741,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"},{"line_number":4742,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_e0ae22e5","line":4739,"updated":"2015-10-16 14:05:49.000000000","message":"What specifically is this testing in the virt driver when there are no changes made to the virt driver?","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"c495c1e3e51c6241b1b26daadbab4f9100f45580","unresolved":false,"context_lines":[{"line_number":4736,"context_line":"    @mock.patch(\u0027nova.utils.get_image_from_system_metadata\u0027)"},{"line_number":4737,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.blockinfo.get_info_from_bdm\u0027)"},{"line_number":4738,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.get_domain\u0027)"},{"line_number":4739,"context_line":"    def test_attach_volume_with_vir_domain_affect_live_flag_with_discard("},{"line_number":4740,"context_line":"            self, mock_get_domain, mock_get_info, get_image):"},{"line_number":4741,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"},{"line_number":4742,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_ecc36636","line":4739,"in_reply_to":"5a2ca52d_e0ae22e5","updated":"2015-10-18 02:26:57.000000000","message":"Uhh looks like its just validating that the connect path isn\u0027t broken when discard\u003dTrue is passed as part of the connection_info... and I guess that that is passed to _connect_volume().\n\nI might just delete this test addition... not sure it really adds much.","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c888127f31b7a9a95f1cec687794f789d3972383","unresolved":false,"context_lines":[{"line_number":5057,"context_line":"    @mock.patch(\u0027nova.utils.get_image_from_system_metadata\u0027)"},{"line_number":5058,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.blockinfo.get_info_from_bdm\u0027)"},{"line_number":5059,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.get_domain\u0027)"},{"line_number":5060,"context_line":"    def test_attach_volume_with_vir_domain_affect_live_flag_with_discard("},{"line_number":5061,"context_line":"            self, mock_get_domain, mock_get_info, get_image):"},{"line_number":5062,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"},{"line_number":5063,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_4ac7a212","line":5060,"updated":"2015-11-03 18:31:26.000000000","message":"What is this supposed to test?  That the warning is logged?  I don\u0027t see an assertion for that.  Let\u0027s move the validation into a private method, unit test *that* and then just add an assert_called_once_with check on the private method in the existing test \"test_attach_volume_with_vir_domain_affect_live_flag\" that calls the private method.","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"ce31c4ee2f87041898185df09c0d86ed47e2b973","unresolved":false,"context_lines":[{"line_number":5057,"context_line":"    @mock.patch(\u0027nova.utils.get_image_from_system_metadata\u0027)"},{"line_number":5058,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.blockinfo.get_info_from_bdm\u0027)"},{"line_number":5059,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.get_domain\u0027)"},{"line_number":5060,"context_line":"    def test_attach_volume_with_vir_domain_affect_live_flag_with_discard("},{"line_number":5061,"context_line":"            self, mock_get_domain, mock_get_info, get_image):"},{"line_number":5062,"context_line":"        drvr \u003d libvirt_driver.LibvirtDriver(fake.FakeVirtAPI(), False)"},{"line_number":5063,"context_line":"        instance \u003d objects.Instance(**self.test_instance)"}],"source_content_type":"text/x-python","patch_set":9,"id":"5a710552_5942ed3f","line":5060,"in_reply_to":"fa80f949_4ac7a212","updated":"2015-12-16 02:44:21.000000000","message":"Done","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"}],"nova/tests/unit/virt/libvirt/volume/test_volume.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"7a728bbc0b3a19d972d640dd951ef84e7f453f21","unresolved":false,"context_lines":[{"line_number":309,"context_line":"        tree \u003d conf.format_dom()"},{"line_number":310,"context_line":"        self.assertIsNone(tree.find(\"driver[@discard]\"))"},{"line_number":311,"context_line":""},{"line_number":312,"context_line":"    def test_libvirt_volume_driver_discard_missing(self):"},{"line_number":313,"context_line":"        # Check the discard attrib is not present in driver section"},{"line_number":314,"context_line":"        libvirt_driver \u003d volume.LibvirtVolumeDriver(self.fake_conn)"},{"line_number":315,"context_line":"        connection_info \u003d {"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a50d1a3_7e1c666a","line":312,"updated":"2015-07-31 09:35:37.000000000","message":"This test case is pointless as all the existing test cases cover the scenario where discard is missing in connection_info.","commit_id":"33855974254205f5b956ca122f47e95543760353"},{"author":{"_account_id":13868,"name":"Danny Wilson","email":"daniel.wilson@purestorage.com","username":"dannywilson"},"change_message_id":"8917e205ce53c7c81df1c6bcf6060de1272542d3","unresolved":false,"context_lines":[{"line_number":309,"context_line":"        tree \u003d conf.format_dom()"},{"line_number":310,"context_line":"        self.assertIsNone(tree.find(\"driver[@discard]\"))"},{"line_number":311,"context_line":""},{"line_number":312,"context_line":"    def test_libvirt_volume_driver_discard_missing(self):"},{"line_number":313,"context_line":"        # Check the discard attrib is not present in driver section"},{"line_number":314,"context_line":"        libvirt_driver \u003d volume.LibvirtVolumeDriver(self.fake_conn)"},{"line_number":315,"context_line":"        connection_info \u003d {"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a50d1a3_2268e3b9","line":312,"in_reply_to":"3a50d1a3_7e1c666a","updated":"2015-07-31 20:06:11.000000000","message":"Good call, removed this test case and added check in test_libvirt_volume_driver_serial above.","commit_id":"33855974254205f5b956ca122f47e95543760353"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"93dc369f4eb4a45892aad3b98e412c780e42ff11","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        self.assertIsNotNone(driver_node)"},{"line_number":285,"context_line":"        self.assertEqual(\u0027unmap\u0027, driver_node.attrib[\u0027discard\u0027])"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.has_min_version\u0027)"},{"line_number":288,"context_line":"    def test_libvirt_volume_driver_discard_false(self, mock_has_min_version):"},{"line_number":289,"context_line":"        # Check the discard attrib is not present in driver section"},{"line_number":290,"context_line":"        mock_has_min_version.return_value \u003d True"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_00c8fea6","line":287,"updated":"2015-10-16 14:05:49.000000000","message":"You don\u0027t need to mock this out, it shouldn\u0027t be called since discard is False in the connection properties.","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"c495c1e3e51c6241b1b26daadbab4f9100f45580","unresolved":false,"context_lines":[{"line_number":284,"context_line":"        self.assertIsNotNone(driver_node)"},{"line_number":285,"context_line":"        self.assertEqual(\u0027unmap\u0027, driver_node.attrib[\u0027discard\u0027])"},{"line_number":286,"context_line":""},{"line_number":287,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.has_min_version\u0027)"},{"line_number":288,"context_line":"    def test_libvirt_volume_driver_discard_false(self, mock_has_min_version):"},{"line_number":289,"context_line":"        # Check the discard attrib is not present in driver section"},{"line_number":290,"context_line":"        mock_has_min_version.return_value \u003d True"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_0ced82b6","line":287,"in_reply_to":"5a2ca52d_00c8fea6","updated":"2015-10-18 02:26:57.000000000","message":"Done","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7dfdebb68e25c0eab8342cfa027bfab10465ec07","unresolved":false,"context_lines":[{"line_number":265,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.has_min_version\u0027)"},{"line_number":266,"context_line":"    def test_libvirt_volume_driver_discard_true(self, mock_has_min_version):"},{"line_number":267,"context_line":"        # Check the discard attrib is present in driver section"},{"line_number":268,"context_line":"        mock_has_min_version.return_value \u003d True"},{"line_number":269,"context_line":"        libvirt_driver \u003d volume.LibvirtVolumeDriver(self.fake_conn)"},{"line_number":270,"context_line":"        connection_info \u003d {"},{"line_number":271,"context_line":"            \u0027driver_volume_type\u0027: \u0027fake\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_46eeca23","line":268,"updated":"2015-12-22 21:59:08.000000000","message":"Should have a test where mock_has_min_version is False but discard is True in the connection_info, just so we know we don\u0027t set discard to unmap.","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"b93955a2f15ff1270f6768796044376c830f37fe","unresolved":false,"context_lines":[{"line_number":265,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.has_min_version\u0027)"},{"line_number":266,"context_line":"    def test_libvirt_volume_driver_discard_true(self, mock_has_min_version):"},{"line_number":267,"context_line":"        # Check the discard attrib is present in driver section"},{"line_number":268,"context_line":"        mock_has_min_version.return_value \u003d True"},{"line_number":269,"context_line":"        libvirt_driver \u003d volume.LibvirtVolumeDriver(self.fake_conn)"},{"line_number":270,"context_line":"        connection_info \u003d {"},{"line_number":271,"context_line":"            \u0027driver_volume_type\u0027: \u0027fake\u0027,"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_66c8a675","line":268,"in_reply_to":"1a7b0d38_46eeca23","updated":"2015-12-22 22:54:07.000000000","message":"Done","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2cf7839dd73fdb2d1bc446ba871ce8c173eaf525","unresolved":false,"context_lines":[{"line_number":301,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.has_min_version\u0027)"},{"line_number":302,"context_line":"    def test_libvirt_volume_driver_discard_true_bad_version("},{"line_number":303,"context_line":"            self, mock_has_min_version):"},{"line_number":304,"context_line":"        # Check the discard attrib is present in driver section"},{"line_number":305,"context_line":"        mock_has_min_version.return_value \u003d False"},{"line_number":306,"context_line":"        libvirt_driver \u003d volume.LibvirtVolumeDriver(self.fake_conn)"},{"line_number":307,"context_line":"        connection_info \u003d {"}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_670ac428","line":304,"range":{"start_line":304,"start_character":38,"end_line":304,"end_character":45},"updated":"2015-12-23 15:05:30.000000000","message":"is not present","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"46823273dabe654e3f904c66db4ffc1ac76944d8","unresolved":false,"context_lines":[{"line_number":301,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.host.Host.has_min_version\u0027)"},{"line_number":302,"context_line":"    def test_libvirt_volume_driver_discard_true_bad_version("},{"line_number":303,"context_line":"            self, mock_has_min_version):"},{"line_number":304,"context_line":"        # Check the discard attrib is present in driver section"},{"line_number":305,"context_line":"        mock_has_min_version.return_value \u003d False"},{"line_number":306,"context_line":"        libvirt_driver \u003d volume.LibvirtVolumeDriver(self.fake_conn)"},{"line_number":307,"context_line":"        connection_info \u003d {"}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_f0f2e899","line":304,"range":{"start_line":304,"start_character":38,"end_line":304,"end_character":45},"in_reply_to":"1a7b0d38_670ac428","updated":"2015-12-23 17:20:49.000000000","message":"Done","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"}],"nova/virt/libvirt/driver.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"5465b8bdd15d09157b80d4bd7a735e9e788c1d00","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_59d052f6","line":1083,"updated":"2015-07-28 09:50:08.000000000","message":"No, we must not silently change the type of bus used for a disk. The desired bus is specified in the BDM and we must *always* honour that. If some buses don\u0027t support discard then we should simply ignore the discard setting, as it is a mere optional optimization.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"60e7074486d559aa4006ac2986bd2f5611f95cf3","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_bc8a8bbd","line":1083,"in_reply_to":"3a50d1a3_11ccc8b9","updated":"2015-07-28 17:29:04.000000000","message":"But the fact that disk type like virtio or legacy IDE, they don\u0027t support \u0027discard\u0027, i.e. discard command is ignored.  The point of specifying \u0027discard\u0027 for an attachment is to make it work as expected when possible.\n\nIn our production environment, we did some hack, which we update our image properties to add \u0027hw_scsi_model\u0027 and let volume attachment piggyback that to support discard/trim.  It works for newly created VMs but doesn\u0027t work for existing VMs.  So I\u0027d love to see Danny\u0027s work gets into Nova.  Would you willing to consider the option I mentioned in previous comment - let Cinder passes \u0027bus\u0027 and Nova honors that?","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"ca0726bf210e5a058ee4882e94d60cac51e00b6c","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_289a8414","line":1083,"in_reply_to":"3a50d1a3_4556d92a","updated":"2015-07-29 15:41:17.000000000","message":"Consider a guest image which only has driver support for the virtio-blk device - which is in fact true in alot of images, since virtio-scsi is a fairly recently implemented device type.  Now if we automatically switch to virtio-scsi, you are going to break the ability of many guests to use the volume, since they lack virtio-scsi support. Conversely if we honour the bus that the guest is already configured to use, we know that the guest will be able to use the volume, but it may not have discard support.  The issue of varying guest hardware support is why we must always honour the disk bus that the tenant user and / or Nova has already configured the guest to use.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":13868,"name":"Danny Wilson","email":"daniel.wilson@purestorage.com","username":"dannywilson"},"change_message_id":"9a09f3c0da7940dd2d7c0f52dcdd4a09adac28ca","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_8b9c2b6b","line":1083,"in_reply_to":"3a50d1a3_59d052f6","updated":"2015-07-28 16:30:16.000000000","message":"Thank you Daniel.  I have been basing my work off previous work in this area so it seems like this should be possible somehow.\n\nIn this patch it looks like the type of bus is being changed.\nhttps://review.openstack.org/#/c/70263/6/nova/virt/libvirt/driver.py\nIs this okay in this case because it is at start-up time and not a hot-plug of a bus? Or is this not a \u0027silent\u0027 change because an operator had to specify the bus in an image-property?\n\nIn this patch it seems like you are saying that enabling discard for an attached volume would be possible.\nhttps://review.openstack.org/#/c/152823/3//COMMIT_MSG\nIf you can point me in a different direction that would be great.\n\nThank you so much in advance.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"919f589c5efea2c44ea0fdaaf2221a504251fd7c","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_bccfeb7d","line":1083,"in_reply_to":"3a50d1a3_59d052f6","updated":"2015-07-28 17:20:31.000000000","message":"The problem is, in current API and workflow, user doesn\u0027t have option to provide a bus for attachment thus BDM is created _before_ Cinder has a chance to indicate the desired bus.  \nWe may either modify volume attach API to allow user specify additional information for attachment or when Nova gets connection info from Cinder, which _will_ have *both* bus and discard option for the volume, Nova updates BDM and honor that bus (by doing what Danny proposed here).  Comments?","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"39983b0a6f4104be63d005201bc8307023151c7d","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_11ccc8b9","line":1083,"in_reply_to":"3a50d1a3_8b9c2b6b","updated":"2015-07-28 17:00:47.000000000","message":"The code which you reference in change 70263 is actually reading the user\u0027s requested disk bus and honouring that. So you\u0027re in fact overriding that code.\n\nLooking at the code, discard support is in fact a setting on the backend, so is independent of the type of guest device exposed to the guest. ie discard is not restricted to use with virtio-scsi, it can be set for any disk type.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":13425,"name":"Simon Dodsley","email":"simon@everpuredata.com","username":"sdodsley"},"change_message_id":"85f14bc34860620f93f8ab73e95abccddd6fc533","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_8e6f2b5f","line":1083,"in_reply_to":"3a50d1a3_a58c92d7","updated":"2015-07-29 13:25:31.000000000","message":"Whilst it may be true that discard is a best effort feature, but it is a critical requirement for the best practice operation of modern storage arrays. I think you are being a little short-sighted in not allowing Cinder to have some input into the best \u0027hardware\u0027 to use to try and enable its features.\nThe storage backend is as complex as the compute frontend, therefore Nova can never fully cater for the ever increasing functionality provided by Cinder.\nYou need to know what Cinder can do to implement the correct bus and therefore Cinder has to give you that information. If you won\u0027t allow bus choice, specifically virtio-scsi, at instance creation, or volume attachment, then you have to allow a third party to influence as an interim fix.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":2759,"name":"Huang Zhiteng","email":"winston.d@gmail.com","username":"zhiteng-huang"},"change_message_id":"d7ddc0c3ef9a012af82a633e1f5be747bf1fabd6","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_4556d92a","line":1083,"in_reply_to":"3a50d1a3_a58c92d7","updated":"2015-07-29 15:36:03.000000000","message":"Yes, Cinder isn\u0027t providing the storage backend, it just pass-through what storage backend\u0027s requirement to Nova.  I agree there is no guarantee that discard will work and it\u0027s best effort feature.  But IMHO, best effort means Nova shall help to make it happen when possible, in this case, installing a HBA aka changing the bus is something Nova can do to help.  \n\nWhere is the boundary of Nova\u0027s best effort?  Along the data path for guest IO to storage backend ( http://paste.openstack.org/show/406254/ ), Nova is already helping to setup necessary facilities to get IO under better control, for example: 2) virtual device IO throttling and 3) multipathing.  Danny\u0027s proposal is to clear the blocker for TRIM at 2), *when possible*.\n\nSo what I am expecting is when Cinder tells Nova how to connect the volume - \u0027New drive coming, protocol X, TRIM ready, please help and find a proper slot for it to attach\u0027, we can change existing behavior like this:\n\u0027Bus doesn\u0027t support TRIM, you can put drive to slot Y though without TRIM\u0027 or \u0027You are lucky, use slot Y please\u0027\n\nto:\n\n\u0027No bus on VM supports TRIM, but I can get a new bus for you. Done, please put the drive to slot Z\u0027 or \u0027Bus doesn\u0027t support TRIM and I can\u0027t get a new bus that does for you, you can put drive to slot Y without TRIM\u0027\n\nThe new behavior is still best effort.  And that\u0027s all we need.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"befd823e0997b7a4aeee72cc828b4d0c9ea6fac4","unresolved":false,"context_lines":[{"line_number":1080,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":1081,"context_line":"            scsi_controller \u003d vconfig.LibvirtConfigGuestController()"},{"line_number":1082,"context_line":"            scsi_controller.type \u003d \u0027scsi\u0027"},{"line_number":1083,"context_line":"            scsi_controller.model \u003d \u0027virtio-scsi\u0027"},{"line_number":1084,"context_line":""},{"line_number":1085,"context_line":"        try:"},{"line_number":1086,"context_line":"            state \u003d self._get_power_state(guest._domain)"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a50d1a3_a58c92d7","line":1083,"in_reply_to":"3a50d1a3_bccfeb7d","updated":"2015-07-29 08:37:26.000000000","message":"It is not cinder\u0027s job to specify the guest hardware - in fact it is none of cinder\u0027s business at all. Guest hardware configuration is 100% responsibility of Nova to decide. \n\nCinder is merely providing the storage backend. Discard is only ever a best effort feature - there is never any guarantee that it will work with the guest or virtual machine as the virtual machine emulator and guest are always free to completely ignore it. So the fact that it will not work with certain disk types is not a problem - it is expected.","commit_id":"c302e8ebdb860098d0da75d4a9ade760e1a7caf6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c888127f31b7a9a95f1cec687794f789d3972383","unresolved":false,"context_lines":[{"line_number":1096,"context_line":"        conf \u003d self._get_volume_config(connection_info, disk_info)"},{"line_number":1097,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1098,"context_line":""},{"line_number":1099,"context_line":"        if (conf.driver_discard \u003d\u003d \u0027unmap\u0027 and"},{"line_number":1100,"context_line":"                guest.get_controller(\u0027virtio-scsi\u0027) is None):"},{"line_number":1101,"context_line":"            LOG.warning(_LW(\u0027Attempting to attach a volume with discard \u0027"},{"line_number":1102,"context_line":"                            \u0027support enabled to an instance that does not \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_4ad32239","line":1099,"updated":"2015-11-03 18:31:26.000000000","message":"I\u0027d suggest doing this validation/checking in a private method, i.e. _check_discard_for_attach_volume(conf, guest).  Would be cleaner in a private method and easier to unit test directly w/o having to stub out the rest of this method.","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"ce31c4ee2f87041898185df09c0d86ed47e2b973","unresolved":false,"context_lines":[{"line_number":1096,"context_line":"        conf \u003d self._get_volume_config(connection_info, disk_info)"},{"line_number":1097,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1098,"context_line":""},{"line_number":1099,"context_line":"        if (conf.driver_discard \u003d\u003d \u0027unmap\u0027 and"},{"line_number":1100,"context_line":"                guest.get_controller(\u0027virtio-scsi\u0027) is None):"},{"line_number":1101,"context_line":"            LOG.warning(_LW(\u0027Attempting to attach a volume with discard \u0027"},{"line_number":1102,"context_line":"                            \u0027support enabled to an instance that does not \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"5a710552_33a75051","line":1099,"in_reply_to":"fa80f949_4ad32239","updated":"2015-12-16 02:44:21.000000000","message":"Done","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"6fed3f40c66537ca29eea0f1e24459f6b7d763ba","unresolved":false,"context_lines":[{"line_number":1097,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1098,"context_line":""},{"line_number":1099,"context_line":"        if (conf.driver_discard \u003d\u003d \u0027unmap\u0027 and"},{"line_number":1100,"context_line":"                guest.get_controller(\u0027virtio-scsi\u0027) is None):"},{"line_number":1101,"context_line":"            LOG.warning(_LW(\u0027Attempting to attach a volume with discard \u0027"},{"line_number":1102,"context_line":"                            \u0027support enabled to an instance that does not \u0027"},{"line_number":1103,"context_line":"                            \u0027have a virtio-scsi bus. Trim commands will not \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"3a29b11f_5b6e5eca","line":1100,"updated":"2015-10-19 18:38:05.000000000","message":"Will need to adjust this check since it isn\u0027t the only one that can support discard. Might be easier to just check if the disk is going to be using virtio-blk and log the warning.","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"ce31c4ee2f87041898185df09c0d86ed47e2b973","unresolved":false,"context_lines":[{"line_number":1097,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1098,"context_line":""},{"line_number":1099,"context_line":"        if (conf.driver_discard \u003d\u003d \u0027unmap\u0027 and"},{"line_number":1100,"context_line":"                guest.get_controller(\u0027virtio-scsi\u0027) is None):"},{"line_number":1101,"context_line":"            LOG.warning(_LW(\u0027Attempting to attach a volume with discard \u0027"},{"line_number":1102,"context_line":"                            \u0027support enabled to an instance that does not \u0027"},{"line_number":1103,"context_line":"                            \u0027have a virtio-scsi bus. Trim commands will not \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"5a710552_d3628c16","line":1100,"in_reply_to":"3a29b11f_5b6e5eca","updated":"2015-12-16 02:44:21.000000000","message":"Done","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"7d60010ffbc030505eba29cf792470b7fef075d7","unresolved":false,"context_lines":[{"line_number":1087,"context_line":"            ]"},{"line_number":1088,"context_line":""},{"line_number":1089,"context_line":"            for controller in known_bad_controllers:"},{"line_number":1090,"context_line":"                if guest.get_controller(controller) is not None:"},{"line_number":1091,"context_line":"                    LOG.warning(_LW(\u0027Attempting to attach a volume with \u0027"},{"line_number":1092,"context_line":"                                    \u0027discard support enabled to an instance \u0027"},{"line_number":1093,"context_line":"                                    \u0027using an unsupported controller \u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a710552_757237af","line":1090,"updated":"2015-12-16 09:27:03.000000000","message":"Here you probably want to call guest.has_controller(controller)","commit_id":"fc51091ea71b39c04b1e70749503704cd33d5a9c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"8890389d3a06e8f3bb0c7a4a620836114761dce6","unresolved":false,"context_lines":[{"line_number":1087,"context_line":"            ]"},{"line_number":1088,"context_line":""},{"line_number":1089,"context_line":"            for controller in known_bad_controllers:"},{"line_number":1090,"context_line":"                if guest.get_controller(controller) is not None:"},{"line_number":1091,"context_line":"                    LOG.warning(_LW(\u0027Attempting to attach a volume with \u0027"},{"line_number":1092,"context_line":"                                    \u0027discard support enabled to an instance \u0027"},{"line_number":1093,"context_line":"                                    \u0027using an unsupported controller \u0027"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a710552_bd494fb9","line":1090,"in_reply_to":"5a710552_757237af","updated":"2015-12-16 16:37:58.000000000","message":"Derp... last minute rename fail on my part.","commit_id":"fc51091ea71b39c04b1e70749503704cd33d5a9c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"7d60010ffbc030505eba29cf792470b7fef075d7","unresolved":false,"context_lines":[{"line_number":1134,"context_line":"        conf \u003d self._get_volume_config(connection_info, disk_info)"},{"line_number":1135,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1136,"context_line":""},{"line_number":1137,"context_line":"        self._check_discard_for_attach_volume(conf, guest)"},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"        try:"},{"line_number":1140,"context_line":"            state \u003d guest.get_power_state(self._host)"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a710552_75ceb7b5","line":1137,"updated":"2015-12-16 09:27:03.000000000","message":"nit: What about to call this method:\n\n  _warning_discard_for_attach_volume","commit_id":"fc51091ea71b39c04b1e70749503704cd33d5a9c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"8890389d3a06e8f3bb0c7a4a620836114761dce6","unresolved":false,"context_lines":[{"line_number":1134,"context_line":"        conf \u003d self._get_volume_config(connection_info, disk_info)"},{"line_number":1135,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1136,"context_line":""},{"line_number":1137,"context_line":"        self._check_discard_for_attach_volume(conf, guest)"},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"        try:"},{"line_number":1140,"context_line":"            state \u003d guest.get_power_state(self._host)"}],"source_content_type":"text/x-python","patch_set":11,"id":"5a710552_dd277bc8","line":1137,"in_reply_to":"5a710552_75ceb7b5","updated":"2015-12-16 16:37:58.000000000","message":"The current name was a suggestion from a previous review, if the consensus is that a new name is wanted I\u0027m fine to change it, but lets maybe wait for a few more folks to weigh in.","commit_id":"fc51091ea71b39c04b1e70749503704cd33d5a9c"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"f5b16ecb1fad6c509dc28172c771a4213378d9f2","unresolved":false,"context_lines":[{"line_number":1134,"context_line":"        conf \u003d self._get_volume_config(connection_info, disk_info)"},{"line_number":1135,"context_line":"        self._set_cache_mode(conf)"},{"line_number":1136,"context_line":""},{"line_number":1137,"context_line":"        self._check_discard_for_attach_volume(conf, guest)"},{"line_number":1138,"context_line":""},{"line_number":1139,"context_line":"        try:"},{"line_number":1140,"context_line":"            state \u003d guest.get_power_state(self._host)"}],"source_content_type":"text/x-python","patch_set":11,"id":"3a7e1126_b74ad1aa","line":1137,"in_reply_to":"5a710552_dd277bc8","updated":"2015-12-18 10:51:39.000000000","message":"Yes not a big deal at all.","commit_id":"fc51091ea71b39c04b1e70749503704cd33d5a9c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"787aae1bec9e31e3da8df9654cef81242a1744b1","unresolved":false,"context_lines":[{"line_number":1079,"context_line":"        \"\"\"Perform some checks for volumes configured for discard support."},{"line_number":1080,"context_line":""},{"line_number":1081,"context_line":"        If discard is configured for the volume, and the guest is using a"},{"line_number":1082,"context_line":"        controller known to not work, we will log a warning."},{"line_number":1083,"context_line":"        \"\"\""},{"line_number":1084,"context_line":"        if conf.driver_discard \u003d\u003d \u0027unmap\u0027:"},{"line_number":1085,"context_line":"            known_bad_controllers \u003d ["}],"source_content_type":"text/x-python","patch_set":12,"id":"5a710552_11416c38","line":1082,"updated":"2015-12-16 16:53:14.000000000","message":"Hmm further testing is proving that this method of detecting a \u0027bad\u0027 configuration isn\u0027t quite right... in the case where we are using virtio-blk it doesn\u0027t actually show it as a controller like it does for ones like virtio-scsi...","commit_id":"2c7525f62314f39142415b4fe65f3cf21a57a6d6"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7dfdebb68e25c0eab8342cfa027bfab10465ec07","unresolved":false,"context_lines":[{"line_number":1082,"context_line":"        configuration known to not work, we will log a warning."},{"line_number":1083,"context_line":"        \"\"\""},{"line_number":1084,"context_line":"        if conf.driver_discard \u003d\u003d \u0027unmap\u0027 and conf.target_bus \u003d\u003d \u0027virtio\u0027:"},{"line_number":1085,"context_line":"            LOG.warning(_LW(\u0027Attempting to attach a volume with discard \u0027"},{"line_number":1086,"context_line":"                            \u0027support enabled to an instance using an \u0027"},{"line_number":1087,"context_line":"                            \u0027unsupported configuration. target_bus \u003d \u0027"},{"line_number":1088,"context_line":"                            \u0027%(bus)s. Trim commands will not be issued to \u0027"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_a6af6ee2","line":1085,"updated":"2015-12-22 21:59:08.000000000","message":"We should log the volume id (should be in the conf.serial I think) and the instance (we can pass instance\u003dinstance kwarg to LOG.warning to get the highlighting in the logs. That way we\u0027d have context for this warning.","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"b93955a2f15ff1270f6768796044376c830f37fe","unresolved":false,"context_lines":[{"line_number":1082,"context_line":"        configuration known to not work, we will log a warning."},{"line_number":1083,"context_line":"        \"\"\""},{"line_number":1084,"context_line":"        if conf.driver_discard \u003d\u003d \u0027unmap\u0027 and conf.target_bus \u003d\u003d \u0027virtio\u0027:"},{"line_number":1085,"context_line":"            LOG.warning(_LW(\u0027Attempting to attach a volume with discard \u0027"},{"line_number":1086,"context_line":"                            \u0027support enabled to an instance using an \u0027"},{"line_number":1087,"context_line":"                            \u0027unsupported configuration. target_bus \u003d \u0027"},{"line_number":1088,"context_line":"                            \u0027%(bus)s. Trim commands will not be issued to \u0027"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_8673524d","line":1085,"in_reply_to":"1a7b0d38_a6af6ee2","updated":"2015-12-22 22:54:07.000000000","message":"Done","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"f2596ba8e6fa617f17a1c6a36b5f0d27defe46ee","unresolved":false,"context_lines":[{"line_number":1089,"context_line":"                            \u0027the storage device.\u0027),"},{"line_number":1090,"context_line":"                        {\u0027bus\u0027: conf.target_bus,"},{"line_number":1091,"context_line":"                         \u0027id\u0027: conf.serial},"},{"line_number":1092,"context_line":"                        instance\u003dinstance)"},{"line_number":1093,"context_line":""},{"line_number":1094,"context_line":"    def attach_volume(self, context, connection_info, instance, mountpoint,"},{"line_number":1095,"context_line":"                      disk_bus\u003dNone, device_type\u003dNone, encryption\u003dNone):"}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_3e9ff104","line":1092,"range":{"start_line":1092,"start_character":41,"end_line":1092,"end_character":42},"updated":"2015-12-23 09:58:13.000000000","message":"IMHO this is inappropriate to log as a warning.  The choice of disk bus to use is entirely at the control of the tenant user. The person who sees the logs are the cloud admin. A warning in the logs signifies something wrong that the cloud admin should address in their infrastructure, but this is merely a user\u0027s bad choice.\n\nIf there was a way to provide a warning to the tenant user, then great, but IMHO this should just be a debug message, since its basically irrelevant to the cloud admin, just something we want to see when debugging bugs really.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2cf7839dd73fdb2d1bc446ba871ce8c173eaf525","unresolved":false,"context_lines":[{"line_number":1089,"context_line":"                            \u0027the storage device.\u0027),"},{"line_number":1090,"context_line":"                        {\u0027bus\u0027: conf.target_bus,"},{"line_number":1091,"context_line":"                         \u0027id\u0027: conf.serial},"},{"line_number":1092,"context_line":"                        instance\u003dinstance)"},{"line_number":1093,"context_line":""},{"line_number":1094,"context_line":"    def attach_volume(self, context, connection_info, instance, mountpoint,"},{"line_number":1095,"context_line":"                      disk_bus\u003dNone, device_type\u003dNone, encryption\u003dNone):"}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_8a20159d","line":1092,"range":{"start_line":1092,"start_character":41,"end_line":1092,"end_character":42},"in_reply_to":"1a7b0d38_3e9ff104","updated":"2015-12-23 15:05:30.000000000","message":"@Patrick, sorry for the confusion here, I think I said this should be a warning in the spec which is how you implemented it here but I agree with Dan that debug level is probably best.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"46823273dabe654e3f904c66db4ffc1ac76944d8","unresolved":false,"context_lines":[{"line_number":1089,"context_line":"                            \u0027the storage device.\u0027),"},{"line_number":1090,"context_line":"                        {\u0027bus\u0027: conf.target_bus,"},{"line_number":1091,"context_line":"                         \u0027id\u0027: conf.serial},"},{"line_number":1092,"context_line":"                        instance\u003dinstance)"},{"line_number":1093,"context_line":""},{"line_number":1094,"context_line":"    def attach_volume(self, context, connection_info, instance, mountpoint,"},{"line_number":1095,"context_line":"                      disk_bus\u003dNone, device_type\u003dNone, encryption\u003dNone):"}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_3055d04f","line":1092,"range":{"start_line":1092,"start_character":41,"end_line":1092,"end_character":42},"in_reply_to":"1a7b0d38_8a20159d","updated":"2015-12-23 17:20:49.000000000","message":"Done","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"a70f668a9846f5d4492aba75c0eb8b502b0a1963","unresolved":false,"context_lines":[{"line_number":1079,"context_line":"        \"\"\"Perform some checks for volumes configured for discard support."},{"line_number":1080,"context_line":""},{"line_number":1081,"context_line":"        If discard is configured for the volume, and the guest is using a"},{"line_number":1082,"context_line":"        configuration known to not work, we will log a warning."},{"line_number":1083,"context_line":"        \"\"\""},{"line_number":1084,"context_line":"        if conf.driver_discard \u003d\u003d \u0027unmap\u0027 and conf.target_bus \u003d\u003d \u0027virtio\u0027:"},{"line_number":1085,"context_line":"            LOG.debug(\u0027Attempting to attach volume %(id)s with discard \u0027"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_70bc66dc","line":1082,"updated":"2015-12-27 16:28:28.000000000","message":"this is not a warning below.","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"f5936e3b00bd84cb056546eabaff59db68deb396","unresolved":false,"context_lines":[{"line_number":1079,"context_line":"        \"\"\"Perform some checks for volumes configured for discard support."},{"line_number":1080,"context_line":""},{"line_number":1081,"context_line":"        If discard is configured for the volume, and the guest is using a"},{"line_number":1082,"context_line":"        configuration known to not work, we will log a warning."},{"line_number":1083,"context_line":"        \"\"\""},{"line_number":1084,"context_line":"        if conf.driver_discard \u003d\u003d \u0027unmap\u0027 and conf.target_bus \u003d\u003d \u0027virtio\u0027:"},{"line_number":1085,"context_line":"            LOG.debug(\u0027Attempting to attach volume %(id)s with discard \u0027"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_0ea367b3","line":1082,"in_reply_to":"1a7b0d38_70bc66dc","updated":"2015-12-29 00:25:21.000000000","message":"Done","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"}],"nova/virt/libvirt/volume/volume.py":[{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"51339186b999d10e6c77be4d4235306580a7edf0","unresolved":false,"context_lines":[{"line_number":329,"context_line":"            data \u003d connection_info[\u0027data\u0027]"},{"line_number":330,"context_line":"            if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":331,"context_line":"                conf.target_bus \u003d \"scsi\""},{"line_number":332,"context_line":"                conf.driver_discard \u003d CONF.libvirt.hw_disk_discard"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"        return conf"},{"line_number":335,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3a50d1a3_ce8425b3","line":332,"updated":"2015-07-30 08:33:58.000000000","message":"This should really be done in the LibvirtBaseVolumeDriver class, to future proof us for potential discard support in any of the drivers. Also you should not be overriding conf.target_bus as that is already set by the LibvirtBaseVolumeDriver.\n\nAlso this will need tests in test_volume.py","commit_id":"31935a7cb27079c2ec46c4c6de8efa34512d42fb"},{"author":{"_account_id":13868,"name":"Danny Wilson","email":"daniel.wilson@purestorage.com","username":"dannywilson"},"change_message_id":"cbd0ad897340e8ea342b8a457b510d70a53accb4","unresolved":false,"context_lines":[{"line_number":329,"context_line":"            data \u003d connection_info[\u0027data\u0027]"},{"line_number":330,"context_line":"            if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":331,"context_line":"                conf.target_bus \u003d \"scsi\""},{"line_number":332,"context_line":"                conf.driver_discard \u003d CONF.libvirt.hw_disk_discard"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"        return conf"},{"line_number":335,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3a50d1a3_96d3755f","line":332,"in_reply_to":"3a50d1a3_443549a6","updated":"2015-07-30 18:05:07.000000000","message":"Moved to base driver and removed setting the target_bus.  I also added tests in test_volume.py.  Let me know if more tests are needed.  Thank you for the reviews.","commit_id":"31935a7cb27079c2ec46c4c6de8efa34512d42fb"},{"author":{"_account_id":7730,"name":"Sahid Orentino Ferdjaoui","email":"sahid.ferdjaoui@industrialdiscipline.com","username":"sahid"},"change_message_id":"6ef421fc3d3410c87c6e818711f0faf08e24ea38","unresolved":false,"context_lines":[{"line_number":329,"context_line":"            data \u003d connection_info[\u0027data\u0027]"},{"line_number":330,"context_line":"            if \u0027discard\u0027 in data and data[\u0027discard\u0027]:"},{"line_number":331,"context_line":"                conf.target_bus \u003d \"scsi\""},{"line_number":332,"context_line":"                conf.driver_discard \u003d CONF.libvirt.hw_disk_discard"},{"line_number":333,"context_line":""},{"line_number":334,"context_line":"        return conf"},{"line_number":335,"context_line":""}],"source_content_type":"text/x-python","patch_set":5,"id":"3a50d1a3_443549a6","line":332,"in_reply_to":"3a50d1a3_ce8425b3","updated":"2015-07-30 12:41:57.000000000","message":"Agreed with Daniel this should reside in LibvirtBaseVolumeDriver and we should not override what is set from attach_volume.","commit_id":"31935a7cb27079c2ec46c4c6de8efa34512d42fb"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"7a728bbc0b3a19d972d640dd951ef84e7f453f21","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        # Configure usage of discard"},{"line_number":169,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027] is True:"},{"line_number":170,"context_line":"            conf.driver_discard \u003d CONF.libvirt.hw_disk_discard"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"        return conf"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3a50d1a3_fb10885b","line":170,"updated":"2015-07-31 09:35:37.000000000","message":"This should just be unconditionally doing:\n\n   conf.driver_discard \u003d data.get(\u0027discard\u0027, False)\n\nThe CONF.libvirt.hw_disk_discard setting is used for controlling whether local non-cinder storage has discard turned on. If the admin needs to turn it off for local disk storage, this should not have any impact on discard usage for cinder.","commit_id":"33855974254205f5b956ca122f47e95543760353"},{"author":{"_account_id":13868,"name":"Danny Wilson","email":"daniel.wilson@purestorage.com","username":"dannywilson"},"change_message_id":"8917e205ce53c7c81df1c6bcf6060de1272542d3","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        # Configure usage of discard"},{"line_number":169,"context_line":"        if \u0027discard\u0027 in data and data[\u0027discard\u0027] is True:"},{"line_number":170,"context_line":"            conf.driver_discard \u003d CONF.libvirt.hw_disk_discard"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"        return conf"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":6,"id":"3a50d1a3_82f537ee","line":170,"in_reply_to":"3a50d1a3_fb10885b","updated":"2015-07-31 20:06:11.000000000","message":"Done.  I had to check if discard is True though and then set discard to \u0027unmap\u0027.  It didn\u0027t seem to make sense for Cinder to know that the setting is called \u0027unmap\u0027 in this case and makes it more flexible as well.  Thanks again.","commit_id":"33855974254205f5b956ca122f47e95543760353"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7f874273d3b757e8a858b726dad9a4b6e7640b38","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        # Configure usage of discard"},{"line_number":169,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":170,"context_line":"            conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"        return conf"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1a4dcd0f_a6b42335","line":170,"updated":"2015-08-10 16:49:50.000000000","message":"Do we need some kind of version check on doing this?  Will older QEMU have issues if this shows up in the domain xml or just ignore it since they don\u0027t support it?\n\nhttps://libvirt.org/formatdomain.html#elementsDisks\n\n\"The optional discard attribute controls whether to discard (also known as \"trim\" or \"unmap\") requests are ignored or passed to the filesystem. The value can be either \"unmap\" (allow the discard request to be passed) or \"ignore\" (ignore the discard request). Since 1.0.6 (QEMU and KVM only)\"","commit_id":"1800c7fc7b9491bb28aa2464bd315ad5f5844d4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"712ab57270b839be7aa7ad2255063a7fa08f8909","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        # Configure usage of discard"},{"line_number":169,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":170,"context_line":"            conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"        return conf"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1a4dcd0f_e6f30b9e","line":170,"in_reply_to":"1a4dcd0f_a6b42335","updated":"2015-08-10 16:58:18.000000000","message":"Or is this already covered with the nova config option as noted in the cinder spec:\n\nhttps://review.openstack.org/#/c/205714/2/specs/liberty/cinder-backend-report-discard.rst\n\nhw_disk_discard \u003d unmap\n\n^ this is the kind of information that should be in the commit message so someone can actually use this thing if they don\u0027t want to have to piece it all together via bp links and the cinder spec.","commit_id":"1800c7fc7b9491bb28aa2464bd315ad5f5844d4c"},{"author":{"_account_id":13868,"name":"Danny Wilson","email":"daniel.wilson@purestorage.com","username":"dannywilson"},"change_message_id":"e151b53f3dce3f5ed4618341ef7cd567817b58c6","unresolved":false,"context_lines":[{"line_number":167,"context_line":""},{"line_number":168,"context_line":"        # Configure usage of discard"},{"line_number":169,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":170,"context_line":"            conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":171,"context_line":""},{"line_number":172,"context_line":"        return conf"},{"line_number":173,"context_line":""}],"source_content_type":"text/x-python","patch_set":7,"id":"1a4dcd0f_55afb529","line":170,"in_reply_to":"1a4dcd0f_e6f30b9e","updated":"2015-08-11 23:30:44.000000000","message":"Added a version check.  This is no longer dependent on hw_disk_discard.  That was due to some of my misunderstanding at the start of this work.","commit_id":"1800c7fc7b9491bb28aa2464bd315ad5f5844d4c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"93dc369f4eb4a45892aad3b98e412c780e42ff11","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from nova.i18n import _LE"},{"line_number":25,"context_line":"from nova.i18n import _LW"},{"line_number":26,"context_line":"from nova.virt.libvirt import config as vconfig"},{"line_number":27,"context_line":"import nova.virt.libvirt.driver"},{"line_number":28,"context_line":"from nova.virt.libvirt import host"},{"line_number":29,"context_line":"from nova.virt.libvirt import utils as libvirt_utils"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_7d8e3d7a","line":27,"updated":"2015-10-16 14:05:49.000000000","message":"This would be better as:\n\nfrom nova.virt.libvirt import driver as libvirt_driver","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"c495c1e3e51c6241b1b26daadbab4f9100f45580","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from nova.i18n import _LE"},{"line_number":25,"context_line":"from nova.i18n import _LW"},{"line_number":26,"context_line":"from nova.virt.libvirt import config as vconfig"},{"line_number":27,"context_line":"import nova.virt.libvirt.driver"},{"line_number":28,"context_line":"from nova.virt.libvirt import host"},{"line_number":29,"context_line":"from nova.virt.libvirt import utils as libvirt_utils"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_d90bced4","line":27,"in_reply_to":"5a2ca52d_7d8e3d7a","updated":"2015-10-18 02:26:57.000000000","message":"Hmm so I tried doing that and ended up with this exception: \"ImportError: Class LibvirtDriver cannot be found\"\n\nFull paste: http://paste.openstack.org/show/476618/\n\nLooks like maybe a cyclic import? (Not sure yet... would need to go dig around the other imports in driver)\n\nIt does however work just fine without using the \u0027from X import Y\u0027 syntax","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"93dc369f4eb4a45892aad3b98e412c780e42ff11","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        # Configure usage of discard"},{"line_number":102,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":103,"context_line":"            min_qemu \u003d nova.virt.libvirt.driver.MIN_QEMU_DISCARD_VERSION"},{"line_number":104,"context_line":"            if not self.connection._host.has_min_version(None,"},{"line_number":105,"context_line":"                                                         min_qemu,"},{"line_number":106,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":107,"context_line":"                LOG.debug(\u0027Volume sets discard option, but qemu %(qemu)s\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_fd800dc1","line":104,"updated":"2015-10-16 14:05:49.000000000","message":"Why don\u0027t you have to specify MIN_LIBVIRT_DISCARD_VERSION here like in the virt driver?\n\nhttps://github.com/openstack/nova/blob/12.0.0/nova/virt/libvirt/driver.py#L3220","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"c495c1e3e51c6241b1b26daadbab4f9100f45580","unresolved":false,"context_lines":[{"line_number":101,"context_line":"        # Configure usage of discard"},{"line_number":102,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":103,"context_line":"            min_qemu \u003d nova.virt.libvirt.driver.MIN_QEMU_DISCARD_VERSION"},{"line_number":104,"context_line":"            if not self.connection._host.has_min_version(None,"},{"line_number":105,"context_line":"                                                         min_qemu,"},{"line_number":106,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":107,"context_line":"                LOG.debug(\u0027Volume sets discard option, but qemu %(qemu)s\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_f9c7aa61","line":104,"in_reply_to":"5a2ca52d_fd800dc1","updated":"2015-10-18 02:26:57.000000000","message":"Seems like we would need to.. I\u0027ll add it in.","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":11904,"name":"Sean McGinnis","email":"sean.mcginnis@gmail.com","username":"SeanM"},"change_message_id":"e653ad6fb352eb4ff2e8f3480ee3ab519d30fe77","unresolved":false,"context_lines":[{"line_number":105,"context_line":"                                                         min_qemu,"},{"line_number":106,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":107,"context_line":"                LOG.debug(\u0027Volume sets discard option, but qemu %(qemu)s\u0027"},{"line_number":108,"context_line":"                          \u0027or later is required.\u0027 %"},{"line_number":109,"context_line":"                          {\u0027qemu\u0027: min_qemu})"},{"line_number":110,"context_line":"            else:"},{"line_number":111,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_17b36aea","line":108,"updated":"2015-10-16 13:11:19.000000000","message":"Should use , instead of % to pass the formatting args in to the logger. That way string formatting won\u0027t occur unless loglevel included debug.","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"c495c1e3e51c6241b1b26daadbab4f9100f45580","unresolved":false,"context_lines":[{"line_number":105,"context_line":"                                                         min_qemu,"},{"line_number":106,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":107,"context_line":"                LOG.debug(\u0027Volume sets discard option, but qemu %(qemu)s\u0027"},{"line_number":108,"context_line":"                          \u0027or later is required.\u0027 %"},{"line_number":109,"context_line":"                          {\u0027qemu\u0027: min_qemu})"},{"line_number":110,"context_line":"            else:"},{"line_number":111,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"}],"source_content_type":"text/x-python","patch_set":8,"id":"5a2ca52d_b9a4d2b9","line":108,"in_reply_to":"5a2ca52d_17b36aea","updated":"2015-10-18 02:26:57.000000000","message":"Done","commit_id":"6f471c38b75f93f705cf04315aec053376bb7a69"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c888127f31b7a9a95f1cec687794f789d3972383","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from nova.i18n import _LE"},{"line_number":25,"context_line":"from nova.i18n import _LW"},{"line_number":26,"context_line":"from nova.virt.libvirt import config as vconfig"},{"line_number":27,"context_line":"import nova.virt.libvirt.driver"},{"line_number":28,"context_line":"from nova.virt.libvirt import host"},{"line_number":29,"context_line":"from nova.virt.libvirt import utils as libvirt_utils"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_2a87be10","line":27,"updated":"2015-11-03 18:31:26.000000000","message":"Would prefer:\n\nfrom nova.virt.libvirt import driver as libvirt_driver","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"ce31c4ee2f87041898185df09c0d86ed47e2b973","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from nova.i18n import _LE"},{"line_number":25,"context_line":"from nova.i18n import _LW"},{"line_number":26,"context_line":"from nova.virt.libvirt import config as vconfig"},{"line_number":27,"context_line":"import nova.virt.libvirt.driver"},{"line_number":28,"context_line":"from nova.virt.libvirt import host"},{"line_number":29,"context_line":"from nova.virt.libvirt import utils as libvirt_utils"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":9,"id":"5a710552_5613ea5c","line":27,"in_reply_to":"fa80f949_2a87be10","updated":"2015-12-16 02:44:21.000000000","message":"Looks like it is maybe causing a circular import and using the full path avoids the error. I\u0027m open to other suggestions, but \"from nova.virt.libvirt import driver as libvirt_driver\" won\u0027t work :(","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"c888127f31b7a9a95f1cec687794f789d3972383","unresolved":false,"context_lines":[{"line_number":102,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":103,"context_line":"            min_qemu \u003d nova.virt.libvirt.driver.MIN_QEMU_DISCARD_VERSION"},{"line_number":104,"context_line":"            min_libvirt \u003d nova.virt.libvirt.driver.MIN_LIBVIRT_DISCARD_VERSION"},{"line_number":105,"context_line":"            if not self.connection._host.has_min_version(min_libvirt,"},{"line_number":106,"context_line":"                                                         min_qemu,"},{"line_number":107,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":108,"context_line":"                LOG.debug(\u0027Unable to attach volume with discard enabled: \u0027"}],"source_content_type":"text/x-python","patch_set":9,"id":"fa80f949_0a999a6e","line":105,"updated":"2015-11-03 18:31:26.000000000","message":"I\u0027m not crazy about touching a private member variable in the connection object, but I see we\u0027re already doing that elsewhere in here so it\u0027s OK - just a note to myself for future cleanup.","commit_id":"26d4a38c446fe64d83ad09e878742f0bbc65894c"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7dfdebb68e25c0eab8342cfa027bfab10465ec07","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from nova.i18n import _LE"},{"line_number":25,"context_line":"from nova.i18n import _LW"},{"line_number":26,"context_line":"from nova.virt.libvirt import config as vconfig"},{"line_number":27,"context_line":"import nova.virt.libvirt.driver"},{"line_number":28,"context_line":"from nova.virt.libvirt import host"},{"line_number":29,"context_line":"from nova.virt.libvirt import utils as libvirt_utils"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_83e2649e","line":27,"updated":"2015-12-22 21:59:08.000000000","message":"nit: I\u0027d prefer:\n\nfrom nova.virt.libvirt import driver as libvirt_driver","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"b93955a2f15ff1270f6768796044376c830f37fe","unresolved":false,"context_lines":[{"line_number":24,"context_line":"from nova.i18n import _LE"},{"line_number":25,"context_line":"from nova.i18n import _LW"},{"line_number":26,"context_line":"from nova.virt.libvirt import config as vconfig"},{"line_number":27,"context_line":"import nova.virt.libvirt.driver"},{"line_number":28,"context_line":"from nova.virt.libvirt import host"},{"line_number":29,"context_line":"from nova.virt.libvirt import utils as libvirt_utils"},{"line_number":30,"context_line":""}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_c6a07a93","line":27,"in_reply_to":"1a7b0d38_83e2649e","updated":"2015-12-22 22:54:07.000000000","message":":( Doesn\u0027t work, see my comments on previous patchsets","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"7dfdebb68e25c0eab8342cfa027bfab10465ec07","unresolved":false,"context_lines":[{"line_number":105,"context_line":"            if not self.connection._host.has_min_version(min_libvirt,"},{"line_number":106,"context_line":"                                                         min_qemu,"},{"line_number":107,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":108,"context_line":"                LOG.warning(_LW(\u0027Unable to attach volume with discard \u0027"},{"line_number":109,"context_line":"                                \u0027enabled: qemu %(qemu)s and libvirt \u0027"},{"line_number":110,"context_line":"                                \u0027%(libvirt)s or later are required.\u0027),"},{"line_number":111,"context_line":"                            {\u0027qemu\u0027: min_qemu, \u0027libvirt\u0027: min_libvirt})"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_83a7c44e","line":108,"updated":"2015-12-22 21:59:08.000000000","message":"I don\u0027t think we really need a warning for this, INFO or DEBUG are probably fine. This is an optional thing and it\u0027s not a bug if you don\u0027t have all of the pieces to make it work, like the right libvirt/qemu versions, you just don\u0027t get the performance boost without it. So I think DEBUG is probably better here.\n\nYou might want to also log the volume provider and possibly the serial (which is usually the volume ID I think).\n\nI think the warning level here must have been confused from the spec, but in the spec I think we were talking about when virtio-blk is being used and we\u0027re asked for discard support, that it\u0027s not supported with that model.","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"b93955a2f15ff1270f6768796044376c830f37fe","unresolved":false,"context_lines":[{"line_number":105,"context_line":"            if not self.connection._host.has_min_version(min_libvirt,"},{"line_number":106,"context_line":"                                                         min_qemu,"},{"line_number":107,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":108,"context_line":"                LOG.warning(_LW(\u0027Unable to attach volume with discard \u0027"},{"line_number":109,"context_line":"                                \u0027enabled: qemu %(qemu)s and libvirt \u0027"},{"line_number":110,"context_line":"                                \u0027%(libvirt)s or later are required.\u0027),"},{"line_number":111,"context_line":"                            {\u0027qemu\u0027: min_qemu, \u0027libvirt\u0027: min_libvirt})"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_0611a2ac","line":108,"in_reply_to":"1a7b0d38_265e1eea","updated":"2015-12-22 22:54:07.000000000","message":"Done","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"1677ac69cec47676fdf41c69dd86ef288879e803","unresolved":false,"context_lines":[{"line_number":105,"context_line":"            if not self.connection._host.has_min_version(min_libvirt,"},{"line_number":106,"context_line":"                                                         min_qemu,"},{"line_number":107,"context_line":"                                                         host.HV_DRIVER_QEMU):"},{"line_number":108,"context_line":"                LOG.warning(_LW(\u0027Unable to attach volume with discard \u0027"},{"line_number":109,"context_line":"                                \u0027enabled: qemu %(qemu)s and libvirt \u0027"},{"line_number":110,"context_line":"                                \u0027%(libvirt)s or later are required.\u0027),"},{"line_number":111,"context_line":"                            {\u0027qemu\u0027: min_qemu, \u0027libvirt\u0027: min_libvirt})"}],"source_content_type":"text/x-python","patch_set":16,"id":"1a7b0d38_265e1eea","line":108,"in_reply_to":"1a7b0d38_83a7c44e","updated":"2015-12-22 22:01:50.000000000","message":"FYI, connection_info[\u0027driver_volume_type\u0027] has the cinder backend type/provider.","commit_id":"fa1414050d677dbba66417ce6d254cd0a5a7bd30"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"f2596ba8e6fa617f17a1c6a36b5f0d27defe46ee","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                          {\u0027qemu\u0027: min_qemu,"},{"line_number":112,"context_line":"                           \u0027libvirt\u0027: min_libvirt,"},{"line_number":113,"context_line":"                           \u0027serial\u0027: conf.serial,"},{"line_number":114,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":115,"context_line":"            else:"},{"line_number":116,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_9e362502","line":114,"range":{"start_line":114,"start_character":70,"end_line":114,"end_character":74},"updated":"2015-12-23 09:58:13.000000000","message":"This is something that could usefully be at level \u0027warning\u0027, because the cloud admin has enabled discard in cinder volumes, but has an outdated libvirt/qemu that cannot use it. This is something the cloud admin should see and take action for.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"46823273dabe654e3f904c66db4ffc1ac76944d8","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                          {\u0027qemu\u0027: min_qemu,"},{"line_number":112,"context_line":"                           \u0027libvirt\u0027: min_libvirt,"},{"line_number":113,"context_line":"                           \u0027serial\u0027: conf.serial,"},{"line_number":114,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":115,"context_line":"            else:"},{"line_number":116,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_b0bc606d","line":114,"range":{"start_line":114,"start_character":70,"end_line":114,"end_character":74},"in_reply_to":"1a7b0d38_6a004935","updated":"2015-12-23 17:20:49.000000000","message":"One data point, hopefully by the time M ships it will be a cinder config option for enabling this flag to be present https://review.openstack.org/#/c/258747/\n\nI\u0027ll change it to the one time warning since it sounds like everyone is on board with that...","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"53559adbdb62e5c14dc324bd201d221d96df2477","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                          {\u0027qemu\u0027: min_qemu,"},{"line_number":112,"context_line":"                           \u0027libvirt\u0027: min_libvirt,"},{"line_number":113,"context_line":"                           \u0027serial\u0027: conf.serial,"},{"line_number":114,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":115,"context_line":"            else:"},{"line_number":116,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_6a004935","line":114,"range":{"start_line":114,"start_character":70,"end_line":114,"end_character":74},"in_reply_to":"1a7b0d38_8a26d566","updated":"2015-12-23 15:23:44.000000000","message":"As discussed in IRC, if we do make this a warning, it should just log the warning once on first usage.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2cf7839dd73fdb2d1bc446ba871ce8c173eaf525","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                          {\u0027qemu\u0027: min_qemu,"},{"line_number":112,"context_line":"                           \u0027libvirt\u0027: min_libvirt,"},{"line_number":113,"context_line":"                           \u0027serial\u0027: conf.serial,"},{"line_number":114,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":115,"context_line":"            else:"},{"line_number":116,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_ca9b7dc8","line":114,"range":{"start_line":114,"start_character":70,"end_line":114,"end_character":74},"in_reply_to":"1a7b0d38_9e362502","updated":"2015-12-23 15:05:30.000000000","message":"The admin hasn\u0027t specifically enabled discard have they? I thought this was just a flag that the specific cinder backend they were using was enabling, so now when they upgrade cinder and they have backlevel libvirt/qemu they start getting warnings. If they are actually opting in to enabling discard support in cinder, then a warning is fine, sure, else I\u0027d think this is INFO at best.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"396e2f367568b2c2f1a4c03d9640297a355ef6c9","unresolved":false,"context_lines":[{"line_number":111,"context_line":"                          {\u0027qemu\u0027: min_qemu,"},{"line_number":112,"context_line":"                           \u0027libvirt\u0027: min_libvirt,"},{"line_number":113,"context_line":"                           \u0027serial\u0027: conf.serial,"},{"line_number":114,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":115,"context_line":"            else:"},{"line_number":116,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":117,"context_line":""}],"source_content_type":"text/x-python","patch_set":18,"id":"1a7b0d38_8a26d566","line":114,"range":{"start_line":114,"start_character":70,"end_line":114,"end_character":74},"in_reply_to":"1a7b0d38_ca9b7dc8","updated":"2015-12-23 15:09:39.000000000","message":"Note that there is no configuration in cinder for this, it\u0027s just the pure storage volume driver that sets discard\u003dTrue:\n\nhttps://review.openstack.org/#/c/205717/2/cinder/volume/drivers/pure.py\n\nAs far as I can tell, the only configuration really is using the right volume driver, having the right libvirt/qemu versions, and having the right image properties set.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"a70f668a9846f5d4492aba75c0eb8b502b0a1963","unresolved":false,"context_lines":[{"line_number":40,"context_line":"CONF \u003d cfg.CONF"},{"line_number":41,"context_line":"CONF.register_opts(volume_opts, \u0027libvirt\u0027)"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"should_log_discard_warning \u003d True"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"class LibvirtBaseVolumeDriver(object):"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_30b25e07","line":43,"updated":"2015-12-27 16:28:28.000000000","message":"Please use upper case for global variables","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"f5936e3b00bd84cb056546eabaff59db68deb396","unresolved":false,"context_lines":[{"line_number":40,"context_line":"CONF \u003d cfg.CONF"},{"line_number":41,"context_line":"CONF.register_opts(volume_opts, \u0027libvirt\u0027)"},{"line_number":42,"context_line":""},{"line_number":43,"context_line":"should_log_discard_warning \u003d True"},{"line_number":44,"context_line":""},{"line_number":45,"context_line":""},{"line_number":46,"context_line":"class LibvirtBaseVolumeDriver(object):"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_6ed6abf2","line":43,"in_reply_to":"1a7b0d38_30b25e07","updated":"2015-12-29 00:25:21.000000000","message":"Done","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":1653,"name":"garyk","email":"gkotton@vmware.com","username":"garyk"},"change_message_id":"a70f668a9846f5d4492aba75c0eb8b502b0a1963","unresolved":false,"context_lines":[{"line_number":108,"context_line":"            if (should_log_discard_warning and"},{"line_number":109,"context_line":"                    not self.connection._host.has_min_version("},{"line_number":110,"context_line":"                            min_libvirt, min_qemu, host.HV_DRIVER_QEMU)):"},{"line_number":111,"context_line":"                LOG.debug(\u0027Unable to attach %(type)s volume %(serial)s \u0027"},{"line_number":112,"context_line":"                          \u0027 with discard enabled: qemu %(qemu)s and \u0027"},{"line_number":113,"context_line":"                          \u0027libvirt %(libvirt)s or later are required.\u0027,"},{"line_number":114,"context_line":"                          {\u0027qemu\u0027: min_qemu,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_90c1d256","line":111,"updated":"2015-12-27 16:28:28.000000000","message":"why is this not a warning?","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"f5936e3b00bd84cb056546eabaff59db68deb396","unresolved":false,"context_lines":[{"line_number":108,"context_line":"            if (should_log_discard_warning and"},{"line_number":109,"context_line":"                    not self.connection._host.has_min_version("},{"line_number":110,"context_line":"                            min_libvirt, min_qemu, host.HV_DRIVER_QEMU)):"},{"line_number":111,"context_line":"                LOG.debug(\u0027Unable to attach %(type)s volume %(serial)s \u0027"},{"line_number":112,"context_line":"                          \u0027 with discard enabled: qemu %(qemu)s and \u0027"},{"line_number":113,"context_line":"                          \u0027libvirt %(libvirt)s or later are required.\u0027,"},{"line_number":114,"context_line":"                          {\u0027qemu\u0027: min_qemu,"}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_aec9f35a","line":111,"in_reply_to":"1a7b0d38_90c1d256","updated":"2015-12-29 00:25:21.000000000","message":"Done","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"1f1f3d5b86fda995c15b4714717dabb4c61124e1","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":118,"context_line":"                should_log_discard_warning \u003d False"},{"line_number":119,"context_line":"            else:"},{"line_number":120,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"        return conf"},{"line_number":123,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_911cafc2","line":120,"range":{"start_line":120,"start_character":44,"end_line":120,"end_character":45},"updated":"2015-12-24 10:31:49.000000000","message":"Err your logic is wrong here. One the second time we run this, should_log_discard_warning will be false, so we\u0027ll never do the min version check and just blindly turn on discard.","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"19d796f86a0073acf91a2b0c921e4d363753e0e9","unresolved":false,"context_lines":[{"line_number":117,"context_line":"                           \u0027type\u0027: connection_info[\u0027driver_volume_type\u0027]})"},{"line_number":118,"context_line":"                should_log_discard_warning \u003d False"},{"line_number":119,"context_line":"            else:"},{"line_number":120,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"},{"line_number":121,"context_line":""},{"line_number":122,"context_line":"        return conf"},{"line_number":123,"context_line":""}],"source_content_type":"text/x-python","patch_set":19,"id":"1a7b0d38_6d135567","line":120,"range":{"start_line":120,"start_character":44,"end_line":120,"end_character":45},"in_reply_to":"1a7b0d38_911cafc2","updated":"2015-12-25 05:09:02.000000000","message":"Oh derp... thats what I get for writing and testing this while rushing out the door for a holiday. Will fix in the next update.","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"94c5ea13b7395ddd573ec1df200066980a44e333","unresolved":false,"context_lines":[{"line_number":104,"context_line":"        if data.get(\u0027discard\u0027, False) is True:"},{"line_number":105,"context_line":"            min_qemu \u003d nova.virt.libvirt.driver.MIN_QEMU_DISCARD_VERSION"},{"line_number":106,"context_line":"            min_libvirt \u003d nova.virt.libvirt.driver.MIN_LIBVIRT_DISCARD_VERSION"},{"line_number":107,"context_line":"            if self.connection._host.has_min_version(min_libvirt,"},{"line_number":108,"context_line":"                                                     min_qemu,"},{"line_number":109,"context_line":"                                                     host.HV_DRIVER_QEMU):"},{"line_number":110,"context_line":"                conf.driver_discard \u003d \u0027unmap\u0027"}],"source_content_type":"text/x-python","patch_set":20,"id":"fa69d971_d8cd0b86","line":107,"updated":"2016-01-05 21:27:46.000000000","message":"It\u0027s unfortunate that we have to do this version check every time even though we know the result after the first check, but the only flag we have is the one for the warning, and working that into the logic gets messy as noted in PS19.  I guess if we wanted to avoid it we could set another global version for the version check result, but it\u0027s probably unnecessary, I assume this check is inexpensive.","commit_id":"1dc326007436c83a8d1fb9c499aa56c6d8926753"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"94c5ea13b7395ddd573ec1df200066980a44e333","unresolved":false,"context_lines":[{"line_number":116,"context_line":"                                    \u0027%(serial)s with discard enabled: qemu \u0027"},{"line_number":117,"context_line":"                                    \u0027%(qemu)s and libvirt %(libvirt)s or \u0027"},{"line_number":118,"context_line":"                                    \u0027later are required.\u0027),"},{"line_number":119,"context_line":"                                {"},{"line_number":120,"context_line":"                        \u0027qemu\u0027: min_qemu,"},{"line_number":121,"context_line":"                        \u0027libvirt\u0027: min_libvirt,"},{"line_number":122,"context_line":"                        \u0027serial\u0027: conf.serial,"}],"source_content_type":"text/x-python","patch_set":20,"id":"fa69d971_98ec73f4","line":119,"updated":"2016-01-05 21:27:46.000000000","message":"nit: alignment of the sub vars is a bit wonky here.","commit_id":"1dc326007436c83a8d1fb9c499aa56c6d8926753"}],"releasenotes/notes/cinder-backend-report-discard-1def1c28140def9b.yaml":[{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2cf7839dd73fdb2d1bc446ba871ce8c173eaf525","unresolved":false,"context_lines":[{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Add support for enabling discard support for block devices with libvirt."},{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt and qemu used along with the"},{"line_number":7,"context_line":"    configured virtual drivers for the instance."},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":18,"id":"1a7b0d38_8aa57505","line":5,"updated":"2015-12-23 15:05:30.000000000","message":"Might want to note which cinder backends support it as of mitaka-1.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"46823273dabe654e3f904c66db4ffc1ac76944d8","unresolved":false,"context_lines":[{"line_number":2,"context_line":"features:"},{"line_number":3,"context_line":"  - Add support for enabling discard support for block devices with libvirt."},{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt and qemu used along with the"},{"line_number":7,"context_line":"    configured virtual drivers for the instance."},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":18,"id":"1a7b0d38_db8d8d32","line":5,"in_reply_to":"1a7b0d38_8aa57505","updated":"2015-12-23 17:20:49.000000000","message":"Ideally this will be any cinder backend by the time M is being released. If I understand correctly these release notes are going to be for the whole thing and not milestone based (right?) so I\u0027d rather not specify here since it will be out of date very quickly. Sort of taking the \"less is better than wrong\" approach.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":1779,"name":"Daniel Berrange","email":"berrange@redhat.com","username":"berrange"},"change_message_id":"f2596ba8e6fa617f17a1c6a36b5f0d27defe46ee","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt and qemu used along with the"},{"line_number":7,"context_line":"    configured virtual drivers for the instance."},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":18,"id":"1a7b0d38_89ffd100","line":7,"range":{"start_line":7,"start_character":47,"end_line":7,"end_character":48},"updated":"2015-12-23 09:58:13.000000000","message":"We should really tell the user what versions of libvirt \u0026 qemu they need here.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"46823273dabe654e3f904c66db4ffc1ac76944d8","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt and qemu used along with the"},{"line_number":7,"context_line":"    configured virtual drivers for the instance."},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":18,"id":"1a7b0d38_3bf93181","line":7,"range":{"start_line":7,"start_character":47,"end_line":7,"end_character":48},"in_reply_to":"1a7b0d38_6a94c98a","updated":"2015-12-23 17:20:49.000000000","message":"Done","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"2cf7839dd73fdb2d1bc446ba871ce8c173eaf525","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt and qemu used along with the"},{"line_number":7,"context_line":"    configured virtual drivers for the instance."},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":18,"id":"1a7b0d38_6a94c98a","line":7,"range":{"start_line":7,"start_character":47,"end_line":7,"end_character":48},"in_reply_to":"1a7b0d38_89ffd100","updated":"2015-12-23 15:05:30.000000000","message":"Agreed. Should also point out that virtio-blk as the bus won\u0027t work.","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"46823273dabe654e3f904c66db4ffc1ac76944d8","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt and qemu used along with the"},{"line_number":7,"context_line":"    configured virtual drivers for the instance."},{"line_number":8,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":18,"id":"1a7b0d38_7bfed96c","line":7,"range":{"start_line":7,"start_character":47,"end_line":7,"end_character":48},"in_reply_to":"1a7b0d38_89ffd100","updated":"2015-12-23 17:20:49.000000000","message":"Done","commit_id":"2de4ef78db7350d3dab51e1100b940cf82712563"},{"author":{"_account_id":6873,"name":"Matt Riedemann","email":"mriedem.os@gmail.com","username":"mriedem"},"change_message_id":"9d5b80b270405fa4a781f0efb8e3b82381ead913","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt (v1.0.6+) and qemu (v1.6.0+) used"},{"line_number":7,"context_line":"    along with the configured virtual drivers for the instance. The default"},{"line_number":8,"context_line":"    virtio-blk driver does not support this functionality."},{"line_number":9,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":19,"id":"1a7b0d38_a9d711be","line":7,"range":{"start_line":7,"start_character":68,"end_line":7,"end_character":75},"updated":"2015-12-24 00:09:48.000000000","message":"I\u0027m not sure what you mean by \u0027default\u0027 here, but OK.","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"},{"author":{"_account_id":12924,"name":"Patrick East","email":"east.patrick@gmail.com","username":"patrick.east"},"change_message_id":"f5936e3b00bd84cb056546eabaff59db68deb396","unresolved":false,"context_lines":[{"line_number":4,"context_line":"    This will be enabled for Cinder volume attachments that specify support"},{"line_number":5,"context_line":"    for the feature in their connection properties. This requires support to"},{"line_number":6,"context_line":"    be present in the version of libvirt (v1.0.6+) and qemu (v1.6.0+) used"},{"line_number":7,"context_line":"    along with the configured virtual drivers for the instance. The default"},{"line_number":8,"context_line":"    virtio-blk driver does not support this functionality."},{"line_number":9,"context_line":""}],"source_content_type":"text/x-yaml","patch_set":19,"id":"1a7b0d38_8ed3d703","line":7,"range":{"start_line":7,"start_character":68,"end_line":7,"end_character":75},"in_reply_to":"1a7b0d38_a9d711be","updated":"2015-12-29 00:25:21.000000000","message":"Done","commit_id":"3c970af6cb56d08dbd4b2def3409c23a11565a57"}]}