Skip to content

Commit f064590

Browse files
akalenyuakalenyu
authored
Expose advised restore size on cron snapshots (#4008)
Some provisioners (trident nfs) have a weird restoresize which may direct users towards using a wrong disk restore size. Expose our own advised size in the annotations. Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
1 parent 13063be commit f064590

3 files changed

Lines changed: 208 additions & 52 deletions

File tree

‎pkg/controller/common/util.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -235,6 +235,8 @@ const (
235235

236236
// AnnSourceVolumeMode is the volume mode of the source PVC specified as an annotation on snapshots
237237
AnnSourceVolumeMode = AnnAPIGroup + "/storage.import.sourceVolumeMode"
238+
// AnnAdvisedRestoreSize is the advised restore size for disks restored from the snapshot
239+
AnnAdvisedRestoreSize = AnnAPIGroup + "/storage.import.advisedRestoreSize"
238240

239241
// AnnOpenShiftImageLookup is the annotation for OpenShift image stream lookup
240242
AnnOpenShiftImageLookup = "alpha.image.policy.openshift.io/resolve-names"

‎pkg/controller/dataimportcron-controller.go‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -434,6 +434,12 @@ func (r *DataImportCronReconciler) update(ctx context.Context, dataImportCron *c
434434
cc.AddAnnotation(snapshot, cc.AnnSourceVolumeMode, string(*volMode))
435435
}
436436
}
437+
if _, ok := snapshot.Annotations[cc.AnnAdvisedRestoreSize]; !ok {
438+
size := inferAdvisedRestoreSizeForSnapshot(&dataImportCron.Spec.Template, snapshot, nil)
439+
if size != nil {
440+
cc.AddAnnotation(snapshot, cc.AnnAdvisedRestoreSize, size.String())
441+
}
442+
}
437443
// Copy labels found on dataSource to the existing snapshot in case of upgrades.
438444
dataSource, err := r.getDataSource(ctx, dataImportCron)
439445
if err != nil {
@@ -1036,6 +1042,11 @@ func (r *DataImportCronReconciler) handleSnapshot(ctx context.Context, dataImpor
10361042
return err
10371043
}
10381044
cc.AddAnnotation(desiredSnapshot, AnnLastUseTime, time.Now().UTC().Format(time.RFC3339Nano))
1045+
pvcSize := pvc.Spec.Resources.Requests[corev1.ResourceStorage]
1046+
size := inferAdvisedRestoreSizeForSnapshot(&dataImportCron.Spec.Template, desiredSnapshot, &pvcSize)
1047+
if size != nil {
1048+
cc.AddAnnotation(desiredSnapshot, cc.AnnAdvisedRestoreSize, size.String())
1049+
}
10391050
if pvc.Spec.VolumeMode != nil {
10401051
cc.AddAnnotation(desiredSnapshot, cc.AnnSourceVolumeMode, string(*pvc.Spec.VolumeMode))
10411052
}
@@ -1865,3 +1876,27 @@ func getAccessModesFromDVSpec(dv *cdiv1.DataVolume) []corev1.PersistentVolumeAcc
18651876

18661877
return nil
18671878
}
1879+
1880+
func inferAdvisedRestoreSizeForSnapshot(dv *cdiv1.DataVolume, snapshot *snapshotv1.VolumeSnapshot, fallback *resource.Quantity) *resource.Quantity {
1881+
var dvSize resource.Quantity
1882+
1883+
if dv.Spec.PVC != nil {
1884+
dvSize = dv.Spec.PVC.Resources.Requests[corev1.ResourceStorage]
1885+
}
1886+
1887+
if dv.Spec.Storage != nil {
1888+
dvSize = dv.Spec.Storage.Resources.Requests[corev1.ResourceStorage]
1889+
}
1890+
1891+
if dvSize.IsZero() && fallback != nil {
1892+
return fallback
1893+
}
1894+
1895+
if snapshot.Status != nil {
1896+
if rs := snapshot.Status.RestoreSize; rs != nil && dvSize.Cmp(*rs) < 0 {
1897+
return snapshot.Status.RestoreSize
1898+
}
1899+
}
1900+
1901+
return &dvSize
1902+
}

‎pkg/controller/dataimportcron-controller_test.go‎

Lines changed: 171 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ import (
3939
storagev1 "k8s.io/api/storage/v1"
4040
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
4141
k8serrors "k8s.io/apimachinery/pkg/api/errors"
42+
"k8s.io/apimachinery/pkg/api/resource"
4243
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4344
"k8s.io/apimachinery/pkg/runtime"
4445
"k8s.io/apimachinery/pkg/types"
@@ -1391,6 +1392,7 @@ var _ = Describe("All DataImportCron Tests", func() {
13911392
err = reconciler.client.Get(context.TODO(), dvKey(dvName), snap)
13921393
Expect(err).ToNot(HaveOccurred())
13931394
Expect(snap.Annotations[cc.AnnSourceVolumeMode]).To(Equal("dummy"))
1395+
Expect(snap.Annotations[cc.AnnAdvisedRestoreSize]).To(Equal("1G"))
13941396
snap.Status = &snapshotv1.VolumeSnapshotStatus{
13951397
ReadyToUse: ptr.To[bool](true),
13961398
}
@@ -1567,65 +1569,110 @@ var _ = Describe("All DataImportCron Tests", func() {
15671569
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
15681570
})
15691571

1570-
It("Should set snapshot source volume mode annotation on carried-over-upgrade snapshot", func() {
1571-
cron = newDataImportCron(cronName)
1572-
dataSource = nil
1573-
retentionPolicy := cdiv1.DataImportCronRetainNone
1574-
cron.Spec.RetentionPolicy = &retentionPolicy
1575-
err := reconciler.client.Create(context.TODO(), cron)
1576-
Expect(err).ToNot(HaveOccurred())
1577-
verifyConditions("Before DesiredDigest is set", false, false, false, noImport, noDigest, "", &snapshotv1.VolumeSnapshot{})
1572+
Context("Convenience snapshot annotations", func() {
1573+
var dvName string
1574+
var pvc *corev1.PersistentVolumeClaim
15781575

1579-
cc.AddAnnotation(cron, AnnSourceDesiredDigest, testDigest)
1580-
err = reconciler.client.Update(context.TODO(), cron)
1581-
Expect(err).ToNot(HaveOccurred())
1582-
dataSource = &cdiv1.DataSource{}
1583-
verifyConditions("After DesiredDigest is set", false, false, false, noImport, outdated, noSource, &snapshotv1.VolumeSnapshot{})
1576+
BeforeEach(func() {
1577+
cron = newDataImportCron(cronName)
1578+
cron.Spec.Template.Spec.Storage = &cdiv1.StorageSpec{
1579+
Resources: corev1.VolumeResourceRequirements{
1580+
Requests: corev1.ResourceList{
1581+
corev1.ResourceStorage: resource.MustParse("1Gi"),
1582+
},
1583+
},
1584+
}
1585+
dataSource = nil
1586+
retentionPolicy := cdiv1.DataImportCronRetainNone
1587+
cron.Spec.RetentionPolicy = &retentionPolicy
1588+
err := reconciler.client.Create(context.TODO(), cron)
1589+
Expect(err).ToNot(HaveOccurred())
1590+
verifyConditions("Before DesiredDigest is set", false, false, false, noImport, noDigest, "", &snapshotv1.VolumeSnapshot{})
15841591

1585-
imports := cron.Status.CurrentImports
1586-
Expect(imports).ToNot(BeNil())
1587-
Expect(imports).ToNot(BeEmpty())
1588-
dvName := imports[0].DataVolumeName
1589-
Expect(dvName).ToNot(BeEmpty())
1590-
digest := imports[0].Digest
1591-
Expect(digest).To(Equal(testDigest))
1592+
cc.AddAnnotation(cron, AnnSourceDesiredDigest, testDigest)
1593+
err = reconciler.client.Update(context.TODO(), cron)
1594+
Expect(err).ToNot(HaveOccurred())
1595+
dataSource = &cdiv1.DataSource{}
1596+
verifyConditions("After DesiredDigest is set", false, false, false, noImport, outdated, noSource, &snapshotv1.VolumeSnapshot{})
1597+
1598+
imports := cron.Status.CurrentImports
1599+
Expect(imports).ToNot(BeNil())
1600+
Expect(imports).ToNot(BeEmpty())
1601+
dvName = imports[0].DataVolumeName
1602+
Expect(dvName).ToNot(BeEmpty())
1603+
digest := imports[0].Digest
1604+
Expect(digest).To(Equal(testDigest))
1605+
1606+
dv := &cdiv1.DataVolume{}
1607+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1608+
Expect(err).ToNot(HaveOccurred())
1609+
Expect(*dv.Spec.Source.Registry.URL).To(Equal(testRegistryURL + "@" + testDigest))
1610+
Expect(dv.Annotations[cc.AnnImmediateBinding]).To(Equal("true"))
15921611

1593-
dv := &cdiv1.DataVolume{}
1594-
err = reconciler.client.Get(context.TODO(), dvKey(dvName), dv)
1595-
Expect(err).ToNot(HaveOccurred())
1596-
Expect(*dv.Spec.Source.Registry.URL).To(Equal(testRegistryURL + "@" + testDigest))
1597-
Expect(dv.Annotations[cc.AnnImmediateBinding]).To(Equal("true"))
1612+
// DV GCed after hitting succeeded
1613+
err = reconciler.client.Delete(context.TODO(), dv)
1614+
Expect(err).ToNot(HaveOccurred())
1615+
pvc = cc.CreatePvc(dv.Name, dv.Namespace, nil, nil)
1616+
})
15981617

1599-
// DV GCed after hitting succeeded
1600-
err = reconciler.client.Delete(context.TODO(), dv)
1601-
Expect(err).ToNot(HaveOccurred())
1602-
pvc := cc.CreatePvc(dv.Name, dv.Namespace, nil, nil)
1603-
// Snap already exists, without the source volume mode annotation
1604-
snap := &snapshotv1.VolumeSnapshot{
1605-
ObjectMeta: metav1.ObjectMeta{
1606-
Name: pvc.Name,
1607-
Namespace: metav1.NamespaceDefault,
1608-
},
1609-
Spec: snapshotv1.VolumeSnapshotSpec{
1610-
Source: snapshotv1.VolumeSnapshotSource{
1611-
PersistentVolumeClaimName: &pvc.Name,
1618+
It("Should set snapshot source volume mode annotation on carried-over-upgrade snapshot", func() {
1619+
// Snap already exists, without the source volume mode annotation
1620+
snap := &snapshotv1.VolumeSnapshot{
1621+
ObjectMeta: metav1.ObjectMeta{
1622+
Name: pvc.Name,
1623+
Namespace: metav1.NamespaceDefault,
16121624
},
1613-
},
1614-
Status: &snapshotv1.VolumeSnapshotStatus{
1615-
ReadyToUse: ptr.To[bool](true),
1616-
},
1617-
}
1618-
err = reconciler.client.Create(context.TODO(), snap)
1619-
Expect(err).ToNot(HaveOccurred())
1625+
Spec: snapshotv1.VolumeSnapshotSpec{
1626+
Source: snapshotv1.VolumeSnapshotSource{
1627+
PersistentVolumeClaimName: &pvc.Name,
1628+
},
1629+
},
1630+
Status: &snapshotv1.VolumeSnapshotStatus{
1631+
ReadyToUse: ptr.To[bool](true),
1632+
},
1633+
}
1634+
err := reconciler.client.Create(context.TODO(), snap)
1635+
Expect(err).ToNot(HaveOccurred())
16201636

1621-
verifyConditions("Import succeeded", false, true, true, noImport, upToDate, ready, &snapshotv1.VolumeSnapshot{})
1637+
verifyConditions("Import succeeded", false, true, true, noImport, upToDate, ready, &snapshotv1.VolumeSnapshot{})
16221638

1623-
snap = &snapshotv1.VolumeSnapshot{}
1624-
err = reconciler.client.Get(context.TODO(), dvKey(dvName), snap)
1625-
Expect(err).ToNot(HaveOccurred())
1626-
Expect(*snap.Status.ReadyToUse).To(BeTrue())
1627-
Expect(*snap.Spec.Source.PersistentVolumeClaimName).To(Equal(dvName))
1628-
Expect(snap.Annotations[cc.AnnSourceVolumeMode]).To(Equal("dummyfromsp"))
1639+
snap = &snapshotv1.VolumeSnapshot{}
1640+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), snap)
1641+
Expect(err).ToNot(HaveOccurred())
1642+
Expect(*snap.Status.ReadyToUse).To(BeTrue())
1643+
Expect(*snap.Spec.Source.PersistentVolumeClaimName).To(Equal(dvName))
1644+
Expect(snap.Annotations[cc.AnnSourceVolumeMode]).To(Equal("dummyfromsp"))
1645+
})
1646+
1647+
It("Should set snapshot advised restore size annotation on carried-over-upgrade snapshot", func() {
1648+
// Snap already exists, without the advised restore size annotation
1649+
snap := &snapshotv1.VolumeSnapshot{
1650+
ObjectMeta: metav1.ObjectMeta{
1651+
Name: pvc.Name,
1652+
Namespace: metav1.NamespaceDefault,
1653+
},
1654+
Spec: snapshotv1.VolumeSnapshotSpec{
1655+
Source: snapshotv1.VolumeSnapshotSource{
1656+
PersistentVolumeClaimName: &pvc.Name,
1657+
},
1658+
},
1659+
Status: &snapshotv1.VolumeSnapshotStatus{
1660+
ReadyToUse: ptr.To[bool](true),
1661+
RestoreSize: ptr.To[resource.Quantity](resource.MustParse("100Mi")),
1662+
},
1663+
}
1664+
err := reconciler.client.Create(context.TODO(), snap)
1665+
Expect(err).ToNot(HaveOccurred())
1666+
1667+
verifyConditions("Import succeeded", false, true, true, noImport, upToDate, ready, &snapshotv1.VolumeSnapshot{})
1668+
1669+
snap = &snapshotv1.VolumeSnapshot{}
1670+
err = reconciler.client.Get(context.TODO(), dvKey(dvName), snap)
1671+
Expect(err).ToNot(HaveOccurred())
1672+
Expect(*snap.Status.ReadyToUse).To(BeTrue())
1673+
Expect(*snap.Spec.Source.PersistentVolumeClaimName).To(Equal(dvName))
1674+
Expect(snap.Annotations[cc.AnnAdvisedRestoreSize]).To(Equal("1Gi"), "advised restore size should be set to the size of the source PVC")
1675+
})
16291676
})
16301677
})
16311678
})
@@ -1640,6 +1687,78 @@ var _ = Describe("untagURL", func() {
16401687
})
16411688
})
16421689

1690+
var _ = Describe("inferAdvisedRestoreSizeForSnapshot", func() {
1691+
var (
1692+
dv *cdiv1.DataVolume
1693+
snapshot *snapshotv1.VolumeSnapshot
1694+
)
1695+
1696+
BeforeEach(func() {
1697+
dv = &cdiv1.DataVolume{
1698+
Spec: cdiv1.DataVolumeSpec{
1699+
Storage: &cdiv1.StorageSpec{},
1700+
},
1701+
}
1702+
snapshot = &snapshotv1.VolumeSnapshot{}
1703+
})
1704+
1705+
DescribeTable("should return the correct size", func(dvSize, snapshotRestoreSize, fallbackSize, expectedSize string) {
1706+
if dvSize != "" {
1707+
dv.Spec.Storage = &cdiv1.StorageSpec{
1708+
Resources: corev1.VolumeResourceRequirements{
1709+
Requests: corev1.ResourceList{
1710+
corev1.ResourceStorage: resource.MustParse(dvSize),
1711+
},
1712+
},
1713+
}
1714+
}
1715+
1716+
if snapshotRestoreSize != "" {
1717+
restoreSize := resource.MustParse(snapshotRestoreSize)
1718+
snapshot.Status = &snapshotv1.VolumeSnapshotStatus{
1719+
RestoreSize: &restoreSize,
1720+
}
1721+
}
1722+
1723+
var fallback *resource.Quantity
1724+
if fallbackSize != "" {
1725+
fb := resource.MustParse(fallbackSize)
1726+
fallback = &fb
1727+
}
1728+
1729+
result := inferAdvisedRestoreSizeForSnapshot(dv, snapshot, fallback)
1730+
1731+
if expectedSize == "" {
1732+
Expect(result.IsZero()).To(BeTrue())
1733+
} else {
1734+
Expect(result).NotTo(BeNil())
1735+
Expect(result.String()).To(Equal(expectedSize))
1736+
}
1737+
},
1738+
Entry("should return dvSize when snapshot restoreSize is nil", "1Gi", "", "", "1Gi"),
1739+
Entry("should return dvSize when dvSize is larger than snapshot restoreSize", "2Gi", "1Gi", "", "2Gi"),
1740+
Entry("should return snapshot restoreSize when it is larger than dvSize", "1Gi", "2Gi", "", "2Gi"),
1741+
Entry("should return snapshot restoreSize when dvSize equals snapshot restoreSize", "1Gi", "1Gi", "", "1Gi"),
1742+
Entry("should return fallback when dvSize is zero and fallback is provided", "", "", "500Mi", "500Mi"),
1743+
Entry("should return zero when dvSize is zero, no snapshot restoreSize, and no fallback", "", "", "", ""),
1744+
Entry("should return dvSize when dvSize is set even if fallback is provided", "1Gi", "", "500Mi", "1Gi"),
1745+
)
1746+
1747+
It("should handle nil snapshot status", func() {
1748+
dv.Spec.Storage = &cdiv1.StorageSpec{
1749+
Resources: corev1.VolumeResourceRequirements{
1750+
Requests: corev1.ResourceList{
1751+
corev1.ResourceStorage: resource.MustParse("1Gi"),
1752+
},
1753+
},
1754+
}
1755+
snapshot.Status = nil
1756+
1757+
result := inferAdvisedRestoreSizeForSnapshot(dv, snapshot, nil)
1758+
Expect(result.String()).To(Equal("1Gi"))
1759+
})
1760+
})
1761+
16431762
func createDataImportCronReconciler(objects ...runtime.Object) *DataImportCronReconciler {
16441763
cdiConfig := cc.MakeEmptyCDIConfigSpec(common.ConfigName)
16451764
objs := []runtime.Object{cdiConfig}

0 commit comments

Comments
 (0)