Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix idempotency and improve testability #165

Merged
merged 8 commits into from
Nov 25, 2024
Merged

Fix idempotency and improve testability #165

merged 8 commits into from
Nov 25, 2024

Conversation

hrak
Copy link
Member

@hrak hrak commented Nov 22, 2024 •
edited
Loading

Issue #, if available:

Description of changes:

This PR:

  • Addresses idempotency issues which under some circumstances resulted in volumes being attached hundreds of times to the same mountpoint
  • (Re)introduces locking in several places (controller and node publish/unpublish)
  • Adds checks for max attached volumes per node
  • Refactors interfaces and types to allow us to add tests. A mock cloud package is added too using gomock, along with some initial tests.

Testing performed:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@hrak hrak marked this pull request as draft November 22, 2024 12:41
hrak added 2 commits November 22, 2024 23:00
@hrak
fix: Ensure NodePublishVolume is idempotent
ef2da5b
@hrak
fix: Restore locking on node publish/unpublish volume
9a30d2c 
We cant rely on the assumption that kubelet will neatly serialize these calls. In clusters under load, this could lead to volumes being mounted hundreds of times on the same mount target.
hrak added 2 commits November 25, 2024 10:32
@hrak
fix: Actually check for max attached volumes per node
8f1c69f 
+ add locks to controller publish/unpublish and adjust some logging
@hrak
refactor: Reduce complexity in NodePublishVolume + fix volumepath in …
5f3c4ef 
…stats

Split off the block/mount capability logic into separate functions & adjust logging
@hrak hrak changed the title Feat/testability Fix idempotency and improve testability Nov 25, 2024
@hrak hrak marked this pull request as ready for review November 25, 2024 09:45
FarnazBGH
FarnazBGH approved these changes Nov 25, 2024
View reviewed changes
Copy link
Collaborator

@FarnazBGH FarnazBGH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@hrak hrak merged commit a6c8784 into master Nov 25, 2024
2 checks passed
@hrak hrak deleted the feat/testability branch November 25, 2024 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FarnazBGH FarnazBGH FarnazBGH approved these changes

@lanord lanord Awaiting requested review from lanord

@kasperhendriks kasperhendriks Awaiting requested review from kasperhendriks

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hrak @FarnazBGH