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

Add annotations to allow minimum-increase amount and a maximum-increase amount #303

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

mohammed-uddin
Copy link

Adding a feature that will take the greater of minimum-increase annotation and increase annotation and then the less of that result and the maximum-increase annotation.

Signed-off-by: Mohammed Uddin <[email protected]>
Signed-off-by: Mohammed Uddin <[email protected]>
Signed-off-by: Mohammed Uddin <[email protected]>
Copy link
Contributor

@ushitora-anqou ushitora-anqou left a comment

Choose a reason for hiding this comment

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

I'm curious about the expected use cases of this feature. Could you share an example with me?

internal/runners/pvc_autoresizer.go Outdated Show resolved Hide resolved
internal/runners/pvc_autoresizer.go Outdated Show resolved Hide resolved
internal/runners/pvc_autoresizer.go Outdated Show resolved Hide resolved
@mohammed-uddin
Copy link
Author

mohammed-uddin commented Jan 21, 2025

I'm curious about the expected use cases of this feature. Could you share an example with me?

@ushitora-anqou @llamerada-jp

Sure thing 👍

We have some PVCs that start with an initial 5Gi for example. We don't want to size those up by 10% (cieling(5.2Gi) = 6Gi). The reason is because we are only able to perform 1 resize per X hours. And we know that 1Gi increase has a high likelihood of not being enough. In such cases we would just increase by 5Gi for example.

The inclusion of maximum-increase is both for the sake of being consistent and because we don't wan't to resize a 100Gi PVC by 25% for example rather we'd up by 15Gi. (numbers chosen entirely arbitrarily but I hope I've demonstrated the use case).

Signed-off-by: Mohammed Uddin <[email protected]>
@ushitora-anqou
Copy link
Contributor

Thanks for explaining that. I'm still a bit confused about why the existing resize.topolvm.io/increase annotation doesn't meet your needs. For instance, you can increase your PVC from 5Gi to 10Gi by using resize.topolvm.io/increase: 100% or resize.topolvm.io/increase: 5Gi. Could you share more details about your situation?

@mohammed-uddin
Copy link
Author

mohammed-uddin commented Jan 22, 2025

Thanks for explaining that. I'm still a bit confused about why the existing resize.topolvm.io/increase annotation doesn't meet your needs. For instance, you can increase your PVC from 5Gi to 10Gi by using resize.topolvm.io/increase: 100% or resize.topolvm.io/increase: 5Gi. Could you share more details about your situation?

No problem,

Say that we have a PVC at 5Gi and min-increase is 10Gi and and increase is 20%. This for us is a small PVC that may soon face a large load so we want to size up to 15Gi then 25Gi then 35Gi and so on... While it's "small" we want to increase the memory more aggressively as the percentage of a small number is a small number and may not be enough. But once this same PVC reaches 100Gi for example an increase of 5Gi will not be enough for us. At this point an increase of 10% is what we want.

you can increase your PVC from 5Gi to 10Gi by using resize.topolvm.io/increase: 100% or resize.topolvm.io/increase: 5Gi. Could you share more details about your situation?

You are right, but this is becomes bad for us when that PVC gets large. While yes we would resize from 5Gi to 10Gi this PVC would eventually become 100Gi after enough resizes. If the threshold is crossed there when the disk is already at 100Gi the resize 100% would give us a 200Gi PVC (way too big and wasteful). If the threshold were crossed and the resize was 5Gi that resize would be too small and this disk would fill up within the 6 hours that we are not able to resize again.

@llamerada-jp
Copy link
Contributor

I understand the situation you want to handle. On the other hand, it is difficult for me to agree with the proposed expansion. This is because if there are people who want to make more detailed adjustments than you have proposed, then each time this happens, additional options will be added. Instead of adding options, I would like to propose the following way.

Deploy a shell script that roughly performs the following processing as a K8s cronjob. Since it is a shell script, it is possible to make detailed adjustments for each situation.

NS=<target namespace>
PVCs=$(kubectl get pvc -n ${NS} -l <labels>)
for PVC in ${PVCs}; do
  size=$(kubectl get pvc -n ${NS} ${PVC} -o jsonpath='{.spec.resources.requests.storage}')
  sizeByte=<convert size unit to Byte from Gi or Mi>
  if [ ${sizeByte} -le <boundary value 1> ]; then
    kubectl annotate pvc --overwrite -n ${NS} ${PVC} resize.topolvm.io/increase='<the varlue at your discretion>'
  else if [ ${sizeByte} -le <boundary value 2> ]; then
    kubectl annotate pvc --overwrite -n ${NS} ${PVC} resize.topolvm.io/increase='<the varlue at your discretion>'
  else
    # Add conditions as needed.
  fi
done

I would be happy if you could consider it.

@mohammed-uddin
Copy link
Author

mohammed-uddin commented Jan 24, 2025

@llamerada-jp Thanks for the reply,

This is because if there are people who want to make more detailed adjustments than you have proposed.

The adjustments I propose, I think, are not too detailed.

I understand we don't want to turn the annotations into a language of their own but a minimum/maximum increase provides basic utility without the added complexity of the detailed adjustments others have proposed. What I am proposing isn't complex logic nor is it an ultra-granular options being added to the controller.

Furthermore, I have added them in such a way that they are entirely optional. If not set, the controller behaves the same way.

Deploy a shell script that roughly performs the following processing as a K8s cronjob. Since it is a shell script, it is possible to make detailed adjustments for each situation.

This adds complexity to the use case. There're volumes that are controlled by STSs as well, in that case the annotations belong there and not on the PVC. Rather than look there and patch there it would be much better if the controller did the simple increase = max(min-increase, increase); increase = min(max-increase, increase).

@mohammed-uddin
Copy link
Author

@llamerada-jp @ushitora-anqou

hi friends, any updates?

@llamerada-jp
Copy link
Contributor

We want to avoid customizing things individually. Instead, I've started thinking about using CEL. If my expectations are correct, You don't have to send patches for customizations as needed. It's taking time because I'm still investigating.

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

Successfully merging this pull request may close these issues.

3 participants