-
Notifications
You must be signed in to change notification settings - Fork 53
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
Fetch node status conditions and unschedulable state from API server rather than KSM #194
Conversation
d700bfb
to
c390404
Compare
c390404
to
2345229
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.
Code looks mostly good, except some nits like refactoring, imports, dots and typos.
I also left some suggestions how commits could be improved to optimize for nice git history and for reviewing.
continue | ||
} | ||
|
||
// Since conditions is a list, there could be duplicate conditions. Check ff we have added this condition before |
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.
Can the condition really be duplicate? I was not able to observe that in my tests, though I couldn't find code preventing it 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.
I believe the API should disallow that, but took the defensive programming approach here and since the input is a list, there should be possible for that to happen.
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.
would make sense to log when this happen? oh I see no log here
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.
Yup, I thought the same but definitely not worth passing a logger this deep down :(
Rumbles about global loggers
2345229
to
f58ddb4
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
continue | ||
} | ||
|
||
// Since conditions is a list, there could be duplicate conditions. Check ff we have added this condition before |
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.
would make sense to log when this happen? oh I see no log here
Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
So metrics can be generated using API server data instead of KSM.
f58ddb4
to
cd66ac7
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
To be used for kubelet metrics. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
So all transform functions are in a single file. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com>
cd66ac7
to
7ff306f
Compare
…rather than KSM (#194) * src/kubelet/group_test.go: fix imports Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> * src/kubelet/group_test.go: fix formatting Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> * src/kubelet/metric/testdata: fix imports Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> * src/kubelet/metric/testdata: fix comments formatting Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> * src/kubelet/group: fetch conditions and unschedulable from API server So metrics can be generated using API server data instead of KSM. * cmd/kubernetes-static: add conditions to mocked API server * src/metric: remove node condition fetching from KSM metrics * src/kubelet/metric: add PrefixFromMapInt function To be used for kubelet metrics. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> * src/metric: add condition and unschedulable metrics to kubelet definitions * src/kubelet/metric: move OneMetricPerLabel to separate file So all transform functions are in a single file. Signed-off-by: Mateusz Gozdek <mgozdek@microsoft.com> Co-authored-by: Mateusz Gozdek <mgozdek@microsoft.com>
Replaces implementation in #155.
Previous implementation fetched this information from KSM metrics. This caused those metrics to be reported for one node by a different one on multi-node clusters, since there is only one instance of the DaemonSet fetching data for all the cluster objects. In turn, this caused some attributes to have unexpected values, such as
hostname
not matchingnodeName
.This implementation fetches this information from the local node, as an additional step to parsing data from the kubelet.
Additionally, the
unschedulable
metric is also fetched this way.