)]}'
{"ironic/conductor/manager.py":[{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"148fac1cac95c2d8aaa8a163e7a3ed6a62b91e54","unresolved":false,"context_lines":[{"line_number":674,"context_line":"            if rebuild:"},{"line_number":675,"context_line":"                event \u003d \u0027rebuild\u0027"},{"line_number":676,"context_line":"                try:"},{"line_number":677,"context_line":"                    task.driver.deploy.clean_up(task)"},{"line_number":678,"context_line":"                except Exception as e:"},{"line_number":679,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":680,"context_line":"                        LOG.warning(_LW(\u0027Error in clean_up of node %(node)s: \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_9b14c2a0","line":677,"updated":"2015-01-11 15:57:18.000000000","message":"We are trying to do a clean_up() on the same thread which is processing the rpc message. I think we should do this in the spawned worker - may be in _do_node_deploy() ??","commit_id":"c448b381c20489ae74642b3114950b77fdf284f5"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"0dab318381f7af70edca24f83a3dc3cbeeb99850","unresolved":false,"context_lines":[{"line_number":674,"context_line":"            if rebuild:"},{"line_number":675,"context_line":"                event \u003d \u0027rebuild\u0027"},{"line_number":676,"context_line":"                try:"},{"line_number":677,"context_line":"                    task.driver.deploy.clean_up(task)"},{"line_number":678,"context_line":"                except Exception as e:"},{"line_number":679,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":680,"context_line":"                        LOG.warning(_LW(\u0027Error in clean_up of node %(node)s: \u0027"}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_8d239aa9","line":677,"in_reply_to":"3a961159_9b14c2a0","updated":"2015-01-12 13:03:30.000000000","message":"Done","commit_id":"c448b381c20489ae74642b3114950b77fdf284f5"},{"author":{"_account_id":9315,"name":"ramesh","email":"rameshg87@gmail.com","username":"rameshg87"},"change_message_id":"148fac1cac95c2d8aaa8a163e7a3ed6a62b91e54","unresolved":false,"context_lines":[{"line_number":677,"context_line":"                    task.driver.deploy.clean_up(task)"},{"line_number":678,"context_line":"                except Exception as e:"},{"line_number":679,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":680,"context_line":"                        LOG.warning(_LW(\u0027Error in clean_up of node %(node)s: \u0027"},{"line_number":681,"context_line":"                                        \u0027%(err)s\u0027),"},{"line_number":682,"context_line":"                                    {\u0027node\u0027: task.node.uuid, \u0027err\u0027: e})"},{"line_number":683,"context_line":"                        node.last_error \u003d _(\"Failed to clean up before doing \""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_7b28f6ee","line":680,"updated":"2015-01-11 15:57:18.000000000","message":"Why only a warning ? This is failing the rebuild. It should be error, right ?","commit_id":"c448b381c20489ae74642b3114950b77fdf284f5"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"0dab318381f7af70edca24f83a3dc3cbeeb99850","unresolved":false,"context_lines":[{"line_number":677,"context_line":"                    task.driver.deploy.clean_up(task)"},{"line_number":678,"context_line":"                except Exception as e:"},{"line_number":679,"context_line":"                    with excutils.save_and_reraise_exception():"},{"line_number":680,"context_line":"                        LOG.warning(_LW(\u0027Error in clean_up of node %(node)s: \u0027"},{"line_number":681,"context_line":"                                        \u0027%(err)s\u0027),"},{"line_number":682,"context_line":"                                    {\u0027node\u0027: task.node.uuid, \u0027err\u0027: e})"},{"line_number":683,"context_line":"                        node.last_error \u003d _(\"Failed to clean up before doing \""}],"source_content_type":"text/x-python","patch_set":2,"id":"3a961159_ad4f7669","line":680,"in_reply_to":"3a961159_7b28f6ee","updated":"2015-01-12 13:03:30.000000000","message":"Seems that it should as it includes exception. Done.","commit_id":"c448b381c20489ae74642b3114950b77fdf284f5"}],"ironic/drivers/modules/image_cache.py":[{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"0e6eccd214bbcdf8c3400eed3cf8a649bf6430fc","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        # TODO(dtantsur): lock expiration time"},{"line_number":108,"context_line":"        with lockutils.lock(img_download_lock_name, \u0027ironic-\u0027):"},{"line_number":109,"context_line":"            path_exists \u003d os.path.exists(dest_path)"},{"line_number":110,"context_line":"            if path_exists and os.readlink(dest_path) \u003d\u003d master_path:"},{"line_number":111,"context_line":"                LOG.debug(\"Destination %(dest)s already exists for \""},{"line_number":112,"context_line":"                            \"image %(uuid)s\" %"},{"line_number":113,"context_line":"                          {\u0027uuid\u0027: href,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a961159_8dbb08c3","line":110,"updated":"2015-01-07 11:59:52.000000000","message":"nit: os.readlink may return the relative path to the file it\u0027s pointing too, it would be good if we used the full path instead:\n\n os.path.join(os.path.dirname(dest_path), os.readlink(dest_path))","commit_id":"2608fc2982f83f67af030f05f0418323c75fc259"},{"author":{"_account_id":9420,"name":"Matthew Gilliard","email":"matthew.gilliard@hp.com","username":"gilliard"},"change_message_id":"dcc159a44239988dddd280564d216d6e6c1e1490","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        # TODO(dtantsur): lock expiration time"},{"line_number":108,"context_line":"        with lockutils.lock(img_download_lock_name, \u0027ironic-\u0027):"},{"line_number":109,"context_line":"            path_exists \u003d os.path.exists(dest_path)"},{"line_number":110,"context_line":"            if path_exists and os.readlink(dest_path) \u003d\u003d master_path:"},{"line_number":111,"context_line":"                LOG.debug(\"Destination %(dest)s already exists for \""},{"line_number":112,"context_line":"                            \"image %(uuid)s\" %"},{"line_number":113,"context_line":"                          {\u0027uuid\u0027: href,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a961159_6fb20de7","line":110,"in_reply_to":"3a961159_2c62d33d","updated":"2015-01-09 14:31:09.000000000","message":"Yes, you are right. os.readlink follows symlinks, but os.link (line#124) creates hard links. So this fails with InvalidArgument because dest_path is not a symlink.","commit_id":"2608fc2982f83f67af030f05f0418323c75fc259"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"56e6c5c8fc774391f36c580d1b691fc574e7b171","unresolved":false,"context_lines":[{"line_number":107,"context_line":"        # TODO(dtantsur): lock expiration time"},{"line_number":108,"context_line":"        with lockutils.lock(img_download_lock_name, \u0027ironic-\u0027):"},{"line_number":109,"context_line":"            path_exists \u003d os.path.exists(dest_path)"},{"line_number":110,"context_line":"            if path_exists and os.readlink(dest_path) \u003d\u003d master_path:"},{"line_number":111,"context_line":"                LOG.debug(\"Destination %(dest)s already exists for \""},{"line_number":112,"context_line":"                            \"image %(uuid)s\" %"},{"line_number":113,"context_line":"                          {\u0027uuid\u0027: href,"}],"source_content_type":"text/x-python","patch_set":1,"id":"3a961159_2c62d33d","line":110,"in_reply_to":"3a961159_8dbb08c3","updated":"2015-01-09 13:47:31.000000000","message":"I may be wrong, but I think it\u0027s not a correct way to fix it.\n\ndest_path is path to image file in \"\u003cnode_uuid\u003e\" folder, while master_path is pointing to same image file in \"master_images\" folder. You should check here that master_path inode number is equal to dest_path inode number and if not, unlink dest_path.","commit_id":"2608fc2982f83f67af030f05f0418323c75fc259"},{"author":{"_account_id":10239,"name":"Dmitry Tantsur","email":"dtantsur@protonmail.com","username":"dtantsur"},"change_message_id":"9deab3589f147f9fbe5620b1dc367d0876252420","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        with lockutils.lock(img_download_lock_name, \u0027ironic-\u0027):"},{"line_number":109,"context_line":"            if os.path.exists(dest_path):"},{"line_number":110,"context_line":"                # NOTE(vdrok): After rebuild requested image can change, so we"},{"line_number":111,"context_line":"                # should ensure that dest_path and master_path (if exists) are"},{"line_number":112,"context_line":"                # pointing to the same file"},{"line_number":113,"context_line":"                if (os.path.exists(master_path) and"},{"line_number":114,"context_line":"                        (os.stat(dest_path).st_ino \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a961159_3153826d","line":111,"updated":"2015-01-14 16:39:51.000000000","message":"nit: I guess s/exists/exist/, but not sure","commit_id":"2f0c8cd8d8d8b2846b641b518b742ca051425028"},{"author":{"_account_id":8688,"name":"Alexis Lee","email":"openstack@lxsli.co.uk","username":"lxsli"},"change_message_id":"bb710ce9bb2c9e4251331b91fe439d12c0d56ebb","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        with lockutils.lock(img_download_lock_name, \u0027ironic-\u0027):"},{"line_number":109,"context_line":"            if os.path.exists(dest_path):"},{"line_number":110,"context_line":"                # NOTE(vdrok): After rebuild requested image can change, so we"},{"line_number":111,"context_line":"                # should ensure that dest_path and master_path (if exists) are"},{"line_number":112,"context_line":"                # pointing to the same file"},{"line_number":113,"context_line":"                if (os.path.exists(master_path) and"},{"line_number":114,"context_line":"                        (os.stat(dest_path).st_ino \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a961159_03055d0f","line":111,"updated":"2015-01-14 16:43:58.000000000","message":"nit: should be \"if it exists\"","commit_id":"2f0c8cd8d8d8b2846b641b518b742ca051425028"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"e529bba9560149a9155be004864cef63ba1eaffa","unresolved":false,"context_lines":[{"line_number":108,"context_line":"        with lockutils.lock(img_download_lock_name, \u0027ironic-\u0027):"},{"line_number":109,"context_line":"            if os.path.exists(dest_path):"},{"line_number":110,"context_line":"                # NOTE(vdrok): After rebuild requested image can change, so we"},{"line_number":111,"context_line":"                # should ensure that dest_path and master_path (if exists) are"},{"line_number":112,"context_line":"                # pointing to the same file"},{"line_number":113,"context_line":"                if (os.path.exists(master_path) and"},{"line_number":114,"context_line":"                        (os.stat(dest_path).st_ino \u003d\u003d"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a961159_9127d6c6","line":111,"in_reply_to":"3a961159_3153826d","updated":"2015-01-14 16:43:46.000000000","message":"That relates only to master_path :) dest_path exists because of prev line.","commit_id":"2f0c8cd8d8d8b2846b641b518b742ca051425028"},{"author":{"_account_id":6773,"name":"Lucas Alvares Gomes","email":"lucasagomes@gmail.com","username":"lucasagomes"},"change_message_id":"ee9fe77c866cfca6178f0c06585b9c868feb1e9c","unresolved":false,"context_lines":[{"line_number":123,"context_line":"            try:"},{"line_number":124,"context_line":"                # NOTE(dtantsur): ensure we\u0027re not in the middle of clean up"},{"line_number":125,"context_line":"                with lockutils.lock(\u0027master_image\u0027, \u0027ironic-\u0027):"},{"line_number":126,"context_line":"                    os.link(master_path, dest_path)"},{"line_number":127,"context_line":"            except OSError:"},{"line_number":128,"context_line":"                LOG.info(_LI(\"Master cache miss for image %(uuid)s, \""},{"line_number":129,"context_line":"                             \"starting download\"),"}],"source_content_type":"text/x-python","patch_set":6,"id":"3a961159_0088cf69","line":126,"updated":"2015-01-15 12:21:03.000000000","message":"Oh, now I see we use hard-links","commit_id":"2f0c8cd8d8d8b2846b641b518b742ca051425028"}],"ironic/tests/drivers/test_image_cache.py":[{"author":{"_account_id":10343,"name":"Jim Rollenhagen","email":"jim@jimrollenhagen.com","username":"jimrollenhagen"},"change_message_id":"799005a9e2a8e1ed5554a14ce97abbb6a03a593d","unresolved":false,"context_lines":[{"line_number":80,"context_line":"        self.assertTrue(os.path.isfile(self.dest_path))"},{"line_number":81,"context_line":"        self.assertEqual(os.stat(self.dest_path).st_ino,"},{"line_number":82,"context_line":"                         os.stat(self.master_path).st_ino)"},{"line_number":83,"context_line":"        self.assertFalse(mock_clean_up.called)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @mock.patch.object(image_cache.ImageCache, \u0027clean_up\u0027)"},{"line_number":86,"context_line":"    @mock.patch.object(image_cache.ImageCache, \u0027_download_image\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a961159_f01bb86c","line":83,"updated":"2015-01-14 15:14:35.000000000","message":"I think we should assert that unlink() was called here. Or at least `assertFalse(os.stat(self.dest_path).st_ino, os.stat(self.master_path).st_ino)` before calling fetch_image().","commit_id":"d7a2ec6d49619f735ec976f63fd2bde7d15336a4"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"4001646b722db151e522b35bac2bcdc12411e539","unresolved":false,"context_lines":[{"line_number":80,"context_line":"        self.assertTrue(os.path.isfile(self.dest_path))"},{"line_number":81,"context_line":"        self.assertEqual(os.stat(self.dest_path).st_ino,"},{"line_number":82,"context_line":"                         os.stat(self.master_path).st_ino)"},{"line_number":83,"context_line":"        self.assertFalse(mock_clean_up.called)"},{"line_number":84,"context_line":""},{"line_number":85,"context_line":"    @mock.patch.object(image_cache.ImageCache, \u0027clean_up\u0027)"},{"line_number":86,"context_line":"    @mock.patch.object(image_cache.ImageCache, \u0027_download_image\u0027)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a961159_b62a08e5","line":83,"in_reply_to":"3a961159_f01bb86c","updated":"2015-01-14 16:32:20.000000000","message":"If I\u0027ll mock os.unlink, I\u0027ll have to mock os.link also as it will fail to do os.link(master_path, dest_path) because dest_path was not unlinked. And if I\u0027ll mock os.link check on L81 won\u0027t work as they will remain different files. As for doing st_ino comparison before fetch_image, I think it\u0027s unnecessary, because touch creates 2 different files which can\u0027t have same inode number. But if you want I\u0027ll add it.","commit_id":"d7a2ec6d49619f735ec976f63fd2bde7d15336a4"},{"author":{"_account_id":12356,"name":"Vladyslav Drok","email":"vdrok@mirantis.com","username":"vdrok"},"change_message_id":"4001646b722db151e522b35bac2bcdc12411e539","unresolved":false,"context_lines":[{"line_number":88,"context_line":"                                          mock_clean_up, mock_fetch):"},{"line_number":89,"context_line":"        touch(self.dest_path)"},{"line_number":90,"context_line":"        self.cache.fetch_image(self.uuid, self.dest_path)"},{"line_number":91,"context_line":"        self.assertFalse(mock_fetch.called)"},{"line_number":92,"context_line":"        mock_download.assert_called_once_with("},{"line_number":93,"context_line":"            self.uuid, self.master_path, self.dest_path,"},{"line_number":94,"context_line":"            ctx\u003dNone, force_raw\u003dTrue)"}],"source_content_type":"text/x-python","patch_set":5,"id":"3a961159_f1c57a90","line":91,"updated":"2015-01-14 16:32:20.000000000","message":"Though here os.unlink can be mocked, will add it here.","commit_id":"d7a2ec6d49619f735ec976f63fd2bde7d15336a4"}]}