Improve error handling #916

mrkisaolamb · 2025-01-10T11:18:37Z

To follow best practices for error and exception handling in Go and to prevent regressions,
err113 and gosec checks in golang-ci-linter are now enabled.
Errors now use API errors from k8s.io/apimachinery and predefined messages for status updates.

Resolve: https://issues.redhat.com/browse/OSPRH-9046

openshift-ci · 2025-01-10T11:18:42Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci · 2025-01-10T11:18:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrkisaolamb

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~~ [mrkisaolamb]

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

softwarefactory-project-zuul · 2025-01-16T12:28:47Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/3d86a4af47ab4c45957184d8e9c14dde

✔️ openstack-meta-content-provider SUCCESS in 2h 40m 52s
❌ nova-operator-kuttl RETRY_LIMIT in 30m 20s
✔️ nova-operator-tempest-multinode SUCCESS in 2h 05m 24s
✔️ nova-operator-tempest-multinode-ceph SUCCESS in 2h 23m 32s

gibizer · 2025-01-20T11:47:04Z

controllers/novaapi_controller.go

 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,


This is used in common.go ensureNetworkAttachments() as well, but you did not changed it there. Is it because there the message using an error coming from a call instead of defined locally?

yes exactly that was the reasone

gibizer · 2025-01-20T11:47:49Z

api/v1beta1/conditions.go

@@ -56,6 +56,9 @@ const (

 // Common Messages used by API objects.
 const (
+	//NovaNetworkAttachmentsReadyInitMessage
+	NovaNetworkAttachmentsReadyErrorMessage = "NetworkAttachments error occurred not all pods have interfaces with ips as configured in NetworkAttachments: %s"
+


Will this eventually move to lib-common to be used across the operators? E.g. in https://github.com/openstack-k8s-operators/placement-operator/blob/5c39b092c37feddb1d94539b759ee8cebab371e2/controllers/placementapi_controller.go#L1159-L1160

yes eventually we can move to lib-common all common errors

gibizer · 2025-01-20T11:50:00Z

controllers/common.go

@@ -225,7 +223,10 @@ func ensureSecret(
 	for _, field := range expectedFields {
 		val, ok := secret.Data[field]
 		if !ok {
-			err := fmt.Errorf("field '%s' not found in secret/%s", field, secretName.Name)
+			// Usage in the code


I don't get this comment

gibizer · 2025-01-20T11:50:26Z

controllers/common.go

+			// ensureSecret - ensures that the Secret object exists and the expected fields
+			// are in the Secret. It returns a hash of the values of the expected fields.


This belongs to the function definition.

gibizer · 2025-01-20T11:51:30Z

controllers/common.go

@@ -579,7 +580,7 @@ func getNovaClient(
 			return nil, err
 		}
 		if (ctrlResult != ctrl.Result{}) {
-			return nil, fmt.Errorf("the CABundleSecret %s not found", auth.GetCABundleSecretName())
+			return nil, fmt.Errorf("the CABundleSecret %s not found: %w", auth.GetCABundleSecretName(), err)


as far as I see err will be always nil here as if not then the previous condition is true and the function returns.

Ou true so I create originall error that we lost during geting secret

gibizer · 2025-01-20T11:55:37Z

controllers/common.go

@@ -225,7 +223,10 @@ func ensureSecret(
 	for _, field := range expectedFields {
 		val, ok := secret.Data[field]
 		if !ok {
-			err := fmt.Errorf("field '%s' not found in secret/%s", field, secretName.Name)
+			// Usage in the code
+			err := k8s_errors.NewBadRequest(fmt.Sprintf("field '%s' not found in secret/%s", field, secretName.Name))


Do we call ensureSecret from the validation webhook? As far as I remember we don't. So it is a bit strange to me to use HTTP response codes as errors here. I.e. the NewBadRequest creates a StatusError where the description says:

StatusError is an error intended for consumption by a REST API server; it can also be reconstructed by clients from a REST response. Public to allow easy type switches.

https://pkg.go.dev/k8s.io/apimachinery/pkg/api/errors#StatusError

I used error that I already created ErrFieldNotFound

gibizer · 2025-01-20T11:56:31Z

controllers/common.go

@@ -641,7 +642,7 @@ func ensureMemcached(
 				condition.RequestedReason,
 				condition.SeverityInfo,
 				condition.MemcachedReadyWaitingMessage))
-			return nil, fmt.Errorf("memcached %s not found", memcachedName)
+			return nil, fmt.Errorf("%w: memcached %s not found", err, memcachedName)


yepp this is a good improvement. Thanks.

gibizer · 2025-01-20T11:58:04Z

controllers/nova_controller.go

@@ -1631,8 +1631,8 @@ func (r *NovaReconciler) ensureMQ(

 	url, ok := secret.Data[TransportURLSelector]
 	if !ok {
-		return "", nova.MQFailed, fmt.Errorf(
-			"the TransportURL secret %s does not have 'transport_url' field", transportURL.Status.SecretName)
+		return "", nova.MQFailed, k8s_errors.NewBadRequest(fmt.Sprintf(


ditto REST errors feels strange to me inside the reconciler loop

mrkisaolamb

Thanks for review!

mrkisaolamb · 2025-02-12T15:43:45Z

api/v1beta1/conditions.go

@@ -56,6 +56,9 @@ const (

 // Common Messages used by API objects.
 const (
+	//NovaNetworkAttachmentsReadyInitMessage
+	NovaNetworkAttachmentsReadyErrorMessage = "NetworkAttachments error occurred not all pods have interfaces with ips as configured in NetworkAttachments: %s"
+


yes eventually we can move to lib-common all common errors

mrkisaolamb · 2025-02-12T15:43:54Z

controllers/common.go

@@ -225,7 +223,10 @@ func ensureSecret(
 	for _, field := range expectedFields {
 		val, ok := secret.Data[field]
 		if !ok {
-			err := fmt.Errorf("field '%s' not found in secret/%s", field, secretName.Name)
+			// Usage in the code


mrkisaolamb · 2025-02-12T15:44:33Z

controllers/common.go

@@ -225,7 +223,10 @@ func ensureSecret(
 	for _, field := range expectedFields {
 		val, ok := secret.Data[field]
 		if !ok {
-			err := fmt.Errorf("field '%s' not found in secret/%s", field, secretName.Name)
+			// Usage in the code
+			err := k8s_errors.NewBadRequest(fmt.Sprintf("field '%s' not found in secret/%s", field, secretName.Name))


I used error that I already created ErrFieldNotFound

mrkisaolamb · 2025-02-12T15:45:35Z

controllers/common.go

+			// ensureSecret - ensures that the Secret object exists and the expected fields
+			// are in the Secret. It returns a hash of the values of the expected fields.


mrkisaolamb · 2025-02-12T16:02:11Z

controllers/common.go

@@ -579,7 +580,7 @@ func getNovaClient(
 			return nil, err
 		}
 		if (ctrlResult != ctrl.Result{}) {
-			return nil, fmt.Errorf("the CABundleSecret %s not found", auth.GetCABundleSecretName())
+			return nil, fmt.Errorf("the CABundleSecret %s not found: %w", auth.GetCABundleSecretName(), err)


Ou true so I create originall error that we lost during geting secret

mrkisaolamb · 2025-02-12T16:03:25Z

controllers/nova_controller.go

@@ -1631,8 +1631,8 @@ func (r *NovaReconciler) ensureMQ(

 	url, ok := secret.Data[TransportURLSelector]
 	if !ok {
-		return "", nova.MQFailed, fmt.Errorf(
-			"the TransportURL secret %s does not have 'transport_url' field", transportURL.Status.SecretName)
+		return "", nova.MQFailed, k8s_errors.NewBadRequest(fmt.Sprintf(


mrkisaolamb · 2025-02-12T16:04:13Z

controllers/novaapi_controller.go

 		instance.Status.Conditions.Set(condition.FalseCondition(
 			condition.NetworkAttachmentsReadyCondition,
 			condition.ErrorReason,
 			condition.SeverityWarning,
-			condition.NetworkAttachmentsReadyErrorMessage,


yes exactly that was the reasone

To follow best practices for error and exception handling in Go and to prevent regressions, err113 and gosec checks in golang-ci-linter are now enabled. Errors now use API errors from k8s.io/apimachinery and predefined messages for status updates.

softwarefactory-project-zuul · 2025-02-12T23:45:39Z

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/d46f9fd903354941affead375c45cedb

✔️ openstack-meta-content-provider SUCCESS in 5h 42m 59s
❌ nova-operator-kuttl FAILURE in 34m 33s
✔️ nova-operator-tempest-multinode SUCCESS in 2h 16m 22s
✔️ nova-operator-tempest-multinode-ceph SUCCESS in 2h 54m 10s

openshift-ci bot added the do-not-merge/work-in-progress label Jan 10, 2025

openshift-ci bot added the approved label Jan 10, 2025

mrkisaolamb force-pushed the error-fix branch from 9fb1cee to 05ccd22 Compare January 10, 2025 13:15

mrkisaolamb changed the title ~~WIP error handling~~ Improve error handling Jan 10, 2025

mrkisaolamb force-pushed the error-fix branch from 05ccd22 to eeae165 Compare January 10, 2025 13:20

mrkisaolamb marked this pull request as ready for review January 10, 2025 13:20

openshift-ci bot removed the do-not-merge/work-in-progress label Jan 10, 2025

openshift-ci bot requested review from frenzyfriday and olliewalsh January 10, 2025 13:20

mrkisaolamb requested review from gibizer and stuggi January 13, 2025 10:39

mrkisaolamb force-pushed the error-fix branch from eeae165 to d8398dc Compare January 16, 2025 09:46

gibizer reviewed Jan 20, 2025

View reviewed changes

This was referenced Feb 12, 2025

Improve error handling and gosec measures openstack-k8s-operators/barbican-operator#202

Open

Improve error handling and gosec measures openstack-k8s-operators/keystone-operator#530

Draft

mrkisaolamb force-pushed the error-fix branch from d8398dc to f7d3ecc Compare February 12, 2025 16:32

mrkisaolamb commented Feb 12, 2025

View reviewed changes

Improve error handling

10f87cd

To follow best practices for error and exception handling in Go and to prevent regressions, err113 and gosec checks in golang-ci-linter are now enabled. Errors now use API errors from k8s.io/apimachinery and predefined messages for status updates.

mrkisaolamb force-pushed the error-fix branch from f7d3ecc to 10f87cd Compare February 12, 2025 16:34

mrkisaolamb requested a review from gibizer February 12, 2025 17:05

mrkisaolamb requested a review from auniyal61 February 13, 2025 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error handling #916

Improve error handling #916

mrkisaolamb commented Jan 10, 2025 •

edited

Loading

openshift-ci bot commented Jan 10, 2025

openshift-ci bot commented Jan 10, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

gibizer Jan 20, 2025

gibizer Jan 20, 2025

mrkisaolamb Feb 12, 2025

mrkisaolamb left a comment

mrkisaolamb Feb 12, 2025

mrkisaolamb Feb 12, 2025

mrkisaolamb Feb 12, 2025

mrkisaolamb Feb 12, 2025

mrkisaolamb Feb 12, 2025

mrkisaolamb Feb 12, 2025

mrkisaolamb Feb 12, 2025

softwarefactory-project-zuul bot commented Feb 12, 2025

		// ensureSecret - ensures that the Secret object exists and the expected fields
		// are in the Secret. It returns a hash of the values of the expected fields.

Improve error handling #916

Are you sure you want to change the base?

Improve error handling #916

Conversation

mrkisaolamb commented Jan 10, 2025 • edited Loading

openshift-ci bot commented Jan 10, 2025

openshift-ci bot commented Jan 10, 2025

softwarefactory-project-zuul bot commented Jan 16, 2025

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

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

mrkisaolamb left a 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

softwarefactory-project-zuul bot commented Feb 12, 2025

mrkisaolamb commented Jan 10, 2025 •

edited

Loading