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.

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

Jump to bottom

fix:claim cached in pvcontroller is not the newest may cause unexpected issue #105211

Merged
merged 1 commit into from
Oct 14, 2021
Merged

fix:claim cached in pvcontroller is not the newest may cause unexpected issue #105211

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions pkg/controller/volume/persistentvolume/binder_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,21 @@ func TestSync(t *testing.T) {
noevents, noerrors, testSyncVolume,
},

{
// syncVolume with volume bound to claim which exists in etcd but not updated to local cache.
"4-12 - volume bound to newest claim but not updated to local cache",
newVolumeArray("volume4-12", "10Gi", "uid4-12-new", "claim4-12", v1.VolumeAvailable, v1.PersistentVolumeReclaimDelete, classEmpty),
newVolumeArray("volume4-12", "10Gi", "uid4-12-new", "claim4-12", v1.VolumeBound, v1.PersistentVolumeReclaimDelete, classEmpty),
func() []*v1.PersistentVolumeClaim {
newClaim := newClaimArray("claim4-12", "uid4-12", "10Gi", "volume4-12", v1.ClaimBound, nil, "")
// update uid to new-uid and not sync to cache.
newClaim = append(newClaim, newClaimArray("claim4-12", "uid4-12-new", "10Gi", "volume4-12", v1.ClaimBound, nil, annSkipLocalStore)...)
return newClaim
}(),
newClaimArray("claim4-12", "uid4-12-new", "10Gi", "volume4-12", v1.ClaimBound, nil, annSkipLocalStore),
noevents, noerrors, testSyncVolume,
},

// PVC with class
{
// syncVolume binds a claim to requested class even if there is a
Expand Down
19 changes: 16 additions & 3 deletions pkg/controller/volume/persistentvolume/pv_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,9 +622,22 @@ func (ctrl *PersistentVolumeController) syncVolume(volume *v1.PersistentVolume)
if claim != nil && claim.UID != volume.Spec.ClaimRef.UID {
// The claim that the PV was pointing to was deleted, and another
// with the same name created.
klog.V(4).Infof("synchronizing PersistentVolume[%s]: claim %s has different UID, the old one must have been deleted", volume.Name, claimrefToClaimKey(volume.Spec.ClaimRef))
// Treat the volume as bound to a missing claim.
claim = nil
// in some cases, the cached claim is not the newest, and the volume.Spec.ClaimRef.UID is newer than cached.
jsafrane marked this conversation as resolved.
Show resolved Hide resolved
// so we should double check by calling apiserver and get the newest claim, then compare them.
klog.V(4).Infof("Maybe cached claim: %s is not the newest one, we should fetch it from apiserver", claimrefToClaimKey(volume.Spec.ClaimRef))

claim, err = ctrl.kubeClient.CoreV1().PersistentVolumeClaims(volume.Spec.ClaimRef.Namespace).Get(context.TODO(), volume.Spec.ClaimRef.Name, metav1.GetOptions{})
if err != nil && !apierrors.IsNotFound(err) {
return err
} else if claim != nil {
// Treat the volume as bound to a missing claim.
if claim.UID != volume.Spec.ClaimRef.UID {
klog.V(4).Infof("synchronizing PersistentVolume[%s]: claim %s has a newer UID than pv.ClaimRef, the old one must have been deleted", volume.Name, claimrefToClaimKey(volume.Spec.ClaimRef))
claim = nil
} else {
klog.V(4).Infof("synchronizing PersistentVolume[%s]: claim %s has a same UID with pv.ClaimRef", volume.Name, claimrefToClaimKey(volume.Spec.ClaimRef))
}
Copy link
Member

Choose a reason for hiding this comment

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

There should be a log that the newly retrieved claim is newer.

Copy link
Author

Choose a reason for hiding this comment

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

updated.

Copy link
Member

Choose a reason for hiding this comment

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

I meant, now the controller logs

Maybe cached claim: %s is not the newest one, we should fetch it from apiserver

If the retrieved PVC is wrong, it then logs:

synchronizing PersistentVolume[%s]: claim %s has a newer UID than pv.ClaimRef, the old one must have been deleted

This is fine.

But if the retrieved PVC is OK, there is no log, which leaves potential log reader with confusing "we should fetch it from apiserver", but no result of the fetch. Can you add else and some log that the fetched PVC is OK?

Copy link
Author

Choose a reason for hiding this comment

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

thanks, updated

}
}

if claim == nil {
Expand Down