Skip to content

make pvc count in group configurable #803

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

Merged
merged 1 commit into from
May 14, 2025

Conversation

Nikhil-Ladha
Copy link
Contributor

@Nikhil-Ladha Nikhil-Ladha commented May 6, 2025

Added support for configuring the pvc count in a group. The min and max values are 1 and 100, anything less and more than that would result in an error in the controller manager.
Also, added necessary unit tests to validate the values in the config.

Fixes: #787

@nixpanic
Copy link
Collaborator

nixpanic commented May 7, 2025

Looks good, but 100 PVCs in a group sounds like really a lot. Are there any use-cases known that make groups, and do we know how large such a group is common?

Please update the documentation in docs/csi-addons-config.md too.

@Nikhil-Ladha
Copy link
Contributor Author

Looks good, but 100 PVCs in a group sounds like really a lot. Are there any use-cases known that make groups, and do we know how large such a group is common?

I am not aware of a direct use case as such were such a large number would be required. This is the upper limit that we would allow, and that is the max count that has been tested by ceph team too, some DR use cases would require somewhere around 60-80 at max. Hence, setting the limit and default value to 100, so that it the users wont' really have to tweak the value until and unless there is some exception.

Please update the documentation in docs/csi-addons-config.md too.

Done.

@nixpanic
Copy link
Collaborator

nixpanic commented May 8, 2025

This is the upper limit that we would allow, and that is the max count that has been tested by ceph team too, some DR use cases would require somewhere around 60-80 at max.

would allow in case of Ceph, I assume? If Ceph is the reference for the value, it would be good to mention that in a comment where the default is set in the code.

That is the only thing that I'd like to see improved, looks good otherwise.

@Nikhil-Ladha
Copy link
Contributor Author

Nikhil-Ladha commented May 8, 2025

This is the upper limit that we would allow, and that is the max count that has been tested by ceph team too, some DR use cases would require somewhere around 60-80 at max.

would allow in case of Ceph, I assume? If Ceph is the reference for the value, it would be good to mention that in a comment where the default is set in the code.

That is the only thing that I'd like to see improved, looks good otherwise.

Ack, a note like the following in the docs/csi-addons-config.yaml should be fine I think?

NOTE: `max-group-pvcs` default value is set based on ceph's support/testing. User can tweak this value based on the supported count for their storage vendor.

@Nikhil-Ladha
Copy link
Contributor Author

Added the note, please take a look now.
Thanks :)

Copy link
Member

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Thanks

Added support for configuring the pvc count in  a group.
The min and max values are 1 and 100, anything less and more than
that would result in an error in the controller manager.
Also, added necessary unit tests to validate the values in
the config.

Signed-off-by: Nikhil-Ladha <[email protected]>
@mergify mergify bot force-pushed the configure-count branch from 7ee2bf7 to f201bb6 Compare May 14, 2025 05:39
@mergify mergify bot merged commit 130ed87 into csi-addons:main May 14, 2025
15 checks passed
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.

make pvc count in a group configurable
3 participants