Skip to content

Commit

Permalink
CR fixes
Browse files Browse the repository at this point in the history 
Signed-off-by: Arnon Gilboa <agilboa@redhat.com>
  • Loading branch information
@arnongilboa
arnongilboa committed Nov 6, 2023
1 parent 2d3b684 commit 5392808
Show file tree
Hide file tree
Showing 8 changed files with 115 additions and 66 deletions.
13 changes: 8 additions & 5 deletions pkg/controller/clone/common.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,13 @@ func GetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.P
}

func getSnapshotClassForClaim(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*string, error) {
if pvc.Spec.StorageClassName == nil || *pvc.Spec.StorageClassName == "" {
return nil, nil
sc, err := cc.GetStorageClassByNameWithK8sFallback(ctx, c, pvc.Spec.StorageClassName)
if err != nil || sc == nil {
return nil, err
}

sp := &cdiv1.StorageProfile{}
exists, err := getResource(ctx, c, "", *pvc.Spec.StorageClassName, sp)
exists, err := getResource(ctx, c, "", sc.Name, sp)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -247,7 +248,9 @@ func getCommonSnapshotClass(ctx context.Context, c client.Client, pvcs ...*corev
}

// GetCompatibleVolumeSnapshotClass returns a VolumeSnapshotClass name that works for all PVCs
func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, log logr.Logger, pvcs ...*corev1.PersistentVolumeClaim) (*string, error) {
// Note the last PVC passed is considered the target PVC for logs and events
func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, log logr.Logger, recorder record.EventRecorder, pvcs ...*corev1.PersistentVolumeClaim) (*string, error) {
targetClaim := pvcs[len(pvcs)-1]
driver, err := GetCommonDriver(ctx, c, pvcs...)
if err != nil {
return nil, err
Expand All @@ -261,7 +264,7 @@ func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, log
return nil, err
}

return cc.GetVolumeSnapshotClass(context.TODO(), c, *driver, snapshotClassName, log)
return cc.GetVolumeSnapshotClass(context.TODO(), c, targetClaim, *driver, snapshotClassName, log, recorder)
}

// SameVolumeMode returns true if all pvcs have the same volume mode
Expand Down
4 changes: 2 additions & 2 deletions pkg/controller/clone/planner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ func (p *Planner) computeStrategyForSourcePVC(ctx context.Context, args *ChooseS
}

if strategy == cdiv1.CloneStrategySnapshot {
n, err := GetCompatibleVolumeSnapshotClass(ctx, p.Client, args.Log, sourceClaim, args.TargetClaim)
n, err := GetCompatibleVolumeSnapshotClass(ctx, p.Client, args.Log, p.Recorder, sourceClaim, args.TargetClaim)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -660,7 +660,7 @@ func (p *Planner) planSnapshotFromPVC(ctx context.Context, args *PlanArgs) ([]Ph
return nil, fmt.Errorf("source claim does not exist")
}

vsc, err := GetCompatibleVolumeSnapshotClass(ctx, p.Client, args.Log, args.TargetClaim)
vsc, err := GetCompatibleVolumeSnapshotClass(ctx, p.Client, args.Log, p.Recorder, args.TargetClaim)
if err != nil {
return nil, err
}
Expand Down
69 changes: 39 additions & 30 deletions pkg/controller/clone/planner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ var _ = Describe("Planner test", func() {
)

var (
planner *Planner

storageClassName = "sc"

small = resource.MustParse("5Gi")
Expand Down Expand Up @@ -242,6 +244,13 @@ var _ = Describe("Planner test", func() {
Expect(found).To(BeTrue())
}

AfterEach(func() {
if planner != nil && planner.Recorder != nil {
close(planner.Recorder.(*record.FakeRecorder).Events)
planner = nil
}
})

Context("ChooseStrategy tests", func() {

It("should error if unsupported kind", func() {
Expand All @@ -251,7 +260,7 @@ var _ = Describe("Planner test", func() {
DataSource: source,
Log: log,
}
planner := createPlanner()
planner = createPlanner()
_, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("unsupported datasource"))
Expand All @@ -266,7 +275,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner()
planner = createPlanner()
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).To(BeNil())
Expand All @@ -280,7 +289,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner()
planner = createPlanner()
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("claim has emptystring storageclass, will not work"))
Expand All @@ -293,7 +302,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner()
planner = createPlanner()
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("target storage class not found"))
Expand All @@ -306,7 +315,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass())
planner = createPlanner(createStorageClass())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).To(BeNil())
Expand All @@ -322,7 +331,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), source)
planner = createPlanner(createStorageClass(), source)
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(HavePrefix("target resources requests storage size is smaller than the source"))
Expand All @@ -336,7 +345,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createSourceClaim())
planner = createPlanner(createStorageClass(), createSourceClaim())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -352,7 +361,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass(), createSourceVolume())
planner = createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass(), createSourceVolume())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -365,7 +374,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass())
planner = createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -382,7 +391,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createVolumeSnapshotClass(), sourceClaim, sourceVolume)
planner = createPlanner(createStorageClass(), createVolumeSnapshotClass(), sourceClaim, sourceVolume)
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -397,7 +406,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass())
planner = createPlanner(createStorageClass(), createSourceClaim(), createVolumeSnapshotClass())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -414,7 +423,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(storageClass, createSourceClaim(), createVolumeSnapshotClass())
planner = createPlanner(storageClass, createSourceClaim(), createVolumeSnapshotClass())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -433,7 +442,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createVolumeSnapshotClass(), source)
planner = createPlanner(createStorageClass(), createVolumeSnapshotClass(), source)
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -450,7 +459,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), createSourceClaim())
planner = createPlanner(createStorageClass(), createSourceClaim())
cdi := &cdiv1.CDI{}
err := planner.Client.Get(context.Background(), client.ObjectKeyFromObject(cc.MakeEmptyCDICR()), cdi)
Expect(err).ToNot(HaveOccurred())
Expand Down Expand Up @@ -478,7 +487,7 @@ var _ = Describe("Planner test", func() {
CloneStrategy: &cs,
},
}
planner := createPlanner(sp, createStorageClass(), createSourceClaim())
planner = createPlanner(sp, createStorageClass(), createSourceClaim())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -505,7 +514,7 @@ var _ = Describe("Planner test", func() {
sourceVolume := createSourceVolume()
sourceVolume.Spec.StorageClassName = "foo"
sourceVolume.Spec.PersistentVolumeSource.CSI.Driver = "baz"
planner := createPlanner(sp, createStorageClass(), sourceClaim, sourceVolume)
planner = createPlanner(sp, createStorageClass(), sourceClaim, sourceVolume)
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand Down Expand Up @@ -533,7 +542,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass())
planner = createPlanner(createStorageClass())
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).To(BeNil())
Expand All @@ -546,7 +555,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner()
planner = createPlanner()
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("target storage class not found"))
Expand All @@ -562,7 +571,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), source)
planner = createPlanner(createStorageClass(), source)
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("volumeSnapshotContent name not found"))
Expand All @@ -577,7 +586,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("test"))
planner = createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("test"))
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -596,7 +605,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("driver"))
planner = createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("driver"))
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("snapshot has no RestoreSize"))
Expand All @@ -613,7 +622,7 @@ var _ = Describe("Planner test", func() {
}
sc := createStorageClass()
sc.AllowVolumeExpansion = nil
planner := createPlanner(sc, source, createDefaultVolumeSnapshotContent("driver"))
planner = createPlanner(sc, source, createDefaultVolumeSnapshotContent("driver"))
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand All @@ -631,7 +640,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("driver"))
planner = createPlanner(createStorageClass(), source, createDefaultVolumeSnapshotContent("driver"))
csr, err := planner.ChooseStrategy(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(csr).ToNot(BeNil())
Expand Down Expand Up @@ -750,7 +759,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(cdiConfig, createStorageClass(), source)
planner = createPlanner(cdiConfig, createStorageClass(), source)
plan, err := planner.Plan(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expand All @@ -768,7 +777,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(cdiConfig, createStorageClass(), createVolumeSnapshotClass(), source)
planner = createPlanner(cdiConfig, createStorageClass(), createVolumeSnapshotClass(), source)
plan, err := planner.Plan(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expand All @@ -788,7 +797,7 @@ var _ = Describe("Planner test", func() {
DataSource: createPVCDataSource(),
Log: log,
}
planner := createPlanner(cdiConfig, createStorageClass(), source)
planner = createPlanner(cdiConfig, createStorageClass(), source)
plan, err := planner.Plan(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expand All @@ -807,7 +816,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(cdiConfig, createStorageClass(), source, createVolumeSnapshotClass(), createDefaultVolumeSnapshotContent())
planner = createPlanner(cdiConfig, createStorageClass(), source, createVolumeSnapshotClass(), createDefaultVolumeSnapshotContent())
plan, err := planner.Plan(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expand All @@ -826,7 +835,7 @@ var _ = Describe("Planner test", func() {
DataSource: createSnapshotDataSource(),
Log: log,
}
planner := createPlanner(cdiConfig, createStorageClass(), source, createVolumeSnapshotClass(), createDefaultVolumeSnapshotContent())
planner = createPlanner(cdiConfig, createStorageClass(), source, createVolumeSnapshotClass(), createDefaultVolumeSnapshotContent())
plan, err := planner.Plan(context.Background(), args)
Expect(err).ToNot(HaveOccurred())
Expect(plan).ToNot(BeNil())
Expand All @@ -848,7 +857,7 @@ var _ = Describe("Planner test", func() {
}
sc := createStorageClass()
sc.Provisioner = "test-error"
planner := createPlanner(cdiConfig, sc, source, createVolumeSnapshotClass(), createDefaultVolumeSnapshotContent())
planner = createPlanner(cdiConfig, sc, source, createVolumeSnapshotClass(), createDefaultVolumeSnapshotContent())
plan, err := planner.Plan(context.Background(), args)
Expect(err).To(HaveOccurred())
Expect(err.Error()).To(Equal("unable to find a valid storage class for the temporal source claim"))
Expand Down Expand Up @@ -893,7 +902,7 @@ var _ = Describe("Planner test", func() {
It("should cleanup tmp resources", func() {
tempObjs := tempResources()
target := createTargetClaim()
planner := createPlanner(tempObjs...)
planner = createPlanner(tempObjs...)
err := planner.Cleanup(context.Background(), log, target)
Expect(err).ToNot(HaveOccurred())
for _, r := range tempResources() {
Expand Down

0 comments on commit 5392808

Please sign in to comment.