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

[testing-on-gke part 6.7] Improvements in run script #2498

Open
wants to merge 24 commits into
base: master
Choose a base branch
from
Open

[testing-on-gke part 6.7] Improvements in run script #2498

wants to merge 24 commits into from

Conversation

gargnitingoogle
Copy link
Collaborator

@gargnitingoogle gargnitingoogle commented Sep 17, 2024 •
edited
Loading

Description

Improvements in run-script:

  • handle pod status 'error'
  • unique AR image-name for each run
  • implement only_parse
  • remove unused code
  • fix package-bucket name
  • improve logs
  • use existing cluster machine-type
  • simplify gcloud auth login
  • pass project-id in gcloud commands
  • Remove need to pass absolute paths
  • support setting gcsfuse_branch for cloning
  • Throw error if exactly one out of project_id and project_number has been set.
  • Improve error logs by adding Error: as prefix.

This is on top of #2497 and is followed up in #2499, #2322, and #2528 .

Link to the issue in case of a bug fix.

NA

Testing details

  1. Manual - NA
  2. Unit tests - NA
  3. Integration tests - NA

@gargnitingoogle gargnitingoogle mentioned this pull request Sep 17, 2024
Merged
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch 2 times, most recently from 69dd733 to 06df415 Compare September 17, 2024 07:09
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from b77d5ef to bf0661a Compare September 17, 2024 07:10
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from 06df415 to fb05a44 Compare September 17, 2024 10:18
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch 2 times, most recently from c49f080 to ac9c164 Compare September 17, 2024 10:30
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from fb05a44 to 2d72ed3 Compare September 17, 2024 10:30
@gargnitingoogle gargnitingoogle marked this pull request as ready for review September 17, 2024 10:37
@gargnitingoogle gargnitingoogle requested review from kislaykishore and removed request for a team September 17, 2024 10:37
@gargnitingoogle gargnitingoogle mentioned this pull request Sep 17, 2024
Closed
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from ac9c164 to 4d3d3fb Compare September 18, 2024 06:25
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from 2d72ed3 to b94e91f Compare September 18, 2024 06:25
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from 4d3d3fb to d77db67 Compare September 18, 2024 06:53
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from b94e91f to 58948ee Compare September 18, 2024 06:53
@gargnitingoogle gargnitingoogle mentioned this pull request Sep 18, 2024
Merged
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from d77db67 to 5825918 Compare September 18, 2024 08:17
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from b92fa47 to 0364b7c Compare September 18, 2024 08:18
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from 5825918 to ee6d69c Compare September 18, 2024 08:27
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from 0364b7c to 9613a45 Compare September 18, 2024 08:27
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from ee6d69c to c93fde8 Compare September 18, 2024 08:41
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from 9613a45 to 302395c Compare September 18, 2024 08:41
@gargnitingoogle gargnitingoogle changed the title [testing-on-gke part 6.6] Improvements in run script [testing-on-gke part 6.7] Improvements in run script Sep 18, 2024
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from c93fde8 to 39ece93 Compare September 18, 2024 08:55
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from 302395c to 5cad921 Compare September 18, 2024 08:55
@gargnitingoogle gargnitingoogle mentioned this pull request Sep 18, 2024
Merged
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from 39ece93 to 5d75bca Compare September 18, 2024 17:19
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/enforce-one-pod-per-node branch from d4a44f3 to db1581a Compare October 3, 2024 04:07
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from 9329aae to b4db597 Compare October 3, 2024 04:08
@gargnitingoogle
Add project id<->number autoconversion
6fa2ad2
@gargnitingoogle
support setting gcsfuse_branch for cloning
b26b17e
@gargnitingoogle
Remove need to pass absolute paths
e3783fe
@gargnitingoogle
pass project-id in gcloud commands
8a79b5f
@gargnitingoogle
simplify gcloud auth login
d9973a3
@gargnitingoogle
use existing cluster machine-type
b214e4a
@gargnitingoogle
improve logs
6e81cf1
@gargnitingoogle
fix package-bucket name
a6f3a9c
@gargnitingoogle
remove unused code
d333c07
@gargnitingoogle
implement only_parse
3c54009
@gargnitingoogle
unique AR image-name for each run
62a397e
@gargnitingoogle
handle pod status error
583f995
@gargnitingoogle
make use_custom_csi_driver=true by default
364b2bb
@gargnitingoogle
remove a redundant project_id setting in a gcloud command
e4b4b57
@gargnitingoogle
Add option to force-update existing gcsfuse code
e281249 
Option: force_update_gcsfuse_code
Default: false

This applies when $gcsfuse_src_dir has been passed,
 or when $src_dir/gcsfuse already exists.
@gargnitingoogle
fix implementation of force_fetch_gcsfuse
2f81341
@gargnitingoogle
improve debug log
a89578c
@gargnitingoogle
Add check for missing sudoless-docker
720dfe3 
Adds logs for missing sudoless-docker,
helpful instruction to install it,
and then to re-run the script.
@gargnitingoogle
add some error checks in run-script
6bd2870
@gargnitingoogle
formatting in run-script
d141ea7
@gargnitingoogle gargnitingoogle changed the base branch from garnitin/add-gke-load-testing/enforce-one-pod-per-node to master October 3, 2024 04:55
@gargnitingoogle gargnitingoogle force-pushed the garnitin/add-gke-load-testing/improvements-in-run-script branch from b4db597 to d141ea7 Compare October 3, 2024 04:56
@codecov Codecov
Copy link

codecov bot commented Oct 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.28%. Comparing base (d6bb23e) to head (ec549db).
Report is 1 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #2498      +/-   ##
==========================================
- Coverage   78.32%   78.28%   -0.05%     
==========================================
  Files         107      107              
  Lines       11770    11770              
==========================================
- Hits         9219     9214       -5     
- Misses       2068     2071       +3     
- Partials      483      485       +2
Flag Coverage Δ
unittests 78.28% <ø> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gargnitingoogle
Revert "Add project id<->number autoconversion"
65980c0 
This reverts commit 6fa2ad2.

Reverting this because the above commit exposed PII
in form of the gcsfuse-internal project
IDs and numbers.
@gargnitingoogle
Tie project_id and project_number together
27ebcb6 
The run-gke-tests script should fail
if exactly one out of project_id
and project_number has been set.
It will error out in such as a case.
@gargnitingoogle
Improve error-logs in cli-script
dfbd513 
Add 'Error: ' prefix in all
the error logs for easy spotting
in log files.
@gargnitingoogle
fix couple of comments
ec549db
kislaykishore
kislaykishore reviewed Oct 4, 2024
View reviewed changes
perfmetrics/scripts/testing_on_gke/examples/run-gke-tests.sh
test -n "${project_number}" || export project_number=${DEFAULT_PROJECT_NUMBER}
if test -n "${project_id}"; then
if test -z "${project_number}"; then
echo "Error: project_id was set, but not project_number. Either both should be specified, or neither."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we output these errors to stderr?

perfmetrics/scripts/testing_on_gke/examples/run-gke-tests.sh
@@ -285,19 +343,19 @@ function validateMachineConfig() {
case "${machine_type}" in
"n2-standard-96")
if [ ${num_ssd} -ne 0 -a ${num_ssd} -ne 16 -a ${num_ssd} -ne 24 ]; then
echo "Unsupported num-ssd "${num_ssd}" with given machine-type "${machine_type}". It should be 0, 16 or 24"
echo "Error: Unsupported num-ssd "${num_ssd}" with given machine-type "${machine_type}". It should be 0, 16 or 24"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as before - Should we output these errors to stderr?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kislaykishore kislaykishore kislaykishore left review comments

@Tulsishah Tulsishah Awaiting requested review from Tulsishah Tulsishah is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gargnitingoogle @kislaykishore