Avoid ServiceCIDR flapping on agent start #5017
Conversation
tnqn
added
the
action/release-note
tnqn
force-pushed
the
stabilize-service-cidr
branch
from
90d5e82
to
206e5c3
Compare
| return | ||
| } | ||
| svcs, _ := d.serviceLister.List(labels.Everything()) | ||
| d.updateServiceCIDR(svcs...) |
There was a problem hiding this comment.
Here all existing Services are listed, and a calculated Service CIDR will be got, how could we clean the stale Service routes? Previously, we use the first ClusterIP to collect all routes whose destination includes the ClusterIP.
There was a problem hiding this comment.
Previous cleanup logic is not reliable, there is no guarantee the first clusterIP has been covered the previous routes. Please check the latest code about how stale routes are collected.
tnqn
force-pushed
the
stabilize-service-cidr
branch
3 times, most recently
from
569025d
to
00ec216
Compare
tnqn
force-pushed
the
stabilize-service-cidr
branch
from
00ec216
to
d39f100
Compare
The previous implementation always generated intermediate values for ServiceCIDR on agent start, which may interrupt the Service traffic and causes difficulty for cleaning up stale routes as the value calculated at one point may not be reliable to identify all stale routes. This commit waits for the Service Informer to be synced first, and calculates the ServiceCIDR based on all Services. Ideally the Service route won't change in most cases, and hence avoid the above issues. Besides, it fixes an issue that stale routes on Linux were not cleaned up correctly due to incorrect check. Signed-off-by: Quan Tian <qtian@vmware.com>
tnqn
force-pushed
the
stabilize-service-cidr
branch
from
d39f100
to
741df1b
Compare
|
/test-all |
|
|
||
| func (d *Discoverer) updateServiceCIDR(svcs ...*corev1.Service) { |
There was a problem hiding this comment.
It's not clear to me that the current implementation of updateServiceCIDR will always be correct if multiple workers / goroutines can call updateServiceCIDR simultaneously?
That being said, it should not matter given that you start a single worker goroutine in Run.
There was a problem hiding this comment.
It may not work if it's called simultaneously. There is no need to have multiple workers for this task as it just checks if an IP is in a subnet in most cases (CIDR expansion can only happen few times).
|
@hongliangl do you have other comments for this one? |
The previous implementation always generated intermediate values for ServiceCIDR on agent start, which may interrupt the Service traffic and causes difficulty for cleaning up stale routes as the value calculated at one point may not be reliable to identify all stale routes. This commit waits for the Service Informer to be synced first, and calculates the ServiceCIDR based on all Services. Ideally the Service route won't change in most cases, and hence avoid the above issues. Besides, it fixes an issue that stale routes on Linux were not cleaned up correctly due to incorrect check. Signed-off-by: Quan Tian <qtian@vmware.com>
The previous implementation always generated intermediate values for ServiceCIDR on agent start, which may interrupt the Service traffic and causes difficulty for cleaning up stale routes as the value calculated at one point may not be reliable to identify all stale routes.
This commit waits for the Service Informer to be synced first, and calculates the ServiceCIDR based on all Services. Ideally the Service route won't change in most cases, and hence avoid the above issues.
Besides, it fixes an issue that stale routes on Linux were not cleaned up correctly due to incorrect check.