Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add microbenchmark scripts and outline for program to run them #2

Closed
wants to merge 2 commits into from

Conversation

sploiselle
Copy link
Contributor

@nvanbenschoten Could I get you to take a look at this? Verified everything worked but wanted to get someone else to at least take a look at it before I started building a tool. If I could even get a high-level blessing on the reproduction steps, that's legit; anything more thorough than that's icing on the cake.

@sploiselle
Copy link
Contributor Author

@nvanbenschoten Added prototype to actually run the microbenchmarks just once on GCP with n1-standard-16. Tested and verified that it works, but very open to any suggestions on improving it.

Copy link
Contributor

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job on this! I left a couple of code related comments, but the general structure looks good.

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated
if err != nil {
log.Fatal(err)
}
strVal, err := out.ReadString('\n')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will only read the first line. Is that what you want?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, if there is only one line then it will return err = io.EOF. I don't think that's what you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything I've wanted is just a single-line value returned from a VM, so they've been newline-terminated; haven't had any problems yet but will figure out how to make this more robust.

main.go Outdated Show resolved Hide resolved
main.go Outdated
case "gcp":
runCmd("roachprod", "create", clusterName, "-n", "2", "--gce-machine-type", machineType)
default:
log.Fatal("Unsupported cloud option")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Include cloud.name in the fatal log so that we can track this down if we ever hit it.

log.Fatalf("Unsupported cloud option: %s", cloud.name)

main.go Outdated
// -
// - **Node 2**: `./scripts/network-iperf-server.sh`
// - **Node 1**: `./scripts/network-iperf-client.sh {ip:2}`
// - **Node 1**: `./scripts/network-ping.sh {ip:2}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run this first, right?

main.go Outdated
runCmd("roachprod", "run", clusterName, "sudo apt-get update -y; sudo apt-get install -y unzip; unzip -o scripts.zip; chmod a+x scripts/*")
fmt.Println("Prepped scripts")

// Execute scripts like this:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we wanted to go crazy, we could make this a little more general to allow for arbitrary benchmarks to be added without having to mess with any of the procedural code. For instance, consider a structure like:

type benchmark struct {
    name      string
    run       func(cluster string)
    artifacts []artifact
}

type artifact struct {
    file string
    node int
}

var benchmarks = []benchmark {
    {
        name: "cpu"
        run: func(cluster string) {
            runCmd("roachprod", "run", cluster+":1", "./scripts/cpu.sh")
        },
        artifacts: []artifact{{"cpu.log", 1}},
    },
    ...
}

for _, b := range benchmarks {
    fmt.Printf("Running %s\n", b.name)
    b.run(clusterName)
    fmt.Printf("Finished %s\n", b.name)
    for _, art := range b.artifacts {
        src := fmt.Sprintf("%s:%d", clusterName, art.node)
        runCmd("roachprod", "get", src, art.file, resultsPath)
    }
}

main.go Outdated
if err != nil {
log.Fatal(err)
}
fmt.Println(out.String(), searchString, strings.Contains(out.String(), searchString))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull strings.Contains(out.String(), searchString) into a variable. Also, why are we printing in some of these functions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The errant prints are just debugging points I hadn't yanked out yet; will make sure that's all cleaned up.

@sploiselle
Copy link
Contributor Author

@nvanbenschoten Realizing I should have broken this up but was never at a good spot to stop and wait for a review. Sorry for the size of the PR. One quick question:

  • Do we need to test both gp2 and io1 EBS volumes?

Copy link
Contributor

@nvanbenschoten nvanbenschoten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This LGTM if it all runs. The structure looks fine and it will be easy for us to add new tests or modify existing ones. Thanks for breaking it out like this.

Do we need to test both gp2 and io1 EBS volumes?

I don't think so. I think we're just going to go with the top of the line io1 EBS volumes.

@sploiselle
Copy link
Contributor Author

Superseded by #3

@sploiselle sploiselle closed this Sep 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants