mgr/nfs: disallow non-existent paths when creating export by dparmar18 · Pull Request #49460 · ceph/ceph

dparmar18 · 2022-12-15T11:25:32Z

Fixes:

https://tracker.ceph.com/issues/58228
https://tracker.ceph.com/issues/58744
https://tracker.ceph.com/issues/58758
helper function _check_nfs_cluster_status() in test_nfs.py

Signed-off-by: Dhairya Parmar dparmar@redhat.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/pybind/mgr/nfs/export.py

dparmar18 · 2022-12-20T11:17:40Z

jenkins test make check

dparmar18 · 2022-12-20T11:17:50Z

jenkins test api

github-actions · 2022-12-22T18:36:20Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

dparmar18 · 2022-12-23T11:37:03Z

no changes made, resolved a conflicted and rebased.

src/pybind/cephfs/cephfs.pyx

dparmar18 · 2023-03-27T06:22:12Z

@dparmar18 Seeing your latest commits it looks like integrating mocking for the added changes is on-going (as we discussed in IRC). Let me know if you need help.

As we discussed it's a test that is causing the issue, im figuring out how to fix it

dparmar18 · 2023-03-27T13:29:08Z

@phlogistonjohn Hi there. Seeing your contributions to the mgr/nfs code, pinging you case you have some insights on the issue this PR is facing.

Summary of the code before moving to the issue: This PR uses mgr_util.py's CephfsClient and open_filesystem() in export.py to open a fs handle to check if the cephfs path is valid or not before creating cephfs exports.

Issues are not faced in teuthology, it's all green but rather make check tests never complete, runs till timeout i.e. 3 hours. I had to run tox tests locally to find out the actual issue that at the time of exit, the run would halt and need manual interruption (CTRL + C) else it would wait till infinity.

At first the issue seemed to be with fs not monkey patched in test_nfs.py and thus leaded to Exception getting caught and raised which I fixed by wrapping the code that checks for cephfs path into a helper function under utils.py and mocking it as true in test_nfs.py (52c5812). Once this got sorted the tests passed but the halting issue still persisted, then i pushed a commit a9cb9e5 that comments a test case(how did i figure this out? trial and error with every test case) in test_nfs.py and also monkey patches CephfsClient but very vaguely, i mean we would need all the things to be patched but this was just to see if bypassing it makes it work or not and to my surprise it went fine and make check passed. So there's a lot weird going on. @vshankar and I agree to completely monkey patch CephfsClient which should this issue however we thought to ping and ask you if you have any thoughts on this. Especially the mystery of the test case that is commented out.

phlogistonjohn · 2023-03-27T15:31:17Z

@phlogistonjohn Hi there. Seeing your contributions to the mgr/nfs code, pinging you case you have some insights on the issue this PR is facing.

Summary of the code before moving to the issue: This PR uses mgr_util.py's CephfsClient and open_filesystem() in export.py to open a fs handle to check if the cephfs path is valid or not before creating cephfs exports.

Issues are not faced in teuthology, it's all green but rather make check tests never complete, runs till timeout i.e. 3 hours. I had to run tox tests locally to find out the actual issue that at the time of exit, the run would halt and need manual interruption (CTRL + C) else it would wait till infinity.

At first the issue seemed to be with fs not monkey patched in test_nfs.py and thus leaded to Exception getting caught and raised which I fixed by wrapping the code that checks for cephfs path into a helper function under utils.py and mocking it as true in test_nfs.py (52c5812). Once this got sorted the tests passed but the halting issue still persisted, then i pushed a commit a9cb9e5 that comments a test case(how did i figure this out? trial and error with every test case) in test_nfs.py and also monkey patches CephfsClient but very vaguely, i mean we would need all the things to be patched but this was just to see if bypassing it makes it work or not and to my surprise it went fine and make check passed. So there's a lot weird going on. @vshankar and I agree to completely monkey patch CephfsClient which should this issue however we thought to ping and ask you if you have any thoughts on this. Especially the mystery of the test case that is commented out.

I haven't dug into the behavior of CephfsClient much myself but it seems that if adding:

                mock.patch('nfs.export.check_cephfs_path',
                           return_value=(0, "cephfs path is valid")), \

did not fix the hang but adding:

                mock.patch('nfs.export.CephfsClient', return_value=mock.MagicMock()), \

did I bet the fact that the CephfsClient is created and assigned in ExportMgr.init is an issue:

self.fs_client = CephfsClient(mgr)

IOW, if there's state that needs to connect to some "real" ceph process this could interfere with the unit tests.

I really like that you moved the path check to a function anyway, I think that is a good change regardless of the test hang. But I do think it would be better to only mock check_cephfs_path if possible. Could you experiment with moving the instantiation of the CephfsClient into check_cephfs_path instead of in __init__?

I'm not sure about the other aspect. I would probably need to experiment with it locally myself. If you think that would help please just say so, otherwise I'll let you try the above first.

Once you have things working I'll be happy to review this as well. I will make one small style suggestion today.

src/pybind/mgr/nfs/export.py

src/pybind/mgr/nfs/utils.py

this helper instantiates CephfsClient, however this was initially planned in ExportMgr class in export.py but due to make check failure where main python thread experienced a dead lock which after several efforts pointed at instantiation of CephfsClient in ExportMgr was problematic, it was decided in order to achieve singleton behavior, func has been added inside this helper func that restricts instantiation using functool's lru_cache. Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

src/pybind/mgr/nfs/utils.py

dparmar18 · 2023-03-30T11:30:52Z

small run went fine http://pulpito.front.sepia.ceph.com/dparmar-2023-03-30_10:08:17-orch:cephadm-wip-58228-distro-default-smithi/

scheduling complete run now

dparmar18 · 2023-03-30T12:27:00Z

full run passed http://pulpito.front.sepia.ceph.com/dparmar-2023-03-30_11:33:08-orch:cephadm-wip-58228-distro-default-smithi/

Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

Fixes: https://tracker.ceph.com/issues/58228 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

Also adds a function _nfs_complete_cmd() that returns process obj so that stdout/stderr can be used for evaluation(_nfs_cmd() uses raw_cluster_cmd() that returns just stdout and it became difficult to time cluster creation errors in _test_create_cluster()). It takes sometime to update the cluster data, therefore running the command set (check nfs server status -> nfs cluster create test -> check cluster status) in a loop (max six iteration with sleep of 5 secs at each iteration) fixes the issue. Fixes: https://tracker.ceph.com/issues/58744 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

…terid' Fixes: https://tracker.ceph.com/issues/58758 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

Comment in the code says to wait for two minutes as cluster creation takes time but actually it's waiting for thirteen minutes, it's not required to wait this long, i think a minute here is more than enough, also switched to using safe_while(). Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

_get_port_ip_info() fails to fetch port and ip due to empty 'backend' key: 2023-02-24T20:49:09.084 DEBUG:teuthology.orchestra.run.smithi042:> sudo adjust-ulimits ceph-coverage /home/ubuntu/cephtest/archive/coverage timeout 120 ceph --cluster ceph nfs cluster info test 2023-02-24T20:49:09.471 INFO:teuthology.orchestra.run.smithi042.stdout:{ 2023-02-24T20:49:09.472 INFO:teuthology.orchestra.run.smithi042.stdout: "test": { 2023-02-24T20:49:09.472 INFO:teuthology.orchestra.run.smithi042.stdout: "backend": [], 2023-02-24T20:49:09.472 INFO:teuthology.orchestra.run.smithi042.stdout: "virtual_ip": null 2023-02-24T20:49:09.472 INFO:teuthology.orchestra.run.smithi042.stdout: } 2023-02-24T20:49:09.472 INFO:teuthology.orchestra.run.smithi042.stdout:} it then raises: 2023-02-24T20:49:10.323 INFO:tasks.cephfs_test_runner: info_output = json.loads(self._nfs_cmd('cluster', 'info', self.cluster_id))['test']['backend'][0] 2023-02-24T20:49:10.323 INFO:tasks.cephfs_test_runner:IndexError: list index out of range Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2023-03-30T14:01:01Z

no major changes in last push - https://github.com/ceph/ceph/compare/0ae4720686608595dcf475a83292464a745d3e3c..310286fa185194fdd27bfe7692d516703e18fa11

just the removal of (0, "cephfs path is valid") tuple as return value as its not needed anymore. helper check_cephfs_path doesn't return anything now.

vshankar

Nice work @dparmar18

dparmar18 · 2023-03-31T05:07:14Z

Nice work @dparmar18

thanks venky!

phlogistonjohn · 2023-04-04T14:09:50Z

Hi, sorry for the delayed response to this but I am behind on many tasks.
In go-ceph we had some tests that were impacted by this change. I updated the tests but I also noticed that the text of the error message was a bit awkward. (See ceph/go-ceph#858)

Specifically No such file or directory: "error in stat: b'/march': No such file or directory [Errno 2] is a bit long and redundant IMO. If you don't care to tweak this in a follow up PR, let me know and I'll submit a PR to improve the error text. Thanks @dparmar18

dparmar18 · 2023-04-04T14:35:50Z

That looks quite ugly to be honest, don't worry I'll push appropriate change in a follow up PR soon. Thanks for reporting this @phlogistonjohn

dparmar18 · 2023-05-02T09:17:28Z

Follow up PR #51005

dparmar18 · 2023-05-02T09:28:05Z

@phlogistonjohn above commented PR contains patch for the redundancy mentioned in #49460 (comment), as per test results, it should be now something like this:

No such file or directory path <non_existent_path> does not exist

dparmar18 · 2023-05-02T09:34:05Z

@vshankar #51005 this need to backported right? Should I create a tracker for this or just add the commits to the open backport PRs i.e. #50807, #50808, #50809?

vshankar · 2023-05-02T09:35:31Z

@vshankar this need to backported right? Should I create a tracker for this or just add the commits to the open backport PRs i.e. #50807, #50808, #50809?

Just add the additional commits. I've updated the tracker mentioning a note for backporters.

dparmar18 · 2023-05-02T10:22:12Z

@vshankar this need to backported right? Should I create a tracker for this or just add the commits to the open backport PRs i.e. #50807, #50808, #50809?

Just add the additional commits. I've updated the tracker mentioning a note for backporters.

Done

github-actions bot added nfs pybind cephfs Ceph File System tests labels Dec 15, 2022

dparmar18 marked this pull request as ready for review December 15, 2022 12:02

dparmar18 requested a review from a team December 15, 2022 12:02

vshankar requested changes Dec 15, 2022

View reviewed changes

src/pybind/mgr/nfs/export.py Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-58228 branch 4 times, most recently from 857a116 to 7f9e2c3 Compare December 15, 2022 19:51

dparmar18 requested review from a team and vshankar December 15, 2022 19:52

vshankar requested changes Dec 16, 2022

View reviewed changes

src/pybind/mgr/nfs/export.py Outdated Show resolved Hide resolved

src/pybind/mgr/nfs/export.py Outdated Show resolved Hide resolved

src/pybind/mgr/nfs/export.py Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-58228 branch 5 times, most recently from b41a50c to e0c613c Compare December 19, 2022 11:05

dparmar18 requested review from a team and vshankar December 19, 2022 11:09

github-actions bot added the needs-rebase label Dec 22, 2022

dparmar18 force-pushed the wip-58228 branch from e0c613c to b4c7b3c Compare December 23, 2022 11:35

github-actions bot removed the needs-rebase label Dec 23, 2022

vshankar reviewed Jan 2, 2023

View reviewed changes

src/pybind/cephfs/cephfs.pyx Outdated Show resolved Hide resolved

dparmar18 force-pushed the wip-58228 branch from b4c7b3c to 6be680f Compare January 2, 2023 12:57

phlogistonjohn reviewed Mar 27, 2023

View reviewed changes

src/pybind/mgr/nfs/export.py Outdated Show resolved Hide resolved

phlogistonjohn reviewed Mar 27, 2023

View reviewed changes

src/pybind/mgr/nfs/utils.py Outdated Show resolved Hide resolved

vshankar reviewed Mar 30, 2023

View reviewed changes

src/pybind/mgr/nfs/utils.py Show resolved Hide resolved

dparmar18 added 8 commits March 30, 2023 19:29

mgr/nfs/tests: mock check_cephfs_path

6334e10

Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

mgr/nfs: disallow non-existent paths when creating export

992c686

Fixes: https://tracker.ceph.com/issues/58228 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

qa: added test case test_nfs_export_with_invalid_path

b0bb250

Fixes: https://tracker.ceph.com/issues/58228 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

qa: test export creation at filepath and symlink

afaf251

Fixes: https://tracker.ceph.com/issues/58228 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

qa: fix testcase 'test_cluster_set_user_config_with_non_existing_clus…

3b3fa71

…terid' Fixes: https://tracker.ceph.com/issues/58758 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

vshankar approved these changes Mar 31, 2023

View reviewed changes

This was referenced Mar 31, 2023

quincy: mgr/nfs: disallow non-existent paths when creating export #50807

Merged

reef: mgr/nfs: disallow non-existent paths when creating export #50808

Merged

pacific: mgr/nfs: disallow non-existent paths when creating export #50809

Merged

Conversation

dparmar18 commented Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dparmar18 commented Dec 20, 2022

Uh oh!

dparmar18 commented Dec 20, 2022

Uh oh!

github-actions bot commented Dec 22, 2022

Uh oh!

dparmar18 commented Dec 23, 2022

Uh oh!

Uh oh!

dparmar18 commented Mar 27, 2023

Uh oh!

dparmar18 commented Mar 27, 2023

Uh oh!

phlogistonjohn commented Mar 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dparmar18 commented Mar 30, 2023

Uh oh!

dparmar18 commented Mar 30, 2023

Uh oh!

dparmar18 commented Mar 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Mar 31, 2023

Uh oh!

phlogistonjohn commented Apr 4, 2023

Uh oh!

dparmar18 commented Apr 4, 2023

Uh oh!

dparmar18 commented May 2, 2023

Uh oh!

dparmar18 commented May 2, 2023

Uh oh!

dparmar18 commented May 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented May 2, 2023

Uh oh!

dparmar18 commented May 2, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dparmar18 commented Dec 15, 2022 •

edited

Loading

phlogistonjohn commented Mar 27, 2023 •

edited

Loading

dparmar18 commented Mar 30, 2023 •

edited

Loading

dparmar18 commented May 2, 2023 •

edited

Loading