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

Jump to bottom

PVC Clone Populator #2709

Merged
merged 12 commits into from
May 24, 2023
Merged

PVC Clone Populator #2709

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
1bac742
touch up zero restoresize snapshot
mhenriks Apr 22, 2023
fd84df9
clone populator
mhenriks May 5, 2023
3e1d894
more unit tests
mhenriks May 11, 2023
188e448
unit test for clone populator
mhenriks May 17, 2023
a5ac4cb
func tests for clone populator
mhenriks May 18, 2023
aa2dd2f
move clone populator cleanup function to planner
mhenriks May 18, 2023
855b8dc
add readonly flag to test executor pods
mhenriks May 19, 2023
55e56db
increase linter timeout
mhenriks May 19, 2023
c7be492
better/explicit readonly support for test pods
mhenriks May 21, 2023
eeb5ec9
check pv for driver info before looking up storageclass as it may not…
mhenriks May 22, 2023
ec33442
addressed review comments
mhenriks May 22, 2023
23935e1
chooseStrategy shoud generate more events
mhenriks May 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/cdi-controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ func start() {
os.Exit(1)
}
if _, err := populators.NewClonePopulator(ctx, mgr, log, clonerImage, pullPolicy, installerLabels); err != nil {
klog.Errorf("Unable to setup import populator: %v", err)
klog.Errorf("Unable to setup clone populator: %v", err)
os.Exit(1)
}

Expand Down
4 changes: 4 additions & 0 deletions pkg/controller/clone/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,11 @@ go_test(
"clone_suite_test.go",
"csi-clone_test.go",
"host-clone_test.go",
"planner_test.go",
"prep-claim_test.go",
"rebind_test.go",
"snap-clone_test.go",
"snapshot_test.go",
],
embed = [":go_default_library"],
deps = [
Expand All @@ -61,6 +64,7 @@ go_test(
"//vendor/k8s.io/apimachinery/pkg/api/resource:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
"//vendor/k8s.io/client-go/kubernetes/scheme:go_default_library",
"//vendor/k8s.io/client-go/tools/record:go_default_library",
"//vendor/k8s.io/utils/pointer:go_default_library",
Expand Down
14 changes: 14 additions & 0 deletions pkg/controller/clone/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,20 @@ func GetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.P
return nil, nil
}

// MustGetStorageClassForClaim returns the storageclass for a PVC
func MustGetStorageClassForClaim(ctx context.Context, c client.Client, pvc *corev1.PersistentVolumeClaim) (*storagev1.StorageClass, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this function name is a little bit awkward. I also think it doesn't make that much sense to have it since we still need to check its error return (we are only saving two checks in planner.go).

Copy link
Member Author

Choose a reason for hiding this comment

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

fine

sc, err := GetStorageClassForClaim(ctx, c, pvc)
if err != nil {
return nil, err
}

if sc == nil {
return nil, fmt.Errorf("no storageclass for pvc")
}

return sc, nil
}

// GetCompatibleVolumeSnapshotClass returns a VolumeSNapshotClass name that works for all PVCs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Minor typo: Uppercase N in VolumeSNapshotClass.

func GetCompatibleVolumeSnapshotClass(ctx context.Context, c client.Client, pvcs ...*corev1.PersistentVolumeClaim) (*string, error) {
var storageClasses []*storagev1.StorageClass
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/clone/csi-clone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ func (p *CSIClonePhase) Name() string {
return "CSIClone"
}

// Reconcile ensures a snapshot is created correctly
// Reconcile ensures a csi cloned pvc is created correctly
func (p *CSIClonePhase) Reconcile(ctx context.Context) (*reconcile.Result, error) {
pvc := &corev1.PersistentVolumeClaim{}
exists, err := getResource(ctx, p.Client, p.Namespace, p.DesiredClaim.Name, pvc)
Expand Down
4 changes: 4 additions & 0 deletions pkg/controller/clone/host-clone.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ type HostClonePhase struct {
DesiredClaim *corev1.PersistentVolumeClaim
ImmediateBind bool
OwnershipLabel string
Preallocation bool
ContentType string
Client client.Client
Log logr.Logger
Recorder record.EventRecorder
Expand Down Expand Up @@ -101,6 +103,8 @@ func (p *HostClonePhase) createClaim(ctx context.Context) (*corev1.PersistentVol
claim := p.DesiredClaim.DeepCopy()

claim.Namespace = p.Namespace
cc.AddAnnotation(claim, cc.AnnContentType, p.ContentType)
cc.AddAnnotation(claim, cc.AnnPreallocationRequested, fmt.Sprintf("%t", p.Preallocation))
cc.AddAnnotation(claim, cc.AnnOwnerUID, string(p.Owner.GetUID()))
cc.AddAnnotation(claim, cc.AnnPodRestarts, "0")
cc.AddAnnotation(claim, cc.AnnCloneRequest, fmt.Sprintf("%s/%s", p.Namespace, p.SourceName))
Expand Down
4 changes: 4 additions & 0 deletions pkg/controller/clone/host-clone_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,8 @@ var _ = Describe("HostClonePhase test", func() {
SourceName: "source",
DesiredClaim: desired,
ImmediateBind: true,
ContentType: "kubevirt",
Preallocation: false,
Client: cl,
Recorder: rec,
Log: log,
Expand All @@ -99,6 +101,8 @@ var _ = Describe("HostClonePhase test", func() {

pvc := getDesiredClaim(p)
Expect(pvc.Spec.DataSourceRef).To(BeNil())
Expect(pvc.Annotations[cc.AnnContentType]).To(Equal("kubevirt"))
Expect(pvc.Annotations[cc.AnnPreallocationRequested]).To(Equal("false"))
Expect(pvc.Annotations[cc.AnnOwnerUID]).To(Equal(string(p.Owner.GetUID())))
Expect(pvc.Annotations[cc.AnnPodRestarts]).To(Equal("0"))
Expect(pvc.Annotations[cc.AnnCloneRequest]).To(Equal("ns/source"))
Expand Down
24 changes: 16 additions & 8 deletions pkg/controller/clone/planner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func (p *Planner) watchOwned(log logr.Logger, obj client.Object) error {
}

func (p *Planner) strategyForSourcePVC(ctx context.Context, args *ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {
Copy link
Collaborator

@alromeros alromeros May 22, 2023 •
edited

Choose a reason for hiding this comment

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

Probably just preference but I miss having the verb in several function names. For example, I think getStrategyForSourcePVC sounds better and it's more informative than just strategyForSourcePVC.

Copy link
Member Author

Choose a reason for hiding this comment

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

perhaps this is a bit pedantic, but I like the prefix get/set for simple operations. simply returning a value that is already set/stored somewhere for example. This is actually computing a value that is not stored anywhere. I will add 'compute' prefix

if ok, err := p.validateTargetStorageClassAssignment(args); !ok || err != nil {
if ok, err := p.validateTargetStorageClassAssignment(ctx, args); !ok || err != nil {
return nil, err
}

Expand Down Expand Up @@ -239,7 +239,7 @@ func (p *Planner) strategyForSourcePVC(ctx context.Context, args *ChooseStrategy
}

if !exists {
args.Log.V(1).Info("missing storageprofile for", "name", *args.TargetClaim.Spec.StorageClassName)
args.Log.V(3).Info("missing storageprofile for", "name", *args.TargetClaim.Spec.StorageClassName)
}

if exists && sp.Status.CloneStrategy != nil {
Expand Down Expand Up @@ -273,7 +273,7 @@ func (p *Planner) strategyForSourcePVC(ctx context.Context, args *ChooseStrategy
return &strategy, nil
}

func (p *Planner) validateTargetStorageClassAssignment(args *ChooseStrategyArgs) (bool, error) {
func (p *Planner) validateTargetStorageClassAssignment(ctx context.Context, args *ChooseStrategyArgs) (bool, error) {
if args.TargetClaim.Spec.StorageClassName == nil {
args.Log.V(3).Info("Target PVC has nil storage class, cannot compute strategy")
return false, nil
Expand All @@ -284,6 +284,11 @@ func (p *Planner) validateTargetStorageClassAssignment(args *ChooseStrategyArgs)
return false, fmt.Errorf("claim has emptystring storageclass, will not work")
Copy link
Contributor

Choose a reason for hiding this comment

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

why here you return error that the storageclass name is empty but return nil if the storage class is nil? shouldn't they both end with the same result?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, with k8s 1.26 a nil storageClassName can be updated when there is no default storageclass. So we should wait until a storageclass is assigned

}

_, err := MustGetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
if err != nil {
return false, err
}

return true, nil
}

Expand All @@ -305,15 +310,11 @@ func (p *Planner) validateAdvancedClonePVC(ctx context.Context, args *ChooseStra
return false, nil
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to emit an event here indicating why we switched to host assisted clone? In most cases we are not logging at V(3) and this piece of information gets lost. I would settle for a V(1) log honestly.

}

sc, err := GetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
sc, err := MustGetStorageClassForClaim(ctx, p.Client, args.TargetClaim)
if err != nil {
return false, err
}

if sc == nil {
return false, fmt.Errorf("target storageclass should exist")
}

srcCapacity, hasSrcCapacity := sourceClaim.Status.Capacity[corev1.ResourceStorage]
targetRequest, hasTargetRequest := args.TargetClaim.Spec.Resources.Requests[corev1.ResourceStorage]
allowExpansion := sc.AllowVolumeExpansion != nil && *sc.AllowVolumeExpansion
Expand Down Expand Up @@ -344,13 +345,20 @@ func (p *Planner) planHostAssistedFromPVC(ctx context.Context, args *PlanArgs) (
desiredClaim.Spec.Resources.Requests[corev1.ResourceStorage] = is
}

ct := cdiv1.DataVolumeKubeVirt
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use: cc.GetContentType(string(uploadSource.Spec.ContentType))

Copy link
Member Author

Choose a reason for hiding this comment

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

This is outdated code, contentType was removed

if args.DataSource.Spec.ContentType != "" {
ct = args.DataSource.Spec.ContentType
}

hcp := &HostClonePhase{
Owner: args.TargetClaim,
Namespace: args.DataSource.Namespace,
SourceName: args.DataSource.Spec.Source.Name,
DesiredClaim: desiredClaim,
ImmediateBind: true,
OwnershipLabel: p.OwnershipLabel,
ContentType: string(ct),
Preallocation: cc.GetPreallocation(ctx, p.Client, args.DataSource.Spec.Preallocation),
Client: p.Client,
Log: args.Log,
Recorder: p.Recorder,
Expand Down