Skip to content

Extend librbdfio.py with a new workloads block. Add minor fixes.#306

Merged
perezjosibm merged 1 commit intoceph:masterfrom
perezjosibm:wip.fio-parse-jsons
Jul 23, 2024
Merged

Extend librbdfio.py with a new workloads block. Add minor fixes.#306
perezjosibm merged 1 commit intoceph:masterfrom
perezjosibm:wip.fio-parse-jsons

Conversation

@perezjosibm
Copy link
Contributor

@perezjosibm perezjosibm commented May 2, 2024

This is my first PR, please be gentle in the review. We add the following new features into the CBT for the benchmark librbdfio:

  • Extend the test plan .yml with a new block section 'workloads' to allow a list of performance tests to be indicated, each with its own range of iodepth and numjobs to iterate. An example of this is provided in example/rbd_fio_test.yml

  • Extend librbdfio.py with support for such 'workloads' block.

New workloads block

The value of this new workloads block is to allow a finer description of a test job, for example, indicating a range for the iodepth and numjobs FIO parameters, as well as whether to monitor the execution (eg. with perf, top, or collectl). This allow the collection of measurements for computing a response curve.
Here is a snippet that describes:

  • a precondition phase consisting on a random write workload, that is not being monitored, then
  • a random read workload over the ranges indicated for the FIO parameters numjobs and iodepth
  • a random write workload.
 workloads:
    precond1:
      jobname: 'precond1rw'
      mode: 'randwrite'
      numjobs: [ 1 ]
      iodepth: [ 8 ]
      monitor: False
    test1:
      jobname: 'rr'
      mode: 'randread'
      numjobs: [ 1, 4, 8 ]
      iodepth: [ 1, 4, 16]
    test2:
      jobname: 'rw'
      mode: 'randwrite'
      numjobs: [ 4 ]
      iodepth: [ 16 ]

The following snippet shows the execution of the precondition phase:
Screenshot 2024-05-07 at 16 07 38

The following snippet shows the execution of the tests1 and 2:
Screenshot 2024-05-07 at 16 08 14

The following snippet shows the resulting files from the execution of the test:
Screenshot 2024-05-13 at 11 56 20

@perezjosibm perezjosibm force-pushed the wip.fio-parse-jsons branch from c0674af to 5259d8b Compare May 13, 2024 11:21
@perezjosibm perezjosibm marked this pull request as ready for review May 13, 2024 11:21
@perezjosibm
Copy link
Contributor Author

@markhpc : here is my PR for the CBT extensions, would you be able to review please? thanks!

@neha-ojha neha-ojha requested review from markhpc and sseshasa May 20, 2024 14:59
@perezjosibm perezjosibm force-pushed the wip.fio-parse-jsons branch from 5259d8b to 172ecf4 Compare May 21, 2024 10:47
@sseshasa
Copy link
Contributor

@perezjosibm I have started taking a look at this. Please allow me some time to understand the changes. I plan to complete the review by the end of this week or in the worst case early next week due to the long weekend here.

Copy link
Contributor

@sseshasa sseshasa left a comment

Choose a reason for hiding this comment

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

I have added my first set of review comments. Will continue the review.

@sseshasa
Copy link
Contributor

I have added my first set of review comments. Will continue the review.

Hmm...looks like my initial comments were lost. I will add them again. Sorry about that.

Copy link
Contributor

@sseshasa sseshasa left a comment

Choose a reason for hiding this comment

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

Okay, now my first set of comments are added. Will continue reviewing this.

Copy link
Contributor

@sseshasa sseshasa left a comment

Choose a reason for hiding this comment

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

I am done with the first pass and have provided my comments.

  • I mainly focused on the main parts of the code changes related to benchmark.
  • One important recommendation is to ensure through testing that the original behavior of librbdfio benchmark is preserved

cbt.py Outdated
#logger.debug("Settings.cluster.is_teuthology:%s",settings.cluster.get('is_teuthology', False))
# Run the benchmarks
return_code = 0

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove newline.

sys.exit(NOTOK)


# next_branch: token from the input path
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the changes in this file is formatting related. I'd suggest this change alone be moved to a different commit along with an explanation of why changes were made to this file.

@@ -0,0 +1,382 @@
#!/usr/bin/python3
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this is a standalone tool, I'd suggest the following:

  1. Introduce this tool as part of a separate commit.
  2. Move it to it's own directory under 'tools'.
  3. Include a README.md in the new directory with details around the usage, examples, charts and so on as you have described in this file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, yes I'll do that asap

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sseshasa: I've opened PR #307 for only the new script, thanks!

Copy link
Contributor

@sseshasa sseshasa Jun 5, 2024

Choose a reason for hiding this comment

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

Hi @perezjosibm I mean introduce the new script in a new commit as a part of this PR(#306) itself. I will take look again once you have pushed all the requested changes. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @sseshasa sorry about that, I hope you don't mind reviewing them separately. I have updated this PR with the requested changes to the best of my understanding. I did a git ci -s -a --amend to have them all in a single batch. Many thanks.

@perezjosibm perezjosibm force-pushed the wip.fio-parse-jsons branch from 172ecf4 to ea446a9 Compare June 5, 2024 15:12
@perezjosibm perezjosibm requested a review from sseshasa June 5, 2024 15:15
@perezjosibm perezjosibm changed the title Add fio-parse-jsons.py to parse a set of json FIO files. Extend librb… Extend librbdfio.py with a new workloads block. Add minor fixes. Jun 5, 2024
@perezjosibm
Copy link
Contributor Author

Tested the remote path by running CBT from my laptop, connecting to the remote nodes:
Screenshot 2024-06-05 at 12 51 09

@perezjosibm perezjosibm force-pushed the wip.fio-parse-jsons branch from ea446a9 to eb0670b Compare June 5, 2024 19:15
@perezjosibm
Copy link
Contributor Author

Apologies, I just fixed some subtle bugs I introduced when doing the changes that didn't show up in the preliminary tests, I need to push the fixes again, sorry for the disturbance!

@perezjosibm
Copy link
Contributor Author

Hi @sseshasa changes are ready for review please, many thanks!

Copy link
Contributor

@sseshasa sseshasa left a comment

Choose a reason for hiding this comment

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

Just added a couple of new review comments. But this looks good to me. I would prefer another pair of eyes to have a look at this as well. @markhpc can you please review this?

Signed-off-by: Jose J Palacios-Perez <perezjos@uk.ibm.com>
@perezjosibm perezjosibm force-pushed the wip.fio-parse-jsons branch from eb0670b to 5823498 Compare July 22, 2024 16:17
@perezjosibm
Copy link
Contributor Author

Hi @Matan-B wonder if you could please review/approve this PR?, Thanks

@perezjosibm perezjosibm merged commit 655afe2 into ceph:master Jul 23, 2024
@perezjosibm perezjosibm deleted the wip.fio-parse-jsons branch July 23, 2024 15:39
@harriscr harriscr mentioned this pull request May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants