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/prometheus: fix PG state names #21288

Merged
merged 2 commits into from Apr 11, 2018

Conversation

Projects
None yet
4 participants
@jcsp
Copy link
Contributor

commented Apr 8, 2018

...and add a couple of common missing ones.

Fixes: https://tracker.ceph.com/issues/23584
Signed-off-by: John Spray john.spray@redhat.com

@jcsp jcsp added bug fix mgr labels Apr 8, 2018

@jcsp jcsp requested a review from jan--f Apr 8, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2018

@jan--f this doesn't add all of the possible PG state strings from osd_types.cc -- I can't remember if it was intentional to have this be a subset of all the more obscure states, or not?

@k0ste

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2018

@jcsp, proper pg_states. From osd_types and special deep.
*_wait and other should be with underscores, not dashes.

PG_STATES = ['creating', 'active', 'clean', 'down', 'recovery_unfound',        
             'backfill_unfound', 'scrubbing', 'degraded', 'inconsistent',      
             'peering', 'repair', 'recovering', 'backfill_wait', 'incomplete', 
             'stale', 'remapped', 'deep', 'backfilling',                       
             'backfill_toofull', 'recovery_wait', 'undersized', 'activating',  
             'peered', 'snaptrim', 'snaptrim_wait', 'recovery_toofull',        
             'snaptrim_error', 'forced_recovery', 'forced_backfill']           
@jcsp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2018

Hmm, I had assume the - vs _ thing was getting handled somewhere else but now I can't see it, were all the states with - in just broken the whole time?

@k0ste

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2018

all the states with - in just broken the whole time?

yep.

@jan--f

jan--f approved these changes Apr 9, 2018

Copy link
Member

left a comment

Sorry I had this on my list as well but then got distracted. I don't see a reason not to expose all possible pg states, but I also don't have strong feelings about it.

@jan--f

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Regarding the broken states with dashes: I got the original list of pg states from the docs, so if this is a legitimate bug, the docs need a fix as well. Happy to push a PR if its valid to just list the states from osd_types.cc in the doc page.

@jcsp jcsp force-pushed the jcsp:wip-23584 branch from 46e1644 to 007fc2a Apr 9, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

OK -- I've just gone ahead and updated this PR to match osd_types.cc

Probably ambiguous if the docs are to be considered buggy, as those are "sort of" human readable names, but it probably is simpler to update the docs with underscores to exactly match the output of ceph status and prometheus exactly if you have time to do that.

@jcsp jcsp added the needs-qa label Apr 9, 2018

@k0ste

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

@jcsp, please, don't ignore my pg_states list. As mentioned before state deep_scrub is not exists, instead this - deep. I don't know why.

John Spray added some commits Apr 8, 2018

John Spray
mgr/prometheus: fix and complete PG state names
...and reformat into a flat list in the same
order as found in osd_types.cc so that it's
easy to cross check.

Fixes: https://tracker.ceph.com/issues/23584
Signed-off-by: John Spray <john.spray@redhat.com>
John Spray
osd: make PG "deep" state name consistent
Previously the state-to-string path called
deep scrubbing "deep", but the string-to-state
path called it "deep_scrub".  Confusing!  Also
meant that the "pg ls" help output was wrong
because it uses the state-to-string path.

Signed-off-by: John Spray <john.spray@redhat.com>

@jcsp jcsp force-pushed the jcsp:wip-23584 branch from 007fc2a to 48acc6c Apr 9, 2018

@jcsp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

@k0ste good catch. I wasn't so much ignoring your list, as I was using osd_types.cc to make sure we got everything. However, it looks like there's actually an inconsistency (just for deep vs. deep_scrub) between pg_string_state and pg_state_string (I was copying from pg_string_state).

I've updated this now to make everything use deep instead of deep_scrub (including updating osd_types.cc to be consistent).

@k0ste

This comment has been minimized.

Copy link
Contributor

commented Apr 9, 2018

@jcsp, nice.

@tchaikov tchaikov merged commit 28e543f into ceph:master Apr 11, 2018

4 of 5 checks passed

make check make check failed
Details
Docs: build check OK - docs built
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.