Add inhibitions expressions for CAPI clusters #1153
Conversation
weseven
changed the title
helm/prometheus-rules/templates/alerting-rules/inhibit.management-cluster.rules.yml
Outdated
Show resolved
Hide resolved
| @@ -80,7 +80,7 @@ spec: | |||
| - alert: InhibitionClusterNodePoolsNotReady | |||
| annotations: | |||
| description: '{{`Cluster {{ $labels.cluster_id }} node pools are not ready. Either they have been scaled down to 0 or they are not up yet.`}}' | |||
| expr: cluster_operator_node_pool_desired_workers == 0 and cluster_operator_node_pool_ready_workers == 0 | |||
| expr: (cluster_operator_node_pool_desired_workers == 0 and cluster_operator_node_pool_ready_workers == 0) or capi_machinepool_status_condition{type="Ready", status="False"} == 1 | |||
There was a problem hiding this comment.
Thinking about it some more, what happens if there are multiple node pools? (e.g. one per AZ as is recommend) This wouldn't equal to 1 would it?
There was a problem hiding this comment.
Correct, this would equal to 1 even if only one nodePool among N is not ready.
Is that not the desired behaviour? I see the cancel_if_cluster_with_notready_nodepools is used in DNS errorrate, APIserver latency and network error rate alerts, so I assumed even only one nodepool not ready would affect those.
There was a problem hiding this comment.
I'm not sure but I think the non-capi query would only match if all node pools were not ready. I'm not sure what is desired though, I was just pointing out that it looked odd to me.
There was a problem hiding this comment.
I think it works the same way as in vintage, there's a node_pool_id label on the cluster_operator_node_pool_desired_workers and cluster_operator_node_pool_ready_workers metrics.
So for example it is currently triggering for deu01, which has a mix of active nodepools with nodes and nodepools scaled to 0.
It does sound odd though the more I think about it... @QuentinBisson, do you have any historical context on this? (Just because git blame points to your commit on these lines 😅 )
There was a problem hiding this comment.
I have no context over this, this was moved from another repo 3 years ago, What you need to be sure about is that the inhibitoin and alert can relate to a nodepool label but I'm afraid we do not have it in alerts that use this inhibition?
There was a problem hiding this comment.
The doubt is about triggering the inhibition for just one nodepool that has been scaled to 0... but if we don't have any alerts using the inhibition currently, I'm also wondering whether we actually need it 😅
I would still keep it in this PR (with the CAPI update), and remove it in a later PR if it's actually useless, what do you think?
There was a problem hiding this comment.
(maybe it could be useful for the cluster downscaler?)
There was a problem hiding this comment.
Sure you can add and remove :D
Towards: giantswarm/roadmap#3338
This PR adds the expressions to have inhibitions working on capi clusters.
Checklist
oncall-kaas-cloudGitHub group).