OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE #404

anispate · 2025-04-04T18:42:45Z

OSD-18645 - CAD implementation for CannotRetrieveUpdatesSRE

Sample ticket: https://redhat.pagerduty.com/incidents/Q1S45W54TK1QKU#:~:text=%E2%9A%A0%EF%B8%8F%20ClusterVersion%20error%20detected,primary%20for%20review

Updated sample ticket: https://redhat.pagerduty.com/incidents/Q2UVRI8YGLPP3G

openshift-ci · 2025-04-04T18:43:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anispate
Once this PR has been reviewed and has the lgtm label, please assign dustman9000 for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

codecov-commenter · 2025-04-07T19:10:45Z

Codecov Report

Attention: Patch coverage is 28.37838% with 53 lines in your changes missing coverage. Please review.

Project coverage is 32.01%. Comparing base (6ab773b) to head (685883f).

Files with missing lines	Patch %	Lines
...nnotretrieveupdatessre/cannotretrieveupdatessre.go	28.37%	52 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #404      +/-   ##
==========================================
- Coverage   32.04%   32.01%   -0.03%     
==========================================
  Files          35       36       +1     
  Lines        2431     2505      +74     
==========================================
+ Hits          779      802      +23     
- Misses       1593     1643      +50     
- Partials       59       60       +1

Files with missing lines	Coverage Δ
pkg/investigations/registry.go	`0.00% <ø> (ø)`
...nnotretrieveupdatessre/cannotretrieveupdatessre.go	`28.37% <28.37%> (ø)`

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

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

test/generate_incident.sh

pkg/investigations/registry.go

pkg/investigations/CannotRetrieveUpdatesSRE/CannotRetrieveUpdatesSRE.go

pkg/investigations/cannotretrieveupdatesre/cannotRetrieveUpdateSRE.go

tnierman · 2025-05-12T15:23:27Z

pkg/investigations/cannotretrieveupdatessre/testing/README.md

Looks like this is still the boilerplate document from running the bootstrap-new-investigation make target.

Can you revise this to include detailed steps for another SRE to test your work? Additional objects, scripts, etc can also be added to this testing/ directory

tnierman · 2025-05-12T15:25:12Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+	defer func() {
+		deferErr := k8sclient.Cleanup(r.Cluster.ID(), r.OcmClient, remediationName)
+		if deferErr != nil {
+			logging.Error(deferErr)
+			err = errors.Join(err, deferErr)
+		}
+	}()
+
+	defer func(r *investigation.Resources) {
+		logging.Infof("Cleaning up investigation resources for cluster %s", r.Cluster.ID())
+		if cleanupErr := k8sclient.Cleanup(r.Cluster.ID(), r.OcmClient, remediationName); cleanupErr != nil {
+			logging.Errorf("Failed to cleanup Kubernetes client: %v", cleanupErr)
+		} else {
+			logging.Infof("Cleanup completed successfully for cluster %s", r.Cluster.ID())
+		}
+	}(r)


Looks like you're cleaning up twice here

tnierman · 2025-05-12T15:26:05Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+		logging.Error("Network verifier ran into an error: %s", err.Error())
+		notes.AppendWarning("NetworkVerifier failed to run:\n\t %s", err.Error())


I don't believe we need to both log and note this - using notes.AppendWarning should be sufficient to add this message to the logs as well

tnierman · 2025-05-12T15:27:28Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+		err = r.PdClient.AddNote(notes.String())
+		if err != nil {
+			// We do not return as we want the alert to be escalated either no matter what.
+			logging.Error("could not add failure reason incident notes")
+		}


Calling AddNote() here without returning will mean that ~~the network verifier results~~ the network verifier error response potentially gets added to the incident twice: once here and again below, no?

Would it make more sense to notes.AppendWarning in this block, and just add those notes at the end?

tnierman · 2025-05-12T15:56:49Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+	logging.Infof("ClusterVersion channel: %s", clusterVersion.Spec.Channel)
+	logging.Infof("ClusterVersion found: %s", clusterVersion.Status.Desired.Version)
+	logging.Debugf("ClusterVersion conditions: %+v", clusterVersion.Status.Conditions)


Do we need to log this info? Or is it leftover from debugging?

We also log the condition below, so it seems rather repetitive

tnierman · 2025-05-12T15:59:20Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+				logging.Warnf("Detected ClusterVersion error: Reason=%s, Message=%s", condition.Reason, condition.Message)
+				return "", fmt.Sprintf("ClusterVersion error detected: %s. Current version %s not found in channel %s",
+						condition.Message, clusterVersion.Status.Desired.Version, clusterVersion.Spec.Channel),
+					fmt.Errorf("clusterversion validation failed: %s", condition.Reason)


I find it a tad bit confusing that we're returning an error here: it's not the state we want to see the clusterversion in, but I'd expect an error return to mean that we couldn't check the clusterversion, not that we were able to check the clusterversion but didn't like the state we saw it in

tnierman · 2025-05-12T21:19:23Z

pkg/investigations/cannotretrieveupdatessre/testing/README.md

+# Testing CannotRetrieveUpdatesSRE Investigation
+
+TODO:
+- Add a test script or test objects to this  directory for future maintainers to use
+- Edit this README file and add detailed instructions on how to use the script/objects to recreate the conditions for the investigation. Be sure to include any assumptions or prerequisites about the environment (disable hive syncsetting, etc)


This README still contains just the default text. Can we populate it with instructions on how to test this investigation?

tnierman · 2025-05-12T21:21:35Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+		switch verifierResult {
+		case networkverifier.Failure:
+			logging.Infof("Network verifier reported failure: %s", failureReason)
+			// XXX: metrics.Inc(metrics.ServicelogPrepared, investigationName)


Suggested change

// XXX: metrics.Inc(metrics.ServicelogPrepared, investigationName)

I was going to remove it but it exists on other cpd.go and insightoperatordown.go file so, i kept it.

I think we should remove it for now 🙂 If/when we add metrics to CAD, we'll have to scan the codebase for instances of servicelogs being sent anyway

tnierman · 2025-05-12T21:22:46Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+	} else {
+		switch verifierResult {
+		case networkverifier.Failure:
+			logging.Infof("Network verifier reported failure: %s", failureReason)


Suggested change

logging.Infof("Network verifier reported failure: %s", failureReason)

We probably can exclude this logging line since we're noting the failure below too?

tnierman · 2025-05-12T21:25:32Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+				logging.Warnf("Detected ClusterVersion issue: Reason=%s, Message=%s", condition.Reason, condition.Message)
+				return "", fmt.Sprintf("ClusterVersion issue detected: %s. Current version %s not found in channel %s",
+						condition.Message, clusterVersion.Status.Desired.Version, clusterVersion.Spec.Channel),
+					fmt.Errorf("clusterversion has undesirable state: %s", condition.Reason)


The way this function is written and called above, returning an error here won't allow the note to be added to the PD incident.

I'd prefer if we didn't return an error at all, honestly, because that should be reserved for cases where we couldn't check the state of the cluster, rather than cases where we did check the state of the cluster, but didn't like what we saw.

tnierman · 2025-05-12T21:34:15Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+	for _, condition := range clusterVersion.Status.Conditions {
+		if condition.Type == "RetrievedUpdates" && condition.Status == "False" {
+			if (condition.Reason == "VersionNotFound" || condition.Reason == "RemoteFailed") &&
+				strings.Contains(strings.TrimSpace(condition.Message), "Unable to retrieve available updates") {
+				logging.Warnf("Detected ClusterVersion issue: Reason=%s, Message=%s", condition.Reason, condition.Message)
+				return "", fmt.Sprintf("ClusterVersion issue detected: %s. Current version %s not found in channel %s",
+						condition.Message, clusterVersion.Status.Desired.Version, clusterVersion.Spec.Channel),
+					fmt.Errorf("clusterversion has undesirable state: %s", condition.Reason)
+			}
+		}
+	}


Suggested change

for _, condition := range clusterVersion.Status.Conditions {

if condition.Type == "RetrievedUpdates" && condition.Status == "False" {

if (condition.Reason == "VersionNotFound" || condition.Reason == "RemoteFailed") &&

strings.Contains(strings.TrimSpace(condition.Message), "Unable to retrieve available updates") {

logging.Warnf("Detected ClusterVersion issue: Reason=%s, Message=%s", condition.Reason, condition.Message)

return "", fmt.Sprintf("ClusterVersion issue detected: %s. Current version %s not found in channel %s",

condition.Message, clusterVersion.Status.Desired.Version, clusterVersion.Spec.Channel),

fmt.Errorf("clusterversion has undesirable state: %s", condition.Reason)

}

}

}

for _, condition := range clusterVersion.Status.Conditions {

if condition.Type == "RetrievedUpdates" {

note, err := checkRetrievedUpdatesCondition(condition)

return clusterVersion.Status.DesiredVersion, note, err

}

}

It looks like we're only looking to analyze the state of one condition here, correct? Would it make more sense to break some of this out into it's own function?

Something like the following may make the nested conditionals more readable:

func checkRetrievedUpdatesCondition(condition corev1.StatusCondition) (string, err) { if condition.Status == corev1.ConditionFalse { return ... } if (condition.Reason == ...) { return ... } }

Additionally, we won't waste CPU cycles continuing to loop through conditions we don't need to analyze once we find the one we're after.

openshift-ci · 2025-05-13T10:35:49Z

@anispate: 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.

typeid · 2025-05-13T10:42:33Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+func (c *Investigation) Run(r *investigation.Resources) (investigation.InvestigationResult, error) {
+	result := investigation.InvestigationResult{}
+	notes := notewriter.New("CannotRetrieveUpdatesSRE", logging.RawLogger)
+	k8scli, err := k8sclient.New(r.Cluster.ID(), r.OcmClient, remediationName)


issue: I think this will fail as backplane will look for the metadata.yaml in the path pkg/investigations/CannotRetrieveUpdatesSRE/metadata.yaml, but the path is lowercase.

But, i have updated the path as part of the PR to the lowercase for the metadata.yaml as part of the PR so, would it still cause an issue?

remediationName uses camel case in your PR, correct? And the path is not camel case in this PR, so it's a mismatch as far as I can tell

typeid · 2025-05-13T10:45:55Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+	notes.AppendWarning("Alert escalated to on-call primary for review.")
+	logging.Infof("Escalating incident with notes for cluster %s", r.Cluster.ID())
+	err = r.PdClient.EscalateIncidentWithNote(notes.String())
+	if err != nil {
+		logging.Errorf("Failed to escalate incident to PagerDuty: %v", err)
+		return result, fmt.Errorf("failed to escalate incident: %w", err)
+	}
+	logging.Infof("Investigation completed and escalated successfully for cluster %s", r.Cluster.ID())
+
+	return result, nil


We don't need to add in the PD note that we escalated the alert to SRE, it should be obvious if the SRE has the alert on their board ;)

There might be a few superfluous logs here as well, the following should suffice:

Suggested change

notes.AppendWarning("Alert escalated to on-call primary for review.")

logging.Infof("Escalating incident with notes for cluster %s", r.Cluster.ID())

err = r.PdClient.EscalateIncidentWithNote(notes.String())

if err != nil {

logging.Errorf("Failed to escalate incident to PagerDuty: %v", err)

return result, fmt.Errorf("failed to escalate incident: %w", err)

}

logging.Infof("Investigation completed and escalated successfully for cluster %s", r.Cluster.ID())

return result, nil

return result, r.PdClient.EscalateIncidentWithNote(notes.String())

typeid · 2025-05-13T10:52:27Z

pkg/investigations/cannotretrieveupdatessre/cannotretrieveupdatessre.go

+}
+
+// checkClusterVersion retrieves the cluster version
+func checkClusterVersion(k8scli client.Client, clusterID string) (version string, note string, err error) {


This function does more than the name suggests:

it retrieves the cluster version

checks if the updates could be retrieved

pre-formats a note

Could we separate the logic here for the functions to be more clear cut on what they are doing?

E.g.

func getClusterVersion() -> no logs, just gets the cluster version object
func getUpdateRetrievalFailures(clusterversion) -> looks for update retrieval failures in the clusterversion

Logs and note creation should be outside of this logic, ideally in the main investigation function logic. This way, we could even move the above functions in common packages as they are re-usable.

openshift-ci bot requested review from joshbranham and Nikokolas3270 April 4, 2025 18:43

anispate force-pushed the OSD-18645 branch from 0ff7781 to 9524efe Compare April 7, 2025 18:45

anispate force-pushed the OSD-18645 branch 4 times, most recently from bcb9b3c to f246d1f Compare April 8, 2025 18:17

joshbranham reviewed Apr 9, 2025

View reviewed changes

test/generate_incident.sh Outdated Show resolved Hide resolved

joshbranham reviewed Apr 9, 2025

View reviewed changes

pkg/investigations/registry.go Outdated Show resolved Hide resolved

joshbranham reviewed Apr 9, 2025

View reviewed changes

anispate force-pushed the OSD-18645 branch 3 times, most recently from c933fc3 to 02d2c44 Compare April 10, 2025 21:11

tnierman reviewed Apr 15, 2025

View reviewed changes

pkg/investigations/cannotretrieveupdatesre/cannotRetrieveUpdateSRE.go Outdated Show resolved Hide resolved

anispate force-pushed the OSD-18645 branch from 02d2c44 to 31f61a2 Compare May 8, 2025 18:58

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 8, 2025

anispate force-pushed the OSD-18645 branch from 31f61a2 to 1193f00 Compare May 9, 2025 01:06

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2025

anispate force-pushed the OSD-18645 branch 5 times, most recently from ac7a009 to ea1ba08 Compare May 9, 2025 04:09

anispate changed the title ~~[DRAFT] - OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE~~ OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE May 9, 2025

tnierman reviewed May 12, 2025

View reviewed changes

anispate force-pushed the OSD-18645 branch 4 times, most recently from 74aa572 to 7f8407a Compare May 12, 2025 21:15

tnierman reviewed May 12, 2025

View reviewed changes

Initial implementation for CannotRetrieveUpdatesSRE

685883f

anispate force-pushed the OSD-18645 branch from 7f8407a to 685883f Compare May 13, 2025 10:03

typeid requested changes May 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE #404

OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE #404

anispate commented Apr 4, 2025 •

edited

Loading

openshift-ci bot commented Apr 4, 2025

codecov-commenter commented Apr 7, 2025 •

edited

Loading

tnierman May 12, 2025

tnierman May 12, 2025

tnierman May 12, 2025 •

edited

Loading

tnierman May 12, 2025 •

edited

Loading

tnierman May 12, 2025

tnierman May 12, 2025

tnierman May 12, 2025

tnierman May 12, 2025

anispate May 12, 2025

tnierman May 12, 2025

tnierman May 12, 2025

tnierman May 12, 2025

tnierman May 12, 2025

openshift-ci bot commented May 13, 2025

typeid May 13, 2025

anispate May 13, 2025

typeid May 14, 2025 •

edited

Loading

typeid May 13, 2025

typeid May 13, 2025 •

edited

Loading

		logging.Error("Network verifier ran into an error: %s", err.Error())
		notes.AppendWarning("NetworkVerifier failed to run:\n\t %s", err.Error())

OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE #404

Are you sure you want to change the base?

OSD-18645 - Initial implementation for CannotRetrieveUpdatesSRE #404

Conversation

anispate commented Apr 4, 2025 • edited Loading

openshift-ci bot commented Apr 4, 2025

codecov-commenter commented Apr 7, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnierman May 12, 2025 • edited Loading

Choose a reason for hiding this comment

tnierman May 12, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented May 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeid May 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typeid May 13, 2025 • edited Loading

Choose a reason for hiding this comment

anispate commented Apr 4, 2025 •

edited

Loading

codecov-commenter commented Apr 7, 2025 •

edited

Loading

tnierman May 12, 2025 •

edited

Loading

tnierman May 12, 2025 •

edited

Loading

typeid May 14, 2025 •

edited

Loading

typeid May 13, 2025 •

edited

Loading