Add resize volume functionality to Logstash operator #7519
Conversation
0d6edbd to
346d19b
Compare
6b23bf1 to
c0815cb
Compare
There was a problem hiding this comment.
The PVC check in validation can add a checking to ensure the request size is increase like here, otherwise, LGTM
aba1347 to
86ca421
Compare
|
@kaisecheng I've added the additional check that you suggested, and reworked the webhook validation to check for this too |
This commit moves some of the statefulset functonality from an elasticsearch package to a common one to facilitate its use in other resources
This commit adds support for expanding Logstash volumes by editing the storage requests in spec.VolumeClaimTemplate, based on the existing implenentation in Elasticsearch implemented in elastic#3752. The same constraints hold - * Volume size can only be increased, not decreased * Storage class must specify allowVolumeExpansion: true * Filesystem resize without pod recreation is only possible if the storage driver allows it This is based on the Elasticsearch implementation An update of the storage request in the volumeClaimTemplate will * Update the storage requests spec of all existing PVCs: they are immediately resized by the storage driver, if inline expansion is supported. Otherwise Pods need to be recreated. * Delete the StatefulSet, but not the pod that it owns, storing recreation details in an annotation on the owning Logstash resource * Recreate the StatefulSet with the new volumeClaimTemplate spec * Remove the recreation annotation from the Logstash resource
Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com>
This commit reworks the webhook validation to validate that storage updates fulfill the requirements (only increasing storage, using a valid storage class that allows storage expansion). This required moving the webhook validation into the controller package to allow use of the k8sclient without a dependency cycle, and requires some rework in webhook registration
86ca421 to
1fa362a
Compare
|
buildkite test this -f p=gke,t=TestVolumeExpansion |
| }) | ||
| } | ||
| } | ||
|
|
||
| func Test_recreateStatefulSets(t *testing.T) { |
There was a problem hiding this comment.
This seems to almost duplicate the test with the same name in the common/volumes package which tests the function that this recreateStatefulSets delegates too. The only difference seems the additional kind attribute.
| @@ -113,7 +117,8 @@ type ReconcileLogstash struct { | |||
| dynamicWatches watches.DynamicWatches | |||
| operator.Parameters | |||
| // iteration is the number of times this controller has run its Reconcile method | |||
| iteration uint64 | |||
| iteration uint64 | |||
| expectations *expectations.ClustersExpectation | |||
There was a problem hiding this comment.
ClustersExpectations seems like a name that does not fit well for anything not Elasticsearch.
There was a problem hiding this comment.
We used the ClustersExpectation, rather than the standard Expectation to allow expectations for multiple sets of Logstashes to be stored safely in the same ECK instance, and my understanding was that this was required
There was a problem hiding this comment.
Yes my point was more about the naming, because Logstash is not really a clustered application, not about the fact that you are using it which is appropriate.
Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>
2cda740 to
fbed4df
Compare
Also some refactoring to move NewPodTemplateValidator to common, and stop exporting validatePodTemplate function from common Adds TODO: as reminder to improve log message to include parent type
8f2b898 to
7822d0c
Compare
538238f to
697ac88
Compare
|
I think this is ready for another round @pebrc |
|
Thank you @pebrc! I don't have merge access to this repo, so would you mind hitting the merge button? |
This commit adds support for expanding Logstash volumes by editing the storage requests
in spec.VolumeClaimTemplate, based on the existing implenentation in Elasticsearch
implemented in #3752.
The same constraints hold -
This is based on the Elasticsearch implementation
An update of the storage request in the volumeClaimTemplate will
Note:
Testing Notes
minikube does not have a storage class that supports volume expansion - functionality has been tested in google cloud