Recommend metadata cache type and stat max sizes. #1020
Conversation
uriel-guzman
force-pushed
the
recommend-metadata-cache
branch
2 times, most recently
from
58f2745 to
2e9ad3a
Compare
|
/gemini review |
| }{ | ||
| { | ||
| name: "Bytes greater than 0 - Should add formatted option", | ||
| initialOptions: []string{"opt1"}, |
There was a problem hiding this comment.
Do we need a test case for what happens when mountOptionKey is already set to different value in initialOptions? Your current implementation appends, so we would have duplicate options in wantOptions.
There was a problem hiding this comment.
Sure, I can make the addRecommendationToMountOptions only append if the key doesn't exist. I was planning on doing this until we get to NodePublishVolume (at that point, the customer's PV / ephemeral storage mount options are merged with the output from RecommendMountOptions and deduplication occurs, making the customer's PV / ephemeral storage options take precedence over the recommendations), but it wouldn't hurt to also add the deduplication here too. In fact, we can re-use this function later so it would be a good idea.
There was a problem hiding this comment.
Fixed. Also added test case for this.
| } | ||
| } | ||
|
|
||
| func TestAddRecommendationToMountOptions(t *testing.T) { |
There was a problem hiding this comment.
I see all test cases use config file format for existing mount options. Should we also test cmd line flag mount option? https://cloud.google.com/storage/docs/cloud-storage-fuse/config-file#format-and-fields https://cloud.google.com/storage/docs/cloud-storage-fuse/cli-options#options
There was a problem hiding this comment.
Just as a note: This formatting (key=val) is already validated in https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/blob/main/pkg/profiles/recommender_test.go#L61, but I can add another test case to this test group to include a key=val format mount option (cmd line).
There was a problem hiding this comment.
Fixed, added test case for this.
|
|
||
| // Calculate the recommended metadata cache sizes. The memoryBudget gets decreased after each recommendation. | ||
| recommendation.metadataStatCacheBytes, memoryBudget = recommendMetadataCacheSize(config, cacheRequirements.metadataStatCacheBytes, memoryBudget, "stat") | ||
| recommendation.metadataTypeCacheBytes, _ = recommendMetadataCacheSize(config, cacheRequirements.metadataTypeCacheBytes, memoryBudget, "type") |
There was a problem hiding this comment.
Why is memoryBudget ignored here
There was a problem hiding this comment.
It's not used yet. It will be used in the next PR, where the memoryBudget is used to calculate if the file cache will fit or not in the remaining budget.
uriel-guzman
force-pushed
the
recommend-metadata-cache
branch
from
2e9ad3a to
3e15c0e
Compare
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This PR implements an initial version of an intelligent recommender in
recommender.goto automatically generate optimal GCS FUSE mount options. The primary goal is to enhance performance by dynamically configuring GCS FUSE cache settings based on available cluster resources and PersistentVolume characteristics.The key components introduced are:
cacheRequirements: Defines ideal sizes for different cache types (metadata stat, metadata type, file cache) derived from PV details like object count and total size.calculateFuseResourceBudgetandcalculateResourceBudgetsdetermine the memory and ephemeral storage capacity available for GCS FUSE, considering node allocatables, StorageClass factors (fuseMemoryAllocatableFactor,fuseEphemeralStorageAllocatableFactor), and sidecar container limits.recommendMetadataCacheSizecalculates recommended sizes for the metadata stat and type caches, ensuring they do not exceed the computed memory budget.RecommendMountOptionsorchestrates the process, callingrecommendCacheConfigsto get cache size recommendations and then usingaddRecommendationToMountOptionsto append the calculatedmetadata-cache:stat-cache-max-size-mbandmetadata-cache:type-cache-max-size-mboptions, returning a slice that expands upon the StorageClass's existing mount options.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?: