Remove vmi/vm watch and reduce default sync time to 90 seconds

[davidvossel]

Issue #100 and #78 have two things in common, they both involve issues derived from tracking VM/VMIs on external infra.

So far, we've been treating external infra as an advanced usecase, and not the default use case. The result is that external infra has been treated as a second class citizen to the use case where the capi components and VM/VMIs are running on the same k8s cluster.

I'd like to return to a single code path that satisfies the use case where the capk/capi components are running on the same cluster as the VM/VMIs (centralized infra use case) and the use case where the controller components run on a separate cluster from the VM/VMIs (external infra use case)

To achieve this, we can't assume that the VM/VMI objects are even registered on the same cluster as the capi/capk controllers. Which means we can't watch these objects by default using the default in cluster config. I propose we return back to depending on syncing the KubeVirtMachine and KubeVirtCluster objects regularly in order to pickup VM/VMI changes (basically polling). For polling to be responsive enough to pick up things like IP changes after a VM reboot, I think we should lower the default polling interval to 60 seconds.

Reduce default sync time to 90 seconds for reconcile loops

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidvossel

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:

• OWNERS [davidvossel]

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

[davidvossel]

/ok-to-test

[coveralls]

Pull Request Test Coverage Report for Build 3856719842

• 0 of 0 changed or added relevant lines in 0 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+1.2%) to 52.655%

---

Totals
Change from base Build 3599569808:	1.2%
Covered Lines:	952
Relevant Lines:	1808

---

💛 - Coveralls

[qinqon]

If think we can do both, so we have responsiveness and we don't miss stuff, what do you think ?

[davidvossel]

If think we can do both, so we have responsiveness and we don't miss stuff, what do you think ?

How would you do this.

[qinqon]

If think we can do both, so we have responsiveness and we don't miss stuff, what do you think ?

How would you do this.

You have a pair of "knobs":

• The global SyncPeriod that defaults to 10h "https://github.com/kubernetes-sigs/controller-runtime/blob/007d240d6c95ce87ba01e7a679e5b85ba294fdab/pkg/cluster/cluster.go#L98"
• And you can return at RequeueAfter at the Reconcile response https://github.com/kubernetes-sigs/controller-runtime/blob/d3120b6cf6a68705cdec8646fbc9e5b9f6855c77/pkg/reconcile/reconcile.go#L33

I think with both you can have both features.

[davidvossel]

I think with both you can have both features.

The problem is that we need the capk controller to be able to function when the VM/VMI objects are not registered in the infra cluster. The watches fail when there are no VM/VMI crds registered.

I could dynamically detect of vm/vmis are present at launch, and only watch if they are, but i'd rather have a single code path to test rather than multiple (one when crds are available, one when they are not). So just using a reduced sync period with no watches seems to satisfy that requirement (at the cost of efficiency, which might be okay for us at our current scale)

[qinqon]

I think with both you can have both features.

The problem is that we need the capk controller to be able to function when the VM/VMI objects are not registered in the infra cluster. The watches fail when there are no VM/VMI crds registered.

I could dynamically detect of vm/vmis are present at launch, and only watch if they are, but i'd rather have a single code path to test rather than multiple (one when crds are available, one when they are not). So just using a reduced sync period with no watches seems to satisfy that requirement (at the cost of efficiency, which might be okay for us at our current scale)

An alternative is to run the polling for external clusters and as a result enqueue to the controller so they enter the Reconcile function, this way we don't have to remove the watchers.

[qinqon]

/lgtm
Let's hope we don't hit control plane too much.