Skip to content
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

add endpoint slice handler #435

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

lmxia
Copy link
Contributor

@lmxia lmxia commented Jul 29, 2022

Signed-off-by: lmxia xialingming@gmail.com

What type of PR is this?

kind/feature

What this PR does / why we need it:

Add endpoint slice event handler.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Add remove endpoint slices logic, if endpoint slices exist in parent cluster but not in child cluster.

@lmxia lmxia requested a review from a team as a code owner July 29, 2022 05:20
@lmxia lmxia requested a review from dixudx July 29, 2022 05:20
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #435 (640d33c) into main (73325a4) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 640d33c differs from pull request most recent head 81deb97. Consider uploading reports for the commit 81deb97 to get more accurate results

@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   13.72%   13.74%   +0.02%     
==========================================
  Files          61       61              
  Lines        6595     6607      +12     
==========================================
+ Hits          905      908       +3     
- Misses       5623     5629       +6     
- Partials       67       70       +3     
Impacted Files Coverage Δ
.../framework/plugins/defaultbinder/default_binder.go 61.76% <0.00%> (-20.06%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73325a4...81deb97. Read the comment docs.

@dixudx dixudx added this to the v0.12.0 milestone Jul 29, 2022
@dixudx dixudx added the kind/feature New feature or request label Jul 29, 2022
@lmxia lmxia force-pushed the feat/add-endpointslice-handler branch from 640d33c to 81deb97 Compare July 30, 2022 02:58
LabelSelector: labels.SelectorFromSet(labels.Set{discoveryv1.LabelServiceName: utils.DerivedName(namespace, seName)}).String()}); err == nil {
shouldRetry := false
for _, item := range parentEndpointSliceList.Items {
if !localEndpointSliceMap[item.Name] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be right, since L144 uses namespaced name as keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be right, since L144 uses namespaced name as keys.

On L144, the enpointslice item is in child cluster, and the name is raw enpointslice name, like my-service-pt9nk.

While here, L149, the item is in parent cluster enpointslice, the name is default-my-service-pt9nk formated on L270.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I got it.

This is a little bit tricky. Maybe we should add some comments here. Feel free to improve this in a new PR.

pkg/controllers/mcs/serviceexport_controller.go Outdated Show resolved Hide resolved
Signed-off-by: lmxia <xialingming@gmail.com>
@lmxia lmxia force-pushed the feat/add-endpointslice-handler branch from 81deb97 to 1cd55ee Compare August 1, 2022 09:52
@dixudx dixudx merged commit 8d184ff into clusternet:main Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants