Introduce common cephError type to compare errors between packages

[nixpanic]

A new internal/errutil.cephError can be used as a common type to replace all error types that are specific to the different packages (cephfsError, radosError, rbdError). By using the new cephError type everywhere, it becomes possible to match rados.ErrNotFound and rbd.ErrNotFound with the standard errors.Is() function.

Some go-ceph functions from the cephfs and rbd packages can return errors generated by the lower-level rados package. In such a case, a caller may need to check for e.g. rados.ErrNotFound as well as rbd.ErrNotFound, depending on the function that was called. This is prone to errors, as users of go-ceph may not inspect what the go-ceph function does and which error type it could return.

Closes: #1031

Checklist

• Added tests for features and functional changes
• Public functions and types are documented
• Standard formatting is applied to Go code
• Is this a new API? Added a new file that begins with //go:build ceph_preview
• Ran make api-update to record new APIs

New or infrequent contributors may want to review the go-ceph Developer's Guide including the section on how we track API Status and the API Stability Plan.

The go-ceph project uses mergify. View the mergify command guide for information on how to interact with mergify. Add a comment with @Mergifyio rebase to rebase your PR when github indicates that the PR is out of date with the base branch.

[anoopcs9]

Generally looks good to me.

Shouldn't we consider updating the API docs about those older implementations getting removed?

[anoopcs9]

We could also either:

• rename and move internal/errors_test.go to internal/errutil/error_test.go
  OR
• rename internal/errutil/error.go to internal/errutil/errors.go and move internal/errutil/errors_test.go to internal/errutil/errors_test.go

[nixpanic]

@anoopcs9 the file naming an location is actually intentional, because of two reasons:

1. the tests do not really test the errutil package but users of the package,
2. importing the rados package in the errutil package that gets imported by rados causes recursive imports (although that may be allowed for tests?)

If you insist on renaming/moving, I'll do that. Please state your preference in that case. Thanks!

[nixpanic]

Shouldn't we consider updating the API docs about those older implementations getting removed?

The implementation is an internal-only change. I do not expect any visible changes on the outside, all should be backwards compatible.

[anoopcs9]

@anoopcs9 the file naming an location is actually intentional, because of two reasons:

1. the tests do not really test the errutil package but users of the package,
2. importing the rados package in the errutil package that gets imported by rados causes recursive imports (although that may be allowed for tests?)

In that case how about we rewrite errors_test.go in the direction of strerror_test.go and put in under internal/errutil?

Shouldn't we consider updating the API docs about those older implementations getting removed?

The implementation is an internal-only change. I do not expect any visible changes on the outside, all should be backwards compatible.

I agree but those APIs would still remain listed in docs/api-status.json.

[nixpanic]

I agree but those APIs would still remain listed in docs/api-status.json.

oh, right, I see rbdError and so on there 🤔

[nixpanic]

Hey @anoopcs9,

* rename and move _internal/errors_test.go_ to _internal/errutil/error_test.go_

Just to confirm, also tests may not do the 2-way imports:

# github.com/ceph/go-ceph/internal/errutil
package github.com/ceph/go-ceph/internal/errutil
	imports github.com/ceph/go-ceph/cephfs
	imports github.com/ceph/go-ceph/internal/errutil: import cycle not allowed in test
*** ERROR: returned 1
*** ERROR: internal/errutil tests failed

In that case how about we rewrite errors_test.go in the direction of strerror_test.go and put in under internal/errutil?

The main thing it tests is comparing errors between the rados, rbd and cephfs packages. Such a test needs to live outside of the internal/errutil package to prevent the import cycle.

Do you have an other idea where to place errors_test.go?

I agree but those APIs would still remain listed in docs/api-status.json.

The references to internal error types have been removed now.

[anoopcs9]

Do you have an other idea where to place errors_test.go?

May be we choose option 1 to rename and move internal/errors_test.go to internal/errutil/error_test.go as follows:

package errutil

import (
	"errors"
	"fmt"
	"testing"

	"github.com/stretchr/testify/assert"
)

func TestCephError(t *testing.T) {
	errstr := cephErrno(107).Error()
	assert.Equal(t, "Transport endpoint is not connected", errstr)

	cephFSErr := GetError("cephfs", 2)
	assert.Equal(t, "cephfs: ret=2, No such file or directory",
		cephFSErr.Error())

	rbdErr := fmt.Errorf("RBD image not found: %w",
			GetError("rbd", 2))
	assert.True(t, errors.Is(cephFSErr, errors.Unwrap(rbdErr)))
}

I agree but those APIs would still remain listed in docs/api-status.json.

The references to internal error types have been removed now.

Are you sure you pushed those changes? I don't see any update here.

[phlogistonjohn]

Gentle reminder that the next release is on 2024-10-15. @anoopcs9 and @ansiwen please provide any feedback & @nixpanic check existing feedback please. If it misses the train, it misses the train (there will be another coming along later).

Updated with required changes.

[nixpanic]

Great, looks good to me 👍

Thanks for the changes!

[anoopcs9]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased

[anoopcs9]

lgtm, thanks.

[anoopcs9]

I agree but those APIs would still remain listed in docs/api-status.json.

The references to internal error types have been removed now.

Are you sure you pushed those changes? I don't see any update here.

Oh..I totally forgot this afterwards. Do you want to take this final step?

Pull request has been modified.

[anoopcs9]

@Mergifyio rebase

[mergify]

rebase

✅ Branch has been successfully rebased