Lustre Script Coding Style

Bash Style

• Bash is a programming language. It includes functions. Shell code outside of functions is effectively code in an implicit main() function. An entire function should be fully seen on one page (~70-90 lines) and be readily comprehensible. If you have any doubts, then it is too complicated. Make it easier to understand by separating it into subroutines.
• Scripts should start with #!/usr/bin/bash, unless they need to run in MacOS Terminals, which need /bin/bash.
• The total length of a line (including comment) must not exceed 80 characters. Take advantage of bash's += operator for constants or linefeed escapes \.
  • Lines can be split without the need for a linefeed escape after |, ||, & and && operators at the end of the line.
• The indentation must use 8-column tabs and not spaces. For line continuation, an additional tab should be used to indent the continued line, or align after [ or ( for continued logic operations.
• Comments are just as important in a shell script as in C code.
• Use $(...) instead of `...` for subshell commands:
  • $(...) is easier to see the start and end of the subshell command
  • $(...) avoids confusion between '...' and `...` with a small font
  • $(...) can be nested
• Use the subshell syntax only when necessary:
  • When you need to capture the output of a separate program
  • Using the construct with functions leads to stray output and/or convoluted code struggling to avoid output pollution
  • It is more computationally efficient to not fork() the Bash process. Bash is slow enough already.
• Use "here string" like function <<<$var instead of echo $var | function to avoid forking a subshell and pipe
• Use file arguments like awk '...' $file or input redirection like function << $file instead of a useless use of cat
• Use built-in Bash Parameter Expansion for variable/string manipulation rather than forking sed/tr:
  • Use ${VAR#prefix} or ${VAR%suffix} to remove prefix or suffix respectively
  • Use ${VAR/pattern/string} to replace pattern with string instead of echo $VAR | sed
• Avoid use of "grep foo | awk '{ print $2 }'" since "awk '/foo/ { print $2 }' works just as well and avoids a separate fork + pipe
• If a variable is intended to be used as a boolean, then it must be assigned as follows:

        local mybool=false         # or true
        
        if $mybool; then
                do_stuff
        fi
 

• for loops it is possible to avoid a subshell for $(seq 10) using the built-in iterator for fixed-length loops:
  • Unfortunately, {1..$var} does not work, so use the (( ... )) arithmetic operator

        for ((i=0; i < $var; i++)); do
                something_with $i
        done

• Use export FOOBAR=val instead of FOOBAR=val; export FOOBAR for clarity and simplicity
• Use [[ expr ]] for bash instead of [ expr ]
  • The [[ test understands regular expression matching with the =~ operator
  • The easiest way to use it is by putting the expression in a variable and expanding it after the operator without quotes.

• Use (( expr )) instead of [ expr ] or let expr when evaluating numerical expressions
  • This can include mathematical operators like $((...))
  • Will properly compare numeric values, unlike ... that is comparing strings

        # wrong: this is surprisingly "true" because '5' > '3' and the rest of the string is ignored
        [[ 5 > 33 ]] && echo "y" || echo "n"
        y
        # right: this is what you expect for the '>' operator
        (( 5 > 33 )) && echo "y" || echo "n"
        n

• • This uses normal <=, >=, == comparisons instead of -lt, -eq, -gt
  • Can use for (( i=0; i <= END; i++ )); do and other numerical expressions instead of an external subshell for seq

• Use $((...)) for arithmetic expressions instead of expr ...
  • No need for $ when referencing variable names inside $((...))
  • $((...)) can handle hex values and common math operators
• Error checks should prefer the form [[ check ]] || action to avoid leaving a dangling "false" on the return stack
  • Otherwise, [[ check ]] && action will leave a dangling "false" on the stack if check fails and an immediately following return/end of function will return an error

Test Framework

Variables

• Names of variables local to current test function which are not exported to the environment should be declared with "local" and use lowercase letters
• Names of global variables or variables that are exported to the environment should be UPPERCASE letters
• Use $TMP/ for temporary non-Lustre files instead of /tmp/
• Use $SECONDS to get the current time when measuring test durations instead of $(date +%s) fork+exec:

    local start=$SECONDS
    
    do something
    local elapsed=$((SECONDS - start))

or

    local end=$((SECONDS + delay))
    
    while ((SECONDS < end)); do
        something
    done

Functions

• Each function must have a section describing what it does and explain the list of parameters

# One line description of this function's purpose
#
# More detailed description of what the function is doing if necessary
#
# usage: function_name [--option argument] {required_argument} ...
# option: meaning of "option" and its argument
# required_argument: meaning of "required_argument"
# 
# expected output and/or return value(s)

• Function arguments should be given local variable names for clarity, rather than being used as $1 $2 $3 in the function

    local facet=$1
    local file="$2"
    local size=$3

• Use sleep 0.1 instead of usleep 100000, since usleep is RHEL-specific

Tests and Libraries

• To avoid clustering a single test-framework.sh file, there should be a <test-lib>.sh file for each test that contains specific functions and variables for that test.
• Any functions, variables that global to all tests should be put in test-framework.sh
• A test file only need to source test-framework.sh and necessary <test-lib>.sh file

Subtests

• subtest names/numbers are not strictly meaningful in themselves, but there are some conventions that should be followed
  • prefer subtest numbers below 1000 by convention
    • some wrapper scripts (e.g. sanity-compr.sh) run other scripts like sanity.sh and sanityn.sh as well as their own subtests

• • it is convenient to cluster related tests together when adding new subtests
    • easier to find related subtests to determine if there is coverage for some functionality
    • adding subtests in different parts of the script (instead of at the end) avoids needless context patch conflicts
    • adding subtests with disjoint numbers avoids semantic test numbering conflicts
  • when adding subtests at the end of the file it is convenient to leave occasional gaps in the numbering (e.g. 100, 150, 200 instead of 33, 34, 35,)
    • this leaves room for other subtests to be added with fewer context/number conflicts
• do not mix pure numbered subtests with suffixes for the same number (e.g. 56 and 56a)
  • test-framework.sh allows running groups of subtests together (e.g. all 56X subtests), but if there is also plain-numbered subtest (e.g. 56) then it cannot be run in isolation
  • if a numbered subtest is growing a new variant, rename the original subtest to the a suffix (e.g. 56->56a) then add the new variant with the b suffix
  • if the plain a-z suffixes are occupied or you want to make a variant of an already-suffixed subtest, use aa, ab (or ea or similar) and locate it after a (or e)
  • continue a-z suffixes with A-Z, but do not add aa, ab, etc. after the z or Z suffix to avoid clashes of subtest names
• subtests should not reference their test number explicitly, since the subtest name may change in the future (for various reasons).
  • the $testnum variable holds the test number (e.g. 11 or 27M)
  • the $TESTNAME variable holds the test name (e.g. test_27)
• test files should be named $tfile for the filename, or base name like $tfile.1 or $tfile.source to simplify debugging
• test directories should be named $tdir, and should be used insetad of $DIR if more than a handful files are created for the subtest
• small/few test files/dirs do not need to be explicitly deleted at the end of the test, that is done by test-framework.sh at the start/end of each test script
• large (over 1MB)/many (over 50) test files/dirs in a subtest should be cleaned up explicitly with a stack_trap so that they are always cleaned up even if the test exits with an error, and do not fill the test filesystem

 
    stack_trap "rm -f $DIR/$tfile.big"
    fallocate -l 100M $DIR/$tfile.big || error "$tfile.big create failed"
    
    stack_trap "unlinkmany $DIR/$tdir/$tfile- 1000"
    createmany -o $DIR/$tdir/$tfile- 1000 || error "$tfile creation failed"

• creating large test files is by far the fastest with "fallocate" *when supported* (ldiskfs only), as determined by check_set_fallocate
  • fallocate'd files will only produce zeroes when read, so it is not useful where the data contents are used since it is identical to a sparse file except by allocated blocks
• use test_mkdir to add some variety to directory creation (random local, striped, remote) if directory location is not critical to the test
• ensure that directory location and MDS facet are aligned. Since 2.14.54 directories may be created on any MDT, so "do_facet mds1 ..." may be on the wrong MDS.
• Use "mkdir_on_mdt0 $DIR/$tdir" if necessary to create directories on MDT0000 for use with mds1, or preferentially "$LFS getdirstripe -m $DIR/$tdir" to determine MDT index, and "mds$((idx+1))" for facet name.
• the error messages in a subtest should be unique so that it is easy to determine which check failed

 
    lfs migrate -c3 $tfile || error "'lfs migrate -c3' failed"
    lfs migrate -c1 $tfile || error "second 'lfs migrate -c1' failed"

• use skip to skip subtests that should not run because of permanent functional deficiency (e.g. non-existent functionality in backing filesystem, older version of client/server, wrong configuration)

 
    (( MDS1_VERSION_CODE >= $(version_code 2.17.58.63) )) ||
        skip "need MDS >= 2.15.53 to check foobar works"
    [[ $mds1_FSTYPE == "ldiskfs" ]] || skip "MDT0000 is not ldiskfs"

• version checks for interoperability depend on where the change being tested runs:
  • a subtest for a server-side change should check the server version against the version of the patched build itself, as reported by git describe --match "[0-9]*" on the patch (e.g. 2.17.58-63-g1234abc is 2.17.58.63), and never the next release, since that would skip the subtest on the patched build. Use MDS1_VERSION for an MDS-side change, and OST1_VERSION for an OST-side change
  • a subtest for a client-only change that is tested on the local client does not need a version check, since the tests are run from the client's own lustre-tests package
  • a subtest for a change that is exercised on another client node should check the version of that node, e.g. with $(version_code $(lustre_build_version_node $node))
  • if a patch is making a change that causes an old client to fail against a new server, then an exception can be added in lustre/tests/except/$TEST_SCRIPT.ex
• use skip_env for minor environmental deficiency in developer test environment (e.g. missing binary) that _should_ exist in autotest:

 
    kinit || skip_env "Kerberos not installed"

• when subtests are run in a loop with ONLY_REPEAT=N or ONLY_MINUTES=M then $ONLY_REPEAT_ITER holds the iteration number (1-based). In some cases this can be useful for adapting the test case to handle running many iterations in a row (e.g. adding wait_delete_completed) that isn't needed when run in isolation.