OSD-28525 - Initial implementation for MachineHealthCheckUnterminatedShortCircuitSRE investigation

[tnierman]

https://issues.redhat.com/browse/OSD-28525

---

These changes introduce the initial implementation for the machineHealthCheckUnterminatedShortCircuitSRE alert. The alert investigation takes the following steps:

• Retrieves the machines managed by any failling machinehealthcheck object. Machines & nodes that are failing which are otherwise un-managed by a machinehealthcheck that is short-circuited (ie - masters) are not investigated
  • Machines that are deleting will have their node checked to see if it's stuck draining. At the moment, this check is imperfect: once the ability to query metrics is introduced, we can more precisely confirm what workloads are blocking a node drain based on the pods_preventing_node_drain metric
  • Machines that are in a failed state will have their .Status.ErrorReason and .Status.ErrorMessages examined
  • For each of the machines that's examined and found to be problematic, a recommended action will be added to the investigation's recommendations map. This map is used to holistically evaluate the state of the cluster following the investigation
• Once all machines have been evaluated, the investigation moves on to the machines' nodes.
  • Nodes whose machines have already been identified as problematic are not evaluated: this cuts down on noise and prevents us from accidentally recommending two different courses of action for the same underlying compute instance
  • Currently, the node investigation is a bit bare, it primarily determines whether a specific node warrants further human investigation. Future iterations involving metrics may be able to expand on this aspect of the investigation
• Finally, once all objects have been evaluated, the investigation's recommendations are summarized. By compiling the summary at the end of the investigation, rather than during each iteration, a single action can be recommended for a number of different nodes & machines (ie - "send this one servicelog regarding machine A, B, and C" rather than "send servicelog regarding machine A" + "send servicelog regarding machine B" + "send servicelog regarding machine C", etc)

Additionally, recommended test methods are supplied in the testing/ directory, along with a (somewhat) detailed README to help newcomers get started. This includes test objects that should be directly applicable to any OSD/ROSA staging cluster.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 52.54902% with 121 lines in your changes missing coverage. Please review.

Project coverage is 33.44%. Comparing base (7b0f2f0) to head (c1da1a1).

Files with missing lines	Patch %	Lines
...e/machinehealthcheckunterminatedshortcircuitsre.go	50.88%	79 Missing and 4 partials ⚠️
...checkunterminatedshortcircuitsre/recommendation.go	21.73%	18 Missing ⚠️
pkg/investigations/utils/machine/machine.go	66.66%	12 Missing and 2 partials ⚠️
pkg/investigations/utils/node/node.go	78.94%	4 Missing ⚠️
pkg/k8s/scheme.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #395      +/-   ##
==========================================
+ Coverage   31.09%   33.44%   +2.34%     
==========================================
  Files          29       33       +4     
  Lines        2074     2329     +255     
==========================================
+ Hits          645      779     +134     
- Misses       1376     1491     +115     
- Partials       53       59       +6     

Files with missing lines	Coverage Δ
pkg/investigations/registry.go	0.00% <ø> (ø)
pkg/k8s/scheme.go	0.00% <0.00%> (ø)
pkg/investigations/utils/node/node.go	78.94% <78.94%> (ø)
pkg/investigations/utils/machine/machine.go	66.66% <66.66%> (ø)
...checkunterminatedshortcircuitsre/recommendation.go	21.73% <21.73%> (ø)
...e/machinehealthcheckunterminatedshortcircuitsre.go	50.88% <50.88%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[bergmannf]

I think this looks really good already - I guess it could check more things, but I think the PR is already quite big and it's better to get this in and running in stage before adding even more checks.
(Something that happens and could be found by this is customers running out of available CIDRs for new nodes: machine = Running, Node never becoming ready, which would need to inspect the network configuration)

[typeid]

Added some suggestions/nit comments.

Additional suggestions/thoughts (non-blocking):
I generally find it a bit odd we're going through all machines and looking for MHC available remediation. Would it be more logical to look at the MHCs and triaging by everything that has unhealthyCount > maxUnhealthy and to focus on those?

As the file grew quite large, we could also consider splitting up the large file here into something like the following:

• machine_investigation.go: InvestigateMachines, investigateFailingMachine, investigateDeletingMachine, getMachineRole
• node_investigation.go: InvestigateNode, InvestigateNodes, findNodeReadyCondition, checkForStuckDrain
• utils.go: findNoScheduleTaint, getNodeRole
• recommendations.go: recommendations, investigationResult, and String() logic

[typeid]

Suggestion: Instead of the big switch here, you could centralize the mapping of errorReason to recommendation logic, e.g.:

var machineFailureHandlers = map[machinev1beta1.MachineStatusError]func(machinev1beta1.Machine) (recommendedAction, string){
	machinev1beta1.IPAddressInvalidReason: func(m machinev1beta1.Machine) (recommendedAction, string) {
		return recommendationDeleteMachine, fmt.Sprintf("invalid IP: %q", *m.Status.ErrorMessage)
	},
	// etc...
}

Then you could replace the long switch case:

handler, ok := machineFailureHandlers[errorReason]
if !ok {
	i.notes.AppendInfo("Unknown error reason %s on failed machine %s", errorReason, machine.Name)
	return nil
}

action, note := handler(machine)
i.recommendations.addRecommendation(action, machine.Name, note)
return nil

This would decouple things and should be easier on the eyes and decouple things:)

[tnierman]

After thinking on it for a bit, I personally prefer the big-switch: it's easier to tell at-a-glance what recommendation we're making for each machine failure-state, and it feels like less complexity for the same end-result.

If this is something that you or others feel strongly about, I'm happy to convert this, however 🙂

[tnierman]

I added newlines between the case statements in an effort to improve readability

[tnierman]

@typeid - thanks for the recommendations!

Specifically regarding

I generally find it a bit odd we're going through all machines and looking for MHC available remediation. Would it be more logical to look at the MHCs and triaging by everything that has unhealthyCount > maxUnhealthy and to focus on those?

It sounds like you're suggesting we remediate only the machines owned by a failing machineHealthCheck, correct?

That was the goal of getMachinesFromFailingMHC() (previously getTargetMachines()), though upon re-examining that logic, I had mismatched the return value to the name, and then misused the value to still get the right set of machines (realistically, I probably changed the name of that function, and forgot to change the behavior 😬). I pushed a fix for that now, but let me know if your concern extends beyond that error

[tnierman]

/hold to implement some additional recommendations

[tnierman]

/unhold - I think most recommendations have been accounted for

[openshift-ci]

@tnierman: all tests passed!

Full PR test history. Your PR dashboard.

Instructions 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. I understand the commands that are listed here.

[tnierman]

/unhold

[typeid]

/lgtm
/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tnierman, typeid

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [tnierman,typeid]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment