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

Set PVC name on VolumeGroupSnapshot members #1177

Merged
merged 1 commit into from
Nov 7, 2024
Merged

Set PVC name on VolumeGroupSnapshot members #1177

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
38 changes: 36 additions & 2 deletions pkg/common-controller/groupsnapshot_controller_helper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,7 +609,7 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
},
Spec: crdv1.VolumeSnapshotSpec{
Source: crdv1.VolumeSnapshotSource{
VolumeSnapshotContentName: &volumeSnapshotContentName,
PersistentVolumeClaimName: &pv.Spec.ClaimRef.Name,
},
},
// The status will be set by VolumeSnapshot reconciler
Expand All @@ -621,7 +621,7 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
"createSnapshotsForGroupSnapshotContent: creating volumesnapshotcontent %w", err)
}

_, err = ctrl.clientset.SnapshotV1().VolumeSnapshots(volumeSnapshotNamespace).Create(ctx, volumeSnapshot, metav1.CreateOptions{})
createdVolumeSnapshot, err := ctrl.clientset.SnapshotV1().VolumeSnapshots(volumeSnapshotNamespace).Create(ctx, volumeSnapshot, metav1.CreateOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, the VolumeSnapshotContents already exist. Can you set status.boundVolumeSnapshotContentName here?
(I don't remember if it's possible to create instances with status directly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would have loved to do that.

Unfortunately it is not possible. That's why I'm forced to hit the API server two times.

if err != nil && !apierrs.IsAlreadyExists(err) {
return groupSnapshotContent, fmt.Errorf(
"createSnapshotsForGroupSnapshotContent: creating volumesnapshot %w", err)
Expand All @@ -637,6 +637,40 @@ func (ctrl *csiSnapshotCommonController) createSnapshotsForGroupSnapshotContent(
Name: pv.Name,
}
}

// bind the volume snapshot content to the volume snapshot
// like a dynamically provisioned snapshot would do
volumeSnapshotContent.Spec.VolumeSnapshotRef.UID = createdVolumeSnapshot.UID
_, err = utils.PatchVolumeSnapshotContent(volumeSnapshotContent, []utils.PatchOp{
{
Op: "replace",
Path: "/spec/volumeSnapshotRef/uid",
Value: volumeSnapshotContent.Spec.VolumeSnapshotRef.UID,
},
}, ctrl.clientset)
if err != nil {
return groupSnapshotContent, fmt.Errorf(
"createSnapshotsForGroupSnapshotContent: binding volumesnapshotcontent to volumesnapshot %w", err)
}

// bind the volume snapshot to the volume snapshot content
// like a dynamically provisioned snapshot would do
_, err = utils.PatchVolumeSnapshot(createdVolumeSnapshot, []utils.PatchOp{
{
Op: "replace",
Path: "/status",
Value: &crdv1.VolumeSnapshotStatus{},
},
{
Op: "replace",
Path: "/status/boundVolumeSnapshotContentName",
Value: volumeSnapshotContentName,
},
}, ctrl.clientset, "status")
if err != nil {
return groupSnapshotContent, fmt.Errorf(
"createSnapshotsForGroupSnapshotContent: binding volumesnapshot to volumesnapshotcontent %w", err)
}
}

// Phase 2: set the backlinks
Expand Down
40 changes: 40 additions & 0 deletions pkg/common-controller/snapshot_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,40 @@ func (ctrl *csiSnapshotCommonController) syncUnreadySnapshot(snapshot *crdv1.Vol
return nil
}

// member of a dynamically provisioned volume group snapshot
if _, ok := snapshot.Labels[utils.VolumeGroupSnapshotNameLabel]; ok {
if snapshot.Status == nil || snapshot.Status.BoundVolumeSnapshotContentName == nil {
klog.V(5).Infof(
"syncUnreadySnapshot [%s]: detected group snapshot member with no content, retrying",
utils.SnapshotKey(snapshot))
return fmt.Errorf("detected group snapshot member %s with no content, retrying",
utils.SnapshotKey(snapshot))
}

volumeSnapshotContentName := *snapshot.Status.BoundVolumeSnapshotContentName

content, err := ctrl.getContentFromStore(volumeSnapshotContentName)
if err != nil {
return err
}
if content == nil {
// can not find the desired VolumeSnapshotContent from cache store
// we'll retry
return fmt.Errorf("group snapshot member %s requests an non-existing content %s", utils.SnapshotKey(snapshot), volumeSnapshotContentName)
}

// update snapshot status
klog.V(5).Infof("syncUnreadySnapshot [%s]: trying to update group snapshot member status", utils.SnapshotKey(snapshot))
if _, err = ctrl.updateSnapshotStatus(snapshot, content); err != nil {
// update snapshot status failed
klog.V(4).Infof("failed to update group snapshot member %s status: %v", utils.SnapshotKey(snapshot), err)
ctrl.updateSnapshotErrorStatusWithEvent(snapshot, false, v1.EventTypeWarning, "SnapshotStatusUpdateFailed", fmt.Sprintf("Snapshot status update failed, %v", err))
return err
}

return nil
}

// snapshot.Spec.Source.VolumeSnapshotContentName == nil - dynamically creating snapshot
klog.V(5).Infof("getDynamicallyProvisionedContentFromStore for snapshot %s", uniqueSnapshotName)
contentObj, err := ctrl.getDynamicallyProvisionedContentFromStore(snapshot)
Expand Down Expand Up @@ -1390,6 +1424,12 @@ func (ctrl *csiSnapshotCommonController) SetDefaultSnapshotClass(snapshot *crdv1
return nil, snapshot, nil
}

if _, ok := snapshot.Labels[utils.VolumeGroupSnapshotNameLabel]; ok {
// don't return error for volume group snapshot members
klog.V(5).Infof("Don't need to find SnapshotClass for volume group snapshot member [%s]", snapshot.Name)
return nil, snapshot, nil
}

// Find default snapshot class if available
list, err := ctrl.classLister.List(labels.Everything())
if err != nil {
Expand Down