mount: Add a stress test with multiple clients #25

Shwetha-Acharya · 2023-09-07T13:21:23Z

This test simulates multiple clients performing file operations using the concept of threading.

synarete · 2023-09-07T14:40:01Z

testcases/mount/mount_stress.py

+
+num_clients = 10
+num_operations = 50
+file_size = 1024  # 1 KB


Two comments:

Python supports 2 ** 10 notation (2 to the power of 10), thus no need for extra comment

I would vote for later file size in stress-test, say 8M. With such small file-size each thread may end its I/O before the other had a chance to run.

synarete · 2023-09-07T14:42:26Z

testcases/mount/mount_stress.py

+def _generate_random_string(length: int) -> str:
+    return "".join(random.choice(string.ascii_letters) for _ in range(length))


This is ok for small files but if enlarging the file size (to 8M) this would be very inefficient. In mount_io.py I implemented a pseudo-random bytes sequence generator called _generate_random_bytes. Maybe we should make it public in common helper module.

Sent PR: #27 to address this

synarete · 2023-09-07T14:44:53Z

testcases/mount/mount_stress.py

+            with open(os.path.join(root_dir, filename), "w") as file:
+                file.write(file_content)
+
+            with open(os.path.join(root_dir, filename), "r") as file:
+                file.read()
+
+            os.remove(os.path.join(root_dir, filename))


I would prefer using Python's modern pathlib, as it makes more compact code (without duplication). Something like:

path = pathlib.Path(root_dir, filename) path.write_text(file_content) file_content_out = path.read_text() if file_content_out != file_content: raise IOError("content mismatch") path.unlink()

Note that is this code fragment I also check data consistency.

synarete · 2023-09-07T14:47:58Z

testcases/mount/mount_stress.py

+    for i in range(num_clients):
+        thread = threading.Thread(
+            target=_perform_file_operations, args=(i, root_dir)
+        )
+        threads.append(thread)
+        thread.start()


Maybe have another loop for thread.start to ensure maximum parallelism. That is, so that no thread will start running while we are still constructing the initial threads-set.

(This one is only nice to have).

synarete · 2023-09-07T14:49:14Z

testcases/mount/test_mount.py

@@ -30,6 +31,7 @@ def mount_check(ipaddr: str, share_name: str) -> None:
        os.mkdir(test_dir)
        check_io_consistency(test_dir)
        check_dbm_consistency(test_dir)
+        stress_test(test_dir)


Consider rename to check_mt_stress or something like it (align names with other test functions).

synarete · 2023-09-07T14:50:50Z

testcases/mount/mount_stress.py

+num_clients = 10
+num_operations = 50
+file_size = 1024  # 1 KB


Would be nice to have those parameters as arguments. Perhaps even run the test multiple times with different set of args.

Shwetha-Acharya · 2023-09-11T11:08:53Z

I will post the next set of changes corresponding to your comments after the PR: #27 gets merged @synarete , so that the CI runs will not fail

spuiuk · 2023-09-15T13:52:19Z

Thanks for doing this. I am wondering if it is possible to split this out from the current test? The simple mount test which I thought of removing at one time has now evolved to perform io tests and multi mount tests. Maybe we should instead split it into separate io and multimount tests.

Shwetha-Acharya · 2023-09-19T10:35:17Z

Thanks for doing this. I am wondering if it is possible to split this out from the current test? The simple mount test which I thought of removing at one time has now evolved to perform io tests and multi mount tests. Maybe we should instead split it into separate io and multimount tests.

Yes, that can be done, I can take it up as a next PR: #30

spuiuk · 2023-09-19T11:29:30Z

Having had a quick chat with Shweta, we can consider setting up each of these tests as a separate pytest with a similar setup as described in
https://stackoverflow.com/questions/7786648/how-to-call-setup-once-for-all-tests-and-teardown-after-all-are-finished

Shwetha-Acharya · 2023-09-20T12:13:20Z

@synarete addressed your comments, though regression will fail (as we have few dependent PRs under review), added next change set to keep this PR open for general review to be in progress.

Shwetha-Acharya · 2023-09-21T06:58:36Z

/retest all

synarete · 2023-09-21T09:17:02Z

testcases/mount/mount_stress.py

+            path.write_text(file_content.decode())
+            file_content_out = path.read_text()


Why do you prefer write_text/read_text instead of write_bytes/read_bytes ? After all, file_content is bytes, so what is the added value of converting it to text?

That makes more sense, updated.

synarete · 2023-09-21T09:20:41Z

testcases/mount/mount_stress.py

+    # Case-1: num_clients = 5, num_operations = 20, file_size = 2**22
+    _stress_test(root_dir, 5, 20, 2**22)
+    # Case-2: num_clients = 10, num_operations = 30, file_size = 2**23
+    _stress_test(root_dir, 10, 30, 2**23)
+    # Case-3: num_clients = 15, num_operations = 40, file_size = 2**24
+    _stress_test(root_dir, 10, 40, 2**24)
+    # Case-4: num_clients = 50, num_operations = 50, file_size = 2**25
+    _stress_test(root_dir, 10, 25, 2**25)


Comments are inconsistent with actual function call.
Consider using Python's explicit argument names instead of redundant comments. Something like:
_stree_test(root_dir, num_clients=5, num_operations=20, file_size=2**22)

I was experimenting with the values, forgot to edit the comments.

phlogistonjohn · 2023-09-21T13:21:39Z

testcases/mount/mount_stress.py

+    print("Stress test complete.")
+
+
+def check_mnt_stress(root_dir: str) -> None:


If this were a test we could consider using pytest's parametrize feature for generating the various combinations.

Added a tracker in issue-30

phlogistonjohn · 2023-09-21T13:25:06Z

testcases/mount/mount_stress.py

+        root_dir, num_clients=20, num_operations=40, file_size=2**24
+    )
+    _stress_test(
+        root_dir, num_clients=15, num_operations=25, file_size=2**25


If I did my math right this means were creating files on the multiple gigabytes range, right? If so, I'm a little leery of using APIs like write_bytes that have to consume a single huge buffer in memory. I would much rather see a loop where the buffer size is something more typical like 4k and then write() out each chunk in a loop. In my past experience we had a python program reading files from http into a single huge buffer and that ended up taxing the memory of the system, when changing it to a read-write-loop it was much more manageable.

@phlogistonjohn I am a bit confused here: the largest file-size here is 32M, and the overall data-set size if up 480M. Those numbers looks reasonably. In all odds, if we want to split those write/reads into multiple chunks, I would vote for larger chunk sized (say, 1M).

I really can't do math. You are right about the sizes.

I'm fine with the code for now, I just don't find the use-one-buffer approach for all file sizes scalable and so I tend to shy away from it. I'd be open to a buffer size of 1M.

synarete

Need one final fix.

synarete · 2023-10-04T09:41:27Z

testcases/mount/mount_stress.py

+            path.write_bytes(file_content)
+            file_content_out = path.read_text()


path.write_bytes should be paired with path.read_bytes (instead of path.read_text). We the current code, I have no idea how this test run successfully given that we use random-bytes for file_content.

This test simulates multiple clients performing file operations using the concept of threading. Four cases with different number of clients, file operations and file sizes are included to ensure maximum coverage. Signed-off-by: Shwetha K Acharya <Shwetha.K.Acharya@ibm.com>

spuiuk · 2023-10-18T11:40:17Z

/retest all

spuiuk · 2023-10-18T11:48:20Z

samba-in-kubernetes/sit-environment#51 reported for CI reporting issues.

spuiuk

ACK

With this change, multiple test modules triggered from testcases/mount can be run as individual tests complying to pytest standards. Also any number of new tests can be added efficiently on the mount. Old approach: - All the different tests are called from test_mount.py: https://github.com/samba-in-kubernetes/sit-test-cases/blob/main/testcases/mount/test_mount.py - Test_mount.py creates a mount, the required tests are called and then test_mount.py cleans the mount. New approach : - This PR complies with the proposal to use fixtures in order to remove all the load on one single module: samba-in-kubernetes#25 (comment) samba-in-kubernetes#25 (comment) - The tests are no more called from test_mount.py. - Setting up of mount and tearing down of mount is taken care by setup and teardown fixture which is defined in testcases/mount/conftest.py More information about fixtures: https://docs.pytest.org/en/6.2.x/fixture.html - With this change, test files remain the same, but the initiator function starts with keyword test and the each test module starts with keyword 'test' That way the tests are called by pytest. Usage of setup and teardown fixture becomes easily possible. We no more need to to call the test function explictly. This also helps with easy debugging and better understanding of the code, as the tests will be failed at individual mount/IO test. In this case, pytest clearly points which test among test_mount_io.py, test_mount_dbm.py and test_mount_stress.py are failing. Can be very useful - when more and more tests are added - when we want to parse test results in sit-environment project Fixes: samba-in-kubernetes#30 Signed-off-by: Shwetha K Acharya <Shwetha.K.Acharya@ibm.com>

With this change, multiple test modules triggered from testcases/mount can be run as individual tests complying to pytest standards. Also any number of new tests can be added efficiently on the mount. Old approach: - All the different tests are called from test_mount.py: https://github.com/samba-in-kubernetes/sit-test-cases/blob/main/testcases/mount/test_mount.py - Test_mount.py creates a mount, the required tests are called and then test_mount.py cleans the mount. New approach : - This PR complies with the proposal to use fixtures in order to remove all the load on one single module: #25 (comment) #25 (comment) - The tests are no more called from test_mount.py. - Setting up of mount and tearing down of mount is taken care by setup and teardown fixture which is defined in testcases/mount/conftest.py More information about fixtures: https://docs.pytest.org/en/6.2.x/fixture.html - With this change, test files remain the same, but the initiator function starts with keyword test and the each test module starts with keyword 'test' That way the tests are called by pytest. Usage of setup and teardown fixture becomes easily possible. We no more need to to call the test function explictly. This also helps with easy debugging and better understanding of the code, as the tests will be failed at individual mount/IO test. In this case, pytest clearly points which test among test_mount_io.py, test_mount_dbm.py and test_mount_stress.py are failing. Can be very useful - when more and more tests are added - when we want to parse test results in sit-environment project Fixes: #30 Signed-off-by: Shwetha K Acharya <Shwetha.K.Acharya@ibm.com>

Shwetha-Acharya requested review from spuiuk and synarete and removed request for spuiuk and synarete September 7, 2023 13:21

Shwetha-Acharya marked this pull request as draft September 7, 2023 13:24

Shwetha-Acharya force-pushed the add-stress-test branch from c32013d to 461783b Compare September 7, 2023 13:41

synarete reviewed Sep 7, 2023

View reviewed changes

Shwetha-Acharya marked this pull request as ready for review September 11, 2023 11:23

Shwetha-Acharya force-pushed the add-stress-test branch from 461783b to 51f7601 Compare September 20, 2023 12:09

Shwetha-Acharya force-pushed the add-stress-test branch 2 times, most recently from cfc75c0 to a3fd16e Compare September 21, 2023 07:52

Shwetha-Acharya requested review from spuiuk, phlogistonjohn, anoopcs9 and synarete September 21, 2023 08:40

synarete reviewed Sep 21, 2023

View reviewed changes

Shwetha-Acharya force-pushed the add-stress-test branch 3 times, most recently from 19af979 to 8dea54b Compare September 21, 2023 11:38

phlogistonjohn reviewed Sep 21, 2023

View reviewed changes

Shwetha-Acharya requested review from synarete and phlogistonjohn October 4, 2023 07:51

synarete reviewed Oct 4, 2023

View reviewed changes

spuiuk approved these changes Oct 19, 2023

View reviewed changes

spuiuk merged commit 62c7c6f into samba-in-kubernetes:main Oct 19, 2023
6 of 7 checks passed

Shwetha-Acharya deleted the add-stress-test branch November 23, 2023 07:06

Shwetha-Acharya mentioned this pull request Nov 23, 2023

Introduce setup and teardown fixtures #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mount: Add a stress test with multiple clients #25

mount: Add a stress test with multiple clients #25

Shwetha-Acharya commented Sep 7, 2023

synarete Sep 7, 2023

synarete Sep 7, 2023

Shwetha-Acharya Sep 11, 2023

synarete Sep 7, 2023

synarete Sep 7, 2023

synarete Sep 7, 2023

synarete Sep 7, 2023

Shwetha-Acharya commented Sep 11, 2023 •

edited

Loading

spuiuk commented Sep 15, 2023

Shwetha-Acharya commented Sep 19, 2023 •

edited

Loading

spuiuk commented Sep 19, 2023

Shwetha-Acharya commented Sep 20, 2023 •

edited

Loading

Shwetha-Acharya commented Sep 21, 2023

synarete Sep 21, 2023

Shwetha-Acharya Sep 21, 2023

synarete Sep 21, 2023

Shwetha-Acharya Sep 21, 2023

phlogistonjohn Sep 21, 2023

Shwetha-Acharya Sep 22, 2023

phlogistonjohn Sep 21, 2023

synarete Sep 21, 2023

phlogistonjohn Sep 21, 2023

synarete left a comment

synarete Oct 4, 2023

spuiuk commented Oct 18, 2023

spuiuk commented Oct 18, 2023

spuiuk left a comment

		def _generate_random_string(length: int) -> str:
		return "".join(random.choice(string.ascii_letters) for _ in range(length))

		path.write_text(file_content.decode())
		file_content_out = path.read_text()

		print("Stress test complete.")


		def check_mnt_stress(root_dir: str) -> None:

		path.write_bytes(file_content)
		file_content_out = path.read_text()

mount: Add a stress test with multiple clients #25

mount: Add a stress test with multiple clients #25

Conversation

Shwetha-Acharya commented Sep 7, 2023

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

Shwetha-Acharya commented Sep 11, 2023 • edited Loading

spuiuk commented Sep 15, 2023

Shwetha-Acharya commented Sep 19, 2023 • edited Loading

spuiuk commented Sep 19, 2023

Shwetha-Acharya commented Sep 20, 2023 • edited Loading

Shwetha-Acharya commented Sep 21, 2023

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

synarete left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spuiuk commented Oct 18, 2023

spuiuk commented Oct 18, 2023

spuiuk left a comment

Choose a reason for hiding this comment

Shwetha-Acharya commented Sep 11, 2023 •

edited

Loading

Shwetha-Acharya commented Sep 19, 2023 •

edited

Loading

Shwetha-Acharya commented Sep 20, 2023 •

edited

Loading