test(gRPC): Add GCSFuse gRPC metrics tests#1240
Conversation
|
@alleaditya: 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, 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 new end-to-end tests to validate the emission and collection of gRPC metrics from GCSFuse within a GKE sidecar integration. The changes ensure that when GCSFuse operates with gRPC as its client protocol, the expected gRPC-specific metrics are correctly reported, which is vital for monitoring and understanding the performance of gRPC-enabled GCSFuse deployments. 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 adds end-to-end tests for GCSFuse gRPC metrics, which is a valuable addition. A security review confirmed that no vulnerabilities were identified, as changes are confined to the test suite and involve hardcoded constants or values generated within the test framework. However, there are a few areas for improvement to enhance code quality and maintainability. Specifically, I've identified some leftover conversational comments and an unnecessary configuration in testdriver.go. In metrics.go, the metric filtering logic is overly complex and could be simplified for better readability and robustness, also ensuring pod_uid is excluded from metric labels as per repository guidelines, and a debug statement should be removed.
|
Ran the below e2e test command on the branch: add_grpc_metrics_test make e2e-test E2E_TEST_USE_GKE_MANAGED_DRIVER=false Test results are as below: [ReportAfterSuite] PASSED [3327.121 seconds]
|
test/e2e/specs/testdriver.go
Outdated
| case EnableGrpcAndMetricsPrefix: | ||
| // Ensure gRPC is used for this test, as it's required for gRPC metrics. | ||
| if !strings.Contains(mountOptions, "client-protocol=grpc") { | ||
| mountOptions += ",client-protocol=grpc" | ||
| } | ||
| mountOptions += ",experimental-enable-grpc-metrics" | ||
| v.enableMetrics = true | ||
| v.fileCacheCapacity = "100Mi" |
There was a problem hiding this comment.
Please do not set this here:
// Ensure gRPC is used for this test, as it's required for gRPC metrics.
if !strings.Contains(mountOptions, "client-protocol=grpc") {
mountOptions += ",client-protocol=grpc"
}
mountOptions += ",experimental-enable-grpc-metrics"
Instead, check within the metrics test if client protocol is already set to grpc on the test driver and if GCSFuse version supports experimental-enable-grpc-metrics, then run the new grpc metrics. See here: https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/pull/1240/changes#r2927336840.
For v.fileCacheCapacity = "100Mi", please try to set that within the if checks described in https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/pull/1240/changes#r2927336840 in metrics.go itself. That way, we wouldn't need EnableGrpcAndMetricsPrefix at all. As mentioned in other comment, v.enableMetrics = true isn't needed since metrics is enabled by default on all GKE versions where we run the tests.
|
|
||
| //nolint:maintidx | ||
| func (t *gcsFuseCSIMetricsTestSuite) DefineTests(driver storageframework.TestDriver, pattern storageframework.TestPattern) { | ||
| gcsfuseDriver, ok := driver.(*specs.GCSFuseCSITestDriver) |
There was a problem hiding this comment.
Here is where you would check if gcsfuseDriver.ClientProtocol is grpc, then you need to append expectedGrpcMetricNames to expectedMetricNames for all of the metrics tests to use. This means when running the tests manually, you need to set GCSFUSE_CLIENT_PROTOCOL=grpc since we don't force grpc to be enabled now in mountOption. We rely on GCSFUSE_CLIENT_PROTOCOL
In follow up PR, when you address https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/pull/1240/changes#r2927119458 comment , you will change the if gcsfuseDriver.ClientProtocol == grpc check to also check that the current min GCSFuseVersion supports the experimental flag metric. If it does, you would add the experimental flag mountOption from https://github.com/GoogleCloudPlatform/gcs-fuse-csi-driver/pull/1240/changes#r2927289786 here.
For the v.fileCacheCapacity = "100Mi", please check on where you should set that. Try to set it in here if possible.
alleaditya
force-pushed
the
add_grpc_metrics_test
branch
from
6e9dff7 to
8ac3644
Compare
|
Can you please update the PR description with how you tested? We want to see that E2E_TEST_FOCUS=metrics.* tests are passing for make e2e-test with |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: alleaditya, amacaskill 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 |
Testing resultsResults indicate |
Testing resultsResults indicate |
4 participants
What type of PR is this?
/kind failing-test
What this PR does / why we need it:
The changes test for gRPC metrics from GCSFuse. The test changes are needed for gRPC metrics integration into GCSFuse GKE sidecar integration. More information go/gcsfuse-metrics-onboarding-on-gke
Which issue(s) this PR fixes:
Fixes #489422934
Special notes for your reviewer:
There is a special handling for
rls(Route Lookup Service) metrics since RLS metrics count cannot be verified without mocking the Backend connection.Testing directions for the PR
Run the e2e tests on the clusters with different
GCSFUSE_TAG(example v3.8.0 and v3.7.1) to check if the tests are version specific and gRPCWith the help of debug logs, I verified that the tests skipped gRPC metrics for v3.7.1 and validated gRPC metrics for v3.8.0.
Verified the required metrics were made available when gRPC was enabled (logs)
Does this PR introduce a user-facing change?: