Skip to content

Spot vm integration #4830

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
Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
Draft

Spot vm integration #4830

wants to merge 1 commit into from
+389 −1

Conversation

@simrankaurb
Copy link
Contributor

@simrankaurb simrankaurb commented Nov 10, 2025 •
edited
Loading

This pull request introduces support for Spot VMs in A3Mega Slurm clusters and adds a new daily test to validate this functionality. It also adds a spot label to instances when Spot VMs are enabled.

Submission Checklist

NOTE: Community submissions can take up to 2 weeks to be reviewed.

Please take the following actions before submitting this pull request.

  • Fork your PR branch from the Toolkit "develop" branch (not main)
  • Test all changes with pre-commit in a local branch #
  • Confirm that "make tests" passes all tests
  • Add or modify unit tests to cover code changes
  • Ensure that unit test coverage remains above 80%
  • Update all applicable documentation
  • Follow Cluster Toolkit Contribution guidelines #

@simrankaurb simrankaurb force-pushed the spot_vm_integration branch 10 times, most recently from 3f11b43 to 28cd55d Compare November 12, 2025 13:08
@simrankaurb simrankaurb marked this pull request as ready for review November 12, 2025 13:09
@simrankaurb simrankaurb requested review from a team and samskillman as code owners November 12, 2025 13:09
@simrankaurb simrankaurb added the release-chore To not include into release notes label Nov 12, 2025
@simrankaurb simrankaurb added release-module-improvements Added to release notes under the "Module Improvements" heading. and removed release-chore To not include into release notes labels Nov 12, 2025
LAVEEN
LAVEEN reviewed Nov 12, 2025
View reviewed changes
tools/cloud-build/test_spot_vm.sh
# See the License for the specific language governing permissions and
# limitations under the License.

set +e
Copy link
Contributor

@LAVEEN LAVEEN Nov 12, 2025

Choose a reason for hiding this comment

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

why are we not existing on error?

Copy link
Contributor Author

@simrankaurb simrankaurb Nov 12, 2025

Choose a reason for hiding this comment

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

I think you mean exit instead of exist. If we exit on error, it will not loop through regions if gcloud create command fails for one region on line 84.

@LAVEEN LAVEEN requested a review from cboneti November 12, 2025 13:27
bytetwin
bytetwin reviewed Nov 12, 2025
View reviewed changes
community/modules/compute/schedmd-slurm-gcp-v6-nodeset/main.tf Outdated
locals {
spot_label = { spot = tostring(var.enable_spot_vm) }
}

Copy link
Collaborator

@bytetwin bytetwin Nov 12, 2025

Choose a reason for hiding this comment

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

What is the rationale for adding this label ?

tools/cloud-build/test_spot_vm.sh
IMAGE_PROJECT=$PROJECT_ID
PROVISIONING_MODEL="SPOT"
TERMINATION_ACTION="DELETE"
OPTIONS_GCS_PATH="gs://hpc-ctk1357/options/options.txt"
Copy link
Collaborator

@bytetwin bytetwin Nov 12, 2025

Choose a reason for hiding this comment

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

Why are we storing the zone and region in a GCS file ? Can they be stored in the script ?

@bytetwin
Copy link
Collaborator

bytetwin commented Nov 12, 2025

/gemini review

gemini-code-assist[bot]
gemini-code-assist bot reviewed Nov 12, 2025
View reviewed changes
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for Spot VMs to A3Mega Slurm clusters, including a new daily test. My review focuses on improving the maintainability and robustness of the new Terraform and Ansible code. I've suggested refactoring repetitive code in Ansible playbooks, improving efficiency in a shell script, and simplifying some logic for better readability. I also found a potential copy-paste error in a build configuration file that could affect test concurrency.

@LAVEEN LAVEEN requested a review from bytetwin November 12, 2025 13:48
@simrankaurb simrankaurb marked this pull request as draft November 13, 2025 12:04
@simrankaurb simrankaurb force-pushed the spot_vm_integration branch 2 times, most recently from 83260c7 to 7bafb7b Compare November 18, 2025 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LAVEEN LAVEEN LAVEEN left review comments

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

@cboneti cboneti Awaiting requested review from cboneti

@bytetwin bytetwin Awaiting requested review from bytetwin

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

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

Assignees

No one assigned

Labels

release-module-improvements Added to release notes under the "Module Improvements" heading.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@simrankaurb @bytetwin @LAVEEN