Add PVC expansion logic for MinIO Operator

[fengyinqiao]

Add PVC Expansion Logic for MinIO Operator

Summary

This PR adds functionality to automatically expand PersistentVolumeClaims (PVCs) when the storage capacity specified in the MinIO Tenant Custom Resource Definition (CRD) changes.

Changes

• Added pvc.go to handle PVC expansion logic.
• Updated syncHandler in main-controller.go to call the new PVC expansion function.
• Added unit tests in pvc_test.go to validate PVC expansion functionality.

Testing

1. Created unit tests for PVC expansion logic.
2. Verified that PVCs are correctly expanded when storage requests change.

Documentation

• Added comments in pvc.go to explain the logic.

Checklist

• Followed coding conventions.
• Added tests.
• Ran golint and go vet.
• All CI checks passed.

[fengyinqiao]

@pjuarezd fix #1780

[fengyinqiao]

@harshavardhana @dvaldivia Please review my PR when you are free. Thank you.
fix #1780

[cniackz]

Let me talk to the right people and evaluate

[balamurugana]

This works for DirectPV storage class.

[fengyinqiao]

@balamurugana May I ask if this PR can be merged?

[cniackz]

@jiuker if this will cause STS issue, should we merge?...

[jiuker]

@jiuker if this will cause STS issue, should we merge?...

tenant => generate statuefulset => update the statuefulset storage, will update failed, non-modifiable fields / if use patch, pod will restart. @cniackz
A quick test steps are update the tenant.yaml stroage,
You will got this

StatefulSet.apps "myminio-pool-0" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden

[fengyinqiao]

@jiuker if this will cause STS issue, should we merge?...

tenant => generate statuefulset => update the statuefulset storage, will update failed, non-modifiable fields / if use patch, pod will restart. @cniackz A quick test steps are update the tenant.yaml stroage, You will got this

StatefulSet.apps "myminio-pool-0" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden

@cniackz , your question has nothing to do with this PR. This PR will only update PVC according to CR without updating the STS.

[jiuker]

@jiuker if this will cause STS issue, should we merge?...

tenant => generate statuefulset => update the statuefulset storage, will update failed, non-modifiable fields / if use patch, pod will restart. @cniackz A quick test steps are update the tenant.yaml stroage, You will got this

StatefulSet.apps "myminio-pool-0" is invalid: spec: Forbidden: updates to statefulset spec for fields other than 'replicas', 'ordinals', 'template', 'updateStrategy', 'persistentVolumeClaimRetentionPolicy' and 'minReadySeconds' are forbidden

@cniackz , your question has nothing to do with this PR. This PR will only update PVC according to CR without updating the STS.

I think this is relevant, this is not a complete pr, if only expand pvc, without considering the statefulset update error, I think it is inappropriate.
Tenant will generate the statefulset and then compare latest and before, if that have diff, will do a update. @fengyinqiao
And statefulset have template fields point the storage.
I don't see any code to resolve this.

operator/pkg/controller/main-controller.go

Lines 1284 to 1312 in 9d45aba

	poolMatchesSS, err := poolSSMatchesSpec(expectedStatefulSet, existingStatefulSet)
	if err != nil {
	return WrapResult(Result{}, err)
	}
	// if the pool doesn't match the spec
	if !poolMatchesSS {
	// for legacy reasons, if the zone label is present in SS we must carry it over
	carryOverLabels := make(map[string]string)
	if val, ok := existingStatefulSet.Spec.Template.ObjectMeta.Labels[miniov1.ZoneLabel]; ok {
	carryOverLabels[miniov1.ZoneLabel] = val
	}
	newStatefulSet := existingStatefulSet.DeepCopy()
	newStatefulSet.Spec.Template = expectedStatefulSet.Spec.Template
	newStatefulSet.Spec.UpdateStrategy = expectedStatefulSet.Spec.UpdateStrategy
	if existingStatefulSet.Spec.Template.ObjectMeta.Labels == nil {
	newStatefulSet.Spec.Template.ObjectMeta.Labels = make(map[string]string)
	}
	for k, v := range carryOverLabels {
	newStatefulSet.Spec.Template.ObjectMeta.Labels[k] = v
	}
	if existingStatefulSet, err = c.kubeClientSet.AppsV1().StatefulSets(tenant.Namespace).Update(ctx, newStatefulSet, uOpts); err != nil {
	klog.Errorf("[Will try again in 5sec] Update tenant %s statefulset %s error %s", tenant.Name, ssName, err)
	return WrapResult(Result{RequeueAfter: time.Second * 5}, nil)
	}
	}

[fengyinqiao]

And statefulset have template fields point the storage.

@jiuker It is in sts.spec.volumeClaimtemplates field, not in the sts.spec.template field, The former will not be updated, so there is no problem. You can test it.

[cniackz]

If necessary, we can revert or fix it later on, as I don't see any issues on my end.
@jiuker, thank you for the details. Please allow users to thoroughly test all possible scenarios, and only make a fix if there are concerns on your end; otherwise, we can leave it as is and wait for further input.

[jiuker]

If necessary, we can revert or fix it later on, as I don't see any issues on my end. @jiuker, thank you for the details. Please allow users to thoroughly test all possible scenarios, and only make a fix if there are concerns on your end; otherwise, we can leave it as is and wait for further input.

@cesnietor @fengyinqiao Checked. There have an case must be fixed. Resize pvc and then update the image.
It will have the error that I said.

[fengyinqiao]

If necessary, we can revert or fix it later on, as I don't see any issues on my end. @jiuker, thank you for the details. Please allow users to thoroughly test all possible scenarios, and only make a fix if there are concerns on your end; otherwise, we can leave it as is and wait for further input.

@cesnietor @fengyinqiao Checked. There have an case must be fixed. Resize pvc and then update the image. It will have the error that I said.

https://github.com/minio/operator/blob/9d45aba5ecc5c8c08f047db4bd2a411bd06b37e2/pkg/controller/main-controller.go#L1217C3-L1232C4

@jiuker Is this logic causing the problem? If that's the case, I think this logic needs to be restructured. It's not possible to update all fields of STS completely. Instead, like this logic, we should selectively update some fields of STS：

https://github.com/minio/operator/blob/9d45aba5ecc5c8c08f047db4bd2a411bd06b37e2/pkg/controller/main-controller.go#L1290C4-L1311C5

Similarly, when you update the tenant.spec.podManagementPolicy and then update the tenant.spec.image, it will also trigger this problem(because this will cause sts.spec.podManagementPolicy to be updated, but this field of STS is prohibited from being updated)，not only the storage field.

[jiuker]

If necessary, we can revert or fix it later on, as I don't see any issues on my end. @jiuker, thank you for the details. Please allow users to thoroughly test all possible scenarios, and only make a fix if there are concerns on your end; otherwise, we can leave it as is and wait for further input.

@cesnietor @fengyinqiao Checked. There have an case must be fixed. Resize pvc and then update the image. It will have the error that I said.

https://github.com/minio/operator/blob/9d45aba5ecc5c8c08f047db4bd2a411bd06b37e2/pkg/controller/main-controller.go#L1217C3-L1232C4

@jiuker Is this logic causing the problem? If that's the case, I think this logic needs to be restructured. It's not possible to update all fields of STS completely. Instead, like this logic, we should selectively update some fields of STS：

https://github.com/minio/operator/blob/9d45aba5ecc5c8c08f047db4bd2a411bd06b37e2/pkg/controller/main-controller.go#L1290C4-L1311C5

Similarly, when you update the tenant.spec.podManagementPolicy and then update the tenant.spec.image, it will also trigger this problem(because this will cause sts.spec.podManagementPolicy to be updated, but this field of STS is prohibited from being updated)，not only the storage field.

Yesh.

[fengyinqiao]

This below snippet needs to be fixed, but this fix has nothing to do with this PR, even without this PR, there is a problem, for it's not possible to update all fields of STS completely.

@cniackz @jiuker If I have time, I will propose a new PR to fix it, but to be clear, this is a known bug, not caused by this PR.

https://github.com/minio/operator/blob/9d45aba5ecc5c8c08f047db4bd2a411bd06b37e2/pkg/controller/main-controller.go#L1217C3-L1232C4

[jiuker]

This below snippet needs to be fixed, but this fix has nothing to do with this PR, even without this PR, there is a problem, for it's not possible to update all fields of STS completely.

@cniackz @jiuker If I have time, I will propose a new PR to fix it, but to be clear, this is a known bug, not caused by this PR.

https://github.com/minio/operator/blob/9d45aba5ecc5c8c08f047db4bd2a411bd06b37e2/pkg/controller/main-controller.go#L1217C3-L1232C4

Yesh. We know that. Please send a pr fix this.