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.

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

rbd: add ParentInTrash parameter in rbdImage struct #4522

Merged
merged 2 commits into from
Apr 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
37 changes: 36 additions & 1 deletion e2e/rbd.go
Original file line number Diff line number Diff line change
Expand Up @@ -3553,7 +3553,7 @@ var _ = Describe("RBD", func() {
validateRBDImageCount(f, 1, defaultRBDPool)
validateOmapCount(f, 1, rbdType, defaultRBDPool, volumesType)
for i := 0; i < snapChainDepth; i++ {
var pvcClone *v1.PersistentVolumeClaim
var pvcClone, smartClonePVC *v1.PersistentVolumeClaim
snap := getSnapshot(snapshotPath)
snap.Name = fmt.Sprintf("%s-%d", snap.Name, i)
snap.Namespace = f.UniqueName
Expand Down Expand Up @@ -3610,6 +3610,41 @@ var _ = Describe("RBD", func() {
validateOmapCount(f, 1, rbdType, defaultRBDPool, volumesType)
validateOmapCount(f, 0, rbdType, defaultRBDPool, snapsType)

// create pvc-pvc clone to validate pvc-pvc clone creation
// of child PVC created from a snapshot which no longer exits.
// Snapshot-> restore PVC -> delete Snapshot -> PVC-PVC clone.
smartClonePVC, err = loadPVC(pvcSmartClonePath)
if err != nil {
framework.Failf("failed to load smart clone PVC: %v", err)
}

smartClonePVC.Name = fmt.Sprintf("%s-%d", smartClonePVC.Name, i)
smartClonePVC.Namespace = f.UniqueName
smartClonePVC.Spec.DataSource.Name = pvcClone.Name
err = createPVCAndvalidatePV(f.ClientSet, smartClonePVC, deployTimeout)
if err != nil {
framework.Failf("failed to create smart clone PVC %q: %v",
smartClonePVC.Name, err)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

validate the omap

// validate created backend rbd images = clone + smart clone + temp image
totalImages = 3
validateRBDImageCount(f, totalImages, defaultRBDPool)
validateOmapCount(f, 2, rbdType, defaultRBDPool, volumesType)
validateOmapCount(f, 0, rbdType, defaultRBDPool, snapsType)

err = deletePVCAndValidatePV(f.ClientSet, smartClonePVC, deployTimeout)
if err != nil {
framework.Failf("failed to delete smart clone PVC %q: %v",
smartClonePVC.Name, err)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

validate the omap after deletion as well

// validate created backend rbd images = clone
totalImages = 1
validateRBDImageCount(f, totalImages, defaultRBDPool)
validateOmapCount(f, 1, rbdType, defaultRBDPool, volumesType)
validateOmapCount(f, 0, rbdType, defaultRBDPool, snapsType)

app.Spec.Volumes[0].PersistentVolumeClaim.ClaimName = pvcClone.Name
// create application
err = createApp(f.ClientSet, app, deployTimeout)
Expand Down
7 changes: 6 additions & 1 deletion internal/rbd/rbd_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,8 @@ type rbdImage struct {

// Set metadata on volume
EnableMetadata bool
// ParentInTrash indicates the parent image is in trash.
ParentInTrash bool
Copy link
Member

Choose a reason for hiding this comment

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

why move it around? grouping all parent related attributes looks nicer to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why move it around? grouping all parent related attributes looks nicer to me

A linter throws up error when i do that and expects grouping similar data types together for better padding in the struct.

}

// rbdVolume represents a CSI volume and its RBD image specifics.
Expand Down Expand Up @@ -1615,6 +1617,7 @@ func (ri *rbdImage) getImageInfo() error {
} else {
ri.ParentName = parentInfo.Image.ImageName
ri.ParentPool = parentInfo.Image.PoolName
ri.ParentInTrash = parentInfo.Image.Trash
}
// Get image creation time
tm, err := image.GetCreateTimestamp()
Expand All @@ -1633,7 +1636,9 @@ func (ri *rbdImage) getParent() (*rbdImage, error) {
if err != nil {
return nil, err
}
if ri.ParentName == "" {
// The image may not have a parent or the parent maybe in trash.
// Return nil in both the cases.
if ri.ParentName == "" || ri.ParentInTrash {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like we are going to get ParentName even it its in trash, pleas add a comment here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that looks weird. An image in trash does not have a name, I think?

So, does this check actually make a difference? If a parent image does not have a name, it will be set to "" anyway, and the new ParentInTrash is not needed.

Copy link
Contributor Author

@Rakshith-R Rakshith-R Apr 2, 2024

Choose a reason for hiding this comment

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

It does have name.
We just can't refer to it by its name when it is in trash.
The failed ci that I linked in the comment above proves it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thats strange, can you please add a comment about it.

return nil, nil
}

Expand Down