-
Notifications
You must be signed in to change notification settings - Fork 590
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
chore(K8s): migrate from CoreV1 Endpoints to DiscoveryV1 EndpointSlice #3997
chore(K8s): migrate from CoreV1 Endpoints to DiscoveryV1 EndpointSlice #3997
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #3997 +/- ##
=======================================
- Coverage 59.7% 59.6% -0.1%
=======================================
Files 149 149
Lines 16399 16406 +7
=======================================
- Hits 9793 9782 -11
- Misses 5970 5987 +17
- Partials 636 637 +1
☔ View full report in Codecov by Sentry. |
0064eab
to
bcd2b3e
Compare
d59d2dc
to
c89e77e
Compare
6e54349
to
e40ef40
Compare
e40ef40
to
ce86d95
Compare
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.
2 things that I'd like to raise:
- the left over issues that you mention: ideally we'd have
issues for those so that we don't loose track of those
- I believe we should add a changelog entry for this because
- we'll have a trace in history (apart from git log) stating where we've changed from one to another
- some users might be genuinely interested in this change, so let's now hide it from them
When you're ready with this you might want to add |
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.
Generally LGTM, only some minor points remaining. Also, Since the user-facing manifests are changed, I think this needs an update on CHANGELOG.
ce86d95
to
4230094
Compare
@programmer04 There seem to be some failures in GKE e2e tests 🤔 I'll be happy to help you out in debugging them. |
1751081
to
0a2fec4
Compare
GKE e2e tests failed, because we don't have mechanism of building&loading images from PR to GKE clusters, there is issue KTF #587 to implement this. Curently it uses an image from nightlies that does not include changes from this PR |
As discussed on zoom: I'm OK merging this and seeing the results of tests the next day but I'd suggest we invest in the GKE image loading so that we don't need to do this in the future. The fact that this works on |
0a2fec4
to
2829cbf
Compare
What this PR does / why we need it:
EndpointSlice API is available as V1 since K8s 1.21. KIC 2.9's minimum supported K8s version is 1.22.
Which issue this PR fixes:
#3916
Leftovers to address separately
Special notes for your reviewer:
K8s permissions - get rid of
Endpoints
,EndpointSlices
are already allowed -get
(from annotation),watch
,list
. Subresourcestatus
is redundant for bothEndpoints
orEndpointSlices
because they don't implement it. Affected filesconfig/rbac/role.yaml
deploy/single/*.yaml
I've spotted
secrets/status
too, it seems redundant (it's addressed separately in chore(rbac): get rid of rbac permissions for secrets/status #4017).One K8s
Service
can have multipleEndpointSlices
thus now we have to work with a slice instead of a single resource.In terms of readiness for an endpoint the way that ensures the same semantic as in CoreV1 is used.
EndpointSlice by default handles 100 endpoints, Service can have multiple EndpointSlices. In integration/e2e tests having a Service that has more than one EndpointSlice is rather not sensible. But in controller manager with
--max-endpoints-per-slice
flag number of allowed endpoints per EndpointSlice can be adjusted. Maybe it's worth to add a test case that would use a cluster that will have configured--max-endpoints-per-slice 1
so multiple EndpointSlices per Service will be easily achievable.PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR