Skip to content

Conversation

@mrodm
Copy link
Contributor

@mrodm mrodm commented Feb 1, 2023

This PR adds into the Buildkite pipeline that migrates this Jenkins pipeline the following stages/steps:

  • Release step (removed from Jenkinsfile)
  • Integration tests added as a group of steps
    • Used same credentials as in Jenkins

Examples tested:

  • If unit tests or check static steps fail, integration tests are not run:
    no integrations run
  • All tests failing are reported using annotation:
    failing tests different steps
  • Tested re-triggering a building using a comment

Pending

  • Coverage reports
  • Upload artifacts safely

@mrodm mrodm self-assigned this Feb 1, 2023
@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 1, 2023

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2023-02-15T17:29:37.295+0000

  • Duration: 28 min 47 sec

Test stats 🧪

Test Results
Failed 0
Passed 891
Skipped 0
Total 891

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 1, 2023

🌐 Coverage report

Name Metrics % (covered/total) Diff
Packages 100.0% (35/35) 💚
Files 65.909% (87/132) 👍
Classes 61.376% (116/189) 👍
Methods 49.091% (405/825) 👍
Lines 31.725% (3576/11272) 👍
Conditionals 100.0% (0/0) 💚

@mrodm mrodm force-pushed the add_more_stages_buildkite branch from ba27a37 to 4703e65 Compare February 1, 2023 16:26
mrodm added 3 commits February 2, 2023 10:15
In jenkins it is needed, but in Buildkite as the repository is
not shallow, the --unshallow parameter should be removed
This requires to install in the VM created in GCP gvm, golang,
docker-compose , kind and kubectl binaries.
Golang installation requires to add some environment variables to the
PATH as GOPATH.
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from 29c4cf6 to f6c8544 Compare February 2, 2023 09:22
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from adbb44e to a46fc29 Compare February 2, 2023 10:09
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from a46fc29 to 663bab8 Compare February 2, 2023 10:15
@mrodm mrodm force-pushed the add_more_stages_buildkite branch 2 times, most recently from fea81fa to 70d17cb Compare February 2, 2023 11:30
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from 70d17cb to fc6229d Compare February 2, 2023 11:44
@mrodm mrodm force-pushed the add_more_stages_buildkite branch 3 times, most recently from 83081c5 to 0f8f137 Compare February 7, 2023 16:53
Add chocolatey profile

Create target for windows in Makefile
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from 0f8f137 to d62d3ee Compare February 7, 2023 17:06
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from d62d3ee to f6637a5 Compare February 7, 2023 17:12
@mrodm mrodm force-pushed the add_more_stages_buildkite branch from d884a76 to 5454e7c Compare February 7, 2023 18:28
echo " - label: \":go: Running integration test: ${test}\""
echo " command: ./.buildkite/scripts/integration_tests.sh -t ${test}"
echo " agents:"
echo " provider: \"gcp\""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Provider gcp is required to use docker-like commands.

}

GCP_SERVICE_ACCOUNT_SECRET_PATH=secret/ci/elastic-elastic-package/gcp-service-account
AWS_SERVICE_ACCOUNT_SECRET_PATH=secret/ci/elastic-elastic-package/elastic-temp-aws-account-auth
Copy link
Contributor Author

@mrodm mrodm Feb 10, 2023

Choose a reason for hiding this comment

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

Should this be moved to the ci-shared path ?

This credential should be updated.
The advantage of moving it already to the shared path is that no other PR should be required to update this file.

If it is moved, what about this path kv/ci-shared/platform-ingest/aws_account_auth ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this credential to the ci-shared path

@mrodm mrodm marked this pull request as ready for review February 10, 2023 13:09
@mrodm mrodm requested review from alexsapran and jlind23 February 10, 2023 13:10
@mrodm
Copy link
Contributor Author

mrodm commented Feb 10, 2023

/test

cpu: "8"
memory: "4G"

- wait: ~
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we are adding this wait step?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This wait forces that the two steps defined previously (Run check static and Run unit tests) are finished before executing the following ones.

I wanted to force that way, so in case one of these two steps fail, integration tests are not run. Same behavior as in Jenkins.

@mrodm
Copy link
Contributor Author

mrodm commented Feb 13, 2023

As it is now the status of this Buildkite pipeline, there are a some differences from what it is running in Jenkins:

  • in Jenkins, before running the pipeline, the default branch is merged into the changeset pushed, creating a new changeset that will be the one to be tested
    • in Buildkite, the changeset tested is the one pushed to the PR.
  • in Jenkins, there are coverage reports generated and added comments back to the PR.
    • in Buildkite, not able to migrate that feature for now.
  • still pending to store some of the artifacts generated during the progress (e.g. container logs from test/packages/parallel/* tests)

Given that, I would keep the Jenkins workflow for now too, WDYT ? @jsoriano @alexsapran @jlind23

@mrodm mrodm requested review from alexsapran and jsoriano February 13, 2023 16:34
@jlind23
Copy link
Contributor

jlind23 commented Feb 13, 2023

@mrodm I would rather remove the Jenkins job and create follow up issue for Buildkite pipeline otherwise I think we will keep both for a while.

@jsoriano
Copy link
Member

in Jenkins, before running the pipeline, the default branch is merged into the changeset pushed, creating a new changeset that will be the one to be tested

  • in Buildkite, the changeset tested is the one pushed to the PR.

I think we want this, otherwise concurrent PRs can break main branches, specially on busy repositories. It is a pity that Buildkite doesn't offer it.

Maybe we can explore the use of Github merge queues (beta) for this https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue.
If I understand well, once a PR is queued, and before merging it, a temporary branch is created with the merge and the required checks executed.
This is enabled in the branch protection rules of a repository.
Captura desde 2023-02-14 10-37-16

We could give it a try.

in Jenkins, there are coverage reports generated and added comments back to the PR.

  • in Buildkite, not able to migrate that feature for now.

Perhaps we can ignore this by now, and have an explicit task on next sprint to explore alternatives. I see this as a nice to have by now.

Given that, I would keep the Jenkins workflow for now too, WDYT ?

I would only consider a blocker the first point, as this can affect development. For the reports I would be ok if we lose this by now, but have an specific plan to solve it soon.

@mrodm
Copy link
Contributor Author

mrodm commented Feb 14, 2023

Currently, testing for the first point this plugin:
https://github.com/seek-oss/github-merged-pr-buildkite-plugin

Probably, it would require to change some settings in the buildkite pipeline definition to disable Skip pull requests builds for existing commits. To be checked for those PRs created directly in upstream (e.g. PRs created by dependabot).

Instead of using pull/id/merge , a merge commit is created manually.
Github reference is not updated in case there is a conflict and it could
cause that Buildkite runs the pipeline in an old changeset.
@mrodm
Copy link
Contributor Author

mrodm commented Feb 15, 2023

Following the idea in https://github.com/seek-oss/github-merged-pr-buildkite-plugin, it has been added a new post-checkout hook in the repository (.buildkite/hooks/post-checkout) that allows us to have the same behavior as in Jenkins.
After checking out the repository, it performs these steps:

  • Fetch latest changes from target branch (main) and checks out that branch
  • Creates a new temporal branch based on target branch
  • Merges the PR in that temporal branch

That new temporal branch with the merge commit is the one to be tested/run in each step.

This process is repeated for each step in the pipeline.

Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I think that we are increasing the complexity with these buildkite pipelines based on bash, but I guess that we will get used to it as we migrate more things.


- label: "Junit annotate"
- label: ":pipeline: Trigger Integration tests"
command: ".buildkite/pipeline.trigger.integration.tests.sh | buildkite-agent pipeline upload"
Copy link
Member

Choose a reason for hiding this comment

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

There is a bit of complexity here. To run integration tests we have this stack of scripts:

  • .buildkite/pipeline.yml
  • pipeline.trigger.integration.tests.sh
  • ./.buildkite/scripts/integration_tests.sh
  • make targets
  • scripts/test-...

There would be any option to reduce this complexity? Maybe we can remove the make targets and call directly the scripts? Though the make targets can be useful to reproduce locally with the same parameters.

Or most of the things integration_tests.sh does is to install things, could this be also done by providing an image with these dependencies?

Food for thought, no need to do anything about this on this 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.

That's true, there are more scripts than before with Jenkins. All that logic was defined inside the same groovy file (.ci/Jenkinsfile) i.e. triggering dynamically stages in Jenkins depending on the existing test folders, using specific commands in each one.

Maybe we can remove the make targets and call directly the scripts? Though the make targets can be useful to reproduce locally with the same parameters.

As you mention, I would keep make targets since they are helpful to run and test everything locally.

Or most of the things integration_tests.sh does is to install things, could this be also done by providing an image with these dependencies?

Those dependencies installed in integration_tests.sh probably they could be moved to a custom agent image. It would also make the builds faster since each step would not install everything.

@mrodm mrodm merged commit 79ee173 into elastic:main Feb 16, 2023
@mrodm mrodm deleted the add_more_stages_buildkite branch February 16, 2023 14:55
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.

5 participants