Improve partial profile cache mount option override handling

[uriel-guzman]

What type of PR is this?
/kind feature

What this PR does / why we need it:

Before: If the customer specifies any of the following mountOptions:

metadata-cache:type-cache-max-size-mb
metadata-cache:stat-cache-max-size-mb
file-cache:max-size-mb

or configures a custom file cache medium, all smart recommendation are disabled. The rationale was that it is complex to integrate the customer's cache preferences into the recommendation algortihm.

After: If the customer specifies any of the mountOptions above, they will be incorporated into the recommendation algorithm in the following manner:

Metadata caches
The same rationale applies for metadata-cache:stat-max-size-mb.

1. metadata-cache:type-cache-max-size-mb:0: Disabling a single cache size doesn't affect other recommendations, so this should work flawlessly.
2. metadata-cache:type-cache-max-size-mb:-1: Since the desired cache size is infinite, we can assume it can potentially grow up to the estimated cache size. Therefore, we subtract the estimated cache size from the memory budget for the remaining recommendations, and log a warning if the estimated cache size exceeds the RAM available, while still allowing the customer's override, instead of capping it.
3. metadata-cache:type-cache-max-size-mb:<some finite value>: Since the desired cache size is finite, we can use it as input for the algorithm. Since we need to respect the override, a warning is logged if the override exceeds the RAM available, while still allowing the customer's override, instead of capping it.

File cache
File cache max size will be overriden and smart file cache size calculation + medium evalutation will be skipped because we can't provide a medium if the cache size exceeds all available mediums, and we shouldn't disable file caching because the customer explicitly requested their file cache to be of this size.

Custom file cache medium
If the customer set a custom file cache medium, we can't know what the size of that medium is, since it could literally be any type of volume. Therefore, we disable smart file cache size calculation, and allow the default size to take place, which is unlimited when this feature is used: https://docs.cloud.google.com/storage/docs/cloud-storage-fuse/cli-options#file-cache-max-size-mb.

This PR essentially allows partial overrides, i.e. overriding file cache should still allow metadata stat cache to be automatically calculated, since it should be independent.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[google-oss-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[gemini-code-assist]

Summary of Changes

Hello @uriel-guzman, I'm Gemini Code Assist¹! 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 cache recommendation system by providing more granular control and better handling of user-defined cache settings. It ensures that explicit user overrides for metadata and file cache sizes are respected, while still attempting to find optimal storage mediums. The changes prevent unintended overrides of default profile values and improve the robustness of cache allocation in various scenarios.

Highlights

• Improved User Override Handling: Introduced a new cacheOverrides struct and logic to explicitly parse and respect user-provided cache size settings (metadata stat, metadata type, and file cache) from mount options, allowing users to override automatic recommendations.
• Refined File Cache Recommendation Logic: Modified the file cache recommendation process to be more robust when user overrides are present. If a user specifies a file cache size, the system will attempt to find the best available medium for that size, even if it exceeds calculated requirements, with appropriate warnings. If no override is given and no suitable medium is found, the file cache is disabled to prevent resource exhaustion.
• Conditional File Cache Application: Added a fileCacheRecommendationSkipped flag to prevent the system from applying a 0 byte file cache mount option when the recommendation was intentionally skipped due to user-defined custom cache mediums, ensuring the default profile values are honored instead.
• Enhanced Mount Option Parsing: Updated the getMountOptionKey function to return both the key and value of a mount option, and introduced a parseCacheOverrides method to systematically extract cache-related overrides from the provided mount options.

Changelog

• pkg/profiles/recommender.go
  • Added cacheOverrides struct to model user-defined cache size overrides.
  • Introduced fileCacheRecommendationSkipped field to the recommendation struct.
  • Renamed shouldSkipCacheRecommendations to shouldSkipFileCacheRecommendation and updated its logic to only check for custom cache medium labels.
  • Updated comments for MergeRecommendedMountOptionsOnMissingKeys to reflect best-effort cache recommendation and file cache skipping.
  • Removed the shouldSkipCacheRecommendations call from MergeRecommendedMountOptionsOnMissingKeys.
  • Modified cache option appending logic to conditionally skip file cache size and medium if fileCacheRecommendationSkipped is true.
  • Refactored buildCacheRequirements for clearer initialization.
  • Updated recommendCacheConfigs function signature to accept mountOptions and incorporated cacheOverrides parsing.
  • Modified recommendFileCacheSizeAndMedium to accept override values and implement override handling, including warnings for potential OOM.
  • Modified recommendMetadataCacheSize to accept override values and implement override handling for metadata caches.
  • Added mibToBytes function and updated bytesToMiB to handle -1 for infinite values.
  • Updated getMountOptionKey to return both key and value, and added parseCacheOverrides method to ProfileConfig.
  • Adjusted mergeMountOptionsOnMissingKeys to correctly use the updated getMountOptionKey signature.

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. ↩

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: uriel-guzman

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[gemini-code-assist]

Code Review

This pull request refactors the cache recommendation logic to better handle user-provided overrides from mount options. The changes introduce a cacheOverrides struct and update the recommendation functions to respect these overrides. The logic for skipping recommendations is also refined to only skip file cache recommendations when a custom medium is used.

I've identified a critical bug in the parseCacheOverrides function that prevents user overrides from being applied correctly, as detailed in the specific comment. Additionally, the tests for recommender.go appear to be outdated and do not cover the new functionality, which is a significant gap. Please ensure that the tests are updated to reflect these changes and provide adequate coverage.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the cache recommendation logic to better handle user-provided overrides for cache sizes. The changes introduce a more granular approach, allowing the recommender to respect user-defined values on a per-cache basis rather than skipping all recommendations. However, a significant bug was identified in the handling of infinite file cache overrides where the system does not correctly respect the user's setting. A detailed comment with a suggested fix is provided to address this high-severity issue.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves how user-provided cache configuration overrides are handled. Instead of skipping all smart cache recommendations when an override is detected, the new logic intelligently incorporates user-defined values into the recommendation process. It makes a best-effort attempt to respect user overrides while still considering available resource budgets, providing warnings if the overrides might lead to issues like OOM. The changes are well-structured, and the addition of comprehensive test cases for various override scenarios is excellent. I've found a couple of minor typos in comments and error messages.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request refactors the cache recommendation logic to better handle user-provided overrides for cache sizes. The changes introduce a cacheOverrides struct to represent user settings, and the recommendation functions are updated to respect these overrides while still providing warnings about potential resource exhaustion. The logic for skipping file cache recommendations is also refined. The accompanying test changes are thorough and cover various override scenarios.

My feedback focuses on improving the clarity of warning messages by logging sizes in MiB instead of bytes, and on adhering to standard Go formatting (gofmt) for case statements. These changes will enhance user experience during debugging and improve code readability.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves the cache recommendation logic by incorporating user-provided overrides instead of disabling smart recommendations. The changes are well-structured, introducing a cacheOverrides struct and a parsing mechanism to handle user inputs gracefully. The recommendation functions for both metadata and file caches have been updated to respect these overrides, with clear logic for handling finite, infinite, and disabled cache sizes. The test suite has been expanded with comprehensive cases covering the new override scenarios.

I've found a couple of minor issues in warning log messages where format specifiers and units could be improved for clarity. I've left specific comments with suggestions for these. Overall, this is a solid improvement to the feature.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves how user-provided cache configuration overrides are handled. Instead of disabling all smart recommendations when an override is present, the new logic incorporates user preferences into the recommendation algorithm. This allows for partial overrides, providing more flexibility. The changes are extensive, touching the core recommendation logic, logging, and mount option merging. The test suite has also been expanded to cover these new override scenarios. The changes are well-structured and improve the feature's usability. I've found a few minor issues related to logging and comments.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves the handling of user-provided cache configuration overrides. Instead of disabling all smart recommendations, the new logic intelligently incorporates user overrides for metadata and file caches into the recommendation algorithm. The changes are well-structured, with clear separation of concerns for parsing overrides and applying them. The introduction of new constants for CLI mount option keys and a normalizeOpt function enhances robustness and deduplication. The test suite has been commendably expanded to cover a wide range of override scenarios, ensuring the new logic is well-vetted. Overall, this is a solid enhancement. I have one minor suggestion to improve a log message for clarity and consistency.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces a significant improvement by handling user-provided cache configuration overrides more gracefully, rather than completely disabling smart recommendations. The changes are well-structured, with clear separation of concerns for parsing overrides and applying them to the recommendation logic. The introduction of the cacheOverrides struct and the refactoring of mount option key handling are excellent. I have one suggestion to simplify the logic for handling LSSD on TPU nodes, which should make the code cleaner and more robust, aligning with principles of maintainability and redundancy reduction. Overall, this is a great enhancement.

[uriel-guzman]

/gemini review

[gemini-code-assist]

Code Review

This pull request significantly improves the handling of partial profile cache mount option overrides in the GCS FUSE CSI driver. It allows users to override specific cache settings (metadata stat, metadata type, or file cache) while still benefiting from smart recommendations for the remaining parameters. The changes include robust parsing of mount options in both CLI and config-file formats, improved resource budget calculations that respect overrides, and enhanced logging for better transparency. Additionally, several bug fixes in E2E tests and log formatting were addressed.