mds: add a command to dump directory information by zhsgao · Pull Request #49756 · ceph/ceph

zhsgao · 2023-01-16T10:02:16Z

Signed-off-by: Zhansong Gao zhsgao@hotmail.com

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

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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

src/mds/MDSRank.cc

vshankar

@zhsgao Could you add a test case for this? qa/tasks/cephfs/test_fragment.py

zhsgao · 2023-01-25T07:47:14Z

@zhsgao Could you add a test case for this? qa/tasks/cephfs/test_fragment.py

The tests for dump inode is in test_misc.py and tests for dump tree is in test_dump_tree.py, maybe we can put tests for dump dir in one of these two files?

vshankar · 2023-01-25T07:55:05Z

@zhsgao Could you add a test case for this? qa/tasks/cephfs/test_fragment.py

The tests for dump inode is in test_misc.py and tests for dump tree is in test_dump_tree.py, maybe we can put tests for dump dir in one of these two files?

Sounds good.

kotreshhr · 2023-01-25T07:40:43Z

src/mds/MDCache.cc

  f->open_object_section("resolve_status");
  f->dump_stream("resolve_gather") << resolve_gather;
-  f->dump_stream("resolve_ack_gather") << resolve_gather;
+  f->dump_stream("resolve_ack_gather") << resolve_ack_gather;


Advisable to make this a separate commit ?

Advisable to make this a separate commit ?

I have separated it into a new PR #49882.

zhsgao · 2023-01-26T05:23:47Z

@zhsgao Could you add a test case for this? qa/tasks/cephfs/test_fragment.py

The tests for dump inode is in test_misc.py and tests for dump tree is in test_dump_tree.py, maybe we can put tests for dump dir in one of these two files?

Sounds good.

OK! I have done it.

dparmar18 · 2023-01-27T17:17:58Z

@zhsgao can you create a separate commit for the test? It would be less cluttered that way

zhsgao · 2023-01-29T03:27:10Z

@zhsgao can you create a separate commit for the test? It would be less cluttered that way

OK! I have done it.

dparmar18 · 2023-01-30T08:42:40Z

@zhsgao can you create a separate commit for the test? It would be less cluttered that way

OK! I have done it.

Thanks, although a slight change can be done for the commit title - qa: add test for 'dump dir'

Also, this might also need to be documented somewhere, maybe in PendingReleaseNotes or in doc/

zhsgao · 2023-01-30T11:43:48Z

@zhsgao can you create a separate commit for the test? It would be less cluttered that way

OK! I have done it.

Thanks, although a slight change can be done for the commit title - qa: add test for 'dump dir'

Also, this might also need to be documented somewhere, maybe in PendingReleaseNotes or in doc/

I have modified the commit title.
I tried to search for similar commands like dump inode or dump tree in doc/ but found nothing, so I have no idea how to add documentation for dump dir.

dparmar18 · 2023-01-30T12:44:43Z

@zhsgao can you create a separate commit for the test? It would be less cluttered that way

OK! I have done it.

Thanks, although a slight change can be done for the commit title - qa: add test for 'dump dir'
Also, this might also need to be documented somewhere, maybe in PendingReleaseNotes or in doc/

I have modified the commit title. I tried to search for similar commands like dump inode or dump tree in doc/ but found nothing, so I have no idea how to add documentation for dump dir.

yeah, those are asok commands, and a lot of them have not been documented anywhere. maybe we need a place in docs or somewhere else to dump info about these commands. @vshankar what do you think?

vshankar · 2023-01-30T16:54:18Z

src/mds/MDCache.cc


+bool MDCache::dump_dir(Formatter *f, CDir *dir) {
+  f->open_object_section("dir");
+  dir->dump(f);


This, by default, does not dump dentry items. Should that be made optional?

This, by default, does not dump dentry items. Should that be made optional?

Yes, I think it's a good idea and I have added an option for it. Thanks!

vshankar · 2023-01-31T05:10:25Z

@zhsgao can you create a separate commit for the test? It would be less cluttered that way

OK! I have done it.

Thanks, although a slight change can be done for the commit title - qa: add test for 'dump dir'
Also, this might also need to be documented somewhere, maybe in PendingReleaseNotes or in doc/

I have modified the commit title. I tried to search for similar commands like dump inode or dump tree in doc/ but found nothing, so I have no idea how to add documentation for dump dir.

yeah, those are asok commands, and a lot of them have not been documented anywhere. maybe we need a place in docs or somewhere else to dump info about these commands. @vshankar what do you think?

Let's do doc changes in a separate PR.

kotreshhr

LGTM

vshankar

LGTM

qa/tasks/cephfs/test_misc.py

vshankar · 2023-02-01T08:56:50Z

qa/tasks/cephfs/test_misc.py

+    def test_dump_dir(self):
+        self.mount_a.run_shell(["mkdir", "-p", "foo/bar"])
+        dirs = self.fs.mds_asok(['dump', 'dir', '/foo'])
+        assert type(dirs) is list


minor nit: use self.assertTrue()

minor nit: use self.assertTrue()

Thanks! I have modified them.

vshankar

Otherwise LGTM.

dparmar18 · 2023-02-01T09:10:01Z

src/mds/MDCache.cc

+bool MDCache::dump_dir(Formatter *f, CDir *dir, bool dentry_dump) {
+  f->open_object_section("dir");
+  dir->dump(f, dentry_dump ? CDir::DUMP_ALL : CDir::DUMP_DEFAULT);
+  f->close_section();
+  return true;
+}


code looks good, just a question: is there any reason for this function to have bool return type? I think void would do too

code looks good, just a question: is there any reason for this function to have bool return type? I think void would do too

I have modified it. At first I referred to dump_inode and the type of its return value is boolean.

dparmar18 · 2023-02-01T09:12:43Z

qa/tasks/cephfs/test_misc.py

+        dirs = self.fs.mds_asok(['dump', 'dir', '/foo'])
+        assert type(dirs) is list
+        for dir in dirs:
+            assert dir['path'] == "/foo"


as venky mentioned, using self.assert* would be better, do try to replace all assertions if possible, for e.g you can use self.assertEquals() here

as venky mentioned, using self.assert* would be better, do try to replace all assertions if possible, for e.g you can use self.assertEquals() here

OK! I have replaced all assertions in this file.

thanks! but i guess you got me a little bit wrong, by all i meant those in your test case. However if you feel you want to replace them as well, maybe that can be done separately in another commit, but i'm not very sure about it.

Signed-off-by: Zhansong Gao <zhsgao@hotmail.com>

dparmar18

just some minor touchups in test case file, else LGTM

vshankar · 2023-02-27T08:59:04Z

Test ran successfully with this change - https://pulpito.ceph.com/vshankar-2023-02-24_02:11:45-fs-wip-vshankar-testing-20230222.025426-testing-default-smithi/

Failures are not related to this PR.

vshankar · 2023-02-27T08:59:13Z

jenkins retest this please

batrick · 2023-02-28T16:17:05Z

jenkins test make check

batrick · 2023-02-28T16:17:13Z

jenkins test make check arm64

src/mds/MDSRank.cc

* refs/pull/49756/head: qa: add test for 'dump dir' mds: add a command to dump directory information Reviewed-by: Dhairya Parmar <dparmar@redhat.com> Reviewed-by: Kotresh Hiremath Ravishankar <khiremat@redhat.com> Reviewed-by: Venky Shankar <vshankar@redhat.com> Reviewed-by: Rishabh Dave <ridave@redhat.com>

trociny · 2023-10-04T07:28:35Z

@vshankar @zhsgao It looks like a very useful command, but it seems like there are no plans to backport it? Right now even reef misses it. Do you think it would be a good idea to create backport tickets (and PRs) for it? I could do it. I would like to have backports down to pacific unless it relies on a new functionality.

vshankar · 2023-10-04T08:20:40Z

@vshankar @zhsgao It looks like a very useful command, but it seems like there are no plans to backport it? Right now even reef misses it. Do you think it would be a good idea to create backport tickets (and PRs) for it? I could do it. I would like to have backports down to pacific unless it relies on a new functionality.

Sure thing - please do the needful @trociny. I think we should also address https://github.com/ceph/ceph/pull/49756/files#r1120348923 and then backport both changes. I'll create a tracker.

github-actions bot added the cephfs Ceph File System label Jan 16, 2023

neesingh-rh reviewed Jan 24, 2023

View reviewed changes

src/mds/MDSRank.cc Show resolved Hide resolved

vshankar reviewed Jan 25, 2023

View reviewed changes

kotreshhr reviewed Jan 25, 2023

View reviewed changes

zhsgao force-pushed the mds_dump_dir branch from 8332f52 to c5ef6da Compare January 26, 2023 05:02

github-actions bot added the tests label Jan 26, 2023

dparmar18 mentioned this pull request Jan 27, 2023

mds: fix bug of dump_resolve_status #49882

Merged

15 tasks

zhsgao force-pushed the mds_dump_dir branch from c5ef6da to a0248a3 Compare January 29, 2023 03:25

zhsgao force-pushed the mds_dump_dir branch from a0248a3 to 30e5e86 Compare January 30, 2023 11:41

rishabh-d-dave added the needs-review label Jan 30, 2023

vshankar reviewed Jan 30, 2023

View reviewed changes

zhsgao force-pushed the mds_dump_dir branch from 30e5e86 to ce8d3ad Compare January 31, 2023 05:27

kotreshhr approved these changes Jan 31, 2023

View reviewed changes

vshankar reviewed Jan 31, 2023

View reviewed changes

qa/tasks/cephfs/test_misc.py Show resolved Hide resolved

zhsgao force-pushed the mds_dump_dir branch from ce8d3ad to 2a73307 Compare February 1, 2023 04:24

vshankar reviewed Feb 1, 2023

View reviewed changes

vshankar approved these changes Feb 1, 2023

View reviewed changes

vshankar added the wip-vshankar-testing label Feb 1, 2023

dparmar18 reviewed Feb 1, 2023

View reviewed changes

zhsgao added 2 commits February 1, 2023 18:28

mds: add a command to dump directory information

42492d5

Signed-off-by: Zhansong Gao <zhsgao@hotmail.com>

qa: add test for 'dump dir'

d4438d3

Signed-off-by: Zhansong Gao <zhsgao@hotmail.com>

zhsgao force-pushed the mds_dump_dir branch from 2a73307 to d4438d3 Compare February 1, 2023 10:53

dparmar18 approved these changes Feb 1, 2023

View reviewed changes

rishabh-d-dave approved these changes Feb 7, 2023

View reviewed changes

batrick reviewed Feb 28, 2023

View reviewed changes

src/mds/MDSRank.cc Show resolved Hide resolved

vshankar merged commit 7f65724 into ceph:main Mar 2, 2023

vshankar removed the wip-vshankar-testing label Mar 2, 2023

joscollin mentioned this pull request Mar 15, 2024

reef: mds: add a command to dump directory information #55987

Merged

14 tasks

Conversation

zhsgao commented Jan 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

zhsgao commented Jan 25, 2023

Uh oh!

vshankar commented Jan 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhsgao commented Jan 26, 2023

Uh oh!

dparmar18 commented Jan 27, 2023

Uh oh!

zhsgao commented Jan 29, 2023

Uh oh!

dparmar18 commented Jan 30, 2023

Uh oh!

zhsgao commented Jan 30, 2023

Uh oh!

dparmar18 commented Jan 30, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar commented Jan 31, 2023

Uh oh!

kotreshhr left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dparmar18 left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Feb 27, 2023

Uh oh!

vshankar commented Feb 27, 2023

Uh oh!

batrick commented Feb 28, 2023

Uh oh!

batrick commented Feb 28, 2023

Uh oh!

Uh oh!

trociny commented Oct 4, 2023

Uh oh!

vshankar commented Oct 4, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

zhsgao commented Jan 16, 2023 •

edited

Loading