feat: Add bootstrap NRC duration metric - #294
Conversation
✅ Deploy Preview for node-readiness-controller ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
[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 |
|
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. |
10d451f to
f85970c
Compare
| ) | ||
|
|
||
| // bootstrapAnnotationPayload is the JSON value stored in a bootstrap-completion annotation. | ||
| type bootstrapAnnotationPayload struct { |
There was a problem hiding this comment.
IMO, TaintAppliedAt and Completed are better suited for the dedicated NodeEvaluation 'status' api we planned. Can we discuss this during our next sync on how we could shape it in the alpha2 api?
|
@rawadhossain can you resolve the conflicts please! |
f85970c to
a9d441f
Compare
3069c56 to
59e4f78
Compare
|
Hi @ajaysundark @AvineshTripathi Updated the PR. Moved the timestamp from node metadata to status.nodeEvaluations, following the current API shape as we discussed. Also handled the adopted taint (--register-with-taints) case by only stamping the anchor when the taint is applied by NRC. |
|
While validating this metric for Section 2 Grafana dashboard, I found that it wasn't emitting any data. It turned out I added a fix to recover the missing anchor when needed and make sure it isn't lost again during later status updates. Since the Section 2 dashboard depends on this metric, I wanted to fix this before moving forward. |
|
@rawadhossain can we two separate PRs? Maybe the fix can go in the different PR and we can try merge this. Please let me know your thoughts |
| // taintAppliedAt is the timestamp when the controller applied the readiness taint to this Node. | ||
| // | ||
| // +optional | ||
| TaintAppliedAt metav1.Time `json:"taintAppliedAt,omitempty,omitzero"` |
There was a problem hiding this comment.
cc @Karthik-K-N for note as we should keep this field name and semantics same when we split NodeEvaluation status
That makes sense and its cleaner. My only concern is that the cache fix is tied to this metric, and without it the metric doesn't reliably emit data. That's why I kept them together. If you prefer splitting them, I'm happy to do that. |
|
@rawadhossain i see there are conflicts, can you resolve it please |
6f60849 to
d72fbc1
Compare
d72fbc1 to
9832aaa
Compare
|
Done, resolved the merge conflicts. Thanks. |
9832aaa to
7456ac8
Compare
fbed91b to
3966293
Compare
| Help: "Time from readiness taint application or observation to bootstrap completion. Measures only NRC-attributable hold time.", | ||
| Buckets: []float64{1, 5, 10, 30, 60, 120, 300, 600, 1200, 1800, 3600}, | ||
| }, | ||
| []string{"rule", "controller"}, // controller: applied, adopted |
There was a problem hiding this comment.
can you x-ref the design on this one? taint_origin for this label is better with values adopted vs controller, to show initial ownership.
There was a problem hiding this comment.
Updated it. taint_origin now uses controller and adopted to show the taint ownership.
8ede5ad to
987eb12
Compare
|
please resolve conflicts |
987eb12 to
5163b26
Compare
|
@ajaysundark resolved the conflicts. PTAL. Thanks. |
5163b26 to
f889aee
Compare
Description
This PR adds
node_readiness_bootstrap_hold_duration_seconds, which measures how long a node is held by a bootstrap-only rule's taint, from when the taint is first present on the node until bootstrap completes.The status object now tracks two anchor timestamps on
NodeEvaluation:taintAppliedAt— when NRC itself applies the tainttaintObservedAt— when NRC first observes the taint present, whether applied by NRC or already present (e.g. via kubelet-register-with-taints)The metric carries a new
controllerlabel (applied/adopted) so the two cases are distinguishable: anchored ontaintAppliedAtwhen NRC applied the taint, falling back totaintObservedAtfor the adopted case, wheretaintAppliedAtis never set.Related to Issue #182
Type of Change
/kind feature
Testing
isBootstrapCompleted, including backward compatibility and in progress statesChecklist
make testpassesmake lintpasses