-
Notifications
You must be signed in to change notification settings - Fork 267
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
DataVolume Controller uses VolumeCloneSource Populator #2750
Conversation
/hold |
@@ -468,20 +468,32 @@ func (r *PvcCloneReconciler) isSourceReadyToClone(datavolume *cdiv1.DataVolume) | |||
|
|||
// detectCloneSize obtains and assigns the original PVC's size when cloning using an empty storage value | |||
func (r *PvcCloneReconciler) detectCloneSize(syncState *dvSyncState) (bool, error) { | |||
var targetSize int64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
special note on all the changes done in this function...was all done to get this crazy test to work:
It("bz:2079781 Should clone data from filesystem to block, when using storage API ", func() { |
/test pull-containerized-data-importer-e2e-ceph |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good start! Just some comments after an initial review.
@@ -84,7 +85,7 @@ const ( | |||
// MessageSmartCloneInProgress provides a const to form snapshot for smart-clone is in progress message | |||
MessageSmartCloneInProgress = "Creating snapshot for smart-clone is in progress (for pvc %s/%s)" | |||
// MessageCloneFromSnapshotSourceInProgress provides a const to form clone from snapshot source is in progress message | |||
MessageCloneFromSnapshotSourceInProgress = "Creating PVC from snapshot source is in progress (for snapshot %s/%s)" | |||
MessageCloneFromSnapshotSourceInProgress = "Creating PVC from snapshot source is in progress (for %s %s/%s)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you are now requiring the source type as argument, maybe change the message so it can be used for both snapshots and pvcs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I think that error string can be improved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I think I'm not sure what you're asking...the message is used whenever we create a pvc from a snapshot. That happens with pvc-smart clone and snapshot-clone. Are you suggesting the message be use when a pvc is snapshotted as well? That only happens with pvc-smart clone. Examples would be great.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of the variable should change maybe drop the "snapshot"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm...I think the source
part may be the most confusing since someone may be thinking of the DataVolume source and not the pvc dataSource.
pkg/controller/common/util.go
Outdated
} | ||
|
||
// MergePatch patches a resource | ||
func MergePatch(ctx context.Context, args *PatchArgs) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
@@ -178,6 +187,67 @@ func (r *CloneReconcilerBase) ensureExtendedToken(pvc *corev1.PersistentVolumeCl | |||
return nil | |||
} | |||
|
|||
func (r *CloneReconcilerBase) reconcileVolumeCloneSourceCR(syncState *dvSyncState, kind string) error { | |||
dv := syncState.dvMutated | |||
cloneSource := &cdiv1.VolumeCloneSource{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd improve readability to use volumeCloneSource
and volumeCloneSourceName
to differentiate from the actual clone source.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
sourcePvc, err := r.findSourcePvc(syncState.dvMutated) | ||
if err != nil { | ||
return false, err | ||
} | ||
|
||
// because of filesystem overhead calculations when cloning | ||
// even if storage size is requested we have to calucuale source size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: calucuale
// TODO: Fix this in next PR that uses actual size also in validation | ||
isPermissiveClone = sourceCapacity.CmpInt64(targetSize) == 1 | ||
} else { | ||
isPermissiveClone = requestedSize.CmpInt64(targetSize) >= 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this has to do with the test you commented above, right? But why would we need to set the permissiveClone annotation if the comparison is equal or larger than 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
requestedSize
may still be smaller than source capacity (as in that contrived test case) but populator layer doesn't know that and will understandably not let you clone your 4G source to a 2G target
@@ -203,10 +203,15 @@ func (r *UploadReconciler) reconcilePVC(log logr.Logger, pvc *corev1.PersistentV | |||
} | |||
|
|||
if len(podsUsingPVC) > 0 { | |||
es, err := cc.GetAnnotatedEventSource(context.TODO(), r.client, pvc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to bother this much about styling but I feel like these variable names work for more common objects like dvs or pvcs. For most cases I think it's better to use the full version, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you are suggesting here...you don't like 'es'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I've seen some of these in the PR and I always think it'd be better to have the full name, like cloneType
instead of ct
. Just a small complaint though, not a must.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't necessarily disagree with you. I typically use descriptive names (I think). But I also like to use short names to signify "this is something you shouldn't really care about". Similar to 'i' in loops. Variables that are very short in scope.
/retest-required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! have some comments, as usual going over commit by commit some were already addressed..
@@ -20,7 +20,6 @@ import ( | |||
"context" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do love the bye bye commit message, but maybe share why is it being removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure you know why, but since populators are handling all CSI storage the DataVolume controller will only be doing host assisted clones
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant share in the commit message
@@ -582,3 +590,77 @@ func (r *SnapshotCloneReconciler) isSnapshotValidForClone(snapshot *snapshotv1.V | |||
} | |||
return true, nil | |||
} | |||
|
|||
func newPvcFromSnapshot(obj metav1.Object, name string, snapshot *snapshotv1.VolumeSnapshot, targetPvcSpec *corev1.PersistentVolumeClaimSpec) (*corev1.PersistentVolumeClaim, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this function relevant to this this commit? I dont see it being used anywhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was in smart clone controller file which was deleted but this function and a couple other defs are needed
const snapshotCloneControllerName = "datavolume-snapshot-clone-controller" | ||
const ( | ||
//AnnSmartCloneRequest sets our expected annotation for a CloneRequest | ||
AnnSmartCloneRequest = "k8s.io/SmartCloneRequest" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if newPvcFromSnapshot is not relevant to this commit (as asked in another comment) then I guess this const are neither?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remnants from a deleted file
pkg/controller/common/util.go
Outdated
// GetCloneSourceNameAndNamespace returns the name and namespace of the cloning source | ||
func GetCloneSourceNameAndNamespace(dv *cdiv1.DataVolume) (name, namespace string) { | ||
var sourceName, sourceNamespace string | ||
// GetCloneSourceInfo returns the name and namespace of the cloning source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can also update the comment with returns sourceType
@@ -84,7 +85,7 @@ const ( | |||
// MessageSmartCloneInProgress provides a const to form snapshot for smart-clone is in progress message | |||
MessageSmartCloneInProgress = "Creating snapshot for smart-clone is in progress (for pvc %s/%s)" | |||
// MessageCloneFromSnapshotSourceInProgress provides a const to form clone from snapshot source is in progress message | |||
MessageCloneFromSnapshotSourceInProgress = "Creating PVC from snapshot source is in progress (for snapshot %s/%s)" | |||
MessageCloneFromSnapshotSourceInProgress = "Creating PVC from snapshot source is in progress (for %s %s/%s)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name of the variable should change maybe drop the "snapshot"
cc.AddAnnotation(claimCpy, AnnCloneError, lastError.Error()) | ||
|
||
if !apiequality.Semantic.DeepEqual(pvc, claimCpy) { | ||
if err := r.client.Update(ctx, claimCpy); err != nil { | ||
if err := r.patchClaim(ctx, log, pvc, claimCpy); err != nil { | ||
r.log.V(1).Info("error setting error annotations") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use here log instead of r.log?
}, | ||
Spec: cdiv1.VolumeCloneSourceSpec{ | ||
Source: corev1.TypedLocalObjectReference{ | ||
Kind: "PersistentVolumeClaim", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess you wanted to put Kind: kind
return nil | ||
} | ||
|
||
return r.reconcileVolumeCloneSourceCR(syncState, "PersistentVolumeClaim") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
calling to this function even when not using populators will work but not sure if we should..
var desiredCloneAnnotations []string | ||
|
||
func init() { | ||
desiredCloneAnnotations = append(desiredCloneAnnotations, desiredAnnotations...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just add the AnnCloneOf
to the desiredannotations list.. it wont hurt anything, maybe the name is not the best and should change maybe you have better name
} | ||
|
||
if dv.DeletionTimestamp != nil { | ||
if err := r.reconcileVolumeCloneSourceCR(syncState); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure we dont want to call this also when dv succeeded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will get called here:
containerized-data-importer/pkg/controller/datavolume/snapshot-clone-controller.go
Line 233 in 9da9c01
if err := r.reconcileVolumeCloneSourceCR(&syncRes); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if for some reason we will exit here:
containerized-data-importer/pkg/controller/datavolume/snapshot-clone-controller.go
Line 181 in 9da9c01
return syncRes, syncErr |
containerized-data-importer/pkg/controller/datavolume/snapshot-clone-controller.go
Line 193 in 9da9c01
return syncRes, nil |
then we will never delete the volumeclonesource. dont see why shouldn't just call it few lines after https://github.com/kubevirt/containerized-data-importer/pull/2750/files/ac7a342dcd831bb0746e634be774942d05d20086..9da9c01e032c1c6d26e242f3b3fe5789c456eb6e#diff-57c4756c3e19744255585cd838f48d98a4410e30b31813b74b23874c998b3268R441
after the check of deleted and succeeded..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless there is an unexpected error (which is retried), it should get called. But I suppose the behavior of the base controller could change and mess things up as well. This is such spaghetti code. IMO controllers should always execute the same code block at each reconcile. I don't like that the derived controller sync
is not called if the DataVolume is deleted.
Anyway, my intention was to reduce special cases and have the cleanup function do less. Because I don't think it should exist in the first place.
@arnongilboa - please check general integration with DataVolume controllers. A lot of code is gone! |
Not sure what that consistent after suite failure is about, seems some DV has a finalizer remaining Some content in the namespace has finalizers remaining: cdi.kubevirt.io/dataVolumeFinalizer in 1 resource instances}]}}] We might want to dump out some info about dangling resources after func test runs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akalenyu - please check out clone from snapshot path
Looks good, we should also have most permutations covered by snapclone func tests
so that makes me feel better
My only concern was that we are losing coverage in that host assisted path
return syncRes, err | ||
} | ||
targetHostAssistedPvc, err := r.createPvcForDatavolume(datavolume, pvcSpec, r.updateAnnotations) | ||
targetPvc, err := r.createPvcForDatavolume(datavolume, pvcSpec, pvcModifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went ahead and debugged the sizeless clone failure from volumesnapshot clones:
Since we now create the target PVC through createPvcForDatavolume, nothing takes
care of filling out the missing size
spec.resources[storage]: Invalid value: \"0\": must be greater than zero
I think we can simply have something similar to this func
syncState.pvcSpec.Resources.Requests[corev1.ResourceStorage] = targetCapacity |
(but way simpler, just grabs the size over from the snapshot)
@@ -772,11 +723,6 @@ var _ = Describe("all clone tests", func() { | |||
targetDiskImagePath = testBaseDir | |||
} | |||
|
|||
if cloneType == "snapshot" && sourceRef { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙏
} | ||
|
||
f.ForceBindIfWaitForFirstConsumer(targetPvc) | ||
|
||
cloneType := utils.GetCloneType(f.CdiClient, dataVolume) | ||
if cloneType != "copy" { | ||
Skip("only valid for copy clone") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't this skip enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With populators don't know clone type until populator starts working (first consumer triggered)
Expect(pvc.Annotations[controller.AnnCloneRequest]).To(HaveSuffix(suffix)) | ||
Expect(pvc.Spec.DataSource).To(BeNil()) | ||
Expect(pvc.Spec.DataSourceRef).To(BeNil()) | ||
if pvc.Spec.DataSourceRef == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should go for a more correct way of ensuring dumbclone took place?
IIUC when this runs on a CSI lane the check is not exercised
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is specifically for checking non-csi case. With populators, the pvc that was used for dumb clone is deleted after rebinding to target. But can check clonetype annotation on the dv
/test pull-containerized-data-importer-e2e-destructive destructive lane fails because of #2744 |
/retest-required |
One failure seems to be exclusive to this PR: Another one isn't: containerized-data-importer/tests/cloner_test.go Line 3245 in d173e08
|
/test pull-containerized-data-importer-e2e-nfs https://search.ci.kubevirt.io/?search=+preallocate+data+on+target+PVC&maxAge=336h&context=1&type=bug%2Bissue%2Bjunit&name=containerized-data-importer&excludeName=&maxMatches=5&maxBytes=20971520&groupBy=job Let me know how you want to proceed... for the other flakes could use a review at |
Thanks @akalenyu I think I have a lead on the failure in this pr. I'll take a look at the fixes you reference |
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
also add more validation to some snapshot clone tests Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
Using the controller runtime Patch API with controller runtime cached client seems to be a pretty bad fit At least given the way the CR API is designed where an old object is compared to new. I like patch in theory though and will revisit Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
It was possible to miss "preallocation applied" annotation otherwise Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
/test pull-containerized-data-importer-e2e-nfs |
Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
Should have been done back when annotations were addded to "progress" Also, if pvc is bound do not call phase Reconcile functions only Status Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
/unhold |
@mhenriks: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/retest-required |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @ShellyKa13
whoops |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: akalenyu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/cherrypick release-v1.57 |
@mhenriks: new pull request created: #2783 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
What this PR does / why we need it:
The DataVolume controller should use our populators internally for CSI storage provisioners. This PR builds off of #2722 to include
VolumeCloneSource
populators.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
I think everything is here but I bet there will be more whackamole on func tests
Release note: