-
Notifications
You must be signed in to change notification settings - Fork 38.9k
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
Graduate EndpointSlice API to GA #99662
Graduate EndpointSlice API to GA #99662
Conversation
Hi @swetharepakula. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Thanks for the work on this! /ok-to-test |
4529b0c
to
0c0feaa
Compare
I have made an update to PR with the new approach outlined in the above comment. It is ready for review again. Thank you all for all the feedback and help! |
}, | ||
} | ||
|
||
for _, tc := range testcases { |
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.
Great testcases
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.
This looks pretty solid to me
} | ||
|
||
requestGV := schema.GroupVersion{Group: info.APIGroup, Version: info.APIVersion} | ||
if requestGV == discoveryv1.SchemeGroupVersion { |
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.
suggest checking our version and returning early for v1beta1 specifically. we'd want to drop deprecated topology the same way in future versions without needing to revisit this code.
func dropTopologyOnV1(ctx context.Context, oldEPS, newEPS *discovery.EndpointSlice) {
if info, ok := genericapirequest.RequestInfoFrom(ctx); ok {
// ... return early if v1beta1
}
... check deepequal, drop, etc...
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.
that has a nice property of forcing us to look at this when v1b1 goes away
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.
updated so we check for v1beta1 requests and return early.
if ep.NodeName != nil { | ||
if ep.DeprecatedTopology == nil { | ||
ep.DeprecatedTopology = make(map[string]string, 1) | ||
} | ||
ep.DeprecatedTopology[corev1.LabelHostname] = *ep.NodeName | ||
} |
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.
why isn't this limited to endpointListUpdated
as well? this means a v1 writer touching only metadata fields (like the garbage collector or something) would cause nodeName to get propagated into deprecatedTopology. is that what we want?
edit: I guess if we drop the hostname topology label in internal→v1 conversion as described in #99662 (comment), we have to restore it in v1→internal conversion
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.
I think we need to have nodeName propagated in deprecatedTopology, otherwise older kube-proxies would not be able to read nodeName information since we don't do anything special in the conversion in regards to the hostname topology label. When we remove v1beta1, we could then remove this logic to stop populating topology and eventually deprecatedTopology will be an unused field.
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.
I think we need to have nodeName propagated in deprecatedTopology, otherwise older kube-proxies would not be able to read nodeName information since we don't do anything special in the conversion in regards to the hostname topology label.
When endpoints are being modified via v1, I agree. If the endpoint was written via v1beta1, doing this unconditionally (not just when endpoints were modified via v1) means we'll copy nodeName into topology
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.
We still have the version check, but I see your point that it makes sense only to do this when the v1 client is writing to Endpoints
I agree this looks close. The only open question I have is whether we want |
* Removes discovery v1alpha1 API * Replaces per Endpoint Topology with a read only DeprecatedTopology in GA API * Adds per Endpoint Zone field in GA API
244dddd
to
a9891b4
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.
YAY!
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: swetharepakula, thockin 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:
Approvers can indicate their approval by writing |
Impressive 👏 |
/test pull-kubernetes-dependencies |
/retest |
/retest |
1 similar comment
/retest |
https://storage.googleapis.com/k8s-gubernator/triage/index.html?date=2021-03-06&pr=1&test=TestAdmit
|
(the unit test flakes don't look related at all, just capturing them for referencing in flake issues) |
What type of PR is this?
/kind feature
/kind api-change
What this PR does / why we need it:
Graduates the EndpointSlice API to GA and removes the v1alpha1 API.
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:
/cc @aojea @andrewsykim @liggitt @robscott @wojtek-t
/assign @thockin