-
Notifications
You must be signed in to change notification settings - Fork 306
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
Only use endpoint slices to generate network endpoints #1973
Only use endpoint slices to generate network endpoints #1973
Conversation
* remove Endpoints code path and only use EndpointSlices for endpoint decisions * removes --enable-endpoints-slices flag and makes that code path default and only option
The neg syncer was doing some wrong assumptions: 1. Endpoints that are not Ready can have Pods with deletionTimestamp not nil. The Endpoint and EndpointSlice controller will remove the Pod from the corresponding object once it gets deleted (DeletionTimestamp != nil) 2. Any pod with deletionTimestamp == nil is valid, this is the most dangerous assumption, since a pod can be Unready and be used.
* terminating pods are already removed from the endpoints from the EPS, so this check is no longer needed. * this check was a remanent of using endpoints as the source of truth since endpoints did not have a terminating field
@@ -253,7 +253,6 @@ L7 load balancing. CSV values accepted. Example: -node-port-ranges=80,8080,400-5 | |||
flag.StringVar(&F.GKEClusterHash, "gke-cluster-hash", "", "The cluster hash of the GKE cluster this Ingress Controller will be interacting with") | |||
flag.StringVar(&F.GKEClusterType, "gke-cluster-type", "ZONAL", "The cluster type of the GKE cluster this Ingress Controller will be interacting with") | |||
flag.BoolVar(&F.EnableTrafficScaling, "enable-traffic-scaling", false, "Enable support for Service {max-rate-per-endpoint, capacity-scaler}") | |||
flag.BoolVar(&F.EnableEndpointSlices, "enable-endpoint-slices", false, "Enable using Endpoint Slices API instead of Endpoints API") |
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.
Do we not use this flag anymore? Usually when retiring flags, you keep the command line param, but make it log so existing callers on the executable aren't broken. Then fix all of the callers and then delete the flag. If we can fix all of the associated call sites, then ok, we can delete this in one shot.
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 will be able to fix the call sites, so this is safe to delete.
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.
so bowei nailed it i the review 😄
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bowei, swetharepakula 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 |
…elease-1.22 [Cherry pick #1973] Only use endpoint slices to generate network endpoints
removes support for using endpoints to generate network endpoints
This PR combines #1957 and does additional cleanup
/assign @aojea
/assign @bowei