Refactor procedure to compute endpoint target map #1950
Conversation
|
Hi @sawsa307. 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. |
|
/assign @swetharepakula |
0d160cf to
8ed95f6
Compare
pkg/neg/syncers/transaction.go
Outdated
| targetMap, endpointPodMap, err = s.computeTargetMap(endpointSlices) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } else { | ||
| ep, exists, err := s.endpointLister.Get( |
There was a problem hiding this comment.
Interesting, since EndpointSlices is GA since 1.21, maybe we can start removing all the Endpoints code
There was a problem hiding this comment.
Hi Antontio. Regarding this, I asked Swetha about it. This change is already on the calendar, and it will be in a separate PR. For now, we refactor this part of code since in degraded mode, we will do this calculation differently.
There was a problem hiding this comment.
Made #1973 to remove endpoints codepath
8ed95f6 to
530a18d
Compare
pkg/neg/syncers/transaction.go
Outdated
| } else { | ||
| targetMap, endpointPodMap, err = s.computeTargetMap(endpointSlices) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Why is the computation in the else block? Don't we always need to compute the target map?
There was a problem hiding this comment.
Remove the condition. Now we only do computeTargetMap.
530a18d to
9e0f1ae
Compare
9e0f1ae to
27e5ff3
Compare
pkg/neg/syncers/transaction.go
Outdated
| } | ||
| if err != nil { | ||
| return negtypes.NewNegSyncResult(fmt.Errorf("endpoints calculation error in mode %q, err: %w", s.endpointsCalculator.Mode(), err), negtypes.ResultOtherError) | ||
| if s.inErrorState() { |
There was a problem hiding this comment.
this means we will not compute the target map if we are in error state? What do we do in that case?
|
/ok-to-test |
4975308 to
6f35925
Compare
6f35925 to
e61adc8
Compare
|
PR needs rebase. 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. |
0265b92 to
8da6eb2
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sawsa307 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Closed due to change of implementation |
Refactor the code that compute endpoint target map into a separate function computeTargetMap.