Clone populator: Add clone source watches

[alromeros]

What this PR does / why we need it:

We lack proper clone source watches in clone populator, which can cause unwanted behaviors when we depend on source updates to trigger a reconcile.

This PR adds two watches for each clone source type.

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 # https://issues.redhat.com/browse/CNV-56518

Release note:

Add clone source watches in clone populator

[kubevirt-bot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign awels for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[alromeros]

/cc @akalenyu @arnongilboa
Sadly the test passes also without the fix, so it's useless as it is. Any idea to ensure this fixes the issue with gcp?

[alromeros]

/test pull-containerized-data-importer-e2e-ceph

[mhenriks]

@alromeros I'm not saying we shouldn't do this but we could do a cheap fix like this when the snapshot source does not exist:

containerized-data-importer/pkg/controller/clone/csi-clone.go

Line 67 in 249b004

return &reconcile.Result{RequeueAfter: 2 * time.Second}, nil

If you go with the watches, make sure to change the line above

[akalenyu]

You probably have a second watch in that dynamic snap one

containerized-data-importer/pkg/controller/clone/planner.go

Lines 179 to 183 in 249b004

	if args.Strategy == cdiv1.CloneStrategySnapshot {
	if err := p.watchSnapshots(ctx, args.Log); err != nil {
	return nil, err
	}
	}

Should be able to remove this

[akalenyu]

/cc @akalenyu @arnongilboa Sadly the test passes also without the fix, so it's useless as it is. Any idea to ensure this fixes the issue with gcp?

Yeah testing watches is not something we do, I think you would use https://book.kubebuilder.io/reference/envtest.html but clearly it's not integrated at all in CDI today.
You could unit test the watch func or something like that, make sure it doesn't return nil when the snapshot source isn't CDI-owned.

[alromeros]

You probably have a second watch in that dynamic snap one

containerized-data-importer/pkg/controller/clone/planner.go

Lines 179 to 183 in 249b004

	if args.Strategy == cdiv1.CloneStrategySnapshot {
	if err := p.watchSnapshots(ctx, args.Log); err != nil {
	return nil, err
	}
	}

Should be able to remove this

My watch only watches source clone snapshots (when they are referenced in the volumeCloneSource spec). This is for smart clone and seem to be watching snapshots created by the controller. Don't know if we should remove that one.

@mhenriks proposal is also interesting, we can fix this with a single line by requeuing here:

containerized-data-importer/pkg/controller/clone/snap-clone.go

Line 63 in 249b004

return &reconcile.Result{}, nil

[akalenyu]

@alromeros I'm not saying we shouldn't do this but we could do a cheap fix like this when the snapshot source does not exist:

containerized-data-importer/pkg/controller/clone/csi-clone.go

Line 67 in 249b004

return &reconcile.Result{RequeueAfter: 2 * time.Second}, nil

If you go with the watches, make sure to change the line above

Interesting, yeah, you could do that in

containerized-data-importer/pkg/controller/clone/snap-clone.go

Lines 62 to 64 in 249b004

	if !cc.IsSnapshotReady(snapshot) {
	return &reconcile.Result{}, nil
	}

but it looks like the original intention was to use watches anyway with that TODO.
Should we be concerned about the hit we take with that new watch?

[alromeros]

Removed both the func test and the requeue when source PVC is not ready.

[akalenyu]

This is for smart clone and seem to be watching snapshots created by the controller. Don't know if we should remove that one.

You're right my bad

I think this looks good. Maybe you have to digest volumesnapshots not existing in the cluster, but I am not sure. Feel free to undraft when you're ready and we'll see

[alromeros]

/retest-required

[arnongilboa]

@alromeros since the only thing we are interested in is the VolumeSnapshot update to Ready a few secs after it's created, why not go with @mhenriks RequeueAfter?

[akalenyu]

@alromeros since the only thing we are interested in is the VolumeSnapshot update to Ready a few secs after it's created, why not go with @mhenriks RequeueAfter?

You'd be spamming requeues for no reason, especially for provisioners where snap taking is long #2531

[arnongilboa]

@alromeros since the only thing we are interested in is the VolumeSnapshot update to Ready a few secs after it's created, why not go with @mhenriks RequeueAfter?

You'd be spamming requeues for no reason, especially for provisioners where snap taking is long #2531

Isn't watching for any update also potentially expansive? why not add a ReadyToUse == true predicate, so we won't reconcile when not needed?

[akalenyu]

@alromeros since the only thing we are interested in is the VolumeSnapshot update to Ready a few secs after it's created, why not go with @mhenriks RequeueAfter?

You'd be spamming requeues for no reason, especially for provisioners where snap taking is long #2531

Isn't watching for any update also potentially expansive?

It's not free but the alternative is occupying an entire thread on potentially useless requeues

why not add a ReadyToUse == true predicate, so we won't reconcile when not needed?

I think it's a good idea, if that is the only thing we care about, we should be doing that

[alromeros]

I prefer to keep requeue logic within a watch and handle all potential issues there.

@alromeros since the only thing we are interested in is the VolumeSnapshot update to Ready a few secs after it's created, why not go with @mhenriks RequeueAfter?

You'd be spamming requeues for no reason, especially for provisioners where snap taking is long #2531

Isn't watching for any update also potentially expansive?

It's not free but the alternative is occupying an entire thread on potentially useless requeues

why not add a ReadyToUse == true predicate, so we won't reconcile when not needed?

I think it's a good idea, if that is the only thing we care about, we should be doing that

By that logic wouldn't we might as well drop most of our watches and rely on requeues when necessary? I prefer to keep requeue logic within a watch and handle all potential issues there, but I'm not an expert in the potential performance implications. Just that having watches for sources it's what we do in the DV controller and makes sense to keep it here.

[arnongilboa]

It's not free but the alternative is occupying an entire thread on potentially useless requeues

why not add a ReadyToUse == true predicate, so we won't reconcile when not needed?

I think it's a good idea, if that is the only thing we care about, we should be doing that

By that logic wouldn't we might as well drop most of our watches and rely on requeues when necessary? I prefer to keep requeue logic within a watch and handle all potential issues there, but I'm not an expert in the potential performance implications. Just that having watches for sources it's what we do in the DV controller and makes sense to keep it here.

I meant adding the mentioned predicate in the watch like we do with Pending DVs.

[akalenyu]

/test pull-cdi-goveralls
/test pull-cdi-unit-test

[alromeros]

@mhenriks @akalenyu @arnongilboa adding back the requeue for source PVCs since we need to trigger a requeue if the PVC is in use by other pods. We also do this with the non-populator flow. Hope we can still keep the watches and decrease the number of requeues if the PVC is not being used.

[alromeros]

/test pull-cdi-unit-test

[alromeros]

/retest-required

[alromeros]

/test pull-containerized-data-importer-e2e-ceph-wffc
/test pull-containerized-data-importer-e2e-upg

[alromeros]

/test pull-containerized-data-importer-fossa

[akalenyu]

So I don't know if this is a real world issue, but, if there is not snapshot support in the cluster, you'd back out and not set up a PVC watch

[alromeros]

I considered it but I don't know how much I should worry about this case... Don't mind handling it if necessary but code might become more complex.

[arnongilboa]

Not so complex:

containerized-data-importer/pkg/controller/dataimportcron-controller.go

Lines 1199 to 1207 in f4ddcc1

	if err := mgr.GetClient().List(context.TODO(), &snapshotv1.VolumeSnapshotList{}); err != nil {
	if meta.IsNoMatchError(err) {
	// Back out if there's no point to attempt watch
	return nil
	}
	if !cc.IsErrCacheNotStarted(err) {
	return err
	}
	}

[alromeros]

By complex I mean that it requires duplicating the watches and mappers instead of having a list of supported sources, but yeah I'll change it if we prefer to handle this error.

[akalenyu]

I think this will requeue for any readyToUse snapshot. Instead, what you're after is a single case where readyToUse switches from false to true, so something like

containerized-data-importer/pkg/controller/datasource-controller.go

Lines 307 to 309 in ce1e83c

	UpdateFunc: func(e event.TypedUpdateEvent[*snapshotv1.VolumeSnapshot]) bool {
	return !reflect.DeepEqual(e.ObjectOld.Status, e.ObjectNew.Status) ||
	!reflect.DeepEqual(e.ObjectOld.Labels, e.ObjectNew.Labels)

[alromeros]

I assume that we still want to requeue any volume snapshot update even after it becomes ready? This would only allow for one single update requeue.

[akalenyu]

The controller requeues the request if there's an error/some other reason. You only have to catch the transition

[akalenyu]

I think watchOwned could also use a predicate, I believe what happens today is that any snap goes through it's EnqueueRequestsFromMapFunc