-
Notifications
You must be signed in to change notification settings - Fork 254
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
Start smart clone controller from datavolume controller when needed #2265
Start smart clone controller from datavolume controller when needed #2265
Conversation
cmd/cdi-controller/controller.go
Outdated
@@ -288,7 +289,7 @@ func deleteReadyFile() { | |||
} | |||
|
|||
func addCrdInformerEventHandlers(crdInformer cache.SharedIndexInformer, extclient extclientset.Interface, mgr manager.Manager, log logr.Logger) { | |||
crdInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | |||
crdInformer.AddEventHandlerWithResyncPeriod(cache.ResourceEventHandlerFuncs{ | |||
AddFunc: func(obj interface{}) { |
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.
Should handle UpdateFunc
as well as AddFunc
as I believe UpdateFunc
is called on resync
cmd/cdi-controller/controller.go
Outdated
} | ||
|
||
func startSmartController(extclient extclientset.Interface, mgr manager.Manager, log logr.Logger) { | ||
if controller.IsCsiCrdsDeployed(extclient) { | ||
if controller.IsCsiCrdsDeployed(extclient, log) { | ||
log.Info("CSI CRDs detected, starting smart clone controller") | ||
if _, err := controller.NewSmartCloneController(mgr, log, installerLabels); 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.
Have to make sure to not do this multiple times I think
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, this probably worked because we never tried to do it more than once.
fdec6b2
to
c751542
Compare
What about panicking when this GET fails? just putting it out there, curious as to why you ruled it out? This GET failing is an indication of pretty bad things going on right? |
It could be a transient network issue for instance, no reason to panic. In essence a panic would cause the control plane of CDI to disappear for some period of time, which could add to whatever causing the transient failure. So why not handle it properly. |
c751542
to
4d9f1a3
Compare
/retest |
@@ -291,6 +294,7 @@ func NewDatavolumeController( | |||
if err := addDatavolumeControllerWatches(mgr, datavolumeController); err != nil { | |||
return nil, err | |||
} | |||
go reconciler.startSnapshotCsiCrdDetection() |
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 would be nicer, rather than creating our own goroutine, let the controller runtime manager deal with it. See here: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/manager/manager.go#L61
Basically just pass a Runnable and when the manager is started, it will create/manage the goroutine.
I believe this will clean up all the funniness in the unit test with the dummy goroutine and passing the additional 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.
We take advantage of this in cdi-operator here: https://github.com/kubevirt/containerized-data-importer/blob/main/pkg/operator/controller/certrotation.go#L80
It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly. This adds a periodic sync of 1 minute for checking the CRDs. We also log failures that are not is not found so we can more easily detect this situation as humans. Signed-off-by: Alexander Wels <awels@redhat.com>
4d9f1a3
to
f68376f
Compare
log.Info("Missing CSI snapshotter CRDs, falling back to host assisted clone") | ||
return "", nil | ||
} | ||
r.sccs.StartController() |
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 seems a little too hidden in this function, how about here:
Check if strategy is smart clone and start controller if so. In this location (main reconcile func) it is more explicit, no weird side effects
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.
But we should only start the smart clone controller if the snapshot CRDs are there. I guess us creating the controller will fail because the watches won't work, and we can try again later.
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.
We should not choose smart clone strategy without checking the CRDs. Not sure what you mean about failing.
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 mean if we do manage to attempt to start the smart clone controller, it will fail when we add the watches in the controller because the CRDs don't exist. but you make a good point that we won't pick that strategy without checking if the CRDs exist. I already fixed it btw.
Signed-off-by: Alexander Wels <awels@redhat.com>
/retest |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mhenriks 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 |
/cherrypick release-v1.43 |
@awels: #2265 failed to apply on top of branch "release-v1.43":
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. |
…ubevirt#2265) * periodic sync CSI snapshot CRD check It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly. This adds a periodic sync of 1 minute for checking the CRDs. We also log failures that are not is not found so we can more easily detect this situation as humans. Signed-off-by: Alexander Wels <awels@redhat.com> * Change location of the start controller call. Signed-off-by: Alexander Wels <awels@redhat.com>
…ubevirt#2265) * periodic sync CSI snapshot CRD check It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly. This adds a periodic sync of 1 minute for checking the CRDs. We also log failures that are not is not found so we can more easily detect this situation as humans. Signed-off-by: Alexander Wels <awels@redhat.com> * Change location of the start controller call. Signed-off-by: Alexander Wels <awels@redhat.com>
…2265) (#2271) * periodic sync CSI snapshot CRD check It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly. This adds a periodic sync of 1 minute for checking the CRDs. We also log failures that are not is not found so we can more easily detect this situation as humans. Signed-off-by: Alexander Wels <awels@redhat.com> * Change location of the start controller call. Signed-off-by: Alexander Wels <awels@redhat.com>
…ubevirt#2265) * periodic sync CSI snapshot CRD check It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly. This adds a periodic sync of 1 minute for checking the CRDs. We also log failures that are not is not found so we can more easily detect this situation as humans. Signed-off-by: Alexander Wels <awels@redhat.com> * Change location of the start controller call. Signed-off-by: Alexander Wels <awels@redhat.com>
…er when needed #2265 (#2307) * Start smart clone controller from datavolume controller when needed (#2265) * periodic sync CSI snapshot CRD check It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly. This adds a periodic sync of 1 minute for checking the CRDs. We also log failures that are not is not found so we can more easily detect this situation as humans. Signed-off-by: Alexander Wels <awels@redhat.com> * Change location of the start controller call. Signed-off-by: Alexander Wels <awels@redhat.com> * Modifications needed for backport. In particular added check for both betav1 and v1 crds. Signed-off-by: Alexander Wels <awels@redhat.com>
Signed-off-by: Alexander Wels awels@redhat.com
What this PR does / why we need it:
It was possible for the CSI snapshot CRD check to fail silently and prevent the smart clone controller from starting during the cdi deployment pod start up. This would prevent smart clone from working properly.
This moves the creation of the smart clone controller into the datavolume controller when someone attempts to do a smart clone. Until that happens the smart clone controller is not started at all. To ensure we don't have multiple smart clone controllers started, we use a channel to synchronize creation, and once created the function will be a no-op.
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:
Release note: