[VDO-5749] Add tests for Thin Pools using VDO as data devices. #182
Conversation
There was a problem hiding this comment.
I discussed these commits with Bruce on Slack. I had meant to add a comment here, but apparently forgot.
My main points, I believe, were
a) It makes more sense to me to split the commits as one refactoring the old thin device, and one adding the new thinvdo device. Probably the test changes associated with each of those could be in the same commit since they're small.
b) The new ThinVDO.pm file share a lot of code with LVMManaged.pm and we discussed factoring out a common base class to allow sharing code between those two devices.
Also c), (not mentioned before) we should probably add at least one or two BlockDevice01 cases that use the new device(s).
bjohnsto
force-pushed
the
bjohnston_add_lvm_thin
branch
from
0aabb98 to
dde9d07
Compare
github-actions
bot
added
public testing running
public testing done
and removed
untested
public testing running
public testing done
labels
There was a problem hiding this comment.
Mostly minor cleanup things, and a couple small typos. The diffs for LVMVDO.pm are much better now, thank you. The new classes and the new test all seem to make sense.
This series could be split into more commits to make it clearer, but I won't insist. (The -an change is logically separate and could be its own commit; similarly the nex test is logically separate and could be described in a separate commit.)
| sub createThinVolume { | ||
| my ($self, $name, $size) = assertNumArgs(3, @_); | ||
| sub createThinPool { | ||
| my ($self, $name, $size, $config) = assertMinMaxArgs(["", 0], 3, 4, @_); | ||
| my $machine = $self->{storageDevice}->getMachine(); |
There was a problem hiding this comment.
This won't fail, but it is technically incorrect. You only have one optional parameter, so you should really only be providing one default value instead of two.
There was a problem hiding this comment.
I've split out the -an -kn change into a separate commit. and I've cleaned up this extra option issue.
| # needed for both the metadata and spare metadata devices. Then we see if the VG | ||
| # has sufficient space. Currently that space is 30 extents for each device. | ||
| my $metaDataSize = 60 * ($self->{physicalExtentSize} / $KB); | ||
| my $fullSize = $ksize + $metaDataSize; |
There was a problem hiding this comment.
The comment makes this much clearer now.
(60 * $self->{physicalExtentSize}) / $KB as a spelling communicate the idea of "60 extents" better, to me, but it's a minor thing.
There was a problem hiding this comment.
I've changed this to do this ordering
| my $newSize = $kfree - $metaDataSize; | ||
| $log->info("requested size ${ksize}K plus metadata is too large for VG, using ${newSize}K"); | ||
| $ksize = $newSize; | ||
| } |
There was a problem hiding this comment.
You're adjusting to not exceed $kfree, but we've already done that once, up at line 172. Woudln't it be better to add up the requested size plus metadata, and then correct for $kfree once?
src/perl/Permabit/VolumeGroup.pm
Outdated
| @@ -250,17 +276,8 @@ sub createVDOVolume { | |||
| } | |||
|
|
|||
| my $args = join(' ', map { "--$_ $args{$_}" } keys(%args)); | |||
| $machine->runSystemCmd("sudo lvcreate $args --yes -ay $config" | |||
| $machine->runSystemCmd("sudo lvcreate $args --yes -an -kn $config" | |||
There was a problem hiding this comment.
We've been enough rounds on the PR that I won't insist, but using the -a and -k flags is the sort of logically separate change that could easily be its own commit. In the future, please do think about how to cut up changes in to logical separate commits as much as possible.
| lvmvdo-iscsi-linear | ||
| lvmvdo-tracer-raw | ||
| thin-lvmvdo-thin | ||
| thin-vdo-thin | ||
| tracer-corruptor-raw |
There was a problem hiding this comment.
Okay, looks mostly reasonable. But the now thin-thinpool cases have more devices in the stack. So thin-thinpool is as two-devices stack, in should be listed with the other two device stacks. Similarly, the other new cases are three-device stacks and should go with the three-device stacks.
| ## | ||
| sub growLogical { | ||
| my ($self, $logicalSize) = assertNumArgs(2, @_); |
There was a problem hiding this comment.
It seem to me that, much like renameVDO(), growPlogical() and growPhysical() should still be defined in the parent class, with an implmentation thta errors out if a subclass fails to override it. (That would also make these diffs cleaner, I think.)
| # @inherit | ||
| ## | ||
| sub setup { | ||
| my ($self) = assertNumArgs(1, @_); |
There was a problem hiding this comment.
Potentially a stub for setup() should still exist for common bits like finding the vdoformat executable. I don't know how hard that would be to do.
There was a problem hiding this comment.
I'd like to skip this work for now if thats ok with you
bjohnsto
force-pushed
the
bjohnston_add_lvm_thin
branch
from
99cca03 to
018fa70
Compare
The first step to supporting a thin pool device using VDO is to split our current perl/Permabit/BlockDevice/LVM/Thin.pm file (which is a combination of a thin pool device and a single thin device on top) into two separate files, one for just the thin pool and one for thin. This will allow us to test (in a future commit) multiple thin devices on top of a single thin pool using VDO. The thin device can no longer be made without a thin pool device below it in the stack, acting as its storage device. As such, tests that set the deviceType to just thin have been updated to add thinpool underneath. This includes changes to BlockDevice01.pm and DiscardThin.pm BlockDevice01.pm has also been updated to remove two tests that used to have thin above and below vdos. These tests are actually scenarios we do not support. VDOTest.pm has been modified to fix a bug dealing with getting the vdo device for things like stats. Signed-off-by: Bruce Johnston <[email protected]>
The second step to supporting a thin pool device using VDO is to add the new device. This commit takes the current LVMManaged.pm file and puts common LVM VDO code into a base class called LVMVDO.pm. Then an LVMVDO directory is created and what is left of the LVMManaged.pm class is put in a new file called Managed.pm in the LVMVDO directory. Finally, the ThinPool.pm (thin using vdo) class is put in the LVMVDO directory, sharing the LVMVDO.pm features. StorageStack.pm is updated with the new file locations and VolumeGroup.pm is updated to support creating the new vdo thinpool device. VolumeGroup.pm is updated to allow creation of thin pools that use vdo. This is done through the use of an optional parameter. Finally a number of tests are updated to handle the new location of the default vdo device, as it has been renamed and put in the LVMVDO directory. Signed-off-by: Bruce Johnston <[email protected]>
This commit replaces the post creation disabling of devices in VolumeGroup.pm with the create flags of -an and -kn now that they work with the lvm version we are using. Signed-off-by: Bruce Johnston <[email protected]>
bjohnsto
force-pushed
the
bjohnston_add_lvm_thin
branch
from
018fa70 to
19d295d
Compare
In order to support multiple devices on top on a VDO, LVM has designed a setup where it will use VDO as a data device underneath a Thin Pool device. This will allow the user to create multiple thin devices on top of the thin pool, thus allowing for sharing of a dedupe pool.
To support this in our test environment, we have split apart our former Thin.pm test device (which was a combination of a thin pool and single thin device on top) into two devices; a normal ThinPool test device (BlockDevice/LVM/ThinPool) and a thin volume (BlockDevice/LVM/Thin). We then created a VDO backed thin pool test device that provides VDO and LVM support, similar to what LVMManaged (BlockDevice/VDO/LVMManged) does now.
To make things cleaner, we have created an LVMVDO (BlockDevice/VDO/LVMVDO) base test device that inherits from VDO (BlockDevice/VDO) and LVM (BlockDevice/LVM). The old LVMManaged (now renamed to Managed) and the new thin vdo device (also named ThinPool) have been moved to a new directory called LVMVDO (BlockDevice/VDO/LVMVDO/Managed and BlockDevice/VDO/LVMVDO/ThinPool). BlockDevice/VDO/LVMVDO/ThinPool inherits from BlockDevice/LVM/ThinPool)
Changes were made to VolumeGroup.pm to match the new devices and modified old Thin device. We now have two new functions in VolumeGeroup.pm; createThinVolume and createThinPoolVolume, which cleans up some of the code but also makes it a bit more complicated since we have to add it some additional assertions in createThinVolume to make sure it can only be called when the storage device is a ThinPool device.
We have modified StorageStack to handle the new test devices and updated any old tests that used thin to now create a thinpool and then a thin device on top. This will make all old tests continue to work. Finally we have added one new test called ThinVDO.pm to test Thin with VDO. This currently requires RAWHIDE devices since it has the LVM version we need. There are two subtests in it; one basic test and one with multiple thin volumes on top of a single vdo backed thin pool. Both test writing data and verifying dedupe.