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

mds: fix mantle script to not fail for last rank #14704

Merged
merged 2 commits into from May 11, 2017

Conversation

Projects
None yet
4 participants
@batrick
Member

batrick commented Apr 21, 2017

@batrick

This comment has been minimized.

Member

batrick commented Apr 21, 2017

@michaelsevilla

I think the only major issue is opening all the Lua libraries. Other than that, LGTM.

end
end
-- Shed load when you have load and your neighbor doesn't
function when()
local function when()
if not mds[whoami+1] then return false end -- i'm the last rank

This comment has been minimized.

@michaelsevilla

michaelsevilla Apr 21, 2017

Contributor

Thanks for fixing this. Might we spit out a message saying why it failed or would this spam the log? That would align with the description in the documentation, at least.

This comment has been minimized.

@michaelsevilla

michaelsevilla Apr 21, 2017

Contributor

Also, this makes me think that if C++ land fails to execute Lua code, we shouldn't fall back to the default CephFS balancer because it will start migrating load with that original policy -- it will look almost random to someone who was trying to use Mantle. Thoughts? I started implementing it but it messed up test cases in /qa so I need a little more time.

This comment has been minimized.

@batrick

batrick Apr 21, 2017

Member

I'll throw a message in at a high debug level for that and fix the documentation.

I think removing the fallback makes sense. Can you add an issue for that to the tracker Michael?

mds[i]["load"] = mds[i]["all.meta_load"]
BAL_LOG(0, s.."> load="..mds[i]["load"])
mds.load = mds["all.meta_load"]
BAL_LOG(5, s.."> load="..mds.load)

This comment has been minimized.

@michaelsevilla

michaelsevilla Apr 21, 2017

Contributor

Why do you bother changing to log level 5? Does 2, 5, 10 have a special meaning (I noticed those were used in the most in C++ land)?

This comment has been minimized.

@batrick

batrick Apr 21, 2017

Member

0-2 is pretty verbose and these messages are pretty spammy. I think 5 or even 10 is appropriate here.

}
/* balancer policies can use basic Lua functions */
luaL_openlibs(L);

This comment has been minimized.

@michaelsevilla

michaelsevilla Apr 21, 2017

Contributor

Do we want to open all libraries? I think at one of the CDMs I was told that that could be dangerous (we don't want the OS modules, for example). lua_openbase() might be safer?

This comment has been minimized.

@batrick

batrick Apr 21, 2017

Member

Right, that's an oops. I'll change it back.

batrick added some commits Oct 28, 2016

mds: misc mantle cleanup
In particular, logging now works correctly. DOUT_COND was obsoleted in
f41887e.

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
mds: fix mantle script to not fail for last rank
Fixes: http://tracker.ceph.com/issues/19589

Signed-off-by: Patrick Donnelly <pdonnell@redhat.com>
@michaelsevilla

LGTM. Thanks.

@michaelsevilla

This comment has been minimized.

Contributor

michaelsevilla commented Apr 21, 2017

Good to go. Do I have to do something to get the "Review Required" message to go away?

@batrick

This comment has been minimized.

Member

batrick commented Apr 22, 2017

No, but thanks for your review though! I think another Ceph member has to review it to be approved for merging.

@tchaikov tchaikov requested a review from jcsp May 2, 2017

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented May 2, 2017

@jcsp could you take a look at your convenience when you are back ?

@batrick

This comment has been minimized.

@jcsp

jcsp approved these changes May 11, 2017

I'm happy if Michael is happy

@jcsp jcsp merged commit 1b1fc05 into ceph:master May 11, 2017

3 checks passed

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

@batrick batrick deleted the batrick:i19589 branch May 11, 2017

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