Simplify attach process #322

guilhem · 2024-11-25T23:04:15Z

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

Also fix already mount error code

codecov · 2024-11-25T23:35:14Z

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 ⚠️

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

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

srust · 2024-11-26T15:28:07Z

internal/driver/controllerserver_helper.go

+			switch {
+			case strings.Contains(apiErr.Message, "is already attached"):
+				return errAlreadyAttached
+			case strings.Contains(apiErr.Message, "Maximum number of block storage volumes are attached to this Linode"):


Ok, but this seems brittle when the API changes their text message. How can we check this error, but not rely on the spelling of an API text error message?

That's an excellent question.
API doesn't document errors

And linodego report request as it

But we have to notice this logic was already done before in CSI :/

linode-blockstorage-csi-driver/internal/driver/controllerserver_helper.go

Line 725 in 17b7d1b

if errors.As(err, &apiErr) && strings.Contains(apiErr.Message, "is already attached") {

Yeah def not the ideal way to do it. Hopefully linodego can provide better error codes in future so we don't have to build logic based on the err strings :/

srust · 2024-11-26T15:28:39Z

internal/driver/controllerserver_helper.go

@@ -677,33 +650,6 @@ func (cs *ControllerServer) getInstance(ctx context.Context, linodeID int) (*lin
 	return instance, nil
 }



Is there really no value in having these checks client side at all?

I tend to prefer to rely on API internal logic and validation and for CSI to act as a simple pass-through.

The problem I see could be a rate-limiting issue on API (GET vs. POST).
But attacher already has a backoff on error
https://github.com/kubernetes-csi/external-attacher?tab=readme-ov-file#csi-error-and-timeout-handling

But that may be questionable, or we may have to wait for linodego to implement rate-limiting.
That's why this PR was in draft state at first.

guilhem · 2024-11-26T22:40:58Z

internal/driver/errors.go

@@ -34,6 +34,10 @@ var (
 	// attachments allowed for the instance, call errMaxVolumeAttachments.
 	errMaxAttachments = status.Error(codes.ResourceExhausted, "max number of volumes already attached to instance")

+	// errAlreadyAttached is used to indicate that a volume is already attached
+	// to a Linode instance.
+	errAlreadyAttached = status.Error(codes.FailedPrecondition, "volume is already attached")


Before merging, I want to wait for an answer on this issue
kubernetes-csi/external-attacher#604

komer3 · 2024-11-27T19:30:55Z

internal/driver/controllerserver_helper.go

+			case strings.Contains(apiErr.Message, "is already attached"):
+				return errAlreadyAttached


Question: We only get this err when the volume is attached to another linode?

I think pass the specific err message from the api which could include linode id of another node. This would be helpful in debugging issue in future.

komer3 · 2024-11-27T19:32:10Z

internal/driver/controllerserver_helper.go

+			case strings.Contains(apiErr.Message, "is already attached"):
+				return errAlreadyAttached
+			case strings.Contains(apiErr.Message, "Maximum number of block storage volumes are attached to this Linode"):
+				return errMaxAttachments


Similar to my suggestion from other comment. We should try to pass along the err message by the api here too

komer3 · 2024-11-27T19:34:11Z

internal/driver/controllerserver_helper.go

+			switch {
+			case strings.Contains(apiErr.Message, "is already attached"):
+				return errAlreadyAttached
+			case strings.Contains(apiErr.Message, "Maximum number of block storage volumes are attached to this Linode"):


Yeah def not the ideal way to do it. Hopefully linodego can provide better error codes in future so we don't have to build logic based on the err strings :/

guilhem requested review from a team as code owners November 25, 2024 23:04

guilhem mentioned this pull request Nov 25, 2024

refactor: remove capacity checking when attaching #312

Closed

10 tasks

guilhem force-pushed the simpleattach branch 2 times, most recently from 2bc84b3 to 7449925 Compare November 25, 2024 23:26

guilhem added 2 commits November 26, 2024 00:31

refactor: remove capacity checking when attaching

154621f

feat: report RESOURCE_EXHAUSTED when max volume

efdefae

Also fix already mount error code

guilhem force-pushed the simpleattach branch from 7449925 to efdefae Compare November 25, 2024 23:32

guilhem changed the title ~~Simpleattach~~ Simplify attach process Nov 26, 2024

srust reviewed Nov 26, 2024

View reviewed changes

guilhem commented Nov 26, 2024

View reviewed changes

komer3 reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify attach process #322

Simplify attach process #322

guilhem commented Nov 25, 2024

codecov bot commented Nov 25, 2024

srust Nov 26, 2024

guilhem Nov 26, 2024

komer3 Nov 27, 2024

srust Nov 26, 2024

guilhem Nov 26, 2024 •

edited

Loading

guilhem Nov 26, 2024

komer3 Nov 27, 2024

komer3 Nov 27, 2024

komer3 Nov 27, 2024

		@@ -677,33 +650,6 @@ func (cs ControllerServer) getInstance(ctx context.Context, linodeID int) (lin
		return instance, nil
		}

		case strings.Contains(apiErr.Message, "is already attached"):
		return errAlreadyAttached

Simplify attach process #322

Are you sure you want to change the base?

Simplify attach process #322

Conversation

guilhem commented Nov 25, 2024

Context

Motivation

Risk

Possible Mitigations

Simplification of volume attachment logic:

Removal of related test cases:

General:

Pull Request Guidelines:

codecov bot commented Nov 25, 2024

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhem Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guilhem Nov 26, 2024 •

edited

Loading