Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

teuthology/schedule: Add "descr" option #1973

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

teuthology/schedule: Add "descr" option #1973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions scripts/suite.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
usage: teuthology-suite --help
teuthology-suite [-v | -vv ] --suite <suite> [options] [<config_yaml>...]
teuthology-suite [-v | -vv ] --rerun <name> [options] [<config_yaml>...]
teuthology-suite [-v | -vv ] --descr <descr> [options] [<config_yaml>...]

Run a suite of ceph integration tests. A suite is a directory containing
facets. A facet is a directory containing config snippets. Running a suite
Expand Down Expand Up @@ -126,6 +127,7 @@
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 <list> Schedule a suite based on comma separated list of descriptions.
-p <priority>, --priority <priority>
Job priority (lower is sooner)
[default: 1000]
Expand Down
110 changes: 89 additions & 21 deletions teuthology/suite/run.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,45 @@

log = logging.getLogger(__name__)

def descr_to_yamls(descriptions, suites_path):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

"""
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.
"""
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

def expand_descr(test_yamls, source, prefix, base_pos):
"""
Expand description using production rules (explanation, not Chomsky context-free formalism):
rule 1: (simplification)
"A{BX}" => AB A{X}
rule 2: (termination)
"A" => "suites_pathA.yaml"
"""
pos = base_pos
while (pos < len(source)):
if source[pos] == '{':
more_prefix=source[base_pos:pos]
pos = expand_descr(test_yamls, source, prefix + more_prefix, pos + 1)
base_pos = pos
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:
pos = pos + 1
result = []
desc_tab = descriptions.split(',')
for d in desc_tab:
dd = d.strip()
Comment on lines +63 to +64
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])

test_yamls = []
expand_descr(test_yamls, dd, "", 0)
result.append((dd, test_yamls))
return result

class Run(object):
WAIT_MAX_JOB_TIME = 30 * 60
Expand Down Expand Up @@ -70,7 +109,7 @@ def make_run_name(self):
[
self.user,
str(self.timestamp),
self.args.suite,
self.args.suite or "rerun",
self.args.ceph_branch,
self.args.kernel_branch or '-',
self.args.flavor, worker
Expand Down Expand Up @@ -357,8 +396,14 @@ def build_base_config(self):
job_config.timestamp = self.timestamp
job_config.priority = self.args.priority
job_config.seed = self.args.seed
if self.args.subset and self.args.descr:
util.schedule_fail("--subset is not compatible with --descr")
if self.args.suite and self.args.descr:
util.schedule_fail("--suite is not compatible with --descr")
if self.args.subset:
job_config.subset = '/'.join(str(i) for i in self.args.subset)
if self.args.descr:
job_config.suite = "rerun"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if self.args.email:
job_config.email = self.args.email
if self.args.owner:
Expand Down Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

suites_path = os.path.normpath(os.path.join(
self.suite_repo_path,
self.args.suite_relpath,
'suites'
))
suite_path = os.path.normpath(os.path.join(
suites_path,
suite_name.replace(':', '/'),
))
log.debug('Suites in %s' % (suites_path))
if self.args.descr:
log.debug(f'Rerun by description in {suites_path} in %s')
configs = descr_to_yamls(self.args.descr, suites_path)
use_suite_name = None
generated = len(configs)
log.info(f'Rerun from description in {suite_path} generated {generated} jobs')
else:
log.debug('Suite %s in %s' % (suite_name, suites_path))
log.debug(f"subset = {self.args.subset}")
log.debug(f"no_nested_subset = {self.args.no_nested_subset}")
configs = build_matrix(suite_path,
subset=self.args.subset,
no_nested_subset=self.args.no_nested_subset,
seed=self.args.seed)
use_suite_name = self.base_config.suite
generated = len(configs)
log.info(f'Suite {suite_name} in {suites_path} generated {generated} jobs (not yet filtered or merged)')
configs = list(config_merge(configs,
filter_in=self.args.filter_in,
filter_out=self.args.filter_out,
filter_all=self.args.filter_all,
filter_fragments=self.args.filter_fragments,
seed=self.args.seed,
suite_name=use_suite_name))
return configs

def schedule_suite(self):
"""
Schedule the suite-run. Returns the number of jobs scheduled.
Expand All @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

suites_path = os.path.normpath(os.path.join(
self.suite_repo_path,
self.args.suite_relpath,
'suites',
self.base_config.suite.replace(':', '/'),
'suites'
))
log.debug('Suite %s in %s' % (suite_name, suite_path))
log.debug(f"subset = {self.args.subset}")
log.debug(f"no_nested_subset = {self.args.no_nested_subset}")
configs = build_matrix(suite_path,
subset=self.args.subset,
no_nested_subset=self.args.no_nested_subset,
seed=self.args.seed)
configs = self.prepare_configs()
generated = len(configs)
log.info(f'Suite {suite_name} in {suite_path} generated {generated} jobs (not yet filtered or merged)')
configs = list(config_merge(configs,
filter_in=self.args.filter_in,
filter_out=self.args.filter_out,
filter_all=self.args.filter_all,
filter_fragments=self.args.filter_fragments,
seed=self.args.seed,
suite_name=suite_name))

if self.args.dry_run:
log.debug("Base job config:\n%s" % self.base_config)
Expand Down Expand Up @@ -696,7 +764,7 @@ def schedule_suite(self):
total_count *= self.args.num
log.info(
'Suite %s in %s scheduled %d jobs.' %
(suite_name, suite_path, count)
(suite_name, suites_path, count)
)
log.info('%d/%d jobs were filtered out.',
(generated - count),
Expand Down
49 changes: 49 additions & 0 deletions teuthology/suite/test/test_run_.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,3 +360,52 @@ def test_newest_success(
m_find_git_parents.assert_has_calls(
[call('ceph', 'ceph_sha1', 10)]
)

def test_dupa(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

self
):
X = run.descr_to_yamls(
"rados/objectstore/{backends/objectstore-bluestore-b supported-random-distro$/{ubuntu_latest}}",
"/cephfs/github.com_ceph_build/qa/suites"
)
assert(X ==
[
('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'])
]
)

X = run.descr_to_yamls(
"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"
)
print(str(X))
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',
Copy link
Contributor

@kshtsk kshtsk Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

'/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/mon_election/connectivity.yaml',
'/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/rados.yaml',
'/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_aclamk-testing/qa/suites/rados/singleton-nomsgr/supported-random-distro$/ubuntu_latest.yaml'])
]
)

X = run.descr_to_yamls(
"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/akupczyk/src/git.ceph.com_ceph-c_squid/qa/suites"
)
assert(X ==
[
("rados/cephadm/osds/{0-distro/centos_9.stream_runc 0-nvme-loop 1-start 2-ops/rm-zap-flag}",
['/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/0-distro/centos_9.stream_runc.yaml',
'/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/0-nvme-loop.yaml',
'/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_squid/qa/suites/rados/cephadm/osds/1-start.yaml',
'/cephfs/home/akupczyk/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/akupczyk/src/git.ceph.com_ceph-c_squid/qa/suites/rados/standalone/supported-random-distro$/ubuntu_latest.yaml',
'/cephfs/home/akupczyk/src/git.ceph.com_ceph-c_squid/qa/suites/rados/standalone/workloads/scrub.yaml'])
]
)
Loading