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

Migrate CI job definitions into the .ci directory. #1395

Closed
wants to merge 3 commits into from

Conversation

michaelbaamonde
Copy link
Contributor

This commit migrates our Jenkins job definitions into the Rally repository. It also adds a view called "Rally" to the CI cluster's landing page that includes the jobs defined here.

I tested each of these jobs--except for the docker-release job--with a local Jenkins master that is configured identically to the one controlling https://elasticsearch-ci.elastic.co. It spun up ephemeral workers in GCP to run the builds, which were also configured identically to those that a production build would run on.

Regarding docker-release, I didn't want to assume that running the job with an already-released version of Rally would be a true no-op. The actual contents of the job are identical to what we've been using, but if folks would like to test it out somehow anyway, I'm happy to figure out how to do that safely.

This commit migrates our Jenkins job definitions into the Rally repository. It
also adds a view called "Rally" to the CI cluster's landing page that includes
the jobs defined here.
Copy link
Contributor

@dliappis dliappis left a comment

Choose a reason for hiding this comment

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

This is very very helpful!

Left a few comments. We can sync offline too.

.ci/jobs/defaults.yml Outdated Show resolved Hide resolved
.ci/jobs/defaults.yml Outdated Show resolved Hide resolved
JAVA15_HOME=$HOME/.java/openjdk15
JAVA16_HOME=$HOME/.java/openjdk16
JAVA17_HOME=$HOME/.java/openjdk17
ES_JAVA_HOME=$JAVA17_HOME
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in a future PR we could remove it from here, and move it to the shell script below with something like

#!/usr/bin/env bash

JAVA_ROOT=/var/lib/jenkins/.java

export JAVA7_HOME=${JAVA_ROOT}/java7
export JAVA8_HOME=${JAVA_ROOT}/java8
export JAVA9_HOME=${JAVA_ROOT}/java9
export JAVA10_HOME=${JAVA_ROOT}/java10
export JAVA11_HOME=${JAVA_ROOT}/java11

# handle all Javas after that generically
for java in ${JAVA_ROOT}/openjdk??
do
  version=${java##*openjdk}
  export JAVA${version}_HOME=$java
done

Or we could leverage the latest truth of the latest elasticsearch master branch here: https://github.com/elastic/elasticsearch/blob/master/.ci/java-versions.properties and construct the necessary JAVAX_HOME variables.

Also I think we could remove ES_JAVA_HOME. As I mentioned this is an improvement so could like in a future PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I like this idea!

allow-whitelist-orgs-as-admins: true
trigger-phrase: '.*run\W+tests.*'
github-hooks: true
status-context: tests
Copy link
Contributor

Choose a reason for hiding this comment

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

should this match the name of the Jenkins cluster? e.g. elasticsearch-ci? (it's possible it doesn't matter, would be nice to know what this is about).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's just the name of the PR check. If you take a look at this PR's checks, for example, you'll see one called "tests".

#!/usr/bin/env bash
set -eo pipefail
set +x
VAULT_TOKEN=$(vault write -field=token auth/approle/login role_id="$VAULT_ROLE_ID" secret_id="$VAULT_SECRET_ID")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to test / look a bit deeper here.

For ref check these internal links:

link 1
link 2
link 3

and

link 4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed offline, I'll look into how to test that we're good here.

RALLY_HOME=$WORKSPACE
- shell: |
#!/usr/local/bin/runbld
bash .ci/build.sh build
Copy link
Contributor

Choose a reason for hiding this comment

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

We've faced this issue elsewhere, we need to prefix with set -o errexit because it's possible that tests failed via build.sh but then CI reports success due to the archive function below returning true.

@michaelbaamonde
Copy link
Contributor Author

I'm going to close this one out. After talking with @dliappis, we have some new, better patterns that we're using elsewhere. When I'm able to focus on this work again, I'll re-issue using the latest and greatest.

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.

2 participants