Skip to content

Commit f1bc082

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1448 from xing-yang/cleanup
Allow provisioning to proceed to prevent leaking resources
2 parents b84b08f + 62c2b5d commit f1bc082

File tree

2 files changed

+167
-25
lines changed

2 files changed

+167
-25
lines changed

‎pkg/controller/controller.go‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,8 @@ const (
150150

151151
snapshotNotBound = "snapshot %s not bound"
152152

153-
pvcCloneFinalizer = "provisioner.storage.kubernetes.io/cloning-protection"
153+
pvcCloneFinalizer = "provisioner.storage.kubernetes.io/cloning-protection"
154+
snapshotSourceProtectionFinalizer = "snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection"
154155

155156
annAllowVolumeModeChange = "snapshot.storage.kubernetes.io/allow-volume-mode-change"
156157
)
@@ -1161,7 +1162,15 @@ func (p *csiProvisioner) getSnapshotSource(ctx context.Context, claim *v1.Persis
11611162
}
11621163

11631164
if snapshotObj.ObjectMeta.DeletionTimestamp != nil {
1164-
return nil, fmt.Errorf("snapshot %s is currently being deleted", dataSource.Name)
1165+
// VolumeSnapshot is being deleted. Check if provisioning already started by looking for
1166+
// the specific finalizer added by external-snapshotter when a snapshot is used as a data source.
1167+
// If the finalizer exists, it means provisioning was started before deletion began,
1168+
// so we should continue to prevent resource leaks.
1169+
// If the finalizer doesn't exist, this is a new provisioning attempt and should be rejected.
1170+
if !checkFinalizer(snapshotObj, snapshotSourceProtectionFinalizer) {
1171+
return nil, fmt.Errorf("snapshot %s is being deleted", dataSource.Name)
1172+
}
1173+
klog.V(3).Infof("Snapshot %s/%s is being deleted but has volumesnapshot-as-source-protection finalizer, allowing provisioning to continue", dataSource.Namespace, dataSource.Name)
11651174
}
11661175
klog.V(5).Infof("VolumeSnapshot %+v", snapshotObj)
11671176

‎pkg/controller/controller_test.go‎

Lines changed: 156 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2946,29 +2946,32 @@ func TestProvisionFromSnapshot(t *testing.T) {
29462946
}
29472947

29482948
type testcase struct {
2949-
volOpts controller.ProvisionOptions
2950-
restoredVolSizeSmall bool
2951-
wrongDataSource bool
2952-
snapshotStatusReady bool
2953-
expectedPVSpec *pvSpec
2954-
expectErr bool
2955-
expectCSICall bool
2956-
notPopulated bool
2957-
misBoundSnapshotContentUID bool
2958-
misBoundSnapshotContentNamespace bool
2959-
misBoundSnapshotContentName bool
2960-
nilBoundVolumeSnapshotContentName bool
2961-
nilSnapshotStatus bool
2962-
nilReadyToUse bool
2963-
nilContentStatus bool
2964-
nilSnapshotHandle bool
2965-
allowVolumeModeChange bool
2966-
xnsEnabled bool // set to use CrossNamespaceVolumeDataSource feature, default false
2967-
snapNamespace string
2968-
withreferenceGrants bool // set to use ReferenceGrant, default false
2969-
refGrantsrcNamespace string
2970-
referenceGrantFrom []gatewayv1beta1.ReferenceGrantFrom
2971-
referenceGrantTo []gatewayv1beta1.ReferenceGrantTo
2949+
volOpts controller.ProvisionOptions
2950+
restoredVolSizeSmall bool
2951+
wrongDataSource bool
2952+
snapshotStatusReady bool
2953+
expectedPVSpec *pvSpec
2954+
expectErr bool
2955+
expectCSICall bool
2956+
notPopulated bool
2957+
misBoundSnapshotContentUID bool
2958+
misBoundSnapshotContentNamespace bool
2959+
misBoundSnapshotContentName bool
2960+
nilBoundVolumeSnapshotContentName bool
2961+
nilSnapshotStatus bool
2962+
nilReadyToUse bool
2963+
nilContentStatus bool
2964+
nilSnapshotHandle bool
2965+
allowVolumeModeChange bool
2966+
snapshotBeingDeleted bool // set DeletionTimestamp on snapshot
2967+
snapshotWithoutSourceProtectionFinalizer bool // simulate snapshot being deleted without the volumesnapshot-as-source-protection finalizer
2968+
snapshotWithDifferentFinalizer bool // simulate snapshot with a different finalizer (not the source protection one)
2969+
xnsEnabled bool // set to use CrossNamespaceVolumeDataSource feature, default false
2970+
snapNamespace string
2971+
withreferenceGrants bool // set to use ReferenceGrant, default false
2972+
refGrantsrcNamespace string
2973+
referenceGrantFrom []gatewayv1beta1.ReferenceGrantFrom
2974+
referenceGrantTo []gatewayv1beta1.ReferenceGrantTo
29722975
}
29732976
testcases := map[string]testcase{
29742977
"provision with volume snapshot data source": {
@@ -4486,6 +4489,123 @@ func TestProvisionFromSnapshot(t *testing.T) {
44864489
referenceGrantFrom: []gatewayv1beta1.ReferenceGrantFrom{},
44874490
referenceGrantTo: []gatewayv1beta1.ReferenceGrantTo{},
44884491
},
4492+
"provision with snapshot being deleted without source-protection-finalizer should fail": {
4493+
volOpts: controller.ProvisionOptions{
4494+
StorageClass: &storagev1.StorageClass{
4495+
ReclaimPolicy: &deletePolicy,
4496+
Parameters: map[string]string{},
4497+
Provisioner: "test-driver",
4498+
},
4499+
PVName: "test-name",
4500+
PVC: &v1.PersistentVolumeClaim{
4501+
ObjectMeta: metav1.ObjectMeta{
4502+
UID: "testid",
4503+
Annotations: driverNameAnnotation,
4504+
},
4505+
Spec: v1.PersistentVolumeClaimSpec{
4506+
StorageClassName: &snapClassName,
4507+
Resources: v1.VolumeResourceRequirements{
4508+
Requests: v1.ResourceList{
4509+
v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)),
4510+
},
4511+
},
4512+
AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce},
4513+
DataSource: &v1.TypedLocalObjectReference{
4514+
Name: snapName,
4515+
Kind: "VolumeSnapshot",
4516+
APIGroup: &apiGrp,
4517+
},
4518+
},
4519+
},
4520+
},
4521+
snapshotStatusReady: true,
4522+
snapshotBeingDeleted: true,
4523+
snapshotWithoutSourceProtectionFinalizer: true,
4524+
expectErr: true,
4525+
},
4526+
"provision with snapshot being deleted with different finalizer (not source-protection) should fail": {
4527+
volOpts: controller.ProvisionOptions{
4528+
StorageClass: &storagev1.StorageClass{
4529+
ReclaimPolicy: &deletePolicy,
4530+
Parameters: map[string]string{},
4531+
Provisioner: "test-driver",
4532+
},
4533+
PVName: "test-name",
4534+
PVC: &v1.PersistentVolumeClaim{
4535+
ObjectMeta: metav1.ObjectMeta{
4536+
UID: "testid",
4537+
Annotations: driverNameAnnotation,
4538+
},
4539+
Spec: v1.PersistentVolumeClaimSpec{
4540+
StorageClassName: &snapClassName,
4541+
Resources: v1.VolumeResourceRequirements{
4542+
Requests: v1.ResourceList{
4543+
v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)),
4544+
},
4545+
},
4546+
AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce},
4547+
DataSource: &v1.TypedLocalObjectReference{
4548+
Name: snapName,
4549+
Kind: "VolumeSnapshot",
4550+
APIGroup: &apiGrp,
4551+
},
4552+
},
4553+
},
4554+
},
4555+
snapshotStatusReady: true,
4556+
snapshotBeingDeleted: true,
4557+
snapshotWithDifferentFinalizer: true,
4558+
expectErr: true,
4559+
},
4560+
"provision with snapshot being deleted with source-protection-finalizer should proceed for cleanup": {
4561+
volOpts: controller.ProvisionOptions{
4562+
StorageClass: &storagev1.StorageClass{
4563+
ReclaimPolicy: &deletePolicy,
4564+
Parameters: map[string]string{},
4565+
Provisioner: "test-driver",
4566+
},
4567+
PVName: "test-name",
4568+
PVC: &v1.PersistentVolumeClaim{
4569+
ObjectMeta: metav1.ObjectMeta{
4570+
UID: "testid",
4571+
Annotations: driverNameAnnotation,
4572+
},
4573+
Spec: v1.PersistentVolumeClaimSpec{
4574+
StorageClassName: &snapClassName,
4575+
Resources: v1.VolumeResourceRequirements{
4576+
Requests: v1.ResourceList{
4577+
v1.ResourceName(v1.ResourceStorage): resource.MustParse(strconv.FormatInt(requestedBytes, 10)),
4578+
},
4579+
},
4580+
AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce},
4581+
DataSource: &v1.TypedLocalObjectReference{
4582+
Name: snapName,
4583+
Kind: "VolumeSnapshot",
4584+
APIGroup: &apiGrp,
4585+
},
4586+
},
4587+
},
4588+
},
4589+
snapshotStatusReady: true,
4590+
snapshotBeingDeleted: true,
4591+
expectedPVSpec: &pvSpec{
4592+
Name: "test-testi",
4593+
ReclaimPolicy: v1.PersistentVolumeReclaimDelete,
4594+
AccessModes: []v1.PersistentVolumeAccessMode{v1.ReadWriteOnce},
4595+
Capacity: v1.ResourceList{
4596+
v1.ResourceName(v1.ResourceStorage): bytesToQuantity(requestedBytes),
4597+
},
4598+
CSIPVS: &v1.CSIPersistentVolumeSource{
4599+
Driver: "test-driver",
4600+
VolumeHandle: "test-volume-id",
4601+
FSType: "ext4",
4602+
VolumeAttributes: map[string]string{
4603+
"storage.kubernetes.io/csiProvisionerIdentity": "test-provisioner",
4604+
},
4605+
},
4606+
},
4607+
expectCSICall: true,
4608+
},
44894609
}
44904610

44914611
tmpdir := tempDir(t)
@@ -4516,6 +4636,19 @@ func TestProvisionFromSnapshot(t *testing.T) {
45164636
if tc.nilReadyToUse {
45174637
snap.Status.ReadyToUse = nil
45184638
}
4639+
if tc.snapshotBeingDeleted {
4640+
deletionTime := metav1.Now()
4641+
snap.ObjectMeta.DeletionTimestamp = &deletionTime
4642+
// Set up finalizers based on what the test is checking:
4643+
// - snapshotWithDifferentFinalizer: add a different finalizer to verify we check for the specific source-protection one
4644+
// - snapshotWithoutSourceProtectionFinalizer: don't add any finalizers (simulates new provisioning attempt)
4645+
// - default: add the volumesnapshot-as-source-protection finalizer (simulates in-flight provisioning)
4646+
if tc.snapshotWithDifferentFinalizer {
4647+
snap.ObjectMeta.Finalizers = []string{"some-other-finalizer"}
4648+
} else if !tc.snapshotWithoutSourceProtectionFinalizer {
4649+
snap.ObjectMeta.Finalizers = []string{"snapshot.storage.kubernetes.io/volumesnapshot-as-source-protection"}
4650+
}
4651+
}
45194652
return true, snap, nil
45204653
})
45214654

0 commit comments

Comments
 (0)