Develop to main test PR - #6111
Conversation
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 significantly enhances the Cluster Toolkit's GKE job submission capabilities and streamlines the user experience for cluster management. Key improvements include better support for custom Kubernetes templates, flexible namespace targeting, and clearer dependency management. Additionally, it enforces Slurm Native Authentication by removing legacy MUNGE configuration options and updates the CI/CD daily test infrastructure for better reliability and resource cleanup. 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 migrates daily test pipelines to run as GKE Kueue jobs, introduces support for custom GKE namespaces and manifest templates in the gcluster job commands, and sets Slurm Native Authentication as the default standard across blueprints and modules. Feedback highlights duplicate hash entries in the updated cryptography package requirements and requests the removal of accidentally committed .bak backup files.
Neelabh94
added
do-not-merge
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request significantly updates the Cluster Toolkit by enhancing documentation, introducing support for custom GKE templates, and implementing namespace-aware job management. It also updates key dependencies, enforces Slurm Native Authentication by removing the 'enable_slurm_auth' variable across examples, and migrates daily integration tests to run as GKE Kueue jobs. I have no feedback to provide.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to GKE job orchestration, including support for custom namespaces, GKE template extraction, and advanced GKE infrastructure features such as dynamic TPU slicing and Node Auto-Provisioning. It also migrates daily CI/CD tests to use GKE Jobs with Kueue and updates several dependencies. Feedback includes addressing a silent fallback in namespace resolution and removing redundant backup files.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for disk_storage_pool across multiple Terraform modules (including Slurm nodesets, partitions, login nodes, GKE node pools, and VM instances) to enable the use of Google Cloud storage pools with Hyperdisks. It also updates TPU shorthand mappings in Go, adds extended telemetry error parsing, and migrates several Slurm GCP v6 integration tests to GKE Kueue in Cloud Build. However, multiple newly added Cloud Build YAML files contain unescaped bash variable references (such as ${ANSIBLE_PID:-} and ${SCRIPT_PID:-}) that must be escaped with $$ to prevent Cloud Build validation failures.
| echo "CAUGHT SIGTERM OR SCRIPT ERROR!" | ||
| echo "Halting primary Ansible execution..." | ||
| echo "==========================================================================" | ||
| if [ -n "\$${ANSIBLE_PID:-}" ]; then |
There was a problem hiding this comment.
In Cloud Build YAML configurations, variable references like ${ANSIBLE_PID:-} must have their dollar sign escaped using $$ (i.e., $${ANSIBLE_PID:-}) to prevent Cloud Build from attempting to interpret them as Cloud Build substitutions. Leaving it as a single $ will cause Cloud Build validation to fail or the build to crash.
if [ -n "$${ANSIBLE_PID:-}" ]; thenReferences
- In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g.,
$${VAR}). Avoid adding an extra dollar sign (like $$ ${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.
| echo "CAUGHT SIGTERM OR SCRIPT ERROR!" | ||
| echo "Halting primary script execution..." | ||
| echo "==========================================================================" | ||
| if [ -n "\$${SCRIPT_PID:-}" ]; then |
There was a problem hiding this comment.
In Cloud Build YAML configurations, variable references like ${SCRIPT_PID:-} must have their dollar sign escaped using $$ (i.e., $${SCRIPT_PID:-}) to prevent Cloud Build from attempting to interpret them as Cloud Build substitutions. Leaving it as a single $ will cause Cloud Build validation to fail or the build to crash.
if [ -n "$${SCRIPT_PID:-}" ]; thenReferences
- In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g.,
$${VAR}). Avoid adding an extra dollar sign (like $$ ${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.
| echo "CAUGHT SIGTERM OR SCRIPT ERROR!" | ||
| echo "Halting primary Ansible execution..." | ||
| echo "==========================================================================" | ||
| if [ -n "\$${ANSIBLE_PID:-}" ]; then |
There was a problem hiding this comment.
In Cloud Build YAML configurations, variable references like ${ANSIBLE_PID:-} must have their dollar sign escaped using $$ (i.e., $${ANSIBLE_PID:-}) to prevent Cloud Build from attempting to interpret them as Cloud Build substitutions. Leaving it as a single $ will cause Cloud Build validation to fail or the build to crash.
if [ -n "$${ANSIBLE_PID:-}" ]; thenReferences
- In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g.,
$${VAR}). Avoid adding an extra dollar sign (like $$ ${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.
| echo "CAUGHT SIGTERM OR SCRIPT ERROR!" | ||
| echo "Halting primary Ansible execution..." | ||
| echo "==========================================================================" | ||
| if [ -n "\$${ANSIBLE_PID:-}" ]; then |
There was a problem hiding this comment.
In Cloud Build YAML configurations, variable references like ${ANSIBLE_PID:-} must have their dollar sign escaped using $$ (i.e., $${ANSIBLE_PID:-}) to prevent Cloud Build from attempting to interpret them as Cloud Build substitutions. Leaving it as a single $ will cause Cloud Build validation to fail or the build to crash.
if [ -n "$${ANSIBLE_PID:-}" ]; thenReferences
- In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g.,
$${VAR}). Avoid adding an extra dollar sign (like $$ ${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.
| echo "Halting primary Ansible execution..." | ||
| echo "==========================================================================" | ||
| if [ -n "\$${ANSIBLE_PID:-}" ]; then | ||
| kill -TERM \$$ANSIBLE_PID 2>/dev/null || true |
There was a problem hiding this comment.
In Cloud Build YAML configurations, variable references like ${ANSIBLE_PID:-} must have their dollar sign escaped using $$ (i.e., $${ANSIBLE_PID:-}) to prevent Cloud Build from attempting to interpret them as Cloud Build substitutions. Leaving it as a single $ will cause Cloud Build validation to fail or the build to crash.
if [ -n "$${ANSIBLE_PID:-}" ]; thenReferences
- In Cloud Build YAML configurations, escaping a dollar sign for variable references is correctly done using $$ (e.g.,
$${VAR}). Avoid adding an extra dollar sign (like $$ ${VAR}), as it leaves an unescaped ${VAR} which will cause Cloud Build validation to fail or the build to crash.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several significant enhancements to the Cluster Toolkit, primarily focusing on dependency management, storage configuration, and CI/CD improvements. Key changes include the implementation of an explicit dependency check mechanism for tools like Terraform and Packer, the addition of storage pool support for various compute and storage modules, and updates to the daily test CI/CD pipelines to utilize GKE Kueue for job management. Additionally, the PR updates the Google provider version range and includes various bug fixes and improvements to job logging and error handling.
Co-authored-by: Ishita Chail <ishitachail@google.com>
15 participants
This PR allows us to fix the suggestions from the gemini, So that it will be hassle free for the RC PR.
Submission Checklist
NOTE: Community submissions can take up to 2 weeks to be reviewed.
Please take the following actions before submitting this pull request.