refactor: remove capacity checking when attaching

guilhem commented 6 days ago

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:

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

wbh1 commented 5 days ago

@guilhem - do you intend to re-open https://github.com/linode/linode-blockstorage-csi-driver/pull/310? I believe those changes will still be beneficial in order to properly implement the CSI spec and behave the way that kube-scheduler expects.

guilhem commented 5 days ago

@wbh1 totally. Was on draft just for discussion. I'm opening it off team is interested

codecov[bot] commented 2 days ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 74.79%. Comparing base (8df2d46) to head (6f49ca5). Report is 2 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #312 +/- ## ======================================= Coverage 74.79% 74.79% ======================================= Files 22 22 Lines 2396 2396 ======================================= Hits 1792 1792 Misses 499 499 Partials 105 105 ```

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

guilhem commented 2 days ago

duplicate #322

linode / linode-blockstorage-csi-driver