Is the use of shared informers within virt-api still recommended?

[Lee Yarwood]

/cc kubevi...@googlegroups.com

Hey Roman,

Hope you're well, just a quick question regarding some feedback you gave
in a PR a while ago around the use of shared informers in virt-api [1].

I was skimming over the reviewer guide [2] and noticed that it still
contains the following suggestions regarding their use:

> * Avoid using api GETs/LISTs to retrieve an object from the api-server
> when an informer is more appropriate. In general, informers should be
> used in cluster wide components such as virt-controller, virt-api, and
> virt-operator.
>
> [..]
>
> * Avoid adding informers to node level components such as
> virt-handler. This causes api-server pressure at scale.

Before I post any changes to this I just wanted to make sure I
understood your comments correctly in my original PR. Are you advocating
for the removal of shared informers from the virt-api webhooks entirely
and for the cluster wide components to always use client calls instead?

FWIW I have already followed up and switched the instancetype and
preference informers over to client calls [3], hopefully that's in-line
with your views on their use.

Many thanks in advance,

Lee

[1] https://github.com/kubevirt/kubevirt/pull/7806#issuecomment-1153577569
[2] https://github.com/kubevirt/kubevirt/blob/main/docs/reviewer-guide.md#common-architecture-flaws-to-avoid
[3] https://github.com/kubevirt/kubevirt/pull/7935
--
Lee Yarwood

[Roman Mohr]

Hi Lee,

thanks for spotting this.

On Wed, Sep 28, 2022 at 11:59 AM Lee Yarwood <lyar...@redhat.com> wrote:

/cc kubevi...@googlegroups.com

Hey Roman,

Hope you're well, just a quick question regarding some feedback you gave
in a PR a while ago around the use of shared informers in virt-api [1].

I was skimming over the reviewer guide [2] and noticed that it still
contains the following suggestions regarding their use:

> * Avoid using api GETs/LISTs to retrieve an object from the api-server
>   when an informer is more appropriate. In general, informers should be
>   used in cluster wide components such as virt-controller, virt-api, and
>   virt-operator.

Yep, that needs an update. For virt-api it is almost always wrong to use an informer. For virt-controller and virt-operator it is almost always correct to use informers.

The main issues with informers and virt-api:

 * virt-api can freely scale, it does not use leader election. Therefore the apiserver load increases with each replica
 * one issue we had illustrating the complexities: if virt-api would watch VMIs to verify something when a VM gets created, it would get on every VMI start 8 VMI objects transferred (status updates), while at the same time not a single webhook review happens. It is then much cheaper to just do a single REST call for a specific VMI, since numbers grow fast: Creating 200 VMs with VMI informers would lead to 200 REST calls by the user creating the VM, but then additionally to 8*200 (1600) VMI watcher transfers for each replica. Since we run two virt-apis by default, that means 3200 VMI transfers. When doing the rest-call for a specific VMI directly,  we end up with only 200 VMI transfers.

For virt-handler it is a little bit special: I should only use an informer for VMIs, and try to talk as little as possible with the apiserver if it needs additional data (like e.g. periodic heart-beat to node object). Here we want to use as little informers as possible, because the number of virt-handlers correlates with the number of nodes. We then either transfer e.g. all VMIs to each node, or set a label filter for the node, which has scaling bottle-necks in etcd itself. Therefore we only do it once for VMIs.

Let me know if something is still unclear.

Best regards,

Roman 
 

[Lee Yarwood]

On 28-09-22 12:12:51, Roman Mohr wrote:
> Hi Lee,
>
> thanks for spotting this.
>
> On Wed, Sep 28, 2022 at 11:59 AM Lee Yarwood <lyar...@redhat.com> wrote:
>
> > /cc kubevi...@googlegroups.com
> >
> > Hey Roman,
> >
> > Hope you're well, just a quick question regarding some feedback you gave
> > in a PR a while ago around the use of shared informers in virt-api [1].
> >
> > I was skimming over the reviewer guide [2] and noticed that it still
> > contains the following suggestions regarding their use:
> >
> > > * Avoid using api GETs/LISTs to retrieve an object from the api-server
> > > when an informer is more appropriate. In general, informers should be
> > > used in cluster wide components such as virt-controller, virt-api, and
> > > virt-operator.
> >
>
> Yep, that needs an update. For virt-api it is almost always wrong to use an
> informer. For virt-controller and virt-operator it is almost always correct
> to use informers.

ACK, so my removal of all use of informers for instancetypes and
preferences in [3] was slightly over the top. I can look at
reintroducing their use in virt-controller at least.

> The main issues with informers and virt-api:
>
> * virt-api can freely scale, it does not use leader election. Therefore
> the apiserver load increases with each replica
> * one issue we had illustrating the complexities: if virt-api would watch
> VMIs to verify something when a VM gets created, it would get on every VMI
> start 8 VMI objects transferred (status updates), while at the same time
> not a single webhook review happens. It is then much cheaper to just do a
> single REST call for a specific VMI, since numbers grow fast: Creating 200
> VMs with VMI informers would lead to 200 REST calls by the user creating
> the VM, but then additionally to 8*200 (1600) VMI watcher transfers for
> each replica. Since we run two virt-apis by default, that means 3200 VMI
> transfers. When doing the rest-call for a specific VMI directly, we end up
> with only 200 VMI transfers.
>
> For virt-handler it is a little bit special: I should only use an informer
> for VMIs, and try to talk as little as possible with the apiserver if it
> needs additional data (like e.g. periodic heart-beat to node object). Here
> we want to use as little informers as possible, because the number of
> virt-handlers correlates with the number of nodes. We then either transfer
> e.g. all VMIs to each node, or set a label filter for the node, which has
> scaling bottle-necks in etcd itself. Therefore we only do it once for VMIs.
>
> Let me know if something is still unclear.

No I think I understand now, thanks for the prompt reply.

I'm happy to post a PR updating this section of the review guide if you
don't have time btw.

Many thanks again,

[Roman Mohr]

On Wed, Sep 28, 2022 at 12:24 PM Lee Yarwood <lyar...@redhat.com> wrote:

On 28-09-22 12:12:51, Roman Mohr wrote:
> Hi Lee,
>
> thanks for spotting this.
>
> On Wed, Sep 28, 2022 at 11:59 AM Lee Yarwood <lyar...@redhat.com> wrote:
>
> > /cc kubevi...@googlegroups.com
> >
> > Hey Roman,
> >
> > Hope you're well, just a quick question regarding some feedback you gave
> > in a PR a while ago around the use of shared informers in virt-api [1].
> >
> > I was skimming over the reviewer guide [2] and noticed that it still
> > contains the following suggestions regarding their use:
> >
> > > * Avoid using api GETs/LISTs to retrieve an object from the api-server
> > >   when an informer is more appropriate. In general, informers should be
> > >   used in cluster wide components such as virt-controller, virt-api, and
> > >   virt-operator.
> >
>
> Yep, that needs an update. For virt-api it is almost always wrong to use an
> informer. For virt-controller and virt-operator it is almost always correct
> to use informers.

ACK, so my removal of all use of informers for instancetypes and
preferences in [3] was slightly over the top. I can look at
reintroducing their use in virt-controller at least.

Yes! virt-controller can and should make heavy use of informers. Let's reintroduce these informers. Controllers typically have to use informers, since they react on every object change and would then query heavily. Here it is exactly the other way round than on virt-api, because we want to react on e.g. every VM change and see if things are right. 

Lets suppose we have a VMI controller which has to check DataVolumes. The VMI controller wakes up on every VMI status update (and it should to see if everything is alright). Here for e.g. all 8 status updates on the VMI we need to do work, and they would now have to fetch the DataVolumes 8 times. Due to the high check-frequency and the fact that it does leader-election and only one instance is actually watching, it is much cheaper to keep a cache up-to-date upfront.

Best regards,

Roman

[Lee Yarwood]

ACK many thanks again, I've posted the following draft PR that's based
on a larger series including various refactors that I didn't want to block:

Instancetype: Reintroduce informers in virt controller
https://github.com/kubevirt/kubevirt/pull/8537

I've also followed up with a PR to update the reviewer guide below:

reviewer-guide: Update notes on use of informers
https://github.com/kubevirt/kubevirt/pull/8538

Again many thanks for your help here Roman!

Cheers,

Lee

--
Lee Yarwood