add Slurm hybrid cloud burst blueprints and instructions - #6262
Conversation
…s for GCP and GCD Change-Id: Ibf93be1024a99cf8d504f997ef304417589f0da8 Signed-off-by: Jun Tang <juntangc@google.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces significant enhancements to the HPC Cluster Toolkit, primarily focusing on hybrid cloud bursting capabilities and improved configuration management. It adds comprehensive blueprints for Slurm hybrid cloud bursting, refines the CLI configuration experience, and introduces automated test infrastructure management. Additionally, it includes updates to support ML Diagnostics and improves the robustness of CI/CD test pipelines. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces interactive configuration editing for 'gcluster job config', adds support for GKE ML Diagnostics, automates GCS Fuse Anywhere Cache IAM role creation, and adds Slurm blueprints for Google Cloud Dedicated (GCD) and hybrid cloud bursting. The review feedback highlights a potential runtime 'NameError' in 'util.py' due to missing imports, an insecure SSH configuration in the GCD Slurm blueprint, unhandled file read errors in the configuration editor, and insecure file permissions ('0644') for the saved configuration file.
| if universe_domain() != DEFAULT_UNIVERSE_DOMAIN: | ||
| assert retry_cb is None, "retry_cb not supported for non-default universe domains" |
There was a problem hiding this comment.
The newly added batch_execute logic uses ThreadPoolExecutor and as_completed without importing them (or concurrent.futures) at the top of util.py. This will cause a NameError at runtime in custom universe domains. Importing them inline inside the if block is a safe way to resolve this.
| if universe_domain() != DEFAULT_UNIVERSE_DOMAIN: | |
| assert retry_cb is None, "retry_cb not supported for non-default universe domains" | |
| # Custom universe domains (GCD / Sovereign Cloud) do not support BatchHttpRequest. | |
| if universe_domain() != DEFAULT_UNIVERSE_DOMAIN: | |
| from concurrent.futures import ThreadPoolExecutor, as_completed | |
| assert retry_cb is None, "retry_cb not supported for non-default universe domains" |
| # 3. Disable StrictHostKeyChecking safely for internal network jumps only | ||
| if ! grep -q "Host 10.\*" /etc/ssh/ssh_config; then | ||
| if ! grep -q "StrictHostKeyChecking no" /etc/ssh/ssh_config; then | ||
| echo -e "Host 10.* 172.16.* 192.168.* *.internal *\n StrictHostKeyChecking no\n UserKnownHostsFile=/dev/null" >> /etc/ssh/ssh_config |
There was a problem hiding this comment.
Disabling StrictHostKeyChecking globally by appending a trailing * to the Host pattern list poses a significant security risk (man-in-the-middle vulnerability) for any external SSH connections initiated from the cluster. It is safer to restrict this configuration strictly to the internal IP ranges and domains.
echo -e "Host 10.* 172.16.* 192.168.* *.internal\n StrictHostKeyChecking no\n UserKnownHostsFile=/dev/null" >> /etc/ssh/ssh_config| fileData, err := os.ReadFile(sourcePath) | ||
| if err == nil { | ||
| if _, err := tempFile.Write(fileData); err != nil { | ||
| _ = tempFile.Close() | ||
| _ = os.Remove(tempFileName) | ||
| return "", fmt.Errorf("failed to write to temp file: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
If os.ReadFile fails with an error other than os.ErrNotExist (e.g., permission denied), the error is silently ignored. This results in an empty temp file being created, which will subsequently overwrite the user's existing configuration with empty or invalid data when saved. The error should be handled explicitly.
| fileData, err := os.ReadFile(sourcePath) | |
| if err == nil { | |
| if _, err := tempFile.Write(fileData); err != nil { | |
| _ = tempFile.Close() | |
| _ = os.Remove(tempFileName) | |
| return "", fmt.Errorf("failed to write to temp file: %w", err) | |
| } | |
| } | |
| fileData, err := os.ReadFile(sourcePath) | |
| if err != nil && !os.IsNotExist(err) { | |
| _ = tempFile.Close() | |
| _ = os.Remove(tempFileName) | |
| return "", fmt.Errorf("failed to read source config file: %w", err) | |
| } | |
| if err == nil { | |
| if _, err := tempFile.Write(fileData); err != nil { | |
| _ = tempFile.Close() | |
| _ = os.Remove(tempFileName) | |
| return "", fmt.Errorf("failed to write to temp file: %w", err) | |
| } | |
| } |
| return fmt.Errorf("config file contains structural errors or invalid JSON: %w", err) | ||
| } | ||
|
|
||
| if err := os.WriteFile(targetFile, tempFileData, 0644); err != nil { |
There was a problem hiding this comment.
The configuration file contains project IDs, cluster names, and locations. Saving it with 0644 permissions allows other local users on the system to read this configuration. It is highly recommended to restrict the file permissions to the owner only (0600).
| if err := os.WriteFile(targetFile, tempFileData, 0644); err != nil { | |
| if err := os.WriteFile(targetFile, tempFileData, 0600); err != nil { |
|
Hi @juntangc , Thanks for the contrinbution. Just FYI: Please avoid raising PRs directly to the main branch, I have changed the base branch to develop. Please check if the commit now covers all the changes you have added. |
3 participants
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.