vstart_runner: make "shell" as default argument by rishabh-d-dave · Pull Request #38443 · ceph/ceph

rishabh-d-dave · 2020-12-04T05:14:56Z

And set its default value to True since methods like
LocalRemote.write_file() pass bash's built-in commands.

Also, update the part where LocalRemote._do_run() sets shell to True
based on whether or not list contains an instance of class Raw.

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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 api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

rishabh-d-dave · 2020-12-04T11:55:50Z

Odd, never requested review from api group.

rishabh-d-dave · 2020-12-04T17:42:42Z

jenkins test make check

rishabh-d-dave · 2020-12-07T16:41:37Z

Since moving the commit that sets shell to True from PR #37655 and PR #37667 to this PR, the Ceph API tests for PR #37655 and #37667 now run successfully and the same tests fails consistently for this PR. Therefore, the issue was certainly caused by this commit.

rishabh-d-dave · 2020-12-07T16:43:07Z

The PR branch update contains the fix for the issue reported here - #38443 (comment).

qa/tasks/ceph_manager.py

rishabh-d-dave · 2021-03-04T05:25:57Z

@batrick ping.

batrick

otherwise lgtm

qa/tasks/ceph_manager.py

rishabh-d-dave · 2021-03-05T16:47:23Z

About the PR branch update: Testing an enhancement with the new commit.

rishabh-d-dave · 2021-03-08T09:13:46Z

Details about last update: #38443 (comment).

rishabh-d-dave · 2021-03-08T13:15:50Z

Tested with vstart_runner - https://gist.github.com/rishabh-d-dave/49fc5ad0a01a96f1d8cefe679c4feca2.

rishabh-d-dave · 2021-03-08T13:20:16Z

@batrick This PR is ready for review and locally it runs fine.

Also, changes to vstart_runner.py have been tested but I am confused if this PR requires more testing since it makes some modifications to qa/tasks/ceph_manager.py and qa/tasks/ceph_test_case.py but in effect those changes lead to nothing different than master.

qa/tasks/ceph_manager.py

rishabh-d-dave · 2021-03-10T16:34:02Z

Tests passed -
~~http://pulpito.front.sepia.ceph.com/rishabh-2021-03-10_09:34:07-fs-wip-rishabh-vr-reuse-AFAP-distro-basic-smithi/~~
http://pulpito.front.sepia.ceph.com/rishabh-2021-03-10_09:33:51-fs-wip-rishabh-vr-reuse-AFAP-distro-basic-smithi/
http://pulpito.front.sepia.ceph.com/rishabh-2021-03-10_16:51:03-fs-wip-rishabh-vr-reuse-AFAP-distro-basic-smithi/

rishabh-d-dave · 2021-03-10T17:28:46Z

@batrick run_ceph_w() is not called anywhere in qa/ except for in assert_cluster_log() which is being called in test_sessionmap, test_forward_scrub and test_mantle. Of these, I ran first 2 (which ran successfully) and I couldn't figure out how to run the last one. Outside qa/tasks/cephfs tests, assert_cluster_log() is called via LocalCephCluster.run_ceph_w() in qa/tasks/mgr tests which already passed on ceph API CI job.

$ grep -rnI run_ceph_w qa/
qa/tasks/ceph_test_case.py:109:                self.watcher_process = ceph_manager.run_ceph_w(watch_channel)
qa/tasks/ceph_manager.py:1439:    def run_ceph_w(self, watch_channel=None):
qa/tasks/vstart_runner.py:787:    def run_ceph_w(self, watch_channel=None):

$ grep -rnI assert_cluster_log qa/
qa/tasks/ceph_test_case.py:85:    def assert_cluster_log(self, expected_pattern, invert_match=False,
qa/tasks/mgr/test_module_selftest.py:238:            with self.assert_cluster_log(log_message):
qa/tasks/mgr/test_module_selftest.py:242:            with self.assert_cluster_log(log_message, watch_channel="audit"):
qa/tasks/cephfs/test_forward_scrub.py:236:        with self.assert_cluster_log("inode table repaired", invert_match=True):
qa/tasks/cephfs/test_forward_scrub.py:261:        with self.assert_cluster_log("inode table repaired"):
qa/tasks/cephfs/test_forward_scrub.py:294:        with self.assert_cluster_log("bad backtrace on inode"):
qa/tasks/cephfs/test_sessionmap.py:186:        with self.assert_cluster_log("client session with non-allowable root '/baz' denied"):
qa/tasks/cephfs/test_mantle.py:25:        with self.assert_cluster_log(failure + obj + " " + expect):
qa/tasks/cephfs/test_mantle.py:36:        with self.assert_cluster_log(failure + " " + expect): pass
qa/tasks/cephfs/test_mantle.py:42:        with self.assert_cluster_log(expect): pass
qa/tasks/cephfs/test_mantle.py:62:        with self.assert_cluster_log(success + "valid.lua"):
qa/tasks/cephfs/test_mantle.py:104:        with self.assert_cluster_log(failure + "valid.lua" + expect, timeout=30):

So, except for test_mantle, everything is covered.

Add "shell" as a parameter for LocalRemoteProcess._do_run() and set its default value to True. This is necessary to align the interface that executes the commands for the teuthology and for vstart_runner.py which reduces compatibility bugs between vstart_runner and teuthology for tests. Also, update the part where LocalRemote._do_run() sets shell to True based on whether or not list contains an instance of class Raw. Signed-off-by: Rishabh Dave <ridave@redhat.com>

Setting shell to True in call to run() in LocalCephManager.run_ceph_w() leads to a crash when self.subproc.communicate() is executed for the process created by running "ceph -w". Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2021-03-15T12:43:37Z

Tests were successful -
http://pulpito.front.sepia.ceph.com/rishabh-2021-03-15_12:02:21-fs-wip-rishabh-vr-set-shell-to-True-distro-basic-smithi/
http://pulpito.front.sepia.ceph.com/rishabh-2021-03-15_12:02:05-fs-wip-rishabh-vr-set-shell-to-True-distro-basic-smithi/

* refs/pull/38443/head: qa: set "shell" to False for run_ceph_w() vstart_runner: make "shell" a default argument

batrick · 2021-03-23T03:00:20Z

https://pulpito.ceph.com/pdonnell-2021-03-18_13:46:31-fs-wip-pdonnell-testing-20210318.024145-distro-basic-smithi/

https://tracker.ceph.com/issues/49466
qa: "Command failed on gibba030 with status 1: 'set -ex\nsudo dd of=/tmp/tmp.ZEeZBasJer'"
https://tracker.ceph.com/issues/48773
qa: scrub does not complete
https://tracker.ceph.com/issues/48805
mds: "cluster [WRN] Scrub error on inode 0x1000000039d (/client.0/tmp/blogbench-1.0/src/blogtest_in) see mds.a log and damage ls output for details"
https://tracker.ceph.com/issues/45434
qa: test_full_fsync (tasks.cephfs.test_full.TestClusterFull) failed
https://tracker.ceph.com/issues/49845
qa: failed umount in test_volumes
https://tracker.ceph.com/issues/49605
mgr: drops command on the floor
https://tracker.ceph.com/issues/48203 (stock kernel update required)
qa: quota failure
https://tracker.ceph.com/issues/49928
client: items pinned in cache preventing unmount x2

batrick · 2021-04-06T17:30:45Z

https://tracker.ceph.com/issues/50178

rishabh-d-dave added tests vstart_runner labels Dec 4, 2020

rishabh-d-dave requested review from a team and removed request for a team December 4, 2020 11:55

rishabh-d-dave force-pushed the vr-set-shell-to-True branch 2 times, most recently from 23660a2 to ff51c81 Compare December 4, 2020 17:12

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from ff51c81 to 1c801ef Compare December 7, 2020 09:29

This was referenced Dec 7, 2020

vstart_runner: accepts multiple commands as str too #37655

Merged

vstart_runner: let LocalRemote inherit Remote #37667

Merged

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from 1c801ef to b1272d5 Compare December 7, 2020 11:58

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from b1272d5 to dd75470 Compare December 7, 2020 16:41

batrick requested changes Dec 7, 2020

View reviewed changes

qa/tasks/ceph_manager.py Show resolved Hide resolved

rishabh-d-dave added the bug-fix label Dec 8, 2020

rishabh-d-dave force-pushed the vr-set-shell-to-True branch 2 times, most recently from 0651ef1 to 73e411e Compare January 11, 2021 04:50

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from 73e411e to c7c5ad8 Compare January 22, 2021 18:12

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from c7c5ad8 to 7cfae0d Compare February 8, 2021 11:34

rishabh-d-dave requested review from a team, batrick and lxbsz February 25, 2021 06:31

batrick requested changes Mar 4, 2021

View reviewed changes

qa/tasks/ceph_manager.py Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from 696a83d to 766b9e2 Compare March 8, 2021 09:09

rishabh-d-dave requested a review from batrick March 8, 2021 09:14

batrick approved these changes Mar 8, 2021

View reviewed changes

batrick added cephfs Ceph File System needs-qa wip-pdonnell-testing labels Mar 8, 2021

batrick requested changes Mar 9, 2021

View reviewed changes

qa/tasks/ceph_manager.py Outdated Show resolved Hide resolved

batrick removed needs-qa wip-pdonnell-testing labels Mar 9, 2021

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from 766b9e2 to 3bdec04 Compare March 10, 2021 09:19

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Mar 10, 2021

rishabh-d-dave added 2 commits March 12, 2021 09:01

qa: set "shell" to False for run_ceph_w()

df88ec3

Setting shell to True in call to run() in LocalCephManager.run_ceph_w() leads to a crash when self.subproc.communicate() is executed for the process created by running "ceph -w". Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave force-pushed the vr-set-shell-to-True branch from 3bdec04 to df88ec3 Compare March 12, 2021 03:34

rishabh-d-dave requested a review from batrick March 13, 2021 06:02

lxbsz approved these changes Mar 15, 2021

View reviewed changes

batrick added needs-qa wip-pdonnell-testing labels Mar 16, 2021

rishabh-d-dave removed the wip-rishabh-testing Rishabh's testing label label Mar 17, 2021

batrick added a commit to batrick/ceph that referenced this pull request Mar 18, 2021

Merge PR ceph#38443 into wip-pdonnell-testing-20210318.024145

42a9575

* refs/pull/38443/head: qa: set "shell" to False for run_ceph_w() vstart_runner: make "shell" a default argument

batrick approved these changes Mar 23, 2021

View reviewed changes

batrick merged commit 42270a5 into ceph:master Mar 23, 2021

rishabh-d-dave deleted the vr-set-shell-to-True branch March 23, 2021 03:47

batrick mentioned this pull request Apr 6, 2021

qa: reuse methods in vstart_runner.LocalCephManager #38481

Merged

3 tasks

Conversation

rishabh-d-dave commented Dec 4, 2020

Checklist

Uh oh!

rishabh-d-dave commented Dec 4, 2020

Uh oh!

rishabh-d-dave commented Dec 4, 2020

Uh oh!

rishabh-d-dave commented Dec 7, 2020

Uh oh!

rishabh-d-dave commented Dec 7, 2020

Uh oh!

Uh oh!

rishabh-d-dave commented Mar 4, 2021

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rishabh-d-dave commented Mar 5, 2021

Uh oh!

rishabh-d-dave commented Mar 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented Mar 8, 2021

Uh oh!

rishabh-d-dave commented Mar 8, 2021

Uh oh!

Uh oh!

rishabh-d-dave commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented Mar 10, 2021

Uh oh!

rishabh-d-dave commented Mar 15, 2021

Uh oh!

batrick commented Mar 23, 2021

Uh oh!

batrick commented Apr 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rishabh-d-dave commented Mar 8, 2021 •

edited

Loading

rishabh-d-dave commented Mar 10, 2021 •

edited

Loading