Skip to content

Draft: Pass special gcsfuse app-name in mounts from CTK blueprints#5240

Draft
gargnitingoogle wants to merge 2 commits intoGoogleCloudPlatform:developfrom
gargnitingoogle:gargnitin/gcsfuse-ctk-useragent/v1
Draft

Draft: Pass special gcsfuse app-name in mounts from CTK blueprints#5240
gargnitingoogle wants to merge 2 commits intoGoogleCloudPlatform:developfrom
gargnitingoogle:gargnitin/gcsfuse-ctk-useragent/v1

Conversation

@gargnitingoogle
Copy link
Contributor

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 #

This ensures the substring 'ctk' in all
gcsfuse mounts created using cluster-toolkit.

GCSFuse mounts created this way have 'ctk' in the useragent
fields in all the GCS calls, which can be used to measure the
GCS traffic coming from gcsfuse mounts created using cluster-toolkit.

This will be used by GCSFuse team for their usage and impact study.
@github-actions github-actions bot added the external PR from external contributor label Feb 13, 2026
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @gargnitingoogle, 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 changes across several scripts to ensure that gcsfuse mounts initiated by Cluster Toolkit (CTK) blueprints are consistently tagged with a special app_name identifier. This enhancement aims to improve the observability and attribution of gcsfuse operations originating from CTK, allowing for easier identification and monitoring of these specific mounts.

Highlights

  • GCSFuse App Name Tagging in Python Script: The setup_network_storage.py script has been updated to automatically append a ctk-from-python tag to gcsfuse app_name mount options if no CTK tag is already present, ensuring better attribution for mounts initiated via this script.
  • GCSFuse App Name Tagging in Shell Scripts: The mount.sh scripts within the cloud-storage-bucket and pre-existing-network-storage modules were modified to append ctk-from-mount-sh or ctk-from-sh tags, respectively, to gcsfuse app_name mount options, standardizing the tagging across different mounting mechanisms.
Changelog
  • community/modules/scheduler/schedmd-slurm-gcp-v6-controller/modules/slurm_files/scripts/setup_network_storage.py
    • Added conditional logic to modify gcsfuse mount options.
    • Ensured app_name option includes a ctk-from-python tag if not already present.
  • modules/file-system/cloud-storage-bucket/scripts/mount.sh
    • Implemented logic to append ctk-from-mount-sh to gcsfuse app_name mount options.
  • modules/file-system/pre-existing-network-storage/scripts/mount.sh
    • Introduced logic to append ctk-from-sh to gcsfuse app_name mount options.
    • Added debug echo statements for tracing mount option modifications.
Activity
  • No human activity (comments, reviews, or explicit progress updates) has been recorded on this pull request yet. The PR description includes a standard submission checklist.
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. 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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

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 aims to add a special app-name to gcsfuse mount options for tracking purposes. The implementation is consistent across a Python script and two shell scripts. My review has identified a critical bug in the Python script where an f-string was missed, which would prevent the feature from working correctly. I've also pointed out several debug statements that should be removed before merging and suggested a minor improvement to a regular expression in the shell scripts for better readability. Overall, the changes are logical, and all identified issues should be addressed.

break

if not app_name_found:
mount_options.append("app_name={ctk_tag}")
Copy link
Contributor

Choose a reason for hiding this comment

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

critical

There appears to be a bug in this line. You are using a standard string, but it seems an f-string was intended to interpolate the ctk_tag variable. As written, it will append the literal text app_name={ctk_tag}.

Suggested change
mount_options.append("app_name={ctk_tag}")
mount_options.append(f"app_name={ctk_tag}")


if fs_type == "gcsfuse":
ctk_tag="ctk-from-python"
log.info(f"DEBUG [setup_network_storage.py]: fs_type={fs_type} mount_options={mount_options}")
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This PR adds several debug logging statements (here and on line 149). Please remember to remove them before this PR is merged.


if [ "${FS_TYPE}" = "gcsfuse" ]; then
ctk_tag="ctk-from-mount-sh"
if [[ ! "${MOUNT_OPTIONS}" =~ [c,C][t,T][k,K] ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For improved readability, the regex [c,C][t,T][k,K] can be simplified. The more standard way to write this for case-insensitivity of single characters is [cC][tT][kK].

Suggested change
if [[ ! "${MOUNT_OPTIONS}" =~ [c,C][t,T][k,K] ]]; then
if [[ ! "${MOUNT_OPTIONS}" =~ [cC][tT][kK] ]]; then


if [ "${FS_TYPE}" = "gcsfuse" ]; then
ctk_tag="ctk-from-sh"
echo "DEBUG [mount.sh]: Before logic fs_type=${FS_TYPE} mount_options=${MOUNT_OPTIONS}"
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This script includes debug echo statements (here and on line 39). Please remember to remove them before this PR is merged.

if [ "${FS_TYPE}" = "gcsfuse" ]; then
ctk_tag="ctk-from-sh"
echo "DEBUG [mount.sh]: Before logic fs_type=${FS_TYPE} mount_options=${MOUNT_OPTIONS}"
if [[ ! "${MOUNT_OPTIONS}" =~ [c,C][t,T][k,K] ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

For improved readability, the regex [c,C][t,T][k,K] can be simplified. The more standard way to write this for case-insensitivity of single characters is [cC][tT][kK].

Suggested change
if [[ ! "${MOUNT_OPTIONS}" =~ [c,C][t,T][k,K] ]]; then
if [[ ! "${MOUNT_OPTIONS}" =~ [cC][tT][kK] ]]; then

@aslam-quad
Copy link
Contributor

/gcbrun

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external PR from external contributor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants