warn_illegal_args fix

[lauracaulfield]

Hey Mark -- I have a question about the code in lib\Recipe.pm. I'd like to add a warning based on properties of a test. I believe "warn_illegal_args" is the right place to do that. It should be called when StorScore processes the recipe ("Phase 1" in the BUILD sub), but it isn't. I've traced it down to the anonymous function on line 375. The foreach loop runs for each kind of step, but a print statement inside the anonymous function doesn't run. Can you help me understand why? I suspect the anonymous function needs to be called, but it's not clear to me where that should happen, or why it's happening for phase 2 but not for phase 1.

For example, this code:

    # Install our handler for each kind of step 
    print("warn? $recipe_warnings\n");
    foreach my $kind ( @step_kinds )
    {
        my $sym = "${package}::$kind";
        print("in foreach loop $kind\n");
        *$sym = sub
        {
            print("here\n");
            return $self->handle_step(
                $callback,
                $recipe_warnings,
                $kind,
                @_
            )
        };
    }
    print("done\n");

prints the following:
warn? 1
in foreach loop test
in foreach loop purge
in foreach loop initialize
in foreach loop precondition
in foreach loop bg_exec
in foreach loop fg_exec
in foreach loop bg_killall
in foreach loop idle
done

[marksantaniello]

Hmm... it's been a while. So, yeah -- warn_illegal_args() is definitely the place where I intended this sort of thing to happen. In your code snippet, all I'm doing is installing a "handler" for each kind of step. Later on line 424, I'm going to eval() the recipe string as a Perl program, and it will contain function calls like "test(...)", so basically the "my $sym..." and "*$sym = sub" business is just hooking up the symbol table such that "test(...)" in the recipe will call handle_step() with an argument of kind=test.

The idea is that in phase 1, we kind of parse the recipe on the first eval run. We can use this to raise warnings (recipe_warnings=1) and also to estimate the runtime and print various details about what we are going to do in phase 2, where the eval will actually do real work.

Before we eval the recipe string, we also wrap a header around it, and one of the things done there is to squelch stdio. I suspect this is why you are not seeing your print "here":
https://github.com/Microsoft/StorScore/blob/master/lib/Recipe.pm#L417

What are you trying to warn about?

[lauracaulfield]

Interesting. The sequence around eval is a braintwister I'll have to think about some more.

The squelch stdio makes sense. Is it possible warning messages would be squashed also? (I'll try turning it off to see.) I've fed it a recipe it should definitely warn on and the warning isn't showing up.

Popping up to what I'm doing -- Basically I'm exposing Diskspd's xml interface through to the recipe, so that I can define a different workload for each file. In this first step, the recipe will look something like:

test(
description => "Multitarget Attempt",
target_count => 2,
xml_profile => "test.xml",
precondition => 0,
warmup_time => 60,
run_time => 3600,
);

If the recipe expects multiple target files, StorScore needs to make sure the recipe references an xml profile and that the io generator is diskspd. Here's the code I'm adding to warn_illegal_args:

sub warn_illegal_args
{
my $self = shift;
my $step_ref = shift;

...

# warn about poorly defined multitarget tests
my $is_multitarget = $step_ref->{'target_count'} > 1;
if( $is_multitarget )
{
    warn "Recipe defines multiple targets and no xml profile, skipping step"
        unless $step_ref->{'xml_profile'};

    warn "Multitarget tests must use Diskspd, skipping step"
        unless $self->cmd_line->io_generator eq 'diskspd';
}
print('should have warned here\n');

}

[lauracaulfield]

Turning on stdio for phase 1 got the warning to pop up. Is it important to squelch IO in phase 1? All it seems to print in this phase are errors and warnings I expect to see before running the tests.

[marksantaniello]

Maybe it was a very old bug? Maybe you could add options to squelch stdio and stderr seperately instead of together?
https://github.com/Microsoft/StorScore/blob/master/lib/Recipe.pm#L325-L330

BTW, with the existing code, are this existing warnings broken?
https://github.com/Microsoft/StorScore/blob/master/lib/Recipe.pm#L164-L183

[marksantaniello]

Also BTW, I fear that adding multiple targets is going to break a lot of assumptions in the code. Today we always get a singular target on the command line.

What will you do if target_num doesn't match the number of targets in xml_profile?

[lauracaulfield]

The existing warning is broken. There's an override that will often fix the problem before the warning trips (https://github.com/Microsoft/StorScore/blob/master/lib/Recipe.pm#L205) but if I take that out, the warning only trips when stdio is turned on.

I could squelch stdio and stderr separately -- which one would I want to let through? I guess I'm not clear on what we're trying to squelch at this point in the execution.

Multiple targets is a pretty big change -- I'm working through the assumptions now :-). It would only be supported when the target is a drive or volume (not a file), and only when there's a file system. For now I'm dividing the active range evenly among the target_count files. Also, I'm planning on submitting a pull request here so you can veto/redirect any silliness.

[marksantaniello]

I bet warn goes to stderr, and I just got overzealous with the be_quiet stuff here:
https://github.com/Microsoft/StorScore/blob/master/lib/Recipe.pm#L325-L330

You could split the argument into $quiet_stdio and $quiet_stderr, to retain some flexibility down the road. I think phase 1 should allow stderr, otherwise we cannot warn. That's clearly a bug. We probably always want to hide stdio in both phase 1 and phase 2. So I guess another solution is something like this:

    # Always quiet STDOUT
    $header .= "local *STDOUT; open STDOUT, '>NUL';\n";
    if( $quiet_stderr )
    {
        # If requested, also quiet STDERR
        $header .= "local *STDERR; open STDERR, '>NUL';\n";
    }

For the multiple targets, did you consider expanding the command-line support to allow a comma separated list? Something like --targets=A,B,C. DiskSpdRunner would have to generate the appropriate command line, including potentially creating an XML file (and cleaning it up).

[lauracaulfield]

I did consider the comma-separated list, but it was a few months ago. I'm glad you brought it up, though -- Now that I've played with the changes it's looking a lot more interesting. It would allow support for targeting existing files, and for defining the workloads for a set of disks. Injecting target names into the XML profile is pretty straightforward, given a template xml profile and libXML (http://search.cpan.org/dist/XML-LibXML/).

I'm not totally happy with needing an xml template profile, but I think this might be a good piece to build out later.

[lauracaulfield]

The tricky part is keeping the number of files and the number of workloads in sync. Right now I can fail if the number of files doesn't match the number of "target" elements in the xml profile. I'm aiming to implement something simple at first, to get a feel for how to work with multiple files, then improve on it. I think the command-line list of targets and generating the profile from fields in the recipe (and somehow keeping those in sync) is the final(ish) goal.

[marksantaniello]

If the XML is mostly a fill-in-the-blanks style thing, you probably don't even need a fancy lib XML solution, you just need a function that generates a string, while filling in the blanks.

[lauracaulfield]

True dat. The main use for the xml lib is in the parser -- I have to be able to parse the profile to get the workload parameters that we normally get through parsing the command line. (so anything in the "runner" is piggy backing on the library)

[marksantaniello]

Hmm.... I don't see why would you ever have to parse the XML?

I figured we only needed to generate the DiskSpd XML as an alternative for the DiskSpd command line, when the former can express something that the latter cannot?

[lauracaulfield]

You're right -- the difference is just in the command line. The results output is still in text. You're probably forgetting that we parse the command line to get the workload:

https://github.com/Microsoft/StorScore/blob/master/lib/DiskSpdParser.pm#L34

[marksantaniello]

Ahh yes, the results parser. I forgot. :)

Yup, you need libXML.