K8s resolver for the gRPC client #4190
Conversation
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4190 +/- ##
==========================================
+ Coverage 59.06% 59.58% +0.52%
==========================================
Files 621 552 -69
Lines 53105 39919 -13186
==========================================
- Hits 31365 23787 -7578
+ Misses 19240 13789 -5451
+ Partials 2500 2343 -157
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
| } | ||
|
|
||
| parts := strings.SplitN(service, ".", 3) | ||
| if len(parts) >= 2 { |
There was a problem hiding this comment.
Should we set namespace to default if parts==1 ?
There was a problem hiding this comment.
it will use the namespace where agent is at if the namespace is None
flytestdlib/resolver/k8s_resolver.go
Outdated
| } | ||
|
|
||
| // NewBuilder creates a kubeBuilder which is used by grpc resolver. | ||
| func NewBuilder(client kubernetes.Interface, schema string) resolver.Builder { |
There was a problem hiding this comment.
Do you want to pass ctx here and use it in .Build() ?
| }) | ||
| } | ||
| } | ||
| err := k.cc.UpdateState(resolver.State{Addresses: newAddrs}) |
There was a problem hiding this comment.
Is cc.UpdateState thread safe? do we need to lock its usage in someway?
There was a problem hiding this comment.
gRPC client uses lock under the hood. https://github.com/grpc/grpc-go/blob/e14d5831b59b0f46853d076640ae85f964164b37/clientconn.go#L829
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
| } | ||
|
|
||
| // NewBuilder creates a kubeBuilder which is used by grpc resolver. | ||
| func NewBuilder(ctx context.Context, client kubernetes.Interface, schema string) resolver.Builder { |
There was a problem hiding this comment.
If we don't intend to expose the scheme, maybe we could have another builder only for test.
There was a problem hiding this comment.
qq: can't we use NewBuilder to create a fake kube Builder?
There was a problem hiding this comment.
I meant we are exposing schema string unnecessarily to users because they are not supposed to freely choose any string they like.
flytestdlib/resolver/k8s_resolver.go
Outdated
| ) | ||
|
|
||
| const ( | ||
| K8sSchema = "k8s" |
There was a problem hiding this comment.
Nit: I think this is called scheme.
| } | ||
|
|
||
| // Make sure watcher is started before we create the endpoint | ||
| time.Sleep(5 * time.Second) |
There was a problem hiding this comment.
Is it possible to make the waiting time shorter? This increases build time.
Signed-off-by: Kevin Su <[email protected]>
| ) | ||
|
|
||
| const ( | ||
| Schema = "k8s" |
There was a problem hiding this comment.
I think you were going to rename it to
| Schema = "k8s" | |
| Scheme = "k8s" |
Tracking issue
#3936
Describe your changes
When running flyte agent with HPA, grpc client needs to know the new pod's IP address to load balance the requests.
To achieve this, we add a custom name resolver (k8sResolver), which can resolve
k8s:///....endpoint.The resolver will create a go routine, and keep watching the k8s endpoints, and updating the
clientConn.addressesfor grpc client.Check all the applicable boxes
Screenshots
Note to reviewers