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.

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

feat: add MaxVolumesPerNode setting #242

Merged
merged 5 commits into from
Dec 4, 2023

Conversation

dongjiang1989
Copy link
Collaborator

This PR adds support for returning max volume limit in CSI NodeGetInfo call, It returns MaxVolumesPerNode in CSINodeGetInfo response, which defaults to 64, This can be configured via MAX_VOLUMES_PERNODE from ENV config.

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2023

Codecov Report

Attention: 15 lines in your changes are missing coverage. Please review.

Comparison is base (3f84fda) 32.31% compared to head (3b4688a) 32.24%.

Files Patch % Lines
pkg/csi/nodeserver.go 0.00% 15 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #242      +/-   ##
==========================================
- Coverage   32.31%   32.24%   -0.08%     
==========================================
  Files          41       41              
  Lines        6411     6426      +15     
==========================================
  Hits         2072     2072              
- Misses       4050     4065      +15     
  Partials      289      289              
Flag Coverage Δ
unittests 32.24% <0.00%> (-0.08%) ⬇️

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

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

Signed-off-by: dongjiang1989 <[email protected]>
Signed-off-by: dongjiang1989 <[email protected]>
@dongjiang1989
Copy link
Collaborator Author

@peter-wangxu addressed comments, please review :)

Copy link
Collaborator

@peter-wangxu peter-wangxu left a comment

Choose a reason for hiding this comment

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

Looks fine, but some nits. thanks

pkg/csi/nodeserver.go Outdated Show resolved Hide resolved
pkg/csi/type.go Outdated Show resolved Hide resolved
helm/values.yaml Outdated Show resolved Hide resolved
helm/values-acka.yaml Outdated Show resolved Hide resolved
helm/templates/agent.yaml Outdated Show resolved Hide resolved
@peter-wangxu
Copy link
Collaborator

BTW, any reason to set this limit, do you meet any limitation on lvm/linux?

@peter-wangxu peter-wangxu reopened this Dec 1, 2023
Signed-off-by: dongjiang1989 <[email protected]>
@dongjiang1989
Copy link
Collaborator Author

BTW, any reason to set this limit, do you meet any limitation on lvm/linux?

Setting a reasonable number of volumes is related to Node disk pressure and Pod balanced deployment.
In my case: lvm2 on aws ebs

@peter-wangxu Please re-check

@dongjiang1989 dongjiang1989 changed the title add MaxVolumesPerNode setting feat: add MaxVolumesPerNode setting Dec 2, 2023
@peter-wangxu peter-wangxu merged commit 8d7aac8 into alibaba:main Dec 4, 2023
6 checks passed
@dongjiang1989
Copy link
Collaborator Author

@peter-wangxu Please release patch version 🤔️

@peter-wangxu
Copy link
Collaborator

I have not verify this version in production env, do you mind a pre-relase for current version?

@dongjiang1989
Copy link
Collaborator Author

I have not verify this version in production env, do you mind a pre-relase for current version?

Yes. Pre-release versions are helpful to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants