From b00e479637955cec8a58a81db934d2ddf743a219 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Tue, 11 Jun 2013 14:28:48 -0400 Subject: Return correct status when deleting non-existing container The code was raising an exception when the container (which happens to be a directory) did not exist. To be compatible with OpenStack Swift, we need to handle an object which its container/directory does not exist. BUG: 960944 (https://bugzilla.redhat.com/show_bug.cgi?id=960944) Change-Id: Ibb2db354a655e040fb70ebbe6a7d8f815d33dc0f Signed-off-by: Luis Pabon Reviewed-on: http://review.gluster.org/5201 Reviewed-by: Peter Portante Tested-by: Peter Portante --- gluster/swift/common/DiskDir.py | 36 +++++++++++++++++++++++++----------- test/unit/common/test_diskdir.py | 34 ++++++++++++++++++++++++++++++++++ test/unit/proxy/test_server.py | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/gluster/swift/common/DiskDir.py b/gluster/swift/common/DiskDir.py index 50e795c..c5793f5 100644 --- a/gluster/swift/common/DiskDir.py +++ b/gluster/swift/common/DiskDir.py @@ -25,6 +25,7 @@ from gluster.swift.common.utils import validate_account, validate_container, \ X_CONTENT_LENGTH, X_TIMESTAMP, X_PUT_TIMESTAMP, X_ETAG, X_OBJECTS_COUNT, \ X_BYTES_USED, X_CONTAINER_COUNT, DIR_TYPE from gluster.swift.common import Glusterfs +from gluster.swift.common.exceptions import FileOrDirNotFoundError DATADIR = 'containers' @@ -182,8 +183,15 @@ class DiskCommon(object): return not os_path.exists(self.datadir) def empty(self): - # FIXME: Common because ported swift AccountBroker unit tests use it. - return dir_empty(self.datadir) + # If it does not exist, then it is empty. A value of True is + # what is expected by OpenStack Swift when the directory does + # not exist. Check swift/common/db.py:ContainerBroker.empty() + # and swift/container/server.py:ContainerController.DELETE() + # for more information + try: + return dir_empty(self.datadir) + except FileOrDirNotFoundError: + return True def update_metadata(self, metadata): assert self.metadata, "Valid container/account metadata should have " \ @@ -404,13 +412,16 @@ class DiskDir(DiskCommon): reported_put_timestamp, reported_delete_timestamp, reported_object_count, and reported_bytes_used. """ - if not Glusterfs.OBJECT_ONLY: - # If we are not configured for object only environments, we should - # update the object counts in case they changed behind our back. - self._update_object_count() - else: - # FIXME: to facilitate testing, we need to update all the time - self._update_object_count() + if self._dir_exists: + if not Glusterfs.OBJECT_ONLY: + # If we are not configured for object only environments, + # we should update the object counts in case they changed + # behind our back. + self._update_object_count() + else: + # FIXME: to facilitate testing, we need to update all + # the time + self._update_object_count() data = {'account': self.account, 'container': self.container, 'object_count': self.metadata.get( @@ -478,8 +489,11 @@ class DiskDir(DiskCommon): :param timestamp: delete timestamp """ - if not dir_empty(self.datadir): - # FIXME: This is a failure condition here, isn't it? + try: + if not dir_empty(self.datadir): + # FIXME: This is a failure condition here, isn't it? + return + except FileOrDirNotFoundError: return rmdirs(self.datadir) diff --git a/test/unit/common/test_diskdir.py b/test/unit/common/test_diskdir.py index 580e76a..be0c922 100644 --- a/test/unit/common/test_diskdir.py +++ b/test/unit/common/test_diskdir.py @@ -344,6 +344,22 @@ class TestDiskCommon(unittest.TestCase): del dc.metadata['X-Container-Meta-foo'] assert dc.metadata == md_copy + def test_empty_dir_is_not_empty(self): + dc = dd.DiskCommon(self.td, self.fake_drives[0], + self.fake_accounts[0], self.fake_logger) + os.makedirs(os.path.join(self.td, self.fake_drives[0], 'aaabbbccc')) + self.assertFalse(dc.empty()) + + def test_empty_dir_is_empty(self): + dc = dd.DiskCommon(self.td, self.fake_drives[0], + self.fake_accounts[0], self.fake_logger) + self.assertTrue(dc.empty()) + + def test_empty_dir_does_not_exist(self): + dc = dd.DiskCommon(self.td, 'non_existent_drive', + self.fake_accounts[0], self.fake_logger) + self.assertTrue(dc.empty()) + class TestContainerBroker(unittest.TestCase): """ @@ -498,6 +514,24 @@ class TestContainerBroker(unittest.TestCase): self.assertEquals(info['x_container_sync_point1'], -1) self.assertEquals(info['x_container_sync_point2'], -1) + def test_get_info_nonexistent_container(self): + broker = dd.DiskDir(self.path, self.drive, account='no_account', + container='no_container', logger=FakeLogger()) + info = broker.get_info() + + # + # Because broker._dir_exists is False and _update_object_count() + # has not been called yet, the values returned for + # object_count, bytes_used, and put_timestamp are '0' as + # a string. OpenStack Swift handles this situation by + # passing the value to float(). + # + self.assertEquals(info['account'], 'no_account') + self.assertEquals(info['container'], 'no_container') + self.assertEquals(info['object_count'], '0') + self.assertEquals(info['bytes_used'], '0') + self.assertEquals(info['put_timestamp'], '0') + def test_set_x_syncs(self): broker = self._get_broker(account='test1', container='test2') diff --git a/test/unit/proxy/test_server.py b/test/unit/proxy/test_server.py index a04380d..57e3111 100644 --- a/test/unit/proxy/test_server.py +++ b/test/unit/proxy/test_server.py @@ -4512,6 +4512,41 @@ class TestContainerController(unittest.TestCase): self.assert_status_map(controller.DELETE, (200, 404, 404, 404), 404) + def test_DELETE_container_that_does_not_exist(self): + prolis = _test_sockets[0] + # Create a container + sock = connect_tcp(('localhost', prolis.getsockname()[1])) + fd = sock.makefile() + fd.write('PUT /v1/a/aaabbbccc HTTP/1.1\r\nHost: localhost\r\n' + 'Connection: close\r\nX-Storage-Token: t\r\n' + 'Content-Length: 0\r\n\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = 'HTTP/1.1 201' + self.assertEquals(headers[:len(exp)], exp) + + # Delete container + sock = connect_tcp(('localhost', prolis.getsockname()[1])) + fd = sock.makefile() + fd.write('DELETE /v1/a/aaabbbccc HTTP/1.1\r\nHost: localhost\r\n' + 'Connection: close\r\nX-Storage-Token: t\r\n' + 'Content-Length: 0\r\n\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = 'HTTP/1.1 204' + self.assertEquals(headers[:len(exp)], exp) + + # Delete again + sock = connect_tcp(('localhost', prolis.getsockname()[1])) + fd = sock.makefile() + fd.write('DELETE /v1/a/aaabbbccc HTTP/1.1\r\nHost: localhost\r\n' + 'Connection: close\r\nX-Storage-Token: t\r\n' + 'Content-Length: 0\r\n\r\n') + fd.flush() + headers = readuntil2crlfs(fd) + exp = 'HTTP/1.1 404' + self.assertEquals(headers[:len(exp)], exp) + def test_response_get_accept_ranges_header(self): with save_globals(): set_http_connect(200, 200, body='{}') -- cgit