New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test/rgw: refactor test_multi.py for use in qa suite #14433

Merged
merged 6 commits into from Apr 19, 2017

Conversation

Projects
None yet
3 participants
@cbodley
Contributor

cbodley commented Apr 10, 2017

at a high level, this patch set:

  • creates a new rgw_multi module under src/test/rgw/
  • moves test cases from test_multi.py into rgw_multi/tests.py,
  • adds a rgw_multi/multisite.py that provides interfaces for rgw_multi/tests.py, and
  • rewrites test_multi.py to implement those interfaces in rgw_multi/multisite.py in terms of mstart.sh and related shell scripts. the usage of test_multi.py remains unchanged

the main goal is to factor out the parts that depend on shell scripts like mstart.sh, so that we can run these test cases inside of the rgw qa suite against clusters set up in teuthology

rgw_multi/multisite.py contains a SystemObject base class and derived classes for Zone, ZoneGroup, Period, Realm, and User. these classes provide wrappers for the radosgw-admin commands that manipulate them, along with the ability to parse the resulting json. rgw_multi/multisite.py also includes some interfaces for Cluster and Gateway that abstract away those details of the test environment

the first commit reverts some changes to underlying shell scripts that forced cluster names to use a zgX-cY format (from zonegroup support in #14216). the second commit goes on to update and complete the unused test-rgw-multisite.sh script

use of the logging module was inspired by @theanalyst's work in #10609

@cbodley cbodley added rgw tests labels Apr 10, 2017

@cbodley cbodley requested review from yehudasa and oritwas Apr 10, 2017

@cbodley

This comment has been minimized.

Contributor

cbodley commented Apr 10, 2017

requesting feedback on the organization of rgw_multi.py, rgw_multi_tests.py, and test_multi.py. i have a feeling that rgw_multi.py and rgw_multi_tests.py belong under ceph/qa/ somewhere, maybe ceph/qa/workunits/rgw? in that case, i'm not sure how to make ceph/src/test/rgw/test_multi.py find them without messing with PYTHONPATH

self.connection = connection
def start(self, args = None):
""" start the gateway """

This comment has been minimized.

@theanalyst

theanalyst Apr 10, 2017

Member

we could enforce this via the abc module, and the @abstractmethod decorator, but this is also fine https://docs.python.org/2/library/abc.html#abc.abstractmethod

This comment has been minimized.

@cbodley

cbodley Apr 10, 2017

Contributor

neat, i didn't know about abc. are there concrete advantages to using it in this case? it's hard for me to tell based on the docs (and lack of python background)

This comment has been minimized.

@theanalyst

theanalyst Apr 10, 2017

Member

well the difference is that this would be caught at runtime when calling the method and the abstract method would raise a TypeError when trying to instantiate an object itself if the derived class didn't implement the method. Since this is a test suite it wouldn't probably matter that much I guess

This comment has been minimized.

@cbodley

cbodley Apr 10, 2017

Contributor

gotcha. i'm really bad at python, so this is exactly the kind of feedback i need!

def meta_master_log_status(master_zone):
cmd = 'mdlog status' + master_zone.zone_args()
(mdlog_status_json, retcode) = master_zone.cluster.admin_ro(cmd)

This comment has been minimized.

@theanalyst

theanalyst Apr 10, 2017

Member

the tuple is not needed as such, but mostly a nit

This comment has been minimized.

@cbodley

cbodley Apr 10, 2017

Contributor

to clarify, you're saying i should drop the ()s?

This comment has been minimized.

@theanalyst
markers={}
i = 0
for s in mdlog_status:

This comment has been minimized.

@theanalyst

theanalyst Apr 10, 2017

Member

for i, s in enumerate(mdlog_status)

log.info('starting meta checkpoint for zone=%s', zone.name)
while True:
(num_shards, sync_status) = meta_sync_status(zone)

This comment has been minimized.

@theanalyst

theanalyst Apr 10, 2017

Member

same as earlier comment wrt to function unpacking

def gen_access_key():
return ''.join(random.SystemRandom().choice(string.ascii_uppercase + string.digits) for _ in range(16))
return ''.join(random.SystemRandom().choice(string.ascii_uppercase + string.digits) for _ in range(16))

This comment has been minimized.

@theanalyst

theanalyst Apr 10, 2017

Member

random.choice(...) would be fine I guess

@cbodley

This comment has been minimized.

Contributor

cbodley commented Apr 12, 2017

pushed an update with a few changes:

  • new files are now in a module located in src/test/rgw/rgw_multi/
  • removed the Log interface, because test_multi.py and teuthology are both using the logging module anyway
  • Cluster.admin() and related functions take commands/arguments as a list of strings
@cbodley

This comment has been minimized.

Contributor

cbodley commented Apr 17, 2017

pushed an update that uses the abc module to enforce abstract methods

i've made enough progress on the rgw_multisite teuthology task that i don't expect it to require any more changes to this rgw_multi module

cbodley added some commits Apr 6, 2017

test/rgw: test_multi.py uses cluster_id for calls
this makes the underlying scripts more flexible, because they don't
depend on having a cluster name in the zgX-cY format

Signed-off-by: Casey Bodley <cbodley@redhat.com>
test/rgw: update and fixes for test-rgw-multisite.sh
the script was incomplete and unused, but it seems useful in itself
to bring up a simple multisite cluster without having to go through
test_multi.py. it's also a good test for functions in the other
test-rgw-*.sh scripts

Signed-off-by: Casey Bodley <cbodley@redhat.com>
test/rgw: add skeleton rgw_multi module
Signed-off-by: Casey Bodley <cbodley@redhat.com>
test/rgw: add rgw_multi/multisite.py
Signed-off-by: Casey Bodley <cbodley@redhat.com>
test/rgw: add tests in rgw_multi/tests.py based on multisite.py
Signed-off-by: Casey Bodley <cbodley@redhat.com>
test/rgw: rewrite test_multi.py in terms of rgw_multi
logging changes inspired by Abhishek Lekshmanan <abhishek@suse.com>

Signed-off-by: Casey Bodley <cbodley@redhat.com>
@yehudasa

This comment has been minimized.

Member

yehudasa commented Apr 18, 2017

@cbodley the abc change doesn't work in python2, see yehudasa@fccddad for a fix

@cbodley

This comment has been minimized.

Contributor

cbodley commented Apr 18, 2017

thanks @yehudasa, i had just pushed a fix for that

@yehudasa

lgtm

@yehudasa yehudasa merged commit 205a39c into ceph:master Apr 19, 2017

2 of 3 checks passed

default Build finished.
Details
Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details

@cbodley cbodley deleted the cbodley:wip-rgw-multi-py branch Apr 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment