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

feat: abstract the endpoints-related logic #563

Merged
merged 20 commits into from
Jul 9, 2021

Conversation

tokers
Copy link
Contributor

@tokers tokers commented Jun 30, 2021

In this Pull Request, we abstracted all the enpoints-related logic to kube.Endpoint, which hides its real entity. These changes are used to implement the EndpointSlice controller.

Please answer these questions before submitting a pull request

  • Why submit this pull request?

  • Bugfix

  • New feature provided

  • Improve performance

  • Backport patches

  • Related issues


Bugfix

  • Description

  • How to fix?


New feature or improvement

  • Describe the details and related test reports.

Backport patches

  • Why need to backport?

  • Source branch

  • Related commits and pull requests

  • Target branch

@gxthrj
Copy link
Contributor

gxthrj commented Jul 1, 2021

Any issue related?

@tokers
Copy link
Contributor Author

tokers commented Jul 1, 2021

@gxthrj See #201 .

@gxthrj gxthrj linked an issue Jul 1, 2021 that may be closed by this pull request
@codecov-commenter
Copy link

codecov-commenter commented Jul 1, 2021

Codecov Report

Merging #563 (8969715) into master (c871bdf) will increase coverage by 0.03%.
The diff coverage is 41.17%.

❗ Current head 8969715 differs from pull request most recent head 2513b47. Consider uploading reports for the commit 2513b47 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #563      +/-   ##
==========================================
+ Coverage   34.86%   34.90%   +0.03%     
==========================================
  Files          55       55              
  Lines        4557     4558       +1     
==========================================
+ Hits         1589     1591       +2     
+ Misses       2747     2744       -3     
- Partials      221      223       +2     
Impacted Files Coverage Δ
pkg/ingress/controller.go 1.12% <0.00%> (ø)
pkg/ingress/endpoint.go 0.00% <0.00%> (ø)
pkg/kube/translation/translator.go 44.94% <76.47%> (-0.94%) ⬇️
pkg/config/config.go 84.31% <100.00%> (+0.31%) ⬆️

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 c871bdf...2513b47. Read the comment docs.

@gxthrj
Copy link
Contributor

gxthrj commented Jul 6, 2021

Please fix lint errors.

@tokers
Copy link
Contributor Author

tokers commented Jul 7, 2021

Please fix lint errors.

Fixed.

Copy link
Contributor

@gxthrj gxthrj left a comment

Choose a reason for hiding this comment

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

Great abstraction ~ with some small suggestions.

ElectionID string `json:"election_id" yaml:"election_id"`
IngressClass string `json:"ingress_class" yaml:"ingress_class"`
IngressVersion string `json:"ingress_version" yaml:"ingress_version"`
WatchEndpointSlices bool `json:"watch_endpoint_slices" yaml:"watch_endpoint_slices"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should add the configuration for (k8s.io/api/discovery/v1)LabelManagedBy ? or reuse the field IngressClass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We only care EndpointSlices managed by endpointslice-controller.k8s.io.

endpointslice.kubernetes.io/managed-by: endpointslice-controller.k8s.io

k8s.io/client-go v0.20.2
k8s.io/api v0.21.1
k8s.io/apimachinery v0.21.1
k8s.io/client-go v0.21.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Once we upgrade the version of client-go, Please make sure what is the minimum k8s version we can support, and modify the doc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As per the compatibility matrix in https://github.com/kubernetes/client-go/tree/v0.21.1#compatibility-client-go---kubernetes-clusters, we should change the minimal supported k8s version to 1.15.

@gxthrj
Copy link
Contributor

gxthrj commented Jul 8, 2021

Will the test cases of endpointslice be added in the next PR?

@tokers
Copy link
Contributor Author

tokers commented Jul 9, 2021

Will the test cases of endpointslice be added in the next PR?

I have changed the e2e framework to just use endpointslice instead of endpoints.

@gxthrj
Copy link
Contributor

gxthrj commented Jul 9, 2021

Will the test cases of endpointslice be added in the next PR?

I have changed the e2e framework to just use endpointslice instead of endpoints.

I can only see the test for endpoints, such as here

@gxthrj gxthrj merged commit a754f69 into apache:master Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support EndpointSlices
5 participants