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

[CI] Enable packaging with Pipeline 2.0 #23854

Merged
merged 50 commits into from
Feb 11, 2021
Merged

Conversation

v1v
Copy link
Member

@v1v v1v commented Feb 4, 2021

What does this PR do?

Proposal to run the packaging in parallel on a PR basis.

See the follow ups section regarding what are the different areas that could be done in the future.

Why is it important?

Split the packaging per beat in order to validate them on a PR basis.

Tasks

  • Test
  • E2E disabled at the moment but already implemented
  • publish packages and docker images

Follow ups

  • Enable e2e
  • ARM support if possible
  • MAC support if possible
  • Deprecate the existing packaging pipeline in favour of this approach

Issues

Requires #23960
Caused elastic/e2e-testing#705

@v1v v1v added automation Team:Automation Label for the Observability productivity team labels Feb 4, 2021
@v1v v1v self-assigned this Feb 4, 2021
@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Feb 4, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Feb 4, 2021

💚 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

Expand to view the summary

Build stats

  • Build Cause: Pull request #23854 updated

  • Start Time: 2021-02-10T18:11:18.191+0000

  • Duration: 57 min 33 sec

  • Commit: 024b732

Test stats 🧪

Test Results
Failed 0
Passed 45794
Skipped 4762
Total 50556

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 45794
Skipped 4762
Total 50556

Jenkinsfile Outdated Show resolved Hide resolved
//'linux/s390x',
'windows/amd64',
'windows/386',
(params.macos ? '' : 'darwin/amd64'),
Copy link
Member Author

Choose a reason for hiding this comment

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

Let's see what we do with this

@@ -0,0 +1,18 @@
when:
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new step, though there is no build in place yet but the packaging itself.

Jenkinsfile Outdated Show resolved Hide resolved
@urso
Copy link

urso commented Feb 10, 2021

Does this change mean the pr-merge will only be green if packaging and E2E tests did succeed? Especially E2E tests are quite red recently and flakyness in Beats tests has increased again.

With this change, will we run all E2E tests? E.g. if I modify filebeat, will metricbeat packaing or auditbeat E2E test be able to mark my PR as failed?

Regarding running packaging automatically. I would expect that packaging jobs are run when any magefile, Makfile, dev-tools or scripts directory is touched in a PR.

@v1v
Copy link
Member Author

v1v commented Feb 10, 2021

Does this change mean the pr-merge will only be green if packaging and E2E tests did succeed? Especially E2E tests are quite red recently and flakyness in Beats tests has increased again.

Exactly, if the concern is regarding the E2E flakiness, I think there were a couple of issues here:

  • some regressions that were not detected since the master branch got broken. (See later on an example of this particular scenario)
  • packages were not generated for x-pack for the last 3 days.

This particular approach will help to detect earlier any broken changes before any merges.

cc @mdelapenya as he can clarify the above.

With this change, will we run all E2E tests? E.g. if I modify filebeat, will metricbeat packaing or auditbeat E2E test be able to mark my PR as failed?

No, E2E will only run for those beats that got E2E to run and only if the changes are related to that particular beat. In other words, if you modify filebeat then only the packaging for filebeat will be executed, and if there are E2E for filebeat then it will be triggered afterwards, if no E2E then nothing will happen but the packaging.

Regarding running packaging automatically. I would expect that packaging jobs are run when any magefile, Makfile, dev-tools or scripts directory is touched in a PR.

Both systems will coexist and eventually we will move to this approach. The more couple the build/test/packaging/e2e stages are the sooner we can find regressions/bugs on a PR basis.

The current approach for every commit in the master branch, or any, follow the below steps:

  • Build/Test/IngTest all the beats.
    • If the above passed then, it runs the Packaging and E2E stage for each beat.
    • If it does not pass, then no Packaging and E2E will be executed

If no packaging and E2E are executed after a few commits, then it means the E2E might not detect regressions and the git bisect between the last successful Packaging and E2E and the latest commit might not be easy to do so.

In order to help with the analysis, we want to shift left all the validations that run by default in any branch. And ensure those validations are synchronous, and move away from the perception that merging on a broken branch does not produce slowness to everyone.

In a nutshell, this process does nothing but validating the build/test/cross-compile/ing-testing/packaging/e2e in parallel for the specific beat. Therefore it's similar what we got already in place but with the addition of the packaging, the e2e will only happen if there is something configured for that particular beat.

@urso
Copy link

urso commented Feb 10, 2021

The current approach for every commit in the master branch, or any, follow the below steps:

Build/Test/IngTest all the beats.

  • If the above passed then, it runs the Packaging and E2E stage for each beat.
  • If it does not pass, then no Packaging and E2E will be executed

If no packaging and E2E are executed after a few commits, then it means the E2E might not detect regressions and the git bisect between the last successful Packaging and E2E and the latest commit might not be easy to do so.

+1 on running packaging, E2E and tests in parallel. When testing branches, no tests should be excluded.

  • some regressions that were not detected since the master branch got broken. (See later on an example of this particular scenario)
  • packages were not generated for x-pack for the last 3 days.

This particular approach will help to detect earlier any broken changes before any merges.

Again +1 for always running all CI on master and release/development branches.

This also shows shortcomming in alerting. We should be able to build from master and 7.x at any time. If builds go red, the sirens should turn on. No packages for 3 days without anyone investigating for 3 workdays is unacceptable. We introduced a Build Manager Rotation last week having Beats developers help in analyzing and fixing tests. Are we missing discoverability/alerting?

No, E2E will only run for those beats that got E2E to run and only if the changes are related to that particular beat. In other words, if you modify filebeat then only the packaging for filebeat will be executed, and if there are E2E for filebeat then it will be triggered afterwards, if no E2E then nothing will happen but the packaging.

+1 on testing only based on related changes. If I modify filebeat, will x-pack/filebeat packaging and E2E tests be run as well?
Changing metricbeat internals might also require auditbeat testsuite to be run.

In general there are serious issues in the process that need to be addressed. Not just during PRs but regular alerting as well, and us getting active. I see how the approach can help, but given that CI is not as stable as it should be yet, I worry about running everything on each PR potentially affecting the developer experience negatively, as we will need to run more things in parallel and increase the amount of sometimes unrelated tests that must pass.

Jenkinsfile Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
@v1v v1v marked this pull request as ready for review February 10, 2021 18:11
@v1v v1v requested review from a team as code owners February 10, 2021 18:11
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ingest-management (Team:Ingest Management)

@v1v v1v merged commit dd12389 into elastic:master Feb 11, 2021
@v1v v1v deleted the feature/packaging-e2e-2.0 branch February 11, 2021 15:53
v1v added a commit to v1v/beats that referenced this pull request Feb 11, 2021
@v1v v1v mentioned this pull request Feb 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automation Team:Automation Label for the Observability productivity team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants