refactor to use shared informers #373
Conversation
| sendHeartBeat(healthChan, "NPC") | ||
| } | ||
| glog.V(1).Info("Performing periodic sync of iptables to reflect network policies") | ||
| err := npc.Sync() |
There was a problem hiding this comment.
I think we can technically get rid of this because it should be covered by the resync period we pass into the informers (this was the case previously too)
There was a problem hiding this comment.
same with service and routes controller, we can probably rely on the OnUpdate events that are generated every resync period
There was a problem hiding this comment.
Alternatively keep this but set the resync period to 0 for all informers
There was a problem hiding this comment.
Writing controller that handles all the corner cases is hard :)
Ideally (in the sense of strong consistency) we recive an update and act on it. Additionally informer can be set to a resync period which will take care of any missed updates from API server for what ever reason.
If we remove periodic sync, in this scenario, there is no control loop to reconcile (eventual consistent) desired state and actual state.
Alternativley if we keep resync period to 0 then if for any reason cache/informer miss an update from API server, then it will have stale data.
AFAIK, earlier design and with your changes, controller (w.r.t resync period, and periodic loop) is similar to kube-proxy.
There was a problem hiding this comment.
I think the right thing for our case is to set resync period on informers to 0 and keep the main period sync loop we have for each controller (can be in a future PR). Reason being is that if each OnUpdate call is just calling npc.sync() then we're calling npc.sync() per resource update event when we only need to call it once.
There was a problem hiding this comment.
Right now we set resync period and run a separate syncloop which is is unnecessary.
andrewsykim
force-pushed
the
shared-informer
branch
from
c1f07bf to
1007c5a
Compare
| return errors.New("Failed to create network routing controller: " + err.Error()) | ||
| } | ||
|
|
||
| nodeInformer.AddEventHandler(nrc.NodeEventHandler) |
There was a problem hiding this comment.
I will follow up in another PR so that routes controller has service and endpoints event handlers
| npc.PodEventHandler = npc.newPodEventHandler() | ||
|
|
||
| npc.nsLister = nsInformer.GetIndexer() | ||
| npc.NamespaceEventHandler = npc.newNetworkPolicyEventHandler() |
andrewsykim
force-pushed
the
shared-informer
branch
2 times, most recently
from
7a7bb57 to
8890e44
Compare
|
There's some conflicts from #348 I'll fix it shortly |
andrewsykim
force-pushed
the
shared-informer
branch
4 times, most recently
from
629ed43 to
81a8c7c
Compare
andrewsykim
force-pushed
the
shared-informer
branch
from
81a8c7c to
d004d55
Compare
| sendHeartBeat(healthChan, "NPC") | ||
| } | ||
| glog.V(1).Info("Performing periodic sync of iptables to reflect network policies") | ||
| err := npc.Sync() |
There was a problem hiding this comment.
Writing controller that handles all the corner cases is hard :)
Ideally (in the sense of strong consistency) we recive an update and act on it. Additionally informer can be set to a resync period which will take care of any missed updates from API server for what ever reason.
If we remove periodic sync, in this scenario, there is no control loop to reconcile (eventual consistent) desired state and actual state.
Alternativley if we keep resync period to 0 then if for any reason cache/informer miss an update from API server, then it will have stale data.
AFAIK, earlier design and with your changes, controller (w.r.t resync period, and periodic loop) is similar to kube-proxy.
|
thanks @andrewsykim this is massive clean up. |
|
looks good, me and my morning coffe gives 5 sunny thumbs up |
3 participants
/app/watcherspackage and all it's global variables.WatchForCacheSyncto consolidate all calls toHasSynced