Support Elasticsearch volumes expansion #3752

sebgl · 2020-09-15T09:18:56Z

Add support for resizing Elasticsearch volumes, by simply editing the storage requests in the volumeClaimTemplates section of the spec.

Demo

Constraints

Volume size can only be increased, not decreased (k8s limitation, not likely to change anytime soon)
Only possible if the storage class specifies allowVolumeExpansion: true
Hot filesystem resize without re-creating the Pod is possible in many cases (GCE-PD, AWS-EBS, Azure, TopoLVM, etc.). If the storage driver does not allow it, it is necessary to manually delete the Pods. ECK does not do it.

Implementation overview

Resizing PVCs is not supported by the StatefulSet controller at this time. The volume section of the StatefulSet spec is immutable.
However, it is possible to work around the StatefulSet limitations this way:

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.
We now have resized volumes, but the StatefulSet still specifies the old volume size. Any new replica will end up with a smaller PVC, that in turn needs to be resized.
Delete the StatefulSet (but not the Pods it owns), then recreate it with the new volume spec.
Address the case where Pods may become orphans if something happens while the StatefulSet is getting recreated, by ensuring we always recreate deleted StatefulSets, and ensure we set a temporary ownerRef on the Pods while they are not owned by the deleted StatefulSet anymore.

Follow-up (in other PRs)

Add E2E tests to cover volume expansion: Add an E2E test to cover volume resize #3769
Write documentation: Document volume expansion and volume claim immutability #3797
(later) Figure out a way to automatically recreate Pods if inline filesystem expansion is not supported: Recreate Pods if necessary after volume expansion #3757
Check whether storage class is compatible with volume expansion: Check if the storage class allows volume expansion #3767

Relates #325.

We'll reintroduce it later, in a form that allows modifying PVC storage reqs.

…sCreation

...when setting the initial replicas values for statefulset re-creation. Otherwise, if we're unlucky and one Pod is missing for some reasons (eg. pod-0), using the Pod count could lead to removing the Pods with highest ordinals.

hack/manifest-gen/assets/charts/eck/templates/_helpers.tpl

pkg/apis/elasticsearch/v1/validations.go

pkg/controller/elasticsearch/driver/nodes.go

pkg/controller/elasticsearch/driver/upscale.go

david-kow

Nice work @sebgl. I did some testing and left some comments, nothing major.

pkg/apis/elasticsearch/v1/validations.go

pkg/controller/elasticsearch/driver/upscale.go

pkg/controller/elasticsearch/driver/pvc_expansion.go

pkg/controller/elasticsearch/driver/upscale.go

pkg/controller/elasticsearch/driver/pvc_expansion.go

sebgl · 2020-09-17T16:13:22Z

There's a concern that some users may not be allowed to deploy ECK if it needs read access to storage classes (cluster-wide) resource.
I'll update this PR to remove any storage class checks and restore a minimal validation webhook that allows resizing.
I created another issue to deal with the storage class thing specifically: #3767

This reverts commit 41e750f.

This reverts commit 000e05a.

sebgl · 2020-09-18T08:27:46Z

Jenkins test this please.

barkbay

I'm still doing some tests but overall it looks good 👍
Also I just want to be sure we don't want to handle the case where the Elasticsearch resource is deleted before the statefulset is re-created, leaving some Pods orphaned ?

pkg/controller/elasticsearch/driver/pvc_expansion.go

pkg/controller/elasticsearch/driver/upscale.go

…ated If the Elasticsearch resource is removed while the StatefulSet is being recreated, orphan Pods will stay around forever. To avoid that situation, we can set append a temporary owner ref to the Pods before deleting their StatefulSet, so Pods don't stay orphans if Elasticsearch is deleted. We can then remove the temporary owner ref that was set after the StatefulSet is recreated.

sebgl · 2020-09-30T14:20:38Z

Also I just want to be sure we don't want to handle the case where the Elasticsearch resource is deleted before the statefulset is re-created, leaving some Pods orphaned ?

@barkbay thanks for pointing that particular case again!
I tried setting an ownerRef to the ES resource in the podTemplate but according to my tests it looks like the StatefulSet controller applies its own ownerRef (only) and discards ours.
So instead in f22af15 I decided to manually add the Pod ownerRef right before deleting the StatefulSet, then remove it right after the StatefulSet was recreated (before removing the annotation).
It seems to work well and address the orphan Pods problem when the ES resource is deleted in the middle of a StatefulSet re-creation.

barkbay

I left some comments re. the unit tests, I think they are not complete.

pkg/controller/elasticsearch/driver/pvc_expansion.go

pkg/controller/elasticsearch/driver/pvc_expansion_test.go

barkbay

LGTM 👍
Great work !

pkg/controller/elasticsearch/driver/pvc_expansion_test.go

pkg/controller/elasticsearch/driver/upscale_state_test.go

david-kow

It worked well through all my attempts to break it :) nice work. Just one scenario I'm concerned about, the rest are nits.

david-kow · 2020-09-30T13:27:47Z

pkg/controller/elasticsearch/driver/nodes.go

+	if err != nil {
+		return results.WithError(fmt.Errorf("StatefulSet recreation: %w", err))
+	}
+	if recreations > 0 {


Is it possible that the spec provided in ES resource results in invalid statefulset? If yes, then I think, since we use the statefulset from the spec when annotating, we won't make progress past this point because the operator will keep trying to recreate the statefulset from annotation and keep failing.

If the above is valid then I'd be a little uncomfortable, because we could effectively get the operator stuck on wrong config. I think that would be first such case (e.g. changing resource won't work, because we process annotation first).

What we set in the annotation always matches an "actual StatefulSet", ie. a StatefulSet that exists in the apiserver, for which an update (or create) operation was successful in the past. We don't set the annotation based on the "expected StatefulSet".
I think in the case you mention where the StatefulSet cannot possibly be created, the update call would probably have failed beforehand? I don't know if there are some cases where a spec would lead to a successful update but an invalid creation. It seems weird.
WDYT?

I was convinced that we set the expected sset in the annotation, which I see now isn't true. Yes, I think we can dismiss the case of successful update and failed create.

pkg/apis/elasticsearch/v1/validations.go

pkg/apis/elasticsearch/v1/validations_test.go

pkg/controller/elasticsearch/sset/getter.go

pkg/apis/elasticsearch/v1/validations.go

david-kow

LGTM! 🚀

This commit modifies the existing documentation in order to account for the volume expansion feature introduced in elastic#3752. With those changes, it is now possible to resize storage requests if the storage size allows it. Every other change is forbidden. The doc also mentions how Pods may need to be restarted if the volume driver does not support online expansion. I added lower-level titles to better structure the volume claim templates page.

* Document volume expansion and volume claim immutability This commit modifies the existing documentation in order to account for the volume expansion feature introduced in #3752. With those changes, it is now possible to resize storage requests if the storage size allows it. Every other change is forbidden. The doc also mentions how Pods may need to be restarted if the volume driver does not support online expansion. I added lower-level titles to better structure the volume claim templates page. * Improvements from PR review

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 works in the same way as 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 This PR moves some of the PVC-expansion code from Elasticsearch into a common area to avoid code duplication

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

This 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 - * 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 This also 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 --------- Co-authored-by: Karen Metts <35154725+karenzone@users.noreply.github.com> Co-authored-by: Peter Brachwitz <peter.brachwitz@gmail.com>

sebgl added 11 commits September 15, 2020 10:55

Remove the PVC validation webhook

000e05a

We'll reintroduce it later, in a form that allows modifying PVC storage reqs.

RBAC access to read storage classes

41e750f

Move GetClaim to a common place for reuse

2a28b6b

Rename pvc.go to pvc_gc.go

1607903

Resize PVCs whose storage req has changed

20e51e1

Delete StatefulSet for recreation on volume expansion

9b24749

Pass the replicas count instead of the entire sset to LimitMasterNode…

6be1279

…sCreation

Take into account temporarily deleted ssets when adjusting replicas

613e650

Plug the volume expansion logic to the upscale phase

607136b

Remove deprecated rolling upgrade partition strategy from upscale tests

cbe0a97

Garbage collect PVCs after having dealt with temporarily deleted ssets

8080ad7

sebgl added >feature Adds or discusses adding a feature to the product release-highlight Candidate for the ECK release highlight summary v1.3.0 labels Sep 15, 2020

sebgl mentioned this pull request Sep 15, 2020

Support dynamic PVC resize #325

Closed

sebgl commented Sep 15, 2020

View reviewed changes

Add missing license headers

f489396

david-kow reviewed Sep 15, 2020

View reviewed changes

Improvements from PR review

e4fe580

sebgl mentioned this pull request Sep 15, 2020

Recreate Pods if necessary after volume expansion #3757

Open

sebgl added 4 commits September 17, 2020 18:20

Revert "RBAC access to read storage classes"

08c54d1

This reverts commit 41e750f.

Remove storage class checks for rbac concerns

aaf0b58

Revert "Remove the PVC validation webhook"

4049840

This reverts commit 000e05a.

Ignore storage requests changes in the pvc validation webhook

ffe8e59

sebgl requested a review from david-kow September 17, 2020 16:42

This was referenced Sep 18, 2020

Check if the storage class allows volume expansion #3767

Closed

Add an E2E test to cover volume resize #3769

Merged

sebgl requested review from barkbay and david-kow September 29, 2020 16:59

barkbay reviewed Sep 30, 2020

View reviewed changes

pkg/controller/elasticsearch/driver/upscale.go Outdated Show resolved Hide resolved

sebgl added 3 commits September 30, 2020 14:35

Remove the special upscale state fix not needed anymore

55a5c77

Improvements from PR review

5121657

barkbay requested changes Oct 1, 2020

View reviewed changes

Fix unit tests

e82ba2d

barkbay approved these changes Oct 1, 2020

View reviewed changes

pkg/controller/elasticsearch/driver/pvc_expansion_test.go Outdated Show resolved Hide resolved

pkg/controller/elasticsearch/driver/upscale_state_test.go Outdated Show resolved Hide resolved

Fix unit tests

9e442b1

david-kow reviewed Oct 1, 2020

View reviewed changes

david-kow approved these changes Oct 2, 2020

View reviewed changes

sebgl merged commit bcbc61d into master Oct 2, 2020

sebgl mentioned this pull request Oct 2, 2020

Document volume expansion and volume claim immutability #3797

Merged

david-kow deleted the resize-pvc-delete-ssets branch December 16, 2020 11:00

vpedosyuk mentioned this pull request Feb 15, 2021

Dynamic volume resize for VMSingle/VMCluster without pods restart VictoriaMetrics/operator#161

Closed

robbavey mentioned this pull request Jan 23, 2024

Add resize volume functionality to Logstash operator #7465

Closed

robbavey mentioned this pull request Jan 30, 2024

Add resize volume functionality to Logstash operator #7519

Merged

icy mentioned this pull request Mar 6, 2024

bug: increase of buffer pvc size results in reconcile error fluent/fluent-operator#1003

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support Elasticsearch volumes expansion #3752

Support Elasticsearch volumes expansion #3752

sebgl commented Sep 15, 2020 •

edited

Loading

david-kow left a comment

sebgl commented Sep 17, 2020 •

edited

Loading

sebgl commented Sep 18, 2020

barkbay left a comment

sebgl commented Sep 30, 2020 •

edited

Loading

barkbay left a comment

barkbay left a comment

david-kow left a comment

david-kow Sep 30, 2020

sebgl Oct 1, 2020

david-kow Oct 2, 2020

david-kow left a comment

Support Elasticsearch volumes expansion #3752

Support Elasticsearch volumes expansion #3752

Conversation

sebgl commented Sep 15, 2020 • edited Loading

david-kow left a comment

Choose a reason for hiding this comment

sebgl commented Sep 17, 2020 • edited Loading

sebgl commented Sep 18, 2020

barkbay left a comment

Choose a reason for hiding this comment

sebgl commented Sep 30, 2020 • edited Loading

barkbay left a comment

Choose a reason for hiding this comment

barkbay left a comment

Choose a reason for hiding this comment

david-kow left a comment

Choose a reason for hiding this comment

david-kow Sep 30, 2020

Choose a reason for hiding this comment

sebgl Oct 1, 2020

Choose a reason for hiding this comment

david-kow Oct 2, 2020

Choose a reason for hiding this comment

david-kow left a comment

Choose a reason for hiding this comment

sebgl commented Sep 15, 2020 •

edited

Loading

sebgl commented Sep 17, 2020 •

edited

Loading

sebgl commented Sep 30, 2020 •

edited

Loading