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

Support Elasticsearch volumes expansion #3752

Merged
merged 25 commits into from
Oct 2, 2020
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
000e05a
Remove the PVC validation webhook
sebgl Sep 15, 2020
41e750f
RBAC access to read storage classes
sebgl Sep 15, 2020
2a28b6b
Move GetClaim to a common place for reuse
sebgl Sep 15, 2020
1607903
Rename pvc.go to pvc_gc.go
sebgl Sep 15, 2020
20e51e1
Resize PVCs whose storage req has changed
sebgl Sep 15, 2020
9b24749
Delete StatefulSet for recreation on volume expansion
sebgl Sep 15, 2020
6be1279
Pass the replicas count instead of the entire sset to LimitMasterNode…
sebgl Sep 15, 2020
613e650
Take into account temporarily deleted ssets when adjusting replicas
sebgl Sep 15, 2020
607136b
Plug the volume expansion logic to the upscale phase
sebgl Sep 15, 2020
cbe0a97
Remove deprecated rolling upgrade partition strategy from upscale tests
sebgl Sep 15, 2020
8080ad7
Garbage collect PVCs after having dealt with temporarily deleted ssets
sebgl Sep 15, 2020
8872518
Use the highest existing ordinal instead of the pods count
sebgl Sep 15, 2020
f489396
Add missing license headers
sebgl Sep 15, 2020
e4fe580
Improvements from PR review
sebgl Sep 15, 2020
08c54d1
Revert "RBAC access to read storage classes"
sebgl Sep 17, 2020
aaf0b58
Remove storage class checks for rbac concerns
sebgl Sep 17, 2020
4049840
Revert "Remove the PVC validation webhook"
sebgl Sep 17, 2020
ffe8e59
Ignore storage requests changes in the pvc validation webhook
sebgl Sep 17, 2020
0793b91
Reword the pvc modification error msg
sebgl Sep 22, 2020
697f1c1
Store statefulsets to recreate in an annotation
sebgl Sep 29, 2020
55a5c77
Remove the special upscale state fix not needed anymore
sebgl Sep 30, 2020
5121657
Improvements from PR review
sebgl Sep 30, 2020
f22af15
Set a temporary ownerref on Pods while the StatefulSet is being recre…
sebgl Sep 30, 2020
e82ba2d
Fix unit tests
sebgl Oct 1, 2020
9e442b1
Fix unit tests
sebgl Oct 1, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 28 additions & 12 deletions pkg/apis/elasticsearch/v1/validations.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,14 @@ import (
"net"
"strings"

apiequality "k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/util/validation/field"

commonv1 "github.com/elastic/cloud-on-k8s/pkg/apis/common/v1"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/version"
esversion "github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/version"
netutil "github.com/elastic/cloud-on-k8s/pkg/utils/net"
corev1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/resource"
"k8s.io/apimachinery/pkg/util/validation/field"
)

const (
Expand All @@ -29,7 +30,7 @@ const (
nodeRolesInOldVersionMsg = "node.roles setting is not available in this version of Elasticsearch"
parseStoredVersionErrMsg = "Cannot parse current Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]"
parseVersionErrMsg = "Cannot parse Elasticsearch version. String format must be {major}.{minor}.{patch}[-{label}]"
pvcImmutableMsg = "Volume claim templates cannot be modified"
pvcImmutableMsg = "Volume claim templates can only have their storage requests increased, if the storage class allows volume expansion. Any other change is forbidden."
unsupportedConfigErrMsg = "Configuration setting is reserved for internal use. User-configured use is unsupported"
unsupportedUpgradeMsg = "Unsupported version upgrade path. Check the Elasticsearch documentation for supported upgrade paths."
unsupportedVersionMsg = "Unsupported version"
Expand Down Expand Up @@ -207,29 +208,44 @@ func checkNodeSetNameUniqueness(es *Elasticsearch) field.ErrorList {
return errs
}

// pvcModification ensures no PVCs are changed, as volume claim templates are immutable in stateful sets
// pvcModification ensures the only part of volume claim templates that can be changed is storage requests.
// VolumeClaimTemplates are immutable in StatefulSets, but we still manage to deal with volume expansion.
func pvcModification(current, proposed *Elasticsearch) field.ErrorList {
var errs field.ErrorList
if current == nil || proposed == nil {
return errs
}
for i, node := range proposed.Spec.NodeSets {
currNode := getNode(node.Name, current)
if currNode == nil {
for i, proposedNodeSet := range proposed.Spec.NodeSets {
currNodeSet := getNode(proposedNodeSet.Name, current)
sebgl marked this conversation as resolved.
Show resolved Hide resolved
if currNodeSet == nil {
// this is a new sset, so there is nothing to check
continue
}

// ssets do not allow modifications to fields other than 'replicas', 'template', and 'updateStrategy'
// reflection isn't ideal, but okay here since the ES object does not have the status of the claims.
// Check that no modification was made to the volumeClaimTemplates, except on storage requests.
// Checking semantic equality here allows providing PVC storage size with different units (eg. 1Ti vs. 1024Gi).
if !apiequality.Semantic.DeepEqual(node.VolumeClaimTemplates, currNode.VolumeClaimTemplates) {
errs = append(errs, field.Invalid(field.NewPath("spec").Child("nodeSet").Index(i).Child("volumeClaimTemplates"), node.VolumeClaimTemplates, pvcImmutableMsg))
// We could conceptually also disallow storage decrease here (unsupported), but it would make it hard for users
// to revert to the previous size if tried increasing but their storage class doesn't support expansion.
sebgl marked this conversation as resolved.
Show resolved Hide resolved
currentClaims := currNodeSet.VolumeClaimTemplates
proposedClaims := proposedNodeSet.VolumeClaimTemplates
if !apiequality.Semantic.DeepEqual(claimsWithoutStorageReq(currentClaims), claimsWithoutStorageReq(proposedClaims)) {
errs = append(errs, field.Invalid(field.NewPath("spec").Child("nodeSet").Index(i).Child("volumeClaimTemplates"), proposedClaims, pvcImmutableMsg))
}
}
return errs
}
sebgl marked this conversation as resolved.
Show resolved Hide resolved

// claimsWithoutStorageReq returns a copy of the given claims, with all storage requests set to the empty quantity.
func claimsWithoutStorageReq(claims []corev1.PersistentVolumeClaim) []corev1.PersistentVolumeClaim {
result := make([]corev1.PersistentVolumeClaim, 0, len(claims))
for _, claim := range claims {
patchedClaim := *claim.DeepCopy()
patchedClaim.Spec.Resources.Requests[corev1.ResourceStorage] = resource.Quantity{}
result = append(result, patchedClaim)
}
return result
}

func noDowngrades(current, proposed *Elasticsearch) field.ErrorList {
var errs field.ErrorList
if current == nil || proposed == nil {
Expand Down
31 changes: 1 addition & 30 deletions pkg/apis/elasticsearch/v1/validations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,7 +326,7 @@ func Test_pvcModified(t *testing.T) {
expectErrors bool
}{
{
name: "resize fails",
name: "resize succeeds",
sebgl marked this conversation as resolved.
Show resolved Hide resolved
current: current,
proposed: &Elasticsearch{
Spec: ElasticsearchSpec{
Expand All @@ -352,35 +352,6 @@ func Test_pvcModified(t *testing.T) {
},
},
},
expectErrors: true,
},
{
name: "same size with different unit accepted",
current: current,
proposed: &Elasticsearch{
Spec: ElasticsearchSpec{
Version: "7.2.0",
NodeSets: []NodeSet{
{
Name: "master",
VolumeClaimTemplates: []corev1.PersistentVolumeClaim{
{
ObjectMeta: metav1.ObjectMeta{
Name: "elasticsearch-data",
},
Spec: corev1.PersistentVolumeClaimSpec{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceStorage: resource.MustParse("5120Mi"),
},
},
},
},
},
},
},
},
},
expectErrors: false,
},
{
Expand Down
29 changes: 24 additions & 5 deletions pkg/controller/elasticsearch/driver/nodes.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,28 @@ func (d *defaultDriver) reconcileNodeSpecs(
return results.WithResult(defaultRequeue)
}

actualStatefulSets, err := sset.RetrieveActualStatefulSets(d.Client, k8s.ExtractNamespacedName(&d.ES))
// recreate any StatefulSet that needs to account for PVC expansion
recreations, err := recreateStatefulSets(d.K8sClient(), d.ES)
if err != nil {
return results.WithError(err)
return results.WithError(fmt.Errorf("StatefulSet recreation: %w", err))
}
if recreations > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

// Some StatefulSets are in the process of being recreated to handle PVC expansion:
// it is safer to requeue until the re-creation is done.
// Otherwise, some operation could be performed with wrong assumptions:
// the sset doesn't exist (was just deleted), but the Pods do actually exist.
log.V(1).Info("StatefulSets recreation in progress, re-queueing.",
"namespace", d.ES.Namespace, "es_name", d.ES.Name, "recreations", recreations)
return results.WithResult(defaultRequeue)
}

expectedResources, err := nodespec.BuildExpectedResources(d.ES, keystoreResources, actualStatefulSets, d.OperatorParameters.IPFamily, d.OperatorParameters.SetDefaultSecurityContext)
actualStatefulSets, err := sset.RetrieveActualStatefulSets(d.Client, k8s.ExtractNamespacedName(&d.ES))
if err != nil {
return results.WithError(err)
}

if err := GarbageCollectPVCs(d.K8sClient(), d.ES, actualStatefulSets, expectedResources.StatefulSets()); err != nil {
expectedResources, err := nodespec.BuildExpectedResources(d.ES, keystoreResources, actualStatefulSets, d.OperatorParameters.IPFamily, d.OperatorParameters.SetDefaultSecurityContext)
if err != nil {
return results.WithError(err)
}

Expand All @@ -74,7 +85,7 @@ func (d *defaultDriver) reconcileNodeSpecs(
esState: esState,
expectations: d.Expectations,
}
actualStatefulSets, err = HandleUpscaleAndSpecChanges(upscaleCtx, actualStatefulSets, expectedResources)
upscaleResults, err := HandleUpscaleAndSpecChanges(upscaleCtx, actualStatefulSets, expectedResources)
if err != nil {
reconcileState.AddEvent(corev1.EventTypeWarning, events.EventReconciliationError, fmt.Sprintf("Failed to apply spec change: %v", err))
var podTemplateErr *sset.PodTemplateError
Expand All @@ -84,12 +95,20 @@ func (d *defaultDriver) reconcileNodeSpecs(
}
return results.WithError(err)
}
if upscaleResults.Requeue {
return results.WithResult(defaultRequeue)
}
actualStatefulSets = upscaleResults.ActualStatefulSets

// Update PDB to account for new replicas.
if err := pdb.Reconcile(d.Client, d.ES, actualStatefulSets); err != nil {
return results.WithError(err)
}

if err := GarbageCollectPVCs(d.K8sClient(), d.ES, actualStatefulSets, expectedResources.StatefulSets()); err != nil {
return results.WithError(err)
}
sebgl marked this conversation as resolved.
Show resolved Hide resolved

// Phase 2: if there is any Pending or bootlooping Pod to upgrade, do it.
attempted, err := d.MaybeForceUpgrade(actualStatefulSets)
if err != nil || attempted {
Expand Down
Loading