Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion src/dmtest/fs.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,8 @@ def check_cmd(self):

def mkfs_cmd(self, opts):
discard_arg = "discard" if opts.get("discard", True) else "nodiscard"
return f"mkfs.ext4 -F -E lazy_itable_init=1,{discard_arg} {self._dev}"
lazy_init = 1 if opts.get("lazy_itable_init", True) else 0
return f"mkfs.ext4 -F -E lazy_itable_init={lazy_init},{discard_arg} {self._dev}"


class Xfs(BaseFS):
Expand Down
60 changes: 60 additions & 0 deletions src/dmtest/vdo/basic_01_tests.py
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)

Copy link
Copy Markdown
Collaborator

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.

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}'")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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)
88 changes: 88 additions & 0 deletions src/dmtest/vdo/basic_fs_dedupe_tests.py
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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)")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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)
70 changes: 70 additions & 0 deletions src/dmtest/vdo/create_03_tests.py
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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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?)
And what you don't say is that this test checks that the VDO instance number rolls over at 3 digits, which is the only reason to run 1000 iterations. (instead of, say, a dozen)

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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.
The regex matches either "vdo###:" or "vdo:[SI]:" ; in the interrupt context case we are matching an explicit string with square brackets in it, not a character class.

# so we allow those, but most messages should have instance numbers
if not re.search(r'vdo([0-9]+:|:\[[SI]\]:)', msg):

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

8 changes: 8 additions & 0 deletions src/dmtest/vdo/register.py
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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The 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)
Loading