Replace prometheus-builder by promrunner

[simonpasquier]

This PR introduces the promrunner program: it takes a PR number (+ CircleCI token), downloads the prometheus binary from the CircleCI artifact (see prometheus/prometheus#4685) and replaces itself with the prometheus program.

The main advantage is that it doesn't require to rebuild the prometheus binary from sources and prombench uses the exact same binary that should be delivered to our users. One concrete example is that it would allow to compare performances between binaries compiled with go1.10 and go1.11 (see prometheus/prometheus#4626).

[krasi-georgiev]

yep in general very good idea, but adding another dependency would make running tests without Prow more difficult #160
Not a big blocker, but also we would be locked into Circle CI

another idea is to push a docker image for every PR, but than how do we cleanup these when we are done

or maybe use promu or golang-builder with a multi stage docker - 1 stage build , 2 stage run
I think we should be able to specify the dolang version to be used for the build, no?

[simonpasquier]

adding another dependency would make running tests without Prow more difficult #160

You can use a CircleCI account token instead of a project token (in fact this is what I used for testing). Anybody with a GitHub or BitBucket account can login to CircleCI, request a token and download build artifacts from the prometheus project (I've just checked with a BitBucket account and it works fine).

The difference between and account and project token is that the project token isn't tied to a user and is limited in scope (it can only read data, no risk to modify something if it leaks).

Not a big blocker, but also we would be locked into Circle CI

I'd say that if we were to replace CircleCI, the substitute would need to offer a similar feature for downloading artifacts.

another idea is to push a docker image for every PR, but than how do we cleanup these when we are done

There is prometheus/prometheus#4453 but as you said, I'm not sure how to keep things clean.

or maybe use promu or golang-builder with a multi stage docker - 1 stage build , 2 stage run
I think we should be able to specify the dolang version to be used for the build, no?

It would still be required to maintain this specific build process in sync with the prometheus CI.

[simonpasquier]

cc @SuperQ which might be interested by the discussion too.

[sipian]

yep in general very good idea, but adding another dependency would make running tests without Prow more difficult

I don't see any dependence on prow for promrunner. The circelci token is passed as a volume-mount.
Even if in the future some other CI system is used, these will be added as secrets or env.

promu crossbuild has an option of adding go version.
It uses docker run quay.io/prometheus/golang-builder:<GO_VERSION>-base to build the images.
But then we would need to somehow fetch the golang version from .circle/config.yml or travis.yml

[krasi-georgiev]

yep in general very good idea, but adding another dependency would make running tests without Prow more difficult

my point is to keep things as simple as possible for people wanting to run tests from the cli.
adding yet another token adds another step in the process. I want to see less steps, not more :) especially if it can be avoided.

promu crossbuild has an option of adding go version.
It uses docker run quay.io/prometheus/golang-builder:<GO_VERSION>-base to build the images.
But then we would need to somehow fetch the golang version from .circle/config.yml or travis.yml

If we can select the version in the golang-builded why do we even need to involve circle?
Why not pass the go version to be used as argument to prow in the comment?

/benchmark  // uses the latest stable golang and tests against master branch
/benchmark go=1.9 // the same as /benchmark v=master go=1.9 
/benchmark v=2.1.1 go=1.9 

// or even just

/benchmark go1.9 // the same as /benchmark master go1.9 
/benchmark 2.1.1 go1.9 

or whichever format would be easier to parse maybe even where we wouldn't have to remember the arguments order

The only disadvantage is that it would still build it on every run. At the moment it takes less than a minute to clone/build/run Prometheus so not a big deal.

[krasi-georgiev]

It would still be required to maintain this specific build process in sync with the prometheus CI.

I don't understand what you mean here, my idea is to use promu which is what is used in the CI so the final results should be identical, no?

[simonpasquier]

Using the artifacts from the upstream CI, we don't have to think about how to build the binary. That being said, I understand your concerns so maybe I can add an option to either get the binary from the CircleCI build or build it from the PR sources?

[krasi-georgiev]

I am really anti adding more depenancies in any form, but let me read the discussion again in few days and will see what I think then 👍

Using the artifacts from the upstream CI, we don't have to think about how to build the binary.

but that would mean knowing the circle API to be able to troubleshoot and review the promrunner PRs so not sure it really means less maintenance overhead. And if we can use promu than it is close enough with hardly any maintenance(since promu is maintained for the CI anyway).

@sipian what do you think?

@simonpasquier promrunner is a great name btw.

[sipian]

I have one question.
How long will the circle-ci artifacts stay in circle-ci.
Acc to https://support.circleci.com/hc/en-us/articles/115014004948-Uses-for-artifacts-and-limitations- and https://circleci.com/docs/2.0/artifacts/#artifacts-overview

We don't make guarantees about how long artifacts are available for. If you're relying on them as a source of documentation / persistent content, we recommend deploying the output to a dedicated output target.

It might happen that a user starts benchmarking after a long time -> when the artifacts are no longer there in circle-ci.

[sipian]

OTOH, passing the go version in the github comment seems a bit tedious from the user point of view. The user might expect prombench to take care of this on its own.
But I do not see a cleaner way around this.
We could try reading the go version value from .circle/config.yaml or travis.yml but this sounds hacky and prone to error.

[krasi-georgiev]

In 99% cases it wont be needed. When no go version is provided it will use the default

[simonpasquier]

It might happen that a user starts benchmarking after a long time -> when the artifacts are no longer there in circle-ci.

The information I've found when looking at artifacts was that they are able for "months" [1]. But I guess the real answer is that there's no official SLA. If the artifacts are gone, you can still kick another build though.

[1] https://circleci.com/docs/2.0/concepts/#workspaces-and-artifacts

I agree with @sipian that passing the Go version is cumbersome. Personally I don't like that prombench tries to mimic the original build process while we could just use its output. And it would even better if we could just use the resulting container image.

[krasi-georgiev]

I agree with @sipian that passing the Go version is cumbersome. Personally I don't like that prombench tries to mimic the original build process while we could just use its output. And it would even better if we could just use the resulting container image.

I also don't like that we are building the image and I am all in for using a container image pushed from the CI. I guess all we need to figure out is about the cleanup of obsolete PR images. Only if docker hub or any other registry had some settings to expire images based on some criteria.

[sipian]

@simonpasquier
I will be out for a few weeks. If it is all right, I will look into this PR after I return.

[krasi-georgiev]

if we use docker images this would make it possible to run local benchmarks even without opening a PR.
We can take the docker image as an argument which will be used for the benchmarking

so instead of

      containers:
      - name: prometheus
        image: docker.io/prombench/prometheus-builder:2.0.0
        args: ["{{ .PR_NUMBER }}"]

we would have

      containers:
      - name: prometheus
        image: {{ .PROMETHEUS_IMAGE }}

this is in https://github.com/prometheus/prombench/blob/8b8828e4967a14fa1ec1a404927f289f4a3c6356/components/prombench/manifests/benchmark/3_prometheus-test.yaml#L736-L750

[simonpasquier]

Even if not for prombench, it can be useful sometime to have the PR image available for people to test (this has been brought up in prometheus/prometheus#4453 too). Again taking the go1.10 -> go1.11 migration as an example, prombench might not be enough to assert that nothing is broken as it doesn't test all possible configurations (see prometheus/prometheus#4626 (comment)).

What I can imagine is the ability for the team to trigger a CI job that would publish the PR image to DockerHub and/or Quay. By having it at the discretion of the maintainers, it won't require special cleaning. Then prombench could trigger this job when a benchmark is started.

[krasi-georgiev]

yep that sounds possible , if you can do some research and see what we can come up that would be great.

[simonpasquier]

I've dug further and it isn't feasible with CircleCI to push a container image from a forked PR (neither with Travis btw). This boils down to security concerns: to push the image, the CI system needs access to the hub's credentials and if this data were available to forked PRs, a malicious user could potentially expose it to 3rd party services.

That being said, there's still ways to address the problem. I can imagine the following process:

1. When Prow gets notified by /benchmark, it takes the PR code and pushes it to a repository's branch (eg pr/XXX).
2. CircleCI runs on the branch and detecting the pr/ prefix, it builds then pushes the image to Docker and/or Quay.
3. Once the CircleCI build is successfully completed, Prow can trigger the creation of the benchmark cluster.

Obviously there should be a configuration flag to skip these steps when testing locally.

[sipian]

@simonpasquier
The approach sounds good.
I presume this will use a different repo than the main Prometheus repo.
Primarily because we don't want circleci build process to take too much time to start.
This can lead to premature timeouts.

[krasi-georgiev]

Now that we are working on migrating to github actions I think we can have a step to build and push an image at /benchmark and delete it at /benchmark cancel

To continue supporting local tests without a CI would need to refactor prombench to take docker images as parameters instead of a PR_NUMBER

so instead of
RELEASE=<master or any prometheus release(ex: v2.3.0) >
PR_NUMBER=<PR to benchmark against the selected $RELEASE>

we would have
DOCKER_MAGE_1
DOCKER_MAGE_2

and the tests will use these specified docker images.

Now for running tests locally without a CI it means that people would need to push an images to a docker registry manually, but this would also enable them to run tests even without opening a PR so I think it is a good trade off.

[geekodour]

Here's an approach that can get us rid of the prometheus-builder, following @krasi-georgiev 's comment on building a docker image and using circle ci artifacts as @simonpasquier suggested:

1. we get rid of prometheus-builder
   https://github.com/prometheus/prombench/blob/aeaf29a9c7b1e23785ef9d3412fca8bd9ad98491/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml#L35-L45
   and replace the busybox image with an image that's build on each /benchmark
   https://github.com/prometheus/prombench/blob/aeaf29a9c7b1e23785ef9d3412fca8bd9ad98491/manifests/prombench/benchmark/3b_prometheus-test_deployment.yaml#L48

The value of image will now be set through a DeploymentVar, which in turn will be set by some combination of DOCKER_REPO and PR_NUMBER environment variables.

Now about how to build this image,
For running locally:

for running tests locally without a CI it means that people would need to push an images to a docker registry manually

When using Github Actions

• set DOCKER_USER, DOCKER_PASS, CIRCLECI_TOKEN secrets
• create a Github Action(we can name this action promrunner maybe) that will use the docker and circle ci credentials. ( FROM https://hub.docker.com/_/docker)
• This action will use CIRCLE_CI token to download the artifact (this can be done in a bash script), then use docker to push this to DOCKER_REPO+PR_NUMBER image on DockerHub.

When /benchmark cancel is run we can use the same github action to delete the image in this way

cc @krasi-georgiev @simonpasquier @sipian

let me know what you think of this approach.

[krasi-georgiev]

yes that sounds like a good idea, will try it.

[bwplotka]

👋🏽 Looks like there was no activity on this PR for some time, closing for now, we can always reopen if still relevant/to-be-reviewed. Thanks!