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 "osd status" command exception if OSD not in pgmap stats #18173

Merged
merged 2 commits into from Nov 20, 2017

Conversation

gmayyyha
Copy link
Contributor

@gmayyyha gmayyyha commented Oct 9, 2017

@gmayyyha
Copy link
Contributor Author

gmayyyha commented Oct 9, 2017

@jcsp pls help review. thanks!

@gmayyyha
Copy link
Contributor Author

gmayyyha commented Oct 9, 2017

before:
before

after:
after

autoout

Copy link
Contributor

@jcsp jcsp left a comment

Choose a reason for hiding this comment

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

Apologies for delay reviewing this, please see comments.

stats = osd_stats[osd_id]

osd_table.add_row([osd_id, metadata['hostname'],
self.format_bytes(stats['kb_used'] * 1024, 5),
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nicer to have a couple of variables for the kb_used/kb_avail columns, so that the overall add_row call could just be there once instead of duplicating the whole thing

@@ -252,7 +252,7 @@ def handle_fs_status(self, cmd):
return 0, "", output

def handle_osd_status(self, cmd):
osd_table = PrettyTable(['id', 'host', 'used', 'avail', 'wr ops', 'wr data', 'rd ops', 'rd data'])
osd_table = PrettyTable(['id', 'host', 'used', 'avail', 'wr ops', 'wr data', 'rd ops', 'rd data', 'state'])
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind adding state, but please put it in a separate commit to the bug fix. Also, please output the state as something like ",".join(osd['state'])

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like this?

+----+--------+-------+-------+--------+---------+--------+---------+------------+
| id |  host  |  used | avail | wr ops | wr data | rd ops | rd data |   state    |
+----+--------+-------+-------+--------+---------+--------+---------+------------+
| 0  | ceph14 | 1092M | 9211M |    0   |     0   |    0   |     0   | exists,up  |
| 1  | ceph14 | 1092M | 9211M |    0   |     0   |    0   |     0   | exists,up  |
| 2  | ceph14 | 1092M | 9211M |    0   |     0   |    0   |     0   | exists,up  |
| 3  |        |    0  |    0  |    0   |     0   |    0   |     0   | exists,new |
+----+--------+-------+-------+--------+---------+--------+---------+------------+

kb_used = stats['kb_used'] * 1024
kb_avail = stats['kb_avail'] * 1024

state = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the right format, but we don't need to write out a for loop like this: you can do it in one line with ",".join(osd['state'])

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.

Signed-off-by: Yanhu Cao <gmayyyha@gmail.com>
@liewegas
Copy link
Member

@jcsp ping

Copy link
Contributor

@jcsp jcsp left a comment

Choose a reason for hiding this comment

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

Looks great, thank you!

@jcsp jcsp merged commit 50412f7 into ceph:master Nov 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants