-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Improve Kubernetes service registry for ALS analysis #5722
Conversation
0a62e2b
to
da44b9e
Compare
Codecov Report
@@ Coverage Diff @@
## master #5722 +/- ##
============================================
- Coverage 51.59% 43.24% -8.35%
+ Complexity 3448 2666 -782
============================================
Files 1637 1635 -2
Lines 34928 34942 +14
Branches 3806 3786 -20
============================================
- Hits 18020 15112 -2908
- Misses 16021 18934 +2913
- Partials 887 896 +9 Continue to review full report at Codecov.
|
FYI, @wu-sheng @hanahmily What it differs from the previous version is that the different versions of a same service are now collapsed into one service, e.g. in the previous version, reviews-v1, reviews-v2, reviews-v3 are 3 different services, but in this version, they are all collapsed into |
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.
Like the idea. Wait for @hanahmily code level review.
beb33c9
to
a2d90ad
Compare
...in/src/main/java/org/apache/skywalking/oap/server/receiver/envoy/als/K8SServiceRegistry.java
Outdated
Show resolved
Hide resolved
final CoreV1Api coreV1Api = new CoreV1Api(apiClient); | ||
final SharedInformerFactory factory = new SharedInformerFactory(executor); | ||
|
||
listenEndpointsEvents(coreV1Api, factory); |
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.
Endpoint slice resources should be listened to either.
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.
@hanahmily even the latest kubernetes-client(10.0.0, 27th, Oct, 2020) doesn't support to listen to the EndpointSlice events. There is no such API to do this 😢 Let's postponed this resource until the newer version can do this.
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.
If no listener, are you reading the data periodically like the old way?
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.
If no listener, are you reading the data periodically like the old way?
Not now, as the doc says, EndpointSlice is to improve the performance of Endpoints, so I think it's OK to just ignore this kind of event for now, (p.s. EndpointSlice is still in beta now), is it OK @hanahmily ?
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.
If java client doesn't support it right now, feel free to leave it alone. But we should mention it in our document and leave todo
market in codes.
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.
How about submitting a request to k8s java client repo about listen to the EndpointSlice ?
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.
They may need a scenario about which java codes need this.
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.
How about submitting a request to k8s java client repo about listen to the EndpointSlice ?
I'm thinking that they don't support it only because EndpointSlice is still in beta(after stabilization, they will), and the methods to list/watch that resources are unstable now, (e.g. need the apiGroup
, version
, which are changing for an alpha/beta feature).
IMO, ignoring EndpointSlice for now is safe because the docs says
Although the EndpointSlice API is providing a newer and more scalable alternative to the Endpoints API, the Endpoints API will continue to be considered generally available and stable.
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 can revamp this after the EndpointSlice API is stabilised
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.
My point is only creating an issue to track this todo as backlog.
aeaa737
to
d23c4aa
Compare
All review comments should be addressed, if I missed anything, please let me know |
The current implementation of envoy ALS K8S analysis is based on the hierarchy, pod -> StatefulSet -> deployment, StatefulSet, or others. It's freaky and different from the Istio Kubernetes registry. The new path is pod -> endpoint -> service, and we should leverage Informer API instead of raw Kubernetes API.
653a326
to
2846d61
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.
LGTM
Improve Kubernetes service registry for ALS analysis
The current implementation of Envoy ALS K8S analysis is based on the hierarchy, pod -> StatefulSet -> deployment, StatefulSet, or others. It's freaky and different from the Istio Kubernetes registry. And the service name pattern changed in recent Kubernetes versions, which generates weird service names in the topology.
The new path is pod -> endpoint -> service, and we should leverage Informer API instead of raw Kubernetes API.
CHANGES
log.