qa/tasks/test_nfs: have TestNFS inherit from CephFSTestCase by adk3798 · Pull Request #52556 · ceph/ceph

adk3798 · 2023-07-19T20:08:54Z

The TestNFS was recently updated to use run_ceph_cmd but doesn't actually have access to it, resulting in

====================================================================== 
ERROR: test_cluster_info (tasks.cephfs.test_nfs.TestNFS) 
---------------------------------------------------------------------- 
Traceback (most recent call last):
  File "/home/teuthworker/src/git.ceph.com_ceph-c_4ae2a76aad461f2c9f6a1456c25df23ec97a5b2f/qa/tasks/cephfs/test_nfs.py", line 578, in test_cluster_info
    self._test_create_cluster()
  File "/home/teuthworker/src/git.ceph.com_ceph-c_4ae2a76aad461f2c9f6a1456c25df23ec97a5b2f/qa/tasks/cephfs/test_nfs.py", line 156, in _test_create_cluster
    cluster_create = self._nfs_complete_cmd(
  File "/home/teuthworker/src/git.ceph.com_ceph-c_4ae2a76aad461f2c9f6a1456c25df23ec97a5b2f/qa/tasks/cephfs/test_nfs.py", line 25, in _nfs_complete_cmd
    return self.run_ceph_cmd(args=f"nfs {cmd}", stdout=StringIO(),
AttributeError: 'TestNFS' object has no attribute 'run_ceph_cmd'

----------------------------------------------------------------------

The function we need is specified in the new RunCephCmd class, so I think inheriting from there should fix this.

Fixes: https://tracker.ceph.com/issues/62084

The run_ceph_cmd function was added in #50569

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

vshankar · 2023-07-25T04:57:18Z

I guess this is caused by commit f8f2154.

@rishabh-d-dave

vshankar · 2023-07-27T05:43:50Z

@rishabh-d-dave https://tracker.ceph.com/issues/62084#note-4

PTAL

rishabh-d-dave · 2023-07-31T12:43:22Z

@rishabh-d-dave https://tracker.ceph.com/issues/62084#note-4

PTAL

Done. I've left reply on this tracker and I've created a new ticket for the failure to reported there. I'll raise a PR for it ASAP.

rishabh-d-dave · 2023-08-05T10:54:28Z

@adk3798 I'll backport this PR along with few other commits from PR #50569.

adk3798 · 2023-08-08T13:38:30Z

qa/tasks/cephfs/test_nfs.py


 # TODO Add test for cluster update when ganesha can be deployed on multiple ports.
-class TestNFS(MgrTestCase):
+class TestNFS(MgrTestCase, RunCephCmd):


at least a different error now

2023-08-08T06:47:14.895 INFO:tasks.cephfs_test_runner:ERROR: test_cluster_info (tasks.cephfs.test_nfs.TestNFS) 2023-08-08T06:47:14.895 INFO:tasks.cephfs_test_runner:---------------------------------------------------------------------- 2023-08-08T06:47:14.895 INFO:tasks.cephfs_test_runner:Traceback (most recent call last): 2023-08-08T06:47:14.896 INFO:tasks.cephfs_test_runner: File "/home/teuthworker/src/git.ceph.com_ceph-c_b438b973c217f09b2e276d43fa768d437079c176/qa/tasks/cephfs/test_nfs.py", line 579, in test_cluster_info 2023-08-08T06:47:14.896 INFO:tasks.cephfs_test_runner: self._test_create_cluster() 2023-08-08T06:47:14.896 INFO:tasks.cephfs_test_runner: File "/home/teuthworker/src/git.ceph.com_ceph-c_b438b973c217f09b2e276d43fa768d437079c176/qa/tasks/cephfs/test_nfs.py", line 157, in _test_create_cluster 2023-08-08T06:47:14.896 INFO:tasks.cephfs_test_runner: cluster_create = self._nfs_complete_cmd( 2023-08-08T06:47:14.896 INFO:tasks.cephfs_test_runner: File "/home/teuthworker/src/git.ceph.com_ceph-c_b438b973c217f09b2e276d43fa768d437079c176/qa/tasks/cephfs/test_nfs.py", line 26, in _nfs_complete_cmd 2023-08-08T06:47:14.897 INFO:tasks.cephfs_test_runner: return self.run_ceph_cmd(args=f"nfs {cmd}", stdout=StringIO(), 2023-08-08T06:47:14.897 INFO:tasks.cephfs_test_runner: File "/home/teuthworker/src/git.ceph.com_ceph-c_b438b973c217f09b2e276d43fa768d437079c176/qa/tasks/cephfs/cephfs_test_case.py", line 58, in run_ceph_cmd 2023-08-08T06:47:14.897 INFO:tasks.cephfs_test_runner: return self.mon_manager.run_cluster_cmd(**kwargs) 2023-08-08T06:47:14.897 INFO:tasks.cephfs_test_runner:AttributeError: 'NoneType' object has no attribute 'run_cluster_cmd'

https://pulpito.ceph.com/adking-2023-08-04_21:56:32-orch:cephadm-wip-adk-testing-2023-08-04-1524-distro-default-smithi/7361645/

This class will either need to have it's own version of _init_mon_manager or we'll need it to inherit from CephFSTestCase directly I think

or just revert to old behavior if those don't work out anyway

This class will either need to have it's own version of _init_mon_manager or we'll need it to inherit from CephFSTestCase directly I think

trying the second of these two options now

The TestNFS was recently updated to use run_ceph_cmd but doesn't actually have access to it, resulting in ====================================================================== ERROR: test_cluster_info (tasks.cephfs.test_nfs.TestNFS) ---------------------------------------------------------------------- Traceback (most recent call last): File "/home/teuthworker/src/git.ceph.com_ceph-c_4ae2a76aad461f2c9f6a1456c25df23ec97a5b2f/qa/tasks/cephfs/test_nfs.py", line 578, in test_cluster_info self._test_create_cluster() File "/home/teuthworker/src/git.ceph.com_ceph-c_4ae2a76aad461f2c9f6a1456c25df23ec97a5b2f/qa/tasks/cephfs/test_nfs.py", line 156, in _test_create_cluster cluster_create = self._nfs_complete_cmd( File "/home/teuthworker/src/git.ceph.com_ceph-c_4ae2a76aad461f2c9f6a1456c25df23ec97a5b2f/qa/tasks/cephfs/test_nfs.py", line 25, in _nfs_complete_cmd return self.run_ceph_cmd(args=f"nfs {cmd}", stdout=StringIO(), AttributeError: 'TestNFS' object has no attribute 'run_ceph_cmd' ---------------------------------------------------------------------- The function we need is specified in the new RunCephCmd class, which CephFSTestCase inherits from as well. We can't directly inherit from RunCephCmd because that would skip the set up function in CephFSTestCase which sets up the `mon_manager` attribute resulting in File "<ceph-dir>/qa/tasks/cephfs/test_nfs.py", line 26, in _nfs_complete_cmd return self.run_ceph_cmd(args=f"nfs {cmd}", stdout=StringIO(), File "<ceph-dir>/qa/tasks/cephfs/cephfs_test_case.py", line 58, in run_ceph_cmd return self.mon_manager.run_cluster_cmd(**kwargs) AttributeError: 'NoneType' object has no attribute 'run_cluster_cmd' Fixes: https://tracker.ceph.com/issues/62084 Signed-off-by: Adam King <adking@redhat.com>

dparmar18 · 2023-08-09T10:37:34Z

@adk3798 I feel we should revert back _nfs_complete_cmd to it's original definition, inheriting a whole class just for a single helper seems a bit overkill to me.

dparmar18 · 2023-08-09T11:19:19Z

similar issue

"/home/teuthworker/src/github.com_dparmar18_ceph_78327d1d5f1c7343b1297b8c6a76fd7e069b5e56/qa/tasks/cephfs/test_nfs.py", line 19, in _cmd
2023-08-09T11:09:18.767 INFO:tasks.cephfs_test_runner:    return self.get_ceph_cmd_stdout(args)
2023-08-09T11:09:18.767 INFO:tasks.cephfs_test_runner:AttributeError: 'TestNFS' object has no attribute 'get_ceph_cmd_stdout'
2023-08-09T11:09:18.767 INFO:tasks.cephfs_test_runner:
2023-08-09T11:09:18.768 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2023-08-09T11:09:18.768 INFO:tasks.cephfs_test_runner:Ran 1 test in 43.760s
2023-08-09T11:09:18.768 INFO:tasks.cephfs_test_runner:
2023-08-09T11:09:18.768 INFO:tasks.cephfs_test_runner:FAILED (errors=1)
2023-08-09T11:09:18.768 INFO:tasks.cephfs_test_runner:
2023-08-09T11:09:18.769 INFO:tasks.cephfs_test_runner:======================================================================
2023-08-09T11:09:18.769 INFO:tasks.cephfs_test_runner:ERROR: test_cephfs_export_update_at_non_dir_path (tasks.cephfs.test_nfs.TestNFS)```

rishabh-d-dave · 2023-08-09T11:58:01Z

@dparmar18's point is correct, inheriting entire CephFSTestCase just for inheriting 3 helpers is a little too much. However, the solution he proposes would defeat the purpose of PR #50569. The correct thing to do IMO is to import RunCephCmd directly or indirectly. I'll propose a rough patch in a short time.

rishabh-d-dave · 2023-08-09T12:06:17Z

Following patch should fix it. It does 3 things -

First, moves RunCephCmd from cephfs_test_case.py to ceph_test_case.py so that other test case classes (like MgrTestCase) can use it directly.
Second, RunCephCmd is inherited by class CephTestCase instead of class CephFSTestCase.
Third, it updates qa/tasks/cephfs/filesystem.py to import RunCephCmd from ceph_test_case.py instead of cephfs_test_case.py.

diff --git a/qa/tasks/ceph_test_case.py b/qa/tasks/ceph_test_case.py
index 3f355825c6f..d6ba4b0906c 100644
--- a/qa/tasks/ceph_test_case.py
+++ b/qa/tasks/ceph_test_case.py
@@ -13,7 +13,32 @@ log = logging.getLogger(__name__)
 class TestTimeoutError(RuntimeError):
     pass
 
-class CephTestCase(unittest.TestCase):
+class RunCephCmd:
+
+    def run_ceph_cmd(self, *args, **kwargs):
+        if kwargs.get('args') is None and args:
+            if len(args) == 1:
+                args = args[0]
+            kwargs['args'] = args
+        return self.ceph_cluster.mon_manager.run_cluster_cmd(**kwargs)
+
+    def get_ceph_cmd_result(self, *args, **kwargs):
+        if kwargs.get('args') is None and args:
+            if len(args) == 1:
+                args = args[0]
+            kwargs['args'] = args
+        return self.run_ceph_cmd(**kwargs).exitstatus
+
+    def get_ceph_cmd_stdout(self, *args, **kwargs):
+        if kwargs.get('args') is None and args:
+            if len(args) == 1:
+                args = args[0]
+            kwargs['args'] = args
+        kwargs['stdout'] = kwargs.pop('stdout', StringIO())
+        return self.run_ceph_cmd(**kwargs).stdout.getvalue()
+
+
+class CephTestCase(unittest.TestCase, RunCephCmd):
     """
     For test tasks that want to define a structured set of
     tests implemented in python.  Subclass this with appropriate
diff --git a/qa/tasks/cephfs/cephfs_test_case.py b/qa/tasks/cephfs/cephfs_test_case.py
index 968bce4093f..3e8aaeacf6e 100644
--- a/qa/tasks/cephfs/cephfs_test_case.py
+++ b/qa/tasks/cephfs/cephfs_test_case.py
@@ -48,32 +48,7 @@ class MountDetails():
         mntobj.hostfs_mntpt = self.hostfs_mntpt
 
 
-class RunCephCmd:
-
-    def run_ceph_cmd(self, *args, **kwargs):
-        if kwargs.get('args') is None and args:
-            if len(args) == 1:
-                args = args[0]
-            kwargs['args'] = args
-        return self.mon_manager.run_cluster_cmd(**kwargs)
-
-    def get_ceph_cmd_result(self, *args, **kwargs):
-        if kwargs.get('args') is None and args:
-            if len(args) == 1:
-                args = args[0]
-            kwargs['args'] = args
-        return self.run_ceph_cmd(**kwargs).exitstatus
-
-    def get_ceph_cmd_stdout(self, *args, **kwargs):
-        if kwargs.get('args') is None and args:
-            if len(args) == 1:
-                args = args[0]
-            kwargs['args'] = args
-        kwargs['stdout'] = kwargs.pop('stdout', StringIO())
-        return self.run_ceph_cmd(**kwargs).stdout.getvalue()
-
-
-class CephFSTestCase(CephTestCase, RunCephCmd):
+class CephFSTestCase(CephTestCase):
     """
     Test case for Ceph FS, requires caller to populate Filesystem and Mounts,
     into the fs, mount_a, mount_b class attributes (setting mount_b is optional)
diff --git a/qa/tasks/cephfs/filesystem.py b/qa/tasks/cephfs/filesystem.py
index 2f0c4ab03ca..0d81da36071 100644
--- a/qa/tasks/cephfs/filesystem.py
+++ b/qa/tasks/cephfs/filesystem.py
@@ -20,7 +20,7 @@ from teuthology import contextutil
 
 from tasks.ceph_manager import write_conf
 from tasks.ceph_manager import CephManager
-from tasks.cephfs.cephfs_test_case import RunCephCmd
+from tasks.ceph_test_case import RunCephCmd
 
 
 log = logging.getLogger(__name__)

adk3798 · 2023-08-09T15:39:30Z

qa/tasks/cephfs/test_nfs.py


 # TODO Add test for cluster update when ganesha can be deployed on multiple ports.
-class TestNFS(MgrTestCase):
+class TestNFS(MgrTestCase, CephFSTestCase):


inheriting the full class doesn't work either

2023-08-09T15:33:45.851 INFO:tasks.cephfs_test_runner:test_cluster_info (tasks.cephfs.test_nfs.TestNFS) ... ERROR 2023-08-09T15:33:45.852 INFO:tasks.cephfs_test_runner: 2023-08-09T15:33:45.852 INFO:tasks.cephfs_test_runner:====================================================================== 2023-08-09T15:33:45.853 INFO:tasks.cephfs_test_runner:ERROR: test_cluster_info (tasks.cephfs.test_nfs.TestNFS) 2023-08-09T15:33:45.853 INFO:tasks.cephfs_test_runner:---------------------------------------------------------------------- 2023-08-09T15:33:45.854 INFO:tasks.cephfs_test_runner:Traceback (most recent call last): 2023-08-09T15:33:45.854 INFO:tasks.cephfs_test_runner: File "/home/teuthworker/src/git.ceph.com_ceph-c_1c7f5f4949aae52e52d69742399b261751d96650/qa/tasks/cephfs/test_nfs.py", line 39, in setUp 2023-08-09T15:33:45.854 INFO:tasks.cephfs_test_runner: super(TestNFS, self).setUp() 2023-08-09T15:33:45.855 INFO:tasks.cephfs_test_runner: File "/home/teuthworker/src/git.ceph.com_ceph-c_1c7f5f4949aae52e52d69742399b261751d96650/qa/tasks/cephfs/cephfs_test_case.py", line 152, in setUp 2023-08-09T15:33:45.855 INFO:tasks.cephfs_test_runner: if len(self.mds_cluster.mds_ids) < self.MDSS_REQUIRED: 2023-08-09T15:33:45.856 INFO:tasks.cephfs_test_runner:AttributeError: 'NoneType' object has no attribute 'mds_ids' 2023-08-09T15:33:45.856 INFO:tasks.cephfs_test_runner: 2023-08-09T15:33:45.856 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------

So we're left with either going back to old behavior or @rishabh-d-dave 's patch.

dparmar18 · 2023-08-10T05:43:26Z

Following patch should fix it. It does 3 things -

First, moves RunCephCmd from cephfs_test_case.py to ceph_test_case.py so that other test case classes (like MgrTestCase) can use it directly.
Second, RunCephCmd is inherited by class CephTestCase instead of class CephFSTestCase.
Third, it updates qa/tasks/cephfs/filesystem.py to import RunCephCmd from ceph_test_case.py instead of cephfs_test_case.py.

diff --git a/qa/tasks/ceph_test_case.py b/qa/tasks/ceph_test_case.py
index 3f355825c6f..d6ba4b0906c 100644
--- a/qa/tasks/ceph_test_case.py
+++ b/qa/tasks/ceph_test_case.py
@@ -13,7 +13,32 @@ log = logging.getLogger(__name__)
 class TestTimeoutError(RuntimeError):
     pass
 
-class CephTestCase(unittest.TestCase):
+class RunCephCmd:
+
+    def run_ceph_cmd(self, *args, **kwargs):
+        if kwargs.get('args') is None and args:
+            if len(args) == 1:
+                args = args[0]
+            kwargs['args'] = args
+        return self.ceph_cluster.mon_manager.run_cluster_cmd(**kwargs)
+
+    def get_ceph_cmd_result(self, *args, **kwargs):
+        if kwargs.get('args') is None and args:
+            if len(args) == 1:
+                args = args[0]
+            kwargs['args'] = args
+        return self.run_ceph_cmd(**kwargs).exitstatus
+
+    def get_ceph_cmd_stdout(self, *args, **kwargs):
+        if kwargs.get('args') is None and args:
+            if len(args) == 1:
+                args = args[0]
+            kwargs['args'] = args
+        kwargs['stdout'] = kwargs.pop('stdout', StringIO())
+        return self.run_ceph_cmd(**kwargs).stdout.getvalue()
+
+
+class CephTestCase(unittest.TestCase, RunCephCmd):
     """
     For test tasks that want to define a structured set of
     tests implemented in python.  Subclass this with appropriate
diff --git a/qa/tasks/cephfs/cephfs_test_case.py b/qa/tasks/cephfs/cephfs_test_case.py
index 968bce4093f..3e8aaeacf6e 100644
--- a/qa/tasks/cephfs/cephfs_test_case.py
+++ b/qa/tasks/cephfs/cephfs_test_case.py
@@ -48,32 +48,7 @@ class MountDetails():
         mntobj.hostfs_mntpt = self.hostfs_mntpt
 
 
-class RunCephCmd:
-
-    def run_ceph_cmd(self, *args, **kwargs):
-        if kwargs.get('args') is None and args:
-            if len(args) == 1:
-                args = args[0]
-            kwargs['args'] = args
-        return self.mon_manager.run_cluster_cmd(**kwargs)
-
-    def get_ceph_cmd_result(self, *args, **kwargs):
-        if kwargs.get('args') is None and args:
-            if len(args) == 1:
-                args = args[0]
-            kwargs['args'] = args
-        return self.run_ceph_cmd(**kwargs).exitstatus
-
-    def get_ceph_cmd_stdout(self, *args, **kwargs):
-        if kwargs.get('args') is None and args:
-            if len(args) == 1:
-                args = args[0]
-            kwargs['args'] = args
-        kwargs['stdout'] = kwargs.pop('stdout', StringIO())
-        return self.run_ceph_cmd(**kwargs).stdout.getvalue()
-
-
-class CephFSTestCase(CephTestCase, RunCephCmd):
+class CephFSTestCase(CephTestCase):
     """
     Test case for Ceph FS, requires caller to populate Filesystem and Mounts,
     into the fs, mount_a, mount_b class attributes (setting mount_b is optional)
diff --git a/qa/tasks/cephfs/filesystem.py b/qa/tasks/cephfs/filesystem.py
index 2f0c4ab03ca..0d81da36071 100644
--- a/qa/tasks/cephfs/filesystem.py
+++ b/qa/tasks/cephfs/filesystem.py
@@ -20,7 +20,7 @@ from teuthology import contextutil
 
 from tasks.ceph_manager import write_conf
 from tasks.ceph_manager import CephManager
-from tasks.cephfs.cephfs_test_case import RunCephCmd
+from tasks.ceph_test_case import RunCephCmd
 
 
 log = logging.getLogger(__name__)

yeah this makes sense since RunCephCmd is not only CephFS specific but should be enabled to be used from anywhere

rishabh-d-dave · 2023-09-05T14:52:07Z

PR #52924 has been merged.

rishabh-d-dave · 2023-09-11T14:11:16Z

@adk3798 Let's close this PR?

adk3798 · 2023-09-11T14:21:16Z

@adk3798 Let's close this PR?

yeah

adk3798 added tests cephadm nfs labels Jul 19, 2023

github-actions bot added the cephfs Ceph File System label Jul 19, 2023

adk3798 added the wip-adk-testing label Jul 19, 2023

vshankar requested a review from rishabh-d-dave July 25, 2023 04:54

rishabh-d-dave approved these changes Jul 27, 2023

View reviewed changes

adk3798 added the wip-adk4-testing label Aug 4, 2023

adk3798 commented Aug 8, 2023

View reviewed changes

adk3798 force-pushed the TestNFS_no_run_ceph_cmd branch from cfe02b8 to f220cf7 Compare August 8, 2023 13:56

adk3798 changed the title ~~qa/tasks/test_nfs: have TestNFS inherit from RunCephCmd~~ qa/tasks/test_nfs: have TestNFS inherit from CephFSTestCase Aug 8, 2023

adk3798 commented Aug 9, 2023

View reviewed changes

adk3798 removed wip-adk-testing wip-adk4-testing labels Aug 10, 2023

vshankar mentioned this pull request Aug 23, 2023

qa: inherit RunCephCmd in CephTestCase instead of CephFSTestCase #52924

Merged

14 tasks

adk3798 closed this Sep 11, 2023

Conversation

adk3798 commented Jul 19, 2023

Contribution Guidelines

Checklist

Uh oh!

vshankar commented Jul 25, 2023

Uh oh!

vshankar commented Jul 27, 2023

Uh oh!

rishabh-d-dave commented Jul 31, 2023

Uh oh!

rishabh-d-dave commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adk3798 Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

adk3798 Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

adk3798 Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

adk3798 Aug 8, 2023

Choose a reason for hiding this comment

Uh oh!

adk3798 Aug 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

dparmar18 commented Aug 9, 2023

Uh oh!

rishabh-d-dave commented Aug 9, 2023

Uh oh!

rishabh-d-dave commented Aug 9, 2023

Uh oh!

adk3798 Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Aug 10, 2023

Uh oh!

rishabh-d-dave commented Sep 5, 2023

Uh oh!

rishabh-d-dave commented Sep 11, 2023

Uh oh!

adk3798 commented Sep 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rishabh-d-dave commented Aug 5, 2023 •

edited

Loading

adk3798 Aug 8, 2023 •

edited

Loading

dparmar18 commented Aug 9, 2023 •

edited

Loading