GCSFuse Profiles: Move Profiles to stable#1212
GCSFuse Profiles: Move Profiles to stable#1212chrisThePattyEater merged 1 commit intoGoogleCloudPlatform:mainfrom
Conversation
|
@chrisThePattyEater: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Summary of ChangesHello @chrisThePattyEater, 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 significantly enhances the GCSFuse CSI driver by introducing and stabilizing the GCSFuse profiles feature. It provides specialized configurations for various AI/ML workloads through new StorageClasses, enabling optimized data access patterns. The changes involve updating deployment configurations, adding new command-line arguments to driver components, and expanding role-based access control to support the new profile-driven functionalities and resource interactions. Highlights
Changelog
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 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 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
|
![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 enables the GCSFuse Profiles feature and marks it as stable. The changes include enabling feature flags across the controller, node, and webhook components, updating RBAC roles with necessary permissions, and adding predefined StorageClasses for common workloads. A notable improvement is the refactoring of the controller deployment, which consolidates sidecar containers into the main driver, simplifying the architecture. However, I have a critical concern: the controller deployment now depends on a ConfigMap named gcsfusecsi-profiles-config, which is not defined within this pull request. This omission will cause the controller Pod to fail on startup, breaking the deployment.
chrisThePattyEater
force-pushed
the
stableProfiles
branch
from
4af5f6c to
c004c19
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request successfully moves the GCSFuse Profiles feature into the stable configuration, enabling it by default. The changes across the various Kubernetes manifests for the controller, node, and webhook components are consistent and correctly apply the necessary feature flags and RBAC permissions. The addition of predefined StorageClass definitions for different profiles is also a key part of this feature enablement. I've identified one critical issue in the Makefile that will cause the generate-spec-yaml target to fail. Please see the detailed comment for the fix.
chrisThePattyEater
force-pushed
the
stableProfiles
branch
from
c004c19 to
9b60a5f
Compare
|
The release note says "Releases the GCSFuse Profiles Feature", but that's not what this PR does. Please change to |
|
Please also delete the profiles overlay, now that this is merged to stable, the overlay is redundant and serves no purpose anymore. |
chrisThePattyEater
force-pushed
the
stableProfiles
branch
2 times, most recently
from
b987cfd to
4c497c8
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request moves the GCSFuse Profiles feature to the stable overlay, making it generally available. The changes involve updating the Makefile, Kubernetes manifests for the controller, node, and webhook components to enable the feature by default, and adding new RBAC rules and StorageClasses required for the profiles functionality. My review includes suggestions to improve the Makefile's conciseness, align the node DaemonSet's imagePullPolicy with best practices for stable components, and fix formatting issues in YAML files by adding missing newlines.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request moves the GCSFuse Profiles feature to the stable configuration. The changes look good overall, modifying Kubernetes manifests for the controller, node, and webhook to enable the feature, and adding the necessary RBAC rules and StorageClass definitions to the base. My review focuses on resource management, production readiness of configurations, and file formatting. I have a few suggestions to improve the code quality.
chrisThePattyEater
force-pushed
the
stableProfiles
branch
3 times, most recently
from
ac2ca88 to
f8d07ed
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request graduates the GCSFuse Profiles feature to stable by moving its configuration from the profiles overlay into the base manifests. The changes include removing the profiles overlay, updating the Makefile to always apply profile-related configurations, enabling the feature in the controller, node, and webhook components via command-line flags, and adjusting RBAC permissions accordingly. The overall changes are well-structured and align with the goal of making the feature generally available. I have one minor suggestion to improve the Makefile's conciseness.
chrisThePattyEater
force-pushed
the
stableProfiles
branch
3 times, most recently
from
c78a0b7 to
7944619
Compare
chrisThePattyEater
force-pushed
the
stableProfiles
branch
2 times, most recently
from
3bf1da5 to
5fe5cbd
Compare
chrisThePattyEater
force-pushed
the
stableProfiles
branch
5 times, most recently
from
efefa42 to
38f1db3
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces and enables the "gcsfuse profiles" feature across the GCS Fuse CSI driver's controller, node, and webhook components. Key changes include refactoring the controller deployment to consolidate functionality and enable leader election and profile management, updating Kubernetes RBAC roles and bindings to grant necessary permissions for profile-related operations, and adding a new ConfigMap for profiles. The E2E tests were also updated to verify this new functionality. A review comment noted a potential issue with the removal of the CPU limit for the gcs-fuse-csi-driver container in the controller's base deployment, which could lead to uncontrolled resource consumption and impact cluster stability.
chrisThePattyEater
force-pushed
the
stableProfiles
branch
from
38f1db3 to
3ff62c9
Compare
chrisThePattyEater
force-pushed
the
stableProfiles
branch
from
3ff62c9 to
660f2f8
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: chrisThePattyEater, uriel-guzman The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
chrisThePattyEater
merged commit c78267c
into
GoogleCloudPlatform:main
2 participants
What type of PR is this?
/kind feature
What this PR does / why we need it:
This pr moves the profiles feature that has been in development on the
profileoverlay tostable.Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
These changes were tested by running e2e tests on the
gcsfuseIntegrationtest suite. The cluster is a 3 node standard gke cluster using n2-standard-8. Command to run the tests:Does this PR introduce a user-facing change?: