linode-blockstorage-csi-driver icon indicating copy to clipboard operation
linode-blockstorage-csi-driver copied to clipboard

Simplify attach process

Open guilhem opened this issue 1 year ago • 1 comments

This pull request focuses on simplifying the volume attachment logic in the ControllerServer by removing redundant checks and related methods. The most important changes include the removal of the checkAttachmentCapacity function and its associated methods, as well as the related test cases.

Context

CSI caller (like Kubernetes) should already respect volume limitation from NodeGetInfo. We don't need another check and logic at attach.

Motivation

Reduce logic and complexity and rely more on Linode API calls. If there is an error, return it to the user for easier debug.

Risk

Making more failing attach requests and maybe hitting a rate limit. This should not be a problem, as CSI attacher have a backoff logic. Moreover, current limit logic is already doing api calls that should have hit API ratelimit.

Possible Mitigations

  • Implementing a rate limiting on api call.
  • As GCP, implementing a csiErrorBackoff mechanism

IMHO, both are not mandatory for the moment.

Simplification of volume attachment logic:

  • Removed the checkAttachmentCapacity function and its invocation from the ControllerPublishVolume method in internal/driver/controllerserver.go.
  • Deleted the canAttach function, which was used to determine if an additional volume could be attached to an instance, from internal/driver/controllerserver_helper.go.
  • Removed the checkAttachmentCapacity function, which checked if an instance could accommodate additional volume attachments, from internal/driver/controllerserver_helper.go.

Removal of related test cases:

  • Deleted the TestCheckAttachmentCapacity test function from internal/driver/controllerserver_helper_test.go, which tested the checkAttachmentCapacity logic.

General:

  • [ ] Have you removed all sensitive information, including but not limited to access keys and passwords?
  • [ ] Have you checked to ensure there aren't other open or closed Pull Requests for the same bug/feature/question?

Pull Request Guidelines:

  1. [ ] Does your submission pass tests?
  2. [ ] Have you added tests?
  3. [ ] Are you addressing a single feature in this PR?
  4. [ ] Are your commits atomic, addressing one change per commit?
  5. [ ] Are you following the conventions of the language?
  6. [ ] Have you saved your large formatting changes for a different PR, so we can focus on your work?
  7. [ ] Have you explained your rationale for why this feature is needed?
  8. [ ] Have you linked your PR to an open issue

guilhem avatar Nov 25 '24 23:11 guilhem

Codecov Report

Attention: Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 75.07%. Comparing base (17b7d1b) to head (efdefae).

Files with missing lines Patch % Lines
internal/driver/controllerserver_helper.go 75.00% 2 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #322      +/-   ##
==========================================
+ Coverage   74.79%   75.07%   +0.28%     
==========================================
  Files          22       22              
  Lines        2396     2359      -37     
==========================================
- Hits         1792     1771      -21     
+ Misses        499      489      -10     
+ Partials      105       99       -6     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Nov 25 '24 23:11 codecov[bot]