Skip to content

fix: Creating non dp grpc client when dp check fails - #4989

Open
vadlakondaswetha wants to merge 3 commits into
masterfrom
fixdpcheck
Open

fix: Creating non dp grpc client when dp check fails#4989
vadlakondaswetha wants to merge 3 commits into
masterfrom
fixdpcheck

Conversation

@vadlakondaswetha

@vadlakondaswetha vadlakondaswetha commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

For rapid, if direct path doesnt work and server rejects the non dp calls as part of dp verification check, then we are ignoring the error and continuing with the mount. The problem with this approach is that since we are passing withDPEnforced header for every call, any call made to GCS after mount will also be rejected by the server.

To avoid this, we should create a new gRPC client which doesnt pass the header if dp check fails.

Note: Today server rejecting non dp calls is behind whitelisting and not fully rolled out.

Description

Link to the issue in case of a bug fix.

Testing details

  1. Manual - Done
  2. Unit tests - Added
  3. Integration tests - NA

Any backward incompatible change? If so, please explain.

@vadlakondaswetha
vadlakondaswetha requested a review from a team as a code owner August 4, 2026 09:59
@github-actions github-actions Bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Aug 4, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

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 refactors the gRPC client creation and DirectPath verification logic, replacing the rapid bucket check with an explicit DirectPath enforcement flag and introducing a unified client creation helper. It also adds comprehensive unit tests to validate the fallback behavior under different strategies. The feedback suggests removing the explicit client close call during verification failure, as the garbage collector handles resource cleanup.

Comment thread internal/storage/storage_handle.go Outdated
@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 87.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.97%. Comparing base (5716f09) to head (f9ac36f).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
internal/storage/storage_handle.go 87.50% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4989      +/-   ##
==========================================
+ Coverage   83.85%   83.97%   +0.11%     
==========================================
  Files         173      173              
  Lines       21171    21204      +33     
==========================================
+ Hits        17752    17805      +53     
+ Misses       2750     2734      -16     
+ Partials      669      665       -4     
Flag Coverage Δ
unittests 83.97% <87.50%> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@vadlakondaswetha
vadlakondaswetha requested review from abhishek10004 and meet2mky and removed request for raj-prince August 4, 2026 10:49
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

Hi @abhishek10004, @meet2mky, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

2 similar comments
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Hi @abhishek10004, @meet2mky, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

@github-actions

github-actions Bot commented Aug 7, 2026

Copy link
Copy Markdown

Hi @abhishek10004, @meet2mky, your feedback is needed to move this pull request forward. This automated reminder was triggered because there has been no activity for over 24 hours. Please provide your input when you have a moment. Thank you!

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

Labels

remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant