HomeMesosphereNo notifications. 4 unresolved issues.

Add SI test for marathon's backup and restore
ClosedAll Users

Authored by zen-dog on May 10 2017, 8:21 PM.

Details

Summary

The test will trigger backup-and-restore by calling DELETE /v2/leader?backup=... and check if the
operation could run successfully e.g. backup file exists and is valid, marathon reelection was successful etc.

Related: MARATHON-7289

Test Plan

si

Diff Detail

Repository
rMARATHON marathon
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
zen-dog created this revision.May 10 2017, 8:21 PM
jenkins requested changes to this revision.May 10 2017, 8:37 PM
This revision now requires changes to proceed.May 10 2017, 8:37 PM
This revision is now accepted and ready to land.May 10 2017, 9:19 PM
aquamatthias accepted this revision.May 15 2017, 9:26 AM

Thanks. Added some suggestions.

tests/system/test_marathon_root.py
235

Perhaps also test, if this is the same task id?

238

An empty tar archive would be valid as well - so this test is not strong enough.
I would recommend something like:

tar tf {} | wc -l

and check the resulting entries in the jar.

kensipe requested changes to this revision.May 15 2017, 5:12 PM
kensipe added inline comments.
.python-version
1 ↗(On Diff #3143)

can we not fight on this ?

lets consolidate on a version if you are using pyenv. I've upgraded mine. mainly because dcos-cli requires 3.5.

tests/system/test_marathon_root.py
230

I would prefer if we add this functionality to marathon.py of the dcos-cli project.
btw... it is likely if you / we do... tamar will want the cli to be extended to include this functionality... which is probably a good thing.

if you like... I can do it.

235

totally agree on the check for task id

238

I'm not understanding this.. does the backup create a tar on the marathon leader? I didn't look at the code, but this looks like a good assumption based on the code .

running this command on the "master" is a bad idea and will likely result in flaky tests results (sometimes succeeding and sometimes not). Your assumption is good on a single master cluster. However on a multi-master it is flaky.

You need to make sure to execute this command on the node that tar was created on. is it the previous leader? It seems likely you will need the mom_ip. I just recently added in the ability to get marathon_leader_ip.

This revision now requires changes to proceed.May 15 2017, 5:12 PM
kensipe added inline comments.May 15 2017, 5:13 PM
tests/system/test_marathon_root.py
204

not an issue... but all previous approaches of creating "random" chars have been done with uuid

app_id = uuid.uuid4().hex

https://github.com/mesosphere/marathon/blob/master/tests/system/marathon_common_tests.py#L28

kensipe added inline comments.May 15 2017, 9:46 PM
tests/system/test_marathon_root.py
201

this test needs to skip if the marathon is less than X where X is the version that defines this behavior.

kensipe added inline comments.May 15 2017, 10:22 PM
tests/system/test_marathon_root.py
201

also @masters(1) doesn't skip this test if there are more than 1 master... if fact @master(1) is pretty useless. it says there must be 1 master... which is always true if there is a working cluster.

jeschkies added inline comments.May 16 2017, 5:46 PM
tests/system/test_marathon_root.py
204

+1

I like the prefix though :).

230

Out of curiosity: Is there a JIRA for creating a Python Marathon client? It feels we are going this direction.

zen-dog updated this revision to Diff 3238.May 16 2017, 7:32 PM
zen-dog marked 5 inline comments as done.

Implemented review feedback and added a check for non empty backup file

All critique points addressed.

tests/system/test_marathon_root.py
201

That's a good catch. Thx!

204

Uuids are good as app_ids but feel less useful as filenames. Actually we don't need random filename at all - it was just for testing (on the same cluster). I'll remove.

230

I agree - v2/leader endpoint is not yet represented in the cli. But I don't think this is in scope of 1.10 for me.

238

That's why I (incorrectly) used @masters(1) annotation. Fixed.

jenkins requested changes to this revision.May 16 2017, 7:38 PM
This revision now requires changes to proceed.May 16 2017, 7:38 PM
kensipe accepted this revision.May 17 2017, 6:14 PM
This revision was automatically updated to reflect the committed changes.