Repository navigation
[DO NOT MERGE] Continuous Review: develop -> main - #6644
ctk-pr-bot wants to merge 38 commits into
Conversation
…#6141) Co-authored-by: ep-nag <ellen.perry@nag.com>
…ions (#6604) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…6602) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…Gemma vLLM benchmark (#6427) Co-authored-by: agrawalkhushi18 <agrawalkhushi@google.com>
Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
…template module (#6399) Co-authored-by: AdarshK15 <adarshkullachi@google.com>
…benchmarks (#6625) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…h4d-hpc-benchmarks (#6623) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…nchmarks (#6624) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
… explain targets (#6619)
…tory with 15 updates (#6613) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…/front-end/ofe with 45 updates (#6607) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…fe (#6608) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
cluster-toolkit-release-robo
Bot
added
do not merge
do-not-merge
|
/gemini review |
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 is an automated tracking PR designed to facilitate continuous code review by aggregating changes from 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 several new community examples and modules, including a private generative AI gateway on GKE, GKE inference benchmarks for DiffusionGemma, browser-based remote desktops for Slurm clusters, and an IAM module for Gemini prediction access. It also optimizes Slurm controller performance for large nodeset configurations, improves GKE CPU limit calculations, and adds automatic image pull secret discovery. Feedback on the changes highlights a critical bug in IsValueSet regarding boolean/numeric evaluation, a MySQL syntax error in the Slurm event table modification, a regex validation issue in the prediction IAM module, a least-privilege violation in GCS bucket permissions, and opportunities to simplify blueprint dependencies using use blocks.
| return isExpr || len(IsProductOfModuleUse(val)) > 0 | ||
| } | ||
| val, _ = val.Unmark() | ||
| switch val.Type() { |
There was a problem hiding this comment.
In IsValueSet, boolean and numeric values are incorrectly evaluated. Currently, cty.False returns false (meaning it is treated as unset), and 0 or negative numbers also return false. However, false and 0 are valid explicitly set values (e.g., enable_public_ips: false or threads_per_core: 0). Any known, non-null boolean or numeric value should be considered set.
case cty.Number, cty.Bool:
return true| "select data_type from information_schema.columns where table_schema='slurm_acct_db'" | ||
| f" and table_name='{table}' and column_name='cluster_nodes'" | ||
| ) | ||
| alter = f"alter table `{table}` modify `cluster_nodes` mediumtext not null default ''" |
There was a problem hiding this comment.
In MySQL, BLOB and TEXT (including MEDIUMTEXT) columns cannot have a default value in many versions (e.g., MySQL 5.7 and earlier, or MySQL 8.0 without expression defaults). Specifying default '' will raise a BLOB/TEXT column 'cluster_nodes' can't have a default value error. Since Slurm's original schema defines cluster_nodes without a default value, you should remove default '' to ensure compatibility across all MySQL and MariaDB versions.
| alter = f"alter table `{table}` modify `cluster_nodes` mediumtext not null default ''" | |
| alter = f"alter table `{table}` modify `cluster_nodes` mediumtext not null" |
| - id: cluster | ||
| source: modules/scheduler/gke-cluster | ||
| use: [network] | ||
| settings: | ||
| version_prefix: $(vars.gke_version_prefix) | ||
| release_channel: REGULAR | ||
| service_account_email: $(node_sa.service_account_email) |
There was a problem hiding this comment.
According to the Repository Style Guide (Blueprint Authoring), the use block is preferred for module dependencies within blueprints. Explicit variable linking (e.g., service_account_email = $(node_sa.service_account_email)) should only be used when necessary to resolve ambiguity. Since node_sa outputs service_account_email and gke-cluster accepts it as an input, you can add node_sa to the use list and omit the explicit setting.
- id: cluster
source: modules/scheduler/gke-cluster
use: [network, node_sa]
settings:
version_prefix: $(vars.gke_version_prefix)
release_channel: REGULARReferences
- Ensure the
useblock is preferred for module dependencies within blueprints. Explicit variable linking (e.g.,setting = $(module.output)) should only be used when necessary to resolve ambiguity. (link) - In the Cluster Toolkit framework, leverage automatic variable wiring by matching output names from a dependency module (defined in 'use') with top-level variable names in the consuming module to avoid redundant manual wiring.
| - id: proxy_wi | ||
| source: modules/project/workload_identity_binding | ||
| settings: | ||
| service_account_email: $(proxy_sa.service_account_email) | ||
| namespace: $(vars.namespace) | ||
| k8s_service_account_name: agentgateway-proxy |
There was a problem hiding this comment.
According to the Repository Style Guide (Blueprint Authoring), the use block is preferred for module dependencies within blueprints. Explicit variable linking (e.g., service_account_email = $(proxy_sa.service_account_email)) should only be used when necessary to resolve ambiguity. Since proxy_sa outputs service_account_email and workload_identity_binding accepts it as an input, you can add proxy_sa to the use list and omit the explicit setting.
- id: proxy_wi
source: modules/project/workload_identity_binding
use: [proxy_sa]
settings:
namespace: $(vars.namespace)
k8s_service_account_name: agentgateway-proxyReferences
- Ensure the
useblock is preferred for module dependencies within blueprints. Explicit variable linking (e.g.,setting = $(module.output)) should only be used when necessary to resolve ambiguity. (link) - In the Cluster Toolkit framework, leverage automatic variable wiring by matching output names from a dependency module (defined in 'use') with top-level variable names in the consuming module to avoid redundant manual wiring.
| - id: vertex_permissions | ||
| source: community/modules/project/aiplatform-predict-iam | ||
| settings: | ||
| service_account_email: $(proxy_sa.service_account_email) |
There was a problem hiding this comment.
According to the Repository Style Guide (Blueprint Authoring), the use block is preferred for module dependencies within blueprints. Explicit variable linking (e.g., service_account_email = $(proxy_sa.service_account_email)) should only be used when necessary to resolve ambiguity. Since proxy_sa outputs service_account_email and aiplatform-predict-iam accepts it as an input, you can add proxy_sa to the use list and omit the explicit setting and settings block entirely.
- id: vertex_permissions
source: community/modules/project/aiplatform-predict-iam
use: [proxy_sa]References
- Ensure the
useblock is preferred for module dependencies within blueprints. Explicit variable linking (e.g.,setting = $(module.output)) should only be used when necessary to resolve ambiguity. (link) - In the Cluster Toolkit framework, leverage automatic variable wiring by matching output names from a dependency module (defined in 'use') with top-level variable names in the consuming module to avoid redundant manual wiring.
| variable "deployment_name" { | ||
| validation { | ||
| condition = can(regex("^[a-zA-Z0-9_.-]{1,56}$", var.deployment_name)) | ||
| error_message = "deployment_name must contain 1–56 letters, digits, underscores, periods or hyphens; the generated role ID adds _predict and replaces hyphens with underscores." | ||
| } |
There was a problem hiding this comment.
The validation regex ^[a-zA-Z0-9_.-]{1,56}$ allows deployment_name to start with a hyphen - or period .. However, the generated role ID replaces hyphens with underscores, which would result in a role ID starting with _ or .. GCP custom role IDs must start with an alphanumeric character. Tighten the regex to ensure the first character is alphanumeric.
validation {
condition = can(regex("^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,55}$", var.deployment_name))
error_message = "deployment_name must start with a letter or digit and contain 1–56 letters, digits, underscores, periods or hyphens; the generated role ID adds _predict and replaces hyphens with underscores."
}
| - monitoring.viewer | ||
| - stackdriver.resourceMetadata.writer | ||
| - storage.objectAdmin | ||
| - storage.bucketViewer |
There was a problem hiding this comment.
According to the general rules, you should use storage.objectUser (Storage Object User) instead of storage.objectAdmin to grant read/write permissions on Cloud Storage buckets in order to follow the principle of least privilege.
- storage.objectUserReferences
- Use the predefined role
roles/storage.objectUser(Storage Object User) instead ofroles/storage.objectAdminto grant read/write permissions on Cloud Storage buckets in order to follow the principle of least privilege.
There was a problem hiding this comment.
Code Review
This pull request introduces several new features and modules, including the agentgateway-gke blueprint for deploying a private generative AI gateway on GKE, a catalog of GKE inference benchmarks (starting with g4-diffusiongemma-26b-a4b), and remote desktop examples using novnc-runtime and novnc-desktop modules. It also adds the aiplatform-predict-iam module, optimizes Slurm controller performance for large clusters, implements ai_assisted telemetry tracking, and upgrades various Go and Python dependencies. The review feedback correctly identifies a validation regex bug in the aiplatform-predict-iam module that incorrectly allows periods in custom role IDs, and a potential bash syntax error in the batch-submit.sh.tftpl template when gcloud is missing. Both comments provide actionable code suggestions to resolve these issues.
| condition = can(regex("^[a-zA-Z0-9_.-]{1,56}$", var.deployment_name)) | ||
| error_message = "deployment_name must contain 1–56 letters, digits, underscores, periods or hyphens; the generated role ID adds _predict and replaces hyphens with underscores." |
There was a problem hiding this comment.
The validation regex for deployment_name allows periods (.), but GCP custom role IDs do not support periods (only alphanumeric characters and underscores are allowed, and hyphens are replaced with underscores in main.tf). If a user provides a deployment_name with a period, the custom role creation will fail at runtime. We should restrict the allowed characters to alphanumeric, underscores, and hyphens.
condition = can(regex("^[a-zA-Z0-9_-]{1,56}$", var.deployment_name))
error_message = "deployment_name must contain 1–56 letters, digits, underscores, or hyphens; the generated role ID adds _predict and replaces hyphens with underscores."
References
- When defining resource identifiers in Terraform (such as custom IAM role IDs), use variables known at plan time (e.g., deployment name or name prefix) rather than computed attributes to ensure a clean plan.
| GCLOUD_MAJOR_VERSION=$(gcloud --version | head -n 1 | awk '{print $NF}' | cut -f1 --delimiter=.) | ||
| if [ $((GCLOUD_MAJOR_VERSION >= 461)) ]; then | ||
| gcloud batch jobs submit ${submit_job_id} --project=${project} --location=${location} --config=${config} | ||
| if (( GCLOUD_MAJOR_VERSION >= 461 )); then |
There was a problem hiding this comment.
If gcloud is not installed or fails to run, GCLOUD_MAJOR_VERSION will be empty, which causes a bash syntax error in the arithmetic evaluation (( GCLOUD_MAJOR_VERSION >= 461 )). We should redirect stderr to /dev/null and default GCLOUD_MAJOR_VERSION to 0 if it is empty to prevent the script from crashing with a syntax error.
| GCLOUD_MAJOR_VERSION=$(gcloud --version | head -n 1 | awk '{print $NF}' | cut -f1 --delimiter=.) | |
| if [ $((GCLOUD_MAJOR_VERSION >= 461)) ]; then | |
| gcloud batch jobs submit ${submit_job_id} --project=${project} --location=${location} --config=${config} | |
| if (( GCLOUD_MAJOR_VERSION >= 461 )); then | |
| GCLOUD_MAJOR_VERSION=$(gcloud --version 2>/dev/null | head -n 1 | awk '{print $NF}' | cut -f1 --delimiter=.) | |
| GCLOUD_MAJOR_VERSION=$${GCLOUD_MAJOR_VERSION:-0} | |
| if (( GCLOUD_MAJOR_VERSION >= 461 )); then |
…ommunity, and resolve v2 doc audit gaps (#6639)
…#6315) Add a pre-flight check (wait_for_controller_nfs) on client nodes so they wait for the controller's NFS shares to be actively exported before mounting: - Verify Exports Are Ready: Checks that port 2049 is open and confirms via showmount -e (when port 111 is reachable) that the required shares or their parent directories (for subdirectory mounts) are exported. - Avoid DNS Boot Delays: Identifies controller mounts on client nodes using fast string checks (controller_mount_server_ip, control_host, control_addr) and a single non-retrying lookup, avoiding the 8.5-minute util.host_lookup() retry delay on external NFS servers. - Safe Timeout Fallback: Scales the wait timeout with controller_startup_scripts_timeout (minimum 600s) and uses jittered polling. If the wait times out, logs a warning and falls back to the standard mount_fstab() retry loop so slow controller startups never fail client setup. - Unit Tests: Added unit tests in test_setup_network_storage.py covering export checks, subdirectory mounts, port 111 fallback, DNS fast-paths, and timeout fallback.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces several major features, including a private generative AI gateway on GKE (agentgateway-gke), GKE LLM inference benchmarks starting with DiffusionGemma 26B-A4B, and browser-based remote desktop modules for Slurm. It also optimizes the Slurm controller for large-scale nodesets, improves config fetching performance, and enhances GKE job submissions with ML Diagnostics sidecar injection and Shared Pathways Service (SPS) workload execution. Valuable feedback from the review highlights a missing shutil import in setup_network_storage.py, a potential index out of bounds error in the remote desktop outputs when instance_count is zero, and a regex validation gap for GCP role IDs. Additionally, suggestions were made to use idiomatic Go flag checks in submit.go and to correct a numeric sign check in expand.go to support negative values.
| from typing import Iterable, List, Optional, Set, Union | ||
|
|
||
| import os | ||
| import posixpath | ||
| import random | ||
| import socket | ||
| import sys | ||
| import stat | ||
| import time |
There was a problem hiding this comment.
The shutil module is used in _check_nfs_exports_showmount (via shutil.which) but is not imported in this file, which will cause a NameError at runtime. Please add import shutil to the imports.
| from typing import Iterable, List, Optional, Set, Union | |
| import os | |
| import posixpath | |
| import random | |
| import socket | |
| import sys | |
| import stat | |
| import time | |
| from typing import Iterable, List, Optional, Set, Union | |
| import os | |
| import posixpath | |
| import random | |
| import shutil | |
| import socket | |
| import sys | |
| import stat | |
| import time |
| } | ||
| if pathways.RunSPSWorkload != "" && cmd != nil { | ||
| for _, name := range unsupportedRunSPSWorkloadFlags { | ||
| if f := cmd.Flags().Lookup(name); f != nil && f.Changed { |
There was a problem hiding this comment.
According to the general rules, you should use pflag.FlagSet.Changed(name) directly to check if a flag was explicitly set by the user, rather than calling Lookup(name).Changed. This is cleaner and more idiomatic.
| if f := cmd.Flags().Lookup(name); f != nil && f.Changed { | |
| if cmd.Flags().Changed(name) { |
References
- Use pflag.FlagSet.Changed(name) to check if a flag was explicitly set by the user when using the github.com/spf13/pflag package in Go, as it is a valid method and does not require calling Lookup(name).Changed.
|
|
||
| variable "deployment_name" { | ||
| validation { | ||
| condition = can(regex("^[a-zA-Z0-9_.-]{1,56}$", var.deployment_name)) |
There was a problem hiding this comment.
The current regex allows var.deployment_name to start with non-alphanumeric characters (like _ or .). Since the generated role_id is derived from var.deployment_name, this can result in a custom role ID starting with a non-alphanumeric character, which is invalid in GCP and will cause a deployment failure. Update the regex to ensure it starts with an alphanumeric character.
condition = can(regex("^[a-zA-Z0-9][a-zA-Z0-9_.-]{0,55}$", var.deployment_name))
| output "iap_tunnel_command" { | ||
| description = "SSH command that forwards localhost:6080 to the desktop broker through IAP. Run it, then browse to http://localhost:6080/vnc.html." | ||
| value = var.instance_count > 0 ? join(" ", [ | ||
| "gcloud", "compute", "ssh", |
There was a problem hiding this comment.
If var.instance_count is set to 0, module.instances.name will be an empty list, and accessing module.instances.name[0] will cause a plan-time index out of bounds error in Terraform. Use try to safely handle the empty list case.
try(module.instances.name[0], ""),
| case cty.String: | ||
| return val.AsString() != "" | ||
| case cty.Number: | ||
| return val.AsBigFloat().Sign() > 0 |
There was a problem hiding this comment.
The current check Sign() > 0 only returns true for positive numbers, meaning negative numbers (which are valid non-zero values) will be incorrectly treated as unset. Use Sign() != 0 to correctly identify all non-zero numbers as set.
| return val.AsBigFloat().Sign() > 0 | |
| return val.AsBigFloat().Sign() != 0 |
References
- Follow standard Go idioms and best practices. (link)
🤖 Continuous Review Tracking PR
This is an automated dummy PR from
developintomaincreated by ReleaseRobo.It collects ongoing commits to
developso that daily/gemini reviewruns can surface code review issues ahead of release cut.