-
Notifications
You must be signed in to change notification settings - Fork 264
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
Add network stats to the enable getting stats directly #1102
Conversation
@@ -31,6 +31,7 @@ type HNSEndpoint struct { | |||
EnableLowMetric bool `json:",omitempty"` | |||
Namespace *Namespace `json:",omitempty"` | |||
EncapOverhead uint16 `json:",omitempty"` | |||
SharedContainers []string `json:",omitempty"` |
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.
What is this change for?
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.
In kubelet, I need to know which containers the endpoint is associated with. I found this function:
hcsshim/internal/hns/hnsendpoint.go
Line 106 in 9bc76cd
func (endpoint *HNSEndpoint) IsAttached(vID string) (bool, error) { |
The two stats calls is just a dockershim problem? |
@Keith-Mange can someone from your team please take a look? |
lgtm |
You are right it is called twice for containerd too since containerd does the call. The networkstats are not returned during this time. |
31d1d57
to
aa76ca7
Compare
@dcantah I've fixed up the comments |
@jsturtevant Thanks! Replied/left one more comment about the exported method |
@jsturtevant Oop didn't even realize also, could you sign off your commits as well. |
4c4aaed
to
5b41133
Compare
Signed-off-by: James Sturtevant <jstur@microsoft.com>
Signed-off-by: James Sturtevant <jstur@microsoft.com>
@katiewasnothere Let me know if you have any other questions or feedback. I'll check in eod if not (or just check it in if you approve) Edit: Oh the CI hasn't ran yet, also depends on that then also 😆 |
5b41133
to
f8784aa
Compare
It had two commits so I had to do a little more than |
All green, so no worries! |
Related work items: microsoft#930, microsoft#962, microsoft#1004, microsoft#1008, microsoft#1039, microsoft#1045, microsoft#1046, microsoft#1047, microsoft#1052, microsoft#1053, microsoft#1054, microsoft#1057, microsoft#1058, microsoft#1060, microsoft#1061, microsoft#1063, microsoft#1064, microsoft#1068, microsoft#1069, microsoft#1070, microsoft#1071, microsoft#1074, microsoft#1078, microsoft#1079, microsoft#1081, microsoft#1082, microsoft#1083, microsoft#1084, microsoft#1088, microsoft#1090, microsoft#1091, microsoft#1093, microsoft#1094, microsoft#1096, microsoft#1098, microsoft#1099, microsoft#1102, microsoft#1103, microsoft#1105, microsoft#1106, microsoft#1108, microsoft#1109, microsoft#1115, microsoft#1116, microsoft#1122, microsoft#1123, microsoft#1126
Add network stats to the enable getting stats directly
The metrics endpoint with containerd running doesn't return stats (kubernetes/kubernetes#104286) and the dockershim component calls metrics twice (kubernetes/kubernetes#104285)
This PR enables the gathering the stats to enable the two scenarios.