feat(job): new flag to pass additional mount options for gcsfuse mounts#6005
feat(job): new flag to pass additional mount options for gcsfuse mounts#6005Neelabh94 wants to merge 5 commits intoGoogleCloudPlatform:developGoogleCloudPlatform/cluster-toolkit:developfrom Neelabh94:gcs-mount-optionsNeelabh94/cluster-toolkit:gcs-mount-optionsCopy head branch name to clipboard
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new --mount-options flag to the gcluster job submit command, enabling custom mount options for GCS fuse volumes, and updates the orchestrator, GKE storage manager, and documentation accordingly. The review feedback suggests enhancing validation to fail-fast if --mount-options is specified without any mounts, and formatting struct fields in pkg/orchestrator/orchestrator.go to comply with standard Go alignment.
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 enhances the Highlights
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
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new --mount-options flag to the gcluster job submit command, enabling custom mount options for GCS fuse volumes, and updates the relevant documentation. Feedback suggests adding a validation check to ensure --mount-options is not silently ignored when no mounts are specified, and formatting the JobDefinition struct to align the new field according to standard Go guidelines.
… options for GCS fuse volumes
…idation and update docs
cf0212d to
9decd22
Compare
| return fmt.Errorf("--mount-options requires at least one GCS mount") | ||
| } | ||
| for _, m := range volumeStr { | ||
| if !strings.HasPrefix(m, "gs://") { |
There was a problem hiding this comment.
If a user runs a job that mounts a GCS bucket alongside a local disk or NFS drive, the command will be completely rejected. In storage.go, the backend handles mixed workloads perfectly by explicitly checking if volType == "gcsfuse". Should we update the validation to just confirm that at least one GCS bucket is mounted when --mount-options is used, rather than an aggressive front-end validation?
example snippet:
func validateMountOptions() error {
if mountOptions != "" {
hasGCSMount := false
for _, m := range volumeStr {
if strings.HasPrefix(m, "gs://") {
hasGCSMount = true
break
}
}
if !hasGCSMount {
return fmt.Errorf("--mount-options was provided, but no GCS fuse volumes (gs://...) were mounted")
}
}
return nil
}
Description
This PR adds support for passing custom mount options when submitting a job, bringing
gclusterinto closer parity withxpkbehavior.Key Changes:
--mount-optionsflag: Users can now pass custom storage options (e.g.,logging:severity:info,enable-atomic-rename-object:true) togcluster job submit.validateMountOptions) that ensures the--mount-optionsflag is currently only utilized for GCS fuse volumes (i.e. it enforces that all supplied--mountvalues start withgs://when mount options are present).docs/gcluster_job_guide.md.