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.

Sign up for GitHub

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

Jump to bottom

Tune etcd presubmit jobs #32731

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Tune etcd presubmit jobs #32731

merged 3 commits into from
Jun 13, 2024

Conversation

ivanvc
Copy link
Member

@ivanvc ivanvc commented Jun 10, 2024 •
edited

Tuning the integration and 386 unit test jobs after reviewing Grafana and Testgrid for the last two weeks.

Part of etcd-io/etcd#18065, kubernetes/k8s.io#6102.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 10, 2024
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/config Issues or PRs related to code in /config area/jobs sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 10, 2024
@ivanvc
Copy link
Member Author

ivanvc commented Jun 10, 2024

/cc @jmhbnz

@k8s-ci-robot k8s-ci-robot requested a review from jmhbnz June 10, 2024 23:55
jmhbnz
jmhbnz reviewed Jun 11, 2024
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

Thanks @ivanvc - Question around artifacts below. I'm still getting my head around how we are supposed to make artifacts work in prow.

config/jobs/etcd/etcd-presubmits.yaml
@@ -168,6 +168,7 @@ presubmits:
- |
set -euo pipefail
make gofail-enable
export JUNIT_REPORT_DIR=${ARTIFACTS}
Copy link
Member

Choose a reason for hiding this comment

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

Curious on how this artifact dir is intended to work? Is there a bucket these artifacts will be stored in after the job has completed?

Copy link
Member Author

Choose a reason for hiding this comment

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

They show up in the "Artifacts" tab of a build. For example, clicking "Artifacts" on build 1800476731895713792 takes you to the bucket. Inside there's an "artifacts" directory with the JUnit XML results.

I'm porting the following fragment, which seems to have been overlooked in the initial migration: https://github.com/etcd-io/etcd/blob/57907746e39da97ed3ce2fbd1f97e8ceb2ce8199/.github/workflows/tests-template.yaml#L69-L73.

However, I'm open to dropping saving the artifacts if we don't find value in storing them 🙂.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation (and for updating it into etcd-io/etcd docs with screenshot) this is great functionality lets continue to save these.

Copy link
Member Author

Choose a reason for hiding this comment

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

To be clear and 100% transparent, the screenshots you saw were from the Robustness tests. These are JUnit results 😄

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2024
@ivanvc
Store etcd presubmit integration tests JUnit reports
0c5ba94
@ivanvc
Tune etcd presubmit integration 2 and 4 CPUs tests
170c9be 
Adjust memory and CPU requests accordingly to Grafana. Remove the
optional argument after successful runs.
@ivanvc
Tune etcd presubmit unit 386 job
cd68949 
Adjust memory and CPU requests accordingly to Grafana. Remove the
optional argumentafter successful runs.
@ivanvc ivanvc force-pushed the tune-etcd-presubmit-integration-tests branch from f6061e1 to cd68949 Compare June 13, 2024 16:10
@ivanvc ivanvc changed the title Tune etcd presubmit integration tests Tune etcd presubmit jobs Jun 13, 2024
@ivanvc ivanvc requested a review from jmhbnz June 13, 2024 16:10
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 13, 2024
jmhbnz
jmhbnz approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@jmhbnz jmhbnz left a comment •
edited

Choose a reason for hiding this comment

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

Thanks @ivanvc
/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 13, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ivanvc, jmhbnz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 13, 2024
@k8s-ci-robot k8s-ci-robot merged commit a1430e9 into kubernetes:master Jun 13, 2024
7 checks passed
@k8s-ci-robot
Copy link
Contributor

@ivanvc: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key etcd-presubmits.yaml using file config/jobs/etcd/etcd-presubmits.yaml

In response to this:

Tuning the integration and 386 unit test jobs after reviewing Grafana and Testgrid for the last two weeks.

Part of etcd-io/etcd#18065, kubernetes/k8s.io#6102.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@ivanvc ivanvc deleted the tune-etcd-presubmit-integration-tests branch June 13, 2024 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jmhbnz jmhbnz jmhbnz approved these changes

@ahrtr ahrtr Awaiting requested review from ahrtr

@wenjiaswe wenjiaswe Awaiting requested review from wenjiaswe

Assignees

@jmhbnz jmhbnz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/jobs cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/etcd Categorizes an issue or PR as relevant to SIG Etcd. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ivanvc @k8s-ci-robot @jmhbnz