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

mon/OSDMonitor: issue pool application related warning #16520

Merged
merged 1 commit into from Jul 28, 2017

Conversation

Projects
None yet
4 participants
@xiexingguo
Member

xiexingguo commented Jul 24, 2017

#15763 says:
In-use pools that are not associated to an application will generate a health warning.

Ceph will unable to do this until this patch is applied.

osd/OSDMap: expose pool application warnings
by moving the code into proper place.

Signed-off-by: xie xingguo <xie.xingguo@zte.com.cn>

@xiexingguo xiexingguo added this to the luminous milestone Jul 24, 2017

@xiexingguo xiexingguo requested a review from liewegas Jul 24, 2017

if (!app_exists) {
ss << "application '" << app << "' is not enabled on pool '" << pool_name
<< "'";
return -ENOENT;

This comment has been minimized.

@liewegas

liewegas Jul 24, 2017

Member

we should return success in this case so that the command is idempotent

if (it == p.application_metadata[app].end()) {
ss << "application '" << app << "' on pool '" << pool_name
<< "' does not have key '" << key << "'";
return -ENOENT;

This comment has been minimized.

@liewegas

liewegas Jul 24, 2017

Member

return success here too

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 24, 2017

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jul 25, 2017

There is existing application metadata health check code here [1]. If it's not working after the rebase to the refactored health warnings, it should be fixed and/or removed.

[1] https://github.com/ceph/ceph/blob/master/src/mon/OSDMonitor.cc#L3942

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 25, 2017

That is actually dead code. It's generating an old-style health warning, but the block is guarded by >= luminous. get_health() isn't called after the upgrade completes. Let's remove it!

@dillaman

This comment has been minimized.

Contributor

dillaman commented Jul 25, 2017

@liewegas So it's dead code starting from https://github.com/ceph/ceph/blob/master/src/mon/OSDMonitor.cc#L3636 as well? I guess that would explain it.

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 25, 2017

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 25, 2017

see also #16568

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 25, 2017

added a commit to #16568 that removes the dead code

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 25, 2017

retest this please

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 26, 2017

retest this please

2 similar comments
@xiexingguo

This comment has been minimized.

Member

xiexingguo commented Jul 27, 2017

retest this please

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 27, 2017

retest this please

@dzafman

This comment has been minimized.

Member

dzafman commented Jul 27, 2017

@liewegas I couldn't reproduce this on my build machine, but I guess we need the same change for the run_mon in test_pidfile.

See #16635

@liewegas

This comment has been minimized.

Member

liewegas commented Jul 27, 2017

I dont' think this should affect the pidfile test; the locking error comes up before the mon is even started.

@liewegas liewegas merged commit c067ccd into ceph:master Jul 28, 2017

3 of 4 checks passed

make check make check failed
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check (arm64) make check succeeded
Details

@xiexingguo xiexingguo deleted the xiexingguo:wip-application-warn branch Jul 29, 2017

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