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

BENCH-4272: Retry post-startup script commands that may fail due to to transient network issues #129

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

Conversation

june-hua
Copy link
Contributor

No description provided.

@june-hua june-hua requested review from a team as code owners October 11, 2024 13:58
@june-hua june-hua force-pushed the BENCH-4272-Log-startup-script-errors-to-local-file-same-pattern-as-Vertex-AI branch from 5167a41 to 60f1643 Compare October 11, 2024 14:01
@@ -14,13 +14,15 @@ readonly USER_NAME="${1}"
readonly WORK_DIRECTORY="${2}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exporting the variables that are used by the scripts that are called later. This is because those scripts cannot be sourced if we support retries.
Exporting the variables will make the variables available to those scripts.

Copy link
Contributor

Choose a reason for hiding this comment

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

i see... can we do it per line.

like readonly USER_NAME
export USER_NAME

so when someone wants to delete or change the variable name, it's harder to miss.

set -o nounset
set -o pipefail
set -o xtrace

Copy link
Contributor

Choose a reason for hiding this comment

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

check if cli is installed and only install if it's not


# Startup script status is propagated out to VM guest attributes
readonly STATUS_ATTRIBUTE="startup_script/status"
readonly MESSAGE_ATTRIBUTE="startup_script/message"
EXPORT STATUS_ATTRIBUTE MESSAGE_ATTRIBUTE
Copy link
Contributor

Choose a reason for hiding this comment

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

what is export?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoopsies let me fix that!

@@ -14,13 +14,15 @@ readonly USER_NAME="${1}"
readonly WORK_DIRECTORY="${2}"
Copy link
Contributor

Choose a reason for hiding this comment

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

i see... can we do it per line.

like readonly USER_NAME
export USER_NAME

so when someone wants to delete or change the variable name, it's harder to miss.

June Hua added 2 commits October 11, 2024 11:13
1. Only install CLI if not already installed
2. Update file comments
3. Put export variables in separate line.
@june-hua june-hua force-pushed the BENCH-4272-Log-startup-script-errors-to-local-file-same-pattern-as-Vertex-AI branch from 7ca7e6d to cb9523c Compare October 11, 2024 15:13
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.

2 participants