MGMT-16157 Remove recover usage from DB transactions

[filanov]

The downside of recover is that it may hide bugs like accessing nil pointer and can create a situation where a function panic and instead of returning an error it will return nil, in some cases it cause a hot loop in our controllers, or just move a bug to an upper later.

To avoid those situations we decided to remove the recover function usage in the transaction code and move to a new transaction style that will make scope of the transaction and handling of errors clearer.

List all the issues related to this PR

• New Feature
• Enhancement
• Bug fix
• Tests
• Documentation
• CI/CD

What environments does this code impact?

• Automation (CI, tools, etc)
• Cloud
• Operator Managed Deployments
• None

How was this code tested?

• assisted-test-infra environment
• dev-scripts environment
• Reviewer's test appreciated
• Waiting for CI to do a full test run
• Manual (Elaborate on how it was tested)
• No tests needed

Checklist

• Title and description added to both, commit and PR.
• Relevant issues have been associated (see CONTRIBUTING guide)
• This change does not require a documentation update (docstring, docs, README, etc)
• Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

• Are the title and description (in both PR and commit) meaningful and clear?
• Is there a bug required (and linked) for this change?
• Should this PR be backported?

[filanov]

I didn't removed all the old transaction style code because this is not the purpose of this PR, i will do it in another ticket.

[filanov]

An example of a code that can create strange issues

package main

import (
	"fmt"
)

func Foo() (*int, error) {
	i := 5
	var b *int

	defer func() {
		if r := recover(); r != nil {
			fmt.Println("foo recover")
		}
	}()

	fmt.Println("create a panic", *b)
	return &i, nil
}

func main() {
	i, err := Foo()
	if err != nil {
		panic("had an error")
	}

	fmt.Println(*i)
}

[codecov]

Codecov Report

Merging #5795 (8aea285) into master (f4bbcf6) will increase coverage by 0.17%.
Report is 1 commits behind head on master.
The diff coverage is 67.09%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5795      +/-   ##
==========================================
+ Coverage   67.88%   68.06%   +0.17%     
==========================================
  Files         235      235              
  Lines       34552    34414     -138     
==========================================
- Hits        23456    23424      -32     
+ Misses       9009     8928      -81     
+ Partials     2087     2062      -25     

Files	Coverage Δ
internal/cluster/transition.go	73.86% <0.00%> (+1.63%)	⬆️
internal/operators/handler/handler.go	61.53% <0.00%> (+8.20%)	⬆️
internal/cluster/registrar.go	46.96% <43.75%> (-1.72%)	⬇️
internal/bminventory/inventory_v2_handlers.go	57.44% <45.00%> (+0.87%)	⬆️
internal/bminventory/inventory.go	71.52% <73.56%> (+1.08%)	⬆️

... and 1 file with indirect coverage changes

[filanov]

/retest

[carbonin]

Nice ratio

👍

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, filanov

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 [carbonin,filanov]

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

[openshift-ci]

@filanov: 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/test-infra repository. I understand the commands that are listed here.

[openshift-bot]

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-agent-installer-api-server-container-v4.16.0-202312121444.p0.g005ba71.assembly.stream for distgit ose-agent-installer-api-server.
All builds following this will include this PR.