federation service controller: fixing a bug so that existing services are created in newly registered clusters #27028
Conversation
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
2 similar comments
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test". This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry. Otherwise, if this message is too spammy, please complain to ixdy. |
|
@quinton-hoole please review if this should be put to 1.3, thanks. |
k8s-github-robot
added
size/S
mfanjie
force-pushed
the
fix_cluster_sync_loop
branch
2 times, most recently
from
5e1e5a8 to
41bfd46
Compare
mfanjie
force-pushed
the
fix_cluster_sync_loop
branch
from
dc231ca to
640d7dc
Compare
|
@nikhiljindal I know it is in critical time, but I think this PR is worthy and safe to merge. Without this fix, the existing services cannot be created in new registered clusters. |
| @@ -638,7 +638,7 @@ func (s *ServiceController) updateAllServicesToCluster(services []*cachedService | |||
| cluster, ok := s.clusterCache.clientMap[clusterName] | |||
| if ok { | |||
| for _, cachedService := range services { | |||
| appliedState := cachedService.appliedState | |||
| appliedState := cachedService.lastState | |||
There was a problem hiding this comment.
same. can you explain why this change?
Will also be good to add a comment here in code explaining why we are using lastState and not appliedState.
|
ok to test |
|
cc @kubernetes/sig-cluster-federation Also cc @madhusudancs since he is writing services e2e. Will be good to include this case there. |
|
GCE e2e build/test passed for commit 640d7dc. |
|
Added to 1.3 milestone |
|
The lastState is assigned when service controller catch the service event, the appliedState is assigned after service controller process the event. When a new cluster added, a service that just be created but not be processed by service controller should also be applied to that cluster. So lastState should be used. clusterSyncPeriod to second before the cluster sync loop interval is 10 minutes which is too long for users to wait for all services to apply to this cluster it should be 1 second interval like other loops. |
ghost
added
the
lgtm
k8s-github-robot
removed
the
lgtm
|
thanks for the explanation @mfanjie |
nikhiljindal
changed the title
nikhiljindal
added
lgtm
|
@nikhiljindal why this is P1? |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e build/test passed for commit 640d7dc. |
|
Automatic merge from submit-queue |
ghost
mentioned this pull request
A defect on federation service controller.
Steps to recreate:
Root cause:
the right sequence should be
then
then
Now the first two lines' sequence is on the contrary, so when updateDNSRecords return no error, the service will be removed from servicesToUpdate, and updateAllServicesToCluster get nothing to update.
This PR make the call sequence correct.