-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework spidercoordinator informer to update pod and service cidr #3249
Rework spidercoordinator informer to update pod and service cidr #3249
Conversation
178617a
to
55f5e8d
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3249 +/- ##
=======================================
Coverage 81.12% 81.12%
=======================================
Files 50 50
Lines 5373 5373
=======================================
Hits 4359 4359
Misses 856 856
Partials 158 158
Flags with carried forward coverage won't be shown. Click here to find out more. |
1524d0c
to
7573699
Compare
cd1ce62
to
18b48b6
Compare
8a71100
to
7e494e4
Compare
99cc9ec
to
8238f05
Compare
The responsibility of the SpiderCoordinator Informer is to monitor changes in the Pod and Service CIDR of the cluster and automatically update them in the Status of SpiderCoordinator. However, due to different sources of Pod and Service CIDR in different cluster environments, a uniform approach cannot be adopted. Additionally, to avoid coroutine leaks and confusion in updating status. Signed-off-by: cyclinder <qifeng.guo@daocloud.io>
8238f05
to
54a92bc
Compare
@@ -60,11 +60,14 @@ type CoordinatorStatus struct { | |||
// +kubebuilder:validation:Requred | |||
Phase string `json:"phase"` | |||
|
|||
// +kubebuilder: validation:Optional | |||
Reason string `json:"reason,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里我的想法: 一个 omitempty 足够满足需求,这里不需要考虑默认值和赋空值的区别,这里的需求是reason 为空,不显示,不为空就显示,比较简单。如果为指针类型,还需要处理一些 nil 的情况,没有必要
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…dinator_informer Rework spidercoordinator informer to update pod and service cidr
The responsibility of the SpiderCoordinator Informer is to monitor changes in the Pod and Service CIDR of the cluster and automatically update them in the Status of SpiderCoordinator. However, due to different sources of Pod and Service CIDR in different cluster environments, a uniform approach cannot be adopted. Additionally, to avoid coroutine leaks and confusion in updating status,
Thanks for contributing!
What type of PR is this?
What this PR does / why we need it:
This PR addresses the following four main issues:
Avoiding goroutine leakage and avoiding frequent goroutine creation and closure affecting performance. In the previous version, every time the spidercoordiantor's instance is updated, it will close and restart the goroutine that listens to the pod and the service, which may affect the performance or even leak the goroutine in large-scale case.
Unify the updating of spidercoordinator status in each reconcile event of spidercoordinator, instead of updating it in different places. Avoid confusing or even unsynchronized status updates.
Avoid unnecessary reconcile events, Reduce requests to interact with the APISever.
Add a "Reason" field to SpiderCoordinator's status, which can explain why the Phase is
NotReady
.Which issue(s) this PR fixes:
Fixes #3225
Fixes #3210
Special notes for your reviewer: