openstack/swift
Code Issues Proposed changes

Require logger passed to broker in unit tests for account-reaper

Browse Source 
This patch removes default value for logger passed to
faked BrokerClass in some unit tests for account reaper,
to make sure that a logger instance is properly passed
when generation BrokerClass instance.

This patch also removes unnecessory parameter, which is
added to confirm logger passed to broker, but is not
useful in fact.

NOTE: This patch is a follow-up of my previous commit[1]

[1] https://review.opendev.org/#/c/295875/

Change-Id: Ica10e3ac42375c2dc141478e647408df90028ae9
This commit is contained in:
Takashi Kajinami
2019-05-03 17:50:15 +09:00
parent ce1ba6a51d
commit 211cf3983c
2 changed files with 26 additions and 63 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
test/unit/account/test_reaper.py
View File

@@ -20,7 +20,6 @@ import shutil
import tempfile import tempfile
import unittest import unittest
from logging import DEBUG
from mock import patch, call, DEFAULT from mock import patch, call, DEFAULT
import eventlet import eventlet
@@ -33,39 +32,6 @@ from test import unit
from swift.common.storage_policy import StoragePolicy, POLICIES from swift.common.storage_policy import StoragePolicy, POLICIES
class FakeLogger(object):
def __init__(self, *args, **kwargs):
self.inc = {'return_codes.4': 0,
'return_codes.2': 0,
'objects_failures': 0,
'objects_deleted': 0,
'objects_remaining': 0,
'objects_possibly_remaining': 0,
'containers_failures': 0,
'containers_deleted': 0,
'containers_remaining': 0,
'containers_possibly_remaining': 0}
self.exp = []
def info(self, msg, *args):
self.msg = msg
def error(self, msg, *args):
self.msg = msg
def timing_since(*args, **kwargs):
pass
def getEffectiveLevel(self):
return DEBUG
def exception(self, *args):
self.exp.append(args)
def increment(self, key):
self.inc[key] += 1
class FakeBroker(object): class FakeBroker(object):
def __init__(self): def __init__(self):
self.info = {} self.info = {}
@@ -75,7 +41,7 @@ class FakeBroker(object):
class FakeAccountBroker(object): class FakeAccountBroker(object):
def __init__(self, containers, logger=None): def __init__(self, containers, logger):
self.containers = containers self.containers = containers
self.containers_yielded = [] self.containers_yielded = []
@@ -593,7 +559,7 @@ class TestReaper(unittest.TestCase):
def test_reap_account(self): def test_reap_account(self):
containers = ('c1', 'c2', 'c3', 'c4') containers = ('c1', 'c2', 'c3', 'c4')
broker = FakeAccountBroker(containers) broker = FakeAccountBroker(containers, unit.FakeLogger())
self.called_amount = 0 self.called_amount = 0
self.r = r = self.init_reaper({}, fakelogger=True) self.r = r = self.init_reaper({}, fakelogger=True)
r.start_time = time.time() r.start_time = time.time()
@@ -629,7 +595,7 @@ class TestReaper(unittest.TestCase):
self.assertEqual(len(self.r.account_ring.devs), 3) self.assertEqual(len(self.r.account_ring.devs), 3)
def test_reap_account_no_container(self): def test_reap_account_no_container(self):
broker = FakeAccountBroker(tuple()) broker = FakeAccountBroker(tuple(), unit.FakeLogger())
self.r = r = self.init_reaper({}, fakelogger=True) self.r = r = self.init_reaper({}, fakelogger=True)
self.called_amount = 0 self.called_amount = 0
r.start_time = time.time() r.start_time = time.time()
@@ -761,6 +727,7 @@ class TestReaper(unittest.TestCase):
container_reaped[0] += 1 container_reaped[0] += 1
fake_ring = FakeRing() fake_ring = FakeRing()
fake_logger = unit.FakeLogger()
with patch('swift.account.reaper.AccountBroker', with patch('swift.account.reaper.AccountBroker',
FakeAccountBroker), \ FakeAccountBroker), \
patch( patch(
@@ -769,27 +736,32 @@ class TestReaper(unittest.TestCase):
patch('swift.account.reaper.AccountReaper.reap_container', patch('swift.account.reaper.AccountReaper.reap_container',
fake_reap_container): fake_reap_container):
fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'],
fake_logger)
r.reap_account(fake_broker, 10, fake_ring.nodes, 0) r.reap_account(fake_broker, 10, fake_ring.nodes, 0)
self.assertEqual(container_reaped[0], 0) self.assertEqual(container_reaped[0], 0)
fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'],
fake_logger)
container_reaped[0] = 0 container_reaped[0] = 0
r.reap_account(fake_broker, 10, fake_ring.nodes, 1) r.reap_account(fake_broker, 10, fake_ring.nodes, 1)
self.assertEqual(container_reaped[0], 1) self.assertEqual(container_reaped[0], 1)
container_reaped[0] = 0 container_reaped[0] = 0
fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'],
fake_logger)
r.reap_account(fake_broker, 10, fake_ring.nodes, 2) r.reap_account(fake_broker, 10, fake_ring.nodes, 2)
self.assertEqual(container_reaped[0], 0) self.assertEqual(container_reaped[0], 0)
container_reaped[0] = 0 container_reaped[0] = 0
fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'],
fake_logger)
r.reap_account(fake_broker, 10, fake_ring.nodes, 3) r.reap_account(fake_broker, 10, fake_ring.nodes, 3)
self.assertEqual(container_reaped[0], 3) self.assertEqual(container_reaped[0], 3)
container_reaped[0] = 0 container_reaped[0] = 0
fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g']) fake_broker = FakeAccountBroker(['c', 'd', 'e', 'f', 'g'],
fake_logger)
r.reap_account(fake_broker, 10, fake_ring.nodes, 4) r.reap_account(fake_broker, 10, fake_ring.nodes, 4)
self.assertEqual(container_reaped[0], 1) self.assertEqual(container_reaped[0], 1)

33
test/unit/container/test_sync.py
View File

@@ -48,7 +48,7 @@ class FakeRing(object):
class FakeContainerBroker(object): class FakeContainerBroker(object):
def __init__(self, path, metadata=None, info=None, deleted=False, def __init__(self, path, metadata=None, info=None, deleted=False,
items_since=None, logger=None): items_since=None):
self.db_file = path self.db_file = path
self.db_dir = os.path.dirname(path) self.db_dir = os.path.dirname(path)
self.metadata = metadata if metadata else {} self.metadata = metadata if metadata else {}
@@ -191,7 +191,7 @@ class TestContainerSync(unittest.TestCase):
mock.patch('swift.container.sync.ContainerBroker', mock.patch('swift.container.sync.ContainerBroker',
lambda p, logger: FakeContainerBroker(p, info={ lambda p, logger: FakeContainerBroker(p, info={
'account': 'a', 'container': 'c', 'account': 'a', 'container': 'c',
'storage_policy_index': 0}, logger=logger)): 'storage_policy_index': 0})):
fake_generator.side_effect = [iter(['container.db']), fake_generator.side_effect = [iter(['container.db']),
iter(['container.db'])] iter(['container.db'])]
cs = sync.ContainerSync({}, container_ring=FakeRing()) cs = sync.ContainerSync({}, container_ring=FakeRing())
@@ -237,7 +237,7 @@ class TestContainerSync(unittest.TestCase):
mock.patch('swift.container.sync.ContainerBroker', mock.patch('swift.container.sync.ContainerBroker',
lambda p, logger: FakeContainerBroker(p, info={ lambda p, logger: FakeContainerBroker(p, info={
'account': 'a', 'container': 'c', 'account': 'a', 'container': 'c',
'storage_policy_index': 0}, logger=logger)): 'storage_policy_index': 0})):
fake_generator.side_effect = [iter(['container.db']), fake_generator.side_effect = [iter(['container.db']),
iter(['container.db'])] iter(['container.db'])]
cs = sync.ContainerSync({}, container_ring=FakeRing()) cs = sync.ContainerSync({}, container_ring=FakeRing())
@@ -339,8 +339,7 @@ class TestContainerSync(unittest.TestCase):
try: try:
sync.ContainerBroker = lambda p, logger: FakeContainerBroker( sync.ContainerBroker = lambda p, logger: FakeContainerBroker(
p, info={'account': 'a', 'container': 'c', p, info={'account': 'a', 'container': 'c',
'storage_policy_index': 0}, 'storage_policy_index': 0})
logger=logger)
cs._myips = ['127.0.0.1'] # No match cs._myips = ['127.0.0.1'] # No match
cs._myport = 1 # No match cs._myport = 1 # No match
cs.container_sync('isa.db') cs.container_sync('isa.db')
@@ -373,8 +372,7 @@ class TestContainerSync(unittest.TestCase):
try: try:
sync.ContainerBroker = lambda p, logger: FakeContainerBroker( sync.ContainerBroker = lambda p, logger: FakeContainerBroker(
p, info={'account': 'a', 'container': 'c', p, info={'account': 'a', 'container': 'c',
'storage_policy_index': 0}, deleted=False, 'storage_policy_index': 0}, deleted=False)
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
# This complete match will cause the 1 container failure since the # This complete match will cause the 1 container failure since the
@@ -384,8 +382,7 @@ class TestContainerSync(unittest.TestCase):
sync.ContainerBroker = lambda p, logger: FakeContainerBroker( sync.ContainerBroker = lambda p, logger: FakeContainerBroker(
p, info={'account': 'a', 'container': 'c', p, info={'account': 'a', 'container': 'c',
'storage_policy_index': 0}, deleted=True, 'storage_policy_index': 0}, deleted=True)
logger=logger)
# This complete match will not cause any more container failures # This complete match will not cause any more container failures
# since the broker indicates deletion # since the broker indicates deletion
cs.container_sync('isa.db') cs.container_sync('isa.db')
@@ -403,8 +400,7 @@ class TestContainerSync(unittest.TestCase):
p, info={'account': 'a', 'container': 'c', p, info={'account': 'a', 'container': 'c',
'storage_policy_index': 0, 'storage_policy_index': 0,
'x_container_sync_point1': -1, 'x_container_sync_point1': -1,
'x_container_sync_point2': -1}, 'x_container_sync_point2': -1})
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
# This complete match will be skipped since the broker's metadata # This complete match will be skipped since the broker's metadata
@@ -418,8 +414,7 @@ class TestContainerSync(unittest.TestCase):
'storage_policy_index': 0, 'storage_policy_index': 0,
'x_container_sync_point1': -1, 'x_container_sync_point1': -1,
'x_container_sync_point2': -1}, 'x_container_sync_point2': -1},
metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1)}, metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1)})
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
# This complete match will be skipped since the broker's metadata # This complete match will be skipped since the broker's metadata
@@ -433,8 +428,7 @@ class TestContainerSync(unittest.TestCase):
'storage_policy_index': 0, 'storage_policy_index': 0,
'x_container_sync_point1': -1, 'x_container_sync_point1': -1,
'x_container_sync_point2': -1}, 'x_container_sync_point2': -1},
metadata={'x-container-sync-key': ('key', 1)}, metadata={'x-container-sync-key': ('key', 1)})
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
# This complete match will be skipped since the broker's metadata # This complete match will be skipped since the broker's metadata
@@ -449,8 +443,7 @@ class TestContainerSync(unittest.TestCase):
'x_container_sync_point1': -1, 'x_container_sync_point1': -1,
'x_container_sync_point2': -1}, 'x_container_sync_point2': -1},
metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1), metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1),
'x-container-sync-key': ('key', 1)}, 'x-container-sync-key': ('key', 1)})
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
cs.allowed_sync_hosts = [] cs.allowed_sync_hosts = []
@@ -466,8 +459,7 @@ class TestContainerSync(unittest.TestCase):
'x_container_sync_point1': -1, 'x_container_sync_point1': -1,
'x_container_sync_point2': -1}, 'x_container_sync_point2': -1},
metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1), metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1),
'x-container-sync-key': ('key', 1)}, 'x-container-sync-key': ('key', 1)})
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
cs.allowed_sync_hosts = ['127.0.0.1'] cs.allowed_sync_hosts = ['127.0.0.1']
@@ -493,8 +485,7 @@ class TestContainerSync(unittest.TestCase):
'x_container_sync_point2': -1}, 'x_container_sync_point2': -1},
metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1), metadata={'x-container-sync-to': ('http://127.0.0.1/a/c', 1),
'x-container-sync-key': ('key', 1)}, 'x-container-sync-key': ('key', 1)},
items_since=['erroneous data'], items_since=['erroneous data'])
logger=logger)
cs._myips = ['10.0.0.0'] # Match cs._myips = ['10.0.0.0'] # Match
cs._myport = 1000 # Match cs._myport = 1000 # Match
cs.allowed_sync_hosts = ['127.0.0.1'] cs.allowed_sync_hosts = ['127.0.0.1']