fix: re-list when target change #1195

LaurenceLiZhixin · 2023-02-07T09:32:12Z

Force re-list pool-scope resource when proxy change between loadbalancer and pool-coordinator

codecov · 2023-02-07T09:42:22Z

Codecov Report

Merging #1195 (5636359) into master (c524116) will decrease coverage by 0.20%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1195      +/-   ##
==========================================
- Coverage   52.35%   52.16%   -0.20%     
==========================================
  Files         106      106              
  Lines       13852    13880      +28     
==========================================
- Hits         7252     7240      -12     
- Misses       5952     5991      +39     
- Partials      648      649       +1

Flag	Coverage Δ
unittests	`52.16% <0.00%> (-0.20%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/yurthub/proxy/remote/loadbalancer.go	`12.54% <0.00%> (ø)`
pkg/yurthub/proxy/util/util.go	`62.88% <0.00%> (-5.91%)`	⬇️
pkg/yurthub/storage/etcd/storage.go	`77.35% <0.00%> (-3.53%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Congrool · 2023-02-07T10:11:48Z

I still have some questions for such problem:

client sent list/watch requests and they were handled by yurthub. Then the yurthub crashed and restarted. And at this time, the handler changed(from pool-coordinator to cloud APIServer or vice versa), will the client relist the resource?
When the long time watch connection break is caused by the server-side(e.g. cloud-edge network breaks or pool-coordinator restarted), will yurthub always make the client relist?

LaurenceLiZhixin · 2023-02-08T05:36:53Z

Thanks for your great comments @Congrool

After my tests, yurthub restart will cause all existing watch requests to be disconnected from client side, and the client will trigger a re-list-watch. If the restart process occurs with the flow change from edge to cloud, after the stability, yurthub got ready coordinator status, and yurthub will cut the cloud to the edge again , the client-side re-list-watch will still be triggered by the error event injection provided by this pr.

If the cloud edge network is disconnected: If yurthub is directly connected to the cloud, the edge pool-coordinator is unavailable. Therefore, there is no problem.
If the pool-coordinator restarts, the existing watch requests are cut and the client re-list-watch. Then yurthub will determine that a pool-coordinator is abnormal, and trigger a re-list-watch for all existing watches through the error event injection provided by this pr, and then client side list-watch agent and yurthub proxy to the cloud. After the pool-coordinator restarted, the flow is changed to local and all re-list-watch are triggered again. There will be no problem after the experiment.

Congrool · 2023-02-08T06:19:44Z

Sounds good to me

Congrool · 2023-02-08T06:18:38Z

pkg/yurthub/proxy/remote/loadbalancer.go

-					if lb.coordinatorGetter == nil {
-						continue
-					}
+					klog.Infof("lb.coordinatorGetter is %s", lb.coordinatorGetter)


We may not need such log, please remove

Congrool · 2023-02-08T06:18:49Z

pkg/yurthub/proxy/remote/loadbalancer.go

 					coordinator := lb.coordinatorGetter()
 					if coordinator == nil {
+						klog.Infof("lb.coordinatorGetter().coordinator is nil")


Congrool · 2023-02-08T06:19:00Z

pkg/yurthub/proxy/remote/loadbalancer.go

 						cloudServeCancel()
 						return
 					}
+					klog.Infof("coordinator.IsReady() is false")


Congrool · 2023-02-08T06:20:58Z

Could you please squash your commits to make the commit message more readable?

Congrool · 2023-02-09T05:48:00Z

Another question, is it possible that:
list request is first handled by one handler(for example cloud APIServer), but the later watch request is handled by another handler(for pool-coordinator becoming healthy)

Will it still have some problems in such situation?

LaurenceLiZhixin · 2023-02-09T06:19:22Z

Another question, is it possible that: list request is first handled by one handler(for example cloud APIServer), but the later watch request is handled by another handler(for pool-coordinator becoming healthy)

Will it still have some problems in such situation?

Yes, that's still a problem. But it's very rare. I think we can talk about it and fix it totally later.

…hange(cloud to pool-coor or pool-coor to cloud)

LaurenceLiZhixin · 2023-02-09T06:35:35Z

I've fixed your great comments above. @Congrool

Congrool · 2023-02-10T02:32:06Z

LGTM as a solution for most cases, thanks for your contribution!

/lgtm

openyurt-bot · 2023-02-10T02:38:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LaurenceLiZhixin, rambohe-ch

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [rambohe-ch]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

rambohe-ch · 2023-02-17T02:23:01Z

/backport release-v1.2

github-actions · 2023-02-17T02:23:22Z

Successfully created backport PR for release-v1.2:

[Backport release-v1.2] fix: re-list when target change #1238

openyurt-bot requested review from Congrool and kadisi February 7, 2023 09:32

openyurt-bot added the size/M size/M: 30-99 label Feb 7, 2023

Congrool reviewed Feb 8, 2023

View reviewed changes

fix: pool-coordinator ask client to re-list-watch when proxy target c…

5636359

…hange(cloud to pool-coor or pool-coor to cloud)

LaurenceLiZhixin force-pushed the fix-re-list-bug branch from acafc79 to 5636359 Compare February 9, 2023 06:34

openyurt-bot assigned Congrool Feb 10, 2023

openyurt-bot added the lgtm lgtm label Feb 10, 2023

rambohe-ch approved these changes Feb 10, 2023

View reviewed changes

rambohe-ch merged commit 08e1619 into openyurtio:master Feb 10, 2023

openyurt-bot added the approved approved label Feb 10, 2023

rambohe-ch added the backport release-v1.2 backport release-v1.2 label Feb 17, 2023

github-actions bot mentioned this pull request Feb 17, 2023

[Backport release-v1.2] fix: re-list when target change #1238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: re-list when target change #1195

fix: re-list when target change #1195

LaurenceLiZhixin commented Feb 7, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

Congrool commented Feb 7, 2023 •

edited

Loading

LaurenceLiZhixin commented Feb 8, 2023

Congrool commented Feb 8, 2023

Congrool Feb 8, 2023

LaurenceLiZhixin Feb 9, 2023

Congrool Feb 8, 2023

Congrool Feb 8, 2023

Congrool commented Feb 8, 2023

Congrool commented Feb 9, 2023

LaurenceLiZhixin commented Feb 9, 2023

LaurenceLiZhixin commented Feb 9, 2023

Congrool commented Feb 10, 2023

openyurt-bot commented Feb 10, 2023

rambohe-ch commented Feb 17, 2023

github-actions bot commented Feb 17, 2023

fix: re-list when target change #1195

fix: re-list when target change #1195

Conversation

LaurenceLiZhixin commented Feb 7, 2023

codecov bot commented Feb 7, 2023 • edited Loading

Codecov Report

Congrool commented Feb 7, 2023 • edited Loading

LaurenceLiZhixin commented Feb 8, 2023

Congrool commented Feb 8, 2023

Congrool Feb 8, 2023

Choose a reason for hiding this comment

LaurenceLiZhixin Feb 9, 2023

Choose a reason for hiding this comment

Congrool Feb 8, 2023

Choose a reason for hiding this comment

Congrool Feb 8, 2023

Choose a reason for hiding this comment

Congrool commented Feb 8, 2023

Congrool commented Feb 9, 2023

LaurenceLiZhixin commented Feb 9, 2023

LaurenceLiZhixin commented Feb 9, 2023

Congrool commented Feb 10, 2023

openyurt-bot commented Feb 10, 2023

rambohe-ch commented Feb 17, 2023

github-actions bot commented Feb 17, 2023

codecov bot commented Feb 7, 2023 •

edited

Loading

Congrool commented Feb 7, 2023 •

edited

Loading