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

HDDS-6618. Require successful basic checks for long-running tests #5267

Merged
merged 34 commits into from Sep 19, 2023

Conversation

devmadhuu
Copy link
Contributor

What changes were proposed in this pull request?

Since basic checks are fairly quick, so we can postpone starting acceptance, kubernetes and integration checks until these finish. So this PR addressees following 2 things:

  • Extract basic(unit) task into a separate job.
  • And doesn't start acceptance, kubernetes and integration checks until basic-test(unit) is completed successfully.

Later this basic-test(unit) may be changed to run as part of integration test as basic unit tests also takes significant amount of time to finish.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6618

How was this patch tested?

This patch was tested by running CI jobs with updated ci.yml file.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @devmadhuu for working on this.

doesn't start acceptance, kubernetes and integration checks until basic-test(unit) is completed successfully

These checks should not wait for unit (which is long-running), rather basic (which includes checkstyle, etc.).

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@devmadhuu devmadhuu marked this pull request as draft September 11, 2023 11:18
@devmadhuu devmadhuu marked this pull request as ready for review September 11, 2023 12:18
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @devmadhuu for updating the patch.

On second look, I would like to ask for two additional changes:

  1. native also runs tests and is somewhat slow, so let's move it from basic to the unit check.
  2. With the current implementation (selective_ci_checks.sh outputting the two separate lists of basic/unit checks), moving a check requires too many changes in the script and its test. So I propose keeping selective_ci_checks.* unchanged, and introducing a separate script. This new script would accept the single list of basic checks (including both kinds), and output two lists (one for basic, another one for unit). This way we separate the concerns of "which checks need to be run" and "which basic checks are quick".

@devmadhuu devmadhuu marked this pull request as draft September 15, 2023 10:46
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @devmadhuu for iterating on this patch. I think it's almost good to go now. A few minor tweaks suggested.

Comment on lines 85 to 87
BASIC_CHECKS=$(grep -r '^#checks:' hadoop-ozone/dev-support/checks \
| sort -u | cut -f1 -d':' | rev | cut -f1 -d'/' | rev \
| cut -f1 -d'.')
Copy link
Contributor

Choose a reason for hiding this comment

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

Can be simplified a bit:

  • grep -l only prints paths with match(es), so we can omit the first cut
  • basename extracts the filename out of a path
  • xargs helps apply a command to each item from stdin
Suggested change
BASIC_CHECKS=$(grep -r '^#checks:' hadoop-ozone/dev-support/checks \
| sort -u | cut -f1 -d':' | rev | cut -f1 -d'/' | rev \
| cut -f1 -d'.')
BASIC_CHECKS=$(grep -lr '^#checks:' hadoop-ozone/dev-support/checks \
| sort -u | xargs -n1 basename \
| cut -f1 -d'.')

The same applies to similar commands in selective_basic_ci_checks.sh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @adoroszlai for re-review. I have incorporated your suggestions. Pls check.

Comment on lines 87 to 91
- name: Selective Basic Checks
id: selective-basic-checks
env:
ALL_BASIC_CHECKS: "${{ needs.build-info.outputs.all-basic-checks }}"
run: dev-support/ci/selective_basic_ci_checks.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This step (and its two outputs) can be added as the last step in build-info, no need for a separate job. The step's input needs to be tweaked a bit to ${{ steps.selective-checks.outputs.basic-checks }}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @adoroszlai for re-review. I have incorporated your suggestions. Pls check.

Copy link
Contributor

Choose a reason for hiding this comment

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

The other script is named selective_ci_checks.sh because it decides which checks to run. This one does not filter the checks, rather categorizes them. So I think a better name for this script and the corresponding CI step would be something like categorize_basic_checks.sh and categorize-basic-checks, respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @adoroszlai for re-review. I have incorporated your suggestions. Pls check.

@devmadhuu devmadhuu marked this pull request as ready for review September 19, 2023 05:14
@adoroszlai adoroszlai changed the title HDDS-6618. [perf] Require successful basic checks for long-running tests HDDS-6618. Require successful basic checks for long-running tests Sep 19, 2023
Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @devmadhuu for updating the patch, LGTM.

@adoroszlai adoroszlai merged commit 041fed2 into apache:master Sep 19, 2023
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants