Skip to content
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

mgr: fix python module teardown & add tests #14232

Merged
merged 5 commits into from
Apr 21, 2017
Merged

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Mar 29, 2017

Depends on #14220 for tests to pass.

@tchaikov
Copy link
Contributor

will review this PR tomorrow.

@@ -686,9 +683,6 @@ def __init__(self, ctx):
super(LocalMgrCluster, self).__init__(ctx)

self.mgr_ids = ctx.daemons.daemons['mgr'].keys()
if not self.mgr_ids:
raise RuntimeError("No manager daemonss found in ceph.conf!")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

daemons.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was being removed anyway


for suite, case in enumerate_methods(overall_suite):
max_required_mds = max(max_required_mds,
getattr(case, "MDSS_REQUIRED", 0))
max_required_clients = max(max_required_clients,
getattr(case, "CLIENTS_REQUIRED", 0))
max_required_mgr = max(max_required_mgr,
getattr(case, "REQUIRE_MGRS", 0))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use "MGRS_REQUIRED" for the sake of consistency?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -906,7 +903,7 @@ def exec_test():
vstart_env["FS"] = "0"
vstart_env["MDS"] = max_required_mds.__str__()
vstart_env["OSD"] = "1"
vstart_env["MGR"] = "1"
vstart_env["MGR"] = max(max_required_mgr, 1).__str__()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just out of curiosity, why don't use

str(max(max_required_mgr, 1))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason, they're completely equivalent afaik

@tchaikov
Copy link
Contributor

@jcsp lgtm modulo some nits.

@yuriw
Copy link
Contributor

yuriw commented Apr 14, 2017

@jcsp pls rebase

remote: Counting objects: 21, done.
remote: Compressing objects: 100% (3/3), done.
remote: Total 21 (delta 15), reused 14 (delta 14), pack-reused 4
Unpacking objects: 100% (21/21), done.
From https://github.com/jcsp/ceph

  • branch wip-19412 -> FETCH_HEAD
    Auto-merging src/mgr/PyModules.cc
    CONFLICT (content): Merge conflict in src/mgr/PyModules.cc
    Auto-merging src/mgr/MgrPyModule.cc
    Auto-merging qa/tasks/vstart_runner.py
    Automatic merge failed; fix conflicts and then commit the result.

@liewegas
Copy link
Member

those might be conflicts with #14507 btw

@jcsp
Copy link
Contributor Author

jcsp commented Apr 15, 2017

Pushed an update (this did not need rebase, as sage says it is just conflicting with the other outstanding PR)

@yuriw
Copy link
Contributor

yuriw commented Apr 17, 2017

Will try to rebuild again

@yuriw
Copy link
Contributor

yuriw commented Apr 17, 2017

@jcsp still see the same conflict

@jcsp
Copy link
Contributor Author

jcsp commented Apr 18, 2017

@yuriw yes, you will see that until one or other of the PRs merges and the one one gets rebased.

@tchaikov
Copy link
Contributor

@jcsp could you rebase your changes?

John Spray added 5 commits April 20, 2017 14:59
Fixes: http://tracker.ceph.com/issues/19412
Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
Reproducers for recent fixes:
http://tracker.ceph.com/issues/19407
http://tracker.ceph.com/issues/19258

Signed-off-by: John Spray <john.spray@redhat.com>
Signed-off-by: John Spray <john.spray@redhat.com>
@jcsp
Copy link
Contributor Author

jcsp commented Apr 20, 2017

@tchaikov done!

@tchaikov
Copy link
Contributor

@tchaikov tchaikov merged commit c237e7e into ceph:master Apr 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants