topolvm / pvc-autoresizer

Auto-resize PersistentVolumeClaim objects based on Prometheus metrics
Apache License 2.0
251 stars 29 forks source link

Scale PVCs collectively with additional annotation: `resize.topolvm.io/resize-group-by` #262

Closed fullykubed closed 3 months ago

fullykubed commented 5 months ago

What should the feature do:

An additional annotation, resize.topolvm.io/resize-group-by that implements the following logic:

What is use case behind this feature:

The volumeClaimTemplates field for Kubernetes StatefulSets is immutable after creation. As a result, there is no simple way to update the resize.topolvm.io annotations across all PVCs created by the StatefulSet, especially if the StatefulSet is dynamically autoscaled up and down.

What we'd like to do is annotate one PVC (the first one) via a separate process outside of the StatefulSet management and then ensure that all PVCs in that group grow in the same way.

While we could attempt to annotate all PVCs instead of just one, this suffers from the following drawbacks:

cupnes commented 5 months ago

@fullykubed

The volumeClaimTemplates field for Kubernetes StatefulSets is immutable after creation.

You can delete only the StatefulSet by specifying the --cascade=orphan option with kubectl delete. This allows you to recreate the StatefulSet while leaving the PVCs intact. Does this solve your problem?

fullykubed commented 5 months ago

Thanks for the suggestion!

We are looking for a solution that does not require manual intervention as we define all of these values as a part of a CI system. Also, I do not believe this would send the new annotations to the existing PVCs.

We actually have some other systems in place that regularly sync annotations as our current solution.

However, I think that could all be avoided if the resizer could be configured to resize PVCs as a unit rather than individually. While we don't strictly need it we can always manually intervene, it would be a significant quality-of-life improvement and I hope in the spirit of the resizer (i.e., reducing the need for manual intervention).

cupnes commented 5 months ago

@fullykubed We discussed this issue with the team. We have concerns about your suggestion. Even if we add resize.topolvm.io/resize-group-by, We think it is appropriate behavior to expand the size of the PVCs that actually have the annotation. Expanding the sizes of other PVCs makes it hard to determine the cause of the expansion from the expanded PVCs.

fullykubed commented 5 months ago

@cupnes

That makes sense. For example, it could be confusing if different annotations were applied to different PVCs in the same resize.topolvm.io/resize-group-by group.

Let me propose a different solution that tackles the problem from a different angle:

Proposal

An additional annotation, resize.topolvm.io/settings-from, where the value can be the name of a ConfigMap in the same namespace. This would implement the following behavior:

I believe this would resolve the ambiguity about what settings are applied to a particular PVC. While this extra level of indirection isn't the nicest, it is opt-in and provides a flexible way to update the resizer behavior in circumstances where annotations would otherwise be immutable without manual intervention.

cupnes commented 5 months ago

@fullykubed Thank you for your suggestion. However, the alternative proposal would introduce additional complexity, which we cannot accept. If the issue is that you cannot modify the volumeClaimTemplates, as mentioned in my previous comment, please use kubectl delete --cascade=orphan. We aim to keep the pvc-autoresizer as simple as possible. If more complex control is required, we recommend writing a separate controller.

fullykubed commented 5 months ago

Thanks for the heads up.

Definitely interested in the easiest possible solution to maintain and implement. That was the reason behind my first design proposal which looks to just be an extension of the existing resize.topolvm.io/initial-resize-group-by logic.

Does your team have any alternative ideas on how to solve this issue without manual intervention?

I would continue to advocate for exploring the solution space as many organizations run deployment systems where human intervention in production is disallowed. More than that, managing StatefulSet PVCs is almost certainly a primary use case for the resizer and this workflow:

isn't much better than manually managing the storage settings without the resizer at all.

If this is something you would still be interested in addressing, we are also happy to commit resources to helping to add whichever design path your team would prefer. We love what your team has done so far, but without this feature, we would probably fork to add it given its importance to a core use case.

cupnes commented 5 months ago

@fullykubed In the first place, may I ask why it is necessary to synchronize the resizing of PVCs within the group? Would it not be sufficient to have each volume resized by the pvc-autoresizer when it approaches depletion?

fullykubed commented 5 months ago

@cupnes

Absolutely that would be completely fine, hence my second proposed solution.

Let me restate the problem to ensure we are on the same page:

The canonical way to create PVCs used by many controllers such as StatefulSets is via spec.volumeClaimTemplates. Thus, the canonical way to add resizer annotations currently is via spec.volumeClaimTemplates[].metadata.annotations. However, spec.volumeClaimTemplates is immutable after creation. Thus there is currently no way to change the resizer settings for a StatefulSet's volumes after the StatefulSet has been created (this applies to many other controllers).

The solution your team has proposed (kubectl delete --cascade=orphan) does not work for the reasons outlined in previous comments, particularly with respect to environments where deployment is done via automated systems.

I have proposed adding the ability to resize PVCs as a group as (1) this provides a mechanism to resolve the problem, (2) the resizer already has a partial notion of this concept (resize.topolvm.io/initial-resize-group-by), and (3) conceptually the StatefulSet wants all the PVCs to be exactly the same (hence why it disallows updates).

That all said, if there is an alternative path that can resolve the problem and be easier to implement in the resizer, I am 100% supportive.

cupnes commented 4 months ago

@fullykubed What is the reason for wanting to continuously resize the PVC as a group? What kind of problems will be resolved by doing so?

I thought that the essence of the problem might be that the StatefulSets are immutable. However, this issue is not limited to the pvc-autoresizer. I think it might be better to create a controller that bypasses the StatefulSets and adds annotations as another general-purpose controller, rather than implementing to the pvc-autoresizer.

fullykubed commented 4 months ago

I thought that the essence of the problem might be that the StatefulSets are immutable.

This is the essence of the problem.

However, this issue is not limited to the pvc-autoresizer

While it is not limited to the pvc-autoresizer, the autoresizer is not very useful if its settings cannot be updated for the very common use case of pvc templates. As PVC templates can never be updated after creation, I have proposed two separate mechanisms to work around this limitation: (1) resize as a group so a user can annotate just one PVC, (2) allow the autoresizer to source settings from a separate, mutable object like a configmap.

Either option would make the pvc-autoresizer useful for managing PVCs created by controllers such as StatefulSets which I hope would be a net positive for this project. We are also 100% open to alternative solutions that do not require manual interventions if your team has one?

I think it might be better to create a controller that bypasses the StatefulSets and adds annotations as another general-purpose controller, rather than implementing to the pvc-autoresizer.

In theory, I would agree. In practice, no such controller exists (unless I am ignorant of one). Needing to copy annotations across objects isn't a very common problem (at least in my experience).

From a pure level-of-effort perspective, it would be easier to fork the autoresizer to add the ability to source settings from a ConfigMap than building an entirely new annotations syncing controller.

That said, our preference is to work with you and team to align on a solution that balances the costs of increased complexity with the ability to use the controller in common scenarios. Are you also interested in finding a solution?

cupnes commented 4 months ago

The annotation issue with StatefulSets is a more common problem than you might think, and it is recognized as a Kubernetes issue. Here is the related issue: https://github.com/kubernetes/kubernetes/issues/121178

Therefore, we believe that the demand for a controller that bypasses StatefulSets is not as low as you might think.

Are you also interested in finding a solution?

It is difficult to accept your specifications as they deviate from the simplicity and existing architecture that the pvc-autoresizer aims for.

fullykubed commented 4 months ago

Regarding building an entirely separate controller: I am 100% in agreement there is a case to be made for such a controller to exist.

However, I think we are also aligned in that we are both looking for the simplest possible solution to the problem at hand: that the autoresizer has limited usefulness in many common scenarios due to this limitation in core Kubernetes.

As we have called out, it would be significantly easier for us to maintain an enhanced fork of this project than build an entirely new controller to workaround its limitations. That said, we would love to collaborate instead.


Can you be more specific about what would be an acceptable enhancement to the resizer for this problem?

I have been trying very hard to collaborate, presenting multiple proposals and offering resources from my company to build and maintain whatever enhancement you would prefer.

As our user base sees it, the resizer is not useful without some enhancement here to allow it be used for StatefulSets and other controllers. Correct me if I am wrong, but it seems like your team is strongly philosophically opposed to solving this problem from within the resizer itself.

If that is the case, can you let us know so we can get started on the fork? If not, can you please offer some guidance that helps move us closer to an acceptable proposal?

cupnes commented 4 months ago

Can you be more specific about what would be an acceptable enhancement to the resizer for this problem?

We can only accept the creation of an entirely separate controller. There is an issue with updating the annotations of the StatefulSet in Kubernetes. If this issue is resolved, the functionality we are discussing here will no longer be necessary. In that case, if it is a separate controller, we can simply remove that controller.

github-actions[bot] commented 3 months ago

This issue has been automatically marked as stale because it has not had any activity for 30 days. It will be closed in a week if no further activity occurs. Thank you for your contributions.

github-actions[bot] commented 3 months ago

This issue has been automatically closed due to inactivity. Please feel free to reopen this issue (or open a new one) if this still requires investigation. Thank you for your contribution.