)]}'
{"/COMMIT_MSG":[{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5ac096d67f82f0738d5a9d2eb1523f616f08c52e","unresolved":false,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This patch makes sure that if the cache dir hasn\u0027t been created yet then"},{"line_number":14,"context_line":"0 disk is reserved for the cache usage instead of raising and logging an"},{"line_number":15,"context_line":"exception."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: Id1bbc955a9099de1abc11b9063fe177896646d03"},{"line_number":18,"context_line":"Related-Bug: #1878024"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_236ec600","line":15,"updated":"2020-06-19 17:46:17.000000000","message":"Unless I\u0027m missing something, this patch appears to do the opposite of what is described here.\n\nIn this PS, if the cache dir does not exist, self.cache_dir_is_on_same_dev_as_instances_dir will return True, but the logic for returning 0 disk usage is \"if not self.cache_dir_is_on_same_dev_as_instances_dir:\"\n\nhttps://github.com/openstack/nova/blob/f5f7c2540150c7ee7640c834d5caec31b3f5a7ab/nova/virt/libvirt/imagecache.py#L359","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d05a7169af11f671a798a8e8bae476c808e705ab","unresolved":false,"context_lines":[{"line_number":12,"context_line":""},{"line_number":13,"context_line":"This patch makes sure that if the cache dir hasn\u0027t been created yet then"},{"line_number":14,"context_line":"0 disk is reserved for the cache usage instead of raising and logging an"},{"line_number":15,"context_line":"exception."},{"line_number":16,"context_line":""},{"line_number":17,"context_line":"Change-Id: Id1bbc955a9099de1abc11b9063fe177896646d03"},{"line_number":18,"context_line":"Related-Bug: #1878024"}],"source_content_type":"text/x-gerrit-commit-message","patch_set":4,"id":"bf51134e_3546f922","line":15,"in_reply_to":"bf51134e_236ec600","updated":"2020-06-19 17:58:23.000000000","message":"No, I think the logic is correct and is doing what we want. However, what will happen is we\u0027ll hit the same does-not-exist exception later than the original bug, where we count the blocks. The original patch didn\u0027t do that because it combined the bailout with \"does not exist yet\" and \"we don\u0027t want to count these things.\"\n\nI still think we want the property to be safe and not explode, for future users of it. We just want the counting code to also handle the does-not-exist case so that both are behaving as we expect.","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"}],"nova/tests/unit/virt/libvirt/test_imagecache.py":[{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4c3e1d7e7e4c0e2977bf5a27d38b649d9f2d3bbe","unresolved":false,"context_lines":[{"line_number":691,"context_line":"        mock_stat.side_effect \u003d [mock.Mock(st_dev\u003d0), mock.Mock(st_dev\u003d0)]"},{"line_number":692,"context_line":"        self.assertTrue(manager.cache_dir_is_on_same_dev_as_instances_dir)"},{"line_number":693,"context_line":""},{"line_number":694,"context_line":"    @mock.patch(\u0027os.path.exists\u0027, return_value\u003dTrue)"},{"line_number":695,"context_line":"    @mock.patch(\u0027nova.virt.libvirt.imagecache.ImageCacheManager.\u0027"},{"line_number":696,"context_line":"                \u0027cache_dir_is_on_same_dev_as_instances_dir\u0027,"},{"line_number":697,"context_line":"                new\u003dmock.PropertyMock(return_value\u003dFalse))"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_886d3834","line":694,"range":{"start_line":694,"start_character":34,"end_line":694,"end_character":51},"updated":"2020-06-19 11:58:18.000000000","message":"Using\n\n  new\u003dmock.Mock(return_value\u003dTrue)\n\nwould avoid the need for the unused mock_exists parameter","commit_id":"49652922a70786fb593189a61dbf41c378ae9e64"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"4c3e1d7e7e4c0e2977bf5a27d38b649d9f2d3bbe","unresolved":false,"context_lines":[{"line_number":722,"context_line":""},{"line_number":723,"context_line":"        self.assertEqual(0, manager.get_disk_usage())"},{"line_number":724,"context_line":""},{"line_number":725,"context_line":"    @mock.patch(\u0027os.path.exists\u0027)"},{"line_number":726,"context_line":"    @mock.patch(\u0027os.path.isfile\u0027)"},{"line_number":727,"context_line":"    @mock.patch(\u0027os.listdir\u0027)"},{"line_number":728,"context_line":"    @mock.patch(\u0027os.stat\u0027)"}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_6868a420","line":725,"range":{"start_line":725,"start_character":0,"end_line":725,"end_character":33},"updated":"2020-06-19 11:58:18.000000000","message":"As above.","commit_id":"49652922a70786fb593189a61dbf41c378ae9e64"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"8efd612af5a1eff8dcac86da035deba5195ef46a","unresolved":false,"context_lines":[{"line_number":732,"context_line":"                \u0027cache_dir_is_on_same_dev_as_instances_dir\u0027,"},{"line_number":733,"context_line":"                new\u003dmock.PropertyMock(return_value\u003dTrue))"},{"line_number":734,"context_line":"    def test_get_disk_usage_cache_missing(self, mock_listdir, mock_stat):"},{"line_number":735,"context_line":"        mock_listdir.return_value \u003d [\u0027onefile\u0027]"},{"line_number":736,"context_line":"        mock_stat.side_effect \u003d FileNotFoundError"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        manager \u003d imagecache.ImageCacheManager()"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_8977f002","line":735,"updated":"2020-06-20 00:06:28.000000000","message":"I tried backing out the try-except part of the fix and this test didn\u0027t fail even though it should have.\n\nI found it\u0027s because here mock_listdir is returning a file in the cache dir, when os.listdir(self.cache_dir) should actually raise FileNotFoundError if the cache dir doesn\u0027t exist.\n\nWhereas with the way this is currently: (brace yourself for these shenanigans), we return a file for a non-existent dir listing from mock_listdir, so then we call os.path.isfile() with cache_dir/onefile, which calls os.stat(cache_dir/onefile) internally, then that raises FileNotFoundError because of the mock_stat.side_effect, then os.path.isfile() CATCHES OSError, of which FileNotFoundError is a subclass, ignores it, and returns False [1], and then we never make the os.stat() call that we wrote in get_disk_usage() and thus don\u0027t fail this test when the fix is removed. We only ever call os.stat() indirectly through os.path.isfile(), which is why the assert on L741 works. GUUHHHH\n\nThat all said ...\n\nWhen I do mock_listdir.side_effect \u003d FileNotFoundError I get the expected result of the test failing without the try-except fix and passing with it. And that behavior matches when I tried a local test, as calling os.listdir() on a non-existent dir will raise FileNotFoundError and then we never call os.path.isfile() nor os.stat().\n\n[1] https://github.com/python/cpython/blob/fe75b62575bcfdf1c39be71c1e50257832a596db/Lib/genericpath.py#L25-L33","commit_id":"bcff545358a786ed315fc49a0e4d566f3715e830"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"1ea2908f8e02760d7cdc4a0694c77567d28b836e","unresolved":false,"context_lines":[{"line_number":732,"context_line":"                \u0027cache_dir_is_on_same_dev_as_instances_dir\u0027,"},{"line_number":733,"context_line":"                new\u003dmock.PropertyMock(return_value\u003dTrue))"},{"line_number":734,"context_line":"    def test_get_disk_usage_cache_missing(self, mock_listdir, mock_stat):"},{"line_number":735,"context_line":"        mock_listdir.return_value \u003d [\u0027onefile\u0027]"},{"line_number":736,"context_line":"        mock_stat.side_effect \u003d FileNotFoundError"},{"line_number":737,"context_line":""},{"line_number":738,"context_line":"        manager \u003d imagecache.ImageCacheManager()"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_205a2029","line":735,"in_reply_to":"bf51134e_8977f002","updated":"2020-06-22 15:47:19.000000000","message":"Nice catch!","commit_id":"bcff545358a786ed315fc49a0e4d566f3715e830"},{"author":{"_account_id":28332,"name":"Alexandre arents","email":"alexandre.arents@corp.ovh.com","username":"aarents"},"change_message_id":"777aada675aa6a842c5e925677643bc2af392201","unresolved":false,"context_lines":[{"line_number":741,"context_line":"            mock.Mock(st_dev\u003d0),"},{"line_number":742,"context_line":"            mock.Mock(st_dev\u003d0),"},{"line_number":743,"context_line":"            # stat calls on each file in the cache directory to get the size"},{"line_number":744,"context_line":"            mock.Mock(st_blocks\u003d10),  # foo"},{"line_number":745,"context_line":"            IOError,  # error while checking bar"},{"line_number":746,"context_line":"        ]"},{"line_number":747,"context_line":"        mock_listdir.return_value \u003d [\u0027foo\u0027, \u0027bar\u0027, \u0027some-dir\u0027]"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_6b72307d","line":744,"range":{"start_line":744,"start_character":12,"end_line":744,"end_character":35},"updated":"2020-06-23 07:25:57.000000000","message":"In that case it is better to return foo size (10 * 512) instead of 0, but it need to add another try/catch when iterating along cache file. Not sure it deserve it.","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":28332,"name":"Alexandre arents","email":"alexandre.arents@corp.ovh.com","username":"aarents"},"change_message_id":"0e4c596607a74c1a04ce8f34c9f23bcef67f19c7","unresolved":false,"context_lines":[{"line_number":741,"context_line":"            mock.Mock(st_dev\u003d0),"},{"line_number":742,"context_line":"            mock.Mock(st_dev\u003d0),"},{"line_number":743,"context_line":"            # stat calls on each file in the cache directory to get the size"},{"line_number":744,"context_line":"            mock.Mock(st_blocks\u003d10),  # foo"},{"line_number":745,"context_line":"            IOError,  # error while checking bar"},{"line_number":746,"context_line":"        ]"},{"line_number":747,"context_line":"        mock_listdir.return_value \u003d [\u0027foo\u0027, \u0027bar\u0027, \u0027some-dir\u0027]"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_18d8211c","line":744,"range":{"start_line":744,"start_character":12,"end_line":744,"end_character":35},"in_reply_to":"bf51134e_2d31695b","updated":"2020-06-23 12:54:47.000000000","message":"Agree that next run(coming very soon), will fix this \"rare\" race condition. that why I\u0027m +1","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c5e9264a6d30476769f84d6b352a54d199c8891a","unresolved":false,"context_lines":[{"line_number":741,"context_line":"            mock.Mock(st_dev\u003d0),"},{"line_number":742,"context_line":"            mock.Mock(st_dev\u003d0),"},{"line_number":743,"context_line":"            # stat calls on each file in the cache directory to get the size"},{"line_number":744,"context_line":"            mock.Mock(st_blocks\u003d10),  # foo"},{"line_number":745,"context_line":"            IOError,  # error while checking bar"},{"line_number":746,"context_line":"        ]"},{"line_number":747,"context_line":"        mock_listdir.return_value \u003d [\u0027foo\u0027, \u0027bar\u0027, \u0027some-dir\u0027]"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_2d31695b","line":744,"range":{"start_line":744,"start_character":12,"end_line":744,"end_character":35},"in_reply_to":"bf51134e_6b72307d","updated":"2020-06-23 12:25:07.000000000","message":"I only expect any kind of IOError to be transient. So the next run will fix the reservation anyhow.","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"}],"nova/virt/libvirt/imagecache.py":[{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"b61e13f890fcc3eaaf503bca3dfbb8bcb3adf62e","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        self._age_and_verify_swap_images(context, base_dir)"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"    def get_disk_usage(self):"},{"line_number":359,"context_line":"        if (not os.path.exists(self.cache_dir) or"},{"line_number":360,"context_line":"                not self.cache_dir_is_on_same_dev_as_instances_dir):"},{"line_number":361,"context_line":"            return 0"},{"line_number":362,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_f78f6c20","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":46},"updated":"2020-06-19 14:34:44.000000000","message":"Why would we not put this in the property? Anything else that calls that will just need to do the same guard here. Surely properties are intended to provide stable results and not raise any exceptions whenever possible...","commit_id":"49652922a70786fb593189a61dbf41c378ae9e64"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"ac0aacaf276eeab0b97b977a474a67a9a2a84c93","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        self._age_and_verify_swap_images(context, base_dir)"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"    def get_disk_usage(self):"},{"line_number":359,"context_line":"        if (not os.path.exists(self.cache_dir) or"},{"line_number":360,"context_line":"                not self.cache_dir_is_on_same_dev_as_instances_dir):"},{"line_number":361,"context_line":"            return 0"},{"line_number":362,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_19f15c02","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":46},"in_reply_to":"bf51134e_1704a07a","updated":"2020-06-22 13:13:50.000000000","message":"Now I think it was a mistake that we make this a public property, it should have been a private helper.\n\nI don\u0027t want to make a assumption here deep in the image cache code how the imagebackend ensures that the directory exists later on. So I don\u0027t like the idea to fold the existence check into the same dev check. If the directory does not exists then the code cannot decide if it will be on the same or on different dev.\n\nIn the other hand when the caller ask the question what is the size of the cache, then if the dir does not exists then we know that it does not occupy any space so we can return 0.","commit_id":"49652922a70786fb593189a61dbf41c378ae9e64"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"1894de46166d008423014a072a3108b0f4302ea9","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        self._age_and_verify_swap_images(context, base_dir)"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"    def get_disk_usage(self):"},{"line_number":359,"context_line":"        if (not os.path.exists(self.cache_dir) or"},{"line_number":360,"context_line":"                not self.cache_dir_is_on_same_dev_as_instances_dir):"},{"line_number":361,"context_line":"            return 0"},{"line_number":362,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_59b3d498","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":46},"in_reply_to":"bf51134e_19f15c02","updated":"2020-06-22 13:34:24.000000000","message":"If it doesn\u0027t exist now, and we return that it\u0027s currently using zero space (as in the latest version) then I don\u0027t see what the difference is.\n\nYou\u0027re making an assumption that st_dev !\u003d st_dev means they\u0027re separate, even though a complex filesystem could have a directory-based quota on the cache directory, etc, so I don\u0027t think that assuming st_dev will be the same on a directory we create in the future is unreasonable.\n\nPlus, even if the assumption about how imagebackend creates this directory in the future changes, we run this periodically and will adjust our reporting if the _base directory moves at that point, right?","commit_id":"49652922a70786fb593189a61dbf41c378ae9e64"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"0bd4253e2a5be0bbbf2750a9ef15a06a8abd1e7e","unresolved":false,"context_lines":[{"line_number":356,"context_line":"        self._age_and_verify_swap_images(context, base_dir)"},{"line_number":357,"context_line":""},{"line_number":358,"context_line":"    def get_disk_usage(self):"},{"line_number":359,"context_line":"        if (not os.path.exists(self.cache_dir) or"},{"line_number":360,"context_line":"                not self.cache_dir_is_on_same_dev_as_instances_dir):"},{"line_number":361,"context_line":"            return 0"},{"line_number":362,"context_line":""}],"source_content_type":"text/x-python","patch_set":2,"id":"bf51134e_1704a07a","line":359,"range":{"start_line":359,"start_character":12,"end_line":359,"end_character":46},"in_reply_to":"bf51134e_f78f6c20","updated":"2020-06-19 14:38:25.000000000","message":"Note that, unless I\u0027m missing something, the fact that _base doesn\u0027t exist yet necessarily means that they will be on the same filesystem, so it\u0027s fine to make the property return True in that case, as it will be created when we go to cache the first image, and will end up on the same filesystem.","commit_id":"49652922a70786fb593189a61dbf41c378ae9e64"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"13ab12c0435dba04448a7eb61ac6c7b854964b56","unresolved":false,"context_lines":[{"line_number":377,"context_line":"    def cache_dir_is_on_same_dev_as_instances_dir(self):"},{"line_number":378,"context_line":"        # NOTE(gibi): this does not work on Windows properly as st_dev is"},{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return (os.path.exists(self.cache_dir) and"},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_83bb520c","line":380,"range":{"start_line":380,"start_character":47,"end_line":380,"end_character":50},"updated":"2020-06-19 17:09:17.000000000","message":"This needs to be not-exists-or, instead of exists-and. If the thing *does not* exist, then it *will* be on the same device when we create it.","commit_id":"9431f613847100bc2ef500725c7559b6ecbbeecf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"ccff6eae300296872945bd83c6cd3e564a6ba106","unresolved":false,"context_lines":[{"line_number":377,"context_line":"    def cache_dir_is_on_same_dev_as_instances_dir(self):"},{"line_number":378,"context_line":"        # NOTE(gibi): this does not work on Windows properly as st_dev is"},{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return (os.path.exists(self.cache_dir) and"},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":3,"id":"bf51134e_e38a0e19","line":380,"range":{"start_line":380,"start_character":47,"end_line":380,"end_character":50},"in_reply_to":"bf51134e_83bb520c","updated":"2020-06-19 17:18:51.000000000","message":"Done","commit_id":"9431f613847100bc2ef500725c7559b6ecbbeecf"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d05a7169af11f671a798a8e8bae476c808e705ab","unresolved":false,"context_lines":[{"line_number":363,"context_line":"        # system as images in the cache will not grow to their virtual size."},{"line_number":364,"context_line":"        # NOTE(gibi): st.blocks is always measured in 512 byte blocks see man"},{"line_number":365,"context_line":"        # fstat"},{"line_number":366,"context_line":"        return sum("},{"line_number":367,"context_line":"            os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":368,"context_line":"            for f in os.listdir(self.cache_dir)"},{"line_number":369,"context_line":"            if os.path.isfile(os.path.join(self.cache_dir, f)))"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_f5cf61ad","line":366,"updated":"2020-06-19 17:58:23.000000000","message":"I think we need to try..except around this to ignore the does-not-exist error.","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"5ac096d67f82f0738d5a9d2eb1523f616f08c52e","unresolved":false,"context_lines":[{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return not os.path.exists(self.cache_dir) or ("},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_23d246d3","line":382,"updated":"2020-06-19 17:46:17.000000000","message":"Note to self: this method will return True if the self.cache_dir does not exist yet. Else it will make the usual st_dev comparison.\n\n(later) This seems to be the opposite of what is wanted in the commit message, to return disk usage 0 if the cache dir does not yet exist. This returns True if the cache dir doesn\u0027t exist, and the logic for returning disk usage 0 is \"if not self.cache_dir_is_on_same_dev_as_instances_dir\"\n\nIt seems like we could use a test case in nova/tests/unit/virt/libvirt/test_driver.py for this to verify the disk usage is what we want when the cache dir doesn\u0027t exist.","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"528492b27b25554014a1b72d2bdbd16eaf757c12","unresolved":false,"context_lines":[{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return not os.path.exists(self.cache_dir) or ("},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_d5bcdddd","line":382,"in_reply_to":"bf51134e_15293537","updated":"2020-06-19 18:07:05.000000000","message":"\u003e OK, that makes sense. I see now that the attempt to calculate disk\n \u003e usage of the cache dir later on is a separate problem from this\n \u003e logic. And I was realizing the logic from PS4 would work well for\n \u003e the disk usage part of things, but that wasn\u0027t really correct wrt\n \u003e the answer this property aims to give \"same dir ... eventually\".\n\nSigh...\n\ns/PS4/PS3/","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"20b94f74790cc72cc9a2bf296bf75093a17f157c","unresolved":false,"context_lines":[{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return not os.path.exists(self.cache_dir) or ("},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_356559e2","line":382,"in_reply_to":"bf51134e_23d246d3","updated":"2020-06-19 17:48:55.000000000","message":"If the cache directory does not exist, then it is not mounted on a separate filesystem. That means this returns True, because when we create it, it will be on the same_dev_as_instances_dir(). The check for this does NOT report separate cache disk usage if the cache disk and the instances disk are the same.","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"d05a7169af11f671a798a8e8bae476c808e705ab","unresolved":false,"context_lines":[{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return not os.path.exists(self.cache_dir) or ("},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_55c60d9c","line":382,"in_reply_to":"bf51134e_356559e2","updated":"2020-06-19 17:58:23.000000000","message":"Note that I still think this logic needs to be here so that merely inspecting this property doesn\u0027t explode for anyone else that uses it later.","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"d0e38bcf76efe9dc63220d37c91a34b32e1f6a86","unresolved":false,"context_lines":[{"line_number":379,"context_line":"        # always 0"},{"line_number":380,"context_line":"        return not os.path.exists(self.cache_dir) or ("},{"line_number":381,"context_line":"                os.stat(CONF.instances_path).st_dev \u003d\u003d"},{"line_number":382,"context_line":"                os.stat(self.cache_dir).st_dev)"}],"source_content_type":"text/x-python","patch_set":4,"id":"bf51134e_15293537","line":382,"in_reply_to":"bf51134e_55c60d9c","updated":"2020-06-19 18:05:59.000000000","message":"OK, that makes sense. I see now that the attempt to calculate disk usage of the cache dir later on is a separate problem from this logic. And I was realizing the logic from PS4 would work well for the disk usage part of things, but that wasn\u0027t really correct wrt the answer this property aims to give \"same dir ... eventually\".","commit_id":"c3120fdddacd6827402d9978f50641cd0c9c2719"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"359710b9e84dfae8482f2587d4499324941639ee","unresolved":false,"context_lines":[{"line_number":368,"context_line":"                os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":369,"context_line":"                for f in os.listdir(self.cache_dir)"},{"line_number":370,"context_line":"                if os.path.isfile(os.path.join(self.cache_dir, f)))"},{"line_number":371,"context_line":"        except FileNotFoundError:"},{"line_number":372,"context_line":"            # NOTE(danms): If the directory does not exist there is"},{"line_number":373,"context_line":"            # nothing to count (yet)"},{"line_number":374,"context_line":"            return 0"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_757cf1b4","line":371,"range":{"start_line":371,"start_character":15,"end_line":371,"end_character":32},"updated":"2020-06-19 18:35:22.000000000","message":"Note: I did some local testing and FileNotFoundError is correct for os.stat\u0027ing something that doesn\u0027t exist in python3, but in python2 I find that OSError is raised in the same situation. Just worth noting for backports...","commit_id":"bcff545358a786ed315fc49a0e4d566f3715e830"},{"author":{"_account_id":26458,"name":"Brin Zhang","email":"zhangbailin@inspur.com","username":"zhangbailin"},"change_message_id":"8f93118362488589b16843c5be5957a303e5bcac","unresolved":false,"context_lines":[{"line_number":368,"context_line":"                os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":369,"context_line":"                for f in os.listdir(self.cache_dir)"},{"line_number":370,"context_line":"                if os.path.isfile(os.path.join(self.cache_dir, f)))"},{"line_number":371,"context_line":"        except FileNotFoundError:"},{"line_number":372,"context_line":"            # NOTE(danms): If the directory does not exist there is"},{"line_number":373,"context_line":"            # nothing to count (yet)"},{"line_number":374,"context_line":"            return 0"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_35a2554c","line":371,"range":{"start_line":371,"start_character":15,"end_line":371,"end_character":32},"in_reply_to":"bf51134e_0086fc66","updated":"2020-06-23 05:43:47.000000000","message":"\u003e I think we can have more than a FileNotFoundError here. Like\n \u003e OSError with Permission denied error code. So for safety I added\n \u003e OSError here.\n\n+1 to use OSError instead of using FileNotFoundError, it\u0027s save.","commit_id":"bcff545358a786ed315fc49a0e4d566f3715e830"},{"author":{"_account_id":4393,"name":"Dan Smith","email":"dms@danplanet.com","username":"danms"},"change_message_id":"91bd0c168d1ae7286a410efee63d7ed06e41a21d","unresolved":false,"context_lines":[{"line_number":368,"context_line":"                os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":369,"context_line":"                for f in os.listdir(self.cache_dir)"},{"line_number":370,"context_line":"                if os.path.isfile(os.path.join(self.cache_dir, f)))"},{"line_number":371,"context_line":"        except FileNotFoundError:"},{"line_number":372,"context_line":"            # NOTE(danms): If the directory does not exist there is"},{"line_number":373,"context_line":"            # nothing to count (yet)"},{"line_number":374,"context_line":"            return 0"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_f82598ac","line":371,"range":{"start_line":371,"start_character":15,"end_line":371,"end_character":32},"in_reply_to":"bf51134e_757cf1b4","updated":"2020-06-19 18:41:37.000000000","message":"Yup, this is the py3 version. I was going to say \"there\u0027s nothing to backport since this was just merged\", but it was backported a long ways back, so... yeah.","commit_id":"bcff545358a786ed315fc49a0e4d566f3715e830"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"1ea2908f8e02760d7cdc4a0694c77567d28b836e","unresolved":false,"context_lines":[{"line_number":368,"context_line":"                os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":369,"context_line":"                for f in os.listdir(self.cache_dir)"},{"line_number":370,"context_line":"                if os.path.isfile(os.path.join(self.cache_dir, f)))"},{"line_number":371,"context_line":"        except FileNotFoundError:"},{"line_number":372,"context_line":"            # NOTE(danms): If the directory does not exist there is"},{"line_number":373,"context_line":"            # nothing to count (yet)"},{"line_number":374,"context_line":"            return 0"}],"source_content_type":"text/x-python","patch_set":5,"id":"bf51134e_0086fc66","line":371,"range":{"start_line":371,"start_character":15,"end_line":371,"end_character":32},"in_reply_to":"bf51134e_f82598ac","updated":"2020-06-22 15:47:19.000000000","message":"I think we can have more than a FileNotFoundError here. Like OSError with Permission denied error code. So for safety I added OSError here.","commit_id":"bcff545358a786ed315fc49a0e4d566f3715e830"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9a634c4e63a1c52125ee5a90a8683e1b752adb2e","unresolved":false,"context_lines":[{"line_number":359,"context_line":"        try:"},{"line_number":360,"context_line":"            # If the cache is on a different device than the instance dir then"},{"line_number":361,"context_line":"            # it does not consume the same disk resource as instances."},{"line_number":362,"context_line":"            # NOTE(gibi): this does not work on Windows properly as st_dev is"},{"line_number":363,"context_line":"            # always 0"},{"line_number":364,"context_line":"            if (os.stat(CONF.instances_path).st_dev !\u003d"},{"line_number":365,"context_line":"                    os.stat(self.cache_dir).st_dev):"},{"line_number":366,"context_line":"                return 0"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_8a969b3d","line":363,"range":{"start_line":362,"start_character":0,"end_line":363,"end_character":22},"updated":"2020-06-23 11:22:13.000000000","message":"As an aside, we should probably (in a separate change) put a check at the start of the libvirt driver to crash and burn if platform !\u003d Linux?","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c5e9264a6d30476769f84d6b352a54d199c8891a","unresolved":false,"context_lines":[{"line_number":359,"context_line":"        try:"},{"line_number":360,"context_line":"            # If the cache is on a different device than the instance dir then"},{"line_number":361,"context_line":"            # it does not consume the same disk resource as instances."},{"line_number":362,"context_line":"            # NOTE(gibi): this does not work on Windows properly as st_dev is"},{"line_number":363,"context_line":"            # always 0"},{"line_number":364,"context_line":"            if (os.stat(CONF.instances_path).st_dev !\u003d"},{"line_number":365,"context_line":"                    os.stat(self.cache_dir).st_dev):"},{"line_number":366,"context_line":"                return 0"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_aa539f6a","line":363,"range":{"start_line":362,"start_character":0,"end_line":363,"end_character":22},"in_reply_to":"bf51134e_8a969b3d","updated":"2020-06-23 12:25:07.000000000","message":"yeah the libvirt driver reads from /sys and /proc so it is pretty Linux dependent. I can fire up a separate patch for that check. See https://review.opendev.org/737508","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9a634c4e63a1c52125ee5a90a8683e1b752adb2e","unresolved":false,"context_lines":[{"line_number":365,"context_line":"                    os.stat(self.cache_dir).st_dev):"},{"line_number":366,"context_line":"                return 0"},{"line_number":367,"context_line":""},{"line_number":368,"context_line":"            # NOTE(gibi): we need to use the disk size occupied from the file"},{"line_number":369,"context_line":"            # system as images in the cache will not grow to their virtual"},{"line_number":370,"context_line":"            # size."},{"line_number":371,"context_line":"            # NOTE(gibi): st.blocks is always measured in 512 byte blocks see"},{"line_number":372,"context_line":"            # man fstat"},{"line_number":373,"context_line":"            return sum("}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_ca225352","line":370,"range":{"start_line":368,"start_character":0,"end_line":370,"end_character":19},"updated":"2020-06-23 11:22:13.000000000","message":"So we\u0027re using \u0027os.stat(path).st_blocks * 512\u0027 instead of e.g. \u0027os.path.getsize(path)\u0027. Checked locally:\n\n  $ dd if\u003d/dev/zero of\u003dsparse_file bs\u003d1 count\u003d0 seek\u003d512M\n  $ python\n  \u003e\u003e\u003e import os.path\n  \u003e\u003e\u003e os.path.getsize(\u0027sparse_file\u0027)\n  536870912\n  \u003e\u003e\u003e os.stat(\u0027sparse_file\u0027).st_blocks * 512\n  0\n\nWith that said, can\u0027t we just do?\n\n  \u003e\u003e\u003e os.stat(\u0027sparse_file\u0027).st_blocks\n  0\n\nLooks like it\u0027s the same thing from the docs [1]\n\n  Number of 512-byte blocks allocated for file. This may be smaller\n  than st_size/512 when the file has holes.\n\n[1] https://docs.python.org/3/library/os.html#os.stat_result.st_blocks","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"97d48437b556df7730aaf0cdb1fb96b378aaefb5","unresolved":false,"context_lines":[{"line_number":365,"context_line":"                    os.stat(self.cache_dir).st_dev):"},{"line_number":366,"context_line":"                return 0"},{"line_number":367,"context_line":""},{"line_number":368,"context_line":"            # NOTE(gibi): we need to use the disk size occupied from the file"},{"line_number":369,"context_line":"            # system as images in the cache will not grow to their virtual"},{"line_number":370,"context_line":"            # size."},{"line_number":371,"context_line":"            # NOTE(gibi): st.blocks is always measured in 512 byte blocks see"},{"line_number":372,"context_line":"            # man fstat"},{"line_number":373,"context_line":"            return sum("}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_587a99f0","line":370,"range":{"start_line":368,"start_character":0,"end_line":370,"end_character":19},"in_reply_to":"bf51134e_2de329db","updated":"2020-06-23 13:02:08.000000000","message":"\u003e We need to return bytes not number of blocks so st_blocks is not\n \u003e enough by itself.\n\nSorry, ignore that. I confused myself and thought it to be a different API /o\\","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c5e9264a6d30476769f84d6b352a54d199c8891a","unresolved":false,"context_lines":[{"line_number":365,"context_line":"                    os.stat(self.cache_dir).st_dev):"},{"line_number":366,"context_line":"                return 0"},{"line_number":367,"context_line":""},{"line_number":368,"context_line":"            # NOTE(gibi): we need to use the disk size occupied from the file"},{"line_number":369,"context_line":"            # system as images in the cache will not grow to their virtual"},{"line_number":370,"context_line":"            # size."},{"line_number":371,"context_line":"            # NOTE(gibi): st.blocks is always measured in 512 byte blocks see"},{"line_number":372,"context_line":"            # man fstat"},{"line_number":373,"context_line":"            return sum("}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_2de329db","line":370,"range":{"start_line":368,"start_character":0,"end_line":370,"end_character":19},"in_reply_to":"bf51134e_ca225352","updated":"2020-06-23 12:25:07.000000000","message":"We need to return bytes not number of blocks so st_blocks is not enough by itself.","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":15334,"name":"Stephen Finucane","display_name":"stephenfin","email":"stephenfin@redhat.com","username":"sfinucan"},"change_message_id":"9a634c4e63a1c52125ee5a90a8683e1b752adb2e","unresolved":false,"context_lines":[{"line_number":371,"context_line":"            # NOTE(gibi): st.blocks is always measured in 512 byte blocks see"},{"line_number":372,"context_line":"            # man fstat"},{"line_number":373,"context_line":"            return sum("},{"line_number":374,"context_line":"                os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":375,"context_line":"                for f in os.listdir(self.cache_dir)"},{"line_number":376,"context_line":"                if os.path.isfile(os.path.join(self.cache_dir, f)))"},{"line_number":377,"context_line":"        except OSError:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_6acea7b4","line":374,"range":{"start_line":374,"start_character":69,"end_line":374,"end_character":72},"updated":"2020-06-23 11:22:13.000000000","message":"fwiw, I checked to see if this was possible to get via os.stat and it doesn\u0027t seem to be. Closest we can get is \u0027os.stat(foo).st_blksize\u0027 which returns the *preferred* block size (4096 on my local machine)","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":9708,"name":"Balazs Gibizer","display_name":"gibi","email":"gibizer@gmail.com","username":"gibi"},"change_message_id":"c5e9264a6d30476769f84d6b352a54d199c8891a","unresolved":false,"context_lines":[{"line_number":371,"context_line":"            # NOTE(gibi): st.blocks is always measured in 512 byte blocks see"},{"line_number":372,"context_line":"            # man fstat"},{"line_number":373,"context_line":"            return sum("},{"line_number":374,"context_line":"                os.stat(os.path.join(self.cache_dir, f)).st_blocks * 512"},{"line_number":375,"context_line":"                for f in os.listdir(self.cache_dir)"},{"line_number":376,"context_line":"                if os.path.isfile(os.path.join(self.cache_dir, f)))"},{"line_number":377,"context_line":"        except OSError:"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_0dc3c539","line":374,"range":{"start_line":374,"start_character":69,"end_line":374,"end_character":72},"in_reply_to":"bf51134e_6acea7b4","updated":"2020-06-23 12:25:07.000000000","message":"Yeah, I went through this.","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"},{"author":{"_account_id":4690,"name":"melanie witt","display_name":"melwitt","email":"melwittt@gmail.com","username":"melwitt"},"change_message_id":"005b10ec0cff172ba1880451a53a4d1fa327d34f","unresolved":false,"context_lines":[{"line_number":378,"context_line":"            # NOTE(gibi): An error here can mean many things. E.g. the cache"},{"line_number":379,"context_line":"            # dir does not exists yet, the cache dir is deleted between the"},{"line_number":380,"context_line":"            # listdir() and the stat() calls, or a file is deleted between"},{"line_number":381,"context_line":"            # the listdir() and the stat() calls."},{"line_number":382,"context_line":"            return 0"},{"line_number":383,"context_line":""},{"line_number":384,"context_line":"    @property"}],"source_content_type":"text/x-python","patch_set":6,"id":"bf51134e_f02bb991","line":381,"updated":"2020-06-23 22:13:13.000000000","message":"Yup, I prefer this to use OSError here. Good comment too","commit_id":"a85753778f710f03616a682d294f8f638dea6baf"}]}