teuthology/schedule: Add "descr" option #1973

aclamk · 2024-07-08T12:18:22Z

A convenience option.
When a large batch has many failed tests, "descr" makes it easy to schedule just selected ones. It is as simple as copy-paste description of failed test.

teuthology-suite -e [email protected] -vv -c aclamk-testing-ceres-2024-07-01-1011 -p 50
--descr "rados/singleton-bluestore/{all/cephtool mon_election/connectivity msgr-failures/none
msgr/async-v2only objectstore/bluestore-comp-lz4 rados supported-random-distro$/{centos_latest}}"

Many tests may be rerun in this fashion; "descr" accepts multiple comma separated test descriptions.

kshtsk · 2024-07-10T17:34:01Z

teuthology/suite/run.py

@@ -27,6 +27,32 @@

 log = logging.getLogger(__name__)

+def descr_to_yamls(descriptions, suites_path):


this methods lacks descriptions, and unit tests, could you please add some

@kshtsk Please help. I need suggestion how unit test should look like.

One test case you've already put to the description, now just turn it into a code.

we're trying to use python typing for new code:

def yamls_from_desc(descriptions: str, suite_path: str) -> list[str]:

kshtsk · 2024-07-24T08:03:23Z

scripts/suite.py

@@ -126,6 +127,9 @@
                              2/<outof> ... <outof>-1/<outof> will schedule all
                              jobs in the suite (many more than once). If specified,
                              this value can be found in results.log.
+  --descr <descr>             Use to rerun tests. Reschedule test based on their


@zmc what was the outcome after talk to author? Was our suggestion correct the feature more about scheduling a new suite rather than rerun?

In that case it must look like:
--descr Schedule a suite based on comma separated list of descriptions.

@kshtsk This is why english-ish users should get their texts double checked.

kshtsk · 2024-07-24T08:13:06Z

teuthology/suite/run.py

        """
-        Schedule the suite-run. Returns the number of jobs scheduled.
+        Function converts description of the run into sequence of .yaml files.


This is not a correct explanation. A description is string representation of a single case corresponding a job from which an arbitrary run consist of.

True. Fixing...

kshtsk · 2024-07-24T08:13:57Z

teuthology/suite/run.py

        """
-        Schedule the suite-run. Returns the number of jobs scheduled.
+        Function converts description of the run into sequence of .yaml files.
+        Input "descriptions" is the value of "--descr" parameter;


No sense to refer the parameter name, because it is external interface for the application.

kshtsk · 2024-07-24T08:58:22Z

teuthology/suite/run.py

+        result = []
+        desc_tab = descriptions.split(',')
+        for d in desc_tab:
+            d = d.strip()


bad style reassigning vars

Fixed by giving new name. I could otherwise invoke strip() twice, but I didn't like it.

kshtsk · 2024-07-24T09:30:43Z

teuthology/suite/run.py

-        Schedule the suite-run. Returns the number of jobs scheduled.
+        Function converts description of the run into sequence of .yaml files.
+        Input "descriptions" is the value of "--descr" parameter;
+        "description" consists of comma-separated list of run description


I think you meant plural form "descriptions" instead of singular "description". I suggest to move the splitting a description parameter out of the method, so you can use it like:

yamls = yamls_from_descr(self.args.descr.split(","), suites_path)

The descriptions for the "suites_path", however it is self description, and because of English, rename to "suite_path".

kshtsk · 2024-07-24T09:32:15Z

teuthology/suite/run.py

+        descriptions="rerun/rados/objectstore/{backends/objectstore-bluestore-b supported-random-distro$/{centos_latest}}"
+        suites_path="/cephfs/github.com_ceph_build/qa/suites/"
+        Output:
+        ('rados/objectstore/{backends/objectstore-bluestore-b supported-random-distro$/{ubuntu_latest}}',
+        ['/cephfs/github.com_ceph_build/qa/suites/rados/objectstore/backends/objectstore-bluestore-b.yaml',
+         '/cephfs/github.com_ceph_build/qa/suites/rados/objectstore/supported-random-distro$/ubuntu_latest.yaml'])


It should be more general, I would suggest to remove this examples from here and make a unittest case for it.

How do I run teuthology unittests?
Can I find somewhere a doc for this?
I might get how to code one by pattern-matching.

tox -e py3
to run only part of it you might try: tox -e py3 -- teuthology/suite or even tox -e py3 -- teuthology/suite/test/test_util.py

or even a separate test:

tox -e py3 -- teuthology/suite/test/test_run_.py::TestRun::test_sha1_exists

if you have pytest installed, being in virtualenv enabled similar:

pytest teuthology/suite/test/test_run_.py::TestRun::test_sha1_exists

@kshtsk Thanks, I was able to add unit tests in form of 3 separate, concrete examples. Please take a look.

kshtsk · 2024-07-24T09:47:29Z

teuthology/suite/run.py

+                elif source[pos] == '}':
+                    if base_pos != pos:
+                        test_yamls.append(suites_path + "/" + prefix + source[base_pos:pos] + ".yaml")
+                    return pos + 1
+                elif source[pos] == ' ':
+                    if base_pos != pos:
+                        test_yamls.append(suites_path + "/" + prefix + source[base_pos:pos] + ".yaml")
+                    pos = pos + 1
+                    base_pos = pos
+                else:


Looks like code duplication.

I know. But I was faced with either repeat same code lines, or make more convoluted state processor.
You think "switch" will make it nicer?

kshtsk · 2024-07-24T09:47:52Z

teuthology/suite/run.py

-            arch = util.get_arch(self.base_config.machine_type)
-        suite_name = self.base_config.suite
-        suite_path = os.path.normpath(os.path.join(
+        def expand_descr(test_yamls, source, prefix, base_pos):


no descriptions for this method? I think since the function returns values and do some test_yaml modifications, it is probably better to describe briefly what is the idea behind and give a better name for it.

Added description.

kshtsk · 2024-07-24T10:18:49Z

teuthology/suite/run.py

+        suites_path = os.path.normpath(os.path.join(
            self.suite_repo_path,
            self.args.suite_relpath,
-            'suites',
-            self.base_config.suite.replace(':', '/'),
+            'suites'
+        ))
+        suite_path = os.path.normpath(os.path.join(
+            suites_path,
+            suite_name.replace(':', '/'),


Having suites_path and suite_path is confusing, there must be only one "suite_path"

And there are many suites in path that contains all suites.
How should I name it?

If you have to distinguish base suite directory from particular suite directory, maybe use another name base_suite_path or root_suite_path or anyways suite and suites is too confusing, because we have some nested suites.

A convenience option. When a large batch has many failed tests, "descr" makes it easy to schedule just selected ones. It is as simple as copy-paste description of failed test. teuthology-suite -e [email protected] -vv -c aclamk-testing-ceres-2024-07-01-1011 -p 50 \ --descr "rados/singleton-bluestore/{all/cephtool mon_election/connectivity msgr-failures/none \ msgr/async-v2only objectstore/bluestore-comp-lz4 rados supported-random-distro$/{centos_latest}}" Many tests may be rerun in this fashion; "descr" accepts multiple comma separated test descriptions. Signed-off-by: Adam Kupczyk <[email protected]>

kshtsk · 2024-08-02T18:56:22Z

teuthology/suite/run.py

+    Function converts description of a job into sequence of .yaml files.
+    Input "descriptions" is a string containing comma-separated list of job descriptions.
+    Input "suites_path" is a posix path to dir containing all suites.
+    """


That's way better, thank you, but could you please take a look at nice example of docstring usage https://github.com/ceph/teuthology/blob/main/teuthology/orchestra/remote.py#L221-L237

kshtsk · 2024-08-02T19:12:47Z

teuthology/suite/run.py

+    for d in desc_tab:
+        dd = d.strip()


instead of creating another variable you could use python style generator expression:

for d in (_.strip() for _ in descriptions.split(',')):

I apologise for the next example, but I was not able to refrain. As far as I understood this part of the code I would refactor it like that:

def expand_path(paths: list[str], source: str, prefix: str, pos: int) -> int: """ Expand description using production rules (explanation, not Chomsky context-free formalism): 1) simplification: "A{BX}" => AB A{X} 2) termination: "A" => "A" :param paths: pointer to the extendible list of yaml paths expanded from the source string :param source: source string containing a single test description :param prefix: suffix a string representation of a part of path :param pos: base position in the source string to scan next portion of prefix :returns: index of a character following closed brace, i.e. character "}". """ p = i = pos while (i < len(source)): path = prefix + source[p:i] match (source[i]): case '{': p = i = expand_path(paths, source, path, i + 1) continue case '}': if p != i: paths.append(path) return i + 1 case ' ': if p != i: paths.append(path) p = i = i + 1 case _: i = i + 1 for d in (_.strip() for _ in descriptions.split(',')): yamls = [] expand_path(yamls, d, "", 0) yield (d, [f"{base_path}/{_}.yaml" for _ in yamls])

kshtsk · 2024-08-02T19:23:40Z

teuthology/suite/test/test_run_.py

@@ -360,3 +360,52 @@ def test_newest_success(
        m_find_git_parents.assert_has_calls(
            [call('ceph', 'ceph_sha1', 10)]
        )
+
+    def test_dupa(


very funny, I guess you've used Polish native-speaking skills here ;-) may we have some more appropriate name, like: test_<func-name-we-test>?

I mean:

@pytest.mark.parametrize('desc, base, result', [ [ "rados/objectstore/{backends/objectstore-bluestore-b supported-random-distro$/{ubuntu_latest}}", "/cephfs/github.com_ceph_build/qa/suites", [ ('rados/objectstore/{backends/objectstore-bluestore-b supported-random-distro$/{ubuntu_latest}}', ['/cephfs/github.com_ceph_build/qa/suites/rados/objectstore/backends/objectstore-bluestore-b.yaml', '/cephfs/github.com_ceph_build/qa/suites/rados/objectstore/supported-random-distro$/ubuntu_latest.yaml']) ] ], [ "rados/singleton-nomsgr/{all/health-warnings mon_election/connectivity rados supported-random-distro$/{ubuntu_latest}}", "/cephfs/home/user/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites", [ ("rados/singleton-nomsgr/{all/health-warnings mon_election/connectivity rados supported-random-distro$/{ubuntu_latest}}", ['/cephfs/home/user/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/all/health-warnings.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/mon_election/connectivity.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/rados.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/supported-random-distro$/ubuntu_latest.yaml']) ] ], [ "rados/cephadm/osds/{0-distro/centos_9.stream_runc 0-nvme-loop 1-start 2-ops/rm-zap-flag}," "rados/standalone/{supported-random-distro$/{ubuntu_latest} workloads/scrub}", "/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites", [ ("rados/cephadm/osds/{0-distro/centos_9.stream_runc 0-nvme-loop 1-start 2-ops/rm-zap-flag}", ['/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/0-distro/centos_9.stream_runc.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/0-nvme-loop.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/1-start.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/2-ops/rm-zap-flag.yaml']) , ("rados/standalone/{supported-random-distro$/{ubuntu_latest} workloads/scrub}", ['/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites/rados/standalone/supported-random-distro$/ubuntu_latest.yaml', '/cephfs/home/user/src/git.ceph.com_ceph-c_squid/qa/suites/rados/standalone/workloads/scrub.yaml']) ] ], ]) def test_yamls_from_desc(self, desc, base, result): res = list(run.descr_to_yamls(desc, base)) print(f"Result: {res}") assert res == result

I took the example you provided, but it would be more general if you check other test for matrix and build_matrix...

kshtsk · 2024-08-02T19:26:47Z

teuthology/suite/test/test_run_.py

+        assert(X ==
+            [
+                ("rados/singleton-nomsgr/{all/health-warnings mon_election/connectivity rados supported-random-distro$/{ubuntu_latest}}",
+                 ['/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/all/health-warnings.yaml',


I think for test cases' data it is better to use impersonal path naming, but I am not hundred percent sure about this, don't thank.

kshtsk · 2024-08-05T20:25:34Z

teuthology/suite/run.py

        if self.args.subset:
            job_config.subset = '/'.join(str(i) for i in self.args.subset)
+        if self.args.descr:
+            job_config.suite = "rerun"


instead of "rerun" maybe it is better to define somehow a suite name from the descriptions, but this is another question, another good question is it supposed to have scheduled descriptions from different suites, this is very interesting.

kshtsk · 2024-08-05T20:27:15Z

teuthology/suite/run.py

@@ -564,6 +609,44 @@ def check_num_jobs(self, jobs_to_schedule):
        if threshold and jobs_to_schedule > threshold:
            util.schedule_fail(msg, dry_run=self.args.dry_run)

+    def prepare_configs(self):
+        suite_name = self.base_config.suite or "rerun"


see the comment above, about detection of suite name for the description

kshtsk · 2024-08-05T20:34:27Z

teuthology/suite/run.py

@@ -574,29 +657,14 @@ def schedule_suite(self):
            log.debug("Using '%s' as an arch" % arch)
        else:
            arch = util.get_arch(self.base_config.machine_type)
-        suite_name = self.base_config.suite
-        suite_path = os.path.normpath(os.path.join(
+        suite_name = self.base_config.suite or "rerun"


Same, suite_name should remain suite name, no "rerun" please.

aclamk closed this Jul 8, 2024

aclamk reopened this Jul 9, 2024

kshtsk reviewed Jul 10, 2024

View reviewed changes

aclamk force-pushed the wip-aclamk-rerun-by-description branch 2 times, most recently from ee7248b to bdedd70 Compare July 19, 2024 12:13

kshtsk reviewed Jul 24, 2024

View reviewed changes

aclamk force-pushed the wip-aclamk-rerun-by-description branch from bdedd70 to 36d0c59 Compare August 2, 2024 15:52

aclamk force-pushed the wip-aclamk-rerun-by-description branch from 36d0c59 to 48c2263 Compare August 2, 2024 16:00

kshtsk reviewed Aug 2, 2024

View reviewed changes

kshtsk reviewed Aug 5, 2024

View reviewed changes

		@@ -27,6 +27,32 @@

		log = logging.getLogger(__name__)

		def descr_to_yamls(descriptions, suites_path):

teuthology/schedule: Add "descr" option #1973

Are you sure you want to change the base?

teuthology/schedule: Add "descr" option #1973

Conversation

aclamk commented Jul 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kshtsk Aug 2, 2024 •

edited

Loading