Skip to content

reconcile: wait till PVC is in bound phase #1971

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.

Sign up for GitHub

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

Open
AndrewChubatiuk wants to merge 1 commit into
base: master
Choose a base branch
from
+53 −13
Open

reconcile: wait till PVC is in bound phase #1971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
61 changes: 48 additions & 13 deletions internal/controller/operator/factory/reconcile/pvc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
k8serrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
"sigs.k8s.io/controller-runtime/pkg/client"

"github.com/VictoriaMetrics/operator/internal/controller/operator/factory/logger"
Expand All @@ -20,24 +21,58 @@ import (
// in case of deletion timestamp > 0 does nothing
// user must manually remove finalizer if needed
func PersistentVolumeClaim(ctx context.Context, rclient client.Client, newObj, prevObj *corev1.PersistentVolumeClaim, owner *metav1.OwnerReference) error {
l := logger.WithContext(ctx)
var existingObj corev1.PersistentVolumeClaim
nsn := types.NamespacedName{Namespace: newObj.Namespace, Name: newObj.Name}
if err := rclient.Get(ctx, nsn, &existingObj); err != nil {
if k8serrors.IsNotFound(err) {
l.Info(fmt.Sprintf("creating new PVC=%s", nsn.String()))
if err := rclient.Create(ctx, newObj); err != nil {
return fmt.Errorf("cannot create new PVC=%s: %w", nsn.String(), err)
var existingObj corev1.PersistentVolumeClaim
err := retryOnConflict(func() error {
if err := rclient.Get(ctx, nsn, &existingObj); err != nil {
if k8serrors.IsNotFound(err) {
logger.WithContext(ctx).Info(fmt.Sprintf("creating new PVC=%s", nsn.String()))
return rclient.Create(ctx, newObj)
}
return fmt.Errorf("cannot get existing PVC=%s: %w", nsn.String(), err)
}
if !existingObj.DeletionTimestamp.IsZero() {
logger.WithContext(ctx).Info(fmt.Sprintf("PVC=%s has non zero DeletionTimestamp, skip update."+
" To fix this, make backup for this pvc, delete pvc finalizers and restore from backup.", nsn.String()))
return nil
}
return fmt.Errorf("cannot get existing PVC=%s: %w", nsn.String(), err)
return updatePVC(ctx, rclient, &existingObj, newObj, prevObj, owner)
})
if err != nil {
return err
}
if !existingObj.DeletionTimestamp.IsZero() {
l.Info(fmt.Sprintf("PVC=%s has non zero DeletionTimestamp, skip update."+
" To fix this, make backup for this pvc, delete pvc finalizers and restore from backup.", nsn.String()))
return nil
var generation int64
switch {
case !newObj.CreationTimestamp.IsZero():
generation = newObj.Generation
case !existingObj.CreationTimestamp.IsZero():
generation = existingObj.Generation
}
return waitForPVCBound(ctx, rclient, nsn, generation)
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P1: Waiting for PVCs to reach Bound here can block valid WaitForFirstConsumer claims before their Deployment is created.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/reconcile/pvc.go, line 51:

<comment>Waiting for PVCs to reach `Bound` here can block valid `WaitForFirstConsumer` claims before their Deployment is created.</comment>

<file context>
@@ -20,24 +21,58 @@ import (
+	case !existingObj.CreationTimestamp.IsZero():
+		generation = existingObj.Generation
 	}
+	return waitForPVCBound(ctx, rclient, nsn, generation)
+}
 
</file context>
Fix with Cubic

}

return updatePVC(ctx, rclient, &existingObj, newObj, prevObj, owner)
func waitForPVCBound(ctx context.Context, rclient client.Client, nsn types.NamespacedName, generation int64) error {
Copy link
Collaborator

@vrutkovs vrutkovs Mar 16, 2026

Choose a reason for hiding this comment

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

Lets add unit tests for this function

var pvc corev1.PersistentVolumeClaim
return wait.PollUntilContextTimeout(ctx, pvcWaitBoundIntervalCheck, pvcWaitReadyTimeout, true, func(ctx context.Context) (done bool, err error) {
if err := rclient.Get(ctx, nsn, &pvc); err != nil {
if k8serrors.IsNotFound(err) {
return false, nil
}
return false, fmt.Errorf("cannot get PVC=%s: %w", nsn.String(), err)
}
if !pvc.DeletionTimestamp.IsZero() {
return true, fmt.Errorf("cannot wait for PVC=%s, which is being terminated", nsn.String())
}
if generation > pvc.Generation {
Copy link
Collaborator

@vrutkovs vrutkovs Mar 16, 2026

Choose a reason for hiding this comment

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

Not sure why we need generation here at all - and why would PVC generation match the deployment?

return false, nil
}
switch pvc.Status.Phase {
case corev1.ClaimBound:
return true, nil
case corev1.ClaimPending:
return false, nil
default:
return false, fmt.Errorf("failed to wait for PVC=%s, which in %s phase", nsn.String(), pvc.Status.Phase)
}
})
}
2 changes: 2 additions & 0 deletions internal/controller/operator/factory/reconcile/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ import (
)

var (
pvcWaitBoundIntervalCheck = 1 * time.Second
pvcWaitReadyTimeout = 5 * time.Second
Copy link
Contributor

@cubic-dev-ai cubic-dev-ai bot Mar 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

P1: The new PVC wait uses a hardcoded 5s timeout, so PVC binding can fail much earlier than the operator's configured readiness deadlines.

Prompt for AI agents 
Check if this issue is valid — if so, understand the root cause and fix it. At internal/controller/operator/factory/reconcile/reconcile.go, line 27:

<comment>The new PVC wait uses a hardcoded 5s timeout, so PVC binding can fail much earlier than the operator's configured readiness deadlines.</comment>

<file context>
@@ -23,6 +23,8 @@ import (
 
 var (
+	pvcWaitBoundIntervalCheck = 1 * time.Second
+	pvcWaitReadyTimeout       = 5 * time.Second
 	podWaitReadyIntervalCheck = 50 * time.Millisecond
 	appWaitReadyDeadline      = 5 * time.Second
</file context>
Fix with Cubic

podWaitReadyIntervalCheck = 50 * time.Millisecond
appWaitReadyDeadline = 5 * time.Second
podWaitReadyTimeout = 5 * time.Second
Expand Down
3 changes: 3 additions & 0 deletions internal/controller/operator/factory/reconcile/statefulset_pvc_expand.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,9 @@ func updateSTSPVC(ctx context.Context, rclient client.Client, sts *appsv1.Statef
if err := updatePVC(ctx, rclient, &pvc, &stsClaim, prevVCT, nil); err != nil {
return err
}
if err := waitForPVCBound(ctx, rclient, nsn, pvc.Generation); err != nil {
return err
}
}
return nil
}
Expand Down
Loading