-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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/dashboard: CephFS statfs REST API #52218
Conversation
For example:
|
Hi @cloudbehl have a code review |
@dparmar18 help me code review if you have time |
Would be good to have a feature tracker for this |
We have Did you check if the "additional information" mentioned contains bytes/files usage stats? |
and 'used_files'. | ||
:rtype: dict | ||
""" | ||
logger.debug("get_statfs by xattr of directory: %s", path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this debug line really needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really needed, I will follow your suggestion here.
If we decide to move with this patch then we need to verify that this works as intended, I'm unsure how dashboard tests works but definitely we need a test case here. |
|
ls_dir
statfs
|
sub_dirs = int(self.cfs.getxattr(path, 'ceph.dir.rsubdirs')) | ||
except cephfs.NoData: | ||
sub_dirs = 0 | ||
return {'used_bytes': used_bytes, 'used_files': (used_files + sub_dirs)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better names could be total_size
and total_files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
files + subdirs is not total files rights, its a mix of files and dirs. Maybe you should segregate them out and return dirs seperately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# ceph daemon /var/run/ceph/ceph-mds.rook-cephfs-b.asok dump inode 1 | grep -A 7 -w rstat
"rstat": {
"version": 198,
"rbytes": 23555211429,
"rfiles": 36,
"rsubdirs": 6,
"rsnaps": 0,
"rctime": "2023-01-11T14:47:27.131023+0000"
},
Are you sure we will use total_size
and total_files
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dashboard is to display data that is easy to understand right; these are the attributes at FS level. I don't think the naming should be same as the attribute name, for e.g. total_size
can be easily understood that it's the rbytes
we're talking about and easy to understand for all. Since it's a matter where people can have varying opinions, it would be good to have some comments from our team. I'll ask a review from FS team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it. For e.g. df -h
# df -h
Filesystem Size Used Avail Use% Mounted on
udev 252G 0 252G 0% /dev
tmpfs 51G 4.1G 47G 9% /run
/dev/nvme0n1p2 110G 18G 87G 17% /
tmpfs 252G 131M 252G 1% /dev/shm
tmpfs 5.0M 0 5.0M 0% /run/lock
tmpfs 252G 0 252G 0% /sys/fs/cgroup
Used
, is it our answer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you decide to use "used" then i think used_bytes look better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, we keep name unchanged, and segregate files and subdirs.
I have not found a test case about ls_dir, get quota and set quota, should we write a test case for these API in another PR? |
Interesting; usually it's always a good idea to test whatever code is being added. I'm surprised ls_dir and other stuff isn't tested. |
37bd667
to
1d98985
Compare
@dparmar18 updated:
|
I think just
|
at this point i think we can just mimic this #52218 (comment) and keep it as simple as |
|
@YiteGu i think just "bytes" instead of "used_bytes" would be better IMO just like what we have in rstat
|
try: | ||
bytes = int(self.cfs.getxattr(path, 'ceph.dir.rbytes')) | ||
except cephfs.NoData: | ||
bytes = 0 | ||
try: | ||
files = int(self.cfs.getxattr(path, 'ceph.dir.rfiles')) | ||
except cephfs.NoData: | ||
files = 0 | ||
try: | ||
subdirs = int(self.cfs.getxattr(path, 'ceph.dir.rsubdirs')) | ||
except cephfs.NoData: | ||
subdirs = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try: | |
bytes = int(self.cfs.getxattr(path, 'ceph.dir.rbytes')) | |
except cephfs.NoData: | |
bytes = 0 | |
try: | |
files = int(self.cfs.getxattr(path, 'ceph.dir.rfiles')) | |
except cephfs.NoData: | |
files = 0 | |
try: | |
subdirs = int(self.cfs.getxattr(path, 'ceph.dir.rsubdirs')) | |
except cephfs.NoData: | |
subdirs = 0 | |
bytes = 0 | |
files = 0 | |
subdirs = 0 | |
with suppress(cephfs.NoData): | |
bytes = int(self.cfs.getxattr(path, 'ceph.dir.rbytes')) | |
files = int(self.cfs.getxattr(path, 'ceph.dir.rfiles')) | |
subdirs = int(self.cfs.getxattr(path, 'ceph.dir.rsubdirs')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import suppress
from contextlib
(contextlib
import already exists in 6th line of this file)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bytes = 0
Warn: Redefining built-in 'bytes' (redefined-builtin), rename to rbytes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah sorry, please rename it to rbytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
It's usually good to cleanup the changes made in a test case; you add some files to the fs; i think you should also make sure to delete them upon exit |
some doc failures
|
a5adc8f
to
63d9ec4
Compare
We don't seem to have an API for delete a file yet |
63d9ec4
to
62cd4ed
Compare
you can use |
got it |
62cd4ed
to
85d0f10
Compare
self.statfs('/') | ||
|
||
self.mk_dirs('/animal') | ||
date = self.statfs('/animal') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
date
? is it a typo? should be data
or better would be stats
since the name of the test case itself is for stats
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, it is a typo.
85d0f10
to
8478c29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fs changes lgtm
Hi, @nizamial09 help me check this make check fail about run_tox_mgr_dashboard_openapi :) |
From make check log
|
you can generate a new openapi spec using the command |
dd5dd1e
to
ea38205
Compare
make check completed |
jenkins test dashboard cephadm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing all the comments @YiteGu
one more thing: you'll need to attach the tracker to the commit with a fixes message.
Fixes: <tracker-url>
Introduce statfs for the CephFS REST API controller, we can easily get statfs of the specified path by it, it returns the used bytes, used files and used subdirs. Fixes: https://tracker.ceph.com/issues/61883 Signed-off-by: Yite Gu <yitegu0@gmail.com>
ea38205
to
bcbf91d
Compare
fixes message done |
jenkins test make check |
jenkins test windows |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
thank you @YiteGu |
@YiteGu should this be backported to reef and quincy? if so can you update the tracker? https://tracker.ceph.com/issues/61883 |
|
Introduce statfs for the CephFS REST API controller, we can get statfs of the specified path by it, it returns the used bytes and the number of files used.
Fixes: https://tracker.ceph.com/issues/61883
Contribution Guidelines
To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
Checklist
Show available Jenkins commands
jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows