Allow time within a test to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML #327

lee-j-sanders · 2025-02-17T16:37:46Z

This PR only changes the functionality of workloads in CBT.

This PR adds two pieces of functionality:

1) Workloads with randrw only let you specify 1 mixed workload within a YAML
   for a given block size,  If you wanted to do 64k 7030 and 64k 3070, you couldn't, as
   the same directory name was used for subsequent randrw workloads.
   Thus overwriting all the previous resuilts in the archive directory with
   the later tests. This commit adds the readwrite ratio into the directory
   name if the mode is randrw thus creating a unique directory name per test for
  each block size.

  The directory name for 100% read and 100% write tests are unaffected.

2) Prior to this PR, all workloads inherited "time" from the
   outer/global part of the YAML. This meant you couldn't set different
   time for each test within the "workloads". Typically for
   preconditioning you'd want to precondition for a set amount of time (
   600 seconds - 10minutes), then set the workload time to be 120 seconds (2 minutes).

By default, if you do not set a "time" within the workload, the time for that
specific test will be inherited from the librbdfio "time" within the YAML.

Here is an example of usage:

 librbdfio:
    time: 10
    ramp: 10
    time_based: True
    norandommap: True
    vol_size: 1000
    use_existing_volumes: True
    procs_per_volume: [1]
    volumes_per_client: [8]
    osd_ra: [4096]
    cmd_path: '/usr/local/bin/fio'
    poolname: 'rbd_replicated'
    log_iops: True
    log_bw:  True
    log_lat: True
    fio_out_format: 'json'
    log_avg_msec: 100
    prefill:
      blocksize: '64k'
      numjobs: 1

    # Each block below uses its own local options during its execution
    workloads:
      precondition:
        jobname: 'precond1rw'
        mode: 'randwrite'
        op_size: 65536
        numjobs: [ 1 ]
        total_iodepth: [ 16 ]
        monitor: False # whether to run the monitors along the test
        time: 8
      seq32kwrite:
        jobname: 'seqwrite'
        mode: 'write'
        op_size: 32768
        numjobs: [ 1 ]
        total_iodepth: [ 1, 2, 3, 4, 5, 8, 16, 24, 32, 64, 128 ]
      16k7030:
        jobname: 'randmix'
        mode: 'randrw'
        op_size: 16384
        rwmixread: 70
        numjobs: [ 1 ]
        total_iodepth: [ 1, 2, 3, 4, 5, 8, 16, 24, 32, 64, 128 ]
        time: 5

precondition will use 8 seconds for the single test with queue depth 16.
seq32kwrite will use the default of 10 seconds for all tests in total_iodepth[] specified in the librbdfio global section.
16k7030 will use 5 seconds for all tests in total_iodepth[].

Signed-off-by: Chris Harris([email protected]

Signed-off-by: CHris Harris([email protected])

…allow multiple mixed read/write workloads within a YAML This commit only changes the functionality of workloads in CBT. This commit adds two pieces of functionality: 1) Workloads with randrw only lets you specify 1 mixed workload within a YAML as the same directory name was used for subsequent randrw workloads, thus overwriting all the previous resuilts in the archive directory with the later tests. This commit adds the readwrite ratio into the directory name if the mode is randrw. The directory name for 100% read and 100% write tests are unaffected. 2) Prior to this commit, all workloads inherited "time" from the outer/global part of the YAML. This meant you couldn't set different time for each test within the "workloads". Typically for preconditioning you'd want to precondition for a set amount of time ( 600 seconds - 10minutes), then set the workload time to be 120 seconds (2 minutes). If you do not set a "time" within the workload, the time for that specific test will be inherited from the global "time" within the YAML. Signed-off-by: lee-j-sanders <[email protected]>

lee-j-sanders · 2025-02-17T16:52:38Z

PYTHONPATH=. python3 -m pytest -p no:cacheprovider tests/
========================================================================================= test session starts ==========================================================================================
platform linux -- Python 3.9.21, pytest-8.3.4, pluggy-1.5.0
rootdir: /cbt.lee
collected 329 items

tests/test_benchmarkfactory.py ...                                                                                                                                                               [  0%]
tests/test_bm_cephtestrados.py ..................                                                                                                                                                [  6%]
tests/test_bm_fio.py .........................................                                                                                                                                   [ 18%]
tests/test_bm_getput.py ..............................                                                                                                                                           [ 27%]
tests/test_bm_hsbench.py ..............................                                                                                                                                          [ 37%]
tests/test_bm_kvmrbdfio.py ...................................                                                                                                                                   [ 47%]
tests/test_bm_librbdfio.py ...................................................                                                                                                                   [ 63%]
tests/test_bm_nullbench.py ............                                                                                                                                                          [ 66%]
tests/test_bm_radosbench.py ...............................                                                                                                                                      [ 76%]
tests/test_bm_rawfio.py ................................                                                                                                                                         [ 86%]
tests/test_bm_rbdfio.py ....................................                                                                                                                                     [ 96%]
tests/test_common.py .sss                                                                                                                                                                        [ 98%]
tests/test_common_output_formatter.py ......                                                                                                                                                     [100%]

=========================================================================================== warnings summary ===========================================================================================
post_processing/formatter/test_run_result.py:21
  /cbt.lee/post_processing/formatter/test_run_result.py:21: PytestCollectionWarning: cannot collect test class 'TestRunResult' because it has a __init__ constructor (from: tests/test_common_output_formatter.py)
    class TestRunResult:

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================================================================== 326 passed, 3 skipped, 1 warning in 0.51s ===============================================================================
[root@cephalasquad6 cbt.lee]#

lee-j-sanders · 2025-02-17T18:02:26Z

Note: there are changes for PR - #324 showing up in the commit that is because I'm testing both together. I do not plan on merging this PR until PR324 is merged.

harriscr

There is a slightly nicer way to do the code at lines 201 - 209. Instead of:

if self.mode == 'randrw':
    self.rwmixread = test['rwmixread']
    self.rwmixwrite = 100 - self.rwmixread
    self.run_dir =  ( f'{self.base_run_dir}/{self.mode}{self.rwmixread}{self.rwmixwrite}_{int(self.op_size)}/'
                               f'iodepth-{int(self.iodepth):03d}/numjobs-{int(self.numjobs):03d}' )
else:
    self.run_dir =  ( f'{self.base_run_dir}/{self.mode}_{int(self.op_size)}/'
                                f'iodepth-{int(self.iodepth):03d}/numjobs-{int(self.numjobs):03d}' )

you could try:

if self.mode == 'randrw':
    self.rwmixread = test['rwmixread']
    self.rwmixwrite = 100 - self.rwmixread
    self.run_dir += f'{self.rwmixread}{self.rwmixwrite}'
self._run_dir += f'_{int(self.op_size)}/iodepth-{int(self.iodepth):03d}/numjobs-{int(self.numjobs):03d}'

Which does the same thing without the repeated code.

It is a small improvement though, and not one I'm going to insist on being made

lee-j-sanders · 2025-02-18T11:38:20Z

This is a rework of #326 which I decided to close as it is going to be tidier within a new PR.

perezjosibm

Suggested code improvements (smaller methods rather than bloating the existing code). Cheers

perezjosibm · 2025-02-27T16:40:36Z

benchmark/librbdfio.py

+            self._ioddepth_per_volume = self._calculate_iodepth_per_volume(
+                int(self.volumes_per_client), int(total_iodepth)
+            )
+


Not very clear why this is needed as a string rather than a numeric value (because it can be validated quickly). As a good practise, it might need its own method, with a short documentation in code.

perezjosibm · 2025-02-27T16:41:29Z

benchmark/librbdfio.py

@@ -90,6 +99,7 @@ def backup_global_fio_options(self):
        Backup/copy the FIO global options into a dictionary
        """
        self.global_fio_options['time_based'] = self.time_based
+        self.global_fio_options['time'] = self.time


Yes, this is the correct way to add new global FIO options that can be inherited to the workloads sections.

perezjosibm · 2025-02-27T16:55:08Z

benchmark/librbdfio.py

+                        self._ioddepth_per_volume = self._calculate_iodepth_per_volume(
+                            int(self.volumes_per_client), int(iod)
+                        )
+


Might sound like criticism, but this code is making the logic obscure rather than improving it, I'd suggest to define a new method that calculates this, with a meaningful name, for example:

def get_iodepth(self, test)->list[str]: """ Returns the appropriate iodepth list of values """ iodepth: list[str] = [] # your code here, ie with the proper identation self.use_total_iodepth: bool = False if "total_iodepth" in test.keys(): iodepth = test["total_iodepth"] self.use_total_iodepth = True else: iodepth = test["iodepth"] return iodepth

Now, the _calculate_iodepth_per_volume() can use the self. use_total_iodepth can test whether its enabled or not, so it make the code easier to understand.

perezjosibm · 2025-02-27T16:55:56Z

benchmark/librbdfio.py

+                    # Needed to allow for different mixed ratio results with the same block size, we
+                    # store the ratio within the directory name. Otherwise workloads would only support
+                    # 1 mixed workload for a given block size. For 100% read, 100% write don't need to
+                    # store the read/write ratio.
+
+                    if self.mode == 'randrw':
+                        self.rwmixread = test['rwmixread']
+                        self.rwmixwrite = 100 - self.rwmixread
+                        self.run_dir =  ( f'{self.base_run_dir}/{self.mode}{self.rwmixread}{self.rwmixwrite}_{int(self.op_size)}/'
+                                          f'iodepth-{int(self.iodepth):03d}/numjobs-{int(self.numjobs):03d}' )
+                    else:
+                        self.run_dir =  ( f'{self.base_run_dir}/{self.mode}_{int(self.op_size)}/'
+                                          f'iodepth-{int(self.iodepth):03d}/numjobs-{int(self.numjobs):03d}' )


This is also crying for its own method, with Chris' suggestion

perezjosibm · 2025-02-27T16:59:12Z

benchmark/librbdfio.py

+                    if use_total_iodepth:
+                        number_of_volumes = len(self._ioddepth_per_volume.keys())
+                    for i in range(number_of_volumes):


If you follow the suggestion above that the flag self.use_total_iodepth be an object's attribute, the condition can be encapsulated in a small method as well.

perezjosibm · 2025-02-27T17:00:10Z

benchmark/librbdfio.py

+        if self._ioddepth_per_volume != {}:
+            iodepth = f"{self._ioddepth_per_volume[volnum]}"


Probably a typo "ioddepth" -- why double 'd'? , but is used consistently

perezjosibm · 2025-02-27T17:04:52Z

benchmark/librbdfio.py

+                remainder -= 1
+            queue_depths[volume_id] = iodepth
+
+        return queue_depths


Since the queue_depths[] is always going to be assigned to the attribute self._ioddepth_per_volume, you might set it here instead. This is updating the object attribute (like the cummulative parameter technique in Functional Programming), and makes clearer the intention IMO

harriscr · 2025-03-03T11:28:53Z

I have made changes in PR 324 based on some of the comment here. I've added @perezjosibm as a reviewer to that one

It is probably worth separating the code changes here from the changes in PR 324

harriscr · 2025-03-13T07:50:08Z

Pulpito results:
crimson-rados/perf
rados/perf
perf-basic

Chris Harris and others added 3 commits January 30, 2025 15:44

Add a yotal_iodepth option to be slit over total volumes

a8bc00f

Signed-off-by: Chris Harris([email protected]

Merge branch 'ceph:master' into ch_wip_total_iodepth

7892fa9

Workload fixes for total_iodepth

dc1a795

Signed-off-by: CHris Harris([email protected])

lee-j-sanders changed the title ~~Allow time to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML~~ Allow time within a test to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML Feb 17, 2025

lee-j-sanders force-pushed the wip-ljs-yamlimprovements2 branch from a21b7c0 to e048c28 Compare February 17, 2025 16:39

lee-j-sanders self-assigned this Feb 17, 2025

lee-j-sanders requested review from perezjosibm and harriscr February 17, 2025 18:02

harriscr approved these changes Feb 18, 2025

View reviewed changes

perezjosibm requested changes Feb 27, 2025

View reviewed changes

harriscr mentioned this pull request Mar 3, 2025

Adding a total_iodepth option that can be used with librbdfio #324

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow time within a test to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML #327

Allow time within a test to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML #327

lee-j-sanders commented Feb 17, 2025 •

edited

Loading

lee-j-sanders commented Feb 17, 2025

lee-j-sanders commented Feb 17, 2025

harriscr left a comment

lee-j-sanders commented Feb 18, 2025

perezjosibm left a comment

perezjosibm Feb 27, 2025

perezjosibm Feb 27, 2025

perezjosibm Feb 27, 2025 •

edited

Loading

perezjosibm Feb 27, 2025

perezjosibm Feb 27, 2025

perezjosibm Feb 27, 2025

perezjosibm Feb 27, 2025 •

edited

Loading

harriscr commented Mar 3, 2025

harriscr commented Mar 13, 2025

		if self._ioddepth_per_volume != {}:
		iodepth = f"{self._ioddepth_per_volume[volnum]}"

Allow time within a test to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML #327

Are you sure you want to change the base?

Allow time within a test to override global time setting in YAML and allow multiple mixed read/write workloads within a YAML #327

Conversation

lee-j-sanders commented Feb 17, 2025 • edited Loading

lee-j-sanders commented Feb 17, 2025

lee-j-sanders commented Feb 17, 2025

harriscr left a comment

Choose a reason for hiding this comment

lee-j-sanders commented Feb 18, 2025

perezjosibm left a comment

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025

Choose a reason for hiding this comment

perezjosibm Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

harriscr commented Mar 3, 2025

harriscr commented Mar 13, 2025

lee-j-sanders commented Feb 17, 2025 •

edited

Loading

perezjosibm Feb 27, 2025 •

edited

Loading

perezjosibm Feb 27, 2025 •

edited

Loading