-
Notifications
You must be signed in to change notification settings - Fork 625
Refactor Volume Auto Grow to support additional volume types #4239
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,188 @@ | ||
// Copyright 2021 - 2025 Crunchy Data Solutions, Inc. | ||
// | ||
// SPDX-License-Identifier: Apache-2.0 | ||
|
||
package postgrescluster | ||
|
||
import ( | ||
"context" | ||
|
||
corev1 "k8s.io/api/core/v1" | ||
"k8s.io/apimachinery/pkg/api/resource" | ||
|
||
"github.com/crunchydata/postgres-operator/internal/feature" | ||
"github.com/crunchydata/postgres-operator/internal/logging" | ||
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1" | ||
) | ||
|
||
// storeDesiredRequest saves the appropriate request value to the PostgresCluster | ||
// status. If the value has grown, create an Event. | ||
func (r *Reconciler) storeDesiredRequest( | ||
ctx context.Context, cluster *v1beta1.PostgresCluster, | ||
volumeType, instanceSetName, desiredRequest, desiredRequestBackup string, | ||
) string { | ||
var current resource.Quantity | ||
var previous resource.Quantity | ||
var err error | ||
log := logging.FromContext(ctx) | ||
|
||
// Parse the desired request from the cluster's status. | ||
if desiredRequest != "" { | ||
current, err = resource.ParseQuantity(desiredRequest) | ||
if err != nil { | ||
log.Error(err, "Unable to parse "+volumeType+" volume request from status ("+ | ||
desiredRequest+") for "+cluster.Name+"/"+instanceSetName) | ||
// If there was an error parsing the value, treat as unset (equivalent to zero). | ||
desiredRequest = "" | ||
current, _ = resource.ParseQuantity("") | ||
|
||
} | ||
} | ||
|
||
// Parse the desired request from the status backup. | ||
if desiredRequestBackup != "" { | ||
previous, err = resource.ParseQuantity(desiredRequestBackup) | ||
if err != nil { | ||
log.Error(err, "Unable to parse "+volumeType+" volume request from status backup ("+ | ||
desiredRequestBackup+") for "+cluster.Name+"/"+instanceSetName) | ||
// If there was an error parsing the value, treat as unset (equivalent to zero). | ||
desiredRequestBackup = "" | ||
previous, _ = resource.ParseQuantity("") | ||
|
||
} | ||
} | ||
|
||
// determine if the appropriate volume limit is set | ||
limitSet := limitIsSet(cluster, volumeType, instanceSetName) | ||
|
||
if limitSet && current.Value() > previous.Value() { | ||
r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "VolumeAutoGrow", | ||
"%s volume expansion to %v requested for %s/%s.", | ||
volumeType, current.String(), cluster.Name, instanceSetName) | ||
} | ||
|
||
// If the desired size was not observed, update with previously stored value. | ||
// This can happen in scenarios where the annotation on the Pod is missing | ||
// such as when the cluster is shutdown or a Pod is in the middle of a restart. | ||
if desiredRequest == "" { | ||
desiredRequest = desiredRequestBackup | ||
} | ||
|
||
return desiredRequest | ||
} | ||
|
||
// limitIsSet determines if the limit is set for a given volume type and returns | ||
// a corresponding boolean value | ||
func limitIsSet(cluster *v1beta1.PostgresCluster, volumeType, instanceSetName string) bool { | ||
|
||
var limitSet bool | ||
|
||
switch volumeType { | ||
|
||
// Cycle through the instance sets to ensure the correct limit is identified. | ||
case "pgData": | ||
for _, specInstance := range cluster.Spec.InstanceSets { | ||
if specInstance.Name == instanceSetName { | ||
limitSet = !specInstance.DataVolumeClaimSpec.Resources.Limits.Storage().IsZero() | ||
} | ||
} | ||
} | ||
// TODO: Add cases for pgWAL and repo volumes | ||
|
||
return limitSet | ||
|
||
} | ||
|
||
// setVolumeSize compares the potential sizes from the instance spec, status | ||
// and limit and sets the appropriate current value. | ||
func (r *Reconciler) setVolumeSize(ctx context.Context, cluster *v1beta1.PostgresCluster, | ||
pvc *corev1.PersistentVolumeClaim, volumeType, instanceSpecName string) { | ||
|
||
log := logging.FromContext(ctx) | ||
|
||
// Store the limit for this instance set. This value will not change below. | ||
volumeLimitFromSpec := pvc.Spec.Resources.Limits.Storage() | ||
|
||
// This value will capture our desired update. | ||
volumeRequestSize := pvc.Spec.Resources.Requests.Storage() | ||
|
||
// A limit of 0 is ignorned, so the volume request is used. | ||
if volumeLimitFromSpec.IsZero() { | ||
return | ||
} | ||
|
||
// If the request value is greater than the set limit, use the limit and issue | ||
// a warning event. | ||
if volumeRequestSize.Value() > volumeLimitFromSpec.Value() { | ||
r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "VolumeRequestOverLimit", | ||
"%s volume request (%v) for %s/%s is greater than set limit (%v). Limit value will be used.", | ||
volumeType, volumeRequestSize, cluster.Name, instanceSpecName, volumeLimitFromSpec) | ||
|
||
pvc.Spec.Resources.Requests = corev1.ResourceList{ | ||
corev1.ResourceStorage: *resource.NewQuantity(volumeLimitFromSpec.Value(), resource.BinarySI), | ||
} | ||
// Otherwise, if the feature gate is not enabled, do not autogrow. | ||
} else if feature.Enabled(ctx, feature.AutoGrowVolumes) { | ||
|
||
// determine the appropriate volume request based on what's set in the status | ||
if dpv, err := getDesiredVolumeSize( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what do we think the "P" in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm also not seeing where There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh is see, it took me a minute to get here - maybe its worth changing something here ♻️
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. thinking about the logic in this function
if limit isn't set, autogrow isn't enabled so do nothing maybe...
this is assuming we will always use the "desired" volume size if autogrow is enabled and I might be wrong in that assumption 💭 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we simplified to that logic, I think that eliminates the warning event for a defined request that is higher than the limit. That's a useful warning in my opinion because it helps explains unexpected behavior (due to a misconfiguration on the user's part; you shouldn't even define a request that's higher than the limit you define in the same object). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For the return by reference, we could definitely return it explicitly. I'm not returning anything that's not used, it's just that the one value is only used in the error log, which feels ok to me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which event would we be eliminating? dvp doesn't seem to be needed outside of getDesiredVolumeSize beyond logging the error. If we log the error in the function the return isn't necessary. We could pass in ctx to the func and have access to the same logger 📝 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would eliminate the I was trying to keep the pulled out function simple, which also makes the test less complicated. It seemed better to just return the string in that case. 🤷♂️ |
||
cluster, volumeType, instanceSpecName, volumeRequestSize, | ||
); err != nil { | ||
log.Error(err, "For "+cluster.Name+"/"+instanceSpecName+ | ||
": Unable to parse "+volumeType+" volume request: "+dpv) | ||
} | ||
|
||
// If the volume request size is greater than or equal to the limit and the | ||
// limit is not zero, update the request size to the limit value. | ||
// If the user manually requests a lower limit that is smaller than the current | ||
// or requested volume size, it will be ignored in favor of the limit value. | ||
if volumeRequestSize.Value() >= volumeLimitFromSpec.Value() { | ||
|
||
r.Recorder.Eventf(cluster, corev1.EventTypeNormal, "VolumeLimitReached", | ||
"%s volume(s) for %s/%s are at size limit (%v).", volumeType, | ||
cluster.Name, instanceSpecName, volumeLimitFromSpec) | ||
|
||
// If the volume size request is greater than the limit, issue an | ||
// additional event warning. | ||
if volumeRequestSize.Value() > volumeLimitFromSpec.Value() { | ||
r.Recorder.Eventf(cluster, corev1.EventTypeWarning, "DesiredVolumeAboveLimit", | ||
"The desired size (%v) for the %s/%s %s volume(s) is greater than the size limit (%v).", | ||
volumeRequestSize, cluster.Name, instanceSpecName, volumeType, volumeLimitFromSpec) | ||
} | ||
|
||
volumeRequestSize = volumeLimitFromSpec | ||
} | ||
pvc.Spec.Resources.Requests = corev1.ResourceList{ | ||
corev1.ResourceStorage: *resource.NewQuantity(volumeRequestSize.Value(), resource.BinarySI), | ||
} | ||
} | ||
} | ||
|
||
// getDesiredVolumeSize compares the volume request size to the suggested autogrow | ||
// size stored in the status and updates the value when the status value is larger. | ||
func getDesiredVolumeSize(cluster *v1beta1.PostgresCluster, | ||
volumeType, instanceSpecName string, | ||
volumeRequestSize *resource.Quantity) (string, error) { | ||
|
||
switch volumeType { | ||
case "pgData": | ||
for i := range cluster.Status.InstanceSets { | ||
if instanceSpecName == cluster.Status.InstanceSets[i].Name { | ||
for _, dpv := range cluster.Status.InstanceSets[i].DesiredPGDataVolume { | ||
if dpv != "" { | ||
desiredRequest, err := resource.ParseQuantity(dpv) | ||
if err == nil { | ||
if desiredRequest.Value() > volumeRequestSize.Value() { | ||
*volumeRequestSize = desiredRequest | ||
} | ||
} else { | ||
return dpv, err | ||
} | ||
} | ||
} | ||
} | ||
} | ||
// TODO: Add cases for pgWAL and repo volumes (requires relevant status sections) | ||
} | ||
return "", nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we passed around
instance
instead ofinstanceSetName
would that mean we didn't need to loop over each instance ❓ 💭There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i wonder if we could make this even more generic. We would use this same function for a pgbackrest repo 🤔 and the repo wouldn't be tied to an instance... right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly most of these functions pass around some reference to the instance. Does that break when we start thinking about pgbackrest repo ❓
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is ultimately being triggered by
observeInstances
, so we don't really have a good way to access the instance object itself if I remember correctly.We'll need to know the volume type, etc so that we can drill down to the correct portion of the cluster spec, so I don't know if we can simplify this much further.
I think a lot of the logic around ensuring the various instance volumes grow in tandem won't be needed for the repo volume work. There can be more than one instance set and each instance set can have more than one replica. All the volumes for a given instance should grow the same, but each instance set can be different (and have different limits). Repo volumes on the other hand should be (assuming I'm not forgetting anything) 1:1 per repo definition. All that to say, I don't know that it will be quite as complicated even though it might require more code.
For this particular function, I was expecting to just pass in an entry string for the
instanceSetName
and key off of thevolumeType
where that will berepo1
,repo2
, etc. That said, we'll have to see how things work out. It might make sense to do things a different way, but I can't say for certain at this point.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if we just passed in a VolumeClaimSpec 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really following. We'd have to associate the VolumeClaimSpec with the appropriate instance name otherwise we can't associate it with a request.