📝 This switch statement will likely be very similar to the one used by limitIsSet. The difference is that this function references status objects that don't yet exist whereas limitIsSet required nothing new outside of itself, so it made sense to move forward a bit there but not here.

Would these be accurate descriptions of these variables? Not necessarily a suggestion... just checking my understanding

Yeah, that's right. The reason it's referred to as a 'backup' here is that that would be how it would be used. In case there's a parsing issue, etc, this value gives us something to roll back to.

why do we need something to roll back to? wouldn't we typically bail if we hit an issue then assume we the change would pass on the next reconcile?

It is also used for comparison purposes by the function, so it's doing a couple of different things. Part of what's happening is we're going from a string to a parsed value, so that has the potential to cause issues. In terms of naming, really 'previous' or 'backup' are both accurate. I guess the difference is a backup is always previous but previous isn't always a backup? Maybe? 🤷‍♂️

if we passed around instance instead of instanceSetName would that mean we didn't need to loop over each instance ❓ 💭

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?

Honestly most of these functions pass around some reference to the instance. Does that break when we start thinking about pgbackrest repo ❓

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 the volumeType where that will be repo1, 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.

what if we just passed in a VolumeClaimSpec 🤔

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.

would this be in the autgrow file 🤔

That's used by config.go in the postgres package. If we moved it to the autogrow.go file, we'd have to import postgrescluster which I don't think we want to do.

related to https://github.com/CrunchyData/postgres-operator/pull/4239/files#r2274537730

if we passed instance in here instead of name would we have access to everything we need... ❓

I suppose, but we're still getting that from the cluster. I think the idea behind passing just the name is to make sure we're updating the statuses one at a time. storeDesiredRequest really only works with a single instance at a time, which is I think what we want to do here.

what do we think the "P" in dpv means?

I'm also not seeing where dpv is used in the rest of this function besides when we log and error.
am i missing something 🔧

Oh is see, volumeRequestSize is a pointer that is passed in and updated.

it took me a minute to get here - maybe its worth changing something here ♻️

• if we aren't using the return value maybe we don't return it
• or maybe return the value instead of updating the pointer

thinking about the logic in this function

	if volumeLimitFromSpec.IsZero() { return }
    if request > limit { // use limit + warn }
    if autogrow {
        dpv = getDesired // this changes value of request
        if requet >= limit { // use limit + warn }
    }

if limit isn't set, autogrow isn't enabled so do nothing
if request has exceeded the limit - use the limit
if autogrow is enabled we see if the request is different
use getDesiredVolumeSize to override the request
if value has met or exceeded the limit - use the limit

maybe...

	if volumeLimitFromSpec.IsZero() {
		return
	}

    request =  pvc.Spec.Resources.Requests.Storage()
    if feature.Enabled(ctx, feature.AutoGrowVolumes) {
        request = getDesiredVolumeSize()
    }

    if request >= limit {
       // use limit and warn
    }

this is assuming we will always use the "desired" volume size if autogrow is enabled and I might be wrong in that assumption 💭

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

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.

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 📝

It would eliminate the VolumeRequestOverLimit Event if I'm following correctly.

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. 🤷‍♂️