Enable zfs-test zpool_expand_002_pos #5757

ofaaland · 2017-02-08T01:28:49Z

Description

Use -pH flags in get_pool_prop so that size or other numeric properties
can be compared. The zpool_expand test suite is the only one which uses
get_pool_prop for a numeric property which would be affected by this.

Add TEMPFILE and TEMPFILE{0,1,2} to default.cfg for tests that benefit
from building pools on top of files, such as this one where expansion
is necessary but the entries in DISKS may not point to entities that
can be expanded..

Base the pool used for testing on file-type VDEVs instead of using zvols
within an underlying pool. This is simpler and avoids issues that come up
when pools are backed by other pools.

Motivation and Context

The zpool_expand test suite is currently disabled on linux. This restores some testing of pools whose LUNs expand.

How Has This Been Tested?

Tested locally by running a small subset of the tests enabled in linux.run including the test I enabled, on RHEL6, using zfs-test.sh in-tree. Checked for impacts on other tests due to the introduction of TEMPFILE and change to get_pool_prop by code inspection / grep.
Relying on buildbot to run the full test suite.

Types of changes

[ x] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

[x ] My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
[x ] I have read the CONTRIBUTING document.
I have added tests to cover my changes.
[x ] All new and existing tests passed.
Change has been approved by a ZFS on Linux member.

mention-bot · 2017-02-08T01:28:50Z

@ofaaland, thanks for your PR! By analyzing the history of the files in this pull request, we identified @behlendorf, @loli10K and @gmelikov to be potential reviewers.

ofaaland · 2017-02-08T01:31:29Z

After I've received feedback and tests pass, I can update the other two tests in the suite. I'd like to get one test right before I modify the others and submit the real PR.

ofaaland · 2017-02-08T01:36:38Z

Also, perhaps it would be better for the changes that are local to the test suite itself, and the runfile, to be in a separate commit from the changes to libtest.shlib and default.cfg.in. If so, let me know and I'll change it.

behlendorf

The basic modification to the test case which switches from zvol's to sparse files seems entirely reasonable. This point of the test case after all is to test the autoexpand code. The only thing we might be loosing the long term is the ability for the FMA code to be noticed of a device size change and automatically run the autoexpand. But we can add a new test case for that or fix this once we have that infrastructure ready.

behlendorf · 2017-02-08T18:30:02Z

tests/zfs-tests/tests/functional/cli_root/zpool_expand/zpool_expand.cfg

-export org_size=1g
-export exp_size=2g
+export org_size=100m
+export exp_size=200m


When #5679 get's merged (very soon!) you'll want to use $MINVDEVSIZE for this.

org_size="$((($MINVDEVSIZE / (1024 * 1024)) * 100))m"

Not that I'm opposed to it but what's the motivation for reducing the file size if they're sparse?

Good point. When running the test case without this change I got complaints about running out of space, but now I'm not sure why. I'll look into it more, maybe this part of the change is unnecessary.

I'm unable to reproduce the error I saw, so I'm dropping those changes to my patch. When #5679 is merged I'll update to use MINVDEVSIZE.

behlendorf · 2017-02-08T18:35:05Z

tests/zfs-tests/tests/functional/cli_root/zpool_expand/zpool_expand.cfg

-export EX_1GB=1073741824
-export EX_3GB=3221225472
+export EX_100MB=117440512
+export EX_300MB=318767104


This could be calculated from $MINVDEVSIZE

don-brady · 2017-02-08T20:17:55Z

I thought the point of zpool_expand_001_pos.ksh was to test the automated (ie ZED/FMA) portion and zpool_expand_002_pos.ksh tests the manual, zpool online -e functionality.

Shouldn't we be enabling zpool_expand_002_pos.ksh first and then use zpool_expand_001_pos.ksh to test the automation?

ofaaland · 2017-02-08T20:40:57Z

@don-brady ,
You're probably right! I'll go look, thanks for catching that.

ofaaland · 2017-02-09T17:25:28Z

As of Thu 9 Feb, all the tests passed. The Centos 6.7 x86_64 (TEST) and Debian 8 arm (BUILD) builders did not update the status visible in the pull request, but the Details link shows both finished and passed.

ofaaland · 2017-02-09T17:30:08Z

@don-brady, you were correct, zpool_expand_002_pos.ksh tests the functionality of zpool online -e with expanded vdevs. This patch now enables 002_pos and leaves the other tests alone.

@behlendorf, I was unable to reproduce my out of space symptoms so I put back the larger file sizes. When #5679 is merged I'll modify the test configuration to use MINVDEVSIZE.

ofaaland · 2017-02-09T18:04:01Z

Test 003_neg does not depend on FMA so I will update it as well.

behlendorf · 2017-02-09T19:30:35Z

@ofaaland merged #5679 you can use MINVDEVSIZE.

ofaaland · 2017-02-09T21:58:23Z

@behlendorf I updated the tests to use MINVDEVSIZE

ofaaland · 2017-02-09T22:00:44Z

I removed the [WIP]. I believe this patch is ready to go, please review and comment. Thanks!

don-brady

Thanks for fixing this pool expand test! The 002 test LGTM. I would wait on the 001 and 003 automated tests until we can use DM based disks that will work with ZED autoexpand logic.

don-brady · 2017-02-09T22:36:56Z

tests/zfs-tests/tests/functional/cli_root/zpool_expand/zpool_expand_001_pos.ksh

@@ -27,6 +27,7 @@



Since we are not going to run 001 test, can we just leave this file as-is? The auto-expand zed functionality probably won't work with files.

@don-brady, I revised my commit to not alter test 001, except that I removed unnecessary use of variables EX_{1,3}GB.

don-brady · 2017-02-09T22:55:44Z

tests/zfs-tests/tests/functional/cli_root/zpool_expand/zpool_expand_003_neg.ksh

@@ -27,23 +27,23 @@



This is similar to zpool_expand_001_pos test but is testing that opt-out of auto-expand prevents expanding the pool. The auto-expand automation logic in ZED uses the devid and phys_path keys for vdev matching. These keys are not present for file based vdevs so the changes below will always report success and doesn't really test autoexpand opt-out for disks.

I would recommend excluding test 003 until we have test 001 working. The auto-expand code exists in ZED but we propbably need to use DM disks during the test for the matching to succeed.

Don,
Are you certain that autoexpand depends on ZED? See the following from spa_vdev_detach():

4944 /*
4945 * If the 'autoexpand' property is set on the pool then automatically
4946 * try to expand the size of the pool. For example if the device we
4947 * just detached was smaller than the others, it may be possible to
4948 * add metaslabs (i.e. grow the pool). We need to reopen the vdev
4949 * first so that we can obtain the updated sizes of the leaf vdevs.
4950 */
4951 if (spa->spa_autoexpand) {
4952 vdev_reopen(tvd);
4953 vdev_expand(tvd, txg);
4954 }
4955
4956 vdev_config_dirty(tvd);

I'm confused ...

Thanks for helping me work through these tests.

So, we're both confused! :) So it turns out there are 2 types of autoexpand. The zpool_expand tests 001 and 003 are only testing one case AFAIKT. One case is where an existing LUN gets bigger and the other case is where a smaller LUN is replaced by a bigger one (that is what the spa_vdev_detach is checking).

The additional use case and missing coverage is described in #5771 and can be addressed later.

@don-brady, I revised my commit so that it does not alter test 003.

Use -pH flags in get_pool_prop so that numeric properties such as size can be compared. The zpool_expand test suite is currently the only one which uses get_pool_prop for a numeric property. Add TEMPFILE and TEMPFILE{0,1,2} to default.cfg for tests that must build pools on top of files, such as this one where expansion is necessary but the entries in DISKS may not point to entities that can be expanded. Base the pool used for testing on file-type VDEVs instead of using zvols within an underlying pool, to avoid issues that come up when pools are backed by other pools. Remove shell variables EX_1GB and EX_2GB used to recognize correct expansion, and instead calculate the appropriate values based on the variables used to control file or volume size, org_size and exp_size. This change is also made in test 001 although that test is not enabled because it depends on FMA. Finally, enable zpool_expand_002_pos. Signed-off-by: Olaf Faaland <faaland1@llnl.gov>

Addressed Don's requests with updated commit.

ofaaland · 2017-02-11T03:17:12Z

@behlendorf ,
The CentOS 7 Mainline builder reported zfstests failed, but the logs for zfstests report all tests passed:

Results Summary
SKIP 5
PASS 721

Running Time: 01:37:48
Percent passed: 99.3%
Log directory: /var/tmp/test_results/20170211T002240

process killed by signal 9
program finished with exit code -1
elapsedTime=5875.543940

So I believe that does not reflect on my patch.

dinatale2 · 2017-02-11T07:19:29Z

tests/zfs-tests/include/default.cfg.in

+export TEMPFILE=${TEST_BASE_DIR%%/}/tempfile$$
+export TEMPFILE0=${TEST_BASE_DIR%%/}/tempfile0$$
+export TEMPFILE1=${TEST_BASE_DIR%%/}/tempfile1$$
+export TEMPFILE2=${TEST_BASE_DIR%%/}/tempfile2$$


Do you use TEMPFILE{0,1,2}? I don't see where you do. It appears that you are only using TEMPFILE.

You are correct. I defined TEMPFILE012 to be consistent with other similar variables such as TESTDIR*

@dinatale2, thanks for the Thumbs Up. Does this mean you approve the patch? If so, I believe there's a special link for that in the upper right corner somewhere. Thanks!

Oh sorry, just acknowledging that I approve of the reasoning behind why you did it that way. I will give this another look over and give it a proper review.

dinatale2

LGTM

Use -pH flags in get_pool_prop so that numeric properties such as size can be compared. The zpool_expand test suite is currently the only one which uses get_pool_prop for a numeric property. Add TEMPFILE and TEMPFILE{0,1,2} to default.cfg for tests that must build pools on top of files, such as this one where expansion is necessary but the entries in DISKS may not point to entities that can be expanded. Base the pool used for testing on file-type VDEVs instead of using zvols within an underlying pool, to avoid issues that come up when pools are backed by other pools. Remove shell variables EX_1GB and EX_2GB used to recognize correct expansion, and instead calculate the appropriate values based on the variables used to control file or volume size, org_size and exp_size. This change is also made in test 001 although that test is not enabled because it depends on FMA. Finally, enable zpool_expand_002_pos. Reviewed-by: George Melikov <mail@gmelikov.ru> Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Don Brady <don.brady@intel.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Olaf Faaland <faaland1@llnl.gov> Closes openzfs#5757

ofaaland added the Status: Work in Progress Not yet ready for general review label Feb 8, 2017

behlendorf reviewed Feb 8, 2017

View reviewed changes

ofaaland force-pushed the b_zpool_expand_tests branch 2 times, most recently from 0390596 to b6d82c4 Compare February 9, 2017 01:42

ofaaland removed the Status: Work in Progress Not yet ready for general review label Feb 9, 2017

ofaaland added the Status: Work in Progress Not yet ready for general review label Feb 9, 2017

ofaaland changed the title ~~[WIP] Enable zfs-tests zpool_expand_001_pos~~ [WIP] Enable zfs-tests zpool_expand_002_pos and 003_neg Feb 9, 2017

ofaaland force-pushed the b_zpool_expand_tests branch from b6d82c4 to 8d2b47d Compare February 9, 2017 19:26

ofaaland force-pushed the b_zpool_expand_tests branch from 8d2b47d to 5433445 Compare February 9, 2017 21:56

ofaaland changed the title ~~[WIP] Enable zfs-tests zpool_expand_002_pos and 003_neg~~ Enable zfs-tests zpool_expand_002_pos and 003_neg Feb 9, 2017

ofaaland removed the Status: Work in Progress Not yet ready for general review label Feb 9, 2017

ofaaland requested review from gmelikov, dinatale2 and don-brady February 9, 2017 21:59

don-brady previously requested changes Feb 9, 2017

View reviewed changes

ofaaland force-pushed the b_zpool_expand_tests branch from 5433445 to 9762b97 Compare February 10, 2017 23:30

ofaaland changed the title ~~Enable zfs-tests zpool_expand_002_pos and 003_neg~~ Enable zfs-test zpool_expand_002_pos Feb 11, 2017

dinatale2 reviewed Feb 11, 2017

View reviewed changes

dinatale2 approved these changes Feb 13, 2017

View reviewed changes

behlendorf approved these changes Feb 13, 2017

View reviewed changes

gmelikov approved these changes Feb 13, 2017

View reviewed changes

Merge branch 'master' into b_zpool_expand_tests

43948fd

behlendorf merged commit a454868 into openzfs:master Feb 13, 2017

ofaaland deleted the b_zpool_expand_tests branch February 17, 2017 21:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable zfs-test zpool_expand_002_pos #5757

Enable zfs-test zpool_expand_002_pos #5757

ofaaland commented Feb 8, 2017 •

edited

Loading

mention-bot commented Feb 8, 2017

ofaaland commented Feb 8, 2017

ofaaland commented Feb 8, 2017

behlendorf left a comment

behlendorf Feb 8, 2017

ofaaland Feb 8, 2017

ofaaland Feb 9, 2017

behlendorf Feb 8, 2017

don-brady commented Feb 8, 2017

ofaaland commented Feb 8, 2017

ofaaland commented Feb 9, 2017

ofaaland commented Feb 9, 2017

ofaaland commented Feb 9, 2017

behlendorf commented Feb 9, 2017

ofaaland commented Feb 9, 2017

ofaaland commented Feb 9, 2017

don-brady left a comment

don-brady Feb 9, 2017

ofaaland Feb 10, 2017

don-brady Feb 9, 2017

ofaaland Feb 9, 2017 •

edited

Loading

don-brady Feb 9, 2017 •

edited

Loading

don-brady Feb 10, 2017

ofaaland Feb 10, 2017

ofaaland commented Feb 11, 2017 •

edited

Loading

dinatale2 Feb 11, 2017

ofaaland Feb 11, 2017

ofaaland Feb 13, 2017

dinatale2 Feb 13, 2017

dinatale2 left a comment

Enable zfs-test zpool_expand_002_pos #5757

Enable zfs-test zpool_expand_002_pos #5757

Conversation

ofaaland commented Feb 8, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

mention-bot commented Feb 8, 2017

ofaaland commented Feb 8, 2017

ofaaland commented Feb 8, 2017

behlendorf left a 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

don-brady commented Feb 8, 2017

ofaaland commented Feb 8, 2017

ofaaland commented Feb 9, 2017

ofaaland commented Feb 9, 2017

ofaaland commented Feb 9, 2017

behlendorf commented Feb 9, 2017

ofaaland commented Feb 9, 2017

ofaaland commented Feb 9, 2017

don-brady left a 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

ofaaland Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

don-brady Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ofaaland commented Feb 11, 2017 • edited Loading

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

dinatale2 left a comment

Choose a reason for hiding this comment

ofaaland commented Feb 8, 2017 •

edited

Loading

ofaaland Feb 9, 2017 •

edited

Loading

don-brady Feb 9, 2017 •

edited

Loading

ofaaland commented Feb 11, 2017 •

edited

Loading