feat: add blocked nodes metric to scrape-time collector - #431
feat: add blocked nodes metric to scrape-time collector#431rawadhossain wants to merge 2 commits into
Conversation
Signed-off-by: Rawad Hossain <rawad.hossain00@gmail.com>
kubernetes-prow
Bot
added
the
kind/feature
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rawadhossain The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @rawadhossain. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
kubernetes-prow
Bot
added
the
size/XXL
|
/ok-to-test |
kubernetes-prow
Bot
added
ok-to-test
AvineshTripathi
left a comment
AvineshTripathi
left a comment
There was a problem hiding this comment.
did my initial round of reviewing, please have a look at the comments!
| } | ||
|
|
||
| // ListBlockedNodes returns the number of blocked nodes for each rule and unsatisfied condition. | ||
| func (r *RuleReadinessController) ListBlockedNodes(ctx context.Context, nodes []corev1.Node) (map[string]metrics.RuleBlockedConditions, error) { |
There was a problem hiding this comment.
this func is very much similar to ListRuleNodeStates in terms of execution. Can we make a util func that gives you execution flow and use it in both places?
for e.g.
func (r *RuleReadinessController) measureRuleNodes(
ctx context.Context,
nodes []corev1.Node,
measureFn func(rule *readinessv1alpha1.NodeReadinessRule, node *corev1.Node) error,
) error {
// Fetch rules, iterate, parse selectors, filter nodes
// Call measureFn(rule, node) for each held node
// measureFn does the actual counting/filtering
}
There was a problem hiding this comment.
Done. Added a shared util forEachRuleNode and reused it in both functions.
| continue | ||
| } | ||
|
|
||
| selector, err := metav1.LabelSelectorAsSelector(&rule.Spec.NodeSelector) |
There was a problem hiding this comment.
we can reuse ruleAppliesTo here. I see we add extra !r.hasTaintBySpec(node, rule.Spec.Taint) check so probably opportunity to create another util that uses runAppliesTo and hasTaint
There was a problem hiding this comment.
yeah, right. Added parseNodeSelector so the selector parsing is shared and added ruleAppliesToWithTaint to handle the rule + taint check together.
| return | ||
| } | ||
|
|
||
| counts, err := c.lister.ListRuleNodeStates(ctx, nodes) |
There was a problem hiding this comment.
missing return in case of err, this could be expected behavior but there is inconsistency. ListBlockedNodes below returns in case of err
There was a problem hiding this comment.
made error handling consistent for both metrics now. Neither returns on error, we log and continue as the two metrics are independent, so a failure in one shouldn't prevent the other from being collected.
|
|
||
| // Collect implements prometheus.Collector. | ||
| func (c *ReadinessCollector) Collect(ch chan<- prometheus.Metric) { | ||
| ctx, cancel := context.WithTimeout(context.Background(), collectTimeout) |
There was a problem hiding this comment.
5sec to do ListNodes, ListRuleNodeStates and ListBlockedNodes can feel little less. wdyt?
There was a problem hiding this comment.
I wasn't fully sure about this. Kept it at 5s since it's below Prometheus's default scrape timeout (10s), so we can fail and log before the scrape times out. I also tested it with larger node counts and it seemed fine. Do you think it'd be better to increase?
| const collectTimeout = 5 * time.Second | ||
|
|
||
| // NodeLister lists Nodes for the collector. | ||
| type NodeLister interface { |
There was a problem hiding this comment.
more like a question not a blocker: why are we creating individual interfaces when at the end we are using only ReadinessLister. Is this for testing purposes?
There was a problem hiding this comment.
yeah, mainly for testing, so each part can be mocked independently
e72d1ac to
367985d
Compare
2 participants
Description
Adds
node_readiness_blocked_nodes{rule, condition}for tracking the number of currently-held nodes blocked by each unsatisfied condition for eachNodeReadinessRule. Ref. New Scrape-Time Collector SurfaceThe metric is collected directly from the controller runtime cache on each Prometheus scrape, sharing the same Node snapshot with
node_readiness_rule_nodes.Changes
node_readiness_blocked_nodesmetric withruleandconditionlabels.ListBlockedNodesto calculate blocked nodes from the live Node state.DefaultStatuswhen a condition is missing from the Node, same as the controller.ListNodeswithnode_readiness_rule_nodesduring each scrape.Related to #397
Type of Change
/kind feature
Testing
make test,make lint,go test ./... -raceall passDefaultStatusbehavior.Checklist
make testpassesmake lintpasses