Trigger NEG syncers to sync on Node Topology CRD changes. #2677
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
sawsa307
force-pushed
the
trigger-sync-on-crd
branch
from
40a6d13
to
ac07d11
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sawsa307 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sawsa307
force-pushed
the
trigger-sync-on-crd
branch
from
ac07d11
to
a7658e5
Compare
* Add NodeInformer to NewController, and add fake informer to test cases.
sawsa307
force-pushed
the
trigger-sync-on-crd
branch
from
a7658e5
to
8f0a499
Compare
k8s-ci-robot
added
size/L
* When zones or subnets of a cluster changes, all syncers react to this change by either creating additional NEGs in new zone/subnet, or mark NEGs as Inactive/To-be-deleted.
sawsa307
force-pushed
the
trigger-sync-on-crd
branch
from
8f0a499
to
8f92fac
Compare
|
/retest |
There was a problem hiding this comment.
Have left a few comments and have one additional one:
Have we considered starting a filtered informer for NodeTopology which only informs changes to a specific resource? You can make the name of that resource pluggable through a flag to get some flexibility.
I can see why choosing the option of having an un-filtered informer might offer more flexibility for future use cases, where things might work naturally if you ever have multiple NodeTopology resources. But on the other hand, I also feel that if such a case actually arises, it would warrant some additional thought and verification (like maybe not syncing all syncers for each NodeTopology resource? or something like that)
This specific aspect may benefit from getting an additional opinion (@swetharepakula)
| @@ -322,6 +324,27 @@ func NewController( | |||
| } | |||
| }, | |||
| }) | |||
| if flags.F.EnableMultiSubnetClusterPhase1 { | |||
| nodeTopologyInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | |||
There was a problem hiding this comment.
If we were to add a very simple unit test which verifies transitions to the NodeTopology CR triggering sync or not, I think we may spot a bug :)
| @@ -322,6 +324,27 @@ func NewController( | |||
| } | |||
| }, | |||
| }) | |||
| if flags.F.EnableMultiSubnetClusterPhase1 { | |||
| nodeTopologyInformer.AddEventHandler(cache.ResourceEventHandlerFuncs{ | |||
There was a problem hiding this comment.
Any reason to not watch for AddEvents?
(For not having DeleteEvents, that does somewhat make sense)
| subnetChanged = true | ||
| } | ||
| if zoneChanged || subnetChanged { | ||
| manager.SyncAllSyncers() |
There was a problem hiding this comment.
Please add a comment here, noting that we are fine with syncing all syncers within the same goroutine as the informer-handler because we don't expect this CR to change very frequently.
Alternatively, if that is not the case, we should likely be using an intermediate queue here (like we do for the rest of the informers)
/assign @swetharepakula
/assign @gauravkghildiyal