-
Notifications
You must be signed in to change notification settings - Fork 3
Basic vdo tests #10
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.
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
base: main
Are you sure you want to change the base?
Basic vdo tests #10
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| """VDO basic functional test. | ||
|
|
||
| Verifies VDO persistence by writing data to a filesystem on VDO, stopping | ||
| the VDO device, restarting it, and verifying the data is still readable. | ||
| """ | ||
| from dmtest.assertions import assert_equal, assert_string_in | ||
| from dmtest.utils import get_dmesg_log | ||
| from dmtest.vdo.utils import standard_vdo, standard_stack, mounted_fs | ||
| import dmtest.process as process | ||
| import time | ||
|
|
||
| def t_basic(fix): | ||
| """Basic VDO functional test: write files, stop/start VDO, verify data persists.""" | ||
| # Create VDO with slab_bits=17 (SLAB_BITS_SMALL) | ||
| with standard_vdo(fix, slab_bits=17) as vdo: | ||
| with mounted_fs(vdo.path, format=True) as mount_point: | ||
| # Create file foo1 with "Hello World" | ||
| file1 = f"{mount_point}/foo1" | ||
| process.run(f"bash -c 'echo Hello World > {file1}'") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need bash here? Is it for the file redirection handling? Wait, down on line 34 we don't invoke a shell. Can we be consistent? |
||
|
|
||
| # Create subdirectory dir2 | ||
| dir2 = f"{mount_point}/dir2" | ||
| process.run(f"mkdir {dir2}") | ||
|
|
||
| # Copy foo1 to dir2/foo2 | ||
| file2 = f"{dir2}/foo2" | ||
| process.run(f"cp {file1} {file2}") | ||
|
|
||
| # Copy foo1 to foo3 | ||
| file3 = f"{mount_point}/foo3" | ||
| process.run(f"cp {file1} {file3}") | ||
|
|
||
| # Drop caches | ||
| process.run("echo 1 > /proc/sys/vm/drop_caches") | ||
|
|
||
| # Verify content of foo1 and foo2 | ||
| result1 = process.run(f"cat {file1}") | ||
| assert_equal(result1[1].strip(), "Hello World") | ||
|
|
||
| result2 = process.run(f"cat {file2}") | ||
| assert_equal(result2[1].strip(), "Hello World") | ||
|
|
||
| # VDO device is now stopped (exited context manager) | ||
| # Get kernel log timestamp before restarting | ||
| start_time = time.time() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. start_time is a bit confusing as a name. Start of what? Given you're using it as the place to start checking the logs, maybe a name that refers to that? |
||
|
|
||
| # Restart VDO device without reformatting | ||
| with standard_vdo(fix, format=False, slab_bits=17) as vdo: | ||
| with mounted_fs(vdo.path) as mount_point: | ||
| # Verify content of foo3 | ||
| file3 = f"{mount_point}/foo3" | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a little weird to me that we check file3 only after vdo restart (and file 2 only before it). But I'll concede that's what the original Basic01 does. |
||
| result3 = process.run(f"cat {file3}") | ||
| assert_equal(result3[1].strip(), "Hello World") | ||
|
|
||
| # Check kernel log for VDO startup message | ||
| log_message = get_dmesg_log(start_time) | ||
| assert_string_in(log_message, "VDO commencing normal operation") | ||
|
|
||
| def register(tests): | ||
| tests.register("/vdo/basic/basic01", t_basic) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| """ | ||
| VDO BasicFSDedupe test - Filesystem-level deduplication verification | ||
| """ | ||
| import logging as log | ||
| import os | ||
| import shutil | ||
| import tempfile | ||
|
|
||
| from dmtest.assertions import assert_near | ||
| from dmtest.gendatablocks import make_block_range | ||
| from dmtest.vdo.stats import vdo_stats, make_delta_stats | ||
| from dmtest.vdo.utils import standard_vdo, fsync, mounted_fs | ||
|
|
||
|
|
||
| def t_basic_fs_dedupe(fix) -> None: | ||
| """ | ||
| Basic filesystem-level deduplication test that writes a dataset twice | ||
| and verifies deduplication achieves expected space savings. | ||
| """ | ||
| MB = 1024 * 1024 | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. MS is defined in vdo/utils.py. Please let's not redefine these things all over the place. |
||
| num_files = 32 | ||
| file_size_mb = 8 | ||
| blocks_per_file = file_size_mb * MB // 4096 # 8MB / 4KB = 2048 blocks | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. BLOCK_SIZE is similrly defined as 4 * kB. |
||
|
|
||
| with standard_vdo(fix) as vdo: | ||
| with mounted_fs(vdo.path, format=True, lazy_itable_init=False) as mount_point: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does the lazy_itable_init matter? In general, and also in this specific test as opposed to basic01, say.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You know, if this is a common pattern (and I kind of suspect it will be) it might be worth a higher-level context manager, that makes a vdo with a filesystem on it, say standard_vdo_with_fs or something. |
||
| # Create subdirectories on VDO filesystem | ||
| original_dir = os.path.join(mount_point, "original") | ||
| copy1_dir = os.path.join(mount_point, "copy1") | ||
| os.makedirs(original_dir) | ||
| os.makedirs(copy1_dir) | ||
|
|
||
| # Record initial stats after filesystem setup | ||
| fsync(vdo.path) | ||
| initial_stats = vdo_stats(vdo) | ||
|
|
||
| # Generate dataset in a scratch directory | ||
| with tempfile.TemporaryDirectory() as scratch_dir: | ||
| dataset_dir = os.path.join(scratch_dir, "dataset") | ||
| os.makedirs(dataset_dir) | ||
|
|
||
| log.info(f"Generating dataset: {num_files} files × {file_size_mb}MB each = 256MB total") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please, can we make a utility like gendatablocks that will make a file-based dataset? I really don't want to be embedding this idea in every test file. (Selfishly, I've also been thinking this is the exact utility I'll want in order to rewrite tests that current use the linked kernel tree for file data.) |
||
| for i in range(num_files): | ||
| file_path = os.path.join(dataset_dir, f"file_{i:08d}") | ||
| # Create the file first | ||
| with open(file_path, 'w') as f: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's slightly odd that you make the file and then separately update it to include data. But that's an implementation detail, so whatever. Putting the file generation in gendatablocks.py might also give you some advantages by being able to invoke that generation at a different level of granularity, maybe? |
||
| pass | ||
| # Write data to the file | ||
| block_range = make_block_range(file_path, blocks_per_file) | ||
| block_range.write(f"BFD{i:04d}", dedupe=0.0, fsync=False) | ||
|
|
||
| # Copy dataset to "original" directory | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again with the redundant comments. If you want the log line, surely that's documentation enough and we don't also need a comment. |
||
| log.info("Copying dataset to 'original' directory") | ||
| shutil.copytree(dataset_dir, os.path.join(original_dir, "data")) | ||
|
|
||
| # Sync and check stats after first write | ||
| fsync(vdo.path) | ||
| stats_after_first = vdo_stats(vdo) | ||
| delta_first = make_delta_stats(stats_after_first, initial_stats) | ||
|
|
||
| data_blocks = delta_first['dataBlocksUsed'] | ||
| logical_blocks = delta_first['logicalBlocksUsed'] | ||
| ratio_first = data_blocks / logical_blocks if logical_blocks > 0 else 0 | ||
|
|
||
| log.info(f"After first write: data={data_blocks}, logical={logical_blocks}, ratio={ratio_first:.3f}") | ||
| # Verify minimal deduplication on first write (filesystem metadata may cause some variance) | ||
| assert_near(ratio_first, 1.0, 0.1, "Data-to-logical ratio after first write") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Huh, you loosened this check considerably. You check 0.1 (10%) but the original limits us to 0.01 (1%). Was this necessary for some reason? |
||
|
|
||
| # Copy the same dataset to "copy1" directory (duplicate copy) | ||
| log.info("Copying dataset to 'copy1' directory (duplicate)") | ||
| shutil.copytree(dataset_dir, os.path.join(copy1_dir, "data")) | ||
|
|
||
| # Sync and check stats after second write | ||
| fsync(vdo.path) | ||
| stats_after_second = vdo_stats(vdo) | ||
| delta_second = make_delta_stats(stats_after_second, initial_stats) | ||
|
|
||
| data_blocks_2 = delta_second['dataBlocksUsed'] | ||
| logical_blocks_2 = delta_second['logicalBlocksUsed'] | ||
| ratio_second = data_blocks_2 / logical_blocks_2 if logical_blocks_2 > 0 else 0 | ||
|
|
||
| log.info(f"After second write: data={data_blocks_2}, logical={logical_blocks_2}, ratio={ratio_second:.3f}") | ||
| # Verify significant deduplication on second write (~50% ratio expected) | ||
| assert_near(ratio_second, 0.5, 0.05, "Data-to-logical ratio after second write (with dedupe)") | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Loosened check again (5% vs 1% in the original). But this does seem deliberate, is there a reason we need this? |
||
|
|
||
|
|
||
| def register(tests): | ||
| tests.register("/vdo/basic/fs-dedupe", t_basic_fs_dedupe) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| """ | ||
| VDO Create03 test - Device lifecycle stability and logging verification | ||
| """ | ||
| import logging as log | ||
| import re | ||
| import time | ||
|
|
||
| from dmtest.assertions import assert_string_in | ||
| from dmtest.utils import get_dmesg_log | ||
| from dmtest.vdo.utils import standard_stack | ||
| import dmtest.process as process | ||
|
|
||
|
|
||
| def t_create_03(fix) -> None: | ||
| """ | ||
| Test creating and tearing down VDO devices many times to verify device | ||
| lifecycle stability and proper logging behavior. Also verifies that VDO | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Uh, is that what this test does? (Specifically "verify lifecycle stability" and "proper logging beavior". What does "proper logging behavior" mean here?) |
||
| messages include device instance numbers in kernel logs. | ||
| """ | ||
| iteration_count = 1024 | ||
|
|
||
| log.info(f"Starting Create03 test with {iteration_count} iterations") | ||
|
|
||
| # Create the VDO stack (formats the device) | ||
| stack = standard_stack(fix, slab_bits=17) | ||
|
|
||
| for i in range(iteration_count): | ||
| # Record time before starting device | ||
| start_time = time.time() | ||
|
|
||
| # Activate the VDO device | ||
| vdo = stack.activate() | ||
|
|
||
| # After first iteration, check kernel logs | ||
| if i > 0: | ||
| log.info(f"Iteration {i + 1}/{iteration_count}: Checking kernel logs") | ||
| kernel_log = get_dmesg_log(start_time) | ||
|
|
||
| # Verify that VDO messages are present (pattern: "vdo[0-9]+:") | ||
| # This regex looks for lines like "vdo0: ..." or "vdo1: ..." | ||
| vdo_messages = [line for line in kernel_log.split('\n') | ||
| if re.search(r'vdo[0-9]+:', line)] | ||
|
|
||
| if vdo_messages: | ||
| # Verify that VDO messages include device instance number | ||
| # Pattern: "vdo([0-9]+:|:\[SI\]:)" | ||
| # This matches "vdo0:", "vdo1:", "vdo:S:", "vdo:I:", etc. | ||
| for msg in vdo_messages: | ||
| # Messages from interrupt context may be anonymous (vdo:S: or vdo:I:) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment, and the one two lines above it, are in fact wrong. |
||
| # so we allow those, but most messages should have instance numbers | ||
| if not re.search(r'vdo([0-9]+:|:\[[SI]\]:)', msg): | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, the actual regex match got corrupted, too. |
||
| log.warning(f"VDO message without instance number: {msg}") | ||
| elif i == 0: | ||
| log.info(f"Iteration {i + 1}/{iteration_count}: First iteration (no log check)") | ||
|
|
||
| # Log progress periodically | ||
| if (i + 1) % 100 == 0: | ||
| log.info(f"Completed {i + 1}/{iteration_count} iterations") | ||
|
|
||
| # Stop the VDO device | ||
| vdo.remove() | ||
|
|
||
| # For the next iteration, we don't need to format (already formatted) | ||
| stack = standard_stack(fix, format=False, slab_bits=17) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So perhaps this is a dumb comment, but since the first iteration is special, maybe we unroll that one ase, and then do the rest of the iterations using "with standard_vdo"? It seems like that might make it easier to understand that the first iteration works differently, instead of having branches throughout the inner loop. |
||
|
|
||
| log.info(f"Create03 test completed successfully after {iteration_count} iterations") | ||
|
|
||
|
|
||
| def register(tests): | ||
| tests.register("/vdo/creation/create-03", t_create_03) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a comment about testcase naming but I'm going to hang it here. First off, other VDO tests do not put a hyphen before the number. Here, in paricular, lexical sorting makes "create-03" come before "create01" instead of after it (in the dependencies file), which would make more sense. So can we drop this and similar hyphens before test numbers? On a larger note, I want to grump about using hyphen vs underscore as a delimiter in testcase names. There is no general consistency in dmtest. most test suites use the hyphen, like you did though a few use underscore (like thin_migrate and also my new recovery tests). Bruce seems to have used camel-casing for many of our rearlier vdo tests. The test groupings, though, always use underscores (even blk_archive). This is an action item for later, but I would like some consistency, if you can think about that for future test sets. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,14 +1,22 @@ | ||
| import dmtest.vdo.basic_01_tests as vdo_basic_01 | ||
| import dmtest.vdo.basic_fs_dedupe_tests as vdo_basic_fs_dedupe | ||
| import dmtest.vdo.compress_tests as vdo_compress | ||
| import dmtest.vdo.create_03_tests as vdo_create_03 | ||
| import dmtest.vdo.creation_tests as vdo_creation | ||
| import dmtest.vdo.dedupe_tests as vdo_dedupe | ||
| import dmtest.vdo.full_tests as vdo_full | ||
| import dmtest.vdo.load_failure_tests as vdo_load_failure | ||
| import dmtest.vdo.recovery_tests as vdo_recovery | ||
| import dmtest.vdo.uds_timeout_tests as vdo_uds_timeout | ||
|
|
||
| def register(tests): | ||
| vdo_basic_01.register(tests) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is so stupid, why do we have to do explicit registration. Not actionable for you, I just wanted to complain about it again. |
||
| vdo_basic_fs_dedupe.register(tests) | ||
| vdo_create_03.register(tests) | ||
| vdo_creation.register(tests) | ||
| vdo_dedupe.register(tests) | ||
| vdo_compress.register(tests) | ||
| vdo_full.register(tests) | ||
| vdo_load_failure.register(tests) | ||
| vdo_recovery.register(tests) | ||
| vdo_uds_timeout.register(tests) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commenting style is going to get real old real fast. This comment doesn't say anything that the very next line of code doesn't also say. Well, other than the name of the SLAB_BITS_SMALL constant, which you could fix by actually using the new constant in the test.
And more importantly, this is true about basically every comment in this test. Possibly the stop are restart comments at lines 4 2 and 47 are useful.
Please remove the comments that don't provide extra information.